[HPA] Move maxReplicas and minReplicas to AutoscalerSpec (#1302)

* move maxReplics and minReplicas to Autoscaler field

* fix descriptions

* run make generate

* make bundle

* make api-docs

* fix unit test

* fix nil pointer dereference

* use .Spec.Replicas if MinReplicas is not set

* initialize pointers

* set .autoscaler.[max|min]Replicas in default webhook

* updating reconcile for hpa

* addressing review feedback

* add comment

* gofmt

* chloggen

.chloggen/1115-move-autoscaler-crd-fields.yaml

@@ -0,0 +1,17 @@
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: deprecation 
+
+# The name of the component, or a single word describing the area of concern, (e.g. operator, target allocator, github action)
+component: Autoscaler
+
+# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: This PR moves MaxReplicas and MinReplicas to .Spec.Autoscaler
+
+# One or more tracking issues related to the change
+issues:
+  - 1115
+
+# (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:

apis/v1alpha1/opentelemetrycollector_types.go

@@ -84,9 +84,11 @@ type OpenTelemetryCollectorSpec struct {
 	Replicas *int32 `json:"replicas,omitempty"`
 	// MinReplicas sets a lower bound to the autoscaling feature.  Set this if your are using autoscaling. It must be at least 1
 	// +optional
+	// Deprecated: use "OpenTelemetryCollector.Spec.Autoscaler.MinReplicas" instead.
 	MinReplicas *int32 `json:"minReplicas,omitempty"`
 	// MaxReplicas sets an upper bound to the autoscaling feature. If MaxReplicas is set autoscaling is enabled.
 	// +optional
+	// Deprecated: use "OpenTelemetryCollector.Spec.Autoscaler.MaxReplicas" instead.
 	MaxReplicas *int32 `json:"maxReplicas,omitempty"`
 	// Autoscaler specifies the pod autoscaling configuration to use
 	// for the OpenTelemetryCollector workload.
@@ -289,6 +291,12 @@ type OpenTelemetryCollectorList struct {
 
 // AutoscalerSpec defines the OpenTelemetryCollector's pod autoscaling specification.
 type AutoscalerSpec struct {
+	// MinReplicas sets a lower bound to the autoscaling feature.  Set this if your are using autoscaling. It must be at least 1
+	// +optional
+	MinReplicas *int32 `json:"minReplicas,omitempty"`
+	// MaxReplicas sets an upper bound to the autoscaling feature. If MaxReplicas is set autoscaling is enabled.
+	// +optional
+	MaxReplicas *int32 `json:"maxReplicas,omitempty"`
 	// +optional
 	Behavior *autoscalingv2.HorizontalPodAutoscalerBehavior `json:"behavior,omitempty"`
 	// TargetCPUUtilization sets the target average CPU used across all replicas.

apis/v1alpha1/opentelemetrycollector_webhook.go

@@ -67,11 +67,22 @@ func (r *OpenTelemetryCollector) Default() {
 		r.Spec.TargetAllocator.Replicas = &one
 	}
 
-	if r.Spec.MaxReplicas != nil {
+	if r.Spec.MaxReplicas != nil || (r.Spec.Autoscaler != nil && r.Spec.Autoscaler.MaxReplicas != nil) {
 		if r.Spec.Autoscaler == nil {
 			r.Spec.Autoscaler = &AutoscalerSpec{}
 		}
 
+		if r.Spec.Autoscaler.MaxReplicas == nil {
+			r.Spec.Autoscaler.MaxReplicas = r.Spec.MaxReplicas
+		}
+		if r.Spec.Autoscaler.MinReplicas == nil {
+			if r.Spec.MinReplicas != nil {
+				r.Spec.Autoscaler.MinReplicas = r.Spec.MinReplicas
+			} else {
+				r.Spec.Autoscaler.MinReplicas = r.Spec.Replicas
+			}
+		}
+
 		if r.Spec.Autoscaler.TargetMemoryUtilization == nil && r.Spec.Autoscaler.TargetCPUUtilization == nil {
 			defaultCPUTarget := int32(90)
 			r.Spec.Autoscaler.TargetCPUUtilization = &defaultCPUTarget
@@ -149,21 +160,45 @@ func (r *OpenTelemetryCollector) validateCRDSpec() error {
 		}
 	}
 
+	maxReplicas := new(int32)
+	if r.Spec.Autoscaler != nil && r.Spec.Autoscaler.MaxReplicas != nil {
+		maxReplicas = r.Spec.Autoscaler.MaxReplicas
+	}
+
+	// check deprecated .Spec.MaxReplicas if maxReplicas is not set
+	if *maxReplicas == 0 {
+		maxReplicas = r.Spec.MaxReplicas
+	}
+
+	minReplicas := new(int32)
+	if r.Spec.Autoscaler != nil && r.Spec.Autoscaler.MinReplicas != nil {
+		minReplicas = r.Spec.Autoscaler.MinReplicas
+	}
+
+	// check deprecated .Spec.MinReplicas if minReplicas is not set
+	if *minReplicas == 0 {
+		if r.Spec.MinReplicas != nil {
+			minReplicas = r.Spec.MinReplicas
+		} else {
+			minReplicas = r.Spec.Replicas
+		}
+	}
+
 	// validate autoscale with horizontal pod autoscaler
-	if r.Spec.MaxReplicas != nil {
-		if *r.Spec.MaxReplicas < int32(1) {
+	if maxReplicas != nil {
+		if *maxReplicas < int32(1) {
 			return fmt.Errorf("the OpenTelemetry Spec autoscale configuration is incorrect, maxReplicas should be defined and one or more")
 		}
 
-		if r.Spec.Replicas != nil && *r.Spec.Replicas > *r.Spec.MaxReplicas {
+		if r.Spec.Replicas != nil && *r.Spec.Replicas > *maxReplicas {
 			return fmt.Errorf("the OpenTelemetry Spec autoscale configuration is incorrect, replicas must not be greater than maxReplicas")
 		}
 
-		if r.Spec.MinReplicas != nil && *r.Spec.MinReplicas > *r.Spec.MaxReplicas {
+		if minReplicas != nil && *minReplicas > *maxReplicas {
 			return fmt.Errorf("the OpenTelemetry Spec autoscale configuration is incorrect, minReplicas must not be greater than maxReplicas")
 		}
 
-		if r.Spec.MinReplicas != nil && *r.Spec.MinReplicas < int32(1) {
+		if minReplicas != nil && *minReplicas < int32(1) {
 			return fmt.Errorf("the OpenTelemetry Spec autoscale configuration is incorrect, minReplicas should be one or more")
 		}
 

apis/v1alpha1/opentelemetrycollector_webhook_test.go

@@ -72,6 +72,34 @@ func TestOTELColDefaultingWebhook(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "Setting Autoscaler MaxReplicas",
+			otelcol: OpenTelemetryCollector{
+				Spec: OpenTelemetryCollectorSpec{
+					Autoscaler: &AutoscalerSpec{
+						MaxReplicas: &five,
+						MinReplicas: &one,
+					},
+				},
+			},
+			expected: OpenTelemetryCollector{
+				ObjectMeta: metav1.ObjectMeta{
+					Labels: map[string]string{
+						"app.kubernetes.io/managed-by": "opentelemetry-operator",
+					},
+				},
+				Spec: OpenTelemetryCollectorSpec{
+					Mode:            ModeDeployment,
+					Replicas:        &one,
+					UpgradeStrategy: UpgradeStrategyAutomatic,
+					Autoscaler: &AutoscalerSpec{
+						TargetCPUUtilization: &defaultCPUTarget,
+						MaxReplicas:          &five,
+						MinReplicas:          &one,
+					},
+				},
+			},
+		},
 		{
 			name: "MaxReplicas but no Autoscale",
 			otelcol: OpenTelemetryCollector{
@@ -91,6 +119,10 @@ func TestOTELColDefaultingWebhook(t *testing.T) {
 					UpgradeStrategy: UpgradeStrategyAutomatic,
 					Autoscaler: &AutoscalerSpec{
 						TargetCPUUtilization: &defaultCPUTarget,
+						// webhook Default adds MaxReplicas to Autoscaler because
+						// OpenTelemetryCollector.Spec.MaxReplicas is deprecated.
+						MaxReplicas: &five,
+						MinReplicas: &one,
 					},
 					MaxReplicas: &five,
 				},
@@ -135,6 +167,9 @@ func TestOTELColDefaultingWebhook(t *testing.T) {
 	}
 }
 
+// TODO: a lot of these tests use .Spec.MaxReplicas and .Spec.MinReplicas. These fields are
+// deprecated and moved to .Spec.Autoscaler. Fine to use these fields to test that old CRD is
+// still supported but should eventually be updated.
 func TestOTELColValidatingWebhook(t *testing.T) {
 	zero := int32(0)
 	one := int32(1)
@@ -373,6 +408,18 @@ func TestOTELColValidatingWebhook(t *testing.T) {
 			},
 			expectedErr: "targetCPUUtilization should be greater than 0 and less than 100",
 		},
+		{
+			name: "autoscaler minReplicas is less than maxReplicas",
+			otelcol: OpenTelemetryCollector{
+				Spec: OpenTelemetryCollectorSpec{
+					Autoscaler: &AutoscalerSpec{
+						MaxReplicas: &one,
+						MinReplicas: &five,
+					},
+				},
+			},
+			expectedErr: "the OpenTelemetry Spec autoscale configuration is incorrect, minReplicas must not be greater than maxReplicas",
+		},
 		{
 			name: "invalid deployment mode incompabible with ingress settings",
 			otelcol: OpenTelemetryCollector{

bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml

@@ -1010,6 +1010,17 @@ spec:
                             type: integer
                         type: object
                     type: object
+                  maxReplicas:
+                    description: MaxReplicas sets an upper bound to the autoscaling
+                      feature. If MaxReplicas is set autoscaling is enabled.
+                    format: int32
+                    type: integer
+                  minReplicas:
+                    description: MinReplicas sets a lower bound to the autoscaling
+                      feature.  Set this if your are using autoscaling. It must be
+                      at least 1
+                    format: int32
+                    type: integer
                   targetCPUUtilization:
                     description: TargetCPUUtilization sets the target average CPU
                       used across all replicas. If average CPU exceeds this value,
@@ -1254,13 +1265,15 @@ spec:
                     type: string
                 type: object
               maxReplicas:
-                description: MaxReplicas sets an upper bound to the autoscaling feature.
-                  If MaxReplicas is set autoscaling is enabled.
+                description: 'MaxReplicas sets an upper bound to the autoscaling feature.
+                  If MaxReplicas is set autoscaling is enabled. Deprecated: use "OpenTelemetryCollector.Spec.Autoscaler.MaxReplicas"
+                  instead.'
                 format: int32
                 type: integer
               minReplicas:
-                description: MinReplicas sets a lower bound to the autoscaling feature.  Set
-                  this if your are using autoscaling. It must be at least 1
+                description: 'MinReplicas sets a lower bound to the autoscaling feature.  Set
+                  this if your are using autoscaling. It must be at least 1 Deprecated:
+                  use "OpenTelemetryCollector.Spec.Autoscaler.MinReplicas" instead.'
                 format: int32
                 type: integer
               mode:

config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml

@@ -1008,6 +1008,17 @@ spec:
                             type: integer
                         type: object
                     type: object
+                  maxReplicas:
+                    description: MaxReplicas sets an upper bound to the autoscaling
+                      feature. If MaxReplicas is set autoscaling is enabled.
+                    format: int32
+                    type: integer
+                  minReplicas:
+                    description: MinReplicas sets a lower bound to the autoscaling
+                      feature.  Set this if your are using autoscaling. It must be
+                      at least 1
+                    format: int32
+                    type: integer
                   targetCPUUtilization:
                     description: TargetCPUUtilization sets the target average CPU
                       used across all replicas. If average CPU exceeds this value,
@@ -1252,13 +1263,15 @@ spec:
                     type: string
                 type: object
               maxReplicas:
-                description: MaxReplicas sets an upper bound to the autoscaling feature.
-                  If MaxReplicas is set autoscaling is enabled.
+                description: 'MaxReplicas sets an upper bound to the autoscaling feature.
+                  If MaxReplicas is set autoscaling is enabled. Deprecated: use "OpenTelemetryCollector.Spec.Autoscaler.MaxReplicas"
+                  instead.'
                 format: int32
                 type: integer
               minReplicas:
-                description: MinReplicas sets a lower bound to the autoscaling feature.  Set
-                  this if your are using autoscaling. It must be at least 1
+                description: 'MinReplicas sets a lower bound to the autoscaling feature.  Set
+                  this if your are using autoscaling. It must be at least 1 Deprecated:
+                  use "OpenTelemetryCollector.Spec.Autoscaler.MinReplicas" instead.'
                 format: int32
                 type: integer
               mode:

config/manager/kustomization.yaml

@@ -1,2 +1,2 @@
 resources:
-- manager.yaml
+- manager.yaml

docs/api.md

@@ -1758,7 +1758,7 @@ OpenTelemetryCollectorSpec defines the desired state of OpenTelemetryCollector.
         <td><b>maxReplicas</b></td>
         <td>integer</td>
         <td>
-          MaxReplicas sets an upper bound to the autoscaling feature. If MaxReplicas is set autoscaling is enabled.<br/>
+          MaxReplicas sets an upper bound to the autoscaling feature. If MaxReplicas is set autoscaling is enabled. Deprecated: use "OpenTelemetryCollector.Spec.Autoscaler.MaxReplicas" instead.<br/>
           <br/>
             <i>Format</i>: int32<br/>
         </td>
@@ -1767,7 +1767,7 @@ OpenTelemetryCollectorSpec defines the desired state of OpenTelemetryCollector.
         <td><b>minReplicas</b></td>
         <td>integer</td>
         <td>
-          MinReplicas sets a lower bound to the autoscaling feature.  Set this if your are using autoscaling. It must be at least 1<br/>
+          MinReplicas sets a lower bound to the autoscaling feature.  Set this if your are using autoscaling. It must be at least 1 Deprecated: use "OpenTelemetryCollector.Spec.Autoscaler.MinReplicas" instead.<br/>
           <br/>
             <i>Format</i>: int32<br/>
         </td>
@@ -3219,6 +3219,24 @@ Autoscaler specifies the pod autoscaling configuration to use for the OpenTeleme
           HorizontalPodAutoscalerBehavior configures the scaling behavior of the target in both Up and Down directions (scaleUp and scaleDown fields respectively).<br/>
         </td>
         <td>false</td>
+      </tr><tr>
+        <td><b>maxReplicas</b></td>
+        <td>integer</td>
+        <td>
+          MaxReplicas sets an upper bound to the autoscaling feature. If MaxReplicas is set autoscaling is enabled.<br/>
+          <br/>
+            <i>Format</i>: int32<br/>
+        </td>
+        <td>false</td>
+      </tr><tr>
+        <td><b>minReplicas</b></td>
+        <td>integer</td>
+        <td>
+          MinReplicas sets a lower bound to the autoscaling feature.  Set this if your are using autoscaling. It must be at least 1<br/>
+          <br/>
+            <i>Format</i>: int32<br/>
+        </td>
+        <td>false</td>
       </tr><tr>
         <td><b>targetCPUUtilization</b></td>
         <td>integer</td>

pkg/collector/horizontalpodautoscaler.go

@@ -80,8 +80,8 @@ func HorizontalPodAutoscaler(cfg config.Config, logger logr.Logger, otelcol v1al
 					Kind:       "OpenTelemetryCollector",
 					Name:       naming.OpenTelemetryCollector(otelcol),
 				},
-				MinReplicas: otelcol.Spec.Replicas,
-				MaxReplicas: *otelcol.Spec.MaxReplicas,
+				MinReplicas: otelcol.Spec.Autoscaler.MinReplicas,
+				MaxReplicas: *otelcol.Spec.Autoscaler.MaxReplicas,
 				Metrics:     metrics,
 			},
 		}
@@ -131,8 +131,8 @@ func HorizontalPodAutoscaler(cfg config.Config, logger logr.Logger, otelcol v1al
 					Kind:       "OpenTelemetryCollector",
 					Name:       naming.OpenTelemetryCollector(otelcol),
 				},
-				MinReplicas: otelcol.Spec.Replicas,
-				MaxReplicas: *otelcol.Spec.MaxReplicas,
+				MinReplicas: otelcol.Spec.Autoscaler.MinReplicas,
+				MaxReplicas: *otelcol.Spec.Autoscaler.MaxReplicas,
 				Metrics:     metrics,
 			},
 		}

pkg/collector/horizontalpodautoscaler_test.go

@@ -49,9 +49,9 @@ func TestHPA(t *testing.T) {
 			Name: "my-instance",
 		},
 		Spec: v1alpha1.OpenTelemetryCollectorSpec{
-			Replicas:    &minReplicas,
-			MaxReplicas: &maxReplicas,
 			Autoscaler: &v1alpha1.AutoscalerSpec{
+				MinReplicas:             &minReplicas,
+				MaxReplicas:             &maxReplicas,
 				TargetCPUUtilization:    &cpuUtilization,
 				TargetMemoryUtilization: &memoryUtilization,
 			},