| Message ID | 20230726124239.66275-1-amusil@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v2] controller: Migrate from ct zone UUID name to component name | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On 7/26/23 14:42, Ales Musil wrote: > There are two scenarios that could cause unwanted > CT zone flush: > > 1) The SB DB is destroyed and recreated. The new > database will end up with different UUIDs for > various components. > > 2) Upgrade of existing SB DB to ovn-ic. > The components are the same as before, but scattered > between multiple SB DBs. This again leads to different > UUIDs in SB DB. > > The CT zone name was based on datapath UUID which > causes flush when the UUID changes. Even if > the datapath is the same. > > To prevent the unwanted flush migrate from UUID > to component name (LR/LS name). This allows > the CT zones to be stable across the before mentioned > scenarios. > > For the migration to be "flush less" itself we need > to make sure to start the restoration process only > after controller is connected to the SB DB e.g. the > restoration can happen only during engine run and not > init as it was done previously. > > Reported-at: https://bugzilla.redhat.com/2224199 > Tested-by: Surya Seetharaman <sseethar@redhat.com> > Signed-off-by: Ales Musil <amusil@redhat.com> > --- > v2: Address comments from Dumitru. > Add TODO for the external_ids tracking. > Simplify the id retrieval in physical.c > --- > controller/ovn-controller.c | 106 ++++++++++++++++++++---- > controller/physical.c | 17 ++-- > lib/ovn-util.c | 4 +- > lib/ovn-util.h | 2 +- > tests/ovn-controller.at | 16 ++-- > tests/ovn.at | 156 ++++++++++++++++++++++++++++++++++++ > 6 files changed, 267 insertions(+), 34 deletions(-) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 236974f4f..d34dba75c 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -744,8 +744,17 @@ update_ct_zones(const struct sset *local_lports, > HMAP_FOR_EACH (ld, hmap_node, local_datapaths) { > /* XXX Add method to limit zone assignment to logical router > * datapaths with NAT */ > - char *dnat = alloc_nat_zone_key(&ld->datapath->header_.uuid, "dnat"); > - char *snat = alloc_nat_zone_key(&ld->datapath->header_.uuid, "snat"); > + const char *name = smap_get(&ld->datapath->external_ids, "name"); > + if (!name) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > + VLOG_ERR_RL(&rl, "Missing name for datapath '"UUID_FMT"' " > + "skipping zone assignment.", > + UUID_ARGS(&ld->datapath->header_.uuid)); > + continue; > + } > + > + char *dnat = alloc_nat_zone_key(name, "dnat"); > + char *snat = alloc_nat_zone_key(name, "snat"); > sset_add(&all_users, dnat); > sset_add(&all_users, snat); > > @@ -882,9 +891,66 @@ struct ed_type_ct_zones { > bool recomputed; > }; > > +/* Replaces a UUID prefix from 'uuid_zone' (if any) with the > + * corresponding Datapath_Binding.external_ids.name. Returns it > + * as a new string that that will be owned by the caller. */ Typo: "that that". This is my fault.. I had suggested it, sorry. But maybe it can be fixed up at apply time. > +static char * > +ct_zone_name_from_uuid(const struct sbrec_datapath_binding_table *dp_table, > + const char *uuid_zone) > +{ > + struct uuid uuid; > + if (!uuid_from_string_prefix(&uuid, uuid_zone)) { > + return NULL; > + } > + > + const struct sbrec_datapath_binding *dp = > + sbrec_datapath_binding_table_get_for_uuid(dp_table, &uuid); > + if (!dp) { > + return NULL; > + } > + > + const char *entity_name = smap_get(&dp->external_ids, "name"); > + if (!entity_name) { > + return NULL; > + } > + > + return xasprintf("%s%s", entity_name, uuid_zone + UUID_LEN); > +} > + > +static void > +ct_zone_restore(const struct sbrec_datapath_binding_table *dp_table, > + struct ed_type_ct_zones *ct_zones_data, const char *name, > + int zone) > +{ > + VLOG_DBG("restoring ct zone %"PRId32" for '%s'", zone, name); > + > + char *new_name = ct_zone_name_from_uuid(dp_table, name); > + const char *current_name = name; > + if (new_name) { > + VLOG_DBG("ct zone %"PRId32" replace uuid name '%s' with '%s'", > + zone, name, new_name); > + > + /* Make sure we remove the uuid one in the next OvS DB commit without > + * flush. */ > + add_pending_ct_zone_entry(&ct_zones_data->pending, CT_ZONE_DB_QUEUED, > + zone, false, name); > + /* Store the zone in OvS DB with name instead of uuid without flush. > + * */ > + add_pending_ct_zone_entry(&ct_zones_data->pending, CT_ZONE_DB_QUEUED, > + zone, true, new_name); > + current_name = new_name; > + } > + > + simap_put(&ct_zones_data->current, current_name, zone); > + bitmap_set1(ct_zones_data->bitmap, zone); > + > + free(new_name); > +} > + > static void > restore_ct_zones(const struct ovsrec_bridge_table *bridge_table, > const struct ovsrec_open_vswitch_table *ovs_table, > + const struct sbrec_datapath_binding_table *dp_table, > struct ed_type_ct_zones *ct_zones_data) > { > memset(ct_zones_data->bitmap, 0, sizeof ct_zones_data->bitmap); > @@ -895,10 +961,8 @@ restore_ct_zones(const struct ovsrec_bridge_table *bridge_table, > struct ct_zone_pending_entry *ctpe = pending_node->data; > > if (ctpe->add) { > - VLOG_DBG("restoring ct zone %"PRId32" for '%s'", ctpe->zone, > - pending_node->name); > - bitmap_set1(ct_zones_data->bitmap, ctpe->zone); > - simap_put(&ct_zones_data->current, pending_node->name, ctpe->zone); > + ct_zone_restore(dp_table, ct_zones_data, > + pending_node->name, ctpe->zone); > } > } > > @@ -936,9 +1000,7 @@ restore_ct_zones(const struct ovsrec_bridge_table *bridge_table, > continue; > } > > - VLOG_DBG("restoring ct zone %"PRId32" for '%s'", zone, user); > - bitmap_set1(ct_zones_data->bitmap, zone); > - simap_put(&ct_zones_data->current, user, zone); > + ct_zone_restore(dp_table, ct_zones_data, user, zone); > } > } > > @@ -1089,6 +1151,10 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl) > ovsdb_idl_track_add_column(ovs_idl, > &ovsrec_flow_sample_collector_set_col_id); > mirror_register_ovs_idl(ovs_idl); > + /* XXX: There is a potential bug in CT zone I-P node, > + * the fact that we have to call recompute for the change of > + * OVS.bridge.external_ids be reflected. Currently, we don't > + * track that column which should be addressed in the future. */ > } > > #define SB_NODES \ > @@ -2416,18 +2482,14 @@ out: > } > > static void * > -en_ct_zones_init(struct engine_node *node, struct engine_arg *arg OVS_UNUSED) > +en_ct_zones_init(struct engine_node *node OVS_UNUSED, > + struct engine_arg *arg OVS_UNUSED) > { > struct ed_type_ct_zones *data = xzalloc(sizeof *data); > - const struct ovsrec_open_vswitch_table *ovs_table = > - EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node)); > - const struct ovsrec_bridge_table *bridge_table = > - EN_OVSDB_GET(engine_get_input("OVS_bridge", node)); > > shash_init(&data->pending); > simap_init(&data->current); > > - restore_ct_zones(bridge_table, ovs_table, data); > return data; > } > > @@ -2458,8 +2520,10 @@ en_ct_zones_run(struct engine_node *node, void *data) > EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node)); > const struct ovsrec_bridge_table *bridge_table = > EN_OVSDB_GET(engine_get_input("OVS_bridge", node)); > + const struct sbrec_datapath_binding_table *dp_table = > + EN_OVSDB_GET(engine_get_input("SB_datapath_binding", node)); > > - restore_ct_zones(bridge_table, ovs_table, ct_zones_data); > + restore_ct_zones(bridge_table, ovs_table, dp_table, ct_zones_data); > update_ct_zones(&rt_data->local_lports, &rt_data->local_datapaths, > &ct_zones_data->current, ct_zones_data->bitmap, > &ct_zones_data->pending); > @@ -2507,7 +2571,15 @@ ct_zones_datapath_binding_handler(struct engine_node *node, void *data) > /* Check if the requested snat zone has changed for the datapath > * or not. If so, then fall back to full recompute of > * ct_zone engine. */ > - char *snat_dp_zone_key = alloc_nat_zone_key(&dp->header_.uuid, "snat"); > + const char *name = smap_get(&dp->external_ids, "name"); > + if (!name) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > + VLOG_ERR_RL(&rl, "Missing name for datapath '"UUID_FMT"' skipping" > + "zone check.", UUID_ARGS(&dp->header_.uuid)); > + continue; > + } > + > + char *snat_dp_zone_key = alloc_nat_zone_key(name, "snat"); > struct simap_node *simap_node = simap_find(&ct_zones_data->current, > snat_dp_zone_key); > free(snat_dp_zone_key); > diff --git a/controller/physical.c b/controller/physical.c > index edb144b9a..75257bc85 100644 > --- a/controller/physical.c > +++ b/controller/physical.c > @@ -209,17 +209,24 @@ static struct zone_ids > get_zone_ids(const struct sbrec_port_binding *binding, > const struct simap *ct_zones) > { > - struct zone_ids zone_ids; > + struct zone_ids zone_ids = { > + .ct = simap_get(ct_zones, binding->logical_port) > + }; Neat! > > - zone_ids.ct = simap_get(ct_zones, binding->logical_port); > + const char *name = smap_get(&binding->datapath->external_ids, "name"); > + if (!name) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > + VLOG_ERR_RL(&rl, "Missing name for datapath '"UUID_FMT"'", > + UUID_ARGS(&binding->datapath->header_.uuid)); > > - const struct uuid *key = &binding->datapath->header_.uuid; > + return zone_ids; > + } > > - char *dnat = alloc_nat_zone_key(key, "dnat"); > + char *dnat = alloc_nat_zone_key(name, "dnat"); > zone_ids.dnat = simap_get(ct_zones, dnat); > free(dnat); > > - char *snat = alloc_nat_zone_key(key, "snat"); > + char *snat = alloc_nat_zone_key(name, "snat"); > zone_ids.snat = simap_get(ct_zones, snat); > free(snat); > > diff --git a/lib/ovn-util.c b/lib/ovn-util.c > index a31788fd9..080ad4c0c 100644 > --- a/lib/ovn-util.c > +++ b/lib/ovn-util.c > @@ -485,9 +485,9 @@ split_addresses(const char *addresses, struct svec *ipv4_addrs, > * > * It is the caller's responsibility to free the allocated memory. */ > char * > -alloc_nat_zone_key(const struct uuid *key, const char *type) > +alloc_nat_zone_key(const char *name, const char *type) > { > - return xasprintf(UUID_FMT"_%s", UUID_ARGS(key), type); > + return xasprintf("%s_%s", name, type); > } > > const char * > diff --git a/lib/ovn-util.h b/lib/ovn-util.h > index c3c8a21df..5ebdd8adb 100644 > --- a/lib/ovn-util.h > +++ b/lib/ovn-util.h > @@ -118,7 +118,7 @@ const char *find_lport_address(const struct lport_addresses *laddrs, > void split_addresses(const char *addresses, struct svec *ipv4_addrs, > struct svec *ipv6_addrs); > > -char *alloc_nat_zone_key(const struct uuid *key, const char *type); > +char *alloc_nat_zone_key(const char *name, const char *type); > > const char *default_nb_db(void); > const char *default_sb_db(void); > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at > index 28c13234c..e1b6491b3 100644 > --- a/tests/ovn-controller.at > +++ b/tests/ovn-controller.at > @@ -2446,8 +2446,7 @@ echo "$ct_zones" > port1_zone=$(get_zone_num "$ct_zones" ls0-hv1) > port2_zone=$(get_zone_num "$ct_zones" ls0-hv2) > > -lr_uuid=$(fetch_column Datapath_Binding _uuid external_ids:name=lr0) > -snat_zone=$(get_zone_num "$ct_zones" ${lr_uuid}_snat) > +snat_zone=$(get_zone_num "$ct_zones" lr0_snat) > echo "snat_zone is $snat_zone" > > check test "$port1_zone" -ne "$port2_zone" > @@ -2456,7 +2455,7 @@ 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 ${lr_uuid}_snat $snat_zone]) > +OVS_WAIT_UNTIL([check_ovsdb_zone lr0_snat $snat_zone]) > > # Now purposely request an SNAT zone for lr0 that conflicts with a zone > # currently assigned to a logical port > @@ -2470,7 +2469,7 @@ 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" ${lr_uuid}_snat) > +snat_zone=$(get_zone_num "$ct_zones" lr0_snat) > > check test "$snat_zone" -eq "$snat_req_zone" > check test "$port1_zone" -ne "$port2_zone" > @@ -2479,7 +2478,7 @@ 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 ${lr_uuid}_snat $snat_zone]) > +OVS_WAIT_UNTIL([check_ovsdb_zone lr0_snat $snat_zone]) > > # Now create a conflict in the OVSDB and restart ovn-controller. > > @@ -2493,7 +2492,7 @@ 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" ${lr_uuid}_snat) > +snat_zone=$(get_zone_num "$ct_zones" lr0_snat) > > check test "$snat_zone" -eq "$snat_req_zone" > check test "$port1_zone" -ne "$port2_zone" > @@ -2502,7 +2501,7 @@ 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 ${lr_uuid}_snat $snat_zone]) > +OVS_WAIT_UNTIL([check_ovsdb_zone lr0_snat $snat_zone]) > > OVN_CLEANUP([hv1]) > AT_CLEANUP > @@ -2575,9 +2574,8 @@ check ovn-nbctl lrp-set-gateway-chassis lrp-gw hv1 > > check ovn-nbctl --wait=hv sync > > -lr_uuid=$(fetch_column Datapath_Binding _uuid external_ids:name=lr0) > ct_zones=$(ovn-appctl -t ovn-controller ct-zone-list) > -zone_num=$(printf "$ct_zones" | grep ${lr_uuid}_snat | cut -d ' ' -f 2) > +zone_num=$(printf "$ct_zones" | grep lr0_snat | cut -d ' ' -f 2) > > check test "$zone_num" -eq 666 > > diff --git a/tests/ovn.at b/tests/ovn.at > index 24da9174e..3de1444f8 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -36707,3 +36707,159 @@ OVN_CLEANUP([hv1]) > > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([Migration of CT zone from UUID to name]) > +ovn_start > +net_add n1 > + > +sim_add hv1 > +as hv1 > +ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.1 > + > +# sw0-port1 -- sw0 -- lr0 > + > +check ovn-nbctl lr-add lr0 > +check ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 192.168.0.1/24 > +check ovn-nbctl lrp-add lr0 lr0-sw1 00:00:00:00:ff:02 11.0.0.1/24 > + > +check ovn-nbctl ls-add sw0 > +check ovn-nbctl lsp-add sw0 sw0-port1 > +check ovn-nbctl lsp-set-addresses sw0-port1 "50:54:00:00:00:01 192.168.0.2" > + > +check ovn-nbctl lsp-add sw0 sw0-lr0 > +check ovn-nbctl lsp-set-type sw0-lr0 router > +check ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:ff:01 > +check ovn-nbctl lsp-set-options sw0-lr0 router-port=lr0-sw0 > + > +ovs-vsctl add-port br-int p1 -- \ > + set Interface p1 external_ids:iface-id=sw0-port1 > + > +check ovn-appctl -t ovn-controller vlog/set dbg:main > + > +wait_for_ports_up > +ovn-nbctl --wait=hv sync > + > +get_zone_num () { > + output=$1 > + name=$2 > + printf "$output" | grep $name | cut -d ' ' -f 2 > +} > + > +replace_with_uuid() { > + name=$1 > + > + id=$(ovn-sbctl --bare --columns _uuid find datapath external_ids:name=${name}) > + dnat=$(ovs-vsctl --bare get bridge br-int external_ids:ct-zone-${name}_dnat) > + snat=$(ovs-vsctl --bare get bridge br-int external_ids:ct-zone-${name}_snat) > + > + check ovs-vsctl remove bridge br-int external_ids ct-zone-${name}_snat > + check ovs-vsctl remove bridge br-int external_ids ct-zone-${name}_dnat > + > + check ovs-vsctl set bridge br-int external_ids:ct-zone-${id}_snat=$snat > + check ovs-vsctl set bridge br-int external_ids:ct-zone-${id}_dnat=$dnat > +} > + > +check_zones_in_ovsdb() { > + zone_list=$1 > + name=$2 > + > + id=$(ovn-sbctl --bare --columns _uuid find datapath external_ids:name=${name}) > + > + dnat=$(get_zone_num "$zone_list" ${name}_dnat) > + snat=$(get_zone_num "$zone_list" ${name}_snat) > + > + OVS_WAIT_UNTIL([ovs-vsctl --bare get bridge br-int external_ids:ct-zone-${name}_dnat]) > + dnat_ovs=$(ovs-vsctl --bare get bridge br-int external_ids:ct-zone-${name}_dnat | tr -d '"') > + OVS_WAIT_UNTIL([ovs-vsctl --bare get bridge br-int external_ids:ct-zone-${name}_dnat]) > + snat_ovs=$(ovs-vsctl --bare get bridge br-int external_ids:ct-zone-${name}_snat | tr -d '"') > + > + check test "$dnat" = "$dnat_ovs" > + check test "$snat" = "$snat_ovs" > + > + AT_CHECK([ovs-vsctl --bare get bridge br-int external_ids:ct-zone-${id}_dnat], [1], [ignore], [ignore]) > + AT_CHECK([ovs-vsctl --bare get bridge br-int external_ids:ct-zone-${id}_snat], [1], [ignore], [ignore]) > +} > + > +AS_BOX([Check newly created are with name only]) > +AT_CHECK([ovn-appctl -t ovn-controller ct-zone-list | sed "s/ [[0-9]]*/ ??/" | sort], [0], [dnl > +lr0_dnat ?? > +lr0_snat ?? > +sw0-port1 ?? > +sw0_dnat ?? > +sw0_snat ?? > +]) > + > +zone_list=$(ovn-appctl -t ovn-controller ct-zone-list) > + > +check_zones_in_ovsdb "$zone_list" lr0 > +check_zones_in_ovsdb "$zone_list" sw0 > + > +# Check that we did just the initial zone flush > +AT_CHECK([grep -c "NXT_CT_FLUSH_ZONE" hv1/ovs-vswitchd.log], [0], [dnl > +5 > +]) > + > +AS_BOX([Check conversion from UUID - recompute]) > +replace_with_uuid lr0 > +replace_with_uuid sw0 > + > +# XXX: This is potentially a bug, the fact that we have to call > +# recompute for the change to be reflected. Currently we don't > +# track the OVS.bridge.external_ids which should be addressed in future. > +ovn-appctl -t ovn-controller inc-engine/recompute > + > +OVS_WAIT_UNTIL([test "$(grep -c 'ct zone .* replace uuid name' hv1/ovn-controller.log)" = "4"]) > + > +AT_CHECK([ovn-appctl -t ovn-controller ct-zone-list | sed "s/ [[0-9]]*/ ??/" | sort], [0], [dnl > +lr0_dnat ?? > +lr0_snat ?? > +sw0-port1 ?? > +sw0_dnat ?? > +sw0_snat ?? > +]) > + > +zone_list=$(ovn-appctl -t ovn-controller ct-zone-list) > + > +check_zones_in_ovsdb "$zone_list" lr0 > +check_zones_in_ovsdb "$zone_list" sw0 > + > +# Check that we did just the initial zone flush > +AT_CHECK([grep -c "NXT_CT_FLUSH_ZONE" hv1/ovs-vswitchd.log], [0], [dnl > +5 > +]) > + > +AS_BOX([Check conversion from UUID - restart]) > +ovn-appctl -t ovn-controller exit --restart > +# Make sure ovn-controller stopped before restarting it > +OVS_WAIT_UNTIL([test "$(ovn-appctl -t ovn-controller debug/status)" != "running"]) > + > +replace_with_uuid lr0 > +replace_with_uuid sw0 > + > +start_daemon ovn-controller --verbose="main:dbg" > + > +OVS_WAIT_UNTIL([test "$(grep -c 'ct zone .* replace uuid name' hv1/ovn-controller.log)" = "8"]) > + > +AT_CHECK([ovn-appctl -t ovn-controller ct-zone-list | sed "s/ [[0-9]]*/ ??/" | sort], [0], [dnl > +lr0_dnat ?? > +lr0_snat ?? > +sw0-port1 ?? > +sw0_dnat ?? > +sw0_snat ?? > +]) > + > +zone_list=$(ovn-appctl -t ovn-controller ct-zone-list) > + > +check_zones_in_ovsdb "$zone_list" lr0 > +check_zones_in_ovsdb "$zone_list" sw0 > + > +# Check that we did just the initial zone flush > +AT_CHECK([grep -c "NXT_CT_FLUSH_ZONE" hv1/ovs-vswitchd.log], [0], [dnl > +5 > +]) > + > +OVN_CLEANUP([hv1]) > +AT_CLEANUP > +]) Acked-by: Dumitru Ceara <dceara@redhat.com>
On Wed, Jul 26, 2023 at 6:21 PM Dumitru Ceara <dceara@redhat.com> wrote: > > On 7/26/23 14:42, Ales Musil wrote: > > There are two scenarios that could cause unwanted > > CT zone flush: > > > > 1) The SB DB is destroyed and recreated. The new > > database will end up with different UUIDs for > > various components. > > > > 2) Upgrade of existing SB DB to ovn-ic. > > The components are the same as before, but scattered > > between multiple SB DBs. This again leads to different > > UUIDs in SB DB. > > > > The CT zone name was based on datapath UUID which > > causes flush when the UUID changes. Even if > > the datapath is the same. > > > > To prevent the unwanted flush migrate from UUID > > to component name (LR/LS name). This allows > > the CT zones to be stable across the before mentioned > > scenarios. > > > > For the migration to be "flush less" itself we need > > to make sure to start the restoration process only > > after controller is connected to the SB DB e.g. the > > restoration can happen only during engine run and not > > init as it was done previously. > > > > Reported-at: https://bugzilla.redhat.com/2224199 > > Tested-by: Surya Seetharaman <sseethar@redhat.com> > > Signed-off-by: Ales Musil <amusil@redhat.com> > > --- > > v2: Address comments from Dumitru. > > Add TODO for the external_ids tracking. > > Simplify the id retrieval in physical.c > > --- > > controller/ovn-controller.c | 106 ++++++++++++++++++++---- > > controller/physical.c | 17 ++-- > > lib/ovn-util.c | 4 +- > > lib/ovn-util.h | 2 +- > > tests/ovn-controller.at | 16 ++-- > > tests/ovn.at | 156 ++++++++++++++++++++++++++++++++++++ > > 6 files changed, 267 insertions(+), 34 deletions(-) > > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index 236974f4f..d34dba75c 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -744,8 +744,17 @@ update_ct_zones(const struct sset *local_lports, > > HMAP_FOR_EACH (ld, hmap_node, local_datapaths) { > > /* XXX Add method to limit zone assignment to logical router > > * datapaths with NAT */ > > - char *dnat = alloc_nat_zone_key(&ld->datapath->header_.uuid, "dnat"); > > - char *snat = alloc_nat_zone_key(&ld->datapath->header_.uuid, "snat"); > > + const char *name = smap_get(&ld->datapath->external_ids, "name"); > > + if (!name) { > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > > + VLOG_ERR_RL(&rl, "Missing name for datapath '"UUID_FMT"' " > > + "skipping zone assignment.", > > + UUID_ARGS(&ld->datapath->header_.uuid)); > > + continue; > > + } > > + > > + char *dnat = alloc_nat_zone_key(name, "dnat"); > > + char *snat = alloc_nat_zone_key(name, "snat"); > > sset_add(&all_users, dnat); > > sset_add(&all_users, snat); > > > > @@ -882,9 +891,66 @@ struct ed_type_ct_zones { > > bool recomputed; > > }; > > > > +/* Replaces a UUID prefix from 'uuid_zone' (if any) with the > > + * corresponding Datapath_Binding.external_ids.name. Returns it > > + * as a new string that that will be owned by the caller. */ > > Typo: "that that". This is my fault.. I had suggested it, sorry. But > maybe it can be fixed up at apply time. > > > +static char * > > +ct_zone_name_from_uuid(const struct sbrec_datapath_binding_table *dp_table, > > + const char *uuid_zone) > > +{ > > + struct uuid uuid; > > + if (!uuid_from_string_prefix(&uuid, uuid_zone)) { > > + return NULL; > > + } > > + > > + const struct sbrec_datapath_binding *dp = > > + sbrec_datapath_binding_table_get_for_uuid(dp_table, &uuid); > > + if (!dp) { > > + return NULL; > > + } > > + > > + const char *entity_name = smap_get(&dp->external_ids, "name"); > > + if (!entity_name) { > > + return NULL; > > + } > > + > > + return xasprintf("%s%s", entity_name, uuid_zone + UUID_LEN); > > +} > > + > > +static void > > +ct_zone_restore(const struct sbrec_datapath_binding_table *dp_table, > > + struct ed_type_ct_zones *ct_zones_data, const char *name, > > + int zone) > > +{ > > + VLOG_DBG("restoring ct zone %"PRId32" for '%s'", zone, name); > > + > > + char *new_name = ct_zone_name_from_uuid(dp_table, name); > > + const char *current_name = name; > > + if (new_name) { > > + VLOG_DBG("ct zone %"PRId32" replace uuid name '%s' with '%s'", > > + zone, name, new_name); > > + > > + /* Make sure we remove the uuid one in the next OvS DB commit without > > + * flush. */ > > + add_pending_ct_zone_entry(&ct_zones_data->pending, CT_ZONE_DB_QUEUED, > > + zone, false, name); > > + /* Store the zone in OvS DB with name instead of uuid without flush. > > + * */ > > + add_pending_ct_zone_entry(&ct_zones_data->pending, CT_ZONE_DB_QUEUED, > > + zone, true, new_name); > > + current_name = new_name; > > + } > > + > > + simap_put(&ct_zones_data->current, current_name, zone); > > + bitmap_set1(ct_zones_data->bitmap, zone); > > + > > + free(new_name); > > +} > > + > > static void > > restore_ct_zones(const struct ovsrec_bridge_table *bridge_table, > > const struct ovsrec_open_vswitch_table *ovs_table, > > + const struct sbrec_datapath_binding_table *dp_table, > > struct ed_type_ct_zones *ct_zones_data) > > { > > memset(ct_zones_data->bitmap, 0, sizeof ct_zones_data->bitmap); > > @@ -895,10 +961,8 @@ restore_ct_zones(const struct ovsrec_bridge_table *bridge_table, > > struct ct_zone_pending_entry *ctpe = pending_node->data; > > > > if (ctpe->add) { > > - VLOG_DBG("restoring ct zone %"PRId32" for '%s'", ctpe->zone, > > - pending_node->name); > > - bitmap_set1(ct_zones_data->bitmap, ctpe->zone); > > - simap_put(&ct_zones_data->current, pending_node->name, ctpe->zone); > > + ct_zone_restore(dp_table, ct_zones_data, > > + pending_node->name, ctpe->zone); > > } > > } > > > > @@ -936,9 +1000,7 @@ restore_ct_zones(const struct ovsrec_bridge_table *bridge_table, > > continue; > > } > > > > - VLOG_DBG("restoring ct zone %"PRId32" for '%s'", zone, user); > > - bitmap_set1(ct_zones_data->bitmap, zone); > > - simap_put(&ct_zones_data->current, user, zone); > > + ct_zone_restore(dp_table, ct_zones_data, user, zone); > > } > > } > > > > @@ -1089,6 +1151,10 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl) > > ovsdb_idl_track_add_column(ovs_idl, > > &ovsrec_flow_sample_collector_set_col_id); > > mirror_register_ovs_idl(ovs_idl); > > + /* XXX: There is a potential bug in CT zone I-P node, > > + * the fact that we have to call recompute for the change of > > + * OVS.bridge.external_ids be reflected. Currently, we don't > > + * track that column which should be addressed in the future. */ > > } > > > > #define SB_NODES \ > > @@ -2416,18 +2482,14 @@ out: > > } > > > > static void * > > -en_ct_zones_init(struct engine_node *node, struct engine_arg *arg OVS_UNUSED) > > +en_ct_zones_init(struct engine_node *node OVS_UNUSED, > > + struct engine_arg *arg OVS_UNUSED) > > { > > struct ed_type_ct_zones *data = xzalloc(sizeof *data); > > - const struct ovsrec_open_vswitch_table *ovs_table = > > - EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node)); > > - const struct ovsrec_bridge_table *bridge_table = > > - EN_OVSDB_GET(engine_get_input("OVS_bridge", node)); > > > > shash_init(&data->pending); > > simap_init(&data->current); > > > > - restore_ct_zones(bridge_table, ovs_table, data); > > return data; > > } > > > > @@ -2458,8 +2520,10 @@ en_ct_zones_run(struct engine_node *node, void *data) > > EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node)); > > const struct ovsrec_bridge_table *bridge_table = > > EN_OVSDB_GET(engine_get_input("OVS_bridge", node)); > > + const struct sbrec_datapath_binding_table *dp_table = > > + EN_OVSDB_GET(engine_get_input("SB_datapath_binding", node)); > > > > - restore_ct_zones(bridge_table, ovs_table, ct_zones_data); > > + restore_ct_zones(bridge_table, ovs_table, dp_table, ct_zones_data); > > update_ct_zones(&rt_data->local_lports, &rt_data->local_datapaths, > > &ct_zones_data->current, ct_zones_data->bitmap, > > &ct_zones_data->pending); > > @@ -2507,7 +2571,15 @@ ct_zones_datapath_binding_handler(struct engine_node *node, void *data) > > /* Check if the requested snat zone has changed for the datapath > > * or not. If so, then fall back to full recompute of > > * ct_zone engine. */ > > - char *snat_dp_zone_key = alloc_nat_zone_key(&dp->header_.uuid, "snat"); > > + const char *name = smap_get(&dp->external_ids, "name"); > > + if (!name) { > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > > + VLOG_ERR_RL(&rl, "Missing name for datapath '"UUID_FMT"' skipping" > > + "zone check.", UUID_ARGS(&dp->header_.uuid)); > > + continue; > > + } > > + > > + char *snat_dp_zone_key = alloc_nat_zone_key(name, "snat"); > > struct simap_node *simap_node = simap_find(&ct_zones_data->current, > > snat_dp_zone_key); > > free(snat_dp_zone_key); > > diff --git a/controller/physical.c b/controller/physical.c > > index edb144b9a..75257bc85 100644 > > --- a/controller/physical.c > > +++ b/controller/physical.c > > @@ -209,17 +209,24 @@ static struct zone_ids > > get_zone_ids(const struct sbrec_port_binding *binding, > > const struct simap *ct_zones) > > { > > - struct zone_ids zone_ids; > > + struct zone_ids zone_ids = { > > + .ct = simap_get(ct_zones, binding->logical_port) > > + }; > > Neat! > > > > > - zone_ids.ct = simap_get(ct_zones, binding->logical_port); > > + const char *name = smap_get(&binding->datapath->external_ids, "name"); > > + if (!name) { > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > > + VLOG_ERR_RL(&rl, "Missing name for datapath '"UUID_FMT"'", > > + UUID_ARGS(&binding->datapath->header_.uuid)); > > > > - const struct uuid *key = &binding->datapath->header_.uuid; > > + return zone_ids; > > + } > > > > - char *dnat = alloc_nat_zone_key(key, "dnat"); > > + char *dnat = alloc_nat_zone_key(name, "dnat"); > > zone_ids.dnat = simap_get(ct_zones, dnat); > > free(dnat); > > > > - char *snat = alloc_nat_zone_key(key, "snat"); > > + char *snat = alloc_nat_zone_key(name, "snat"); > > zone_ids.snat = simap_get(ct_zones, snat); > > free(snat); > > > > diff --git a/lib/ovn-util.c b/lib/ovn-util.c > > index a31788fd9..080ad4c0c 100644 > > --- a/lib/ovn-util.c > > +++ b/lib/ovn-util.c > > @@ -485,9 +485,9 @@ split_addresses(const char *addresses, struct svec *ipv4_addrs, > > * > > * It is the caller's responsibility to free the allocated memory. */ > > char * > > -alloc_nat_zone_key(const struct uuid *key, const char *type) > > +alloc_nat_zone_key(const char *name, const char *type) > > { > > - return xasprintf(UUID_FMT"_%s", UUID_ARGS(key), type); > > + return xasprintf("%s_%s", name, type); > > } > > > > const char * > > diff --git a/lib/ovn-util.h b/lib/ovn-util.h > > index c3c8a21df..5ebdd8adb 100644 > > --- a/lib/ovn-util.h > > +++ b/lib/ovn-util.h > > @@ -118,7 +118,7 @@ const char *find_lport_address(const struct lport_addresses *laddrs, > > void split_addresses(const char *addresses, struct svec *ipv4_addrs, > > struct svec *ipv6_addrs); > > > > -char *alloc_nat_zone_key(const struct uuid *key, const char *type); > > +char *alloc_nat_zone_key(const char *name, const char *type); > > > > const char *default_nb_db(void); > > const char *default_sb_db(void); > > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at > > index 28c13234c..e1b6491b3 100644 > > --- a/tests/ovn-controller.at > > +++ b/tests/ovn-controller.at > > @@ -2446,8 +2446,7 @@ echo "$ct_zones" > > port1_zone=$(get_zone_num "$ct_zones" ls0-hv1) > > port2_zone=$(get_zone_num "$ct_zones" ls0-hv2) > > > > -lr_uuid=$(fetch_column Datapath_Binding _uuid external_ids:name=lr0) > > -snat_zone=$(get_zone_num "$ct_zones" ${lr_uuid}_snat) > > +snat_zone=$(get_zone_num "$ct_zones" lr0_snat) > > echo "snat_zone is $snat_zone" > > > > check test "$port1_zone" -ne "$port2_zone" > > @@ -2456,7 +2455,7 @@ 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 ${lr_uuid}_snat $snat_zone]) > > +OVS_WAIT_UNTIL([check_ovsdb_zone lr0_snat $snat_zone]) > > > > # Now purposely request an SNAT zone for lr0 that conflicts with a zone > > # currently assigned to a logical port > > @@ -2470,7 +2469,7 @@ 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" ${lr_uuid}_snat) > > +snat_zone=$(get_zone_num "$ct_zones" lr0_snat) > > > > check test "$snat_zone" -eq "$snat_req_zone" > > check test "$port1_zone" -ne "$port2_zone" > > @@ -2479,7 +2478,7 @@ 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 ${lr_uuid}_snat $snat_zone]) > > +OVS_WAIT_UNTIL([check_ovsdb_zone lr0_snat $snat_zone]) > > > > # Now create a conflict in the OVSDB and restart ovn-controller. > > > > @@ -2493,7 +2492,7 @@ 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" ${lr_uuid}_snat) > > +snat_zone=$(get_zone_num "$ct_zones" lr0_snat) > > > > check test "$snat_zone" -eq "$snat_req_zone" > > check test "$port1_zone" -ne "$port2_zone" > > @@ -2502,7 +2501,7 @@ 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 ${lr_uuid}_snat $snat_zone]) > > +OVS_WAIT_UNTIL([check_ovsdb_zone lr0_snat $snat_zone]) > > > > OVN_CLEANUP([hv1]) > > AT_CLEANUP > > @@ -2575,9 +2574,8 @@ check ovn-nbctl lrp-set-gateway-chassis lrp-gw hv1 > > > > check ovn-nbctl --wait=hv sync > > > > -lr_uuid=$(fetch_column Datapath_Binding _uuid external_ids:name=lr0) > > ct_zones=$(ovn-appctl -t ovn-controller ct-zone-list) > > -zone_num=$(printf "$ct_zones" | grep ${lr_uuid}_snat | cut -d ' ' -f 2) > > +zone_num=$(printf "$ct_zones" | grep lr0_snat | cut -d ' ' -f 2) > > > > check test "$zone_num" -eq 666 > > > > diff --git a/tests/ovn.at b/tests/ovn.at > > index 24da9174e..3de1444f8 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -36707,3 +36707,159 @@ OVN_CLEANUP([hv1]) > > > > AT_CLEANUP > > ]) > > + > > +OVN_FOR_EACH_NORTHD([ > > +AT_SETUP([Migration of CT zone from UUID to name]) > > +ovn_start > > +net_add n1 > > + > > +sim_add hv1 > > +as hv1 > > +ovs-vsctl add-br br-phys > > +ovn_attach n1 br-phys 192.168.0.1 > > + > > +# sw0-port1 -- sw0 -- lr0 > > + > > +check ovn-nbctl lr-add lr0 > > +check ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 192.168.0.1/24 > > +check ovn-nbctl lrp-add lr0 lr0-sw1 00:00:00:00:ff:02 11.0.0.1/24 > > + > > +check ovn-nbctl ls-add sw0 > > +check ovn-nbctl lsp-add sw0 sw0-port1 > > +check ovn-nbctl lsp-set-addresses sw0-port1 "50:54:00:00:00:01 192.168.0.2" > > + > > +check ovn-nbctl lsp-add sw0 sw0-lr0 > > +check ovn-nbctl lsp-set-type sw0-lr0 router > > +check ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:ff:01 > > +check ovn-nbctl lsp-set-options sw0-lr0 router-port=lr0-sw0 > > + > > +ovs-vsctl add-port br-int p1 -- \ > > + set Interface p1 external_ids:iface-id=sw0-port1 > > + > > +check ovn-appctl -t ovn-controller vlog/set dbg:main > > + > > +wait_for_ports_up > > +ovn-nbctl --wait=hv sync > > + > > +get_zone_num () { > > + output=$1 > > + name=$2 > > + printf "$output" | grep $name | cut -d ' ' -f 2 > > +} > > + > > +replace_with_uuid() { > > + name=$1 > > + > > + id=$(ovn-sbctl --bare --columns _uuid find datapath external_ids:name=${name}) > > + dnat=$(ovs-vsctl --bare get bridge br-int external_ids:ct-zone-${name}_dnat) > > + snat=$(ovs-vsctl --bare get bridge br-int external_ids:ct-zone-${name}_snat) > > + > > + check ovs-vsctl remove bridge br-int external_ids ct-zone-${name}_snat > > + check ovs-vsctl remove bridge br-int external_ids ct-zone-${name}_dnat > > + > > + check ovs-vsctl set bridge br-int external_ids:ct-zone-${id}_snat=$snat > > + check ovs-vsctl set bridge br-int external_ids:ct-zone-${id}_dnat=$dnat > > +} > > + > > +check_zones_in_ovsdb() { > > + zone_list=$1 > > + name=$2 > > + > > + id=$(ovn-sbctl --bare --columns _uuid find datapath external_ids:name=${name}) > > + > > + dnat=$(get_zone_num "$zone_list" ${name}_dnat) > > + snat=$(get_zone_num "$zone_list" ${name}_snat) > > + > > + OVS_WAIT_UNTIL([ovs-vsctl --bare get bridge br-int external_ids:ct-zone-${name}_dnat]) > > + dnat_ovs=$(ovs-vsctl --bare get bridge br-int external_ids:ct-zone-${name}_dnat | tr -d '"') > > + OVS_WAIT_UNTIL([ovs-vsctl --bare get bridge br-int external_ids:ct-zone-${name}_dnat]) > > + snat_ovs=$(ovs-vsctl --bare get bridge br-int external_ids:ct-zone-${name}_snat | tr -d '"') > > + > > + check test "$dnat" = "$dnat_ovs" > > + check test "$snat" = "$snat_ovs" > > + > > + AT_CHECK([ovs-vsctl --bare get bridge br-int external_ids:ct-zone-${id}_dnat], [1], [ignore], [ignore]) > > + AT_CHECK([ovs-vsctl --bare get bridge br-int external_ids:ct-zone-${id}_snat], [1], [ignore], [ignore]) > > +} > > + > > +AS_BOX([Check newly created are with name only]) > > +AT_CHECK([ovn-appctl -t ovn-controller ct-zone-list | sed "s/ [[0-9]]*/ ??/" | sort], [0], [dnl > > +lr0_dnat ?? > > +lr0_snat ?? > > +sw0-port1 ?? > > +sw0_dnat ?? > > +sw0_snat ?? > > +]) > > + > > +zone_list=$(ovn-appctl -t ovn-controller ct-zone-list) > > + > > +check_zones_in_ovsdb "$zone_list" lr0 > > +check_zones_in_ovsdb "$zone_list" sw0 > > + > > +# Check that we did just the initial zone flush > > +AT_CHECK([grep -c "NXT_CT_FLUSH_ZONE" hv1/ovs-vswitchd.log], [0], [dnl > > +5 > > +]) > > + > > +AS_BOX([Check conversion from UUID - recompute]) > > +replace_with_uuid lr0 > > +replace_with_uuid sw0 > > + > > +# XXX: This is potentially a bug, the fact that we have to call > > +# recompute for the change to be reflected. Currently we don't > > +# track the OVS.bridge.external_ids which should be addressed in future. > > +ovn-appctl -t ovn-controller inc-engine/recompute > > + > > +OVS_WAIT_UNTIL([test "$(grep -c 'ct zone .* replace uuid name' hv1/ovn-controller.log)" = "4"]) > > + > > +AT_CHECK([ovn-appctl -t ovn-controller ct-zone-list | sed "s/ [[0-9]]*/ ??/" | sort], [0], [dnl > > +lr0_dnat ?? > > +lr0_snat ?? > > +sw0-port1 ?? > > +sw0_dnat ?? > > +sw0_snat ?? > > +]) > > + > > +zone_list=$(ovn-appctl -t ovn-controller ct-zone-list) > > + > > +check_zones_in_ovsdb "$zone_list" lr0 > > +check_zones_in_ovsdb "$zone_list" sw0 > > + > > +# Check that we did just the initial zone flush > > +AT_CHECK([grep -c "NXT_CT_FLUSH_ZONE" hv1/ovs-vswitchd.log], [0], [dnl > > +5 > > +]) > > + > > +AS_BOX([Check conversion from UUID - restart]) > > +ovn-appctl -t ovn-controller exit --restart > > +# Make sure ovn-controller stopped before restarting it > > +OVS_WAIT_UNTIL([test "$(ovn-appctl -t ovn-controller debug/status)" != "running"]) > > + > > +replace_with_uuid lr0 > > +replace_with_uuid sw0 > > + > > +start_daemon ovn-controller --verbose="main:dbg" > > + > > +OVS_WAIT_UNTIL([test "$(grep -c 'ct zone .* replace uuid name' hv1/ovn-controller.log)" = "8"]) > > + > > +AT_CHECK([ovn-appctl -t ovn-controller ct-zone-list | sed "s/ [[0-9]]*/ ??/" | sort], [0], [dnl > > +lr0_dnat ?? > > +lr0_snat ?? > > +sw0-port1 ?? > > +sw0_dnat ?? > > +sw0_snat ?? > > +]) > > + > > +zone_list=$(ovn-appctl -t ovn-controller ct-zone-list) > > + > > +check_zones_in_ovsdb "$zone_list" lr0 > > +check_zones_in_ovsdb "$zone_list" sw0 > > + > > +# Check that we did just the initial zone flush > > +AT_CHECK([grep -c "NXT_CT_FLUSH_ZONE" hv1/ovs-vswitchd.log], [0], [dnl > > +5 > > +]) > > + > > +OVN_CLEANUP([hv1]) > > +AT_CLEANUP > > +]) > > Acked-by: Dumitru Ceara <dceara@redhat.com> Thanks Ales, Dumitru and Surya. I applied this patch to the main branch after correcting the typo. I'll backport it to branch-23.06 once it passes the tests here - https://github.com/numansiddique/ovn/commit/3581c2887bffb0829e55879c32dd6219e45cdb97 Thanks Numan > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 236974f4f..d34dba75c 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -744,8 +744,17 @@ update_ct_zones(const struct sset *local_lports, HMAP_FOR_EACH (ld, hmap_node, local_datapaths) { /* XXX Add method to limit zone assignment to logical router * datapaths with NAT */ - char *dnat = alloc_nat_zone_key(&ld->datapath->header_.uuid, "dnat"); - char *snat = alloc_nat_zone_key(&ld->datapath->header_.uuid, "snat"); + const char *name = smap_get(&ld->datapath->external_ids, "name"); + if (!name) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); + VLOG_ERR_RL(&rl, "Missing name for datapath '"UUID_FMT"' " + "skipping zone assignment.", + UUID_ARGS(&ld->datapath->header_.uuid)); + continue; + } + + char *dnat = alloc_nat_zone_key(name, "dnat"); + char *snat = alloc_nat_zone_key(name, "snat"); sset_add(&all_users, dnat); sset_add(&all_users, snat); @@ -882,9 +891,66 @@ struct ed_type_ct_zones { bool recomputed; }; +/* Replaces a UUID prefix from 'uuid_zone' (if any) with the + * corresponding Datapath_Binding.external_ids.name. Returns it + * as a new string that that will be owned by the caller. */ +static char * +ct_zone_name_from_uuid(const struct sbrec_datapath_binding_table *dp_table, + const char *uuid_zone) +{ + struct uuid uuid; + if (!uuid_from_string_prefix(&uuid, uuid_zone)) { + return NULL; + } + + const struct sbrec_datapath_binding *dp = + sbrec_datapath_binding_table_get_for_uuid(dp_table, &uuid); + if (!dp) { + return NULL; + } + + const char *entity_name = smap_get(&dp->external_ids, "name"); + if (!entity_name) { + return NULL; + } + + return xasprintf("%s%s", entity_name, uuid_zone + UUID_LEN); +} + +static void +ct_zone_restore(const struct sbrec_datapath_binding_table *dp_table, + struct ed_type_ct_zones *ct_zones_data, const char *name, + int zone) +{ + VLOG_DBG("restoring ct zone %"PRId32" for '%s'", zone, name); + + char *new_name = ct_zone_name_from_uuid(dp_table, name); + const char *current_name = name; + if (new_name) { + VLOG_DBG("ct zone %"PRId32" replace uuid name '%s' with '%s'", + zone, name, new_name); + + /* Make sure we remove the uuid one in the next OvS DB commit without + * flush. */ + add_pending_ct_zone_entry(&ct_zones_data->pending, CT_ZONE_DB_QUEUED, + zone, false, name); + /* Store the zone in OvS DB with name instead of uuid without flush. + * */ + add_pending_ct_zone_entry(&ct_zones_data->pending, CT_ZONE_DB_QUEUED, + zone, true, new_name); + current_name = new_name; + } + + simap_put(&ct_zones_data->current, current_name, zone); + bitmap_set1(ct_zones_data->bitmap, zone); + + free(new_name); +} + static void restore_ct_zones(const struct ovsrec_bridge_table *bridge_table, const struct ovsrec_open_vswitch_table *ovs_table, + const struct sbrec_datapath_binding_table *dp_table, struct ed_type_ct_zones *ct_zones_data) { memset(ct_zones_data->bitmap, 0, sizeof ct_zones_data->bitmap); @@ -895,10 +961,8 @@ restore_ct_zones(const struct ovsrec_bridge_table *bridge_table, struct ct_zone_pending_entry *ctpe = pending_node->data; if (ctpe->add) { - VLOG_DBG("restoring ct zone %"PRId32" for '%s'", ctpe->zone, - pending_node->name); - bitmap_set1(ct_zones_data->bitmap, ctpe->zone); - simap_put(&ct_zones_data->current, pending_node->name, ctpe->zone); + ct_zone_restore(dp_table, ct_zones_data, + pending_node->name, ctpe->zone); } } @@ -936,9 +1000,7 @@ restore_ct_zones(const struct ovsrec_bridge_table *bridge_table, continue; } - VLOG_DBG("restoring ct zone %"PRId32" for '%s'", zone, user); - bitmap_set1(ct_zones_data->bitmap, zone); - simap_put(&ct_zones_data->current, user, zone); + ct_zone_restore(dp_table, ct_zones_data, user, zone); } } @@ -1089,6 +1151,10 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl) ovsdb_idl_track_add_column(ovs_idl, &ovsrec_flow_sample_collector_set_col_id); mirror_register_ovs_idl(ovs_idl); + /* XXX: There is a potential bug in CT zone I-P node, + * the fact that we have to call recompute for the change of + * OVS.bridge.external_ids be reflected. Currently, we don't + * track that column which should be addressed in the future. */ } #define SB_NODES \ @@ -2416,18 +2482,14 @@ out: } static void * -en_ct_zones_init(struct engine_node *node, struct engine_arg *arg OVS_UNUSED) +en_ct_zones_init(struct engine_node *node OVS_UNUSED, + struct engine_arg *arg OVS_UNUSED) { struct ed_type_ct_zones *data = xzalloc(sizeof *data); - const struct ovsrec_open_vswitch_table *ovs_table = - EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node)); - const struct ovsrec_bridge_table *bridge_table = - EN_OVSDB_GET(engine_get_input("OVS_bridge", node)); shash_init(&data->pending); simap_init(&data->current); - restore_ct_zones(bridge_table, ovs_table, data); return data; } @@ -2458,8 +2520,10 @@ en_ct_zones_run(struct engine_node *node, void *data) EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node)); const struct ovsrec_bridge_table *bridge_table = EN_OVSDB_GET(engine_get_input("OVS_bridge", node)); + const struct sbrec_datapath_binding_table *dp_table = + EN_OVSDB_GET(engine_get_input("SB_datapath_binding", node)); - restore_ct_zones(bridge_table, ovs_table, ct_zones_data); + restore_ct_zones(bridge_table, ovs_table, dp_table, ct_zones_data); update_ct_zones(&rt_data->local_lports, &rt_data->local_datapaths, &ct_zones_data->current, ct_zones_data->bitmap, &ct_zones_data->pending); @@ -2507,7 +2571,15 @@ ct_zones_datapath_binding_handler(struct engine_node *node, void *data) /* Check if the requested snat zone has changed for the datapath * or not. If so, then fall back to full recompute of * ct_zone engine. */ - char *snat_dp_zone_key = alloc_nat_zone_key(&dp->header_.uuid, "snat"); + const char *name = smap_get(&dp->external_ids, "name"); + if (!name) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); + VLOG_ERR_RL(&rl, "Missing name for datapath '"UUID_FMT"' skipping" + "zone check.", UUID_ARGS(&dp->header_.uuid)); + continue; + } + + char *snat_dp_zone_key = alloc_nat_zone_key(name, "snat"); struct simap_node *simap_node = simap_find(&ct_zones_data->current, snat_dp_zone_key); free(snat_dp_zone_key); diff --git a/controller/physical.c b/controller/physical.c index edb144b9a..75257bc85 100644 --- a/controller/physical.c +++ b/controller/physical.c @@ -209,17 +209,24 @@ static struct zone_ids get_zone_ids(const struct sbrec_port_binding *binding, const struct simap *ct_zones) { - struct zone_ids zone_ids; + struct zone_ids zone_ids = { + .ct = simap_get(ct_zones, binding->logical_port) + }; - zone_ids.ct = simap_get(ct_zones, binding->logical_port); + const char *name = smap_get(&binding->datapath->external_ids, "name"); + if (!name) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); + VLOG_ERR_RL(&rl, "Missing name for datapath '"UUID_FMT"'", + UUID_ARGS(&binding->datapath->header_.uuid)); - const struct uuid *key = &binding->datapath->header_.uuid; + return zone_ids; + } - char *dnat = alloc_nat_zone_key(key, "dnat"); + char *dnat = alloc_nat_zone_key(name, "dnat"); zone_ids.dnat = simap_get(ct_zones, dnat); free(dnat); - char *snat = alloc_nat_zone_key(key, "snat"); + char *snat = alloc_nat_zone_key(name, "snat"); zone_ids.snat = simap_get(ct_zones, snat); free(snat); diff --git a/lib/ovn-util.c b/lib/ovn-util.c index a31788fd9..080ad4c0c 100644 --- a/lib/ovn-util.c +++ b/lib/ovn-util.c @@ -485,9 +485,9 @@ split_addresses(const char *addresses, struct svec *ipv4_addrs, * * It is the caller's responsibility to free the allocated memory. */ char * -alloc_nat_zone_key(const struct uuid *key, const char *type) +alloc_nat_zone_key(const char *name, const char *type) { - return xasprintf(UUID_FMT"_%s", UUID_ARGS(key), type); + return xasprintf("%s_%s", name, type); } const char * diff --git a/lib/ovn-util.h b/lib/ovn-util.h index c3c8a21df..5ebdd8adb 100644 --- a/lib/ovn-util.h +++ b/lib/ovn-util.h @@ -118,7 +118,7 @@ const char *find_lport_address(const struct lport_addresses *laddrs, void split_addresses(const char *addresses, struct svec *ipv4_addrs, struct svec *ipv6_addrs); -char *alloc_nat_zone_key(const struct uuid *key, const char *type); +char *alloc_nat_zone_key(const char *name, const char *type); const char *default_nb_db(void); const char *default_sb_db(void); diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at index 28c13234c..e1b6491b3 100644 --- a/tests/ovn-controller.at +++ b/tests/ovn-controller.at @@ -2446,8 +2446,7 @@ echo "$ct_zones" port1_zone=$(get_zone_num "$ct_zones" ls0-hv1) port2_zone=$(get_zone_num "$ct_zones" ls0-hv2) -lr_uuid=$(fetch_column Datapath_Binding _uuid external_ids:name=lr0) -snat_zone=$(get_zone_num "$ct_zones" ${lr_uuid}_snat) +snat_zone=$(get_zone_num "$ct_zones" lr0_snat) echo "snat_zone is $snat_zone" check test "$port1_zone" -ne "$port2_zone" @@ -2456,7 +2455,7 @@ 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 ${lr_uuid}_snat $snat_zone]) +OVS_WAIT_UNTIL([check_ovsdb_zone lr0_snat $snat_zone]) # Now purposely request an SNAT zone for lr0 that conflicts with a zone # currently assigned to a logical port @@ -2470,7 +2469,7 @@ 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" ${lr_uuid}_snat) +snat_zone=$(get_zone_num "$ct_zones" lr0_snat) check test "$snat_zone" -eq "$snat_req_zone" check test "$port1_zone" -ne "$port2_zone" @@ -2479,7 +2478,7 @@ 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 ${lr_uuid}_snat $snat_zone]) +OVS_WAIT_UNTIL([check_ovsdb_zone lr0_snat $snat_zone]) # Now create a conflict in the OVSDB and restart ovn-controller. @@ -2493,7 +2492,7 @@ 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" ${lr_uuid}_snat) +snat_zone=$(get_zone_num "$ct_zones" lr0_snat) check test "$snat_zone" -eq "$snat_req_zone" check test "$port1_zone" -ne "$port2_zone" @@ -2502,7 +2501,7 @@ 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 ${lr_uuid}_snat $snat_zone]) +OVS_WAIT_UNTIL([check_ovsdb_zone lr0_snat $snat_zone]) OVN_CLEANUP([hv1]) AT_CLEANUP @@ -2575,9 +2574,8 @@ check ovn-nbctl lrp-set-gateway-chassis lrp-gw hv1 check ovn-nbctl --wait=hv sync -lr_uuid=$(fetch_column Datapath_Binding _uuid external_ids:name=lr0) ct_zones=$(ovn-appctl -t ovn-controller ct-zone-list) -zone_num=$(printf "$ct_zones" | grep ${lr_uuid}_snat | cut -d ' ' -f 2) +zone_num=$(printf "$ct_zones" | grep lr0_snat | cut -d ' ' -f 2) check test "$zone_num" -eq 666 diff --git a/tests/ovn.at b/tests/ovn.at index 24da9174e..3de1444f8 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -36707,3 +36707,159 @@ OVN_CLEANUP([hv1]) AT_CLEANUP ]) + +OVN_FOR_EACH_NORTHD([ +AT_SETUP([Migration of CT zone from UUID to name]) +ovn_start +net_add n1 + +sim_add hv1 +as hv1 +ovs-vsctl add-br br-phys +ovn_attach n1 br-phys 192.168.0.1 + +# sw0-port1 -- sw0 -- lr0 + +check ovn-nbctl lr-add lr0 +check ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 192.168.0.1/24 +check ovn-nbctl lrp-add lr0 lr0-sw1 00:00:00:00:ff:02 11.0.0.1/24 + +check ovn-nbctl ls-add sw0 +check ovn-nbctl lsp-add sw0 sw0-port1 +check ovn-nbctl lsp-set-addresses sw0-port1 "50:54:00:00:00:01 192.168.0.2" + +check ovn-nbctl lsp-add sw0 sw0-lr0 +check ovn-nbctl lsp-set-type sw0-lr0 router +check ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:ff:01 +check ovn-nbctl lsp-set-options sw0-lr0 router-port=lr0-sw0 + +ovs-vsctl add-port br-int p1 -- \ + set Interface p1 external_ids:iface-id=sw0-port1 + +check ovn-appctl -t ovn-controller vlog/set dbg:main + +wait_for_ports_up +ovn-nbctl --wait=hv sync + +get_zone_num () { + output=$1 + name=$2 + printf "$output" | grep $name | cut -d ' ' -f 2 +} + +replace_with_uuid() { + name=$1 + + id=$(ovn-sbctl --bare --columns _uuid find datapath external_ids:name=${name}) + dnat=$(ovs-vsctl --bare get bridge br-int external_ids:ct-zone-${name}_dnat) + snat=$(ovs-vsctl --bare get bridge br-int external_ids:ct-zone-${name}_snat) + + check ovs-vsctl remove bridge br-int external_ids ct-zone-${name}_snat + check ovs-vsctl remove bridge br-int external_ids ct-zone-${name}_dnat + + check ovs-vsctl set bridge br-int external_ids:ct-zone-${id}_snat=$snat + check ovs-vsctl set bridge br-int external_ids:ct-zone-${id}_dnat=$dnat +} + +check_zones_in_ovsdb() { + zone_list=$1 + name=$2 + + id=$(ovn-sbctl --bare --columns _uuid find datapath external_ids:name=${name}) + + dnat=$(get_zone_num "$zone_list" ${name}_dnat) + snat=$(get_zone_num "$zone_list" ${name}_snat) + + OVS_WAIT_UNTIL([ovs-vsctl --bare get bridge br-int external_ids:ct-zone-${name}_dnat]) + dnat_ovs=$(ovs-vsctl --bare get bridge br-int external_ids:ct-zone-${name}_dnat | tr -d '"') + OVS_WAIT_UNTIL([ovs-vsctl --bare get bridge br-int external_ids:ct-zone-${name}_dnat]) + snat_ovs=$(ovs-vsctl --bare get bridge br-int external_ids:ct-zone-${name}_snat | tr -d '"') + + check test "$dnat" = "$dnat_ovs" + check test "$snat" = "$snat_ovs" + + AT_CHECK([ovs-vsctl --bare get bridge br-int external_ids:ct-zone-${id}_dnat], [1], [ignore], [ignore]) + AT_CHECK([ovs-vsctl --bare get bridge br-int external_ids:ct-zone-${id}_snat], [1], [ignore], [ignore]) +} + +AS_BOX([Check newly created are with name only]) +AT_CHECK([ovn-appctl -t ovn-controller ct-zone-list | sed "s/ [[0-9]]*/ ??/" | sort], [0], [dnl +lr0_dnat ?? +lr0_snat ?? +sw0-port1 ?? +sw0_dnat ?? +sw0_snat ?? +]) + +zone_list=$(ovn-appctl -t ovn-controller ct-zone-list) + +check_zones_in_ovsdb "$zone_list" lr0 +check_zones_in_ovsdb "$zone_list" sw0 + +# Check that we did just the initial zone flush +AT_CHECK([grep -c "NXT_CT_FLUSH_ZONE" hv1/ovs-vswitchd.log], [0], [dnl +5 +]) + +AS_BOX([Check conversion from UUID - recompute]) +replace_with_uuid lr0 +replace_with_uuid sw0 + +# XXX: This is potentially a bug, the fact that we have to call +# recompute for the change to be reflected. Currently we don't +# track the OVS.bridge.external_ids which should be addressed in future. +ovn-appctl -t ovn-controller inc-engine/recompute + +OVS_WAIT_UNTIL([test "$(grep -c 'ct zone .* replace uuid name' hv1/ovn-controller.log)" = "4"]) + +AT_CHECK([ovn-appctl -t ovn-controller ct-zone-list | sed "s/ [[0-9]]*/ ??/" | sort], [0], [dnl +lr0_dnat ?? +lr0_snat ?? +sw0-port1 ?? +sw0_dnat ?? +sw0_snat ?? +]) + +zone_list=$(ovn-appctl -t ovn-controller ct-zone-list) + +check_zones_in_ovsdb "$zone_list" lr0 +check_zones_in_ovsdb "$zone_list" sw0 + +# Check that we did just the initial zone flush +AT_CHECK([grep -c "NXT_CT_FLUSH_ZONE" hv1/ovs-vswitchd.log], [0], [dnl +5 +]) + +AS_BOX([Check conversion from UUID - restart]) +ovn-appctl -t ovn-controller exit --restart +# Make sure ovn-controller stopped before restarting it +OVS_WAIT_UNTIL([test "$(ovn-appctl -t ovn-controller debug/status)" != "running"]) + +replace_with_uuid lr0 +replace_with_uuid sw0 + +start_daemon ovn-controller --verbose="main:dbg" + +OVS_WAIT_UNTIL([test "$(grep -c 'ct zone .* replace uuid name' hv1/ovn-controller.log)" = "8"]) + +AT_CHECK([ovn-appctl -t ovn-controller ct-zone-list | sed "s/ [[0-9]]*/ ??/" | sort], [0], [dnl +lr0_dnat ?? +lr0_snat ?? +sw0-port1 ?? +sw0_dnat ?? +sw0_snat ?? +]) + +zone_list=$(ovn-appctl -t ovn-controller ct-zone-list) + +check_zones_in_ovsdb "$zone_list" lr0 +check_zones_in_ovsdb "$zone_list" sw0 + +# Check that we did just the initial zone flush +AT_CHECK([grep -c "NXT_CT_FLUSH_ZONE" hv1/ovs-vswitchd.log], [0], [dnl +5 +]) + +OVN_CLEANUP([hv1]) +AT_CLEANUP +])