Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add TargetMemoryUtilization metric for AutoScaling #1223

Merged

Conversation

kevinearls
Copy link
Member

@kevinearls kevinearls commented Nov 4, 2022

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

This is in response to #1203 It does this in an opinionated way, just adding targetMemoryUtilization for now. Implementing other things that can be set as metrics could be done later.

Note that there is no new e2e test. I originally wrote a new test, and it would pass consistently on my laptop, but then would fail most of the time under CI. Given the short time I have remaining on this project I was not able to resolve this.

I did test it manually, and even built an operator which did not set targetCPUUtilization and ran some tests against that to make sure it would scale correctly on memory.

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

Signed-off-by: Kevin Earls <kearls@redhat.com>
Signed-off-by: Kevin Earls <kearls@redhat.com>
Signed-off-by: Kevin Earls <kearls@redhat.com>
Signed-off-by: Kevin Earls <kearls@redhat.com>
Signed-off-by: Kevin Earls <kearls@redhat.com>
Signed-off-by: Kevin Earls <kearls@redhat.com>
Signed-off-by: Kevin Earls <kearls@redhat.com>
Signed-off-by: Kevin Earls <kearls@redhat.com>
@kevinearls kevinearls marked this pull request as ready for review November 8, 2022 13:49
@kevinearls kevinearls requested a review from a team November 8, 2022 13:49
@kevinearls
Copy link
Member Author

@pavolloffay @frzifus Can I get a review on this?

apis/v1alpha1/opentelemetrycollector_types.go Outdated Show resolved Hide resolved
@@ -46,6 +46,22 @@ func HorizontalPodAutoscaler(cfg config.Config, logger logr.Logger, otelcol v1al
}

if autoscalingVersion == autodetect.AutoscalingVersionV2Beta2 {
metrics := []autoscalingv2beta2.MetricSpec{}

if otelcol.Spec.Autoscaler != nil && otelcol.Spec.Autoscaler.TargetMemoryUtilization != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the otelcol.Spec.Autoscaler != nil needed? Line 71 defines otelcol.Spec.Autoscaler.TargetCPUUtilization and it is not in the if block for the autoscaler, probably it should be moved there.

Also should be the lines 65-74 moved to if block if TargetCPUUtilization is not nil?

The same should be handled on line 114

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed thise if clauses, but at this point TargetCPUUtilization will always be set, as the webhook will set it to default if it is not in the CR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but at this point TargetCPUUtilization will always be set, as the webhook will set it to default if it is not in the CR.

Is this correct behavior?

Shall we allow users to use independently CPU, memory or both?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pavolloffay Good qestion. :-) If we had done this correctly from the beginning I'd say no. But at some point we decided if MaxReplicas was set then we would automatically set TargetCPUUtilization to a default value if it was not in the CR.

I can implement this whatever way you think is correct.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case the operator supports only CPU metric then it makes sense to default it when HPA is used. But if there are multiple metrics users should have choice which one to use and the operator should default only when no user config is provided.

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

@pavolloffay pavolloffay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pavolloffay pavolloffay merged commit 61accfd into open-telemetry:main Dec 1, 2022
@kevinearls kevinearls deleted the add-memory-utilization-metric branch December 1, 2022 09:52
ihalaij1 pushed a commit to ihalaij1/opentelemetry-operator that referenced this pull request Dec 8, 2022
* Add TargetMemoryUtilization metric for AutoScaling

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

* Add changes to v2beta2 as there is no way to un e2e tests just for one version

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

* See if we just have a race condition

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

* Reset kuttl timeout

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

* Add some debugging code to help analyze failures on github

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

* Try to appease the linter

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

* Restore autoscale tests

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

* Cleanup

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

* More cleanup

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

* Respond to comments

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

* Cleanup whitespace so linter will rerun

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

* Don't set TargetCPUUtilization to default if another metric is set

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

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

kapcip commented Dec 18, 2022

Trying to use this YAML for operator but targetMemoryUtilization is yet missing. Am I missing something?

ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
* Add TargetMemoryUtilization metric for AutoScaling

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

* Add changes to v2beta2 as there is no way to un e2e tests just for one version

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

* See if we just have a race condition

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

* Reset kuttl timeout

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

* Add some debugging code to help analyze failures on github

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

* Try to appease the linter

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

* Restore autoscale tests

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

* Cleanup

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

* More cleanup

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

* Respond to comments

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

* Cleanup whitespace so linter will rerun

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

* Don't set TargetCPUUtilization to default if another metric is set

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

Signed-off-by: Kevin Earls <kearls@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants