Hi Ales, Thank you for the insightful review!
On Thu, Aug 3, 2023 at 11:25 AM Ales Musil <[email protected]> wrote: > > > > On Tue, Aug 1, 2023 at 4:11 PM <[email protected]> wrote: >> >> From: Lucas Alvares Gomes <[email protected]> >> >> 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 <[email protected]> > > > Hi Lucas, > > I have a few comments down below. > >> >> --- >> 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 | 33 +++++++++++++++++++++++++++++++-- >> northd/northd.h | 3 ++- >> ovn-nb.xml | 15 +++++++++++++++ >> tests/ovn-northd.at | 34 ++++++++++++++++++++++++++++++++++ >> 5 files changed, 83 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..b2a59ab3a 100644 >> --- a/northd/northd.c >> +++ b/northd/northd.c >> @@ -17780,6 +17780,25 @@ 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) >> +{ >> + struct smap options; >> + smap_clone(&options, &orp->l3dgw_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(orp->l3dgw_port->nbrp, >> + &options); >> + smap_destroy(&options); >> +} > > > We can avoid the expensive smap_clone whatsoever by replacing the > smap_replace/smap_remove > with direct operation on the DB row: > nbrec_logical_router_port_update_options_setkey and > nbrec_logical_router_port_update_options_delkey > Great point, code will get even more simpler with the suggestion. >> >> + >> /* 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 +17808,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 +17827,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 +17883,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 +17893,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..ea439138e 100644 >> --- a/tests/ovn-northd.at >> +++ b/tests/ovn-northd.at >> @@ -169,6 +169,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 > > > I would also remove the chassis to make sure that the hosting-chassis > reflects that. > Will do! >> >> + >> +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 >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > > Thanks, > Ales > > -- > > Ales Musil > > Senior Software Engineer - OVN Core > > Red Hat EMEA > > [email protected] IM: amusil _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
