add autoscale option to enable support for Horizontal Pod Autoscaling #746

binjip978 · 2022-02-26T13:34:52Z

Resolves #729

Signed-off-by: Sergei Semenchuk <[email protected]>

avadhut123pisal · 2022-03-02T10:50:22Z

apis/v1alpha1/opentelemetrycollector_webhook.go

+		}
+
+		if r.Spec.MinReplicas != nil && *r.Spec.MinReplicas > *r.Spec.MaxReplicas {
+			return fmt.Errorf("the OpenTelemetry Spec autoscale configuration is incorrect, minReplicas could be more than maxReplicas")


minReplicas could be more than maxReplicas

The error message is incorrect, it should be like "MinReplicas value must not be greater than MaxReplicas value".

avadhut123pisal · 2022-03-02T11:21:14Z

pkg/collector/horizontalpodautoscaler.go

+	labels["app.kubernetes.io/name"] = naming.Collector(otelcol)
+
+	annotations := Annotations(otelcol)
+	var cpuTarget int32 = 90


var cpuTarget int32 = 90

The value of the cpuTarget is hardcoded here. I think we should take the input of TargetCPUUtilizationPercentage as a part of OpenTelemetryCollectorSpec.

yes, that make sense to do, but I initially tried to be consistent with jaeger operator https://github.com/jaegertracing/jaeger-operator/pull/856/files where it also hardcoded.

It's fine for the first version.

I would recommend adding a comment and extracting the value to a constant.

Signed-off-by: Sergei Semenchuk <[email protected]>

pavolloffay · 2022-03-04T09:03:43Z

apis/v1alpha1/opentelemetrycollector_types.go

@@ -33,10 +33,22 @@ type OpenTelemetryCollectorSpec struct {
 	// +optional
 	Args map[string]string `json:"args,omitempty"`

+	// Autoscale turns on/off the autoscale feature. By default, it's enabled if the Replicas field is not set.
+	// +optional
+	Autoscale *bool `json:"autoscale,omitempty"`


Should this be set to true in the defaulting webhook if replicas is nil?

not sure, right now the default is one replica, if you don't specify replicas, I tried to not change default behaviour

pavolloffay · 2022-03-04T09:06:29Z

apis/v1alpha1/opentelemetrycollector_types.go

+
+	// MaxReplicas sets an upper bound to the autoscaling feature. When autoscaling is enabled and no value is provided, a default value is used.
+	// +optional
+	MaxReplicas *int32 `json:"maxReplicas,omitempty"`


I have a proposal to make the config/CR simpler.

Could we add only MaxReplicas field in this PR to support autostacling?

If the max replicas is set the operator would create HPA. In the validating webhook it would check if maxReplicas is higher than replicas.

It seems that MaxReplicas can be nil (e.g. for infinite scaling) in this case we need Autoscale flag to enable the HPA.

Probably we can do It (remove autoscale), https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/horizontal-pod-autoscaler-v1/#HorizontalPodAutoscalerSpec
because maxReplicas is required:

maxReplicas (int32), required
upper limit for the number of pods that can be set by the autoscaler; cannot be smaller than MinReplicas.

pavolloffay · 2022-03-04T09:07:45Z

pkg/collector/horizontalpodautoscaler.go

+	labels["app.kubernetes.io/name"] = naming.Collector(otelcol)
+
+	annotations := Annotations(otelcol)
+	var cpuTarget int32 = 90


It's fine for the first version.

I would recommend adding a comment and extracting the value to a constant.

pavolloffay · 2022-03-04T09:14:07Z

pkg/collector/reconcile/horizontalpodautoscaler.go

+			}
+			params.Log.V(2).Info("created", "hpa.name", desired.Name, "hpa.namespace", desired.Namespace)
+			continue
+		} else if err != nil {


This construct can be made easier to read e.g.

if err != nil { if isNotFound(err) { // create } else { return fmt.Errorf("failed to get %w", err) } }

or the following if isNotFound can be called with nil (I think it can)

if isNotFound(err){ } else err != nil { }

Signed-off-by: Sergei Semenchuk <[email protected]>

lower Signed-off-by: Sergei Semenchuk <[email protected]>

pavolloffay

LGTM, just some minor comments

pavolloffay · 2022-03-07T17:29:50Z

apis/v1alpha1/opentelemetrycollector_webhook.go

@@ -109,5 +109,16 @@ func (r *OpenTelemetryCollector) validateCRDSpec() error {
 		}
 	}

+	// validate autoscale with horizontal pod autoscaler
+	if r.Spec.MaxReplicas != nil {
+		if r.Spec.MaxReplicas == nil || *r.Spec.MaxReplicas < int32(1) {


r.Spec.MaxReplicas == nil can be removed

pavolloffay · 2022-03-07T17:32:18Z

apis/v1alpha1/opentelemetrycollector_webhook.go

+			return fmt.Errorf("the OpenTelemetry Spec autoscale configuration is incorrect, maxReplicas should be defined and more than one")
+		}
+
+		if r.Spec.Replicas != nil && *r.Spec.Replicas > *r.Spec.MaxReplicas {


while we are at this, could you please set the Replicas in the defaulting webhook if it is null and remove other code that sets the replicas outside of the defaulting webhook?

not sure that I got this right, do you ask to set Spec.Replicas equal to 1 in Default() if r.Spec.Replicas == nil? Because I can see where we set Spec.Replicas outside of it, only in tests and here:

// if autoscale is enabled, use replicas from current Status if params.Instance.Spec.MaxReplicas != nil { currentReplicas := existing.Status.Replicas // if replicas (minReplicas from HPA perspective) is bigger than // current status use it. if *params.Instance.Spec.Replicas > currentReplicas { currentReplicas = *params.Instance.Spec.Replicas } updated.Spec.Replicas = &currentReplicas }

The replicas should be set to 1 in deployment if it is nil.

It's fine I will take a look at it in another PR.

Signed-off-by: Sergei Semenchuk <[email protected]>

…open-telemetry#746) * add autoscale, minReplicas maxReplicas to collector types Signed-off-by: Sergei Semenchuk <[email protected]> * manage hpa with min/max replicas if autoscale is enablad Signed-off-by: Sergei Semenchuk <[email protected]> * fix linter Signed-off-by: Sergei Semenchuk <[email protected]> * use status if autoscale is true Signed-off-by: Sergei Semenchuk <[email protected]> * add validation for autoscale Signed-off-by: Sergei Semenchuk <[email protected]> * remove minReplicas Signed-off-by: Sergei Semenchuk <[email protected]> * remove autoscale parameter, trigger HPA if maxReplicas is not nil Signed-off-by: Sergei Semenchuk <[email protected]> * use collector replicas for minReplicas in HPA if HPA status is currently lower Signed-off-by: Sergei Semenchuk <[email protected]> * remove nil check Signed-off-by: Sergei Semenchuk <[email protected]>

binjip978 added 5 commits February 21, 2022 14:39

add autoscale, minReplicas maxReplicas to collector types

449b51b

Signed-off-by: Sergei Semenchuk <[email protected]>

manage hpa with min/max replicas if autoscale is enablad

e9a09b2

Signed-off-by: Sergei Semenchuk <[email protected]>

Merge branch 'main' into hpa-support

2ba820c

fix linter

0c3f204

Signed-off-by: Sergei Semenchuk <[email protected]>

use status if autoscale is true

5600157

Signed-off-by: Sergei Semenchuk <[email protected]>

binjip978 marked this pull request as ready for review February 27, 2022 09:52

binjip978 requested review from a team and bogdandrutu February 27, 2022 09:52

avadhut123pisal reviewed Mar 2, 2022

View reviewed changes

add validation for autoscale

93d8131

Signed-off-by: Sergei Semenchuk <[email protected]>

binjip978 force-pushed the hpa-support branch from 92c3024 to 93d8131 Compare March 3, 2022 14:12

pavolloffay reviewed Mar 4, 2022

View reviewed changes

binjip978 added 3 commits March 5, 2022 06:29

remove minReplicas

a477345

Signed-off-by: Sergei Semenchuk <[email protected]>

remove autoscale parameter, trigger HPA if maxReplicas is not nil

6fed4dc

Signed-off-by: Sergei Semenchuk <[email protected]>

use collector replicas for minReplicas in HPA if HPA status is currently

03435ba

lower Signed-off-by: Sergei Semenchuk <[email protected]>

pavolloffay reviewed Mar 7, 2022

View reviewed changes

remove nil check

2505026

Signed-off-by: Sergei Semenchuk <[email protected]>

pavolloffay approved these changes Mar 8, 2022

View reviewed changes

pavolloffay merged commit cd4fa9b into open-telemetry:main Mar 8, 2022

secat mentioned this pull request Mar 16, 2022

Add missing scale subresource status in order to use an HPA resource over an OpenTelemetryCollector CR #775

Closed

pavolloffay mentioned this pull request Mar 31, 2022

Change our HorizontalPodAutoScaler (HPA) integration to use otelcol /scale subresource #801

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add autoscale option to enable support for Horizontal Pod Autoscaling #746

add autoscale option to enable support for Horizontal Pod Autoscaling #746

binjip978 commented Feb 26, 2022 •

edited by pavolloffay

Loading

avadhut123pisal Mar 2, 2022 •

edited

Loading

binjip978 Mar 3, 2022

avadhut123pisal Mar 2, 2022 •

edited

Loading

binjip978 Mar 3, 2022

pavolloffay Mar 4, 2022

pavolloffay Mar 4, 2022

binjip978 Mar 5, 2022

pavolloffay Mar 4, 2022

pavolloffay Mar 4, 2022

binjip978 Mar 5, 2022

pavolloffay Mar 4, 2022

pavolloffay Mar 4, 2022

binjip978 Mar 5, 2022

pavolloffay left a comment

pavolloffay Mar 7, 2022

binjip978 Mar 8, 2022

pavolloffay Mar 7, 2022

binjip978 Mar 8, 2022

pavolloffay Mar 8, 2022

add autoscale option to enable support for Horizontal Pod Autoscaling #746

add autoscale option to enable support for Horizontal Pod Autoscaling #746

Conversation

binjip978 commented Feb 26, 2022 • edited by pavolloffay Loading

avadhut123pisal Mar 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avadhut123pisal Mar 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

binjip978 commented Feb 26, 2022 •

edited by pavolloffay

Loading

avadhut123pisal Mar 2, 2022 •

edited

Loading

avadhut123pisal Mar 2, 2022 •

edited

Loading