[ovs-dev] [PATCH ovn 2/2] northd: Fix the missing force_snat_for_lb flows when router_ip is configured.

Numan Siddique numans at ovn.org
Fri Mar 5 01:16:55 UTC 2021


On Fri, Mar 5, 2021 at 2:30 AM Ben Pfaff <blp at ovn.org> wrote:
>
> On Wed, Mar 03, 2021 at 11:32:39PM +0530, numans at ovn.org wrote:
> > From: Numan Siddique <numans at ovn.org>
> >
> > The commit c6e21a23bd8 which supported the option 'lb_force_snat_ip=router_ip'
> > on a gateway router, missed out on
> >   - updating the flows in 'lr_in_dnat' to set 'flags.force_snat_for_lb = 1'.
> >   - removing the flow to drop if ip.dst == router port IP in 'lr_in_ip_input'
> >     stage.
> >
> > This patch fixes these issue and adds a system test to cover the
> > hairpin load balancer traffic for the gateway routers.
> >
> > Fixes: c6e21a23bd8("northd: Provide the Gateway router option 'lb_force_snat_ip' to take router port ips.")
> >
> > CC: Ben Pfaff <blp at ovn.org>
> > Signed-off-by: Numan Siddique <numans at ovn.org>
>
> Thanks!  I took a look at the differences between the C and the DDlog
> versions.  I have a few comments.
>
> The C version adds a new flow to the output.  The DDlog version doesn't
> appear to, yet I do see a similar flow in the DDlog code.  I am not sure
> how that happened.

Thanks for the review.

Can you please tell me in which table the new flow is seen?

If a logical router has the option 'lb_force_nat_ip=router_ip' is set, then both
the C version and ddlog version do the below

  (1) Update the flow in the table lr_in_dnat to add
"flags.force_snat_for_lb = 1" in the action
    for the packets which are destined to load balancer vips.

  (2) Don't add the priority-60 flow in 'lr_in_ip_input' to drop the
packets for router ips.
    The below suggestion provided by you to replace "not
force_lb_snat" with  ".force_lb_snat = false"
    does that.


>
> Looking at the following:
> >      &RouterPort(.lrp = nb::Logical_Router_Port{._uuid = lrp_uuid},
> >                  .router = &Router{.snat_ips = snat_ips,
> > +                                  .force_lb_snat = force_lb_snat,
> >                                    .lr = nb::Logical_Router{._uuid = lr_uuid}},
> >                  .networks = networks),
> >      var addr = FlatMap(networks.ipv4_addrs),
> >      not snat_ips.contains_key(IPv4{addr.addr}),
> > +    not force_lb_snat,
> >      var match_ips = "${addr.addr}".group_by((lr_uuid, lrp_uuid)).to_vec().
>
> You can optimize it by writing
> > +                                  .force_lb_snat = false,
> and then omitting the later clause.  With the way DDlog currently
> works, this isn't just cosmetic, it will actually yield faster
> code because DDlog will discard nonmatching records earlier.  (If
> that's an actual performance problem it will come out later in
> profiling, I'm just giving a tip here that I only recently learned
> myself.)

I tried like below as per your suggestion and the compilation failed

***
ddlog -i ../northd/ovn_northd.dl -o ./northd -L
/home/nusiddiq/.local/ddlog/lib -L ./northd

error: failed to parse input file: "../northd/ovn_northd.dl" (line
5150, column 20):
unexpected "="
expecting "in"
*****

------------
diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
index 90839b663..a7cd0892a 100644
--- a/northd/ovn_northd.dl
+++ b/northd/ovn_northd.dl
@@ -5147,7 +5147,7 @@ Flow(.logical_datapath = lr_uuid,
                 .networks = networks),
     var addr = FlatMap(networks.ipv4_addrs),
     not snat_ips.contains_key(IPv4{addr.addr}),
-    not force_lb_snat,
+    .force_lb_snat = false,
     var match_ips = "${addr.addr}".group_by((lr_uuid, lrp_uuid)).to_vec().
 Flow(.logical_datapath = lr_uuid,
      .stage = router_stage(IN, IP_INPUT),
@@ -5162,7 +5162,7 @@ Flow(.logical_datapath = lr_uuid,
                 .networks = networks),
     var addr = FlatMap(networks.ipv6_addrs),
     not snat_ips.contains_key(IPv6{addr.addr}),
-    not force_lb_snat,
+    .force_lb_snat = false,
     var match_ips = "${addr.addr}".group_by((lr_uuid, lrp_uuid)).to_vec().
 --------------------

I also tried "force_lb_snat = false" and it still failed.

>From what I understand the above code does (2) which I mentioned.

Thanks
Numan

>
> Thanks,
>
> Ben.
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list