On 12/5/22 16:22, Lorenzo Bianconi wrote:
>> On 12/5/22 16:16, Lorenzo Bianconi wrote:
>>>> For the case when multiple LBs (same VIP but different port) share the
>>>> same subset of backends we need to differentiate between them by also
>>>> matching on the L4 port. Without that affinity configuration from one
>>>> load balancer might be incorrectly applied to another.
>>>>
>>>> Adapt the unit and system tests to cover this scenario too.
>>>>
>>>> Fixes: d3926b433e44 ("northd: rely on new actions for lb affinity")
>>>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2150533
>>>> Reported-by: Surya Seetharaman <[email protected]>
>>>> Signed-off-by: Dumitru Ceara <[email protected]>
>>>
>>> Hi Dumitru,
>>>
>>
>> Hi Lorenzo,
>>
>>> thx for fixing this issue, just a small nit inline.
>>>
>>> Acked-by: Lorenzo Bianconi <[email protected]>
>>>
>>
>> Thanks for your review!
>>
>>>> ---
>>>> northd/northd.c | 48 +++++++++++++++++++++++++++-----------
>>>> tests/ovn-northd.at | 8 +++----
>>>> tests/system-ovn.at | 57 ++++++++++++++++++++++++++++++++++++++++++++-
>>>> 3 files changed, 95 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/northd/northd.c b/northd/northd.c
>>>> index 74facce7ac..27047ff74b 100644
>>>> --- a/northd/northd.c
>>>> +++ b/northd/northd.c
>>>> @@ -6984,13 +6984,15 @@ build_lb_rules_pre_stateful(struct hmap *lflows,
>>>> struct ovn_northd_lb *lb,
>>>> * table=lr_in_lb_aff_learn, priority=100
>>>> * match=(REGBIT_KNOWN_LB_SESSION == 0
>>>> * && ct.new && ip4
>>>> - * && REG_ORIG_DIP_IPV4 == V && ip4.dst == B1 && tcp.dst ==
>>>> BP1)
>>>> + * && REG_NEXT_HOP_IPV4 == V && REG_ORIG_TP_DPORT_ROUTER = VP
>>>> + * && ip4.dst == B1 && tcp.dst == BP1)
>>>> * action=(commit_lb_aff(vip = "V:VP", backend = "B1:BP1",
>>>> * proto = tcp, timeout = T));
>>>> * table=lr_in_lb_aff_learn, priority=100
>>>> * match=(REGBIT_KNOWN_LB_SESSION == 0
>>>> * && ct.new && ip4
>>>> - * && REG_ORIG_DIP_IPV4 == V && ip4.dst == B2 && tcp.dst ==
>>>> BP2)
>>>> + * && REG_NEXT_HOP_IPV4 == V && REG_ORIG_TP_DPORT_ROUTER = VP
>>>> + * && ip4.dst == B2 && tcp.dst == BP2)
>>>> * action=(commit_lb_aff(vip = "V:VP", backend = "B2:BP2",
>>>> * proto = tcp, timeout = T));
>>>> *
>>>> @@ -7032,6 +7034,7 @@ build_lb_affinity_lr_flows(struct hmap *lflows,
>>>> struct ovn_northd_lb *lb,
>>>> const char *ip_match = ipv6 ? "ip6" : "ip4";
>>>>
>>>> const char *reg_vip = ipv6 ? REG_NEXT_HOP_IPV6 : REG_NEXT_HOP_IPV4;
>>>> + const char *reg_port = REG_ORIG_TP_DPORT_ROUTER;
>>>
>>> do we need reg_port? I guess we can just use REG_ORIG_TP_DPORT_ROUTER
>>> directly.
>>>
>>
>> We can use it directly but I wanted to match the rest of the flow's
>> style. Would it seem better if I renamed it to 'reg_vport'?
>> Alternatively, if you prefer, I can easily inline it.
>
> I would say to use REG_ORIG_TP_DPORT_ROUTER directly, in the other cases we
> have ternary operator, but I do not have a strong opinion on it, up to you.
>
OK, I made the change you suggested and then pushed the patch to the
main branch and to branch-22.12.
Thanks for the reviews Lorenzo and Ales!
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
