diff mbox series

[ovs-dev] northd: Use reachable sset of 'ovn_lb_ip_set' when adding arp req lflows.

Message ID 20230804192438.12155-1-numans@ovn.org
State Accepted
Headers show
Series [ovs-dev] northd: Use reachable sset of 'ovn_lb_ip_set' when adding arp req lflows. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test fail github build: failed
ovsrobot/github-robot-_ovn-kubernetes fail github build: failed

Commit Message

Numan Siddique Aug. 4, 2023, 7:24 p.m. UTC 
From: Numan Siddique <numans@ovn.org>

When adding the arp req lflows in the 'ls_in_l2_lkup' stage for the
load balancer VIPs associated to a logical router we iterate through
the lb_ips sset of the router datapath.  For each ip of this sset we
check if this is reachable or not from any of the router ports.
We can instead use the reachable sset of ips (which is generated by
northd engine node).

Signed-off-by: Numan Siddique <numans@ovn.org>
---
 northd/northd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Dumitru Ceara Aug. 7, 2023, 11:14 a.m. UTC | #1 
On 8/4/23 21:24, numans@ovn.org wrote:
> From: Numan Siddique <numans@ovn.org>
> 
> When adding the arp req lflows in the 'ls_in_l2_lkup' stage for the
> load balancer VIPs associated to a logical router we iterate through
> the lb_ips sset of the router datapath.  For each ip of this sset we
> check if this is reachable or not from any of the router ports.
> We can instead use the reachable sset of ips (which is generated by
> northd engine node).
> 
> Signed-off-by: Numan Siddique <numans@ovn.org>
> ---

Acked-by: Dumitru Ceara <dceara@redhat.com>

It would be even better if you could also add a test (maybe just
checking for lflows) to validate this behavior.

Thanks,
Dumitru
Numan Siddique Aug. 8, 2023, 3:50 a.m. UTC | #2 
On Mon, Aug 7, 2023 at 4:45 PM Dumitru Ceara <dceara@redhat.com> wrote:
>
> On 8/4/23 21:24, numans@ovn.org wrote:
> > From: Numan Siddique <numans@ovn.org>
> >
> > When adding the arp req lflows in the 'ls_in_l2_lkup' stage for the
> > load balancer VIPs associated to a logical router we iterate through
> > the lb_ips sset of the router datapath.  For each ip of this sset we
> > check if this is reachable or not from any of the router ports.
> > We can instead use the reachable sset of ips (which is generated by
> > northd engine node).
> >
> > Signed-off-by: Numan Siddique <numans@ovn.org>
> > ---
>
> Acked-by: Dumitru Ceara <dceara@redhat.com>
>
> It would be even better if you could also add a test (maybe just
> checking for lflows) to validate this behavior.

Thanks Dumitru for the review.  We do have a test case in ovn-northd
"ovn -- ARP flows for unreachable addresses - NAT and LB" here [1].
I think that's good enough.  So I went ahead and applied this patch to
the main branch.

[1] - https://github.com/ovn-org/ovn/blob/main/tests/ovn-northd.at#L4955

Numan


>
> Thanks,
> Dumitru
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Dumitru Ceara Aug. 8, 2023, 10:13 a.m. UTC | #3 
On 8/8/23 05:50, Numan Siddique wrote:
> On Mon, Aug 7, 2023 at 4:45 PM Dumitru Ceara <dceara@redhat.com> wrote:
>>
>> On 8/4/23 21:24, numans@ovn.org wrote:
>>> From: Numan Siddique <numans@ovn.org>
>>>
>>> When adding the arp req lflows in the 'ls_in_l2_lkup' stage for the
>>> load balancer VIPs associated to a logical router we iterate through
>>> the lb_ips sset of the router datapath.  For each ip of this sset we
>>> check if this is reachable or not from any of the router ports.
>>> We can instead use the reachable sset of ips (which is generated by
>>> northd engine node).
>>>
>>> Signed-off-by: Numan Siddique <numans@ovn.org>
>>> ---
>>
>> Acked-by: Dumitru Ceara <dceara@redhat.com>
>>
>> It would be even better if you could also add a test (maybe just
>> checking for lflows) to validate this behavior.
> 
> Thanks Dumitru for the review.  We do have a test case in ovn-northd
> "ovn -- ARP flows for unreachable addresses - NAT and LB" here [1].
> I think that's good enough.  So I went ahead and applied this patch to
> the main branch.
> 

You're right, thanks for checking!

Regards,
Dumitru
diff mbox series

Patch

diff --git a/northd/northd.c b/northd/northd.c
index b9605862e..bc61b3047 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -8853,7 +8853,7 @@  build_lswitch_rport_arp_req_flows(struct ovn_port *op,
      */
 
     const char *ip_addr;
-    SSET_FOR_EACH (ip_addr, &op->od->lb_ips->ips_v4) {
+    SSET_FOR_EACH (ip_addr, &op->od->lb_ips->ips_v4_reachable) {
         ovs_be32 ipv4_addr;
 
         /* Check if the ovn port has a network configured on which we could
@@ -8866,7 +8866,7 @@  build_lswitch_rport_arp_req_flows(struct ovn_port *op,
                 stage_hint);
         }
     }
-    SSET_FOR_EACH (ip_addr, &op->od->lb_ips->ips_v6) {
+    SSET_FOR_EACH (ip_addr, &op->od->lb_ips->ips_v6_reachable) {
         struct in6_addr ipv6_addr;
 
         /* Check if the ovn port has a network configured on which we could