On Wed, Nov 9, 2022 at 1:42 PM Mark Michelson <[email protected]> wrote:
> An issue was filed where CT zone 0 was assigned to both a logical router > SNAT and to a logical port. CT zone 0 is typically "reserved" and not > assigned by ovn-controller; however, since SNAT zones are configurable, > it is possible for ovn-controller to assign this zone at the CMS's > request. This accounts for how CT zone 0 can be assigned for SNAT. There > was also a small bug in the incremental processing that could result in > a logical port being assigned zone 0. > > In the specific issue report, CT zones were restored from the OVSDB when > ovn-controller started, and the conflicting CT zones were already > present. ovn-controller dutifully loaded these zones up. But then there > was nothing that would allow for the conflict to be resolved afterwards. > > It is unknown how these conflicts entered into the OVSDB in the first > place. This change does not purport to prevent conflicts from entering > the OVSDB. However, it does make the following changes that should > further safeguard against unwanted behavior: > > * ct_zones_runtime_data_handler() now assigns zones starting at > 1 instead of 0. This makes it use the same range as update_ct_zones(). > * update_ct_zones() now keeps a new simap for zones that are assigned > but not due to an SNAT zone request. This allows for us to guarantee > that when there is a conflict between a previously auto-assigned CT > zone and a newly-requested zone, we are guaranteed to remove the > auto-assigned CT zone. > * The removal of conflicting auto-assigned CT zones is now performed > before dealing with the newly requested zone. This makes it so that if > we load conflicting zones from the OVSDB or if there is some issue > that results in conflicting zones being assigned, we will correct the > issue. > * CT zones are restored from the OVSDB on every recompute. This helps in > the case that CT zone conflicts are introduced directly into the > OVSDB. > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2126406 > Signed-off-by: Mark Michelson <[email protected]> > Acked-by: Numan Siddique <[email protected]> > Signed-off-by: Mark Michelson <[email protected]> > --- > v3 -> v4 > Addressed Numan's request to resolve conflicts in the OVSDB during a > recompute. The test case is updated so that instead of an ovn-controller > restart, we just perform a recompute to resolve the conflict. > --- > controller/ovn-controller.c | 110 +++++++++++++++++++++-------------- > tests/ovn-controller.at | 112 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 178 insertions(+), 44 deletions(-) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 8895c7a2b..7dd83e7f4 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -659,7 +659,8 @@ update_ct_zones(const struct shash *binding_lports, > const char *user; > struct sset all_users = SSET_INITIALIZER(&all_users); > struct simap req_snat_zones = SIMAP_INITIALIZER(&req_snat_zones); > - unsigned long unreq_snat_zones[BITMAP_N_LONGS(MAX_CT_ZONES)]; > + unsigned long unreq_snat_zones_map[BITMAP_N_LONGS(MAX_CT_ZONES)]; > + struct simap unreq_snat_zones = SIMAP_INITIALIZER(&unreq_snat_zones); > > struct shash_node *shash_node; > SHASH_FOR_EACH (shash_node, binding_lports) { > @@ -696,49 +697,46 @@ update_ct_zones(const struct shash *binding_lports, > bitmap_set0(ct_zone_bitmap, ct_zone->data); > simap_delete(ct_zones, ct_zone); > } else if (!simap_find(&req_snat_zones, ct_zone->name)) { > - bitmap_set1(unreq_snat_zones, ct_zone->data); > + bitmap_set1(unreq_snat_zones_map, ct_zone->data); > + simap_put(&unreq_snat_zones, ct_zone->name, ct_zone->data); > } > } > > /* Prioritize requested CT zones */ > struct simap_node *snat_req_node; > SIMAP_FOR_EACH (snat_req_node, &req_snat_zones) { > - struct simap_node *node = simap_find(ct_zones, > snat_req_node->name); > - if (node) { > - if (node->data == snat_req_node->data) { > - /* No change to this request, so no action needed */ > - continue; > - } else { > - /* Zone request has changed for this node. delete old > entry */ > - bitmap_set0(ct_zone_bitmap, node->data); > - simap_delete(ct_zones, node); > - } > - } > - > /* Determine if someone already had this zone auto-assigned. > * If so, then they need to give up their assignment since > * that zone is being explicitly requested now. > */ > - if (bitmap_is_set(unreq_snat_zones, snat_req_node->data)) { > - struct simap_node *dup; > - SIMAP_FOR_EACH_SAFE (dup, ct_zones) { > - if (dup != snat_req_node && dup->data == > snat_req_node->data) { > - simap_delete(ct_zones, dup); > - break; > + if (bitmap_is_set(unreq_snat_zones_map, snat_req_node->data)) { > + struct simap_node *unreq_node; > + SIMAP_FOR_EACH_SAFE (unreq_node, &unreq_snat_zones) { > + if (unreq_node->data == snat_req_node->data) { > + simap_find_and_delete(ct_zones, unreq_node->name); > + simap_delete(&unreq_snat_zones, unreq_node); > } > } > + > /* Set this bit to 0 so that if multiple datapaths have > requested > * this zone, we don't needlessly double-detect this > condition. > */ > - bitmap_set0(unreq_snat_zones, snat_req_node->data); > + bitmap_set0(unreq_snat_zones_map, snat_req_node->data); > } > > - add_pending_ct_zone_entry(pending_ct_zones, CT_ZONE_OF_QUEUED, > - snat_req_node->data, true, > - snat_req_node->name); > - > - bitmap_set1(ct_zone_bitmap, snat_req_node->data); > - simap_put(ct_zones, snat_req_node->name, snat_req_node->data); > + struct simap_node *node = simap_find(ct_zones, > snat_req_node->name); > + if (node) { > + if (node->data != snat_req_node->data) { > + /* Zone request has changed for this node. delete old > entry and > + * create new one*/ > + add_pending_ct_zone_entry(pending_ct_zones, > CT_ZONE_OF_QUEUED, > + snat_req_node->data, true, > + snat_req_node->name); > + bitmap_set0(ct_zone_bitmap, node->data); > + } > + bitmap_set1(ct_zone_bitmap, snat_req_node->data); > + node->data = snat_req_node->data; > + } > } > > /* xxx This is wasteful to assign a zone to each port--even if no > @@ -756,6 +754,7 @@ update_ct_zones(const struct shash *binding_lports, > } > > simap_destroy(&req_snat_zones); > + simap_destroy(&unreq_snat_zones); > sset_destroy(&all_users); > } > > @@ -796,11 +795,36 @@ commit_ct_zones(const struct ovsrec_bridge *br_int, > } > } > > +/* Connection tracking zones. */ > +struct ed_type_ct_zones { > + unsigned long bitmap[BITMAP_N_LONGS(MAX_CT_ZONES)]; > + struct shash pending; > + struct simap current; > + > + /* Tracked data. */ > + bool recomputed; > +}; > + > static void > restore_ct_zones(const struct ovsrec_bridge_table *bridge_table, > const struct ovsrec_open_vswitch_table *ovs_table, > - struct simap *ct_zones, unsigned long *ct_zone_bitmap) > + struct ed_type_ct_zones *ct_zones_data) > { > + memset(ct_zones_data->bitmap, 0, sizeof ct_zones_data->bitmap); > + bitmap_set1(ct_zones_data->bitmap, 0); /* Zone 0 is reserved. */ > + > + struct shash_node *pending_node; > + SHASH_FOR_EACH (pending_node, &ct_zones_data->pending) { > + 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); > + } > + } > + > const struct ovsrec_open_vswitch *cfg; > cfg = ovsrec_open_vswitch_table_first(ovs_table); > if (!cfg) { > @@ -826,14 +850,18 @@ restore_ct_zones(const struct ovsrec_bridge_table > *bridge_table, > continue; > } > > + if (shash_find(&ct_zones_data->pending, user)) { > + continue; > + } > + > unsigned int zone; > if (!str_to_uint(node->value, 10, &zone)) { > continue; > } > > VLOG_DBG("restoring ct zone %"PRId32" for '%s'", zone, user); > - bitmap_set1(ct_zone_bitmap, zone); > - simap_put(ct_zones, user, zone); > + bitmap_set1(ct_zones_data->bitmap, zone); > + simap_put(&ct_zones_data->current, user, zone); > } > } > > @@ -2051,16 +2079,6 @@ out: > return true; > } > > -/* Connection tracking zones. */ > -struct ed_type_ct_zones { > - unsigned long bitmap[BITMAP_N_LONGS(MAX_CT_ZONES)]; > - struct shash pending; > - struct simap current; > - > - /* Tracked data. */ > - bool recomputed; > -}; > - > static void * > en_ct_zones_init(struct engine_node *node, struct engine_arg *arg > OVS_UNUSED) > { > @@ -2073,9 +2091,7 @@ en_ct_zones_init(struct engine_node *node, struct > engine_arg *arg OVS_UNUSED) > shash_init(&data->pending); > simap_init(&data->current); > > - memset(data->bitmap, 0, sizeof data->bitmap); > - bitmap_set1(data->bitmap, 0); /* Zone 0 is reserved. */ > - restore_ct_zones(bridge_table, ovs_table, &data->current, > data->bitmap); > + restore_ct_zones(bridge_table, ovs_table, data); > return data; > } > > @@ -2102,6 +2118,12 @@ en_ct_zones_run(struct engine_node *node, void > *data) > struct ed_type_runtime_data *rt_data = > engine_get_input_data("runtime_data", node); > > + 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)); > + > + restore_ct_zones(bridge_table, ovs_table, ct_zones_data); > update_ct_zones(&rt_data->lbinding_data.lports, > &rt_data->local_datapaths, > &ct_zones_data->current, ct_zones_data->bitmap, > &ct_zones_data->pending); > @@ -2178,7 +2200,7 @@ ct_zones_runtime_data_handler(struct engine_node > *node, void *data) > > struct hmap *tracked_dp_bindings = &rt_data->tracked_dp_bindings; > struct tracked_datapath *tdp; > - int scan_start = 0; > + int scan_start = 1; > > bool updated = false; > > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at > index 3c3fb31c7..6a0e83c33 100644 > --- a/tests/ovn-controller.at > +++ b/tests/ovn-controller.at > @@ -2337,3 +2337,115 @@ done > AT_CHECK([grep "deleted interface patch" hv1/ovs-vswitchd.log], [1], > [ignore]) > OVN_CLEANUP([hv1]) > AT_CLEANUP > + > +AT_SETUP([ovn-controller - resolve CT zone conflicts from ovsdb]) > + > +ovn_start > + > +net_add n1 > +sim_add hv1 > +as hv1 > +check ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.1 > + > +get_zone_num () { > + output=$1 > + name=$2 > + printf "$output" | grep $name | cut -d ' ' -f 2 > +} > + > +check_ovsdb_zone() { > + name=$1 > + ct_zone=$2 > + db_zone=$(ovs-vsctl get Bridge br-int external_ids:ct-zone-${name} | > sed -e 's/^"//' -e 's/"$//') > + test $ct_zone -eq $db_zone > +} > + > +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 ovn-nbctl lr-add lr0 > + > +check ovn-nbctl ls-add ls0 > +check ovn-nbctl lsp-add ls0 ls0-lr0 > +check ovn-nbctl lsp-set-type ls0-lr0 router > +check ovn-nbctl lsp-set-addresses ls0-lr0 router > +check ovn-nbctl lrp-add lr0 lr0-ls0 00:00:00:00:00:01 10.0.0.1 > + > +check ovn-nbctl lsp-add ls0 ls0-hv1 > +check ovn-nbctl lsp-set-addresses ls0-hv1 "00:00:00:00:00:02 10.0.0.2" > + > +check ovn-nbctl lsp-add ls0 ls0-hv2 > +check ovn-nbctl lsp-set-addresses ls0-hv2 "00:00:00:00:00:03 10.0.0.3" > + > +check ovn-nbctl lrp-add lr0 lrp-gw 01:00:00:00:00:01 172.16.0.1 > +check ovn-nbctl lrp-set-gateway-chassis lrp-gw hv1 > + > +check ovn-nbctl --wait=hv sync > + > +ct_zones=$(ovn-appctl -t ovn-controller ct-zone-list) > +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) > +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 ${lr_uuid}_snat $snat_zone]) > + > +# Now purposely request an SNAT zone for lr0 that conflicts with a zone > +# currently assigned to a logical port > + > +snat_req_zone=$port1_zone > +check ovn-nbctl set Logical_Router lr0 options:snat-ct-zone=$snat_req_zone > +ovn-nbctl --wait=hv sync > + > +ct_zones=$(ovn-appctl -t ovn-controller ct-zone-list) > +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) > + > +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" > + > +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]) > + > +# Now create a conflict in the OVSDB and restart ovn-controller. > + > +ovs-vsctl set bridge br-int external_ids:ct-zone-ls0-hv1="$snat_req_zone" > +ovs-vsctl set bridge br-int external_ids:ct-zone-ls0-hv2="$snat_req_zone" > + > +ovn-appctl -t ovn-controller inc-engine/recompute > + > +ct_zones=$(ovn-appctl -t ovn-controller ct-zone-list) > +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) > + > +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" > + > +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]) > + > +OVN_CLEANUP([hv1]) > +AT_CLEANUP > -- > 2.38.1 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Hi Mark, 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] IM: amusil <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
