On Wed, Apr 2, 2025 at 10:48 PM Numan Siddique <[email protected]> wrote: > > > > numan > > On Wed, Apr 2, 2025, 7:45 PM Numan Siddique <[email protected]> wrote: >> >> On Mon, Mar 31, 2025 at 6:33 PM Han Zhou <[email protected]> wrote: >> > >> > >> > >> > On Thu, Mar 27, 2025 at 10:16 AM Numan Siddique <[email protected]> wrote: >> > > >> > > On Tue, Mar 25, 2025 at 9:06 AM Han Zhou <[email protected]> wrote: >> > > > >> > > > >> > > > >> > > > On Fri, Mar 14, 2025 at 12:35 AM Numan Siddique <[email protected]> wrote: >> > > > > >> > > > > On Thu, Mar 13, 2025 at 12:21 PM Han Zhou <[email protected]> wrote: >> > > > > > >> > > > > > >> > > > > > >> > > > > > On Sat, Mar 1, 2025 at 6:38 AM <[email protected]> wrote: >> > > > > >> >> > > > > >> From: Numan Siddique <[email protected]> >> > > > > >> >> > > > > >> 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 >> > > > > >> [email protected] >> > > > > >> 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 ? > > > > I think I understood what you mean here. I'm thinking on those lines and > will see if I can come up with something using this approach.
Hi Han, Thanks for the suggestions. I worked based on your suggestiond and submitted v3 - https://patchwork.ozlabs.org/project/ovn/list/?series=451205 Request to please take a look. With this approach we don't need to have the "pure_provider_switch" flag anymore. @Xavier Simonart - I incorporated your test help functions in v3 and I've added you as co-author and added your signed-off tag. Hope that's fine. Please let me know otherwise and please take a look at v3. Thanks Numan > > Thanks > Numan > >> > > >> > >> > 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
