-
Notifications
You must be signed in to change notification settings - Fork 729
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
Prevent primary hpa collision for keda scaled objects when migrating from an hpa #1677
base: main
Are you sure you want to change the base?
Conversation
b446861
to
c8859f4
Compare
…led object Signed-off-by: James Geisler <[email protected]>
c8859f4
to
5965983
Compare
Hey @stefanprodan and @aryan9600, looking for some feedback here on this PR and/or the issue it fixes #1646 |
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.
thanks for raising this PR! i did a quick first pass. will review it once more in the coming days.
} | ||
} | ||
|
||
func initializeTargetSoClone(targetSoClone *keda.ScaledObject) { |
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.
lets update the name to something that indicates that it sets the primary HPA's name
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.
Good idea, will do
MinReplicaCount: targetSoClone.Spec.MinReplicaCount, | ||
MaxReplicaCount: targetSoClone.Spec.MaxReplicaCount, | ||
// Allow the hpa name to be set in the target scaled object, so that it can be used in the primary scaled object | ||
Advanced: &keda.AdvancedConfig{ |
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 think this can still be Advanced: targetSoClone.Spec.Advanced
, since we've added Name
to the HorizontalPodAutoscalerConfig
definition.
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.
Ah yes you are correct, I will update. Thanks
Signed-off-by: James Geisler <[email protected]>
Thanks for the fix @jdgeisler |
This MR fixes #1646 where Flagger fails to create the primary hpa for a keda scaled object when you are migrating from a regular hpa to a scaled object in a canary.
Using the
scaledobject.keda.sh/transfer-hpa-ownership: "true"
annotation, a keda scaled object can take ownership of an already existing HPA. Currently, annotations are not copied over from the canary scaled object to the primary scaled object, so I am adding this functionality.Additionally, we also need to add the
Advanced.HorizontalPodAutoscalerConfig.Name
field to theScaledObjectSpec
so that we can specify the HPA name that the scaled object will take ownership of. When creating the primary scaled object from the canary, we use the same hpa name but append-primary
to the end of it.Testing
With the above changes, we can then successfully use the transfer hpa ownership annotation and specify the HPA that the scaled objects will take ownership of. This prevents KEDA from failing to create the new primary hpa since it already exists. Instead it just uses and modifies the already existing one.
Applying the following diff to test in a kubernetes cluster:
See that the primary scaled object successfully takes over ownership of the primary hpa and it no longer fails.