Add support for Service Traffic Distribution in Antrea Proxy #6604

hongliangl · 2024-08-09T04:09:55Z

ServiceTrafficDistribution enables Traffic Distribution for Services in Antrea Proxy. This
feature allows for more flexible and intelligent routing decisions by considering both
topology and non-topology factors. For more details, refer to this link.

Note: To activate this feature in Antrea Proxy, Kubernetes must be version 1.30 or higher, with
the ServiceTrafficDistribution feature gate (a Kubernetes-specific feature gate) enabled to add
Endpoint zone hints in the Kubernetes EndpointSlice controller. Without these prerequisites,
the feature may not function as intended in Antrea Proxy.

antoninbas · 2024-08-09T20:06:14Z

pkg/agent/proxy/topology.go

-		if hintsAnnotation != "" && hintsAnnotation != "Disabled" && hintsAnnotation != "disabled" {
-			klog.InfoS("Skipping topology aware Endpoint filtering since Service has unexpected value", "annotationTopologyAwareHints", v1.DeprecatedAnnotationTopologyAwareHints, "hints", hintsAnnotation)
+
+	// Ignore the value of hintsAnnotation if the ServiceTrafficDistribution feature is enabled.


Do you know why the code is slightly different for kube-proxy? https://github.com/kubernetes/kubernetes/blob/6fc0a69044f1ac4c13841ec4391224a2df241460/pkg/proxy/topology.go#L153-L161

More generally, I wonder why this code is necessary for correctness. It seems to me that the real consumer of service.kubernetes.io/topology-mode and .spec.trafficDistribution is the EndpointSlice controller, and not the Service proxy. @tnqn do you know?

Good catch! The current code is wrong. I made #6607 to fix it.

@antoninbas both EndpointSlice controller and Service proxy are the consumers of the configuration.

EndpointSlice controller adds hints to each endpoint when the feature is enabled.

Service proxy consumes hints of endpoints when the feature is enabled, instead of unconditionally using the hints.

Ah, I think I get your point. You are talking about each Service's configuration, instead of the global configuration. I agree with you this looks duplicate, and inconsistent for service.kubernetes.io/topology-mode and .spec.trafficDistribution. Perhaps it's just because the feature is implemented by different authors.

Yes, I find the inconsistency strange and that's also related to my other comment. This seems to be the existing logic:

if both features are disabled, ignore the hints

if the TrafficDistribution feature is disabled (hence, the TopologyAwareHints feature is enabled), and we don't see an annotation, ignore the hints

otherwise, consume the hints

For consistency, IMO it should have been:

Consume the hints if and only if at least one the of the following conditions is true

the TrafficDistribution feature is enabled and .spec.trafficDistribution is set

the TopologyAwareHints feature is enabled and we see an annotation

However, I only see the current implementation being an issue if Feature Gates are set inconsistently between the proxy and the EndpointSlice controller. And we should probably just follow what kube-proxy is doing here.

the TrafficDistribution feature is enabled and .spec.trafficDistribution is set

One more thing, we need to bump k8s.io/api v0.29.2 (the version we use right now) to v0.30.x, than we can consume .spec.trafficDistribution in Antrea Proxy.

Since we have decided to keep the implementation consistent with kube-proxy, there is no need to consume .spec.trafficDistribution directly and we don't need to update K8s dependencies just yet.

antoninbas · 2024-08-09T20:34:59Z

pkg/agent/proxy/topology.go

-			klog.InfoS("Skipping topology aware Endpoint filtering since Service has unexpected value", "annotationTopologyAwareHints", v1.DeprecatedAnnotationTopologyAwareHints, "hints", hintsAnnotation)
+
+	// Ignore the value of hintsAnnotation if the ServiceTrafficDistribution feature is enabled.
+	if !p.serviceTrafficDistributionEnabled {


Again, this is what kube-proxy is doing, but it's confusing to me because serviceTrafficDistributionEnabled is a global setting (determined only by the feature gate). It seems that checking svc.Spec.TrafficDistribution would make more sense (specific to this Service). But maybe I am missing something and this is the desired / expected behavior.

I was also confused by that svc.Spec.TrafficDistribution is not consumed by kube-proxy code, and it is consumed by EndpointSlice controller code. If I understood correctly, the serviceTrafficDistributionEnabled is a global setting to decide whether to check svc.Spec.TrafficDistribution for a Service and reconcile hints for the corresponding EndpointSlice of the Service. https://github.com/kubernetes/kubernetes/blob/6cca485fe71dc5fb0cb8004b98d7aec792e9d791/staging/src/k8s.io/endpointslice/trafficdist/trafficdist.go#L28

tnqn · 2024-08-19T13:10:34Z

docs/feature-gates.md

@@ -35,6 +35,7 @@ edit the Agent configuration in the
 | `AntreaProxy`                 | Agent              | `true`  | GA    | v0.8          | v0.11        | v1.14      | Yes                | Must be enabled for Windows.                  |
 | `EndpointSlice`               | Agent              | `true`  | GA    | v0.13.0       | v1.11        | v1.14      | Yes                |                                               |
 | `TopologyAwareHints`          | Agent              | `true`  | Beta  | v1.8          | v1.12        | N/A        | Yes                |                                               |
+| `ServiceTrafficDistribution`  | Agent              | `true`  | Alpha | v2.2          | N/A          | N/A        | Yes                |                                               |


The default is true? If the change is straightfoward and we are confident in its quality and want it to be enabled by default, the stage should be Beta to be consistent.

+1 I personally support going straight to Beta and enabling it by default.
K8s 1.31 was just released and the feature has been promoted already

My bad, this should be false. I copied this from the line above it and forgot to modify the default value.

@hongliangl It may still make sense to introduce this as Beta right away:

there is no requirement that feature gates always go through Alpha stage

code changes are small and mostly ported over from kube-proxy

this change should have no impact unless users set .spec.trafficDistribution

the feature gate is already Beta / enabled by default in the latest K8s version, as it only stayed in Alpha stage for one minor release

It was done as you said.

`ServiceTrafficDistribution` enables Traffic Distribution for Services in Antrea Proxy. This feature allows for more flexible and intelligent routing decisions by considering both topology and non-topology factors. For more details, refer to this [link](https://github.com/kubernetes/enhancements/tree/master/keps/sig-network/4444-service-traffic-distribution). Note: To activate this feature in Antrea Proxy, Kubernetes must be version 1.30 or higher, with the `ServiceTrafficDistribution` feature gate (a Kubernetes-specific feature gate) enabled to add Endpoint zone hints in the Kubernetes EndpointSlice controller. Without these prerequisites, the feature may not function as intended in Antrea Proxy. Signed-off-by: Hongliang Liu <lhongliang@vmware.com>

tnqn

LGTM

tnqn · 2024-08-22T13:07:35Z

/test-all

hongliangl added the action/release-note Indicates a PR that should be included in release notes. label Aug 9, 2024

hongliangl added this to the Antrea v2.2 release milestone Aug 9, 2024

hongliangl requested a review from antoninbas August 9, 2024 04:09

hongliangl force-pushed the 20240808-support-svc-traffic-distribute branch 6 times, most recently from bde680b to 4a6d33d Compare August 9, 2024 07:12

antoninbas reviewed Aug 9, 2024

View reviewed changes

antoninbas requested a review from tnqn August 9, 2024 20:35

hongliangl force-pushed the 20240808-support-svc-traffic-distribute branch from 4a6d33d to b6f6fd0 Compare August 14, 2024 07:28

tnqn reviewed Aug 19, 2024

View reviewed changes

hongliangl force-pushed the 20240808-support-svc-traffic-distribute branch from b6f6fd0 to c8e76c0 Compare August 22, 2024 03:19

hongliangl requested review from antoninbas and tnqn August 22, 2024 03:21

hongliangl force-pushed the 20240808-support-svc-traffic-distribute branch from c8e76c0 to 19c6c43 Compare August 22, 2024 03:57

tnqn approved these changes Aug 22, 2024

View reviewed changes

antoninbas approved these changes Aug 22, 2024

View reviewed changes

antoninbas merged commit b352435 into antrea-io:main Aug 22, 2024
52 of 58 checks passed

hongliangl deleted the 20240808-support-svc-traffic-distribute branch August 23, 2024 03:02

antoninbas mentioned this pull request Sep 20, 2024

Same topology.go info message keeps being logged by Antrea Agent #6682

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Service Traffic Distribution in Antrea Proxy #6604

Add support for Service Traffic Distribution in Antrea Proxy #6604

hongliangl commented Aug 9, 2024

antoninbas Aug 9, 2024

antoninbas Aug 9, 2024

hongliangl Aug 13, 2024

tnqn Aug 13, 2024

tnqn Aug 13, 2024

antoninbas Aug 19, 2024

hongliangl Aug 20, 2024 •

edited

Loading

antoninbas Aug 20, 2024

antoninbas Aug 9, 2024

hongliangl Aug 13, 2024

tnqn Aug 19, 2024

antoninbas Aug 19, 2024

hongliangl Aug 20, 2024

antoninbas Aug 20, 2024

hongliangl Aug 22, 2024

tnqn left a comment

tnqn commented Aug 22, 2024

Add support for Service Traffic Distribution in Antrea Proxy #6604

Add support for Service Traffic Distribution in Antrea Proxy #6604

Conversation

hongliangl commented Aug 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hongliangl Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Aug 22, 2024

hongliangl Aug 20, 2024 •

edited

Loading