Skip to content

Commit 8bb597c

Browse files
danwinshipdanwinship
committed
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.
1 parent fdddd8d commit 8bb597c

File tree

4 files changed

+27
-47
lines changed

4 files changed

+27
-47
lines changed

‎staging/src/k8s.io/endpointslice/metrics/cache.go‎

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ import (
2020
"math"
2121
"sync"
2222

23-
corev1 "k8s.io/api/core/v1"
2423
"k8s.io/apimachinery/pkg/types"
2524
endpointsliceutil "k8s.io/endpointslice/util"
2625
)
@@ -60,12 +59,6 @@ type Cache struct {
6059
servicesByTrafficDistribution map[string]map[types.NamespacedName]bool
6160
}
6261

63-
const (
64-
// Label value for cases when service.spec.trafficDistribution is set to an
65-
// unknown value.
66-
trafficDistributionImplementationSpecific = "ImplementationSpecific"
67-
)
68-
6962
// ServicePortCache tracks values for total numbers of desired endpoints as well
7063
// as the efficiency of EndpointSlice endpoints distribution for each unique
7164
// Service Port combination.
@@ -151,13 +144,6 @@ func (c *Cache) UpdateTrafficDistributionForService(serviceNN types.NamespacedNa
151144
}
152145

153146
trafficDistribution := *trafficDistributionPtr
154-
// If we don't explicitly recognize a value for trafficDistribution, it should
155-
// be treated as an implementation specific value. All such implementation
156-
// specific values should use the label value "ImplementationSpecific" to not
157-
// explode the metric labels cardinality.
158-
if trafficDistribution != corev1.ServiceTrafficDistributionPreferClose {
159-
trafficDistribution = trafficDistributionImplementationSpecific
160-
}
161147
serviceSet, ok := c.servicesByTrafficDistribution[trafficDistribution]
162148
if !ok {
163149
serviceSet = make(map[types.NamespacedName]bool)

‎staging/src/k8s.io/endpointslice/metrics/cache_test.go‎

Lines changed: 18 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -124,53 +124,50 @@ func TestCache_ServicesByTrafficDistribution(t *testing.T) {
124124
corev1.ServiceTrafficDistributionPreferClose: {service1: true, service2: true}, // Delta
125125
}, desc)
126126

127-
desc = "service3 starts using trafficDistribution=InvalidValue"
128-
cache.UpdateTrafficDistributionForService(service3, ptr.To("InvalidValue"))
127+
desc = "service3 starts using trafficDistribution=FutureValue"
128+
cache.UpdateTrafficDistributionForService(service3, ptr.To("FutureValue"))
129129
mustHaveServicesByTrafficDistribution(map[string]map[types.NamespacedName]bool{
130130
corev1.ServiceTrafficDistributionPreferClose: {service1: true, service2: true},
131-
trafficDistributionImplementationSpecific: {service3: true}, // Delta
131+
"FutureValue": {service3: true}, // Delta
132132
}, desc)
133133

134134
desc = "service4 starts using trafficDistribution=nil"
135135
cache.UpdateTrafficDistributionForService(service4, nil)
136136
mustHaveServicesByTrafficDistribution(map[string]map[types.NamespacedName]bool{ // No delta
137137
corev1.ServiceTrafficDistributionPreferClose: {service1: true, service2: true},
138-
trafficDistributionImplementationSpecific: {service3: true},
138+
"FutureValue": {service3: true},
139139
}, desc)
140140

141-
desc = "service2 transitions trafficDistribution: PreferClose -> InvalidValue"
142-
cache.UpdateTrafficDistributionForService(service2, ptr.To("InvalidValue"))
141+
desc = "service2 transitions trafficDistribution: PreferClose -> AnotherFutureValue"
142+
cache.UpdateTrafficDistributionForService(service2, ptr.To("AnotherFutureValue"))
143143
mustHaveServicesByTrafficDistribution(map[string]map[types.NamespacedName]bool{
144-
corev1.ServiceTrafficDistributionPreferClose: {service1: true}, // Delta
145-
trafficDistributionImplementationSpecific: {service3: true, service2: true}, // Delta
144+
corev1.ServiceTrafficDistributionPreferClose: {service1: true}, // Delta
145+
"FutureValue": {service3: true},
146+
"AnotherFutureValue": {service2: true}, // Delta
146147
}, desc)
147148

148149
desc = "service3 gets deleted"
149150
cache.DeleteService(service3)
150151
mustHaveServicesByTrafficDistribution(map[string]map[types.NamespacedName]bool{
151152
corev1.ServiceTrafficDistributionPreferClose: {service1: true},
152-
trafficDistributionImplementationSpecific: {service2: true}, // Delta
153+
"FutureValue": {}, // Delta
154+
"AnotherFutureValue": {service2: true},
153155
}, desc)
154156

155-
desc = "service1 transitions trafficDistribution: PreferClose -> empty"
156-
cache.UpdateTrafficDistributionForService(service1, ptr.To(""))
157-
mustHaveServicesByTrafficDistribution(map[string]map[types.NamespacedName]bool{
158-
corev1.ServiceTrafficDistributionPreferClose: {}, // Delta
159-
trafficDistributionImplementationSpecific: {service1: true, service2: true}, // Delta
160-
}, desc)
161-
162-
desc = "service1 transitions trafficDistribution: InvalidValue -> nil"
157+
desc = "service1 transitions trafficDistribution: PreferClose -> nil"
163158
cache.UpdateTrafficDistributionForService(service1, nil)
164159
mustHaveServicesByTrafficDistribution(map[string]map[types.NamespacedName]bool{
165-
corev1.ServiceTrafficDistributionPreferClose: {},
166-
trafficDistributionImplementationSpecific: {service2: true}, // Delta
160+
corev1.ServiceTrafficDistributionPreferClose: {}, // Delta
161+
"FutureValue": {},
162+
"AnotherFutureValue": {service2: true},
167163
}, desc)
168164

169-
desc = "service2 transitions trafficDistribution: InvalidValue -> nil"
165+
desc = "service2 transitions trafficDistribution: AnotherFutureValue -> nil"
170166
cache.UpdateTrafficDistributionForService(service2, nil)
171167
mustHaveServicesByTrafficDistribution(map[string]map[types.NamespacedName]bool{
172168
corev1.ServiceTrafficDistributionPreferClose: {},
173-
trafficDistributionImplementationSpecific: {}, // Delta
169+
"FutureValue": {},
170+
"AnotherFutureValue": {}, // Delta
174171
}, desc)
175172

176173
}

‎staging/src/k8s.io/endpointslice/metrics/metrics.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@ var (
129129
Help: "Number of Services using some specific trafficDistribution",
130130
StabilityLevel: metrics.ALPHA,
131131
},
132-
[]string{"traffic_distribution"}, // One of ["PreferClose", "ImplementationSpecific"]
132+
[]string{"traffic_distribution"}, // A trafficDistribution value
133133
)
134134
)
135135

‎staging/src/k8s.io/endpointslice/reconciler_test.go‎

Lines changed: 8 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2017,7 +2017,7 @@ func TestReconcile_TrafficDistribution(t *testing.T) {
20172017
desc string
20182018

20192019
trafficDistributionFeatureGateEnabled bool
2020-
trafficDistribution string
2020+
trafficDistribution *string
20212021
topologyAnnotation string
20222022

20232023
// Defines how many hints belong to a particular zone.
@@ -2031,7 +2031,7 @@ func TestReconcile_TrafficDistribution(t *testing.T) {
20312031
name: "trafficDistribution=PreferClose, topologyAnnotation=Disabled",
20322032
desc: "When trafficDistribution is enabled and topologyAnnotation is disabled, hints should be distributed as per the trafficDistribution field",
20332033
trafficDistributionFeatureGateEnabled: true,
2034-
trafficDistribution: corev1.ServiceTrafficDistributionPreferClose,
2034+
trafficDistribution: ptr.To(corev1.ServiceTrafficDistributionPreferClose),
20352035
topologyAnnotation: "Disabled",
20362036
wantHintsDistributionByZone: map[string]int{
20372037
"zone-a": 1, // {pod-0}
@@ -2059,7 +2059,7 @@ func TestReconcile_TrafficDistribution(t *testing.T) {
20592059
name: "feature gate disabled; trafficDistribution=PreferClose, topologyAnnotation=Disabled",
20602060
desc: "When feature gate is disabled, trafficDistribution should be ignored",
20612061
trafficDistributionFeatureGateEnabled: false,
2062-
trafficDistribution: corev1.ServiceTrafficDistributionPreferClose,
2062+
trafficDistribution: ptr.To(corev1.ServiceTrafficDistributionPreferClose),
20632063
topologyAnnotation: "Disabled",
20642064
wantHintsDistributionByZone: map[string]int{"": 6}, // Equivalent to no hints.
20652065
wantMetrics: expectedMetrics{
@@ -2080,7 +2080,7 @@ func TestReconcile_TrafficDistribution(t *testing.T) {
20802080
name: "trafficDistribution=PreferClose, topologyAnnotation=Auto",
20812081
desc: "When trafficDistribution and topologyAnnotation are both enabled, precedence should be given to topologyAnnotation",
20822082
trafficDistributionFeatureGateEnabled: true,
2083-
trafficDistribution: corev1.ServiceTrafficDistributionPreferClose,
2083+
trafficDistribution: ptr.To(corev1.ServiceTrafficDistributionPreferClose),
20842084
topologyAnnotation: "Auto",
20852085
wantHintsDistributionByZone: map[string]int{
20862086
"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) {
21032103
},
21042104
},
21052105
{
2106-
name: "trafficDistribution=<empty>, topologyAnnotation=<empty>",
2107-
desc: "When trafficDistribution and topologyAnnotation are both disabled, no hints should be added, but the servicesCountByTrafficDistribution metric should reflect this",
2106+
name: "trafficDistribution=nil, topologyAnnotation=<empty>",
2107+
desc: "When trafficDistribution and topologyAnnotation are both disabled, no hints should be added",
21082108
trafficDistributionFeatureGateEnabled: true,
2109-
trafficDistribution: "",
2109+
trafficDistribution: nil,
21102110
topologyAnnotation: "",
21112111
wantHintsDistributionByZone: map[string]int{"": 6}, // Equivalent to no hints.
21122112
wantMetrics: expectedMetrics{
@@ -2121,9 +2121,6 @@ func TestReconcile_TrafficDistribution(t *testing.T) {
21212121
slicesChangedPerSync: 1, // 1 means both topologyAnnotation and trafficDistribution were not used.
21222122
slicesChangedPerSyncTopology: 0, // 0 means topologyAnnotation was not used.
21232123
slicesChangedPerSyncTrafficDist: 0, // 0 means trafficDistribution was not used.
2124-
servicesCountByTrafficDistribution: map[string]int{
2125-
"ImplementationSpecific": 1,
2126-
},
21272124
},
21282125
},
21292126
}
@@ -2142,7 +2139,7 @@ func TestReconcile_TrafficDistribution(t *testing.T) {
21422139
r.topologyCache.SetNodes(logger, nodes)
21432140

21442141
service := svc.DeepCopy()
2145-
service.Spec.TrafficDistribution = &tc.trafficDistribution
2142+
service.Spec.TrafficDistribution = tc.trafficDistribution
21462143
service.Annotations = map[string]string{
21472144
corev1.DeprecatedAnnotationTopologyAwareHints: tc.topologyAnnotation,
21482145
}

0 commit comments

Comments
 (0)