On Fri, Mar 05, 2021 at 06:46:55AM +0530, Numan Siddique wrote: > 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.
If they do the same thing, then that's great and no change is needed. What surprises me is that the C version of the changes has the following hunk to add a new flow, but there's no corresponding change to the DDlog version to add a new flow. @@ -9153,10 +9153,17 @@ build_lrouter_force_snat_flows_op(struct ovn_port *op, if (op->lrp_networks.n_ipv4_addrs) { ds_clear(match); ds_clear(actions); + ds_put_format(match, "inport == %s && ip4.dst == %s", + op->json_key, op->lrp_networks.ipv4_addrs[0].addr_s); + ovn_lflow_add(lflows, op->od, S_ROUTER_IN_UNSNAT, 110, + ds_cstr(match), "ct_snat;"); + + ds_clear(match); + /* Higher priority rules to force SNAT with the router port ip. * This only takes effect when the packet has already been * load balanced once. */ ds_put_format(match, "flags.force_snat_for_lb == 1 && ip4 && " "outport == %s", op->json_key); > > 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 Oh, I meant like the following incremental. Sorry I wasn't clear! diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl index 0e208c1f5160..21092cc1bcea 100644 --- a/northd/ovn_northd.dl +++ b/northd/ovn_northd.dl @@ -5142,12 +5142,11 @@ Flow(.logical_datapath = lr_uuid, .external_ids = stage_hint(lrp_uuid)) :- &RouterPort(.lrp = nb::Logical_Router_Port{._uuid = lrp_uuid}, .router = &Router{.snat_ips = snat_ips, - .force_lb_snat = force_lb_snat, + .force_lb_snat = false, .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(). Flow(.logical_datapath = lr_uuid, .stage = router_stage(IN, IP_INPUT), @@ -5157,12 +5156,11 @@ Flow(.logical_datapath = lr_uuid, .external_ids = stage_hint(lrp_uuid)) :- &RouterPort(.lrp = nb::Logical_Router_Port{._uuid = lrp_uuid}, .router = &Router{.snat_ips = snat_ips, - .force_lb_snat = force_lb_snat, + .force_lb_snat = false, .lr = nb::Logical_Router{._uuid = lr_uuid}}, .networks = networks), var addr = FlatMap(networks.ipv6_addrs), not snat_ips.contains_key(IPv6{addr.addr}), - not force_lb_snat, var match_ips = "${addr.addr}".group_by((lr_uuid, lrp_uuid)).to_vec(). for (RouterPortNetworksIPv4Addr( _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev