Hi Mark,
I will adjust my patch to proceed with your suggestions.
Thanks,
Lucas
Em ter., 22 de jul. de 2025 às 12:30, Mark Michelson <mmich...@redhat.com>
escreveu:
> Hi Lucas, thanks for the patch.
>
> I think searching by chassis name makes way more sense than searching by
> the default name generated by ovn-nbctl.
>
> In fact, I would go a step further than this patch and just remove the
> search by "{router_port_name}-{chassis_name}" entirely. Searching by
> chassis name on the logical router's existing gateway chassis is more
> accurate, and potentially more efficient (if the Gateway_Chassis table
> has a lot of rows).
>
> On 7/22/25 9:44 AM, Lucas Vargas Dias via dev wrote:
> > 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.
> >
> > Signed-off-by: Lucas Vargas Dias <lucas.vd...@luizalabs.com>
> > ---
> > tests/ovn-nbctl.at | 16 +++++++++++++++-
> > utilities/ovn-nbctl.c | 18 +++++++++++++++---
> > 2 files changed, 30 insertions(+), 4 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..34caf599b 100644
> > --- a/utilities/ovn-nbctl.c
> > +++ b/utilities/ovn-nbctl.c
> > @@ -5829,12 +5829,22 @@ 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_chassis_name_or_name_or_uuid(struct ctl_context *ctx, const char
> *id,
> > + 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;
> >
> > + for (size_t i = 0; i < lrp->n_gateway_chassis; i++) {
> > + if (!strcmp(lrp->gateway_chassis[i]->chassis_name,
> chassis_name)) {
> > + *gc_p = lrp->gateway_chassis[i];
> > + return NULL;
> > + }
> > + }
> > +
> > struct uuid gc_uuid;
> > bool is_uuid = uuid_from_string(&gc_uuid, id);
> > if (is_uuid) {
> > @@ -5867,6 +5877,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 +5908,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_chassis_name_or_name_or_uuid(ctx, gc_name,
> chassis_name,
> > + false, &gc, lrp);
> > if (error) {
> > ctx->error = error;
> > free(gc_name);
>
>
Em ter., 22 de jul. de 2025 às 12:30, Mark Michelson <mmich...@redhat.com>
escreveu:
> Hi Lucas, thanks for the patch.
>
> I think searching by chassis name makes way more sense than searching by
> the default name generated by ovn-nbctl.
>
> In fact, I would go a step further than this patch and just remove the
> search by "{router_port_name}-{chassis_name}" entirely. Searching by
> chassis name on the logical router's existing gateway chassis is more
> accurate, and potentially more efficient (if the Gateway_Chassis table
> has a lot of rows).
>
> On 7/22/25 9:44 AM, Lucas Vargas Dias via dev wrote:
> > 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.
> >
> > Signed-off-by: Lucas Vargas Dias <lucas.vd...@luizalabs.com>
> > ---
> > tests/ovn-nbctl.at | 16 +++++++++++++++-
> > utilities/ovn-nbctl.c | 18 +++++++++++++++---
> > 2 files changed, 30 insertions(+), 4 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..34caf599b 100644
> > --- a/utilities/ovn-nbctl.c
> > +++ b/utilities/ovn-nbctl.c
> > @@ -5829,12 +5829,22 @@ 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_chassis_name_or_name_or_uuid(struct ctl_context *ctx, const char
> *id,
> > + 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;
> >
> > + for (size_t i = 0; i < lrp->n_gateway_chassis; i++) {
> > + if (!strcmp(lrp->gateway_chassis[i]->chassis_name,
> chassis_name)) {
> > + *gc_p = lrp->gateway_chassis[i];
> > + return NULL;
> > + }
> > + }
> > +
> > struct uuid gc_uuid;
> > bool is_uuid = uuid_from_string(&gc_uuid, id);
> > if (is_uuid) {
> > @@ -5867,6 +5877,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 +5908,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_chassis_name_or_name_or_uuid(ctx, gc_name,
> chassis_name,
> > + false, &gc, lrp);
> > if (error) {
> > ctx->error = error;
> > free(gc_name);
>
>
--
_‘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
