| Message ID | 20230317071422.214842-1-amusil@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2] 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/17/23 08:14, 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 > Signed-off-by: Ales Musil <amusil@redhat.com> > --- Hi Ales, > v2: Make the feature opt-in. I agree with this (as mentioned on v1). > Store the flag in 'struct ovn_controller_lb'. > --- > NEWS | 2 ++ > controller/ovn-controller.c | 6 ++++-- > lib/lb.c | 3 +++ > lib/lb.h | 1 + > ovn-nb.xml | 6 ++++++ > tests/ovn.at | 28 +++++++++++++++++++++++++--- > tests/system-ovn.at | 36 ++++++++++++++++++++++++++++++------ > 7 files changed, 71 insertions(+), 11 deletions(-) > Overall the change looks good to me. I only have some minor comments below. The most important is the missing NEWS default value change notification. > diff --git a/NEWS b/NEWS > index 637adcff3..085b1603a 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" that allows CMS to specify > + if ovn-controller should flush related CT entries for removed LB backends. > We really need to mention that this is opt-in now. So maybe something like "By default, this option is set to false, i.e., CT entries are not flushed when load balancer backends are removed.". Does this sound OK? > 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..252ad3748 100644 > --- a/lib/lb.c > +++ b/lib/lb.c > @@ -798,6 +798,9 @@ 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); > + Nit: I'd remove this newline. > + lb->ct_flush = smap_get_bool(&sbrec_lb->options, "ct_flush", false); > + Nit: I'd remove this newline too. > 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..22254815d 100644 > --- a/lib/lb.h > +++ b/lib/lb.h > @@ -193,6 +193,7 @@ struct ovn_controller_lb { > * as source for hairpinned > * traffic. > */ > + bool ct_flush; > }; > With this I get: $ pahole lib/lb.o [...] struct ovn_controller_lb { struct hmap_node hmap_node; /* 0 16 */ const struct sbrec_load_balancer * slb; /* 16 8 */ uint8_t proto; /* 24 1 */ /* XXX 7 bytes hole, try to pack */ struct ovn_lb_vip * vips; /* 32 8 */ size_t n_vips; /* 40 8 */ _Bool hairpin_orig_tuple; /* 48 1 */ /* XXX 7 bytes hole, try to pack */ struct lport_addresses hairpin_snat_ips; /* 56 56 */ /* --- cacheline 1 boundary (64 bytes) was 48 bytes ago --- */ _Bool ct_flush; /* 112 1 */ /* size: 120, cachelines: 2, members: 8 */ /* sum members: 99, holes: 2, sum holes: 14 */ /* padding: 7 */ /* last cacheline: 56 bytes */ }; Nit: I'd move 'ct_flush' just after 'hairpin_orig_tuple'. Although, there's more packing we could do here but that's not a problem of this patch. Also, please add a comment for 'ct_flush'. > struct ovn_controller_lb *ovn_controller_lb_create( > diff --git a/ovn-nb.xml b/ovn-nb.xml > index 73f707aa0..aec8a2284 100644 > --- a/ovn-nb.xml > +++ b/ovn-nb.xml > @@ -2041,6 +2041,12 @@ 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 when the backends are removed. Being set > + to <code>false</code> by default. Nit: This doesn't sound like a sentence to me. I think I'd rephrase to: "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..c1f6edda7 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" Nit: I'd indent this 4 spaces to the right. > 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" Nit: here too. > 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" Ditto. > 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]) Hmm, shouldn't this be un-commented? > + > +# 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]) These too? > + > +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..d6fe968fc 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" Nit: indent. > 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 Thanks, Dumitru
On Fri, Mar 17, 2023 at 10:33 AM Dumitru Ceara <dceara@redhat.com> wrote: > On 3/17/23 08:14, 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 > > Signed-off-by: Ales Musil <amusil@redhat.com> > > --- > > Hi Ales, > Hi Dumitru, thank you for the review. > > v2: Make the feature opt-in. > > I agree with this (as mentioned on v1). > > > Store the flag in 'struct ovn_controller_lb'. > > --- > > NEWS | 2 ++ > > controller/ovn-controller.c | 6 ++++-- > > lib/lb.c | 3 +++ > > lib/lb.h | 1 + > > ovn-nb.xml | 6 ++++++ > > tests/ovn.at | 28 +++++++++++++++++++++++++--- > > tests/system-ovn.at | 36 ++++++++++++++++++++++++++++++------ > > 7 files changed, 71 insertions(+), 11 deletions(-) > > > > Overall the change looks good to me. I only have some minor comments > below. The most important is the missing NEWS default value change > notification. > > > diff --git a/NEWS b/NEWS > > index 637adcff3..085b1603a 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" that allows CMS to specify > > + if ovn-controller should flush related CT entries for removed LB > backends. > > > > We really need to mention that this is opt-in now. So maybe something > like "By default, this option is set to false, i.e., CT entries are not > flushed when load balancer backends are removed.". Does this sound OK? > Agreed, added in v3. > > > 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..252ad3748 100644 > > --- a/lib/lb.c > > +++ b/lib/lb.c > > @@ -798,6 +798,9 @@ 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); > > + > > Nit: I'd remove this newline. > > > + lb->ct_flush = smap_get_bool(&sbrec_lb->options, "ct_flush", false); > > + > > Nit: I'd remove this newline too. > > > 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..22254815d 100644 > > --- a/lib/lb.h > > +++ b/lib/lb.h > > @@ -193,6 +193,7 @@ struct ovn_controller_lb { > > * as source for hairpinned > > * traffic. > > */ > > + bool ct_flush; > > }; > > > > With this I get: > > $ pahole lib/lb.o > [...] > struct ovn_controller_lb { > struct hmap_node hmap_node; /* 0 16 */ > const struct sbrec_load_balancer * slb; /* 16 8 */ > uint8_t proto; /* 24 1 */ > > /* XXX 7 bytes hole, try to pack */ > > struct ovn_lb_vip * vips; /* 32 8 */ > size_t n_vips; /* 40 8 */ > _Bool hairpin_orig_tuple; /* 48 1 */ > > /* XXX 7 bytes hole, try to pack */ > > struct lport_addresses hairpin_snat_ips; /* 56 56 */ > /* --- cacheline 1 boundary (64 bytes) was 48 bytes ago --- */ > _Bool ct_flush; /* 112 1 */ > > /* size: 120, cachelines: 2, members: 8 */ > /* sum members: 99, holes: 2, sum holes: 14 */ > /* padding: 7 */ > /* last cacheline: 56 bytes */ > }; > > Nit: I'd move 'ct_flush' just after 'hairpin_orig_tuple'. Although, > there's more packing we could do here but that's not a problem of > this patch. > Maybe one day we can revisit most of the structures. Fixed in v3. > > Also, please add a comment for 'ct_flush'. > > > struct ovn_controller_lb *ovn_controller_lb_create( > > diff --git a/ovn-nb.xml b/ovn-nb.xml > > index 73f707aa0..aec8a2284 100644 > > --- a/ovn-nb.xml > > +++ b/ovn-nb.xml > > @@ -2041,6 +2041,12 @@ 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 when the backends are removed. > Being set > > + to <code>false</code> by default. > > Nit: This doesn't sound like a sentence to me. I think I'd rephrase to: > "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..c1f6edda7 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" > > Nit: I'd indent this 4 spaces to the right. > > > 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" > > Nit: here too. > > > 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" > > Ditto. > > > 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]) > > Hmm, shouldn't this be un-commented? > > > + > > +# 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]) > > These too? > > > + > > +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..d6fe968fc 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" > > Nit: indent. > > > 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 > > Thanks, > Dumitru > > Everything should be addressed in v3. Thanks, Ales.
diff --git a/NEWS b/NEWS index 637adcff3..085b1603a 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" 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..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..252ad3748 100644 --- a/lib/lb.c +++ b/lib/lb.c @@ -798,6 +798,9 @@ 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..22254815d 100644 --- a/lib/lb.h +++ b/lib/lb.h @@ -193,6 +193,7 @@ struct ovn_controller_lb { * as source for hairpinned * traffic. */ + bool ct_flush; }; struct ovn_controller_lb *ovn_controller_lb_create( diff --git a/ovn-nb.xml b/ovn-nb.xml index 73f707aa0..aec8a2284 100644 --- a/ovn-nb.xml +++ b/ovn-nb.xml @@ -2041,6 +2041,12 @@ 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 when the backends are removed. Being set + to <code>false</code> by default. + </column> </group> </table> diff --git a/tests/ovn.at b/tests/ovn.at index fa786112c..c1f6edda7 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..d6fe968fc 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
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 Signed-off-by: Ales Musil <amusil@redhat.com> --- v2: Make the feature opt-in. Store the flag in 'struct ovn_controller_lb'. --- NEWS | 2 ++ controller/ovn-controller.c | 6 ++++-- lib/lb.c | 3 +++ lib/lb.h | 1 + ovn-nb.xml | 6 ++++++ tests/ovn.at | 28 +++++++++++++++++++++++++--- tests/system-ovn.at | 36 ++++++++++++++++++++++++++++++------ 7 files changed, 71 insertions(+), 11 deletions(-)