Make HPA behavior configurable #431
Conversation
|
#433 will fix the ci |
|
@sanposhiho CI should be passing now, PTAL when you have a moment 🙏 |
There was a problem hiding this comment.
can you add a new test case or edit existing test case in the controller level test to ensure this behaviour?
https://github.com/mercari/tortoise/blob/main/internal/controller/tortoise_controller_test.go
|
@AVSBharadwaj @jrbangit @AdityaK011 This one as well - Do you want to take a look, as a reviewing training? |
|
@sanposhiho Added 🙏 |
|
My free time is occupied for Kubernetes code freeze and Kubecon preparation. I wouldn't be able to take time until all of them are completed - mid of April. |
|
@sanposhiho @randytqwjp @jrbangit I have merged commit from my PR to this Pr itself and also added support for configuring HPABehavior from the config.yaml file. |
|
will wait for another review/approve from code owners |
pkg/config/config.go
Outdated
| // Validate HPA behavior if specified | ||
| if config.DefaultHPABehavior != nil { | ||
| // Validate scale up policies | ||
| if config.DefaultHPABehavior.ScaleUp != nil && len(config.DefaultHPABehavior.ScaleUp.Policies) > 0 { |
There was a problem hiding this comment.
nit: Looks better to create a separate function validateDefaultHPA, and then call it from here.
Where are these validations coming from, by the way? Copied from the upstream Kubernetes?
There was a problem hiding this comment.
Yes i took them from the upstream autoscaling docs: https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.29/#hpascalingpolicy-v2-autoscaling
| @@ -72,6 +74,7 @@ func New( | |||
| tortoiseHPATargetUtilizationMaxIncrease: tortoiseHPATargetUtilizationMaxIncrease, | |||
| recorder: recorder, | |||
| tortoiseHPATargetUtilizationUpdateInterval: tortoiseHPATargetUtilizationUpdateInterval, | |||
| defaultHPABehavior: defaultHPABehavior, | |||
There was a problem hiding this comment.
You can put defaultHPABehavior (defined at L224) if defaultHPABehavior (at L55) is nil. Then, we can remove getDefaultHPABehavior function because Service.defaultHPABehavior would be always non-nil
pkg/config/config.go
Outdated
| @@ -361,5 +385,50 @@ func validate(config *Config) error { | |||
| } | |||
| } | |||
|
|
|||
| // Validate HPA behavior if specified | |||
| if config.DefaultHPABehavior != nil { | |||
There was a problem hiding this comment.
Also, you need to add unit tests in https://github.com/mercari/tortoise/blob/main/pkg/config/config_test.go#L155
There was a problem hiding this comment.
What this PR does / why we need it:
This allows the end-user to specified desired v2 HPA behavior, instead of using the default HPA behavior specified by tortoise. This is needed because currently tortoise's scale up behavior is very aggressive, which results in poor cost performance on workloads which are bursty and don't need that much aggressive scaling to meet their latency/throughput targets.
Which issue(s) this PR fixes:
Partially fixes #341 (this PR makes it possible for the end-user to prevent too much scaling up but does not put a safeguard in Tortoise by default like that issue is written to suggest)
Special notes for your reviewer:
nilcheck dance is duplicated, but I found nowhere else in the code where defaults can be set in one spot (Defaultin tortoise_webhook.go seemed close, but it seems to not be called in all code paths, as the tests failed).