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 <sseet...@redhat.com>
> Signed-off-by: Ales Musil <amu...@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 <dce...@redhat.com>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
