Change horizontal pod autoscaler to use otelcol scale subresource #941

kevinearls · 2022-06-23T13:11:42Z

Signed-off-by: Kevin Earls kearls@redhat.com

This fixes #801 so that the HPA is created on an OpenTelemetryCollector rather than a Deployment.

I believe it also fixes #922
I believe it also fixes #906 but will confirm

Signed-off-by: Kevin Earls <kearls@redhat.com>

pavolloffay · 2022-06-24T15:32:40Z

apis/v1alpha1/opentelemetrycollector_types.go

@@ -37,6 +37,10 @@ type OpenTelemetryCollectorSpec struct {
 	// +optional
 	Replicas *int32 `json:"replicas,omitempty"`

+	// MinReplicas sets a lower bound to the autoscaling feature
+	// +optional
+	MinReplicas int32 `json:"minReplicas,omitempty"`


What is the relationship between MinReplicas and Replicas? Which one should be used when? This is something that should be documented.

With that we probably need to add some checks to the webhook

opentelemetry-operator/apis/v1alpha1/opentelemetrycollector_webhook.go

Lines 120 to 122 in b77c89d

if r.Spec.Replicas != nil && *r.Spec.Replicas > *r.Spec.MaxReplicas {

return fmt.Errorf("the OpenTelemetry Spec autoscale configuration is incorrect, replicas must not be greater than maxReplicas")

}

to test the minreplicas or any relationship between replicas, minreplias and maxreplicas.

pavolloffay · 2022-06-24T15:33:35Z

pkg/collector/horizontalpodautoscaler.go

@@ -35,18 +35,18 @@ func HorizontalPodAutoscaler(cfg config.Config, logger logr.Logger, otelcol v1al

 	return autoscalingv1.HorizontalPodAutoscaler{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:        naming.Collector(otelcol),
+			Name:        otelcol.Name,


What is the rationale for the name change? Could this produce an invalid name?

We are now scaling on the OpenTelemetryCollector rather than the deployment. If the otelcol.Name is "simple" we append "-collector" to the deployment name.

If you think it's a good idea I could add a function to naming which just does

return DNSName(Truncate("%s", 63, otelcol.Name))

pavolloffay · 2022-06-24T15:41:24Z

I would like to see e2e test for this functionality. It can be actually two tests - scaling up and down with the HPA.

Signed-off-by: Kevin Earls <kearls@redhat.com>

yuriolisa

Please fix the lint.

…ntelemetry-operator into hpa-use-scale-subresource

Signed-off-by: Kevin Earls <kearls@redhat.com>

kevinearls · 2022-07-04T12:50:50Z

@pavolloffay Please review. I think I've covered all of the things we discussed.

@yuriolisa lint now passes

pavolloffay · 2022-07-04T14:46:47Z

apis/v1alpha1/opentelemetrycollector_types.go

 	// +optional
-	MaxReplicas *int32 `json:"maxReplicas,omitempty"`


I am not sure if we can change the CRD. In this case, a field is removed.

controllers/opentelemetrycollector_controller.go

Signed-off-by: Kevin Earls <kearls@redhat.com>

kevinearls · 2022-07-06T10:10:52Z

@pavolloffay Can you please review this, re-running the checks first if needed? (The 1.24 e2e tests are failing in a place I didn't touch.)

I have reverted to using autoscaling v1 and removed the AutoScaleSpec changes (I can do both later) so I think that covers everything we've discussed.

pavolloffay · 2022-07-07T11:03:48Z

hack/install-metrics-server.sh

@@ -0,0 +1,6 @@
+#!/bin/bash
+
+# Install metrics-server for autoscale tests


Is this needed for mimikube as well or only for kind? It should be documented.

No, with minikube you can just add --addons "metrics-server" to the start command.

Where should I document this? In this script? In the Makefile?

Probably closer to the source - script

Signed-off-by: Kevin Earls <kearls@redhat.com>

pavolloffay

@kevinearls could you please confirm that the change of HPA implementation will not disrupt existing instances provisioned by the previous version of the operator?

Signed-off-by: Kevin Earls <kearls@redhat.com>

yuriolisa

@kevinearls, please ensure that change will not disrupt the existing instances like @pavolloffay mentioned above. LGTM.

Signed-off-by: Kevin Earls <kearls@redhat.com>

kevinearls · 2022-07-15T10:39:21Z

@yuriolisa @pavolloffay I have added code that takes care of the upgrade. Please review and merge if it looks ok.

yuriolisa · 2022-07-15T11:11:25Z

pkg/collector/reconcile/horizontalpodautoscaler_test.go

@@ -52,6 +52,7 @@ func TestExpectedHPA(t *testing.T) {
 		maxReplicas := int32(3)
 		updateParms := paramsWithHPA()
 		updateParms.Instance.Spec.Replicas = &minReplicas
+		updateParms.Instance.Spec.Replicas = &minReplicas


Please fix this duplicated code.

yuriolisa · 2022-07-15T11:20:51Z

apis/v1alpha1/opentelemetrycollector_webhook.go

+		}
+
+		if r.Spec.MinReplicas != nil && *r.Spec.MinReplicas < int32(1) {
+			return fmt.Errorf("the OpenTelemetry Spec autoscale configuration is incorrect, minReplicas should be more than one")


I consider MinReplicas should be at least one, correct?

yuriolisa · 2022-07-15T11:26:13Z

bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml

@@ -223,6 +223,10 @@ spec:
                  If MaxReplicas is set autoscaling is enabled.
                format: int32
                type: integer
+              minReplicas:
+                description: MinReplicas sets a lower bound to the autoscaling feature.


Could you please add here the lowest value?

yuriolisa · 2022-07-15T11:26:23Z

config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml

@@ -221,6 +221,10 @@ spec:
                  If MaxReplicas is set autoscaling is enabled.
                format: int32
                type: integer
+              minReplicas:
+                description: MinReplicas sets a lower bound to the autoscaling feature.


Could you please add here the lowest value?

yuriolisa · 2022-07-15T11:26:36Z

docs/api.md

+        <td><b>minReplicas</b></td>
+        <td>integer</td>
+        <td>
+          MinReplicas sets a lower bound to the autoscaling feature.<br/>


yuriolisa · 2022-07-15T11:31:05Z

tests/e2e/autoscale/00-install.yaml

+metadata:
+  name: simplest
+spec:
+  replicas: 1


Should this field be automatically filled, once the minReplicas is the default lower bound of that instance?

Signed-off-by: Kevin Earls <kearls@redhat.com>

yuriolisa

LGTM.

kevinearls · 2022-07-15T14:45:56Z

Closing this in favor of #984 as I managed to get myself into rebase hell.

Change horizontal pod autoscaler to use otelcol scale subresource

c5f0e1d

Signed-off-by: Kevin Earls <kearls@redhat.com>

kevinearls requested a review from a team June 23, 2022 13:11

Fix name

04abf43

Signed-off-by: Kevin Earls <kearls@redhat.com>

pavolloffay reviewed Jun 24, 2022

View reviewed changes

kevinearls added 2 commits June 27, 2022 09:00

Change horizontal pod autoscaler to use otelcol scale subresource

e6e73da

Signed-off-by: Kevin Earls <kearls@redhat.com>

Fix name

6ba5e93

Signed-off-by: Kevin Earls <kearls@redhat.com>

yuriolisa requested changes Jun 28, 2022

View reviewed changes

kevinearls added 12 commits June 29, 2022 09:55

Merge branch 'hpa-use-scale-subresource' of github.com:kevinearls/ope…

23d00f0

…ntelemetry-operator into hpa-use-scale-subresource

Moved autoscale values into their own spec, added scale up test

949a29f

Signed-off-by: Kevin Earls <kearls@redhat.com>

Added a period to a comment to make the linter stop complaining

f088fea

Signed-off-by: Kevin Earls <kearls@redhat.com>

Reran make generate after linting

34f44f7

Signed-off-by: Kevin Earls <kearls@redhat.com>

Fix autoscale test

4974844

Signed-off-by: Kevin Earls <kearls@redhat.com>

Updated to autoscaling/v2beta2 as v1 is not available in k8s 1.24

88575f6

Signed-off-by: Kevin Earls <kearls@redhat.com>

Install metrics-server on kind clusters for autoscale tests

1d45856

Signed-off-by: Kevin Earls <kearls@redhat.com>

Merge branch 'main' into hpa-use-scale-subresource

2149091

Fix more lint complaints

dc55641

Signed-off-by: Kevin Earls <kearls@redhat.com>

Only scale up to 5 replicas in test

36d2dda

Signed-off-by: Kevin Earls <kearls@redhat.com>

Fix test

41b581a

Signed-off-by: Kevin Earls <kearls@redhat.com>

Update test so it will work correctly in CI

c22f6b4

Signed-off-by: Kevin Earls <kearls@redhat.com>

pavolloffay reviewed Jul 4, 2022

View reviewed changes

kevinearls added 5 commits July 4, 2022 18:00

Revert to autoscaling v1

b4a72d1

Signed-off-by: Kevin Earls <kearls@redhat.com>

Reduce maxReplicas count to get kuttl tests to work in CI

33d9c48

Signed-off-by: Kevin Earls <kearls@redhat.com>

Back out addition of AutoScaleSpec to CRD

2f5ade5

Signed-off-by: Kevin Earls <kearls@redhat.com>

Fixed api-docs

0bc348b

Signed-off-by: Kevin Earls <kearls@redhat.com>

Restore accidentally delete test, clean up whitespace

3ce81aa

Signed-off-by: Kevin Earls <kearls@redhat.com>

pavolloffay reviewed Jul 7, 2022

View reviewed changes

Document that this script is not needed for minikube

aeb4a97

Signed-off-by: Kevin Earls <kearls@redhat.com>

pavolloffay approved these changes Jul 7, 2022

View reviewed changes

Fix scaledown

96917a2

Signed-off-by: Kevin Earls <kearls@redhat.com>

yuriolisa approved these changes Jul 8, 2022

View reviewed changes

kevinearls added 5 commits July 11, 2022 10:42

Update webhook to validate minReplicas when needed

2ef9467

Signed-off-by: Kevin Earls <kearls@redhat.com>

Fix tests

ba0423b

Signed-off-by: Kevin Earls <kearls@redhat.com>

Initial upgrade code for instances with an HPA

ef318ef

Signed-off-by: Kevin Earls <kearls@redhat.com>

Make sure old Deploment based autoscalers get updated

27fb26f

Signed-off-by: Kevin Earls <kearls@redhat.com>

Fix lint error

c733b6e

Signed-off-by: Kevin Earls <kearls@redhat.com>

yuriolisa requested changes Jul 15, 2022

View reviewed changes

kevinearls added 2 commits July 15, 2022 14:15

Respond to comments

81aa971

Signed-off-by: Kevin Earls <kearls@redhat.com>

Fixed api-docs

f3d190b

Signed-off-by: Kevin Earls <kearls@redhat.com>

yuriolisa approved these changes Jul 15, 2022

View reviewed changes

kevinearls closed this Jul 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change horizontal pod autoscaler to use otelcol scale subresource #941

Change horizontal pod autoscaler to use otelcol scale subresource #941

kevinearls commented Jun 23, 2022 •

edited by pavolloffay

Loading

pavolloffay Jun 24, 2022

pavolloffay Jun 24, 2022

pavolloffay Jun 24, 2022

kevinearls Jun 27, 2022

pavolloffay commented Jun 24, 2022

yuriolisa left a comment

kevinearls commented Jul 4, 2022

pavolloffay Jul 4, 2022 •

edited

Loading

kevinearls commented Jul 6, 2022

pavolloffay Jul 7, 2022

kevinearls Jul 7, 2022

pavolloffay Jul 7, 2022

pavolloffay left a comment

yuriolisa left a comment

kevinearls commented Jul 15, 2022

yuriolisa Jul 15, 2022

yuriolisa Jul 15, 2022

yuriolisa Jul 15, 2022

yuriolisa Jul 15, 2022

yuriolisa Jul 15, 2022

yuriolisa Jul 15, 2022

yuriolisa left a comment

kevinearls commented Jul 15, 2022 •

edited

Loading

	if r.Spec.Replicas != nil && r.Spec.Replicas > r.Spec.MaxReplicas {
	return fmt.Errorf("the OpenTelemetry Spec autoscale configuration is incorrect, replicas must not be greater than maxReplicas")
	}

		// +optional
		MaxReplicas *int32 `json:"maxReplicas,omitempty"`

		@@ -0,0 +1,6 @@
		#!/bin/bash

		# Install metrics-server for autoscale tests

Change horizontal pod autoscaler to use otelcol scale subresource #941

Change horizontal pod autoscaler to use otelcol scale subresource #941

Conversation

kevinearls commented Jun 23, 2022 • edited by pavolloffay 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

pavolloffay commented Jun 24, 2022

yuriolisa left a comment

Choose a reason for hiding this comment

kevinearls commented Jul 4, 2022

pavolloffay Jul 4, 2022 • edited Loading

Choose a reason for hiding this comment

kevinearls commented Jul 6, 2022

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

yuriolisa left a comment

Choose a reason for hiding this comment

kevinearls commented Jul 15, 2022

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

yuriolisa left a comment

Choose a reason for hiding this comment

kevinearls commented Jul 15, 2022 • edited Loading

kevinearls commented Jun 23, 2022 •

edited by pavolloffay

Loading

pavolloffay Jul 4, 2022 •

edited

Loading

kevinearls commented Jul 15, 2022 •

edited

Loading