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.

Author: danwinship

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)

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)
 
 }

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
 	)
 )
 

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=<empty>, topologyAnnotation=<empty>",
-			desc:                                  "When trafficDistribution and topologyAnnotation are both disabled, no hints should be added, but the servicesCountByTrafficDistribution metric should reflect this",
+			name:                                  "trafficDistribution=nil, topologyAnnotation=<empty>",
+			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,
 			}