From e0e2d5c0e67efd0b47c074f2a22158d50cfee2b9 Mon Sep 17 00:00:00 2001 From: Sanskar Jaiswal Date: Tue, 7 Jun 2022 16:09:55 +0530 Subject: [PATCH] refactor hpa reconcile logic to be generic for both versions Signed-off-by: Sanskar Jaiswal for objectmeta as well --- pkg/canary/hpa_reconciler.go | 171 ++++++++++++++++++------------ pkg/canary/hpa_reconciler_test.go | 4 + 2 files changed, 105 insertions(+), 70 deletions(-) diff --git a/pkg/canary/hpa_reconciler.go b/pkg/canary/hpa_reconciler.go index e4a2e3e2..10f57909 100644 --- a/pkg/canary/hpa_reconciler.go +++ b/pkg/canary/hpa_reconciler.go @@ -82,41 +82,42 @@ func (hr *HPAReconciler) reconcilePrimaryHpaV2(cd *flaggerv1.Canary, hpa *hpav2. // create HPA if errors.IsNotFound(err) { primaryHpa = &hpav2.HorizontalPodAutoscaler{ - ObjectMeta: metav1.ObjectMeta{ - Name: primaryHpaName, - Namespace: cd.Namespace, - Labels: filterMetadata(hpa.Labels), - OwnerReferences: []metav1.OwnerReference{ - *metav1.NewControllerRef(cd, schema.GroupVersionKind{ - Group: flaggerv1.SchemeGroupVersion.Group, - Version: flaggerv1.SchemeGroupVersion.Version, - Kind: flaggerv1.CanaryKind, - }), - }, - }, - Spec: hpaSpec, + ObjectMeta: makeObjectMeta(primaryHpaName, hpa.Labels, cd), + Spec: hpaSpec, } _, err = hr.kubeClient.AutoscalingV2().HorizontalPodAutoscalers(cd.Namespace).Create(context.TODO(), primaryHpa, metav1.CreateOptions{}) if err != nil { - return fmt.Errorf("creating HorizontalPodAutoscaler %s.%s failed: %w", + return fmt.Errorf("creating HorizontalPodAutoscaler v2 %s.%s failed: %w", primaryHpa.Name, primaryHpa.Namespace, err) } hr.logger.With("canary", fmt.Sprintf("%s.%s", cd.Name, cd.Namespace)).Infof( - "HorizontalPodAutoscaler %s.%s created", primaryHpa.GetName(), cd.Namespace) + "HorizontalPodAutoscaler v2 %s.%s created", primaryHpa.GetName(), cd.Namespace) return nil } else if err != nil { - return fmt.Errorf("HorizontalPodAutoscaler %s.%s get query failed: %w", + return fmt.Errorf("HorizontalPodAutoscaler v2 %s.%s get query failed: %w", primaryHpa.Name, primaryHpa.Namespace, err) } // update HPA if !init && primaryHpa != nil { - diffMetrics := cmp.Diff(hpaSpec.Metrics, primaryHpa.Spec.Metrics) - diffBehavior := cmp.Diff(hpaSpec.Behavior, primaryHpa.Spec.Behavior) - diffLabels := cmp.Diff(hpa.ObjectMeta.Labels, primaryHpa.ObjectMeta.Labels) - diffAnnotations := cmp.Diff(hpa.ObjectMeta.Annotations, primaryHpa.ObjectMeta.Annotations) - if diffMetrics != "" || diffBehavior != "" || diffLabels != "" || diffAnnotations != "" || int32Default(hpaSpec.MinReplicas) != int32Default(primaryHpa.Spec.MinReplicas) || hpaSpec.MaxReplicas != primaryHpa.Spec.MaxReplicas { + targetFields := hpaFields{ + metrics: hpaSpec.Metrics, + behavior: hpaSpec.Behavior, + annotations: hpa.Annotations, + labels: hpa.Labels, + min: hpaSpec.MinReplicas, + max: hpaSpec.MaxReplicas, + } + primaryFields := hpaFields{ + metrics: primaryHpa.Spec.Metrics, + behavior: primaryHpa.Spec.Behavior, + annotations: primaryHpa.Annotations, + labels: primaryHpa.Labels, + min: primaryHpa.Spec.MinReplicas, + max: primaryHpa.Spec.MaxReplicas, + } + if hasHPAChanged(targetFields, primaryFields) { err = retry.RetryOnConflict(retry.DefaultRetry, func() error { primaryHpa, err := hr.kubeClient.AutoscalingV2().HorizontalPodAutoscalers(cd.Namespace).Get(context.TODO(), primaryHpaName, metav1.GetOptions{}) if err != nil { @@ -128,28 +129,17 @@ func (hr *HPAReconciler) reconcilePrimaryHpaV2(cd *flaggerv1.Canary, hpa *hpav2. hpaClone.Spec.Metrics = hpaSpec.Metrics hpaClone.Spec.Behavior = hpaSpec.Behavior - // update hpa annotations - hpaClone.ObjectMeta.Annotations = make(map[string]string) - filteredAnnotations := includeLabelsByPrefix(hpa.ObjectMeta.Annotations, hr.includeLabelPrefix) - for k, v := range filteredAnnotations { - hpaClone.ObjectMeta.Annotations[k] = v - } - // update hpa labels - hpaClone.ObjectMeta.Labels = make(map[string]string) - filteredLabels := includeLabelsByPrefix(hpa.ObjectMeta.Labels, hr.includeLabelPrefix) - for k, v := range filteredLabels { - hpaClone.ObjectMeta.Labels[k] = v - } + hr.updateObjectMeta(hpaClone.ObjectMeta) _, err = hr.kubeClient.AutoscalingV2().HorizontalPodAutoscalers(cd.Namespace).Update(context.TODO(), hpaClone, metav1.UpdateOptions{}) return err }) if err != nil { - return fmt.Errorf("updating HorizontalPodAutoscaler %s.%s failed: %w", + return fmt.Errorf("updating HorizontalPodAutoscaler v2 %s.%s failed: %w", primaryHpa.Name, primaryHpa.Namespace, err) } hr.logger.With("canary", fmt.Sprintf("%s.%s", cd.Name, cd.Namespace)). - Infof("HorizontalPodAutoscaler %s.%s updated", primaryHpa.GetName(), cd.Namespace) + Infof("HorizontalPodAutoscaler v2 %s.%s updated", primaryHpa.GetName(), cd.Namespace) } } return nil @@ -176,41 +166,42 @@ func (hr *HPAReconciler) reconcilePrimaryHpaV2Beta2(cd *flaggerv1.Canary, hpa *h // create HPA if errors.IsNotFound(err) { primaryHpa = &hpav2beta2.HorizontalPodAutoscaler{ - ObjectMeta: metav1.ObjectMeta{ - Name: primaryHpaName, - Namespace: cd.Namespace, - Labels: filterMetadata(hpa.Labels), - OwnerReferences: []metav1.OwnerReference{ - *metav1.NewControllerRef(cd, schema.GroupVersionKind{ - Group: flaggerv1.SchemeGroupVersion.Group, - Version: flaggerv1.SchemeGroupVersion.Version, - Kind: flaggerv1.CanaryKind, - }), - }, - }, - Spec: hpaSpec, + ObjectMeta: makeObjectMeta(primaryHpaName, hpa.Labels, cd), + Spec: hpaSpec, } _, err = hr.kubeClient.AutoscalingV2beta2().HorizontalPodAutoscalers(cd.Namespace).Create(context.TODO(), primaryHpa, metav1.CreateOptions{}) if err != nil { - return fmt.Errorf("creating HorizontalPodAutoscaler %s.%s failed: %w", + return fmt.Errorf("creating HorizontalPodAutoscaler v2beta2 %s.%s failed: %w", primaryHpa.Name, primaryHpa.Namespace, err) } hr.logger.With("canary", fmt.Sprintf("%s.%s", cd.Name, cd.Namespace)).Infof( - "HorizontalPodAutoscaler %s.%s created", primaryHpa.GetName(), cd.Namespace) + "HorizontalPodAutoscaler v2beta2 %s.%s created", primaryHpa.GetName(), cd.Namespace) return nil } else if err != nil { - return fmt.Errorf("HorizontalPodAutoscaler %s.%s get query failed: %w", + return fmt.Errorf("HorizontalPodAutoscaler v2beta2 %s.%s get query failed: %w", primaryHpa.Name, primaryHpa.Namespace, err) } // update HPA if !init && primaryHpa != nil { - diffMetrics := cmp.Diff(hpaSpec.Metrics, primaryHpa.Spec.Metrics) - diffBehavior := cmp.Diff(hpaSpec.Behavior, primaryHpa.Spec.Behavior) - diffLabels := cmp.Diff(hpa.ObjectMeta.Labels, primaryHpa.ObjectMeta.Labels) - diffAnnotations := cmp.Diff(hpa.ObjectMeta.Annotations, primaryHpa.ObjectMeta.Annotations) - if diffMetrics != "" || diffBehavior != "" || diffLabels != "" || diffAnnotations != "" || int32Default(hpaSpec.MinReplicas) != int32Default(primaryHpa.Spec.MinReplicas) || hpaSpec.MaxReplicas != primaryHpa.Spec.MaxReplicas { + targetFields := hpaFields{ + metrics: hpaSpec.Metrics, + behavior: hpaSpec.Behavior, + annotations: hpa.Annotations, + labels: hpa.Labels, + min: hpaSpec.MinReplicas, + max: hpaSpec.MaxReplicas, + } + primaryFields := hpaFields{ + metrics: primaryHpa.Spec.Metrics, + behavior: primaryHpa.Spec.Behavior, + annotations: primaryHpa.Annotations, + labels: primaryHpa.Labels, + min: primaryHpa.Spec.MinReplicas, + max: primaryHpa.Spec.MaxReplicas, + } + if hasHPAChanged(targetFields, primaryFields) { err = retry.RetryOnConflict(retry.DefaultRetry, func() error { primaryHpa, err := hr.kubeClient.AutoscalingV2beta2().HorizontalPodAutoscalers(cd.Namespace).Get(context.TODO(), primaryHpaName, metav1.GetOptions{}) if err != nil { @@ -222,28 +213,17 @@ func (hr *HPAReconciler) reconcilePrimaryHpaV2Beta2(cd *flaggerv1.Canary, hpa *h hpaClone.Spec.Metrics = hpaSpec.Metrics hpaClone.Spec.Behavior = hpaSpec.Behavior - // update hpa annotations - hpaClone.ObjectMeta.Annotations = make(map[string]string) - filteredAnnotations := includeLabelsByPrefix(hpa.ObjectMeta.Annotations, hr.includeLabelPrefix) - for k, v := range filteredAnnotations { - hpaClone.ObjectMeta.Annotations[k] = v - } - // update hpa labels - hpaClone.ObjectMeta.Labels = make(map[string]string) - filteredLabels := includeLabelsByPrefix(hpa.ObjectMeta.Labels, hr.includeLabelPrefix) - for k, v := range filteredLabels { - hpaClone.ObjectMeta.Labels[k] = v - } + hr.updateObjectMeta(hpaClone.ObjectMeta) _, err = hr.kubeClient.AutoscalingV2beta2().HorizontalPodAutoscalers(cd.Namespace).Update(context.TODO(), hpaClone, metav1.UpdateOptions{}) return err }) if err != nil { - return fmt.Errorf("updating HorizontalPodAutoscaler %s.%s failed: %w", + return fmt.Errorf("updating HorizontalPodAutoscaler v2beta2 %s.%s failed: %w", primaryHpa.Name, primaryHpa.Namespace, err) } hr.logger.With("canary", fmt.Sprintf("%s.%s", cd.Name, cd.Namespace)). - Infof("HorizontalPodAutoscaler %s.%s updated", primaryHpa.GetName(), cd.Namespace) + Infof("HorizontalPodAutoscaler v2beta2 %s.%s updated", primaryHpa.GetName(), cd.Namespace) } } return nil @@ -256,3 +236,54 @@ func (hr *HPAReconciler) PauseTargetScaler(cd *flaggerv1.Canary) error { func (hr *HPAReconciler) ResumeTargetScaler(cd *flaggerv1.Canary) error { return nil } + +func (hr *HPAReconciler) updateObjectMeta(meta metav1.ObjectMeta) { + // update hpa annotations + meta.Annotations = make(map[string]string) + filteredAnnotations := includeLabelsByPrefix(meta.Annotations, hr.includeLabelPrefix) + for k, v := range filteredAnnotations { + meta.Annotations[k] = v + } + // update hpa labels + meta.Labels = make(map[string]string) + filteredLabels := includeLabelsByPrefix(meta.Labels, hr.includeLabelPrefix) + for k, v := range filteredLabels { + meta.Labels[k] = v + } +} + +type hpaFields struct { + metrics interface{} + behavior interface{} + annotations map[string]string + labels map[string]string + min *int32 + max int32 +} + +func hasHPAChanged(target, primary hpaFields) bool { + diffMetrics := cmp.Diff(target.metrics, primary.metrics) + diffBehavior := cmp.Diff(target.behavior, primary.behavior) + diffLabels := cmp.Diff(target.labels, primary.labels) + diffAnnotations := cmp.Diff(target.annotations, primary.annotations) + if diffMetrics != "" || diffBehavior != "" || diffLabels != "" || diffAnnotations != "" || + int32Default(target.min) != int32Default(primary.min) || target.max != primary.max { + return true + } + return false +} + +func makeObjectMeta(name string, labels map[string]string, cd *flaggerv1.Canary) metav1.ObjectMeta { + return metav1.ObjectMeta{ + Name: name, + Namespace: cd.Namespace, + Labels: filterMetadata(labels), + OwnerReferences: []metav1.OwnerReference{ + *metav1.NewControllerRef(cd, schema.GroupVersionKind{ + Group: flaggerv1.SchemeGroupVersion.Group, + Version: flaggerv1.SchemeGroupVersion.Version, + Kind: flaggerv1.CanaryKind, + }), + }, + } +} diff --git a/pkg/canary/hpa_reconciler_test.go b/pkg/canary/hpa_reconciler_test.go index a72ff9ca..171a06c0 100644 --- a/pkg/canary/hpa_reconciler_test.go +++ b/pkg/canary/hpa_reconciler_test.go @@ -63,6 +63,7 @@ func Test_reconcilePrimaryHpaV2(t *testing.T) { assert.Equal(t, int(*primaryHPA.Spec.Metrics[0].Resource.Target.AverageUtilization), 99) hpa.Spec.Metrics[0].Resource.Target = hpav2.MetricTarget{AverageUtilization: int32p(50)} + hpa.Spec.MaxReplicas = 10 _, err = mocks.kubeClient.AutoscalingV2().HorizontalPodAutoscalers("default").Update(context.TODO(), hpa, metav1.UpdateOptions{}) require.NoError(t, err) @@ -72,6 +73,7 @@ func Test_reconcilePrimaryHpaV2(t *testing.T) { primaryHPA, err = mocks.kubeClient.AutoscalingV2().HorizontalPodAutoscalers("default").Get(context.TODO(), "podinfo-primary", metav1.GetOptions{}) require.NoError(t, err) assert.Equal(t, int(*primaryHPA.Spec.Metrics[0].Resource.Target.AverageUtilization), 50) + assert.Equal(t, int(primaryHPA.Spec.MaxReplicas), 10) } func Test_reconcilePrimaryHpaV2Beta2(t *testing.T) { @@ -94,6 +96,7 @@ func Test_reconcilePrimaryHpaV2Beta2(t *testing.T) { assert.Equal(t, int(*primaryHPA.Spec.Metrics[0].Resource.Target.AverageUtilization), 99) hpa.Spec.Metrics[0].Resource.Target = hpav2beta2.MetricTarget{AverageUtilization: int32p(50)} + hpa.Spec.MaxReplicas = 10 _, err = mocks.kubeClient.AutoscalingV2beta2().HorizontalPodAutoscalers("default").Update(context.TODO(), hpa, metav1.UpdateOptions{}) require.NoError(t, err) @@ -103,4 +106,5 @@ func Test_reconcilePrimaryHpaV2Beta2(t *testing.T) { primaryHPA, err = mocks.kubeClient.AutoscalingV2beta2().HorizontalPodAutoscalers("default").Get(context.TODO(), "podinfo-primary", metav1.GetOptions{}) require.NoError(t, err) assert.Equal(t, int(*primaryHPA.Spec.Metrics[0].Resource.Target.AverageUtilization), 50) + assert.Equal(t, int(primaryHPA.Spec.MaxReplicas), 10) }