Skip to content

Commit 25599b5

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes#89968 from liggitt/automated-cherry-pick-of-#89967-upstream-release-1.16
Automated cherry pick of kubernetes#89963: Drop round-trip annotations in HPA conversion
2 parents 8b6f5dd + 2a62b9e commit 25599b5

File tree

14 files changed

+1217
-58
lines changed

14 files changed

+1217
-58
lines changed

‎pkg/apis/autoscaling/BUILD

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ go_library(
1010
srcs = [
1111
"annotations.go",
1212
"doc.go",
13+
"helpers.go",
1314
"register.go",
1415
"types.go",
1516
"zz_generated.deepcopy.go",

‎pkg/apis/autoscaling/helpers.go

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
/*
2+
Copyright 2020 The Kubernetes Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package autoscaling
18+
19+
// DropRoundTripHorizontalPodAutoscalerAnnotations removes any annotations used to serialize round-tripped fields from later API versions,
20+
// and returns false if no changes were made and the original input object was returned.
21+
// It should always be called when converting internal -> external versions, prior
22+
// to setting any of the custom annotations:
23+
//
24+
// annotations, copiedAnnotations := DropRoundTripHorizontalPodAutoscalerAnnotations(externalObj.Annotations)
25+
// externalObj.Annotations = annotations
26+
//
27+
// if internal.SomeField != nil {
28+
// if !copiedAnnotations {
29+
// externalObj.Annotations = DeepCopyStringMap(externalObj.Annotations)
30+
// copiedAnnotations = true
31+
// }
32+
// externalObj.Annotations[...] = json.Marshal(...)
33+
// }
34+
func DropRoundTripHorizontalPodAutoscalerAnnotations(in map[string]string) (out map[string]string, copied bool) {
35+
_, hasMetricsSpecs := in[MetricSpecsAnnotation]
36+
_, hasMetricsStatuses := in[MetricStatusesAnnotation]
37+
_, hasConditions := in[HorizontalPodAutoscalerConditionsAnnotation]
38+
if hasMetricsSpecs || hasMetricsStatuses || hasConditions {
39+
out = DeepCopyStringMap(in)
40+
delete(out, MetricSpecsAnnotation)
41+
delete(out, MetricStatusesAnnotation)
42+
delete(out, HorizontalPodAutoscalerConditionsAnnotation)
43+
return out, true
44+
}
45+
return in, false
46+
}
47+
48+
// DeepCopyStringMap returns a copy of the input map.
49+
// If input is nil, an empty map is returned.
50+
func DeepCopyStringMap(in map[string]string) map[string]string {
51+
out := make(map[string]string, len(in))
52+
for k, v := range in {
53+
out[k] = v
54+
}
55+
return out
56+
}

‎pkg/apis/autoscaling/v1/BUILD

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,10 +30,13 @@ go_test(
3030
embed = [":go_default_library"],
3131
deps = [
3232
"//pkg/api/legacyscheme:go_default_library",
33+
"//pkg/apis/autoscaling:go_default_library",
3334
"//pkg/apis/autoscaling/install:go_default_library",
3435
"//pkg/apis/core/install:go_default_library",
3536
"//staging/src/k8s.io/api/autoscaling/v1:go_default_library",
37+
"//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
3638
"//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",
39+
"//staging/src/k8s.io/apimachinery/pkg/util/diff:go_default_library",
3740
"//vendor/k8s.io/utils/pointer:go_default_library",
3841
],
3942
)

‎pkg/apis/autoscaling/v1/conversion.go

Lines changed: 45 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -294,6 +294,10 @@ func Convert_autoscaling_HorizontalPodAutoscaler_To_v1_HorizontalPodAutoscaler(i
294294
return err
295295
}
296296

297+
// clear any pre-existing round-trip annotations to make sure the only ones set are ones we produced during conversion
298+
annotations, copiedAnnotations := autoscaling.DropRoundTripHorizontalPodAutoscalerAnnotations(out.Annotations)
299+
out.Annotations = annotations
300+
297301
otherMetrics := make([]autoscalingv1.MetricSpec, 0, len(in.Spec.Metrics))
298302
for _, metric := range in.Spec.Metrics {
299303
if metric.Type == autoscaling.ResourceMetricSourceType && metric.Resource != nil && metric.Resource.Name == core.ResourceCPU && metric.Resource.Target.AverageUtilization != nil {
@@ -323,19 +327,16 @@ func Convert_autoscaling_HorizontalPodAutoscaler_To_v1_HorizontalPodAutoscaler(i
323327
}
324328
}
325329

326-
if len(otherMetrics) > 0 || len(in.Status.CurrentMetrics) > 0 || len(currentConditions) > 0 {
327-
old := out.Annotations
328-
out.Annotations = make(map[string]string, len(old)+3)
329-
for k, v := range old {
330-
out.Annotations[k] = v
331-
}
332-
}
333-
334330
if len(otherMetrics) > 0 {
335331
otherMetricsEnc, err := json.Marshal(otherMetrics)
336332
if err != nil {
337333
return err
338334
}
335+
// copy before mutating
336+
if !copiedAnnotations {
337+
copiedAnnotations = true
338+
out.Annotations = autoscaling.DeepCopyStringMap(out.Annotations)
339+
}
339340
out.Annotations[autoscaling.MetricSpecsAnnotation] = string(otherMetricsEnc)
340341
}
341342

@@ -344,6 +345,11 @@ func Convert_autoscaling_HorizontalPodAutoscaler_To_v1_HorizontalPodAutoscaler(i
344345
if err != nil {
345346
return err
346347
}
348+
// copy before mutating
349+
if !copiedAnnotations {
350+
copiedAnnotations = true
351+
out.Annotations = autoscaling.DeepCopyStringMap(out.Annotations)
352+
}
347353
out.Annotations[autoscaling.MetricStatusesAnnotation] = string(currentMetricsEnc)
348354
}
349355

@@ -352,6 +358,11 @@ func Convert_autoscaling_HorizontalPodAutoscaler_To_v1_HorizontalPodAutoscaler(i
352358
if err != nil {
353359
return err
354360
}
361+
// copy before mutating
362+
if !copiedAnnotations {
363+
copiedAnnotations = true
364+
out.Annotations = autoscaling.DeepCopyStringMap(out.Annotations)
365+
}
355366
out.Annotations[autoscaling.HorizontalPodAutoscalerConditionsAnnotation] = string(currentConditionsEnc)
356367
}
357368

@@ -365,38 +376,32 @@ func Convert_v1_HorizontalPodAutoscaler_To_autoscaling_HorizontalPodAutoscaler(i
365376

366377
if otherMetricsEnc, hasOtherMetrics := out.Annotations[autoscaling.MetricSpecsAnnotation]; hasOtherMetrics {
367378
var otherMetrics []autoscalingv1.MetricSpec
368-
if err := json.Unmarshal([]byte(otherMetricsEnc), &otherMetrics); err != nil {
369-
return err
370-
}
371-
372-
// the normal Spec conversion could have populated out.Spec.Metrics with a single element, so deal with that
373-
outMetrics := make([]autoscaling.MetricSpec, len(otherMetrics)+len(out.Spec.Metrics))
374-
for i, metric := range otherMetrics {
375-
if err := Convert_v1_MetricSpec_To_autoscaling_MetricSpec(&metric, &outMetrics[i], s); err != nil {
376-
return err
379+
if err := json.Unmarshal([]byte(otherMetricsEnc), &otherMetrics); err == nil {
380+
// the normal Spec conversion could have populated out.Spec.Metrics with a single element, so deal with that
381+
outMetrics := make([]autoscaling.MetricSpec, len(otherMetrics)+len(out.Spec.Metrics))
382+
for i, metric := range otherMetrics {
383+
if err := Convert_v1_MetricSpec_To_autoscaling_MetricSpec(&metric, &outMetrics[i], s); err != nil {
384+
return err
385+
}
377386
}
387+
if out.Spec.Metrics != nil {
388+
outMetrics[len(otherMetrics)] = out.Spec.Metrics[0]
389+
}
390+
out.Spec.Metrics = outMetrics
378391
}
379-
if out.Spec.Metrics != nil {
380-
outMetrics[len(otherMetrics)] = out.Spec.Metrics[0]
381-
}
382-
out.Spec.Metrics = outMetrics
383-
delete(out.Annotations, autoscaling.MetricSpecsAnnotation)
384392
}
385393

386394
if currentMetricsEnc, hasCurrentMetrics := out.Annotations[autoscaling.MetricStatusesAnnotation]; hasCurrentMetrics {
387395
// ignore any existing status values -- the ones here have more information
388396
var currentMetrics []autoscalingv1.MetricStatus
389-
if err := json.Unmarshal([]byte(currentMetricsEnc), &currentMetrics); err != nil {
390-
return err
391-
}
392-
393-
out.Status.CurrentMetrics = make([]autoscaling.MetricStatus, len(currentMetrics))
394-
for i, currentMetric := range currentMetrics {
395-
if err := Convert_v1_MetricStatus_To_autoscaling_MetricStatus(&currentMetric, &out.Status.CurrentMetrics[i], s); err != nil {
396-
return err
397+
if err := json.Unmarshal([]byte(currentMetricsEnc), &currentMetrics); err == nil {
398+
out.Status.CurrentMetrics = make([]autoscaling.MetricStatus, len(currentMetrics))
399+
for i, currentMetric := range currentMetrics {
400+
if err := Convert_v1_MetricStatus_To_autoscaling_MetricStatus(&currentMetric, &out.Status.CurrentMetrics[i], s); err != nil {
401+
return err
402+
}
397403
}
398404
}
399-
delete(out.Annotations, autoscaling.MetricStatusesAnnotation)
400405
}
401406

402407
// autoscaling/v1 formerly had an implicit default applied in the controller. In v2beta1, we apply it explicitly.
@@ -420,19 +425,19 @@ func Convert_v1_HorizontalPodAutoscaler_To_autoscaling_HorizontalPodAutoscaler(i
420425

421426
if currentConditionsEnc, hasCurrentConditions := out.Annotations[autoscaling.HorizontalPodAutoscalerConditionsAnnotation]; hasCurrentConditions {
422427
var currentConditions []autoscalingv1.HorizontalPodAutoscalerCondition
423-
if err := json.Unmarshal([]byte(currentConditionsEnc), &currentConditions); err != nil {
424-
return err
425-
}
426-
427-
out.Status.Conditions = make([]autoscaling.HorizontalPodAutoscalerCondition, len(currentConditions))
428-
for i, currentCondition := range currentConditions {
429-
if err := Convert_v1_HorizontalPodAutoscalerCondition_To_autoscaling_HorizontalPodAutoscalerCondition(&currentCondition, &out.Status.Conditions[i], s); err != nil {
430-
return err
428+
if err := json.Unmarshal([]byte(currentConditionsEnc), &currentConditions); err == nil {
429+
out.Status.Conditions = make([]autoscaling.HorizontalPodAutoscalerCondition, len(currentConditions))
430+
for i, currentCondition := range currentConditions {
431+
if err := Convert_v1_HorizontalPodAutoscalerCondition_To_autoscaling_HorizontalPodAutoscalerCondition(&currentCondition, &out.Status.Conditions[i], s); err != nil {
432+
return err
433+
}
431434
}
432435
}
433-
delete(out.Annotations, autoscaling.HorizontalPodAutoscalerConditionsAnnotation)
434436
}
435437

438+
// drop round-tripping annotations after converting to internal
439+
out.Annotations, _ = autoscaling.DropRoundTripHorizontalPodAutoscalerAnnotations(out.Annotations)
440+
436441
return nil
437442
}
438443

‎pkg/apis/autoscaling/v1/defaults_test.go

Lines changed: 65 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,11 @@ import (
2121
"testing"
2222

2323
autoscalingv1 "k8s.io/api/autoscaling/v1"
24-
24+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2525
"k8s.io/apimachinery/pkg/runtime"
26+
"k8s.io/apimachinery/pkg/util/diff"
2627
"k8s.io/kubernetes/pkg/api/legacyscheme"
28+
"k8s.io/kubernetes/pkg/apis/autoscaling"
2729
_ "k8s.io/kubernetes/pkg/apis/autoscaling/install"
2830
. "k8s.io/kubernetes/pkg/apis/autoscaling/v1"
2931
_ "k8s.io/kubernetes/pkg/apis/core/install"
@@ -67,6 +69,68 @@ func TestSetDefaultHPA(t *testing.T) {
6769
}
6870
}
6971

72+
func TestHorizontalPodAutoscalerAnnotations(t *testing.T) {
73+
tests := []struct {
74+
hpa autoscalingv1.HorizontalPodAutoscaler
75+
test string
76+
}{
77+
{
78+
hpa: autoscalingv1.HorizontalPodAutoscaler{
79+
ObjectMeta: metav1.ObjectMeta{
80+
Annotations: map[string]string{
81+
autoscaling.HorizontalPodAutoscalerConditionsAnnotation: "",
82+
autoscaling.MetricSpecsAnnotation: "",
83+
autoscaling.MetricStatusesAnnotation: "",
84+
},
85+
},
86+
},
87+
test: "test empty value for Annotations",
88+
},
89+
{
90+
hpa: autoscalingv1.HorizontalPodAutoscaler{
91+
ObjectMeta: metav1.ObjectMeta{
92+
Annotations: map[string]string{
93+
autoscaling.HorizontalPodAutoscalerConditionsAnnotation: "abc",
94+
autoscaling.MetricSpecsAnnotation: "abc",
95+
autoscaling.MetricStatusesAnnotation: "abc",
96+
},
97+
},
98+
},
99+
test: "test random value for Annotations",
100+
},
101+
{
102+
hpa: autoscalingv1.HorizontalPodAutoscaler{
103+
ObjectMeta: metav1.ObjectMeta{
104+
Annotations: map[string]string{
105+
autoscaling.HorizontalPodAutoscalerConditionsAnnotation: "[]",
106+
autoscaling.MetricSpecsAnnotation: "[]",
107+
autoscaling.MetricStatusesAnnotation: "[]",
108+
},
109+
},
110+
},
111+
test: "test empty array value for Annotations",
112+
},
113+
}
114+
115+
for _, test := range tests {
116+
hpa := &test.hpa
117+
hpaBeforeMuatate := *hpa.DeepCopy()
118+
obj := roundTrip(t, runtime.Object(hpa))
119+
final_obj, ok := obj.(*autoscalingv1.HorizontalPodAutoscaler)
120+
if !ok {
121+
t.Fatalf("unexpected object: %v", obj)
122+
}
123+
if !reflect.DeepEqual(*hpa, hpaBeforeMuatate) {
124+
t.Errorf("diff: %v", diff.ObjectDiff(*hpa, hpaBeforeMuatate))
125+
t.Errorf("expected: %#v\n actual: %#v", *hpa, hpaBeforeMuatate)
126+
}
127+
128+
if len(final_obj.ObjectMeta.Annotations) != 0 {
129+
t.Fatalf("unexpected annotations: %v", final_obj.ObjectMeta.Annotations)
130+
}
131+
}
132+
}
133+
70134
func roundTrip(t *testing.T, obj runtime.Object) runtime.Object {
71135
data, err := runtime.Encode(legacyscheme.Codecs.LegacyCodec(SchemeGroupVersion), obj)
72136
if err != nil {

‎pkg/apis/autoscaling/v2beta1/BUILD

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,9 @@ go_test(
3939
"//staging/src/k8s.io/api/autoscaling/v2beta1:go_default_library",
4040
"//staging/src/k8s.io/api/core/v1:go_default_library",
4141
"//staging/src/k8s.io/apimachinery/pkg/api/equality:go_default_library",
42+
"//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
4243
"//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",
44+
"//staging/src/k8s.io/apimachinery/pkg/util/diff:go_default_library",
4345
"//vendor/github.com/stretchr/testify/assert:go_default_library",
4446
"//vendor/k8s.io/utils/pointer:go_default_library",
4547
],

‎pkg/apis/autoscaling/v2beta1/conversion.go

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -289,3 +289,30 @@ func Convert_v2beta1_PodsMetricStatus_To_autoscaling_PodsMetricStatus(in *autosc
289289
}
290290
return nil
291291
}
292+
293+
func Convert_autoscaling_HorizontalPodAutoscaler_To_v2beta1_HorizontalPodAutoscaler(in *autoscaling.HorizontalPodAutoscaler, out *autoscalingv2beta1.HorizontalPodAutoscaler, s conversion.Scope) error {
294+
if err := autoConvert_autoscaling_HorizontalPodAutoscaler_To_v2beta1_HorizontalPodAutoscaler(in, out, s); err != nil {
295+
return err
296+
}
297+
298+
// clear any pre-existing round-trip annotations to make sure the only ones set are ones we produced during conversion
299+
annotations, _ := autoscaling.DropRoundTripHorizontalPodAutoscalerAnnotations(out.Annotations)
300+
out.Annotations = annotations
301+
302+
return nil
303+
}
304+
305+
func Convert_v2beta1_HorizontalPodAutoscaler_To_autoscaling_HorizontalPodAutoscaler(in *autoscalingv2beta1.HorizontalPodAutoscaler, out *autoscaling.HorizontalPodAutoscaler, s conversion.Scope) error {
306+
if err := autoConvert_v2beta1_HorizontalPodAutoscaler_To_autoscaling_HorizontalPodAutoscaler(in, out, s); err != nil {
307+
return err
308+
}
309+
310+
// drop round-tripping annotations after converting to internal
311+
out.Annotations, _ = autoscaling.DropRoundTripHorizontalPodAutoscalerAnnotations(out.Annotations)
312+
313+
return nil
314+
}
315+
316+
func Convert_autoscaling_HorizontalPodAutoscalerSpec_To_v2beta1_HorizontalPodAutoscalerSpec(in *autoscaling.HorizontalPodAutoscalerSpec, out *autoscalingv2beta1.HorizontalPodAutoscalerSpec, s conversion.Scope) error {
317+
return autoConvert_autoscaling_HorizontalPodAutoscalerSpec_To_v2beta1_HorizontalPodAutoscalerSpec(in, out, s)
318+
}

0 commit comments

Comments
 (0)