On 7/7/22 13:45, Dumitru Ceara wrote:
> On 7/7/22 00:08, Han Zhou wrote:
>> On Wed, Jul 6, 2022 at 8:45 AM Dumitru Ceara <dce...@redhat.com> wrote:
>>>
>>> Hi Han,
>>>
>>> On 7/6/22 00:41, Han Zhou wrote:
>>>> The ls_in_pre_stateful priority 120 flow that saves dst IP and Port to
>>>> registers is causing a critical dataplane performance impact to
>>>> short-lived connections, because it unwildcards megaflows with exact
>>>> match on dst IP and L4 ports. Any new connections with a different
>>>> client side L4 port will encounter datapath flow miss and upcall to
>>>> ovs-vswitchd.
>>>>
>>>> These fields (dst IP and port) were saved to registers to solve a
>>>> problem of LB hairpin use case when different VIPs are sharing
>>>> overlapping backend+port [0]. The change [0] might not have as wide
>>>> performance impact as it is now because at that time one of the match
>>>> condition "REGBIT_CONNTRACK_NAT == 1" was set only for established and
>>>> natted traffic, while now the impact is more obvious because
>>>> REGBIT_CONNTRACK_NAT is now set for all IP traffic (if any VIP
>>>> configured on the LS) since commit [1], after several other indirectly
>>>> related optimizations and refactors.
>>>>
>>>> Since the changes that introduced the performance problem had their
>>>> own values (fixes problems or optimizes performance), so we don't want
>>>> to revert any of the changes (and it is also not straightforward to
>>>> revert any of them because there have been lots of changes and refactors
>>>> on top of them).
>>>>
>>>> Change [0] itself has added an alternative way to solve the overlapping
>>>> backends problem, which utilizes ct fields instead of saving dst IP and
>>>> port to registers. This patch forces to that approach and removes the
>>>> flows/actions that saves the dst IP and port to avoid the dataplane
>>>> performance problem for short-lived connections.
>>>>
>>>> (With this approach, the ct_state DNAT is not HW offload friendly, so it
>>>> may result in those flows not being offloaded, which is supposed to be
>>>> solved in a follow-up patch)
>>>>
>>>> [0] ce0ef8d59850 ("Properly handle hairpin traffic for VIPs with shared
>> backends.")
>>>> [1] 0038579d1928 ("northd: Optimize ct nat for load balancer traffic.")
>>>>
>>>> Signed-off-by: Han Zhou <hz...@ovn.org>
>>>> ---
>>>
>>> I think the main concern I have is that this forces us to choose between:
>>> a. non hwol friendly flows (reduced performance)
>>> b. less functionality (with the knob in patch 3/3 set to false).
>>>
>> Thanks Dumitru for the comments! I agree the solution is not ideal, but if
>> we look at it from a different angle, even with a), for most pod->service
>> traffic the performance is still much better than how it is today (not
>> offloaded kernel datapath is still much better than userspace slowpath).
>> And *hopefully* b) is ok for most use cases to get HW-offload capability.
>>Just a note on this item. I'm a bit confused about why all traffic would be slowpath-ed? It's just the first packet that goes to vswitchd as an upcall, right? Once the megaflow (even if it's more specific than ideal) is installed all following traffic in that session should be forwarded in fast path (kernel). Also, I'm not sure I follow why the same behavior wouldn't happen with your changes too for pod->service. The datapath flow includes the dp_hash() match, and that's likely different for different connections. Or am I missing something? >>> Change [0] was added to address the case when a service in kubernetes is >>> exposed via two different k8s services objects that share the same >>> endpoints. That translates in ovn-k8s to two different OVN load >>> balancer VIPs that share the same backends. For such cases, if the >>> service is being accessed by one of its own backends we need to be able >>> to differentiate based on the VIP address it used to connect to. >>> >>> CC: Tim Rozet, Dan Williams for some more input from the ovn-k8s side on >>> how common it is that an OVN-networked pod accesses two (or more) >>> services that might have the pod itself as a backend. >>> >> >> Yes, we definitely need input from ovn-k8s side. The information we got so >> far: the change [0] was to fix a bug [2] reported by Tim. However, the bug >> description didn't mention anything about two VIPs sharing the same >> backend. Tim also mentioned in the ovn-k8s meeting last week that the >> original user bug report for [2] was [3], and [3] was in fact a completely >> different problem (although it is related to hairpin, too). So, I am under > > I am not completely sure about the link between [3] and [2], maybe Tim > remembers more. > >> the impression that "an OVN-networked pod accesses two (or more) services >> that might have the pod itself as a backend" might be a very rare use case, >> if it exists at all. >> > > I went ahead and set the new ovn-allow-vips-share-hairpin-backend knob > to "false" and pushed it to my fork to run the ovn-kubernetes CI. This > runs a subset of the kubernetes conformance tests (AFAICT) and some > specific e2e ovn-kubernetes tests. > > The results are here: > > https://github.com/dceara/ovn/runs/7230840427?check_suite_focus=true > > Focusing on the conformance failures: > > 2022-07-07T10:31:24.7228157Z [91m[1m[Fail] [0m[90m[sig-network] > Networking [0m[0mGranular Checks: Services [0m[91m[1m[It] should > function for endpoint-Service: http [0m > 2022-07-07T10:31:24.7228940Z > [37mvendor/github.com/onsi/ginkgo/internal/leafnodes/runner.go:113[0m > ... > 2022-07-07T10:31:24.7240313Z [91m[1m[Fail] [0m[90m[sig-network] > Networking [0m[0mGranular Checks: Services [0m[91m[1m[It] should > function for multiple endpoint-Services with same selector [0m > 2022-07-07T10:31:24.7240819Z > [37mvendor/github.com/onsi/ginkgo/internal/leafnodes/runner.go:113[0m > ... > > Checking how these tests are defined: > https://github.com/kubernetes/kubernetes/blob/2a017f94bcf8d04cbbbbdc6695bcf74273d630ed/test/e2e/network/networking.go#L283 > https://github.com/kubernetes/kubernetes/blob/2a017f94bcf8d04cbbbbdc6695bcf74273d630ed/test/e2e/network/networking.go#L236 > > It seems to me that they're testing explicitly for a "pod that accesses > two services that might have the pod itself as a backend". > > So, if I'm not wrong, we'd become non-compliant in this case. > >>> If this turns out to be mandatory I guess we might want to also look >>> into alternatives like: >>> - getting help from the HW to offload matches like ct_tuple() >> >> I believe this is going to happen in the future. HWOL is continuously >> enhanced. >> > > That would make things simpler. > >>> - limiting the impact of "a." only to some load balancers (e.g., would >>> it help to use different hairpin lookup tables for such load balancers?) >> >> I am not sure if this would work, and not sure if this is a good approach, >> either. In general, I believe it is possible to solve the problem with more >> complex pipelines, but we need to keep in mind it is quite easy to >> introduce other performance problems (either control plane or data plane) - >> many of the changes lead to the current implementation were for performance >> optimizations, some for control plane, some for HWOL, and some for reducing >> recirculations. I'd avoid complexity unless it is really necessary. Let's >> get more input for the problem, and based on that we can decide if we want >> to move to a more complex solution. >> > > Sure, I agree we need to find the best solution. > > In my option OVN should be HW-agnostic. We did try to adjust the way > OVN generates openflows in order to make it more "HWOL-friendly" but > that shouldn't come with the cost of breaking CMS features (if that's > the case here). > >> [2] https://bugzilla.redhat.com/show_bug.cgi?id=1931599 >> [3] https://bugzilla.redhat.com/show_bug.cgi?id=1903651 >> >> Thanks, >> Han >> > > Thanks, > Dumitru _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
