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
> +
> /* 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.
> +
> +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 <https://www.redhat.com>
[email protected] IM: amusil
<https://red.ht/sig>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
