diff mbox series

[ovs-dev,v3,02/33] northd: Fix relying on naming coincidences.

Message ID f216c08d4a8d92f3ee7a54b9ffe3facdff7065e5.1732630355.git.felix.huettner@stackit.cloud
State Changes Requested
Headers show
Series OVN Fabric integration | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/github-robot-_ovn-kubernetes success github build: passed

Commit Message

Felix Huettner Nov. 26, 2024, 2:37 p.m. UTC 
We previous relied on the fact the the southbound Port_Binding gets the
same name as the Northbound port. However in all of the cases we use
this we already know the name the southbound port will have. Therefor we
just use this name, instead of getting it from northbound.

This is a prerequisite for later patches that use derived router ports.

Acked-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
Signed-off-by: Felix Huettner <felix.huettner@stackit.cloud>
---
 northd/northd.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

Comments

Dumitru Ceara Dec. 2, 2024, 10:15 a.m. UTC | #1 
On 11/26/24 3:37 PM, Felix Huettner via dev wrote:
> We previous relied on the fact the the southbound Port_Binding gets the
> same name as the Northbound port. However in all of the cases we use
> this we already know the name the southbound port will have. Therefor we

Nit: therefore

> just use this name, instead of getting it from northbound.
> 
> This is a prerequisite for later patches that use derived router ports.
> 
> Acked-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> Signed-off-by: Felix Huettner <felix.huettner@stackit.cloud>
> ---
>  northd/northd.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/northd/northd.c b/northd/northd.c
> index 32a7c8509..fa3a8a882 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -3305,11 +3305,18 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn,
>              if (router_port || chassis || is_cr_port(op)) {
>                  struct smap new;
>                  smap_init(&new);
> -

Nit: unrelated.

>                  if (is_cr_port(op)) {
>                      smap_add(&new, "distributed-port", op->nbsp->name);

Shouldn't this be op->primary_port->key too?

>                  } else if (router_port) {
> -                    smap_add(&new, "peer", router_port);
> +                    /* op->peer can be null if the peer is disabed. In this
> +                     * case we fall back to the router_port string which
> +                     * might be wrong, but since the port does not exist that
> +                     * does not matter. */
> +                    if (op->peer) {
> +                        smap_add(&new, "peer", op->peer->key);
> +                    } else {
> +                        smap_add(&new, "peer", router_port);
> +                    }
>                  }
>                  if (chassis) {
>                      smap_add(&new, "l3gateway-chassis", chassis);
> @@ -4025,7 +4032,7 @@ sync_pb_for_lrp(struct ovn_port *op,
>              lr_stateful_table_find_by_index(lr_stateful_table, op->od->index);
>          ovs_assert(lr_stateful_rec);
>  
> -        smap_add(&new, "distributed-port", op->nbrp->name);
> +        smap_add(&new, "distributed-port", op->primary_port->key);
>  
>          bool always_redirect =
>              !lr_stateful_rec->lrnat_rec->has_distributed_nat &&
> @@ -4050,10 +4057,7 @@ sync_pb_for_lrp(struct ovn_port *op,
>              smap_add(&new, "peer", op->peer->key);
>              if (op->nbrp->ha_chassis_group ||
>                  op->nbrp->n_gateway_chassis) {
> -                char *redirect_name =
> -                    ovn_chassis_redirect_name(op->nbrp->name);
> -                smap_add(&new, "chassis-redirect-port", redirect_name);
> -                free(redirect_name);
> +                smap_add(&new, "chassis-redirect-port", op->cr_port->key);
>              }
>          }
>          if (chassis_name) {

Thanks,
Dumitru
Felix Huettner Dec. 5, 2024, 2:33 p.m. UTC | #2 
On Mon, Dec 02, 2024 at 11:15:08AM +0100, Dumitru Ceara wrote:
> On 11/26/24 3:37 PM, Felix Huettner via dev wrote:
> > We previous relied on the fact the the southbound Port_Binding gets the
> > same name as the Northbound port. However in all of the cases we use
> > this we already know the name the southbound port will have. Therefor we
> 
> Nit: therefore
> 
> > just use this name, instead of getting it from northbound.
> > 
> > This is a prerequisite for later patches that use derived router ports.
> > 
> > Acked-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> > Signed-off-by: Felix Huettner <felix.huettner@stackit.cloud>
> > ---
> >  northd/northd.c | 18 +++++++++++-------
> >  1 file changed, 11 insertions(+), 7 deletions(-)
> > 
> > diff --git a/northd/northd.c b/northd/northd.c
> > index 32a7c8509..fa3a8a882 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -3305,11 +3305,18 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn,
> >              if (router_port || chassis || is_cr_port(op)) {
> >                  struct smap new;
> >                  smap_init(&new);
> > -
> 
> Nit: unrelated.
> 
> >                  if (is_cr_port(op)) {
> >                      smap_add(&new, "distributed-port", op->nbsp->name);
> 
> Shouldn't this be op->primary_port->key too?

Yes, thanks.

> 
> >                  } else if (router_port) {
> > -                    smap_add(&new, "peer", router_port);
> > +                    /* op->peer can be null if the peer is disabed. In this
> > +                     * case we fall back to the router_port string which
> > +                     * might be wrong, but since the port does not exist that
> > +                     * does not matter. */
> > +                    if (op->peer) {
> > +                        smap_add(&new, "peer", op->peer->key);
> > +                    } else {
> > +                        smap_add(&new, "peer", router_port);
> > +                    }
> >                  }
> >                  if (chassis) {
> >                      smap_add(&new, "l3gateway-chassis", chassis);
> > @@ -4025,7 +4032,7 @@ sync_pb_for_lrp(struct ovn_port *op,
> >              lr_stateful_table_find_by_index(lr_stateful_table, op->od->index);
> >          ovs_assert(lr_stateful_rec);
> >  
> > -        smap_add(&new, "distributed-port", op->nbrp->name);
> > +        smap_add(&new, "distributed-port", op->primary_port->key);
> >  
> >          bool always_redirect =
> >              !lr_stateful_rec->lrnat_rec->has_distributed_nat &&
> > @@ -4050,10 +4057,7 @@ sync_pb_for_lrp(struct ovn_port *op,
> >              smap_add(&new, "peer", op->peer->key);
> >              if (op->nbrp->ha_chassis_group ||
> >                  op->nbrp->n_gateway_chassis) {
> > -                char *redirect_name =
> > -                    ovn_chassis_redirect_name(op->nbrp->name);
> > -                smap_add(&new, "chassis-redirect-port", redirect_name);
> > -                free(redirect_name);
> > +                smap_add(&new, "chassis-redirect-port", op->cr_port->key);
> >              }
> >          }
> >          if (chassis_name) {
> 
> Thanks,
> Dumitru
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff mbox series

Patch

diff --git a/northd/northd.c b/northd/northd.c
index 32a7c8509..fa3a8a882 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -3305,11 +3305,18 @@  ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn,
             if (router_port || chassis || is_cr_port(op)) {
                 struct smap new;
                 smap_init(&new);
-
                 if (is_cr_port(op)) {
                     smap_add(&new, "distributed-port", op->nbsp->name);
                 } else if (router_port) {
-                    smap_add(&new, "peer", router_port);
+                    /* op->peer can be null if the peer is disabed. In this
+                     * case we fall back to the router_port string which
+                     * might be wrong, but since the port does not exist that
+                     * does not matter. */
+                    if (op->peer) {
+                        smap_add(&new, "peer", op->peer->key);
+                    } else {
+                        smap_add(&new, "peer", router_port);
+                    }
                 }
                 if (chassis) {
                     smap_add(&new, "l3gateway-chassis", chassis);
@@ -4025,7 +4032,7 @@  sync_pb_for_lrp(struct ovn_port *op,
             lr_stateful_table_find_by_index(lr_stateful_table, op->od->index);
         ovs_assert(lr_stateful_rec);
 
-        smap_add(&new, "distributed-port", op->nbrp->name);
+        smap_add(&new, "distributed-port", op->primary_port->key);
 
         bool always_redirect =
             !lr_stateful_rec->lrnat_rec->has_distributed_nat &&
@@ -4050,10 +4057,7 @@  sync_pb_for_lrp(struct ovn_port *op,
             smap_add(&new, "peer", op->peer->key);
             if (op->nbrp->ha_chassis_group ||
                 op->nbrp->n_gateway_chassis) {
-                char *redirect_name =
-                    ovn_chassis_redirect_name(op->nbrp->name);
-                smap_add(&new, "chassis-redirect-port", redirect_name);
-                free(redirect_name);
+                smap_add(&new, "chassis-redirect-port", op->cr_port->key);
             }
         }
         if (chassis_name) {