[ovs-dev] [PATCH ovn v2] ovn-northd: Fix multiple ARP replies for SNAT entries configured on a distributed router.

Numan Siddique numans at ovn.org
Mon Sep 14 15:28:18 UTC 2020


On Mon, Sep 14, 2020 at 8:47 PM Dumitru Ceara <dceara at redhat.com> wrote:

> On 9/14/20 12:24 PM, numans at ovn.org wrote:
> > From: Numan Siddique <numans at ovn.org>
> >
> > The commit in the Fixes tag, while addressing the issue to send ARP
> replies for
> > the SNAT entries, didn't take into account the gateway router port
> scenario.
> > Because of this, all the chassis which have bridge mappings configured
> reply
> > to ARP request for SNAT entries. This patch fixes the issue by adding
> > "is_chassis_resident()" condition for such flows so that only the
> gateway chassis
> > which claims the gateway router port responds to the ARP request.
> >
> > Note: This patch doesn't require any changes to ovn-northd.8.xml as it
> was already
> > documented with the desired flows for SNAT entries.
> >
> > Reported-at:
> https://mail.openvswitch.org/pipermail/ovs-discuss/2020-September/050679.html
> > Reported-by: Chris <kklimonda at syntaxhighlighted.com>
> > Fixes: e2aa124ff7c2("ovn-northd: Add ARP responder flows for SNAT
> entries.")
> > Signed-off-by: Numan Siddique <numans at ovn.org>
> > ---
> > v1 -> v2
> > ---
> >  * Address review comments from Dumitru.
> >
> >  northd/ovn-northd.c |  18 ++-
> >  tests/ovn-northd.at |   8 +-
> >  tests/ovn.at        | 260 ++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 279 insertions(+), 7 deletions(-)
> >
> > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > index f394f17c3..b37ccef2a 100644
> > --- a/northd/ovn-northd.c
> > +++ b/northd/ovn-northd.c
> > @@ -8856,10 +8856,13 @@ build_lrouter_flows(struct hmap *datapaths,
> struct hmap *ports,
> >                               ext_addrs->ipv4_addrs[0].addr_s;
> >
> >              if (!strcmp(nat->type, "snat")) {
> > -                if (sset_contains(&snat_ips, ext_addr)) {
> > +                /* If its a router with distributed gateway port then
> don't
> > +                 * add the ARP flow for SNAT entries. Otherwise all the
> > +                 * chassis (which have ovn-bridge-mappings) configured
> will
> > +                 * reply for the ARP request to the snat external_ip. */
> > +                if (od->l3dgw_port || !sset_add(&snat_ips, ext_addr)) {
>
> Hi Numan,
>
> Thinking more about it, we don't need "if (od->l3dgw_port" here, right?
> This is where we generate priority-90 flows. Priority 91, 92 (for
> distributed gw ports) are generated below. That should be enough and
> we'd maintain the same logic as for other NAT types.
>
> As a matter of fact, the tests pass just fine if I change this statement
> to:
>
> if (!sset_add(&snat_ips, ext_addr)) {
>

Ok. I'll remove the check.


>
> On the same refactoring note as on v1, we have this kind of check "if
> od->l3dgw_port then do this, otherwise to something completely
> different" in a lot of places in the code. Maybe it's worth dedicating
> separate functions for logical routers with distributed gateway ports vs
> logical routers without. What do you think?
>

Agree.

>
> Similar as we discussed on the v1 patch, we should do this as a follow
> up. But should we write them down somewhere? Would this be worth an
> entry in the TODO.rst file?
>
> I can add an entry in TODO.rst about the refactoring.



> >                      continue;
> >                  }
> > -                sset_add(&snat_ips, ext_addr);
> >              }
> >
> >              /* Priority 91 and 92 flows are added for each gateway
> router
> > @@ -9237,6 +9240,7 @@ build_lrouter_flows(struct hmap *datapaths, struct
> hmap *ports,
> >              continue;
> >          }
> >
> > +        struct sset sset_snat_ips = SSET_INITIALIZER(&sset_snat_ips);
>
> Nit (also for the follow up): this is very confusing. In the same
> function, build_lrouter_flows(), we have "struct sset snat_ips", "struct
> v46_ip *snat_ips", "struct sset sset_snat_ips". We should probably just
> prepopulate a sset as part of struct ovn_datapath and use it everywhere,
> like we do for other things like "nat_entries".
>

Yeah. Agree. Its better if we keep this in ovn_datapath struct,

Thanks
Numan


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


More information about the dev mailing list