diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 236974f4f3..1bba7715d7 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 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 @@ port_groups_runtime_data_handler(struct engine_node *node, void *data) } 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 edb144b9a1..75257bc854 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 a31788fd99..080ad4c0ce 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 c3c8a21df6..5ebdd8adb0 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 28c13234ca..e1b6491b39 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 24da9174e6..3de1444f81 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 +])