Skip to content

Commit eef3840

Browse files
mikkeloscarmikkeloscar
authored
Fix the check for HPA changed (#785)
Signed-off-by: Mikkel Oscar Lyderik Larsen <[email protected]>
1 parent 716d03a commit eef3840

File tree

2 files changed

+213
-9
lines changed

2 files changed

+213
-9
lines changed

‎pkg/provider/hpa.go‎

Lines changed: 44 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,10 @@ import (
2525
"github.com/zalando-incubator/kube-metrics-adapter/pkg/recorder"
2626
)
2727

28+
const (
29+
kubectlLastAppliedAnnotation = "kubectl.kubernetes.io/last-applied-configuration"
30+
)
31+
2832
var (
2933
// CollectionSuccesses is the total number of successful collections.
3034
CollectionSuccesses = promauto.NewCounter(prometheus.CounterOpts{
@@ -120,7 +124,7 @@ func (p *HPAProvider) Run(ctx context.Context) {
120124
// updateHPAs discovers all HPA resources and sets up metric collectors for new
121125
// HPAs.
122126
func (p *HPAProvider) updateHPAs() error {
123-
p.logger.Info("Looking for HPAs")
127+
p.logger.Debug("Looking for HPAs")
124128

125129
hpas, err := p.client.AutoscalingV2().HorizontalPodAutoscalers(metav1.NamespaceAll).List(context.TODO(), metav1.ListOptions{})
126130
if err != nil {
@@ -144,7 +148,7 @@ func (p *HPAProvider) updateHPAs() error {
144148
// if the hpa has changed then remove the previous
145149
// scheduled collector.
146150
if hpaUpdated {
147-
p.logger.Infof("Removing previously scheduled metrics collector: %s", resourceRef)
151+
p.logger.Infof("Removing previously scheduled metrics collector as HPA changed: %s", resourceRef)
148152
p.collectorScheduler.Remove(resourceRef)
149153
}
150154

@@ -197,20 +201,51 @@ func (p *HPAProvider) updateHPAs() error {
197201
p.collectorScheduler.Remove(ref)
198202
}
199203

200-
p.logger.Infof("Found %d new/updated HPA(s)", newHPAs)
204+
if newHPAs > 0 {
205+
p.logger.Infof("Found %d new/updated HPA(s)", newHPAs)
206+
} else {
207+
p.logger.Debug("No new/updated HPAs found")
208+
}
209+
201210
p.hpaCache = newHPACache
202211

203212
return nil
204213
}
205214

206215
// equalHPA returns true if two HPAs are identical (apart from their status).
207216
func equalHPA(a, b autoscalingv2.HorizontalPodAutoscaler) bool {
208-
// reset resource version to not compare it since this will change
209-
// whenever the status of the object is updated. We only want to
210-
// compare the metadata and the spec.
211-
a.ObjectMeta.ResourceVersion = ""
212-
b.ObjectMeta.ResourceVersion = ""
213-
return reflect.DeepEqual(a.ObjectMeta, b.ObjectMeta) && reflect.DeepEqual(a.Spec, b.Spec)
217+
return annotationsUpToDate(a.ObjectMeta, b.ObjectMeta, kubectlLastAppliedAnnotation) && reflect.DeepEqual(a.Spec, b.Spec)
218+
}
219+
220+
// annotationsUpToDate checks whether the annotations of the existing and
221+
// updated resource are up to date.
222+
func annotationsUpToDate(updated, existing metav1.ObjectMeta, ignoredAnnotations ...string) bool {
223+
if len(updated.Annotations) != len(existing.Annotations) {
224+
return false
225+
}
226+
227+
for k, v := range updated.Annotations {
228+
isIgnored := false
229+
for _, ignored := range ignoredAnnotations {
230+
if k == ignored {
231+
isIgnored = true
232+
break
233+
}
234+
}
235+
236+
if isIgnored {
237+
continue
238+
}
239+
240+
existingValue, ok := existing.GetAnnotations()[k]
241+
if ok && existingValue == v {
242+
continue
243+
}
244+
245+
return false
246+
}
247+
248+
return true
214249
}
215250

216251
// collectMetrics collects all metrics from collectors and manages a central

‎pkg/provider/hpa_test.go‎

Lines changed: 169 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -193,3 +193,172 @@ func TestUpdateHPAsDisregardingIncompatibleHPA(t *testing.T) {
193193
// we expect an event when disregardIncompatibleHPAs=false
194194
require.Len(t, eventRecorder.Events, 1)
195195
}
196+
197+
func TestEqualHPA(t *testing.T) {
198+
for _, tc := range []struct {
199+
name string
200+
hpa1 autoscaling.HorizontalPodAutoscaler
201+
hpa2 autoscaling.HorizontalPodAutoscaler
202+
equal bool
203+
}{
204+
{
205+
name: "Identical HPAs are equal",
206+
hpa1: autoscaling.HorizontalPodAutoscaler{
207+
ObjectMeta: metav1.ObjectMeta{
208+
Name: "hpa1",
209+
Namespace: "default",
210+
},
211+
Spec: autoscaling.HorizontalPodAutoscalerSpec{
212+
ScaleTargetRef: autoscaling.CrossVersionObjectReference{
213+
Kind: "Deployment",
214+
Name: "app",
215+
APIVersion: "apps/v1",
216+
},
217+
MinReplicas: &[]int32{1}[0],
218+
MaxReplicas: 10,
219+
},
220+
},
221+
hpa2: autoscaling.HorizontalPodAutoscaler{
222+
ObjectMeta: metav1.ObjectMeta{
223+
Name: "hpa1",
224+
Namespace: "default",
225+
},
226+
Spec: autoscaling.HorizontalPodAutoscalerSpec{
227+
ScaleTargetRef: autoscaling.CrossVersionObjectReference{
228+
Kind: "Deployment",
229+
Name: "app",
230+
APIVersion: "apps/v1",
231+
},
232+
MinReplicas: &[]int32{1}[0],
233+
MaxReplicas: 10,
234+
},
235+
},
236+
equal: true,
237+
},
238+
{
239+
name: "Only kubectl-last-applied diff on HPAs are equal",
240+
hpa1: autoscaling.HorizontalPodAutoscaler{
241+
ObjectMeta: metav1.ObjectMeta{
242+
Name: "hpa1",
243+
Namespace: "default",
244+
Annotations: map[string]string{
245+
kubectlLastAppliedAnnotation: "old value",
246+
},
247+
},
248+
Spec: autoscaling.HorizontalPodAutoscalerSpec{
249+
ScaleTargetRef: autoscaling.CrossVersionObjectReference{
250+
Kind: "Deployment",
251+
Name: "app",
252+
APIVersion: "apps/v1",
253+
},
254+
MinReplicas: &[]int32{1}[0],
255+
MaxReplicas: 10,
256+
},
257+
},
258+
hpa2: autoscaling.HorizontalPodAutoscaler{
259+
ObjectMeta: metav1.ObjectMeta{
260+
Name: "hpa1",
261+
Namespace: "default",
262+
Annotations: map[string]string{
263+
kubectlLastAppliedAnnotation: "new value",
264+
},
265+
},
266+
Spec: autoscaling.HorizontalPodAutoscalerSpec{
267+
ScaleTargetRef: autoscaling.CrossVersionObjectReference{
268+
Kind: "Deployment",
269+
Name: "app",
270+
APIVersion: "apps/v1",
271+
},
272+
MinReplicas: &[]int32{1}[0],
273+
MaxReplicas: 10,
274+
},
275+
},
276+
equal: true,
277+
},
278+
{
279+
name: "diff in annotations are not equal",
280+
hpa1: autoscaling.HorizontalPodAutoscaler{
281+
ObjectMeta: metav1.ObjectMeta{
282+
Name: "hpa1",
283+
Namespace: "default",
284+
Annotations: map[string]string{
285+
"annotation": "old value",
286+
},
287+
},
288+
Spec: autoscaling.HorizontalPodAutoscalerSpec{
289+
ScaleTargetRef: autoscaling.CrossVersionObjectReference{
290+
Kind: "Deployment",
291+
Name: "app",
292+
APIVersion: "apps/v1",
293+
},
294+
MinReplicas: &[]int32{1}[0],
295+
MaxReplicas: 10,
296+
},
297+
},
298+
hpa2: autoscaling.HorizontalPodAutoscaler{
299+
ObjectMeta: metav1.ObjectMeta{
300+
Name: "hpa1",
301+
Namespace: "default",
302+
Annotations: map[string]string{
303+
"annotation": "new value",
304+
},
305+
},
306+
Spec: autoscaling.HorizontalPodAutoscalerSpec{
307+
ScaleTargetRef: autoscaling.CrossVersionObjectReference{
308+
Kind: "Deployment",
309+
Name: "app",
310+
APIVersion: "apps/v1",
311+
},
312+
MinReplicas: &[]int32{1}[0],
313+
MaxReplicas: 10,
314+
},
315+
},
316+
equal: false,
317+
},
318+
{
319+
name: "diff in labels are equal",
320+
hpa1: autoscaling.HorizontalPodAutoscaler{
321+
ObjectMeta: metav1.ObjectMeta{
322+
Name: "hpa1",
323+
Namespace: "default",
324+
Labels: map[string]string{
325+
"label": "old-value",
326+
},
327+
},
328+
Spec: autoscaling.HorizontalPodAutoscalerSpec{
329+
ScaleTargetRef: autoscaling.CrossVersionObjectReference{
330+
Kind: "Deployment",
331+
Name: "app",
332+
APIVersion: "apps/v1",
333+
},
334+
MinReplicas: &[]int32{1}[0],
335+
MaxReplicas: 10,
336+
},
337+
},
338+
hpa2: autoscaling.HorizontalPodAutoscaler{
339+
ObjectMeta: metav1.ObjectMeta{
340+
Name: "hpa1",
341+
Namespace: "default",
342+
Labels: map[string]string{
343+
"label": "new-value",
344+
},
345+
},
346+
Spec: autoscaling.HorizontalPodAutoscalerSpec{
347+
ScaleTargetRef: autoscaling.CrossVersionObjectReference{
348+
Kind: "Deployment",
349+
Name: "app",
350+
APIVersion: "apps/v1",
351+
},
352+
MinReplicas: &[]int32{1}[0],
353+
MaxReplicas: 10,
354+
},
355+
},
356+
equal: true,
357+
},
358+
} {
359+
t.Run(tc.name, func(t *testing.T) {
360+
require.Equal(t, tc.equal, equalHPA(tc.hpa1, tc.hpa2))
361+
})
362+
363+
}
364+
}

0 commit comments

Comments
 (0)