On Mon, Mar 31, 2025 at 6:33 PM Han Zhou <hz...@ovn.org> wrote: > > > > On Thu, Mar 27, 2025 at 10:16 AM Numan Siddique <num...@ovn.org> wrote: > > > > On Tue, Mar 25, 2025 at 9:06 AM Han Zhou <hz...@ovn.org> wrote: > > > > > > > > > > > > On Fri, Mar 14, 2025 at 12:35 AM Numan Siddique <num...@ovn.org> wrote: > > > > > > > > On Thu, Mar 13, 2025 at 12:21 PM Han Zhou <hz...@ovn.org> wrote: > > > > > > > > > > > > > > > > > > > > On Sat, Mar 1, 2025 at 6:38 AM <num...@ovn.org> wrote: > > > > >> > > > > >> From: Numan Siddique <num...@ovn.org> > > > > >> > > > > >> This patch series optimizes ovn-controller to add only relevant > > > > >> datapaths to its "local datapaths" map if the logical topology > > > > >> has a single flat provider logical switch connected to multiple > > > > >> routers with a distributed gateway port. > > > > >> > > > > >> (Patch 3 has a detailed commit message.) > > > > >> > > > > >> v1 -> v2 > > > > >> ----- > > > > >> * Rebased to resolve the conflicts. > > > > >> > > > > >> Numan Siddique (3): > > > > >> controller: Store local binding lports in local_datapath. > > > > >> northd: Add a flag 'only_dgp_peer_ports' to the SB Datapath. > > > > >> controller: Optimize adding 'dps' to the local datapaths. > > > > >> > > > > >> controller/binding.c | 324 +++++++++++--- > > > > >> controller/binding.h | 2 + > > > > >> controller/local_data.c | 98 ++++- > > > > >> controller/local_data.h | 10 +- > > > > >> controller/lport.c | 12 + > > > > >> controller/lport.h | 4 + > > > > >> controller/ovn-controller.c | 38 ++ > > > > >> northd/northd.c | 31 +- > > > > >> northd/northd.h | 4 + > > > > >> tests/multinode.at | 185 +++++++- > > > > >> tests/ovn-northd.at | 59 +++ > > > > >> tests/ovn-performance.at | 6 +- > > > > >> tests/ovn.at | 853 > > > > >> ++++++++++++++++++++++++++++++++++++ > > > > >> 13 files changed, 1545 insertions(+), 81 deletions(-) > > > > >> > > > > >> -- > > > > >> 2.48.1 > > > > >> > > > > >> _______________________________________________ > > > > >> dev mailing list > > > > >> d...@openvswitch.org > > > > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > > > > > > > > > > Hi Numan, > > > > > > > > > > Sorry for the slow review. I have a general comment for this series. > > > > > I believe the exact same performance optimization is supposed to be > > > > > achieved by the below commit: > > > > > > > > > > 22298fd37 ovn-controller: Don't flood fill local datapaths beyond DGP > > > > > boundary. > > > > > > > > > > The scenarios are mentioned in detail in the commit message of that > > > > > patch, which is very similar to what you described here. > > > > > > > > > > The only exception was when there are distributed NAT or when the > > > > > redirection type is set to "bridged". In these cases it will fall > > > > > back to add peer datapath as local. See > > > > > SB:Port_Binding:options:always-redirect. Is this something that your > > > > > patch is targeting? Otherwise I wonder why it didn't work without > > > > > your patch. > > > > > > > > Hi Han, > > > > > > > > I wanted to mention in the commit message and in the cover letter that > > > > this patch series is an extension of the commit 22298fd37. I somehow > > > > missed it. > > > > > > > > Yes, I'm targeting the scenario of distributed NAT, > > > > > > > > > > I'd request to update the commit message and focus on how the patch > > > addresses the distributed NAT scenario. Otherwise it would be misleading, > > > since the problem doesn't exist for the scenario described in the current > > > commit message. > > > > Hi Han, > > > > Thanks for the comments. Sure I'll update the commit message. > > > > > > > > In addition, could you explain a little more about the solution. How does > > > the has_only_dgp_peer_ports address the distributed NAT scenario? For my > > > understanding, when there is distributed NAT associated to the LR, the > > > chassis needs to know the > > provider LS so that the NAT pipeline can work on the chassis. If the > > peer LS (provider LS) is not added just because of > > has_only_dgp_peer_ports, how would the distributed NAT work? > > > > [NUMAN[ The provider LS is added to the local data paths but it > > doesn't go beyond it. > > > > Let's take the topology in the commit message as an example. Let's say > > all the routers lr0, lr1, lr2.... have distributed nats. > > And let's say ovn-controller running on chassis 'A' claims the logical > > port of sw0. It starts by adding 'sw0' to the local_datapaths. > > Since add_local_datapath() traverses the topology through the > > connected peers, it will add lr0 to its local_datapaths. > > Since lr0 is connected to the provider LS public, it will also add > > "public" to its local_datapaths. > > The main thing added by this patch is that it stops at "public" LS if > > the option "pure_provider_switch" set on this datapath. > > > > Note: I'll be renaming the option to "pure_provider_switch" in the > > next version. > > > > So after claiming sw0's port, the local_datapaths map will have [sw0, > > lr0, public] > > Having the logical flows of these datapaths processed into openflows > > is enough for the distributed nats associated with the "sw0"'s logical > > ports to work. > > > > If this option was not set, then by the end of the > > add_local_datapath() finally returns to its original caller, the > > local_datapaths map would have [sw0, lr0, public, lr1, sw1, lr2, sw2, > > ....] > > > > Now let's say ovn-controller of chassis 'A' claims sw3's logical port, > > then it will first call add_local_datapath(sw3). And this will add > > [sw0, lr3] to the local_datapaths map. > > Note that public is already present in the local_datapaths. After > > this the local_datapaths map will look like - [sw0, lr0, public, sw3, > > lr3]. > > If suppose there are no more port bindings of sw3, then the patch also > > makes sure to remove sw3 and lr3 from the local_datapaths map. > > > > If you see the topology, I'm visualising that "public" LS is like a > > root node, and ovn-controller will start traversing from the leaf and > > stop at root. It will not traverse the other branches of "root" when > > it reaches it. > > > > Suppose if there is a logical port of "public" which ovn-controller > > claims, it adds "public" to the local datapaths and stops there. > > If the ovn-controller has not claimed any other datapath logical > > ports, then there is no need to add them to the local datapaths. > > If the logical port of "public" wants to reach other logical ports, > > the packet can traverse through the localnet port to the fabric and > > enter the other chassis where the DNAT IP resides. > > > > "pure_provider_switch" option indicates to ovn-controller that it's a > > "root" node. > > > > > > > > > I didn't see any handling in your change related to distributed NAT. I > > > believe I missed something, > > [Numan] It doesn't require any special handling for distributed NATs > > in ovn-controller. Your commit "22298fd37" will add the option > > "always-redirect" > > for "non distributed NATs" for the logical router datapath. And > > add_local_datapath() will NOT add "public" to its local datapaths map. > > However it will add "public" to its local datapath maps, if other > > logical routers connected to public have distributed NATs. > > > > The main difference between the two scenarios (i.e distributed NATs > > vs non distributed NATs) is that with distributed NATs, this patch > > adds the "public" LS to the local datapaths map and doesn't traverse > > further. > > Whereas with the other scenario, "public" LS is not added at all. > > > > Hi Numan, Thanks for the explanation. Now I see the difference between the > two scenarios. > > > > > because I saw your test case purposely used two LRs, one with > > distributed NAT and one without, and the one with distributed NAT did > > have provider LS added to the chassis, although I didn't find how that > > worked. It would be helpful for code review if you could highlight the > > related logic, probably also helpful for maintaining the code by > > adding related comments in the code for this specific consideration. > > > > Sure. I'll add more comments. > > > > > > > > > > On the other hand, I also have doubts about the criteria of deciding not > > > to add peers according to has_only_dgp_peer_ports. When the peer is a > > > provider LS and connected by DGP, we shouldn't add the peer, regardless > > > of whether there is another LR connected to the provider LS with a > > > distributed LRP, right? The provider LS (and the other LR, say, LR-X) > > > should be added to the chassis if there is a port-binding on the chassis > > > that is connected (directly or indirectly) to that LR-X, right? But > > > before this, I think I need to understand firstly how the distributed NAT > > > scenario is addressed by your patch. > > > > Sorry. I didn't get this question. Can you please see if my above > > comments addressed your questions ? If not can you please elaborate a > > bit further ? > > > > My question was wrong because I didn't distinguish the two scenarios until I > saw your explanation above. But still I have a related question to the > criteria of adding peers of the root (provider LS). In your implementation > the peer will be excluded (not flooded fill) only if the LS satisfies the > pure_provider_switch requirement: > > criteria: > - has one or more localnet ports. > - has one or more router ports and all its peers are distributed > gateway ports. > > I think this criteria is not very generic and hard to reason about.
I'd disagree. That is the case in our deployments. All the VPC logical routers are connected to just one provider logical switch, Even if you take the example of openstack neutron, generally all the tenant neutron routers are connected to an admin provider neutron network which provides the external connectivity. Let's say in your example topology one of the LRP connected to the public LS is fully distributed instead of DGP, but all the other LRPs are still DGP, then according to your criteria, every chassis will add all the DPs. However, in theory only the DPs connected by the distributed LR should be added, and the flood-fill should stop at all the DGPs when considering peers of the "public" LS, right? Correct. Can we change it to be more generic so that when considering a peer DP, if it is traversing from a LS to a LR, we handle it just like "always_redirect == true", while when traversing from a LR to a LS we need to honestly examine the "always_redirect" for distributed NAT. Should this work? I don't completely understand what you mean by handling " just like "always_redirect == true". From what I understand in the present code, we don't consider the peer datapath of the chassisredirect port if "always-redirect" == true. for distributed NAT, we don't set "always_redirect" to true for the chassisredirect port. Can you explain with an example topology ? For the scenario I mentioned, let's say "lr40" is a fully distributed router (with no DGP) and it's connected to the public switch. And if a compute node C1 claims a logical port of sw1, ideally I'd like to add [sw1, lr1, public, lr40, sw40] to its local datapaths i,.e lr40 and sw40 will be added to all the local datapaths of all the compute nodes if they claim any logical port of other switches. But adding the code to handle this will be very complicated and ideally I'd love to get rid of this "pure_provider_switch" option. I think it can be a future improvement. But I want to start with this. Thanks Numan > > Thanks, > Han > > > > > Thanks > > Numan > > > > > > > > Thanks, > > > Han > > > > > > > Numan > > > > > > > > > > > > > Could you help clarify? > > > > > > > > > > Thanks, > > > > > Han > > > > > > > > > >> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
