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

[Dumitru Ceara]

On 9/14/20 12:11 PM, Numan Siddique wrote:
> 
> 
> On Mon, Sep 14, 2020 at 3:17 PM Dumitru Ceara <dceara at redhat.com
> <mailto:dceara at redhat.com>> wrote:
> 
>     On 9/14/20 11:20 AM, numans at ovn.org <mailto:numans at ovn.org> wrote:
>     > From: Numan Siddique <numans at ovn.org <mailto: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
>     <mailto:kklimonda at syntaxhighlighted.com>>
>     > Fixes: e2aa124ff7c2("ovn-northd: Add ARP responder flows for SNAT
>     entries.")
>     > Signed-off-by: Numan Siddique <numans at ovn.org <mailto:numans at ovn.org>>
>     > ---
> 
>     Hi Numan,
> 
>     Please see my comments below.
> 
> 
> 
> Thanks Dumitru for the reviews
>  
> 
> 
>     >  northd/ovn-northd.c |  18 ++-
>     >  tests/ovn-northd.at <http://ovn-northd.at> |   8 +-
>     >  tests/ovn.at <http://ovn.at>        | 260
>     ++++++++++++++++++++++++++++++++++++++++++++
>     >  3 files changed, 280 insertions(+), 6 deletions(-)
>     >
>     > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
>     > index f394f17c3..2569212c7 100644
>     > --- a/northd/ovn-northd.c
>     > +++ b/northd/ovn-northd.c
>     > @@ -8856,7 +8856,11 @@ 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 has ovn-bridge-mappings)
>     configured will
> 
>     Nit: s/has/have
> 
>     > +                 * reply for the ARPrequest to the snat
>     external_ip. */
> 
> 
> Ack
>  
> 
> 
>     Nit: s/ARPrequest/ARP request
> 
> 
> Ack 
> 
>     > +                if (od->l3dgw_port || sset_contains(&snat_ips,
>     ext_addr)) {
>     >                      continue;
>     >                  }
>     >                  sset_add(&snat_ips, ext_addr);
> 
>     This can be rewritten to avoid additional hmap operations:
> 
>     if (od->l3dgw_port || !sset_add(&snat_ips, ext_addr)) {
>         continue;
>     }
> 
> Ack
>  
> 
>     > @@ -9237,6 +9241,7 @@ build_lrouter_flows(struct hmap *datapaths,
>     struct hmap *ports,
>     >              continue;
>     >          }
>     > 
>     > +        struct sset sset_snat_ips = SSET_INITIALIZER(&sset_snat_ips);
>     >          for (size_t i = 0; i < op->od->nbr->n_nat; i++) {
>     >              struct ovn_nat *nat_entry = &op->od->nat_entries[i];
>     >              const struct nbrec_nat *nat = nat_entry->nb;
>     > @@ -9246,8 +9251,15 @@ build_lrouter_flows(struct hmap *datapaths,
>     struct hmap *ports,
>     >                  continue;
>     >              }
>     > 
>     > +            struct lport_addresses *ext_addrs =
>     &nat_entry->ext_addrs;
>     > +            char *ext_addr = nat_entry_is_v6(nat_entry) ?
>     > +                             ext_addrs->ipv6_addrs[0].addr_s :
>     > +                             ext_addrs->ipv4_addrs[0].addr_s;
> 
>     As Ilya pointed out on a different patch, coding style[1] says:
>     "Break long lines before the ternary operators ? and :, rather than
>     after them". This should be:
> 
>                 char *ext_addr = (nat_entry_is_v6(nat_entry)
>                                   ? ext_addrs->ipv6_addrs[0].addr_s
>                                   : ext_addrs->ipv4_addrs[0].addr_s);
> 
>     [1]
>     https://docs.ovn.org/en/latest/internals/contributing/coding-style.html#expressions
> 
> 
> Ack. Thanks for pointing it out.
>  
> 
> 
> 
>     >              if (!strcmp(nat->type, "snat")) {
>     > -                continue;
>     > +                if (sset_contains(&sset_snat_ips, ext_addr)) {
>     > +                    continue;
>     > +                }
>     > +                sset_add(&sset_snat_ips, ext_addr);
> 
>     To avoid additional hmap operations this can be:
> 
>     if (!sset_add(&sset_snat_ips, ext_addr)) {
>         continue;
>     }
> 
> 
> Ack.
>  
> 
>     Also, we walk the nat entries twice and the snat_ips once for each
>     logical router port IP. Why don't we build the snat_ips set earlier and
>     use it in all cases, e.g., here, instead of an array we could build the
>     snat_ips set:
> 
>     https://github.com/ovn-org/ovn/blob/64cc065e2c59c0696edeef738180989d993ceceb/northd/ovn-northd.c#L9116
> 
> 
> Since we have to backport this fix to branch-20.06, I think its better
> to refactor the code as a separate patch.
> 
> I will look into the refactoring after reviewing Anton's patches. Does
> this sound good ? What do you thnk ?
> 
> Thanks
> Numan
>  

Sounds good to me, thanks!