diff --git a/.chloggen/1115-move-autoscaler-crd-fields.yaml b/.chloggen/1115-move-autoscaler-crd-fields.yaml new file mode 100644 index 0000000000..168a1d93b9 --- /dev/null +++ b/.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: \ No newline at end of file diff --git a/apis/v1alpha1/opentelemetrycollector_types.go b/apis/v1alpha1/opentelemetrycollector_types.go index 3331a5adae..205b931221 100644 --- a/apis/v1alpha1/opentelemetrycollector_types.go +++ b/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. diff --git a/apis/v1alpha1/opentelemetrycollector_webhook.go b/apis/v1alpha1/opentelemetrycollector_webhook.go index 3899218678..66e7d2e421 100644 --- a/apis/v1alpha1/opentelemetrycollector_webhook.go +++ b/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") } diff --git a/apis/v1alpha1/opentelemetrycollector_webhook_test.go b/apis/v1alpha1/opentelemetrycollector_webhook_test.go index 29e29dfe38..f0a7e68942 100644 --- a/apis/v1alpha1/opentelemetrycollector_webhook_test.go +++ b/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{ diff --git a/apis/v1alpha1/zz_generated.deepcopy.go b/apis/v1alpha1/zz_generated.deepcopy.go index 6803230c8c..3aba6ad2e1 100644 --- a/apis/v1alpha1/zz_generated.deepcopy.go +++ b/apis/v1alpha1/zz_generated.deepcopy.go @@ -29,6 +29,16 @@ import ( // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *AutoscalerSpec) DeepCopyInto(out *AutoscalerSpec) { *out = *in + if in.MinReplicas != nil { + in, out := &in.MinReplicas, &out.MinReplicas + *out = new(int32) + **out = **in + } + if in.MaxReplicas != nil { + in, out := &in.MaxReplicas, &out.MaxReplicas + *out = new(int32) + **out = **in + } if in.Behavior != nil { in, out := &in.Behavior, &out.Behavior *out = new(v2.HorizontalPodAutoscalerBehavior) diff --git a/bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml b/bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml index 471ff6b949..778401ecbd 100644 --- a/bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml +++ b/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: diff --git a/config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml b/config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml index 3bd1e672e3..6cbd2e26ab 100644 --- a/config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml +++ b/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: diff --git a/config/manager/kustomization.yaml b/config/manager/kustomization.yaml index 5c5f0b84cb..2e6cc79764 100644 --- a/config/manager/kustomization.yaml +++ b/config/manager/kustomization.yaml @@ -1,2 +1,2 @@ resources: -- manager.yaml +- manager.yaml \ No newline at end of file diff --git a/docs/api.md b/docs/api.md index 73ee92b8de..00f6922f14 100644 --- a/docs/api.md +++ b/docs/api.md @@ -1758,7 +1758,7 @@ OpenTelemetryCollectorSpec defines the desired state of OpenTelemetryCollector. maxReplicas integer - MaxReplicas sets an upper bound to the autoscaling feature. If MaxReplicas is set autoscaling is enabled.
+ 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
@@ -1767,7 +1767,7 @@ OpenTelemetryCollectorSpec defines the desired state of OpenTelemetryCollector. minReplicas integer - MinReplicas sets a lower bound to the autoscaling feature. Set this if your are using autoscaling. It must be at least 1
+ 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
@@ -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).
false + + maxReplicas + integer + + MaxReplicas sets an upper bound to the autoscaling feature. If MaxReplicas is set autoscaling is enabled.
+
+ Format: int32
+ + false + + minReplicas + integer + + MinReplicas sets a lower bound to the autoscaling feature. Set this if your are using autoscaling. It must be at least 1
+
+ Format: int32
+ + false targetCPUUtilization integer diff --git a/pkg/collector/horizontalpodautoscaler.go b/pkg/collector/horizontalpodautoscaler.go index c7205562d9..2d3d74e56a 100644 --- a/pkg/collector/horizontalpodautoscaler.go +++ b/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, }, } diff --git a/pkg/collector/horizontalpodautoscaler_test.go b/pkg/collector/horizontalpodautoscaler_test.go index aa5e1c173b..a24778ba14 100644 --- a/pkg/collector/horizontalpodautoscaler_test.go +++ b/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, }, diff --git a/pkg/collector/reconcile/horizontalpodautoscaler.go b/pkg/collector/reconcile/horizontalpodautoscaler.go index 28f756e361..64f8b0f56a 100644 --- a/pkg/collector/reconcile/horizontalpodautoscaler.go +++ b/pkg/collector/reconcile/horizontalpodautoscaler.go @@ -38,7 +38,7 @@ func HorizontalPodAutoscalers(ctx context.Context, params Params) error { desired := []client.Object{} // check if autoscale mode is on, e.g MaxReplicas is not nil - if params.Instance.Spec.MaxReplicas != nil { + if params.Instance.Spec.MaxReplicas != nil || (params.Instance.Spec.Autoscaler != nil && params.Instance.Spec.Autoscaler.MaxReplicas != nil) { desired = append(desired, collector.HorizontalPodAutoscaler(params.Config, params.Log, params.Instance)) } @@ -112,19 +112,19 @@ func expectedHorizontalPodAutoscalers(ctx context.Context, params Params, expect func setAutoscalerSpec(params Params, autoscalingVersion autodetect.AutoscalingVersion, updated client.Object) { one := int32(1) - if params.Instance.Spec.MaxReplicas != nil { + if params.Instance.Spec.Autoscaler.MaxReplicas != nil { if autoscalingVersion == autodetect.AutoscalingVersionV2Beta2 { - updated.(*autoscalingv2beta2.HorizontalPodAutoscaler).Spec.MaxReplicas = *params.Instance.Spec.MaxReplicas - if params.Instance.Spec.MinReplicas != nil { - updated.(*autoscalingv2beta2.HorizontalPodAutoscaler).Spec.MinReplicas = params.Instance.Spec.MinReplicas + updated.(*autoscalingv2beta2.HorizontalPodAutoscaler).Spec.MaxReplicas = *params.Instance.Spec.Autoscaler.MaxReplicas + if params.Instance.Spec.Autoscaler.MinReplicas != nil { + updated.(*autoscalingv2beta2.HorizontalPodAutoscaler).Spec.MinReplicas = params.Instance.Spec.Autoscaler.MinReplicas } else { updated.(*autoscalingv2beta2.HorizontalPodAutoscaler).Spec.MinReplicas = &one } updated.(*autoscalingv2beta2.HorizontalPodAutoscaler).Spec.Metrics[0].Resource.Target.AverageUtilization = params.Instance.Spec.Autoscaler.TargetCPUUtilization } else { - updated.(*autoscalingv2.HorizontalPodAutoscaler).Spec.MaxReplicas = *params.Instance.Spec.MaxReplicas - if params.Instance.Spec.MinReplicas != nil { - updated.(*autoscalingv2.HorizontalPodAutoscaler).Spec.MinReplicas = params.Instance.Spec.MinReplicas + updated.(*autoscalingv2.HorizontalPodAutoscaler).Spec.MaxReplicas = *params.Instance.Spec.Autoscaler.MaxReplicas + if params.Instance.Spec.Autoscaler.MinReplicas != nil { + updated.(*autoscalingv2.HorizontalPodAutoscaler).Spec.MinReplicas = params.Instance.Spec.Autoscaler.MinReplicas } else { updated.(*autoscalingv2.HorizontalPodAutoscaler).Spec.MinReplicas = &one } diff --git a/pkg/collector/reconcile/horizontalpodautoscaler_test.go b/pkg/collector/reconcile/horizontalpodautoscaler_test.go index 1e978d9cb9..933fa9fea2 100644 --- a/pkg/collector/reconcile/horizontalpodautoscaler_test.go +++ b/pkg/collector/reconcile/horizontalpodautoscaler_test.go @@ -61,8 +61,8 @@ func TestExpectedHPA(t *testing.T) { minReplicas := int32(1) maxReplicas := int32(3) updateParms := paramsWithHPA(autodetect.AutoscalingVersionV2Beta2) - updateParms.Instance.Spec.Replicas = &minReplicas - updateParms.Instance.Spec.MaxReplicas = &maxReplicas + updateParms.Instance.Spec.Autoscaler.MinReplicas = &minReplicas + updateParms.Instance.Spec.Autoscaler.MaxReplicas = &maxReplicas updatedHPA := collector.HorizontalPodAutoscaler(updateParms.Config, logger, updateParms.Instance) if autoscalingVersion == autodetect.AutoscalingVersionV2Beta2 { @@ -148,10 +148,10 @@ func paramsWithHPA(autoscalingVersion autodetect.AutoscalingVersion) Params { }, NodePort: 0, }}, - Config: string(configYAML), - Replicas: &minReplicas, - MaxReplicas: &maxReplicas, + Config: string(configYAML), Autoscaler: &v1alpha1.AutoscalerSpec{ + MinReplicas: &minReplicas, + MaxReplicas: &maxReplicas, TargetCPUUtilization: &cpuUtilization, }, }, diff --git a/tests/e2e/autoscale/00-install.yaml b/tests/e2e/autoscale/00-install.yaml index 15f4aed640..4f4b316e06 100644 --- a/tests/e2e/autoscale/00-install.yaml +++ b/tests/e2e/autoscale/00-install.yaml @@ -6,6 +6,9 @@ kind: OpenTelemetryCollector metadata: name: simplest spec: +# TODO: 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. minReplicas: 1 maxReplicas: 2 autoscaler: