Hi Ales, On 3/16/23 19:25, Ales Musil wrote: > The CT flush was enabled by default for every LB, add > config option called "ct_flush_enabled" that allows > users to enable/disable the CT flush. The CT flush is > remaining enabled by default. > > Reported-at: https://bugzilla.redhat.com/2178962
As mentioned in the bug report above: "OVN has started flushing conntrack entries for services (vips->backends) whenever the 5 tuple changes (so either LB is deleted OR VIP, VIP port, backend IP, backend port, proto changes).". I'd like to avoid this kind of problems in the future by making the feature opt-in, so disabled by default. I know that it's a bit of a sensitive subject because the feature was already enabled by default when it was added in v23.03.0 so in theory there might be people using it already. However, given that (AFAIK) nobody else except ovn-kubernetes complained about the lack of "CT flush on backend removal" before v23.03.0 it's probably safe to assume that disabling the feature by default doesn't affect other users. In any case, because ovn-kubernetes is only interested in flushing UDP entries and not other protocols (*), they'll have to change their code anyway to either opt-out for some LBs or opt-in for the rest, depending on the default we choose. Another reason to make it opt-in is to avoid issues due to OVN upgrading to a version that has flush enabled by default for all LB while the CMS code is still unaware of the new option and thus cannot configure OVN to (partially) disable flushing. Regarding the code I have a few minor comments below. Thanks, Dumitru (*) whether that's the right thing to do is not really relevant I guess. > Signed-off-by: Ales Musil <[email protected]> > --- > NEWS | 2 ++ > controller/ovn-controller.c | 6 ++++-- > ovn-nb.xml | 7 +++++++ > tests/ovn.at | 20 ++++++++++++++++++++ > tests/system-ovn.at | 33 ++++++++++++++++++++++++++++----- > 5 files changed, 61 insertions(+), 7 deletions(-) > > diff --git a/NEWS b/NEWS > index 637adcff3..0daac951a 100644 > --- a/NEWS > +++ b/NEWS > @@ -2,6 +2,8 @@ Post v23.03.0 > ------------- > - Enhance LSP.options:arp_proxy to support IPv6, configurable MAC > addresses and CIDRs. > + - Add an option for LBs called "ct_flush_enabled" that allows CMS to > specify > + if ovn-controller should flush related CT entries for removed LB > backends. > > OVN v23.03.0 - 03 Mar 2023 > -------------------------- > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 7dcbfd252..8b85464c6 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -2697,7 +2697,8 @@ static void > lb_data_removed_five_tuples_add(struct ed_type_lb_data *lb_data, > const struct ovn_controller_lb *lb) > { > - if (!ovs_feature_is_supported(OVS_CT_TUPLE_FLUSH_SUPPORT)) { > + if (!ovs_feature_is_supported(OVS_CT_TUPLE_FLUSH_SUPPORT) || > + !smap_get_bool(&lb->slb->options, "ct_flush_enabled", true)) { Let's store this as a field in 'struct ovn_controller_lb' so we don't lookup in the smap every time we need this info. In some cases (since we incrementally process the LBs) we already have the lb structure in memory. > return; > } > > @@ -2716,7 +2717,8 @@ static void > lb_data_removed_five_tuples_remove(struct ed_type_lb_data *lb_data, > const struct ovn_controller_lb *lb) > { > - if (!ovs_feature_is_supported(OVS_CT_TUPLE_FLUSH_SUPPORT)) { > + if (!ovs_feature_is_supported(OVS_CT_TUPLE_FLUSH_SUPPORT) || > + !smap_get_bool(&lb->slb->options, "ct_flush_enabled", true)) { > return; > } > > diff --git a/ovn-nb.xml b/ovn-nb.xml > index 73f707aa0..c5dbebd1d 100644 > --- a/ovn-nb.xml > +++ b/ovn-nb.xml > @@ -2041,6 +2041,13 @@ or > the affinity timeslot. Max supported affinity_timeout is 65535 > seconds. > </column> > + > + <column name="options" key="ct_flush_enabled" > + type='{"type": "boolean"}'> As Ilya mentioned, _enabled is probably redundant. I wonder though whether we need a different name. I'm not sure if it's better to be honest but what do you think of "graceful_cleanup". In the end what we achieve with ct_flush_enabled=false is "graceful termination" of the connection. > + The value indicates whether ovn-controller should flush CT entries > + that are related to this LB when the backends are removed. Being set > + to <code>true</code> by default. > + </column> > </group> > </table> > > diff --git a/tests/ovn.at b/tests/ovn.at > index fa786112c..724e8b6e5 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -35039,6 +35039,26 @@ AT_CHECK([grep -q "Flushing CT for 5-tuple: > vip=192.168.50.10:80, backend=192.16 > > AT_CHECK([test "$(grep -c "Flushing CT for 5-tuple" hv1/ovn-controller.log)" > = "6"], [0]) > > +# Check if disable of the CT flush works > +check ovn-nbctl lb-del lb1 > +check ovn-nbctl lb-add lb1 "192.168.70.10:80" > "192.168.80.10:8080,192.168.90.10:8080" > +check ovn-nbctl set load_balancer lb1 options:ct_flush_enabled="false" > +check ovn-nbctl ls-lb-add sw lb1 > +check ovs-vsctl set interface p1 external_ids:iface-id=lsp1 > +check ovn-nbctl --wait=hv sync > + > +#AT_CHECK([test "$(grep -c "Flushing CT for 5-tuple" > hv1/ovn-controller.log)" = "6"], [0]) > + > +# Remove one backend > +check ovn-nbctl --wait=hv set load_balancer lb1 > vips='"192.168.70.10:80"="192.168.80.10:8080"' > + > +#AT_CHECK([grep -q "Flushing CT for 5-tuple: vip=192.168.70.10:80, > backend=192.168.90.10:8080, protocol=6" hv1/ovn-controller.log], [1]) > +#AT_CHECK([test "$(grep -c "Flushing CT for 5-tuple" > hv1/ovn-controller.log)" = "6"], [0]) > + > +check ovn-nbctl --wait=hv lb-del lb1 > +AT_CHECK([grep -q "Flushing CT for 5-tuple: vip=192.168.70.10:80, > backend=192.168.80.10:8080, protocol=6" hv1/ovn-controller.log], [1]) > +AT_CHECK([test "$(grep -c "Flushing CT for 5-tuple" hv1/ovn-controller.log)" > = "6"], [0]) > + > OVN_CLEANUP([hv1]) > AT_CLEANUP > ]) > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > index ad1188078..afd10c15d 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -10013,16 +10013,18 @@ OVS_START_L7([bar1], [http]) > OVS_START_L7([bar2], [http]) > OVS_START_L7([bar3], [http]) > > -OVS_WAIT_FOR_OUTPUT([ > - for i in `seq 1 20`; do > - ip netns exec foo1 wget 30.0.0.1 -t 5 -T 1 --retry-connrefused -v -o > wget$i.log; > - done > - ovs-appctl dpctl/dump-conntrack | FORMAT_CT(30.0.0.1) | sed -e > 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl > +m4_define([LB1_CT_ENTRIES], [dnl > > tcp,orig=(src=192.168.1.2,dst=30.0.0.1,sport=<cleared>,dport=<cleared>),reply=(src=172.16.1.2,dst=192.168.1.2,sport=<cleared>,dport=<cleared>),zone=<cleared>,mark=2,protoinfo=(state=<cleared>) > > tcp,orig=(src=192.168.1.2,dst=30.0.0.1,sport=<cleared>,dport=<cleared>),reply=(src=172.16.1.3,dst=192.168.1.2,sport=<cleared>,dport=<cleared>),zone=<cleared>,mark=2,protoinfo=(state=<cleared>) > > tcp,orig=(src=192.168.1.2,dst=30.0.0.1,sport=<cleared>,dport=<cleared>),reply=(src=172.16.1.4,dst=192.168.1.2,sport=<cleared>,dport=<cleared>),zone=<cleared>,mark=2,protoinfo=(state=<cleared>) > ]) > > +OVS_WAIT_FOR_OUTPUT([ > + for i in `seq 1 20`; do > + ip netns exec foo1 wget 30.0.0.1 -t 5 -T 1 --retry-connrefused -v -o > wget$i.log; > + done > + ovs-appctl dpctl/dump-conntrack | FORMAT_CT(30.0.0.1) | sed -e > 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [LB1_CT_ENTRIES]) > + > OVS_WAIT_FOR_OUTPUT([ > for i in `seq 1 20`; do > ip netns exec foo1 wget 30.0.0.2:8000 -t 5 -T 1 --retry-connrefused > -v -o wget$i.log; > @@ -10096,6 +10098,27 @@ check ovn-nbctl lb-del lb2 > > OVS_WAIT_UNTIL([test "$(ovs-appctl dpctl/dump-conntrack | > FORMAT_CT(30.0.0.3) | wc -l)" = "0"]) > > +# Config OVN wih disabled CT flush. > +check ovn-nbctl lb-add lb1 30.0.0.1 "172.16.1.2,172.16.1.3,172.16.1.4" \ > +-- set load_balancer lb1 options:ct_flush_enabled="false" > +check ovn-nbctl ls-lb-add foo lb1 > + > +OVS_WAIT_FOR_OUTPUT([ > + for i in `seq 1 20`; do > + ip netns exec foo1 wget 30.0.0.1 -t 5 -T 1 --retry-connrefused -v -o > wget$i.log; > + done > + ovs-appctl dpctl/dump-conntrack | FORMAT_CT(30.0.0.1) | sed -e > 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [LB1_CT_ENTRIES]) > + > +# Remove one backend > +check ovn-nbctl --wait=hv set load_balancer lb1 > vips='"30.0.0.1"="172.16.1.2,172.16.1.3"' > + > +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(30.0.0.1) | sed -e > 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [LB1_CT_ENTRIES]) > + > +# Remove whole LB > +check ovn-nbctl --wait=hv lb-del lb1 > + > +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(30.0.0.1) | sed -e > 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [LB1_CT_ENTRIES]) > + > OVS_APP_EXIT_AND_WAIT([ovn-controller]) > > as ovn-sb _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
