[ovs-dev] [PATCH ovn] ovn-northd: Fix multiple ARP replies for SNAT entries configured on a distributed router.
Numan Siddique
numans at ovn.org
Mon Sep 14 10:25:38 UTC 2020
On Mon, Sep 14, 2020 at 3:47 PM Dumitru Ceara <dceara at redhat.com> wrote:
> 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!
>
Thanks. v2 sent -
https://patchwork.ozlabs.org/project/ovn/patch/20200914102437.693984-1-numans@ovn.org/
Please take a look.
Numan
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
More information about the dev
mailing list