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