On Fri, Mar 5, 2021 at 2:30 AM Ben Pfaff <b...@ovn.org> wrote:
>
> On Wed, Mar 03, 2021 at 11:32:39PM +0530, num...@ovn.org wrote:
> > From: Numan Siddique <num...@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 <b...@ovn.org>
> > Signed-off-by: Numan Siddique <num...@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
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
