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 <amu...@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
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
