Fix Operator crash while upgrading OTEL Collector instance using the …

…old approach for the HPA (#1800) * Fix crash while upgrading one OTEL Collector instance using the old approach for setting up the HPA Signed-off-by: Israel Blancas <[email protected]> * Add changelog Signed-off-by: Israel Blancas <[email protected]> * Fix lint Signed-off-by: Israel Blancas <[email protected]> * Add testing Signed-off-by: Israel Blancas <[email protected]> * Apply changes requested in CR Signed-off-by: Israel Blancas <[email protected]> * Change the code to follow a similar approach to the defaulter webhook --------- Signed-off-by: Israel Blancas <[email protected]>
open-telemetry · Jun 9, 2023 · 846046c · 846046c
1 parent 4534a6b
commit 846046c
Show file tree

Hide file tree

Showing 3 changed files with 98 additions and 52 deletions.
diff --git a/.chloggen/1799-fix-crash-upgrading-collector-with-old-hpa.yaml b/.chloggen/1799-fix-crash-upgrading-collector-with-old-hpa.yaml
@@ -0,0 +1,16 @@
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: bug_fix
+
+# The name of the component, or a single word describing the area of concern, (e.g. operator, target allocator, github action)
+component: operator
+
+# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: Fix the upgrade mechanism to not crash when one OTEL Collector instance uses the old approach to set the autoscaler.
+
+# One or more tracking issues related to the change
+issues: [1799]
+
+# (Optional) One or more lines of additional information to render under the primary note.
+# These lines will be padded with 2 spaces and then inserted directly into the document.
+# Use pipe (|) for multiline entries.
+subtext:
diff --git a/pkg/collector/horizontalpodautoscaler.go b/pkg/collector/horizontalpodautoscaler.go
@@ -49,6 +49,18 @@ func HorizontalPodAutoscaler(cfg config.Config, logger logr.Logger, otelcol v1al
 		return nil
 	}
 
+	if otelcol.Spec.Autoscaler.MaxReplicas == nil {
+		otelcol.Spec.Autoscaler.MaxReplicas = otelcol.Spec.MaxReplicas
+	}
+
+	if otelcol.Spec.Autoscaler.MinReplicas == nil {
+		if otelcol.Spec.MinReplicas != nil {
+			otelcol.Spec.Autoscaler.MinReplicas = otelcol.Spec.MinReplicas
+		} else {
+			otelcol.Spec.Autoscaler.MinReplicas = otelcol.Spec.Replicas
+		}
+	}
+
 	if autoscalingVersion == autodetect.AutoscalingVersionV2Beta2 {
 		metrics := []autoscalingv2beta2.MetricSpec{}
 

diff --git a/pkg/collector/horizontalpodautoscaler_test.go b/pkg/collector/horizontalpodautoscaler_test.go
@@ -44,67 +44,85 @@ func TestHPA(t *testing.T) {
 	var cpuUtilization int32 = 66
 	var memoryUtilization int32 = 77
 
-	otelcol := v1alpha1.OpenTelemetryCollector{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: "my-instance",
+	otelcols := []v1alpha1.OpenTelemetryCollector{
+		{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "my-instance",
+			},
+			Spec: v1alpha1.OpenTelemetryCollectorSpec{
+				Autoscaler: &v1alpha1.AutoscalerSpec{
+					MinReplicas:             &minReplicas,
+					MaxReplicas:             &maxReplicas,
+					TargetCPUUtilization:    &cpuUtilization,
+					TargetMemoryUtilization: &memoryUtilization,
+				},
+			},
 		},
-		Spec: v1alpha1.OpenTelemetryCollectorSpec{
-			Autoscaler: &v1alpha1.AutoscalerSpec{
-				MinReplicas:             &minReplicas,
-				MaxReplicas:             &maxReplicas,
-				TargetCPUUtilization:    &cpuUtilization,
-				TargetMemoryUtilization: &memoryUtilization,
+		{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "my-instance",
+			},
+			Spec: v1alpha1.OpenTelemetryCollectorSpec{
+				MinReplicas: &minReplicas,
+				MaxReplicas: &maxReplicas,
+				Autoscaler: &v1alpha1.AutoscalerSpec{
+					TargetCPUUtilization:    &cpuUtilization,
+					TargetMemoryUtilization: &memoryUtilization,
+				},
 			},
 		},
 	}
 
-	for _, test := range tests {
-		t.Run(test.name, func(t *testing.T) {
-			mockAutoDetector := &mockAutoDetect{
-				HPAVersionFunc: func() (autodetect.AutoscalingVersion, error) {
-					return test.autoscalingVersion, nil
-				},
-			}
-			configuration := config.New(config.WithAutoDetect(mockAutoDetector))
-			err := configuration.AutoDetect()
-			assert.NoError(t, err)
-			raw := HorizontalPodAutoscaler(configuration, logger, otelcol)
-
-			if configuration.AutoscalingVersion() == autodetect.AutoscalingVersionV2Beta2 {
-				hpa := raw.(*autoscalingv2beta2.HorizontalPodAutoscaler)
-
-				// verify
-				assert.Equal(t, "my-instance-collector", hpa.Name)
-				assert.Equal(t, "my-instance-collector", hpa.Labels["app.kubernetes.io/name"])
-				assert.Equal(t, int32(3), *hpa.Spec.MinReplicas)
-				assert.Equal(t, int32(5), hpa.Spec.MaxReplicas)
-				for _, metric := range hpa.Spec.Metrics {
-					if metric.Resource.Name == corev1.ResourceCPU {
-						assert.Equal(t, cpuUtilization, *metric.Resource.Target.AverageUtilization)
-					} else if metric.Resource.Name == corev1.ResourceMemory {
-						assert.Equal(t, memoryUtilization, *metric.Resource.Target.AverageUtilization)
-					}
+	for _, otelcol := range otelcols {
+		for _, test := range tests {
+			t.Run(test.name, func(t *testing.T) {
+				mockAutoDetector := &mockAutoDetect{
+					HPAVersionFunc: func() (autodetect.AutoscalingVersion, error) {
+						return test.autoscalingVersion, nil
+					},
 				}
-			} else {
-				hpa := raw.(*autoscalingv2.HorizontalPodAutoscaler)
-
-				// verify
-				assert.Equal(t, "my-instance-collector", hpa.Name)
-				assert.Equal(t, "my-instance-collector", hpa.Labels["app.kubernetes.io/name"])
-				assert.Equal(t, int32(3), *hpa.Spec.MinReplicas)
-				assert.Equal(t, int32(5), hpa.Spec.MaxReplicas)
-				assert.Equal(t, 2, len(hpa.Spec.Metrics))
-
-				for _, metric := range hpa.Spec.Metrics {
-					if metric.Resource.Name == corev1.ResourceCPU {
-						assert.Equal(t, cpuUtilization, *metric.Resource.Target.AverageUtilization)
-					} else if metric.Resource.Name == corev1.ResourceMemory {
-						assert.Equal(t, memoryUtilization, *metric.Resource.Target.AverageUtilization)
+				configuration := config.New(config.WithAutoDetect(mockAutoDetector))
+				err := configuration.AutoDetect()
+				assert.NoError(t, err)
+				raw := HorizontalPodAutoscaler(configuration, logger, otelcol)
+
+				if configuration.AutoscalingVersion() == autodetect.AutoscalingVersionV2Beta2 {
+					hpa := raw.(*autoscalingv2beta2.HorizontalPodAutoscaler)
+
+					// verify
+					assert.Equal(t, "my-instance-collector", hpa.Name)
+					assert.Equal(t, "my-instance-collector", hpa.Labels["app.kubernetes.io/name"])
+					assert.Equal(t, int32(3), *hpa.Spec.MinReplicas)
+					assert.Equal(t, int32(5), hpa.Spec.MaxReplicas)
+					for _, metric := range hpa.Spec.Metrics {
+						if metric.Resource.Name == corev1.ResourceCPU {
+							assert.Equal(t, cpuUtilization, *metric.Resource.Target.AverageUtilization)
+						} else if metric.Resource.Name == corev1.ResourceMemory {
+							assert.Equal(t, memoryUtilization, *metric.Resource.Target.AverageUtilization)
+						}
+					}
+				} else {
+					hpa := raw.(*autoscalingv2.HorizontalPodAutoscaler)
+
+					// verify
+					assert.Equal(t, "my-instance-collector", hpa.Name)
+					assert.Equal(t, "my-instance-collector", hpa.Labels["app.kubernetes.io/name"])
+					assert.Equal(t, int32(3), *hpa.Spec.MinReplicas)
+					assert.Equal(t, int32(5), hpa.Spec.MaxReplicas)
+					assert.Equal(t, 2, len(hpa.Spec.Metrics))
+
+					for _, metric := range hpa.Spec.Metrics {
+						if metric.Resource.Name == corev1.ResourceCPU {
+							assert.Equal(t, cpuUtilization, *metric.Resource.Target.AverageUtilization)
+						} else if metric.Resource.Name == corev1.ResourceMemory {
+							assert.Equal(t, memoryUtilization, *metric.Resource.Target.AverageUtilization)
+						}
 					}
 				}
-			}
-		})
+			})
+		}
 	}
+
 }
 
 func TestConvertToV2Beta2PodMetrics(t *testing.T) {