Hi, Ilya Maximets.
Thank you for taking the time to review my patch, and I truly admire your
dedication to the OVS community.
If duplicate controller parameters are set, OVS should establish multiple
connections with the controller. If the controller does not perform relevant
verification, it may send flows to the same OVS. It may cause some
abnormalities.
Our operations colleagues configure OVS deployment through the command line.
The frequency is relatively low, but this situation has occurred several times.
All caused by misoperation
There are mainly the following reasons for implementing this function in the
ovs-vsctl module:
1.fix this on the OVSDB side is indeed more comprehensive; however,
fix this on the ovs-vsctl side eliminates the need for database
access, making it faster and more lightweight in terms of overhead.
2.For more manual operations, the command line is often chosen for
configuration, which leads to a higher probability of
misoperations. Direct access to the OVSDB database is generally
implemented through interface calls between functional modules, so the
probability of duplicate parameters should be relatively low.
I understand that this implementation has its advantages, and at the very
least, it has no negative effects. Additionally, sufficient testing has been
conducted.
Is it possible to merge this patch first, and I will figure out how to fix
this on the database side later?
Best regards, Lingfei Yao.
------------------ ???????? ------------------
??????:
"Ilya Maximets"
<i.maxim...@ovn.org>;
????????: 2025??8??27??(??????) ????4:18
??????: "??????????????"<543981...@qq.com>;"dev"<d...@openvswitch.org>;
????: "i.maximets"<i.maxim...@ovn.org>;
????: Re: [ovs-dev] [PATCH v5] ovs-vsctl: Check controller parameters.
On 8/19/25 9:41 AM, yaolingfei via dev wrote:
> Check the parameters when setting up the controller. The parameters will
> only take effect when none of the following three parameters are
> repeated; Otherwise, a prompt message will be returned
> 1.ip address
> 2.port no
> 3.protocol type
>
> v5: modified the subject summary
>
> Signed-off-by: yaolingfei <543981...@qq.com>
> ---
Hi, yaolingfei. Thanks for the patch.
Could you, please, explain why this problem needs to be fixed?
Is it a common issue that users duplicate controllers on the command line?
Sounds a little unlikely. If it is, maybe we need to fix this on the
database side, e.g. by making the controller table indexed by the target.
If we just check on the command line level, users can still put duplicates
into the database.
Best regards, Ilya Maximets.
> tests/ovs-vsctl.at | 4 ++++
> utilities/ovs-vsctl.c | 26 +++++++++++++++++++++++++-
> 2 files changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
> index 59245fff8..76fe74414 100644
> --- a/tests/ovs-vsctl.at
> +++ b/tests/ovs-vsctl.at
> @@ -492,6 +492,8 @@ AT_CHECK([RUN_OVS_VSCTL_TOGETHER(
> [get-controller br0],
>
> [--inactivity-probe=30000 set-controller br0
tcp:1.2.3.4],
> + [get-controller br0],
> + [set-controller br0 tcp:8.9.10.11 tcp:8.9.10.11],
> [get-controller br0])], [0], [
>
>
> @@ -501,6 +503,8 @@ tcp:4.5.6.7
>
> tcp:5.4.3.2\ntcp:8.9.10.11
>
> +tcp:1.2.3.4
> +Controller parameter is duplicated
> tcp:1.2.3.4
> ])
> OVS_VSCTL_CLEANUP
> diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
> index d90db934b..f39952d72 100644
> --- a/utilities/ovs-vsctl.c
> +++ b/utilities/ovs-vsctl.c
> @@ -2348,6 +2348,24 @@ insert_controllers(struct ctl_context *ctx, char
*targets[], size_t n)
> return controllers;
> }
>
> +static int
> +check_dup_controllers(char *targets[], size_t n)
> +{
> + size_t i;
> + size_t j;
> +
> + /* check if the input parameters are duplicated */
> + for (i = 0; i < n; i++) {
> + for (j = i + 1; j < n; j++) {
> + if
(!strcmp(targets[i], targets[j])) {
>
+
return 0;
> + }
> + }
> + }
> +
> + return 1;
> +}
> +
> static void
> cmd_set_controller(struct ctl_context *ctx)
> {
> @@ -2361,9 +2379,15 @@ cmd_set_controller(struct ctl_context *ctx)
> br = find_real_bridge(vsctl_ctx,
ctx->argv[1], true)->br_cfg;
> verify_controllers(br);
>
> + n = ctx->argc - 2;
> + if (!check_dup_controllers(&ctx->argv[2], n)) {
> + /* controller parameters are
duplicated */
> +
ds_put_format(&ctx->output, "Controller parameter is duplicated\n");
> + return;
> + }
> +
> delete_controllers(br->controller,
br->n_controller);
>
> - n = ctx->argc - 2;
> controllers = insert_controllers(ctx,
&ctx->argv[2], n);
> ovsrec_bridge_set_controller(br,
controllers, n);
> free(controllers);
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
