[ovs-dev,v2] Expose distributed gateway port information in NB DB

From: Lucas Alvares Gomes <lucasagomes@gmail.com>

In order for the CMS to know which Chassis a distributed gateway port
is bond to, this patch updates the ovn-northd daemon to populate the
Logical_Router_Port table with that information.

To avoid changing the database schema, ovn-northd is setting a new key
called "hosting-chassis" in the options column from the LRP table. This
key value points to the name of the Chassis that is currently hosting
the distributed port.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2107515
Signed-off-by: Lucas Alvares Gomes <lucasagomes@gmail.com>
---

v1 -> v2
* Rebased on the main branch
* Updated the ovnsb_db_run() and handle_port_binding_changes() functions
  to include the LR ports as a parameter

 northd/en-sync-from-sb.c |  2 +-
 northd/northd.c          | 34 ++++++++++++++++++++++++++++++++--
 northd/northd.h          |  3 ++-
 ovn-nb.xml               | 15 +++++++++++++++
 tests/ovn-northd.at      | 34 ++++++++++++++++++++++++++++++++++
 5 files changed, 84 insertions(+), 4 deletions(-)

--
2.41.0

On 7/5/23 10:36, lmartins@redhat.com wrote:
> From: Lucas Alvares Gomes <lucasagomes@gmail.com>
> 
> In order for the CMS to know which Chassis a distributed gateway port
> is bond to, this patch updates the ovn-northd daemon to populate the
> Logical_Router_Port table with that information.
> 
> To avoid changing the database schema, ovn-northd is setting a new key
> called "hosting-chassis" in the options column from the LRP table. This
> key value points to the name of the Chassis that is currently hosting
> the distributed port.
> 
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2107515
> Signed-off-by: Lucas Alvares Gomes <lucasagomes@gmail.com>
> ---
> 

Hi Lucas!

Thanks for the patch!  And sorry for taking long to review.

> v1 -> v2
> * Rebased on the main branch
> * Updated the ovnsb_db_run() and handle_port_binding_changes() functions
>   to include the LR ports as a parameter
> 
>  northd/en-sync-from-sb.c |  2 +-
>  northd/northd.c          | 34 ++++++++++++++++++++++++++++++++--
>  northd/northd.h          |  3 ++-
>  ovn-nb.xml               | 15 +++++++++++++++
>  tests/ovn-northd.at      | 34 ++++++++++++++++++++++++++++++++++
>  5 files changed, 84 insertions(+), 4 deletions(-)
> 
> diff --git a/northd/en-sync-from-sb.c b/northd/en-sync-from-sb.c
> index 55ece2d16..4109aebe4 100644
> --- a/northd/en-sync-from-sb.c
> +++ b/northd/en-sync-from-sb.c
> @@ -60,7 +60,7 @@ en_sync_from_sb_run(struct engine_node *node, void *data OVS_UNUSED)
>      stopwatch_start(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec());
>      ovnsb_db_run(eng_ctx->ovnnb_idl_txn, eng_ctx->ovnsb_idl_txn,
>                   sb_pb_table, sb_ha_ch_grp_table, sb_ha_ch_grp_by_name,
> -                 &nd->ls_ports);
> +                 &nd->ls_ports, &nd->lr_ports);
>      stopwatch_stop(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec());
>  }
> 
> diff --git a/northd/northd.c b/northd/northd.c
> index 309e0abd0..58da9c086 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -17631,6 +17631,7 @@ handle_port_binding_changes(struct ovsdb_idl_txn *ovnsb_txn,
>                  const struct sbrec_ha_chassis_group_table *sb_ha_ch_grp_table,
>                  struct ovsdb_idl_index *sb_ha_ch_grp_by_name,
>                  struct hmap *ls_ports,
> +                struct hmap *lr_ports,
>                  struct shash *ha_ref_chassis_map)
>  {
>      const struct sbrec_port_binding *sb;
> @@ -17649,6 +17650,34 @@ handle_port_binding_changes(struct ovsdb_idl_txn *ovnsb_txn,
>      }
> 
>      SBREC_PORT_BINDING_TABLE_FOR_EACH (sb, sb_pb_table) {
> +
> +        /* Look for a chassisredirect binding and set the "active-chassis"
> +         * option in the NBDB logical_router_port table indicating on which
> +         * chassis the distributed port is bond to. */
> +        if (!strcmp(sb->type, "chassisredirect")) {
> +             const char *dist_port =
> +                 smap_get(&sb->options, "distributed-port");

Isn't it easier to just use the op->l3dgw_port instead of looking it up
again here?  We lookup 'op' just below in the port binding loop.

> +             if (dist_port) {
> +                 struct ovn_port *router_port =
> +                     ovn_port_find(lr_ports, dist_port);
> +                 if (router_port) {
> +                     struct smap options;
> +                     smap_clone(&options, &router_port->nbrp->options);
> +                     if (sb->chassis) {
> +                         smap_replace(&options, "hosting-chassis",
> +                                      sb->chassis->name);
> +                     } else {
> +                         smap_remove(&options, "hosting-chassis");
> +                     }
> +                     nbrec_logical_router_port_set_options(router_port->nbrp,
> +                                                           &options);

We leak 'options' here.  Also reported in CI:

https://github.com/ovsrobot/ovn/actions/runs/5462364730

You might want to add a 'smap_destroy(&options)' here.

Can we move this "update the options" logic into a separate
chassis-redirect specific function?

> +                  }
> +             }
> +             /* Continue since there are no matching logical port for
> +              * chassisredirect bindings */
> +             continue;
> +        }
> +
>          struct ovn_port *op = ovn_port_find(ls_ports, sb->logical_port);
> 
>          if (!op || !op->nbsp) {
> @@ -17697,7 +17726,8 @@ ovnsb_db_run(struct ovsdb_idl_txn *ovnnb_txn,
>               const struct sbrec_port_binding_table *sb_pb_table,
>               const struct sbrec_ha_chassis_group_table *sb_ha_ch_grp_table,
>               struct ovsdb_idl_index *sb_ha_ch_grp_by_name,
> -             struct hmap *ls_ports)
> +             struct hmap *ls_ports,
> +             struct hmap *lr_ports)
>  {
>      if (!ovnnb_txn ||
>          !ovsdb_idl_has_ever_connected(ovsdb_idl_txn_get_idl(ovnsb_txn))) {
> @@ -17706,7 +17736,7 @@ ovnsb_db_run(struct ovsdb_idl_txn *ovnnb_txn,
> 
>      struct shash ha_ref_chassis_map = SHASH_INITIALIZER(&ha_ref_chassis_map);
>      handle_port_binding_changes(ovnsb_txn, sb_pb_table, sb_ha_ch_grp_table,
> -                                sb_ha_ch_grp_by_name, ls_ports,
> +                                sb_ha_ch_grp_by_name, ls_ports, lr_ports,
>                                  &ha_ref_chassis_map);
>      if (ovnsb_txn) {
>          update_sb_ha_group_ref_chassis(sb_ha_ch_grp_table,
> diff --git a/northd/northd.h b/northd/northd.h
> index f3e63b1e1..44dc11009 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -326,7 +326,8 @@ void ovnsb_db_run(struct ovsdb_idl_txn *ovnnb_txn,
>                    const struct sbrec_port_binding_table *,
>                    const struct sbrec_ha_chassis_group_table *,
>                    struct ovsdb_idl_index *sb_ha_ch_grp_by_name,
> -                  struct hmap *ls_ports);
> +                  struct hmap *ls_ports,
> +                  struct hmap *lr_ports);
>  bool northd_handle_ls_changes(struct ovsdb_idl_txn *,
>                                const struct northd_input *,
>                                struct northd_data *);
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index 4fbf4f7e5..b1e9ba724 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -3113,6 +3113,21 @@ or
>              port on the logical router.  It is otherwise ignored.
>            </p>
>          </column>
> +
> +        <column name="options" key="hosting-chassis">
> +          <p>
> +            This option is populated by <code>ovn-northd</code>, rather
> +            than by the CMS plugin.
> +          </p>
> +
> +          <p>
> +            When a distributed gateway port is bound to a location in
> +            the OVN Southbound database
> +            <ref db="OVN_Southbound" table="Port_Binding"/>
> +            <code>ovn-northd</code> will populate this option with the
> +            name of the Chassis that is currently hosting this port.
> +          </p>
> +        </column>
>        </group>
>      </group>
> 
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 5d871b287..580be42e6 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -144,6 +144,40 @@ AT_CHECK([test x`ovn-nbctl lsp-get-up S1-R1` = xup])
>  AT_CLEANUP
>  ])
> 
> +OVN_FOR_EACH_NORTHD_NO_HV([
> +AT_SETUP([check Logical Router Port hosting-chassis option])
> +ovn_start
> +
> +check ovn-sbctl chassis-add ch1 geneve 127.0.0.2
> +
> +check ovn-nbctl lr-add lr1
> +check ovn-nbctl lrp-add lr1 lrp1 00:00:00:00:00:01 10.0.0.1/24
> +check ovn-nbctl ls-add ls1
> +check ovn-nbctl lsp-add ls1 lsp1 -- \
> +    lsp-set-addresses lsp1 router -- \
> +    lsp-set-type lsp1 router -- \
> +    lsp-set-options lsp1 router-port=lrp1
> +
> +# make lrp a cr-port
> +check ovn-nbctl lrp-set-gateway-chassis lrp1 ch1
> +
> +ovn-nbctl --wait=sb sync
> +
> +wait_row_count Port_Binding 1 logical_port=cr-lrp1 \
> +    options:always-redirect="true" options:distributed-port="lrp1"
> +
> +# Simulate cr-port being bound to ch1
> +ch1_uuid=`ovn-sbctl --bare --columns _uuid find Chassis name="ch1"`
> +ovn-sbctl set Port_Binding cr-lrp1 chassis=${ch1_uuid}
> +
> +ovn-nbctl --wait=sb sync
> +
> +# check for the hosting-chassis option being set by northd
> +wait_row_count nb:Logical_Router_Port 1 name=lrp1 options:hosting-chassis=ch1
> +
> +AT_CLEANUP
> +])
> +
>  OVN_FOR_EACH_NORTHD_NO_HV([
>  AT_SETUP([check LRP external id propagation to SBDB])
>  ovn_start
> --

Regards,
Dumitru

On Tue, Jul 25, 2023 at 2:28 PM Dumitru Ceara <dceara@redhat.com> wrote:
>
> On 7/5/23 10:36, lmartins@redhat.com wrote:
> > From: Lucas Alvares Gomes <lucasagomes@gmail.com>
> >
> > In order for the CMS to know which Chassis a distributed gateway port
> > is bond to, this patch updates the ovn-northd daemon to populate the
> > Logical_Router_Port table with that information.
> >
> > To avoid changing the database schema, ovn-northd is setting a new key
> > called "hosting-chassis" in the options column from the LRP table. This
> > key value points to the name of the Chassis that is currently hosting
> > the distributed port.
> >
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2107515
> > Signed-off-by: Lucas Alvares Gomes <lucasagomes@gmail.com>
> > ---
> >
>
> Hi Lucas!
>
> Thanks for the patch!  And sorry for taking long to review.
>

Thank you for the review! No problem at all, appreciate it.

> > v1 -> v2
> > * Rebased on the main branch
> > * Updated the ovnsb_db_run() and handle_port_binding_changes() functions
> >   to include the LR ports as a parameter
> >
> >  northd/en-sync-from-sb.c |  2 +-
> >  northd/northd.c          | 34 ++++++++++++++++++++++++++++++++--
> >  northd/northd.h          |  3 ++-
> >  ovn-nb.xml               | 15 +++++++++++++++
> >  tests/ovn-northd.at      | 34 ++++++++++++++++++++++++++++++++++
> >  5 files changed, 84 insertions(+), 4 deletions(-)
> >
> > diff --git a/northd/en-sync-from-sb.c b/northd/en-sync-from-sb.c
> > index 55ece2d16..4109aebe4 100644
> > --- a/northd/en-sync-from-sb.c
> > +++ b/northd/en-sync-from-sb.c
> > @@ -60,7 +60,7 @@ en_sync_from_sb_run(struct engine_node *node, void *data OVS_UNUSED)
> >      stopwatch_start(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec());
> >      ovnsb_db_run(eng_ctx->ovnnb_idl_txn, eng_ctx->ovnsb_idl_txn,
> >                   sb_pb_table, sb_ha_ch_grp_table, sb_ha_ch_grp_by_name,
> > -                 &nd->ls_ports);
> > +                 &nd->ls_ports, &nd->lr_ports);
> >      stopwatch_stop(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec());
> >  }
> >
> > diff --git a/northd/northd.c b/northd/northd.c
> > index 309e0abd0..58da9c086 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -17631,6 +17631,7 @@ handle_port_binding_changes(struct ovsdb_idl_txn *ovnsb_txn,
> >                  const struct sbrec_ha_chassis_group_table *sb_ha_ch_grp_table,
> >                  struct ovsdb_idl_index *sb_ha_ch_grp_by_name,
> >                  struct hmap *ls_ports,
> > +                struct hmap *lr_ports,
> >                  struct shash *ha_ref_chassis_map)
> >  {
> >      const struct sbrec_port_binding *sb;
> > @@ -17649,6 +17650,34 @@ handle_port_binding_changes(struct ovsdb_idl_txn *ovnsb_txn,
> >      }
> >
> >      SBREC_PORT_BINDING_TABLE_FOR_EACH (sb, sb_pb_table) {
> > +
> > +        /* Look for a chassisredirect binding and set the "active-chassis"
> > +         * option in the NBDB logical_router_port table indicating on which
> > +         * chassis the distributed port is bond to. */
> > +        if (!strcmp(sb->type, "chassisredirect")) {
> > +             const char *dist_port =
> > +                 smap_get(&sb->options, "distributed-port");
>
> Isn't it easier to just use the op->l3dgw_port instead of looking it up
> again here?  We lookup 'op' just below in the port binding loop.
>

Ah great! I didn't know about that l3dgw_port, I will change the patch
and test it out.


> > +             if (dist_port) {
> > +                 struct ovn_port *router_port =
> > +                     ovn_port_find(lr_ports, dist_port);
> > +                 if (router_port) {
> > +                     struct smap options;
> > +                     smap_clone(&options, &router_port->nbrp->options);
> > +                     if (sb->chassis) {
> > +                         smap_replace(&options, "hosting-chassis",
> > +                                      sb->chassis->name);
> > +                     } else {
> > +                         smap_remove(&options, "hosting-chassis");
> > +                     }
> > +                     nbrec_logical_router_port_set_options(router_port->nbrp,
> > +                                                           &options);
>
> We leak 'options' here.  Also reported in CI:
>
> https://github.com/ovsrobot/ovn/actions/runs/5462364730
>
> You might want to add a 'smap_destroy(&options)' here.
>

Oops, thanks for pointing it out! Will fix.

> Can we move this "update the options" logic into a separate
> chassis-redirect specific function?
>

Certainly, will make the code more organized. I will do it.

> > +                  }
> > +             }
> > +             /* Continue since there are no matching logical port for
> > +              * chassisredirect bindings */
> > +             continue;
> > +        }
> > +
> >          struct ovn_port *op = ovn_port_find(ls_ports, sb->logical_port);
> >
> >          if (!op || !op->nbsp) {
> > @@ -17697,7 +17726,8 @@ ovnsb_db_run(struct ovsdb_idl_txn *ovnnb_txn,
> >               const struct sbrec_port_binding_table *sb_pb_table,
> >               const struct sbrec_ha_chassis_group_table *sb_ha_ch_grp_table,
> >               struct ovsdb_idl_index *sb_ha_ch_grp_by_name,
> > -             struct hmap *ls_ports)
> > +             struct hmap *ls_ports,
> > +             struct hmap *lr_ports)
> >  {
> >      if (!ovnnb_txn ||
> >          !ovsdb_idl_has_ever_connected(ovsdb_idl_txn_get_idl(ovnsb_txn))) {
> > @@ -17706,7 +17736,7 @@ ovnsb_db_run(struct ovsdb_idl_txn *ovnnb_txn,
> >
> >      struct shash ha_ref_chassis_map = SHASH_INITIALIZER(&ha_ref_chassis_map);
> >      handle_port_binding_changes(ovnsb_txn, sb_pb_table, sb_ha_ch_grp_table,
> > -                                sb_ha_ch_grp_by_name, ls_ports,
> > +                                sb_ha_ch_grp_by_name, ls_ports, lr_ports,
> >                                  &ha_ref_chassis_map);
> >      if (ovnsb_txn) {
> >          update_sb_ha_group_ref_chassis(sb_ha_ch_grp_table,
> > diff --git a/northd/northd.h b/northd/northd.h
> > index f3e63b1e1..44dc11009 100644
> > --- a/northd/northd.h
> > +++ b/northd/northd.h
> > @@ -326,7 +326,8 @@ void ovnsb_db_run(struct ovsdb_idl_txn *ovnnb_txn,
> >                    const struct sbrec_port_binding_table *,
> >                    const struct sbrec_ha_chassis_group_table *,
> >                    struct ovsdb_idl_index *sb_ha_ch_grp_by_name,
> > -                  struct hmap *ls_ports);
> > +                  struct hmap *ls_ports,
> > +                  struct hmap *lr_ports);
> >  bool northd_handle_ls_changes(struct ovsdb_idl_txn *,
> >                                const struct northd_input *,
> >                                struct northd_data *);
> > diff --git a/ovn-nb.xml b/ovn-nb.xml
> > index 4fbf4f7e5..b1e9ba724 100644
> > --- a/ovn-nb.xml
> > +++ b/ovn-nb.xml
> > @@ -3113,6 +3113,21 @@ or
> >              port on the logical router.  It is otherwise ignored.
> >            </p>
> >          </column>
> > +
> > +        <column name="options" key="hosting-chassis">
> > +          <p>
> > +            This option is populated by <code>ovn-northd</code>, rather
> > +            than by the CMS plugin.
> > +          </p>
> > +
> > +          <p>
> > +            When a distributed gateway port is bound to a location in
> > +            the OVN Southbound database
> > +            <ref db="OVN_Southbound" table="Port_Binding"/>
> > +            <code>ovn-northd</code> will populate this option with the
> > +            name of the Chassis that is currently hosting this port.
> > +          </p>
> > +        </column>
> >        </group>
> >      </group>
> >
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index 5d871b287..580be42e6 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -144,6 +144,40 @@ AT_CHECK([test x`ovn-nbctl lsp-get-up S1-R1` = xup])
> >  AT_CLEANUP
> >  ])
> >
> > +OVN_FOR_EACH_NORTHD_NO_HV([
> > +AT_SETUP([check Logical Router Port hosting-chassis option])
> > +ovn_start
> > +
> > +check ovn-sbctl chassis-add ch1 geneve 127.0.0.2
> > +
> > +check ovn-nbctl lr-add lr1
> > +check ovn-nbctl lrp-add lr1 lrp1 00:00:00:00:00:01 10.0.0.1/24
> > +check ovn-nbctl ls-add ls1
> > +check ovn-nbctl lsp-add ls1 lsp1 -- \
> > +    lsp-set-addresses lsp1 router -- \
> > +    lsp-set-type lsp1 router -- \
> > +    lsp-set-options lsp1 router-port=lrp1
> > +
> > +# make lrp a cr-port
> > +check ovn-nbctl lrp-set-gateway-chassis lrp1 ch1
> > +
> > +ovn-nbctl --wait=sb sync
> > +
> > +wait_row_count Port_Binding 1 logical_port=cr-lrp1 \
> > +    options:always-redirect="true" options:distributed-port="lrp1"
> > +
> > +# Simulate cr-port being bound to ch1
> > +ch1_uuid=`ovn-sbctl --bare --columns _uuid find Chassis name="ch1"`
> > +ovn-sbctl set Port_Binding cr-lrp1 chassis=${ch1_uuid}
> > +
> > +ovn-nbctl --wait=sb sync
> > +
> > +# check for the hosting-chassis option being set by northd
> > +wait_row_count nb:Logical_Router_Port 1 name=lrp1 options:hosting-chassis=ch1
> > +
> > +AT_CLEANUP
> > +])
> > +
> >  OVN_FOR_EACH_NORTHD_NO_HV([
> >  AT_SETUP([check LRP external id propagation to SBDB])
> >  ovn_start
> > --
>

Thanks again for the review Dumitru!


> Regards,
> Dumitru
>