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

Update KEDA ScaledObject API to include MetricType for Triggers #1241

Merged
merged 2 commits into from
Jul 29, 2022

Conversation

vidhartbhatia
Copy link
Contributor

@vidhartbhatia vidhartbhatia commented Jul 27, 2022

I apologize if I missed anything, but I wanted to initiate this PR to try and fix the KEDA scaled object issue where the trigger metric type isn't copied over correctly, I suspected it was just due to the API being out of date

I will update as needed, any advice is welcome!

@vidhartbhatia
Copy link
Contributor Author

Sorry forgot to link it to the issue, this is intended to fix #1240

@vidhartbhatia
Copy link
Contributor Author

For reference this is the latest API i copied over: https://github.com/kedacore/keda/blob/main/apis/keda/v1alpha1/scaledobject_types.go

Copy link
Member

@aryan9600 aryan9600 left a comment

Choose a reason for hiding this comment

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

Hi @vidhartbhatia, thanks for catching the issue and opening this PR!

pkg/apis/keda/v1alpha1/scaledobject.go Outdated Show resolved Hide resolved
@vidhartbhatia vidhartbhatia marked this pull request as ready for review July 27, 2022 06:40
@vidhartbhatia
Copy link
Contributor Author

vidhartbhatia commented Jul 27, 2022

Hey, @aryan9600 I noticed another issue, it's not strictly related but I figure if you think of a small fix we can include it in this PR.
Basically, when we switch from using scaled object to hpa and vice versa in the canary spec, the old primary resource gets left behind, It would be great to have flagger delete the old -primary hpa or scaledobject when a deployment happens.

Here you can see the state after each deploy, this would casue issues as keda and hpa dont behave well together:
image

image

Update: We just worked around this by running a clean up script so no rush on it

@aryan9600
Copy link
Member

@vidhartbhatia Yes, that'd be ideal (ref: #259), but the fix won't be small enough for it to be included in this PR. If you'd like to give it a shot, please feel free to open another PR for this :D

@vidhartbhatia
Copy link
Contributor Author

vidhartbhatia commented Jul 28, 2022

@vidhartbhatia Yes, that'd be ideal (ref: #259), but the fix won't be small enough for it to be included in this PR. If you'd like to give it a shot, please feel free to open another PR for this :D

@aryan9600 Okay that makes sense! for now then we really need this current bug fix in asap so we can take KEDA to production so Any help getting this through and merged and then a minor release would be appreciated!
@stefanprodan I noticed you were also requested as a review, just wanted to bump :)

Signed-off-by: Vidhart Bhatia <vidhartbhatia@hotmail.com>
Co-authored-by: Sanskar Jaiswal <sanskar.jaiswal@weave.worksl>
@@ -127,7 +127,7 @@ type ScaleTriggers struct {
// +optional
AuthenticationRef *ScaledObjectAuthRef `json:"authenticationRef,omitempty"`
// +optional
FallbackReplicas *int32 `json:"fallback,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking change in Keda API, usually you never remove fields without bumping the API version, instead you mark them as deprecated. Due to this, we need to update the Keda doc with the exact version which Flagger expects.

Signed-off-by: Sanskar Jaiswal <sanskar.jaiswal@weave.works>
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @vidhartbhatia and @aryan9600

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