Message ID | 20230320105313.49650-1-amusil@redhat.com |
---|---|
State | Accepted |
Headers | show |
Series | [ovs-dev,v4] controller: Add config option per LB to enable/disable CT flush | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | success | apply and check: success |
ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On 3/20/23 11:53, Ales Musil wrote: > The CT flush was enabled by default for every LB, add > config option called "ct_flush" that allows > users to enable/disable the CT flush. The CT flush option > is set to "false" by default. > > Reported-at: https://bugzilla.redhat.com/2178962 > Acked-by: Numan Siddique <numans@ovn.org> > Signed-off-by: Ales Musil <amusil@redhat.com> > --- > v2: Make the feature opt-in. > Store the flag in 'struct ovn_controller_lb'. > v3: Update the NEWS. > Address nits from Dumitru. > v4: Update the documentation to be more accurate. > --- > NEWS | 4 ++++ > controller/ovn-controller.c | 6 ++++-- > lib/lb.c | 1 + > lib/lb.h | 1 + > ovn-nb.xml | 8 ++++++++ > tests/ovn.at | 28 +++++++++++++++++++++++++--- > tests/system-ovn.at | 36 ++++++++++++++++++++++++++++++------ > 7 files changed, 73 insertions(+), 11 deletions(-) > > diff --git a/NEWS b/NEWS > index 637adcff3..1298d16a2 100644 > --- a/NEWS > +++ b/NEWS > @@ -2,6 +2,10 @@ 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" that allows CMS to specify > + if ovn-controller should flush related CT entries for removed LB backends. > + By default, this option is set to false, i.e., CT entries are not flushed > + when load balancer backends are removed. I think, the NEWS entry should be more clear that default behavior has changed. Especially if we're planning to backport this to a stable branch, it should be obvious that feature was taken away. Current entry reads as if we just added a new opt-in feature. It's probably not necessary to re-spin the patch again for this, if we can agree on a better wording in this thread. But I'll leave this up to maintainers. Best regards, Ilya Maximets.
Hi Ales, I agree with Ilya about updating the NEWS wording. I also think this is something that a committer can take care of when committing the change. Acked-by: Mark Michelson <mmichels@redhat.com> On 3/20/23 08:00, Ilya Maximets wrote: > On 3/20/23 11:53, Ales Musil wrote: >> The CT flush was enabled by default for every LB, add >> config option called "ct_flush" that allows >> users to enable/disable the CT flush. The CT flush option >> is set to "false" by default. >> >> Reported-at: https://bugzilla.redhat.com/2178962 >> Acked-by: Numan Siddique <numans@ovn.org> >> Signed-off-by: Ales Musil <amusil@redhat.com> >> --- >> v2: Make the feature opt-in. >> Store the flag in 'struct ovn_controller_lb'. >> v3: Update the NEWS. >> Address nits from Dumitru. >> v4: Update the documentation to be more accurate. >> --- >> NEWS | 4 ++++ >> controller/ovn-controller.c | 6 ++++-- >> lib/lb.c | 1 + >> lib/lb.h | 1 + >> ovn-nb.xml | 8 ++++++++ >> tests/ovn.at | 28 +++++++++++++++++++++++++--- >> tests/system-ovn.at | 36 ++++++++++++++++++++++++++++++------ >> 7 files changed, 73 insertions(+), 11 deletions(-) >> >> diff --git a/NEWS b/NEWS >> index 637adcff3..1298d16a2 100644 >> --- a/NEWS >> +++ b/NEWS >> @@ -2,6 +2,10 @@ 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" that allows CMS to specify >> + if ovn-controller should flush related CT entries for removed LB backends. >> + By default, this option is set to false, i.e., CT entries are not flushed >> + when load balancer backends are removed. > > I think, the NEWS entry should be more clear that default behavior > has changed. Especially if we're planning to backport this to > a stable branch, it should be obvious that feature was taken away. > Current entry reads as if we just added a new opt-in feature. > > It's probably not necessary to re-spin the patch again for this, > if we can agree on a better wording in this thread. But I'll > leave this up to maintainers. > > Best regards, Ilya Maximets. > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
Thanks Ales for the patch and Ilya, Mark and Numan for reviews! I applied it to main with the following NEWS entry rephrase: diff --git a/NEWS b/NEWS index 1298d16a23..7edae22ff5 100644 --- a/NEWS +++ b/NEWS @@ -2,10 +2,9 @@ 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" that allows CMS to specify - if ovn-controller should flush related CT entries for removed LB backends. - By default, this option is set to false, i.e., CT entries are not flushed - when load balancer backends are removed. + - CT entries are not flushed by default anymore whenever a load balancer + backend is removed. A new, per-LB, option 'ct_flush' can be used to + restore the previous behavior. Disabled by default. --- Ales, can you please post a backport patch for branch 23.03? There are a few conflicts in the system tests. If you're busy with other stuff, let me know and I'll try to look into fixing those as soon as I get the chance. Regards, Dumitru On 3/20/23 15:58, Mark Michelson wrote: > Hi Ales, > > I agree with Ilya about updating the NEWS wording. I also think this is > something that a committer can take care of when committing the change. > > Acked-by: Mark Michelson <mmichels@redhat.com> > > On 3/20/23 08:00, Ilya Maximets wrote: >> On 3/20/23 11:53, Ales Musil wrote: >>> The CT flush was enabled by default for every LB, add >>> config option called "ct_flush" that allows >>> users to enable/disable the CT flush. The CT flush option >>> is set to "false" by default. >>> >>> Reported-at: https://bugzilla.redhat.com/2178962 >>> Acked-by: Numan Siddique <numans@ovn.org> >>> Signed-off-by: Ales Musil <amusil@redhat.com> >>> --- >>> v2: Make the feature opt-in. >>> Store the flag in 'struct ovn_controller_lb'. >>> v3: Update the NEWS. >>> Address nits from Dumitru. >>> v4: Update the documentation to be more accurate. >>> --- >>> NEWS | 4 ++++ >>> controller/ovn-controller.c | 6 ++++-- >>> lib/lb.c | 1 + >>> lib/lb.h | 1 + >>> ovn-nb.xml | 8 ++++++++ >>> tests/ovn.at | 28 +++++++++++++++++++++++++--- >>> tests/system-ovn.at | 36 ++++++++++++++++++++++++++++++------ >>> 7 files changed, 73 insertions(+), 11 deletions(-) >>> >>> diff --git a/NEWS b/NEWS >>> index 637adcff3..1298d16a2 100644 >>> --- a/NEWS >>> +++ b/NEWS >>> @@ -2,6 +2,10 @@ 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" that allows CMS to specify >>> + if ovn-controller should flush related CT entries for removed LB >>> backends. >>> + By default, this option is set to false, i.e., CT entries are >>> not flushed >>> + when load balancer backends are removed. >> >> I think, the NEWS entry should be more clear that default behavior >> has changed. Especially if we're planning to backport this to >> a stable branch, it should be obvious that feature was taken away. >> Current entry reads as if we just added a new opt-in feature. >> >> It's probably not necessary to re-spin the patch again for this, >> if we can agree on a better wording in this thread. But I'll >> leave this up to maintainers. >> >> Best regards, Ilya Maximets. >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >
diff --git a/NEWS b/NEWS index 637adcff3..1298d16a2 100644 --- a/NEWS +++ b/NEWS @@ -2,6 +2,10 @@ 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" that allows CMS to specify + if ovn-controller should flush related CT entries for removed LB backends. + By default, this option is set to false, i.e., CT entries are not flushed + when load balancer backends are removed. OVN v23.03.0 - 03 Mar 2023 -------------------------- diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 7dcbfd252..64f4d6a2a 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) || + !lb->ct_flush) { 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) || + !lb->ct_flush) { return; } diff --git a/lib/lb.c b/lib/lb.c index e941434c4..66c815275 100644 --- a/lib/lb.c +++ b/lib/lb.c @@ -798,6 +798,7 @@ ovn_controller_lb_create(const struct sbrec_load_balancer *sbrec_lb, lb->hairpin_orig_tuple = smap_get_bool(&sbrec_lb->options, "hairpin_orig_tuple", false); + lb->ct_flush = smap_get_bool(&sbrec_lb->options, "ct_flush", false); ovn_lb_get_hairpin_snat_ip(&sbrec_lb->header_.uuid, &sbrec_lb->options, &lb->hairpin_snat_ips); return lb; diff --git a/lib/lb.h b/lib/lb.h index 7a67b7426..ddd41703d 100644 --- a/lib/lb.h +++ b/lib/lb.h @@ -188,6 +188,7 @@ struct ovn_controller_lb { bool hairpin_orig_tuple; /* True if ovn-northd stores the original * destination tuple in registers. */ + bool ct_flush; /* True if we should flush CT after backend removal. */ struct lport_addresses hairpin_snat_ips; /* IP (v4 and/or v6) to be used * as source for hairpinned diff --git a/ovn-nb.xml b/ovn-nb.xml index 73f707aa0..387185033 100644 --- a/ovn-nb.xml +++ b/ovn-nb.xml @@ -2041,6 +2041,14 @@ or the affinity timeslot. Max supported affinity_timeout is 65535 seconds. </column> + + <column name="options" key="ct_flush" type='{"type": "boolean"}'> + The value indicates whether ovn-controller should flush CT entries + that are related to this LB. The flush happens if the LB is removed, + any of the backends is updated/removed or the LB is not considered + local anymore by the ovn-controller. This option is set to + <code>false</code> by default. + </column> </group> </table> diff --git a/tests/ovn.at b/tests/ovn.at index fa786112c..c2883ffca 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -34997,7 +34997,8 @@ check ovs-vsctl add-port br-int p1 -- set interface p1 external_ids:iface-id=lsp wait_for_ports_up ovn-nbctl --wait=hv sync -check ovn-nbctl lb-add lb1 "192.168.0.10" "192.168.10.10,192.168.10.20" +check ovn-nbctl lb-add lb1 "192.168.0.10" "192.168.10.10,192.168.10.20" \ + -- set load_balancer lb1 options:ct_flush="true" check ovn-nbctl ls-lb-add sw lb1 # Remove a single backend @@ -35020,7 +35021,8 @@ AT_CHECK([grep -q "Flushing CT for 5-tuple: vip=192.168.0.10:0, backend=192.168. AT_CHECK([grep -q "Flushing CT for 5-tuple: vip=192.168.0.10:0, backend=192.168.10.30:0, protocol=0" hv1/ovn-controller.log], [0]) # Check flush for LB with port and protocol -check ovn-nbctl lb-add lb1 "192.168.30.10:80" "192.168.40.10:8080,192.168.40.20:8090" udp +check ovn-nbctl lb-add lb1 "192.168.30.10:80" "192.168.40.10:8080,192.168.40.20:8090" udp \ + -- set load_balancer lb1 options:ct_flush="true" check ovn-nbctl ls-lb-add sw lb1 check ovn-nbctl lb-del lb1 check ovn-nbctl --wait=hv sync @@ -35029,7 +35031,8 @@ AT_CHECK([grep -q "Flushing CT for 5-tuple: vip=192.168.30.10:80, backend=192.16 AT_CHECK([grep -q "Flushing CT for 5-tuple: vip=192.168.30.10:80, backend=192.168.40.20:8090, protocol=17" hv1/ovn-controller.log], [0]) # Check recompute when LB is no longer local -check ovn-nbctl lb-add lb1 "192.168.50.10:80" "192.168.60.10:8080" +check ovn-nbctl lb-add lb1 "192.168.50.10:80" "192.168.60.10:8080" \ + -- set load_balancer lb1 options:ct_flush="true" check ovn-nbctl ls-lb-add sw lb1 check ovs-vsctl remove interface p1 external_ids iface-id check ovn-appctl inc-engine/recompute @@ -35039,6 +35042,25 @@ 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 CT flush is disabled by default +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 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..8afb4db56 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -9993,11 +9993,13 @@ check ovn-nbctl lsp-add bar bar3 \ -- lsp-set-addresses bar3 "f0:00:0f:01:02:05 172.16.1.4" # Config OVN load-balancer with a VIP. -check ovn-nbctl lb-add lb1 30.0.0.1 "172.16.1.2,172.16.1.3,172.16.1.4" +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="true" check ovn-nbctl ls-lb-add foo lb1 # Create another load-balancer with another VIP. lb2_uuid=`ovn-nbctl create load_balancer name=lb2 vips:30.0.0.3="172.16.1.2,172.16.1.3,172.16.1.4"` +check ovn-nbctl set load_balancer lb2 options:ct_flush="true" check ovn-nbctl ls-lb-add foo lb2 # Config OVN load-balancer with another VIP (this time with ports). @@ -10013,16 +10015,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 +10100,26 @@ check ovn-nbctl lb-del lb2 OVS_WAIT_UNTIL([test "$(ovs-appctl dpctl/dump-conntrack | FORMAT_CT(30.0.0.3) | wc -l)" = "0"]) +# Check that LB has CT flush disabled by default +check ovn-nbctl lb-add lb1 30.0.0.1 "172.16.1.2,172.16.1.3,172.16.1.4" +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