On Tue, Jul 15, 2025 at 3:59 PM Lucas Vargas Dias via dev <
ovs-dev@openvswitch.org> wrote:
Hi Lucas,
thank you for the patch. I have some comments down below.
If a CMS configure a gateway chassis with string diffent of format
> "%s-%s" lrp-chassis and user wants to change priority of chassis
> using ovn-nbctl, it creates duplicate entries:
> lrp0_chassis1 1 --> inserted by cms
> lrp0-chassis1 2 --> change priority with ovn-nbctl
> lrp-set-gateway-chassis
>
> This duplicate entries generate loop of messages in ovn-controller due to
> recalculation of priorities.
>
> This commit fix it changing the search by gateway chassis existent.
> Fix compares first the chassis name linked to LRP, second by uuid, and
> last,
> by lrp-chassis name.
>
> Signed-off-by: Lucas Vargas Dias <lucas.vd...@luizalabs.com>
> ---
>
I would slightly reword the commit message:
Configuration of Gateway Chassis with different name than the default
format of "$lrp-$chassis" could lead to duplicates when priority was
changed for the GW Chassis with different name e.g.:
lrp0_chassis1 1 --> inserted by cms
lrp0-chassis1 2 --> change priority with ovn-nbctl
lrp-set-gateway-chassis
ovn-controller would then create a log message every time it tried
recalculate priorities.
Search the GW Chassis linked to specified LRP by chassis_name first
before resorting to uuid or name search.
> tests/ovn-nbctl.at | 16 +++++++++++++++-
> utilities/ovn-nbctl.c | 24 ++++++++++++++++++++----
> 2 files changed, 35 insertions(+), 5 deletions(-)
>
> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> index bdeb3aeee..fe4a984d0 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -1958,7 +1958,21 @@ AT_CHECK([ovn-nbctl lrp-get-gateway-chassis lrp0],
> [0], [dnl
> lrp0-chassis2 10
> lrp0-chassis3 5
> lrp0-chassis1 1
> -])])
> +])
> +gc_uuid=$(ovn-nbctl --bare --colum _uuid find gateway_chassis
> name='lrp0-chassis1')
> +AT_CHECK([ovn-nbctl set gateway_chassis $gc_uuid name='lrp0_chassis1'])
> +AT_CHECK([ovn-nbctl lrp-get-gateway-chassis lrp0], [0], [dnl
> +lrp0-chassis2 10
> +lrp0-chassis3 5
> +lrp0_chassis1 1
> +])
> +AT_CHECK([ovn-nbctl lrp-set-gateway-chassis lrp0 chassis1 2])
> +AT_CHECK([ovn-nbctl lrp-get-gateway-chassis lrp0], [0], [dnl
> +lrp0-chassis2 10
> +lrp0-chassis3 5
> +lrp0_chassis1 2
> +])
> +])
>
> dnl ---------------------------------------------------------------------
>
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index 56a513217..06839176a 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -5829,15 +5829,29 @@ lr_get_name(const struct nbrec_logical_router *lr,
> char uuid_s[UUID_LEN + 1],
> }
>
> static char * OVS_WARN_UNUSED_RESULT
> -gc_by_name_or_uuid(struct ctl_context *ctx, const char *id, bool
> must_exist,
> - const struct nbrec_gateway_chassis **gc_p)
> +gc_by_name_chassis_name_or_uuid(struct ctl_context *ctx, const char *id,
>
nit: 'gc_by_chassis_name_or_name_or_uuid'
> + const char *chassis_name, bool must_exist,
> + const struct nbrec_gateway_chassis **gc_p,
> + const struct nbrec_logical_router_port
> *lrp)
> {
> const struct nbrec_gateway_chassis *gc = NULL;
> *gc_p = NULL;
>
> + if (lrp && chassis_name) {
>
lrp and chassis_name are required so they will always be non-null.
> + for (size_t i = 0; i < lrp->n_gateway_chassis; i++) {
> + if (strlen(lrp->gateway_chassis[i]->chassis_name) ==
> + strlen(chassis_name) &&
>
The strlen check is not needed.
> + !strcmp(lrp->gateway_chassis[i]->chassis_name,
> + chassis_name)) {
> + gc = lrp->gateway_chassis[i];
> + break;
>
You can assign the 'gc_p' and 'return NULL' here.
> + }
> + }
> + }
> +
> struct uuid gc_uuid;
> bool is_uuid = uuid_from_string(&gc_uuid, id);
> - if (is_uuid) {
> + if (is_uuid && !gc) {
> gc = nbrec_gateway_chassis_get_for_uuid(ctx->idl, &gc_uuid);
> }
>
> @@ -5867,6 +5881,7 @@ nbctl_pre_lrp_set_gateway_chassis(struct ctl_context
> *ctx)
>
> ovsdb_idl_add_column(ctx->idl, &nbrec_gateway_chassis_col_name);
> ovsdb_idl_add_column(ctx->idl, &nbrec_gateway_chassis_col_priority);
> + ovsdb_idl_add_column(ctx->idl,
> &nbrec_gateway_chassis_col_chassis_name);
> }
>
> static void
> @@ -5897,7 +5912,8 @@ nbctl_lrp_set_gateway_chassis(struct ctl_context
> *ctx)
>
> gc_name = xasprintf("%s-%s", lrp_name, chassis_name);
> const struct nbrec_gateway_chassis *gc;
> - error = gc_by_name_or_uuid(ctx, gc_name, false, &gc);
> + error = gc_by_name_chassis_name_or_uuid(ctx, gc_name, chassis_name,
> false,
> + &gc, lrp);
> if (error) {
> ctx->error = error;
> free(gc_name);
> --
> 2.34.1
>
>
> --
>
>
>
>
> _'Esta mensagem é direcionada apenas para os endereços constantes no
> cabeçalho inicial. Se você não está listado nos endereços constantes no
> cabeçalho, pedimos-lhe que desconsidere completamente o conteúdo dessa
> mensagem e cuja cópia, encaminhamento e/ou execução das ações citadas
> estão
> imediatamente anuladas e proibidas'._
>
>
> * **'Apesar do Magazine Luiza tomar
> todas as precauções razoáveis para assegurar que nenhum vírus esteja
> presente nesse e-mail, a empresa não poderá aceitar a responsabilidade por
> quaisquer perdas ou danos causados por esse e-mail ou por seus anexos'.*
>
>
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Thanks,
Ales
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
