On Tue, Jul 25, 2023 at 2:28 PM Dumitru Ceara <dce...@redhat.com> wrote:
>
> On 7/5/23 10:36, 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>
> > ---
> >
>
> 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
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
