From 8bb597c0d24f80ff30f673d96e819baba7ac8200 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Tue, 11 Feb 2025 10:13:08 -0500 Subject: [PATCH] Clean up some confusion around TrafficDistribution metrics The EndpointSlice controller metrics had code to deal with unrecognized TrafficDistribution values, and tested both "invalid" and empty values, but TrafficDistribution is validated to always be either nil or a recognized value, so those cases can't happen. --- .../src/k8s.io/endpointslice/metrics/cache.go | 14 ------- .../endpointslice/metrics/cache_test.go | 39 +++++++++---------- .../k8s.io/endpointslice/metrics/metrics.go | 2 +- .../k8s.io/endpointslice/reconciler_test.go | 19 ++++----- 4 files changed, 27 insertions(+), 47 deletions(-) diff --git a/staging/src/k8s.io/endpointslice/metrics/cache.go b/staging/src/k8s.io/endpointslice/metrics/cache.go index 6f102bb498e..30fd4de4436 100644 --- a/staging/src/k8s.io/endpointslice/metrics/cache.go +++ b/staging/src/k8s.io/endpointslice/metrics/cache.go @@ -20,7 +20,6 @@ import ( "math" "sync" - corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" endpointsliceutil "k8s.io/endpointslice/util" ) @@ -60,12 +59,6 @@ type Cache struct { servicesByTrafficDistribution map[string]map[types.NamespacedName]bool } -const ( - // Label value for cases when service.spec.trafficDistribution is set to an - // unknown value. - trafficDistributionImplementationSpecific = "ImplementationSpecific" -) - // ServicePortCache tracks values for total numbers of desired endpoints as well // as the efficiency of EndpointSlice endpoints distribution for each unique // Service Port combination. @@ -151,13 +144,6 @@ func (c *Cache) UpdateTrafficDistributionForService(serviceNN types.NamespacedNa } trafficDistribution := *trafficDistributionPtr - // If we don't explicitly recognize a value for trafficDistribution, it should - // be treated as an implementation specific value. All such implementation - // specific values should use the label value "ImplementationSpecific" to not - // explode the metric labels cardinality. - if trafficDistribution != corev1.ServiceTrafficDistributionPreferClose { - trafficDistribution = trafficDistributionImplementationSpecific - } serviceSet, ok := c.servicesByTrafficDistribution[trafficDistribution] if !ok { serviceSet = make(map[types.NamespacedName]bool) diff --git a/staging/src/k8s.io/endpointslice/metrics/cache_test.go b/staging/src/k8s.io/endpointslice/metrics/cache_test.go index 002917b2aae..9db2e2372a1 100644 --- a/staging/src/k8s.io/endpointslice/metrics/cache_test.go +++ b/staging/src/k8s.io/endpointslice/metrics/cache_test.go @@ -124,53 +124,50 @@ func TestCache_ServicesByTrafficDistribution(t *testing.T) { corev1.ServiceTrafficDistributionPreferClose: {service1: true, service2: true}, // Delta }, desc) - desc = "service3 starts using trafficDistribution=InvalidValue" - cache.UpdateTrafficDistributionForService(service3, ptr.To("InvalidValue")) + desc = "service3 starts using trafficDistribution=FutureValue" + cache.UpdateTrafficDistributionForService(service3, ptr.To("FutureValue")) mustHaveServicesByTrafficDistribution(map[string]map[types.NamespacedName]bool{ corev1.ServiceTrafficDistributionPreferClose: {service1: true, service2: true}, - trafficDistributionImplementationSpecific: {service3: true}, // Delta + "FutureValue": {service3: true}, // Delta }, desc) desc = "service4 starts using trafficDistribution=nil" cache.UpdateTrafficDistributionForService(service4, nil) mustHaveServicesByTrafficDistribution(map[string]map[types.NamespacedName]bool{ // No delta corev1.ServiceTrafficDistributionPreferClose: {service1: true, service2: true}, - trafficDistributionImplementationSpecific: {service3: true}, + "FutureValue": {service3: true}, }, desc) - desc = "service2 transitions trafficDistribution: PreferClose -> InvalidValue" - cache.UpdateTrafficDistributionForService(service2, ptr.To("InvalidValue")) + desc = "service2 transitions trafficDistribution: PreferClose -> AnotherFutureValue" + cache.UpdateTrafficDistributionForService(service2, ptr.To("AnotherFutureValue")) mustHaveServicesByTrafficDistribution(map[string]map[types.NamespacedName]bool{ - corev1.ServiceTrafficDistributionPreferClose: {service1: true}, // Delta - trafficDistributionImplementationSpecific: {service3: true, service2: true}, // Delta + corev1.ServiceTrafficDistributionPreferClose: {service1: true}, // Delta + "FutureValue": {service3: true}, + "AnotherFutureValue": {service2: true}, // Delta }, desc) desc = "service3 gets deleted" cache.DeleteService(service3) mustHaveServicesByTrafficDistribution(map[string]map[types.NamespacedName]bool{ corev1.ServiceTrafficDistributionPreferClose: {service1: true}, - trafficDistributionImplementationSpecific: {service2: true}, // Delta + "FutureValue": {}, // Delta + "AnotherFutureValue": {service2: true}, }, desc) - desc = "service1 transitions trafficDistribution: PreferClose -> empty" - cache.UpdateTrafficDistributionForService(service1, ptr.To("")) - mustHaveServicesByTrafficDistribution(map[string]map[types.NamespacedName]bool{ - corev1.ServiceTrafficDistributionPreferClose: {}, // Delta - trafficDistributionImplementationSpecific: {service1: true, service2: true}, // Delta - }, desc) - - desc = "service1 transitions trafficDistribution: InvalidValue -> nil" + desc = "service1 transitions trafficDistribution: PreferClose -> nil" cache.UpdateTrafficDistributionForService(service1, nil) mustHaveServicesByTrafficDistribution(map[string]map[types.NamespacedName]bool{ - corev1.ServiceTrafficDistributionPreferClose: {}, - trafficDistributionImplementationSpecific: {service2: true}, // Delta + corev1.ServiceTrafficDistributionPreferClose: {}, // Delta + "FutureValue": {}, + "AnotherFutureValue": {service2: true}, }, desc) - desc = "service2 transitions trafficDistribution: InvalidValue -> nil" + desc = "service2 transitions trafficDistribution: AnotherFutureValue -> nil" cache.UpdateTrafficDistributionForService(service2, nil) mustHaveServicesByTrafficDistribution(map[string]map[types.NamespacedName]bool{ corev1.ServiceTrafficDistributionPreferClose: {}, - trafficDistributionImplementationSpecific: {}, // Delta + "FutureValue": {}, + "AnotherFutureValue": {}, // Delta }, desc) } diff --git a/staging/src/k8s.io/endpointslice/metrics/metrics.go b/staging/src/k8s.io/endpointslice/metrics/metrics.go index 236a83c3218..b7228a1810a 100644 --- a/staging/src/k8s.io/endpointslice/metrics/metrics.go +++ b/staging/src/k8s.io/endpointslice/metrics/metrics.go @@ -129,7 +129,7 @@ var ( Help: "Number of Services using some specific trafficDistribution", StabilityLevel: metrics.ALPHA, }, - []string{"traffic_distribution"}, // One of ["PreferClose", "ImplementationSpecific"] + []string{"traffic_distribution"}, // A trafficDistribution value ) ) diff --git a/staging/src/k8s.io/endpointslice/reconciler_test.go b/staging/src/k8s.io/endpointslice/reconciler_test.go index 243bc807c20..f46fcfdabb5 100644 --- a/staging/src/k8s.io/endpointslice/reconciler_test.go +++ b/staging/src/k8s.io/endpointslice/reconciler_test.go @@ -2017,7 +2017,7 @@ func TestReconcile_TrafficDistribution(t *testing.T) { desc string trafficDistributionFeatureGateEnabled bool - trafficDistribution string + trafficDistribution *string topologyAnnotation string // Defines how many hints belong to a particular zone. @@ -2031,7 +2031,7 @@ func TestReconcile_TrafficDistribution(t *testing.T) { name: "trafficDistribution=PreferClose, topologyAnnotation=Disabled", desc: "When trafficDistribution is enabled and topologyAnnotation is disabled, hints should be distributed as per the trafficDistribution field", trafficDistributionFeatureGateEnabled: true, - trafficDistribution: corev1.ServiceTrafficDistributionPreferClose, + trafficDistribution: ptr.To(corev1.ServiceTrafficDistributionPreferClose), topologyAnnotation: "Disabled", wantHintsDistributionByZone: map[string]int{ "zone-a": 1, // {pod-0} @@ -2059,7 +2059,7 @@ func TestReconcile_TrafficDistribution(t *testing.T) { name: "feature gate disabled; trafficDistribution=PreferClose, topologyAnnotation=Disabled", desc: "When feature gate is disabled, trafficDistribution should be ignored", trafficDistributionFeatureGateEnabled: false, - trafficDistribution: corev1.ServiceTrafficDistributionPreferClose, + trafficDistribution: ptr.To(corev1.ServiceTrafficDistributionPreferClose), topologyAnnotation: "Disabled", wantHintsDistributionByZone: map[string]int{"": 6}, // Equivalent to no hints. wantMetrics: expectedMetrics{ @@ -2080,7 +2080,7 @@ func TestReconcile_TrafficDistribution(t *testing.T) { name: "trafficDistribution=PreferClose, topologyAnnotation=Auto", desc: "When trafficDistribution and topologyAnnotation are both enabled, precedence should be given to topologyAnnotation", trafficDistributionFeatureGateEnabled: true, - trafficDistribution: corev1.ServiceTrafficDistributionPreferClose, + trafficDistribution: ptr.To(corev1.ServiceTrafficDistributionPreferClose), topologyAnnotation: "Auto", wantHintsDistributionByZone: map[string]int{ "zone-a": 2, // {pod-0, pod-3} (pod-3 is just an example, it could have also been either of the other two) @@ -2103,10 +2103,10 @@ func TestReconcile_TrafficDistribution(t *testing.T) { }, }, { - name: "trafficDistribution=, topologyAnnotation=", - desc: "When trafficDistribution and topologyAnnotation are both disabled, no hints should be added, but the servicesCountByTrafficDistribution metric should reflect this", + name: "trafficDistribution=nil, topologyAnnotation=", + desc: "When trafficDistribution and topologyAnnotation are both disabled, no hints should be added", trafficDistributionFeatureGateEnabled: true, - trafficDistribution: "", + trafficDistribution: nil, topologyAnnotation: "", wantHintsDistributionByZone: map[string]int{"": 6}, // Equivalent to no hints. wantMetrics: expectedMetrics{ @@ -2121,9 +2121,6 @@ func TestReconcile_TrafficDistribution(t *testing.T) { slicesChangedPerSync: 1, // 1 means both topologyAnnotation and trafficDistribution were not used. slicesChangedPerSyncTopology: 0, // 0 means topologyAnnotation was not used. slicesChangedPerSyncTrafficDist: 0, // 0 means trafficDistribution was not used. - servicesCountByTrafficDistribution: map[string]int{ - "ImplementationSpecific": 1, - }, }, }, } @@ -2142,7 +2139,7 @@ func TestReconcile_TrafficDistribution(t *testing.T) { r.topologyCache.SetNodes(logger, nodes) service := svc.DeepCopy() - service.Spec.TrafficDistribution = &tc.trafficDistribution + service.Spec.TrafficDistribution = tc.trafficDistribution service.Annotations = map[string]string{ corev1.DeprecatedAnnotationTopologyAwareHints: tc.topologyAnnotation, }