On Thu, Aug 3, 2023 at 3:15 PM <lmart...@redhat.com> wrote: > From: Lucas Alvares Gomes <lucasago...@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 <lucasago...@gmail.com> > --- > v4 -> v5 > * Fix test to remove sleep and assert on the right table > > v3 -> v4 > * Removed smap_clone for updating the options and replaced it with > smap_replace/smap_remove > * Updated the test to also assert the removal of the hosting-chassis > key by northd > > v2 -> v3 > * Fixed memory leak > * Separate the logic to update the router port into their own function > * Use attribute l3dgw_port to find the GW port > > 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 | 30 +++++++++++++++++++++++++++-- > northd/northd.h | 3 ++- > ovn-nb.xml | 15 +++++++++++++++ > tests/ovn-northd.at | 41 ++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 87 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 b9605862e..c6752884c 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -17780,6 +17780,22 @@ build_ha_chassis_group_ref_chassis(struct > ovsdb_idl_index *ha_ch_grp_by_name, > } > } > > +/* Set the "hosting-chassis" option in the NBDB logical_router_port > + * table indicating which chassis the distributed port is bond to. */ > +static void > +handle_cr_port_binding_changes(const struct sbrec_port_binding *sb, > + struct ovn_port *orp) > +{ > + if (sb->chassis) { > + nbrec_logical_router_port_update_options_setkey( > + orp->l3dgw_port->nbrp, "hosting-chassis", > + sb->chassis->name); > + } else { > + nbrec_logical_router_port_update_options_delkey( > + orp->l3dgw_port->nbrp, "hosting-chassis"); > + } > +} > + > /* Handle changes to the 'chassis' column of the 'Port_Binding' table. > When > * this column is not empty, it means we need to set the corresponding > logical > * port as 'up' in the northbound DB. */ > @@ -17789,6 +17805,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; > @@ -17807,6 +17824,14 @@ handle_port_binding_changes(struct ovsdb_idl_txn > *ovnsb_txn, > } > > SBREC_PORT_BINDING_TABLE_FOR_EACH (sb, sb_pb_table) { > + > + struct ovn_port *orp = ovn_port_find(lr_ports, sb->logical_port); > + > + if (orp && is_cr_port(orp)) { > + handle_cr_port_binding_changes(sb, orp); > + continue; > + } > + > struct ovn_port *op = ovn_port_find(ls_ports, sb->logical_port); > > if (!op || !op->nbsp) { > @@ -17855,7 +17880,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))) { > @@ -17864,7 +17890,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 3e06f14c9..99634d51f 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -169,6 +169,47 @@ 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 > + > +check 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"` > +check ovn-sbctl set Port_Binding cr-lrp1 chassis=${ch1_uuid} > + > +check 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 > + > +# Now remove the chassis from the port binding record and assert that the > +# hosting-chassis option was removed by northd > +check ovn-sbctl clear Port_Binding cr-lrp1 chassis > +check ovn-nbctl --wait=sb sync > + > +wait_row_count nb:Logical_Router_Port 0 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 > -- > 2.41.0 > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Looks good to me, thanks!
Acked-by: Ales Musil <amu...@redhat.com> -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> amu...@redhat.com IM: amusil <https://red.ht/sig> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
