diff mbox series

[ovs-dev,v4] controller: Add config option per LB to enable/disable CT flush

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

Checks

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

Commit Message

Ales Musil March 20, 2023, 10:53 a.m. UTC
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(-)

Comments

Ilya Maximets March 20, 2023, noon UTC | #1
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.
Mark Michelson March 20, 2023, 2:58 p.m. UTC | #2
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
>
Dumitru Ceara March 20, 2023, 3:51 p.m. UTC | #3
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 mbox series

Patch

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