From 50b377fe4e3c4fa6ac63c14be6e6a3fd04d3f8d0 Mon Sep 17 00:00:00 2001 From: Rob Scott Date: Wed, 31 Mar 2021 15:58:04 -0700 Subject: [PATCH] Updating Topology Aware Hints to support "Auto" value for annotation Previously only "auto" was supported, a value that was inconsistent with Kubernetes naming patterns. --- pkg/apis/core/annotation_key_constants.go | 4 +- .../endpointslice/metrics/metrics.go | 2 +- pkg/controller/endpointslice/reconciler.go | 4 +- .../endpointslice/reconciler_test.go | 12 +++--- pkg/controller/endpointslice/utils.go | 4 +- pkg/controller/endpointslice/utils_test.go | 43 +++++++++++++++++++ pkg/proxy/topology.go | 6 +-- pkg/proxy/topology_test.go | 20 +++++++-- .../api/core/v1/annotation_key_constants.go | 4 +- 9 files changed, 78 insertions(+), 21 deletions(-) diff --git a/pkg/apis/core/annotation_key_constants.go b/pkg/apis/core/annotation_key_constants.go index baacf3f10cd..db68d290dc6 100644 --- a/pkg/apis/core/annotation_key_constants.go +++ b/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 - // other value is treated as "disabled". + // Aware Hints for a Service. This may be set to "Auto" or "Disabled". Any + // other value is treated as "Disabled". AnnotationTopologyAwareHints = "service.kubernetes.io/topology-aware-hints" ) diff --git a/pkg/controller/endpointslice/metrics/metrics.go b/pkg/controller/endpointslice/metrics/metrics.go index 90d0fbb8f4f..6a166703be0 100644 --- a/pkg/controller/endpointslice/metrics/metrics.go +++ b/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 diff --git a/pkg/controller/endpointslice/reconciler.go b/pkg/controller/endpointslice/reconciler.go index 2767af5497a..d8540f9d3e5 100644 --- a/pkg/controller/endpointslice/reconciler.go +++ b/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) diff --git a/pkg/controller/endpointslice/reconciler_test.go b/pkg/controller/endpointslice/reconciler_test.go index b20aa2cfb0a..ec6b576cede 100644 --- a/pkg/controller/endpointslice/reconciler_test.go +++ b/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": "auto"}) + metrics.EndpointSlicesChangedPerSync.Delete(map[string]string{"topology": "Disabled"}) + metrics.EndpointSlicesChangedPerSync.Delete(map[string]string{"topology": "Auto"}) metrics.EndpointSliceSyncs.Delete(map[string]string{"result": "success"}) metrics.EndpointSliceSyncs.Delete(map[string]string{"result": "error"}) } diff --git a/pkg/controller/endpointslice/utils.go b/pkg/controller/endpointslice/utils.go index 2a698405e64..77fa3a1e94a 100644 --- a/pkg/controller/endpointslice/utils.go +++ b/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" } diff --git a/pkg/controller/endpointslice/utils_test.go b/pkg/controller/endpointslice/utils_test.go index d7dc55bd947..f5f2b26bae8 100644 --- a/pkg/controller/endpointslice/utils_test.go +++ b/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) + } + }) + } +} diff --git a/pkg/proxy/topology.go b/pkg/proxy/topology.go index 03657185822..377cc65184c 100644 --- a/pkg/proxy/topology.go +++ b/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 != "" && hintsAnnotation != "disabled" { + if hintsAnnotation != "Auto" && hintsAnnotation != "auto" { + 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 diff --git a/pkg/proxy/topology_test.go b/pkg/proxy/topology_test.go index 24bcad9887e..08317028d36 100644 --- a/pkg/proxy/topology_test.go +++ b/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"}, diff --git a/staging/src/k8s.io/api/core/v1/annotation_key_constants.go b/staging/src/k8s.io/api/core/v1/annotation_key_constants.go index 612f6aa747a..22476b2bd92 100644 --- a/staging/src/k8s.io/api/core/v1/annotation_key_constants.go +++ b/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 - // other value is treated as "disabled". + // Aware Hints for a Service. This may be set to "Auto" or "Disabled". Any + // other value is treated as "Disabled". AnnotationTopologyAwareHints = "service.kubernetes.io/topology-aware-hints" )