On 9/14/20 12:11 PM, Numan Siddique wrote: > > > On Mon, Sep 14, 2020 at 3:17 PM Dumitru Ceara <[email protected] > <mailto:[email protected]>> wrote: > > On 9/14/20 11:20 AM, [email protected] <mailto:[email protected]> wrote: > > From: Numan Siddique <[email protected] <mailto:[email protected]>> > > > > 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 <[email protected] > <mailto:[email protected]>> > > Fixes: e2aa124ff7c2("ovn-northd: Add ARP responder flows for SNAT > entries.") > > Signed-off-by: Numan Siddique <[email protected] <mailto:[email protected]>> > > --- > > 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! _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
