[HPA] Move maxReplicas and minReplicas to AutoscalerSpec #1302
Conversation
moh-osman3
changed the title
maxReplicas and minReplicas to .spec.Autoscaler.spec.Autoscaler
moh-osman3
changed the title
.spec.Autoscaler
moh-osman3
force-pushed
the
mohosman/issue-1115/HPA-move-max-min-replicas
branch
3 times, most recently
from
5f6676e to
f76931b
Compare
| @@ -72,6 +72,32 @@ func HorizontalPodAutoscaler(cfg config.Config, logger logr.Logger, otelcol v1al | |||
| } | |||
| metrics = append(metrics, targetCPUUtilization) | |||
|
|
|||
| // check if fields are provided by .Spec.Autoscaler | |||
| // otherwise check deprecated fields | |||
| maxReplicas := new(int32) | |||
There was a problem hiding this comment.
Could we use defaulting webhook to set the new fields to avoid this logic in this file?
Also how will the upgrade work?
There was a problem hiding this comment.
Thanks for the review @pavolloffay! I updated with your suggestion to use the defaulting webhook.
I am generally unfamiliar with the upgrade process, but took a look at v0_56_0.go which upgraded HPA to change the scaling target reference. This upgrade makes sense because the underlying HPA should be altered from scaling on Deployment to scaling on OpenTelemetryCollector.
For this PR, the deprecated fields are still supported, so I think underlying HPA will remain unchanged. I don't think it's necessary to upgrade the HPA in this case (unless I'm missing something)
| if r.Spec.Autoscaler == nil { | ||
| r.Spec.Autoscaler = &AutoscalerSpec{} | ||
| } | ||
|
|
||
| if r.Spec.Autoscaler.MaxReplicas == nil { |
There was a problem hiding this comment.
please add a test case for this in the webhook unit test
There was a problem hiding this comment.
I think this test case already covers the case when r.Spec.Autoscaler.MaxReplicas == nil. I have now added test case for when r.Spec.Autoscaler.MaxReplicas != nil
| @@ -6,9 +6,9 @@ kind: OpenTelemetryCollector | |||
| metadata: | |||
| name: simplest | |||
| spec: | |||
| minReplicas: 1 | |||
There was a problem hiding this comment.
I would prefer to keep this for now to verify the old config works. Maybe add a comment to update it.
There was a problem hiding this comment.
Reverted this change and added a comment!
| if r.Spec.MinReplicas != nil { | ||
| r.Spec.Autoscaler.MinReplicas = r.Spec.MinReplicas | ||
| } else { | ||
| r.Spec.Autoscaler.MinReplicas = r.Spec.Replicas |
There was a problem hiding this comment.
Are we sure that this is correct?
There was a problem hiding this comment.
Hmm I believe the logic is correct, but maybe there's something I'm missing?
MinReplicas is meant to be an optional argument, but it should eventually always have a value if MaxReplicas is set. So here it defaults to .Spec.Replicas which is validated by this check in the validator. I'm setting this default earlier now instead of waiting for the creation of the autoscaler to set the default here.
Wondering your thoughts and why this might not be the desired logic?
There was a problem hiding this comment.
Wondering your thoughts and why this might not be the desired logic?
Originally I thought that the idea here is to totally decouple these two. But it's reasonable to default to the .spec.replicas
moh-osman3
force-pushed
the
mohosman/issue-1115/HPA-move-max-min-replicas
branch
from
a6ad53b to
d9e9ae8
Compare
…try#1302) * move maxReplics and minReplicas to Autoscaler field * fix descriptions * run make generate * make bundle * make api-docs * fix unit test * fix nil pointer dereference * use .Spec.Replicas if MinReplicas is not set * initialize pointers * set .autoscaler.[max|min]Replicas in default webhook * updating reconcile for hpa * addressing review feedback * add comment * gofmt * chloggen
Resolves #1115