[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 06:07:38 UTC 2021


On Fri, Mar 5, 2021 at 7:10 AM Ben Pfaff <blp at ovn.org> wrote:
>
> On Fri, Mar 05, 2021 at 06:46:55AM +0530, Numan Siddique wrote:
> > 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.
>
> 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);
>

Oops. You're right. I totally missed it. Thanks a lot for spotting this.
I'll address this and send v2.

> > > 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,

Thanks. I'll try this out.

Numan

>                                    .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
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list