refactor hpa reconcile logic to be generic for both versions

Signed-off-by: Sanskar Jaiswal <sanskar.jaiswal@weave.works> for objectmeta as well
2026-04-15 06:57:34 +00:00 · 2022-06-08 12:25:35 +05:30
parent 9b97bff7b1
commit e0e2d5c0e6
2 changed files with 105 additions and 70 deletions
@@ -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,
+			}),
+		},
+	}
+}
@@ -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)
 }