-
Notifications
You must be signed in to change notification settings - Fork 435
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
Expose Horizontal Pod Autoscaler Behavior and add hpa scaledown test #1077
Expose Horizontal Pod Autoscaler Behavior and add hpa scaledown test #1077
Conversation
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>
// for the OpenTelemetryCollector workload. | ||
// | ||
// +optional | ||
Autoscaler *AutoscalerSpec `json:"autoscaler,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: did we consider embedding the HPA spec in here? Or at least embed the autoscaling behavior spec here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just adding enough code to be able to get an e2e test to work within the time allocated, which means we need to scale down much quicker that the default 300 seconds.
@pavolloffay what do you think? Do I need to add the other values of PA scaling rules here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my use case, I know that I would like to be able to specify policies and not just StabilizationWindowSeconds
. Embedding only StabilizationWindowSeconds
is going to make it so we need to add in each feature on request, making a code change for each one.
@@ -129,6 +129,15 @@ func (r *OpenTelemetryCollector) validateCRDSpec() error { | |||
return fmt.Errorf("the OpenTelemetry Spec autoscale configuration is incorrect, minReplicas should be one or more") | |||
} | |||
|
|||
if r.Spec.Autoscaler != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also check that min or max replicas is set in order to use this feature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's inside an if statement that's checking whether maxReplicas is set.
Signed-off-by: Kevin Earls <kearls@redhat.com>
@@ -13,4 +13,7 @@ metadata: | |||
spec: | |||
minReplicas: 1 | |||
maxReplicas: 2 | |||
# This is not neccesarily exact. We really just want to wait until this is no longer <unknown> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a really good "hack"
Signed-off-by: Kevin Earls <kearls@redhat.com>
Signed-off-by: Kevin Earls <kearls@redhat.com>
Does this resolve #942 ? |
@pavolloffay Yes |
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>
…pen-telemetry#1077) * Add scaledown test for autoscaling Signed-off-by: Kevin Earls <kearls@redhat.com> * Fix nits Signed-off-by: Kevin Earls <kearls@redhat.com> * Appease the linter Signed-off-by: Kevin Earls <kearls@redhat.com> * Don't use a default, only set scaleUp/scaleDown if they are in the CR Signed-off-by: Kevin Earls <kearls@redhat.com> * Removed commented out code Signed-off-by: Kevin Earls <kearls@redhat.com> * Change defaults for scaleUp/scaleDown Signed-off-by: Kevin Earls <kearls@redhat.com> * Update autoscaling scaleup/scaledown Signed-off-by: Kevin Earls <kearls@redhat.com> * Run generate Signed-off-by: Kevin Earls <kearls@redhat.com> * Ran make api-docs Signed-off-by: Kevin Earls <kearls@redhat.com> * Update HPA implementation to embed HorizontalPodAutoscalerBehavior Signed-off-by: Kevin Earls <kearls@redhat.com> * Only set behavior if it exists in the collector CR Signed-off-by: Kevin Earls <kearls@redhat.com> * Aded some unit tests Signed-off-by: Kevin Earls <kearls@redhat.com> * Add kuttl assertion that hpa scaled down Signed-off-by: Kevin Earls <kearls@redhat.com> * added whitespace to rerun tests Signed-off-by: Kevin Earls <kearls@redhat.com> Signed-off-by: Kevin Earls <kearls@redhat.com>
No description provided.