On Mon, Sep 14, 2020 at 8:58 PM Numan Siddique <[email protected]> wrote:
> > > 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, > > Please take a look at v3 - https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/ Numan > 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
