On 3/1/23 23:42, Ilya Maximets wrote: > On 2/28/23 16:28, Dumitru Ceara wrote: >> On 2/28/23 16:22, Ilya Maximets wrote: >>> On 2/28/23 16:14, Dumitru Ceara wrote: >>>> On 2/28/23 15:29, Ilya Maximets wrote: >>>>> On 2/28/23 01:03, Dumitru Ceara wrote: >>>>>> Hi Ilya, >>>>>> >>>>>> Thanks for the patch, you're right the hairpin SNAT IP processing in >>>>>> ovn-controller is extremely inefficient today. >>>>>> >>>>>> I didn't review the code closely but I do have some initial remarks >>>>>> below. >>>>>> >>>>>> On 2/20/23 12:42, Ilya Maximets wrote: >>>>>>> It's common to have 'hairpin_snat_ip' to be configured exactly the >>>>>>> same for each and every Load Balancer in a setup. For example, >>>>>>> ovn-kubernetes does that, setting '169.254.169.5 fd69::5' value >>>>>>> unconditionally for every LB: >>>>>>> >>>>>>> https://github.com/ovn-org/ovn-kubernetes/blob/cd78ae1af4657d38bdc41003a8737aa958d62b9d/go-controller/pkg/ovn/controller/services/loadbalancer.go#L314 >>>>>>> >>>>>>> Current implementation of ovn-controller will create an OpenFlow rule >>>>>>> for every datapath for each VIP in case of 'hairpin_snat_ip', because >>>>>>> we need to handle the case where LBs with the same VIP are applied to >>>>>>> different datapaths and have different SNAT IPs. >>>>>> >>>>>> IMO the real issue is that we do this blindly for every datapath. It >>>>>> should actually be only for local datapaths. In ovn-kubernetes (which >>>>>> AFAIK is the only user of hairpin_snat_ip) there will only be a single >>>>>> local logical switch with LBs applied on it. >>>>> >>>>> True, but we will still need to check each datapath this LB is applied >>>>> to for being local, or the other way around, check that each local >>>>> datapath >>>>> is in the list of datapaths this LB is applied to. >>>>> It's definitely way less work than actually creating the flow, but might >>>>> still be not negligible. I didn't test though. >>>>> >>>> >>>> Ack, it would be great if we could get some data on this. Can we take a >>>> 500 node ovn-heater density-heavy NB database and just set >>>> hairpin_snat_ip on all load balancers to see what impact that has on >>>> ovn-controller? >>>> >>>>>> >>>>>>> >>>>>>> In large scale setups with tens of thousands of LBs and hundreds of >>>>>>> nodes, this generates millions of OpenFlow rules, making ovn-controller >>>>>>> choke and fall into unrecoverable disconnection loop with poll >>>>>>> intervals up to 200 seconds in ovn-heater's density-heavy scenario with >>>>>>> just 120 nodes. It also generates rules with thousands of conjunctions >>>>>>> that do not fit into a single FLOW_MOD, breaking the OpenFlow >>>>>>> management communication. >>>>>> >>>>>> These conjunctive flows were added with: >>>>>> >>>>>> 07467cfac499 ("lflow: Refactor OpenFlow snat hairpin flows") >>>>>> >>>>>> in order to reduce load in ovn-controller for the (now obsolete) use >>>>>> case ovn-kubernetes had. >>>>>> >>>>>> However, Han added: >>>>>> >>>>>> 22298fd37908 ("ovn-controller: Don't flood fill local datapaths beyond >>>>>> DGP boundary.") >>>>>> >>>>>> Which allowed ovn-kubernetes to essentially bind a logical switch to a >>>>>> chassis and which means that we will have at most one local datapath >>>>>> with load balancers applied to it. At this point, I suspect the >>>>>> original refactor patch could just be reverted. We would have to check >>>>>> that snat-ip flows (non-conjunctive) are being added only for the local >>>>>> datapath. >>>>> >>>>> This might be OK for ovn-kubernetes. But wouldn't this explode the >>>>> number of flows for OpenStack case? All datapaths can be local in >>>>> OpenStack, IIUC. So, we will go back to N_LBs * N_DPs * N_VIPs >>>>> number of OpenFlow rules. Or am I missing something? >>>>> >>>> >>>> OpenStack doesn't set hairpin_snat_ip today. >>> >>> But revert will impact not only hairpin_snat_ip case, it will impact >>> all the normal VIPs too. Wouldn't it? >>> >> >> True, but OpenStack doesn't apply the same LB to multiple logical >> switches (*) so the actual number of OpenFlows is probably at most N_LBs >> * N_VIPs. I suspect N_VIPs is 1 (or low) in most cases too. > > I posted a "v2" with alternative solution here: > > https://patchwork.ozlabs.org/project/ovn/patch/20230301223600.1607778-1-i.maxim...@ovn.org/ > > It seems that we can preserve current behavior for LBs without > hairpin_snat_ip and avoid affecting OpenStack. I hope, my > assumption is correct. :) >
Great, thanks a lot! I'll have a look at v2 first thing tomorrow. Regards, Dumitru _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
