Merge pull request #100728 from robscott/topology-auto

Updating Topology Aware Hints to support "Auto" value for annotation

pkg/apis/core/annotation_key_constants.go

@@ -126,7 +126,7 @@ const (
 	PodDeletionCost = "controller.kubernetes.io/pod-deletion-cost"
 
 	// AnnotationTopologyAwareHints can be used to enable or disable Topology
-	// Aware Hints for a Service. This may be set to "auto" or "disabled". Any
+	// Aware Hints for a Service. This may be set to "Auto" or "Disabled". Any
-	// other value is treated as "disabled".
+	// other value is treated as "Disabled".
 	AnnotationTopologyAwareHints = "service.kubernetes.io/topology-aware-hints"
 )

pkg/controller/endpointslice/metrics/metrics.go

@@ -102,7 +102,7 @@ var (
 			Name:      "endpointslices_changed_per_sync",
 			Help:      "Number of EndpointSlices changed on each Service sync",
 		},
-		[]string{"topology"}, // either "auto" or "disabled"
+		[]string{"topology"}, // either "Auto" or "Disabled"
 	)
 
 	// EndpointSliceSyncs tracks the number of sync operations the controller

pkg/controller/endpointslice/reconciler.go

@@ -330,9 +330,9 @@ func (r *reconciler) finalize(
 		metrics.EndpointSliceChanges.WithLabelValues("delete").Inc()
 	}
 
-	topologyLabel := "disabled"
+	topologyLabel := "Disabled"
 	if r.topologyCache != nil && hintsEnabled(service.Annotations) {
-		topologyLabel = "auto"
+		topologyLabel = "Auto"
 	}
 
 	numSlicesChanged := len(slicesToCreate) + len(slicesToUpdate) + len(slicesToDelete)

pkg/controller/endpointslice/reconciler_test.go

@@ -1460,9 +1460,9 @@ func TestReconcileTopology(t *testing.T) {
 			slicesChangedPerSyncTopology: 0,
 		},
 	}, {
-		name:                 "topology enabled, hintsAnnotation==auto, more slices and endpoints",
+		name:                 "topology enabled, hintsAnnotation==Auto, more slices and endpoints",
 		topologyCacheEnabled: true,
-		hintsAnnotation:      "auto",
+		hintsAnnotation:      "Auto",
 		existingSlices:       []*discovery.EndpointSlice{slicesByName["zone-a-c"], slicesByName["zone-a-b"]},
 		pods:                 append(slicePods["zone-a-c"], slicePods["zone-a-b"]...),
 		nodes:                nodes,
@@ -1784,13 +1784,13 @@ func expectMetrics(t *testing.T, em expectedMetrics) {
 		t.Errorf("Expected endpointSliceChangesDeleted to be %d, got %v", em.numDeleted, actualDeleted)
 	}
 
-	actualSlicesChangedPerSync, err := testutil.GetHistogramMetricValue(metrics.EndpointSlicesChangedPerSync.WithLabelValues("disabled"))
+	actualSlicesChangedPerSync, err := testutil.GetHistogramMetricValue(metrics.EndpointSlicesChangedPerSync.WithLabelValues("Disabled"))
 	handleErr(t, err, "slicesChangedPerSync")
 	if actualSlicesChangedPerSync != float64(em.slicesChangedPerSync) {
 		t.Errorf("Expected slicesChangedPerSync to be %d, got %v", em.slicesChangedPerSync, actualSlicesChangedPerSync)
 	}
 
-	actualSlicesChangedPerSyncTopology, err := testutil.GetHistogramMetricValue(metrics.EndpointSlicesChangedPerSync.WithLabelValues("auto"))
+	actualSlicesChangedPerSyncTopology, err := testutil.GetHistogramMetricValue(metrics.EndpointSlicesChangedPerSync.WithLabelValues("Auto"))
 	handleErr(t, err, "slicesChangedPerSyncTopology")
 	if actualSlicesChangedPerSyncTopology != float64(em.slicesChangedPerSyncTopology) {
 		t.Errorf("Expected slicesChangedPerSyncTopology to be %d, got %v", em.slicesChangedPerSyncTopology, actualSlicesChangedPerSyncTopology)
@@ -1825,8 +1825,8 @@ func setupMetrics() {
 	metrics.EndpointSliceChanges.Delete(map[string]string{"operation": "create"})
 	metrics.EndpointSliceChanges.Delete(map[string]string{"operation": "update"})
 	metrics.EndpointSliceChanges.Delete(map[string]string{"operation": "delete"})
-	metrics.EndpointSlicesChangedPerSync.Delete(map[string]string{"topology": "disabled"})
+	metrics.EndpointSlicesChangedPerSync.Delete(map[string]string{"topology": "Disabled"})
-	metrics.EndpointSlicesChangedPerSync.Delete(map[string]string{"topology": "auto"})
+	metrics.EndpointSlicesChangedPerSync.Delete(map[string]string{"topology": "Auto"})
 	metrics.EndpointSliceSyncs.Delete(map[string]string{"result": "success"})
 	metrics.EndpointSliceSyncs.Delete(map[string]string{"result": "error"})
 }

pkg/controller/endpointslice/utils.go

@@ -386,11 +386,11 @@ func unchangedSlices(existingSlices, slicesToUpdate, slicesToDelete []*discovery
 }
 
 // hintsEnabled returns true if the provided annotations include a
-// corev1.AnnotationTopologyAwareHints key with a value set to "auto".
+// corev1.AnnotationTopologyAwareHints key with a value set to "Auto" or "auto".
 func hintsEnabled(annotations map[string]string) bool {
 	val, ok := annotations[corev1.AnnotationTopologyAwareHints]
 	if !ok {
 		return false
 	}
-	return val == "auto"
+	return val == "Auto" || val == "auto"
 }

pkg/controller/endpointslice/utils_test.go

@@ -1165,3 +1165,46 @@ func TestSupportedServiceAddressType(t *testing.T) {
 		})
 	}
 }
+
+func Test_hintsEnabled(t *testing.T) {
+	testCases := []struct {
+		name          string
+		annotations   map[string]string
+		expectEnabled bool
+	}{{
+		name:          "empty annotations",
+		expectEnabled: false,
+	}, {
+		name:          "different annotations",
+		annotations:   map[string]string{"topology-hints": "enabled"},
+		expectEnabled: false,
+	}, {
+		name:          "annotation == enabled",
+		annotations:   map[string]string{v1.AnnotationTopologyAwareHints: "enabled"},
+		expectEnabled: false,
+	}, {
+		name:          "annotation == aUto",
+		annotations:   map[string]string{v1.AnnotationTopologyAwareHints: "aUto"},
+		expectEnabled: false,
+	}, {
+		name:          "annotation == auto",
+		annotations:   map[string]string{v1.AnnotationTopologyAwareHints: "auto"},
+		expectEnabled: true,
+	}, {
+		name:          "annotation == Auto",
+		annotations:   map[string]string{v1.AnnotationTopologyAwareHints: "Auto"},
+		expectEnabled: true,
+	}, {
+		name:          "annotation == disabled",
+		annotations:   map[string]string{v1.AnnotationTopologyAwareHints: "disabled"},
+		expectEnabled: false,
+	}}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			actualEnabled := hintsEnabled(tc.annotations)
+			if actualEnabled != tc.expectEnabled {
+				t.Errorf("Expected %t, got %t", tc.expectEnabled, actualEnabled)
+			}
+		})
+	}
+}

pkg/proxy/topology.go

@@ -49,15 +49,15 @@ func FilterEndpoints(endpoints []Endpoint, svcInfo ServicePort, nodeLabels map[s
 // filterEndpointsWithHints provides filtering based on the hints included in
 // EndpointSlices. If any of the following are true, the full list of endpoints
 // will be returned without any filtering:
-// * The AnnotationTopologyAwareHints annotation is not set to "auto" for this
+// * The AnnotationTopologyAwareHints annotation is not set to "Auto" for this
 //   Service.
 // * No zone is specified in node labels.
 // * No endpoints for this Service have a hint pointing to the zone this
 //   instance of kube-proxy is running in.
 // * One or more endpoints for this Service do not have hints specified.
 func filterEndpointsWithHints(endpoints []Endpoint, hintsAnnotation string, nodeLabels map[string]string) []Endpoint {
-	if hintsAnnotation != "auto" {
+	if hintsAnnotation != "Auto" && hintsAnnotation != "auto" {
-		if hintsAnnotation != "" && hintsAnnotation != "disabled" {
+		if hintsAnnotation != "" && hintsAnnotation != "Disabled" && hintsAnnotation != "disabled" {
 			klog.Warningf("Skipping topology aware endpoint filtering since Service has unexpected value for %s annotation: %s", v1.AnnotationTopologyAwareHints, hintsAnnotation)
 		}
 		return endpoints

pkg/proxy/topology_test.go

@@ -77,11 +77,11 @@ func TestFilterEndpoints(t *testing.T) {
 			{ip: "10.1.2.6", zoneHints: sets.NewString("zone-a")},
 		},
 	}, {
-		name:               "hints + eps proxying enabled, hints annotation == Auto (wrong capitalization), hints ignored",
+		name:               "hints + eps proxying enabled, hints annotation == aUto (wrong capitalization), hints ignored",
 		hintsEnabled:       true,
 		epsProxyingEnabled: true,
 		nodeLabels:         map[string]string{v1.LabelTopologyZone: "zone-a"},
-		serviceInfo:        &BaseServiceInfo{nodeLocalExternal: false, hintsAnnotation: "Auto"},
+		serviceInfo:        &BaseServiceInfo{nodeLocalExternal: false, hintsAnnotation: "aUto"},
 		endpoints: []endpoint{
 			{ip: "10.1.2.3", zoneHints: sets.NewString("zone-a")},
 			{ip: "10.1.2.4", zoneHints: sets.NewString("zone-b")},
@@ -234,7 +234,7 @@ func Test_filterEndpointsWithHints(t *testing.T) {
 		endpoints:         []endpoint{{ip: "10.1.2.3", zoneHints: sets.NewString("zone-a")}},
 		expectedEndpoints: []endpoint{{ip: "10.1.2.3", zoneHints: sets.NewString("zone-a")}},
 	}, {
-		name:            "normal endpoint filtering",
+		name:            "normal endpoint filtering, auto annotation",
 		nodeLabels:      map[string]string{v1.LabelTopologyZone: "zone-a"},
 		hintsAnnotation: "auto",
 		endpoints: []endpoint{
@@ -247,6 +247,20 @@ func Test_filterEndpointsWithHints(t *testing.T) {
 			{ip: "10.1.2.3", zoneHints: sets.NewString("zone-a")},
 			{ip: "10.1.2.6", zoneHints: sets.NewString("zone-a")},
 		},
+	}, {
+		name:            "normal endpoint filtering, Auto annotation",
+		nodeLabels:      map[string]string{v1.LabelTopologyZone: "zone-a"},
+		hintsAnnotation: "Auto",
+		endpoints: []endpoint{
+			{ip: "10.1.2.3", zoneHints: sets.NewString("zone-a")},
+			{ip: "10.1.2.4", zoneHints: sets.NewString("zone-b")},
+			{ip: "10.1.2.5", zoneHints: sets.NewString("zone-c")},
+			{ip: "10.1.2.6", zoneHints: sets.NewString("zone-a")},
+		},
+		expectedEndpoints: []endpoint{
+			{ip: "10.1.2.3", zoneHints: sets.NewString("zone-a")},
+			{ip: "10.1.2.6", zoneHints: sets.NewString("zone-a")},
+		},
 	}, {
 		name:            "hintsAnnotation empty, no filtering applied",
 		nodeLabels:      map[string]string{v1.LabelTopologyZone: "zone-a"},

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

@@ -148,7 +148,7 @@ const (
 	PodDeletionCost = "controller.kubernetes.io/pod-deletion-cost"
 
 	// AnnotationTopologyAwareHints can be used to enable or disable Topology
-	// Aware Hints for a Service. This may be set to "auto" or "disabled". Any
+	// Aware Hints for a Service. This may be set to "Auto" or "Disabled". Any
-	// other value is treated as "disabled".
+	// other value is treated as "Disabled".
 	AnnotationTopologyAwareHints = "service.kubernetes.io/topology-aware-hints"
 )