On Wed, Jan 18, 2023 at 2:31 PM Mark Michelson <[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 > Signed-off-by: Mark Michelson <[email protected]> > --- > controller/ovn-controller.c | 5 +++++ > tests/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 b/tests/ovn-controller.at > index 1daf3ae4b..bbe142ae3 100644 > --- a/tests/ovn-controller.at > +++ b/tests/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] > 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]> -- 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
