On 1/20/23 02:16, Ales Musil wrote:
On Wed, Jan 18, 2023 at 2:31 PM Mark Michelson <[email protected] <mailto:[email protected]>> wrote:update_ct_zones() attempts to give preference to requested SNAT zones over auto-assigned CT zones. However, there is a subtle bug. Requested SNAT zones are only assigned if there is an auto-assigned CT zone to replace. If a logical router is created, and its requested SNAT zone is assigned in the same transaction, then that logical router will not yet have an auto-assigned CT zone on it. Therefore, the requested SNAT zone has no CT zone to replace, and so the logical router is auto-assigned a zone. This bug is somewhat hidden. The next time a recompute is needed in ovn-controller, then the requested SNAT zone will replace the auto-assigned zone. This is why none of our tests caught this issue. Our tests always created the logical router in one transaction, then set the requested SNAT zone later. The initial transaction would auto-assign an SNAT zone for the logical router, and when we set the requested zone later, a recompute would properly replace the auto-assigned zone with the requested zone. This change fixes the issue by setting the requested zone no matter if the logical router already has an auto-assigned zone. A simple test has been added to ensure that the behavior works as expected. Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2160403 <https://bugzilla.redhat.com/show_bug.cgi?id=2160403> Signed-off-by: Mark Michelson <[email protected] <mailto:[email protected]>> --- controller/ovn-controller.c | 5 +++++ tests/ovn-controller.at <http://ovn-controller.at> | 27 +++++++++++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 351cf1c5b..265740cab 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -798,6 +798,11 @@ update_ct_zones(const struct shash *binding_lports, } bitmap_set1(ct_zone_bitmap, snat_req_node->data); node->data = snat_req_node->data; + } else { + 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); } } diff --git a/tests/ovn-controller.at <http://ovn-controller.at> b/tests/ovn-controller.at <http://ovn-controller.at> index 1daf3ae4b..bbe142ae3 100644 --- a/tests/ovn-controller.at <http://ovn-controller.at> +++ b/tests/ovn-controller.at <http://ovn-controller.at> @@ -2500,3 +2500,30 @@ AT_CHECK([GET_LOCAL_TEMPLATE_VARS], [1], []) AT_CLEANUP ]) + +OVN_FOR_EACH_NORTHD([ +AT_SETUP([ovn-controller - Requested SNAT Zone in router creation transaction]) +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 + +dnl This is key. Add the snat-ct-zone when creating the logical router and then +dnl do not make any further changes to the logical router settings. +check ovn-nbctl lr-add lr0 -- set Logical_Router lr0 options:snat-ct-zone=666 +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 + +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) + +check test "$zone_num" -eq 666 + +AT_CLEANUP +])-- 2.38.1_______________________________________________ dev mailing list [email protected] <mailto:[email protected]> https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> Looks good to me, thanks. With the issue pointed out by bot being address during merge: Acked-by: Ales Musil <[email protected] <mailto:[email protected]>>
Thanks Ales, I corrected the problem and pushed the change to main, branch-22.12, branch-22.09, branch-22.06, and branch-22.03.
-- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] <mailto:[email protected]> IM: amusil <https://red.ht/sig>
_______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
