Introducing Topology Mode Annotation, Deprecating Topology Hints

Annotation

As part of this change, kube-proxy accepts any value for either
annotation that is not "disabled".

Change-Id: Idfc26eb4cc97ff062649dc52ed29823a64fc59a4

pkg/apis/core/annotation_key_constants.go

@@ -122,8 +122,24 @@ const (
 	// This annotation is beta-level and is only honored when PodDeletionCost feature is enabled.
 	PodDeletionCost = "controller.kubernetes.io/pod-deletion-cost"
 
-	// AnnotationTopologyAwareHints can be used to enable or disable Topology
+	// DeprecatedAnnotationTopologyAwareHints can be used to enable or disable
-	// Aware Hints for a Service. This may be set to "Auto" or "Disabled". Any
+	// Topology Aware Hints for a Service. This may be set to "Auto" or
-	// other value is treated as "Disabled".
+	// "Disabled". Any other value is treated as "Disabled". This annotation has
-	AnnotationTopologyAwareHints = "service.kubernetes.io/topology-aware-hints"
+	// been deprecated in favor of the `service.kubernetes.io/topology-mode`
+	// annotation which also allows "Auto" and "Disabled", but is not limited to
+	// those (it's open ended to provide room for experimentation while we
+	// pursue configuration for topology via specification). When both
+	// `service.kubernetes.io/topology-aware-hints` and
+	// `service.kubernetes.io/topology-mode` annotations are set, the value of
+	// `service.kubernetes.io/topology-aware-hints` has precedence.
+	DeprecatedAnnotationTopologyAwareHints = "service.kubernetes.io/topology-aware-hints"
+
+	// AnnotationTopologyMode can be used to enable or disable Topology Aware
+	// Routing for a Service. Well known values are "Auto" and "Disabled".
+	// Implementations may choose to develop new topology approaches, exposing
+	// them with domain-prefixed values. For example, "example.com/lowest-rtt"
+	// could be a valid implementation-specific value for this annotation. These
+	// heuristics will often populate topology hints on EndpointSlices, but that
+	// is not a requirement.
+	AnnotationTopologyMode = "service.kubernetes.io/topology-mode"
 )

pkg/controller/endpointslice/reconciler_test.go

@@ -1843,7 +1843,7 @@ func TestReconcileTopology(t *testing.T) {
 
 			service := svc.DeepCopy()
 			service.Annotations = map[string]string{
-				corev1.AnnotationTopologyAwareHints: tc.hintsAnnotation,
+				corev1.DeprecatedAnnotationTopologyAwareHints: tc.hintsAnnotation,
 			}
 			r.reconcile(service, tc.pods, tc.existingSlices, time.Now())
 

pkg/controller/endpointslice/utils.go

@@ -378,13 +378,18 @@ func unchangedSlices(existingSlices, slicesToUpdate, slicesToDelete []*discovery
 	return unchangedSlices
 }
 
-// hintsEnabled returns true if the provided annotations include a
+// hintsEnabled returns true if the provided annotations include either
-// v1.AnnotationTopologyAwareHints key with a value set to "Auto" or "auto".
+// v1.AnnotationTopologyMode or v1.DeprecatedAnnotationTopologyAwareHints key
+// with a value set to "Auto" or "auto". When both are set,
+// v1.DeprecatedAnnotationTopologyAwareHints has precedence.
 func hintsEnabled(annotations map[string]string) bool {
-	val, ok := annotations[v1.AnnotationTopologyAwareHints]
+	val, ok := annotations[v1.DeprecatedAnnotationTopologyAwareHints]
+	if !ok {
+		val, ok = annotations[v1.AnnotationTopologyMode]
 		if !ok {
 			return false
 		}
+	}
 	return val == "Auto" || val == "auto"
 }
 

pkg/controller/endpointslice/utils_test.go

@@ -1149,24 +1149,72 @@ func Test_hintsEnabled(t *testing.T) {
 		annotations:   map[string]string{"topology-hints": "enabled"},
 		expectEnabled: false,
 	}, {
-		name:          "annotation == enabled",
+		name:          "hints annotation == enabled",
-		annotations:   map[string]string{v1.AnnotationTopologyAwareHints: "enabled"},
+		annotations:   map[string]string{v1.DeprecatedAnnotationTopologyAwareHints: "enabled"},
 		expectEnabled: false,
 	}, {
-		name:          "annotation == aUto",
+		name:          "hints annotation == aUto",
-		annotations:   map[string]string{v1.AnnotationTopologyAwareHints: "aUto"},
+		annotations:   map[string]string{v1.DeprecatedAnnotationTopologyAwareHints: "aUto"},
 		expectEnabled: false,
 	}, {
-		name:          "annotation == auto",
+		name:          "hints annotation == auto",
-		annotations:   map[string]string{v1.AnnotationTopologyAwareHints: "auto"},
+		annotations:   map[string]string{v1.DeprecatedAnnotationTopologyAwareHints: "auto"},
 		expectEnabled: true,
 	}, {
-		name:          "annotation == Auto",
+		name:          "hints annotation == Auto",
-		annotations:   map[string]string{v1.AnnotationTopologyAwareHints: "Auto"},
+		annotations:   map[string]string{v1.DeprecatedAnnotationTopologyAwareHints: "Auto"},
 		expectEnabled: true,
 	}, {
-		name:          "annotation == disabled",
+		name:          "hints annotation == disabled",
-		annotations:   map[string]string{v1.AnnotationTopologyAwareHints: "disabled"},
+		annotations:   map[string]string{v1.DeprecatedAnnotationTopologyAwareHints: "disabled"},
+		expectEnabled: false,
+	}, {
+		name:          "mode annotation == enabled",
+		annotations:   map[string]string{v1.AnnotationTopologyMode: "enabled"},
+		expectEnabled: false,
+	}, {
+		name:          "mode annotation == aUto",
+		annotations:   map[string]string{v1.AnnotationTopologyMode: "aUto"},
+		expectEnabled: false,
+	}, {
+		name:          "mode annotation == auto",
+		annotations:   map[string]string{v1.AnnotationTopologyMode: "auto"},
+		expectEnabled: true,
+	}, {
+		name:          "mode annotation == Auto",
+		annotations:   map[string]string{v1.AnnotationTopologyMode: "Auto"},
+		expectEnabled: true,
+	}, {
+		name:          "mode annotation == disabled",
+		annotations:   map[string]string{v1.AnnotationTopologyMode: "disabled"},
+		expectEnabled: false,
+	}, {
+		name:          "mode annotation == enabled",
+		annotations:   map[string]string{v1.AnnotationTopologyMode: "enabled"},
+		expectEnabled: false,
+	}, {
+		name:          "mode annotation == aUto",
+		annotations:   map[string]string{v1.AnnotationTopologyMode: "aUto"},
+		expectEnabled: false,
+	}, {
+		name:          "mode annotation == auto",
+		annotations:   map[string]string{v1.AnnotationTopologyMode: "auto"},
+		expectEnabled: true,
+	}, {
+		name:          "mode annotation == Auto",
+		annotations:   map[string]string{v1.AnnotationTopologyMode: "Auto"},
+		expectEnabled: true,
+	}, {
+		name:          "mode annotation == disabled",
+		annotations:   map[string]string{v1.AnnotationTopologyMode: "disabled"},
+		expectEnabled: false,
+	}, {
+		name:          "mode annotation == disabled, hints annotation == auto",
+		annotations:   map[string]string{v1.AnnotationTopologyMode: "disabled", v1.DeprecatedAnnotationTopologyAwareHints: "auto"},
+		expectEnabled: true,
+	}, {
+		name:          "mode annotation == auto, hints annotation == disabled",
+		annotations:   map[string]string{v1.AnnotationTopologyMode: "auto", v1.DeprecatedAnnotationTopologyAwareHints: "disabled"},
 		expectEnabled: false,
 	}}
 	for _, tc := range testCases {

pkg/proxy/service.go

@@ -176,7 +176,13 @@ func (sct *ServiceChangeTracker) newBaseServiceInfo(port *v1.ServicePort, servic
 		externalPolicyLocal:   externalPolicyLocal,
 		internalPolicyLocal:   internalPolicyLocal,
 		internalTrafficPolicy: service.Spec.InternalTrafficPolicy,
-		hintsAnnotation:       service.Annotations[v1.AnnotationTopologyAwareHints],
+	}
+
+	// v1.DeprecatedAnnotationTopologyAwareHints has precedence over v1.AnnotationTopologyMode.
+	var ok bool
+	info.hintsAnnotation, ok = service.Annotations[v1.DeprecatedAnnotationTopologyAwareHints]
+	if !ok {
+		info.hintsAnnotation, _ = service.Annotations[v1.AnnotationTopologyMode]
 	}
 
 	loadBalancerSourceRanges := make([]string, len(service.Spec.LoadBalancerSourceRanges))

pkg/proxy/topology.go

@@ -148,11 +148,9 @@ func canUseTopology(endpoints []Endpoint, svcInfo ServicePort, nodeLabels map[st
 	if !utilfeature.DefaultFeatureGate.Enabled(features.TopologyAwareHints) {
 		return false
 	}
+	// Any non-empty and non-disabled values for the hints annotation are acceptable.
 	hintsAnnotation := svcInfo.HintsAnnotation()
-	if hintsAnnotation != "Auto" && hintsAnnotation != "auto" {
+	if hintsAnnotation == "" || hintsAnnotation == "disabled" || hintsAnnotation == "Disabled" {
-		if hintsAnnotation != "" && hintsAnnotation != "Disabled" && hintsAnnotation != "disabled" {
-			klog.InfoS("Skipping topology aware endpoint filtering since Service has unexpected value", "annotationTopologyAwareHints", v1.AnnotationTopologyAwareHints, "hints", hintsAnnotation)
-		}
 		return false
 	}
 

pkg/proxy/topology_test.go

@@ -105,7 +105,7 @@ func TestCategorizeEndpoints(t *testing.T) {
 		localEndpoints:   nil,
 	}, {
 
-		name:         "hints, hints annotation == aUto (wrong capitalization), hints ignored",
+		name:         "hints, hints annotation == aUto (wrong capitalization), hints no longer ignored",
 		hintsEnabled: true,
 		nodeLabels:   map[string]string{v1.LabelTopologyZone: "zone-a"},
 		serviceInfo:  &BaseServicePortInfo{hintsAnnotation: "aUto"},
@@ -115,7 +115,7 @@ func TestCategorizeEndpoints(t *testing.T) {
 			&BaseEndpointInfo{Endpoint: "10.1.2.5:80", ZoneHints: sets.NewString("zone-c"), Ready: true},
 			&BaseEndpointInfo{Endpoint: "10.1.2.6:80", ZoneHints: sets.NewString("zone-a"), Ready: true},
 		},
-		clusterEndpoints: sets.NewString("10.1.2.3:80", "10.1.2.4:80", "10.1.2.5:80", "10.1.2.6:80"),
+		clusterEndpoints: sets.NewString("10.1.2.3:80", "10.1.2.6:80"),
 		localEndpoints:   nil,
 	}, {
 		name:         "hints, hints annotation empty, hints ignored",

pkg/proxy/types.go

@@ -89,7 +89,7 @@ type ServicePort interface {
 	InternalPolicyLocal() bool
 	// InternalTrafficPolicy returns service InternalTrafficPolicy
 	InternalTrafficPolicy() *v1.ServiceInternalTrafficPolicy
-	// HintsAnnotation returns the value of the v1.AnnotationTopologyAwareHints annotation.
+	// HintsAnnotation returns the value of the v1.DeprecatedAnnotationTopologyAwareHints annotation.
 	HintsAnnotation() string
 	// ExternallyAccessible returns true if the service port is reachable via something
 	// other than ClusterIP (NodePort/ExternalIP/LoadBalancer)

staging/src/k8s.io/api/core/v1/annotation_key_constants.go

@@ -144,8 +144,19 @@ const (
 	// This annotation is beta-level and is only honored when PodDeletionCost feature is enabled.
 	PodDeletionCost = "controller.kubernetes.io/pod-deletion-cost"
 
-	// AnnotationTopologyAwareHints can be used to enable or disable Topology
+	// DeprecatedAnnotationTopologyAwareHints can be used to enable or disable
-	// Aware Hints for a Service. This may be set to "Auto" or "Disabled". Any
+	// Topology Aware Hints for a Service. This may be set to "Auto" or
-	// other value is treated as "Disabled".
+	// "Disabled". Any other value is treated as "Disabled". This annotation has
-	AnnotationTopologyAwareHints = "service.kubernetes.io/topology-aware-hints"
+	// been deprecated in favor of the "service.kubernetes.io/topology-mode"
+	// annotation.
+	DeprecatedAnnotationTopologyAwareHints = "service.kubernetes.io/topology-aware-hints"
+
+	// AnnotationTopologyMode can be used to enable or disable Topology Aware
+	// Routing for a Service. Well known values are "Auto" and "Disabled".
+	// Implementations may choose to develop new topology approaches, exposing
+	// them with domain-prefixed values. For example, "example.com/lowest-rtt"
+	// could be a valid implementation-specific value for this annotation. These
+	// heuristics will often populate topology hints on EndpointSlices, but that
+	// is not a requirement.
+	AnnotationTopologyMode = "service.kubernetes.io/topology-mode"
 )

test/e2e/network/topology_hints.go

@@ -65,7 +65,7 @@ var _ = common.SIGDescribe("[Feature:Topology Hints]", func() {
 			ObjectMeta: metav1.ObjectMeta{
 				Name: "topology-hints",
 				Annotations: map[string]string{
-					v1.AnnotationTopologyAwareHints: "Auto",
+					v1.AnnotationTopologyMode: "Auto",
 				},
 			},
 			Spec: v1.ServiceSpec{