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 <[email protected]>
>
> 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 <[email protected]>
>>> Signed-off-by: Ales Musil <[email protected]>
>>> ---
>>> 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
>> [email protected]
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
