[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:48:49 UTC 2020


On Mon, Sep 14, 2020 at 8:58 PM Numan Siddique <numans at ovn.org> wrote:

>
>
> 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,
>
>
Please take a look at v3 -
https://patchwork.ozlabs.org/project/ovn/patch/20200914154625.1142765-1-numans@ovn.org/

Numan


> 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