On Tue, Sep 17, 2024 at 2:31 AM Ales Musil <[email protected]> wrote:
>
> On Tue, Sep 17, 2024 at 8:22 AM Mansi Sharma <[email protected]>
> wrote:
>
> > This change will be useful for migration cases, where it can be
> > used to sync ct-entries before port is up on new chassis,
> > resulting in reduced network package drops.
> > It also fulfills the need of any other service which might need
> > advance ct-zone reservation in future.
> >
> > Signed-off-by: Mansi Sharma <[email protected]>
Thanks for v6.
During my testing I found one issue related to incremental processing.
Looks like that needs to be fixed.
Below are the steps
---------------
ovn-nbctl ls-add sw0
ovn-nbctl lsp-add sw0 sw0p1
ovs-vsctl \
-- add-port br-int vif1 \
-- set Interface vif1 external_ids:iface-id=sw0p1
ovn-nbctl --wait=hv sync
ovs-vsctl set Open_vSwitch . external_ids:reserve_ct_zones=sw0p1
ovs-vsctl get bridge br-int external_ids:ct-zone-sw0p1
# Delete the lsp - sw0p1
ovn-nbctl --wait=hv lsp-del sw0p1
# This below command returns error since the zone id for sw0p1 is not reserved.
ovs-vsctl get bridge br-int external_ids:ct-zone-sw0p1
# After recompute, the problem is fixed.
ovn-appctl -t ovn-controller inc-engine/recompute
ovs-vsctl get bridge br-int external_ids:ct-zone-sw0p1
------------
I think you also need to add some code in
ct_zones_runtime_data_handler() to address this issue.
Other than this, the patch LGTM.
Thanks
Numan
> > ---
> > v5-->v6
> > Updated tests to check duplicates more frequntly with input as ct_zones.
> > ---
> > controller/ct-zone.c | 25 ++++++++++
> > controller/ovn-controller.8.xml | 15 +++++-
> > tests/ovn-controller.at | 86 ++++++++++++++++++++++++++++++---
> > 3 files changed, 117 insertions(+), 9 deletions(-)
> >
> > diff --git a/controller/ct-zone.c b/controller/ct-zone.c
> > index 77eb16ac9..bd1b0623b 100644
> > --- a/controller/ct-zone.c
> > +++ b/controller/ct-zone.c
> > @@ -183,6 +183,31 @@ ct_zones_update(const struct sset *local_lports,
> > sset_add(&all_users, local_lport);
> > }
> >
> > + /* Add local_lport name which are supposed to come up on the
> > + * chassis and might need ct-zone reservation in advance.
> > + * The data is picked up from ovs_vswitch table. */
> > + const struct ovsrec_open_vswitch *cfg;
> > + cfg = ovsrec_open_vswitch_table_first(ovs_table);
> > +
> > + if (cfg) {
> > + const char *reserve_ct_zone_request_list = smap_get(
> > + &cfg->external_ids, "reserve_ct_zones");
> > +
> > + if (reserve_ct_zone_request_list) {
> > + char *dup_reserve = xstrdup(reserve_ct_zone_request_list);
> > + char *reserve_port;
> > + char *save_ptr = NULL;
> > +
> > + for (reserve_port = strtok_r(dup_reserve, ",", &save_ptr);
> > + reserve_port != NULL;
> > + reserve_port = strtok_r(NULL, ",", &save_ptr)) {
> > + sset_add(&all_users, reserve_port);
> > + }
> > +
> > + free(dup_reserve);
> > + }
> > + }
> > +
> > /* Local patched datapath (gateway routers) need zones assigned. */
> > const struct local_datapath *ld;
> > HMAP_FOR_EACH (ld, hmap_node, local_datapaths) {
> > diff --git a/controller/ovn-controller.8.xml
> > b/controller/ovn-controller.8.xml
> > index faefa77b9..b070b724c 100644
> > --- a/controller/ovn-controller.8.xml
> > +++ b/controller/ovn-controller.8.xml
> > @@ -626,7 +626,20 @@
> > <code>external_ids:ovn-installed-ts</code>.
> > </p>
> > </dd>
> > - </dl>
> > +
> > + <dt>
> > + <code>external-ids:reserve_ct_zones</code> in the
> > <code>Bridge</code>
> > + table
> > + </dt>
> > +
> > + <dd>
> > + <p>
> > + This key represents list of ports which are supposed to come up
> > on
> > + the chassis, and hence need advance reservation of ct-zones.
> > + It is comma seprated list of port names.
> > + </p>
> > + </dd>
> > + </dl>
> >
> > <h1>OVN Southbound Database Usage</h1>
> >
> > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> > index 74bff9035..4377ac6f2 100644
> > --- a/tests/ovn-controller.at
> > +++ b/tests/ovn-controller.at
> > @@ -2533,8 +2533,14 @@ check_ovsdb_zone() {
> > test $ct_zone -eq $db_zone
> > }
> >
> > +check_duplicates() {
> > + output=$1
> > + AT_CHECK([printf "$output" | tr ' ' '\n' | sort | uniq -d | grep .],
> > [1])
> > +}
> > +
> > check ovs-vsctl add-port br-int ls0-hv1 -- set Interface ls0-hv1
> > external-ids:iface-id=ls0-hv1
> > check ovs-vsctl add-port br-int ls0-hv2 -- set Interface ls0-hv2
> > external-ids:iface-id=ls0-hv2
> > +check ovs-vsctl set Open_vSwitch .
> > external_ids:reserve_ct_zones=ls0-req-hv3,ls0-req-hv4
> >
> > check ovn-nbctl lr-add lr0
> >
> > @@ -2560,17 +2566,18 @@ echo "$ct_zones"
> >
> > port1_zone=$(get_zone_num "$ct_zones" ls0-hv1)
> > port2_zone=$(get_zone_num "$ct_zones" ls0-hv2)
> > -
> > +req_port3_zone=$(get_zone_num "$ct_zones" ls0-req-hv3)
> > +req_port4_zone=$(get_zone_num "$ct_zones" ls0-req-hv4)
> > snat_zone=$(get_zone_num "$ct_zones" lr0_snat)
> > echo "snat_zone is $snat_zone"
> >
> > -check test "$port1_zone" -ne "$port2_zone"
> > -check test "$port2_zone" -ne "$snat_zone"
> > -check test "$port1_zone" -ne "$snat_zone"
> > -
> > OVS_WAIT_UNTIL([check_ovsdb_zone ls0-hv1 $port1_zone])
> > OVS_WAIT_UNTIL([check_ovsdb_zone ls0-hv2 $port2_zone])
> > OVS_WAIT_UNTIL([check_ovsdb_zone lr0_snat $snat_zone])
> > +OVS_WAIT_UNTIL([check_ovsdb_zone ls0-req-hv3 $req_port3_zone])
> > +OVS_WAIT_UNTIL([check_ovsdb_zone ls0-req-hv4 $req_port4_zone])
> > +
> > +check_duplicates "$ct_zones"
> >
> > # Now purposely request an SNAT zone for lr0 that conflicts with a zone
> > # currently assigned to a logical port
> > @@ -2585,15 +2592,78 @@ echo "$ct_zones"
> > port1_zone=$(get_zone_num "$ct_zones" ls0-hv1)
> > port2_zone=$(get_zone_num "$ct_zones" ls0-hv2)
> > snat_zone=$(get_zone_num "$ct_zones" lr0_snat)
> > +req_port3_zone=$(get_zone_num "$ct_zones" ls0-req-hv3)
> > +req_port4_zone=$(get_zone_num "$ct_zones" ls0-req-hv4)
> >
> > check test "$snat_zone" -eq "$snat_req_zone"
> > -check test "$port1_zone" -ne "$port2_zone"
> > -check test "$port2_zone" -ne "$snat_zone"
> > -check test "$port1_zone" -ne "$snat_zone"
> > +
> > +check_duplicates "$ct_zones"
> > +
> > +OVS_WAIT_UNTIL([check_ovsdb_zone ls0-hv1 $port1_zone])
> > +OVS_WAIT_UNTIL([check_ovsdb_zone ls0-hv2 $port2_zone])
> > +OVS_WAIT_UNTIL([check_ovsdb_zone lr0_snat $snat_zone])
> > +OVS_WAIT_UNTIL([check_ovsdb_zone ls0-req-hv3 $req_port3_zone])
> > +OVS_WAIT_UNTIL([check_ovsdb_zone ls0-req-hv4 $req_port4_zone])
> > +
> > +# Add port named ls0-req-hv3 and check if same zone assigned
> > +# previously get assigned to it this time as well.
> > +
> > +check ovn-nbctl lsp-add ls0 ls0-req-hv3
> > +check ovs-vsctl -- add-port br-int hv3-vif3 -- \
> > + set interface hv3-vif3 external-ids:iface-id=ls0-req-hv3
> > +ct_zones=$(ovn-appctl -t ovn-controller ct-zone-list)
> > +echo "$ct_zones"
> > +
> > +check ovn-nbctl --wait=hv sync
> > +
> > +req_port3_zone_new=$(get_zone_num "$ct_zones" ls0-req-hv3)
> > +check test "$req_port3_zone -eq $req_port3_zone_new"
> > +check_duplicates "$ct_zones"
> > +
> > +OVS_WAIT_UNTIL([check_ovsdb_zone ls0-hv1 $port1_zone])
> > +OVS_WAIT_UNTIL([check_ovsdb_zone ls0-hv2 $port2_zone])
> > +OVS_WAIT_UNTIL([check_ovsdb_zone lr0_snat $snat_zone])
> > +OVS_WAIT_UNTIL([check_ovsdb_zone ls0-req-hv3 $req_port3_zone])
> > +OVS_WAIT_UNTIL([check_ovsdb_zone ls0-req-hv4 $req_port4_zone])
> > +
> > +# Checks for two cases after removing entry from ovs_vswitch table -
> > +# 1. If port is already up, ct-zone should be reserved.
> > +# 2. If port is not up yet, ct-zone should not be reserved.
> > +
> > +check ovs-vsctl remove Open_vSwitch . external_ids reserve_ct_zones
> > +
> > +ct_zones=$(ovn-appctl -t ovn-controller ct-zone-list)
> > +echo "$ct_zones"
> > +
> > +req_port3_zone_after_delete=$(get_zone_num "$ct_zones" ls0-req-hv3)
> > +req_port4_zone_after_delete=$(get_zone_num "$ct_zones" ls0-req-hv4)
> > +
> > +check test "$req_port3_zone_new" -eq "$req_port3_zone_after_delete"
> > +check test "$req_port4_zone_after_delete" == ""
> > +
> > +# Checks for case when a ct-zone is reserved it comes up on that chassis,
> > and
> > +# gets deleted, but its persisted in ovs_vswitch table, it should persist
> > the
> > +# same zone throughout.
> > +
> > +check ovs-vsctl set Open_vSwitch .
> > external_ids:reserve_ct_zones=ls0-req-hv5
> > +check ovn-nbctl lsp-add ls0 ls0-req-hv5
> > +check ovs-vsctl -- add-port br-int hv5-vif5 -- \
> > + set interface hv5-vif5 external-ids:iface-id=ls0-req-hv5
> > +ct_zones=$(ovn-appctl -t ovn-controller ct-zone-list)
> > +echo "$ct_zones"
> > +req_port5_zone=$(get_zone_num "$ct_zones" ls0-req-hv5)
> > +
> > +check ovs-vsctl remove interface hv5-vif5 external_ids iface-id
> > +echo "$ct_zones"
> > +req_port5_zone_new=$(get_zone_num "$ct_zones" ls0-req-hv5)
> > +
> > +check test "$req_port5_zone" -eq "$req_port5_zone_new"
> > +check_duplicates "$ct_zones"
> >
> > OVS_WAIT_UNTIL([check_ovsdb_zone ls0-hv1 $port1_zone])
> > OVS_WAIT_UNTIL([check_ovsdb_zone ls0-hv2 $port2_zone])
> > OVS_WAIT_UNTIL([check_ovsdb_zone lr0_snat $snat_zone])
> > +OVS_WAIT_UNTIL([check_ovsdb_zone ls0-req-hv3 $req_port3_zone])
> >
> > # Now create a conflict in the OVSDB and restart ovn-controller.
> >
> > --
> > 2.22.3
> >
> >
> Looks good to me, thanks.
>
> Acked-by: Ales Musil <[email protected]>
> --
>
> Ales Musil
>
> Senior Software Engineer - OVN Core
>
> Red Hat EMEA <https://www.redhat.com>
>
> [email protected]
> <https://red.ht/sig>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
