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

feat: validate fallback configuration #5629

Merged
merged 8 commits into from
Apr 23, 2024

Conversation

guopeng0
Copy link
Contributor

@guopeng0 guopeng0 commented Mar 27, 2024

#5515

fallback feature only supports AverageValue metrics type and it doesn't support CPU and Memory.
To prevent misconfigurations and confusions related with these limitations, we should include a validation for them during the admission process

Checklist

Fixes #5515

Relates to #

@guopeng0 guopeng0 requested a review from a team as a code owner March 27, 2024 11:15
Signed-off-by: Guo Peng <370090914@qq.com>
Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

Looking good! ❤️
Could you add a test for this case? We need to update the changelog as well :)

@guopeng0
Copy link
Contributor Author

guopeng0 commented Apr 1, 2024

Sure, I agree. We’ll add a test for this case.

guopeng0 and others added 2 commits April 3, 2024 14:19
Signed-off-by: Guo Peng <370090914@qq.com>
Signed-off-by: guopeng <100843245+guopeng0@users.noreply.github.com>
@guopeng0 guopeng0 requested a review from JorTurFer April 3, 2024 07:07
@JorTurFer
Copy link
Member

Looks nice! Could you update docs as well to include this new rule? https://github.com/kedacore/keda-docs/blob/main/content/docs/2.14/concepts/admission-webhooks.md

@guopeng0
Copy link
Contributor Author

guopeng0 commented Apr 9, 2024

Sounds good, I will update the documentation accordingly.

Signed-off-by: guopeng <100843245+guopeng0@users.noreply.github.com>
@guopeng0
Copy link
Contributor Author

guopeng0 commented Apr 9, 2024

Hi @JorTurFer
I've addressed the issue and updated the documentation to include the new rule.

@zroubalik
Copy link
Member

zroubalik commented Apr 10, 2024

/run-e2e fallback
Update: You can check the progress here

Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

@guopeng0 we should also check that both Replicas and FailureThreshold are set if Fallback is specified.

@guopeng0
Copy link
Contributor Author

guopeng0 commented Apr 12, 2024

Hi @zroubalik
I appreciate your comment. The code considers non-negative values for Replicas and FailureThreshold as valid, and if they are not explicitly set by the user, default processing will apply with values set to 0. Thus, I believe no additional checks are required when fallback is specified.

func validateFallback(scaledObject *kedav1alpha1.ScaledObject) bool {
	return scaledObject.Spec.Fallback.FailureThreshold >= 0 &&
		scaledObject.Spec.Fallback.Replicas >= 0
}

The relevant code is located in the keda/pkg/fallback/fallback.go

Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

@guopeng0 could you please fix the conflict in the Changelog? then we are good to go, thanks!

CHANGELOG.md Outdated Show resolved Hide resolved
@tomkerkhove tomkerkhove mentioned this pull request Apr 16, 2024
35 tasks
tomkerkhove and others added 2 commits April 16, 2024 07:47
Signed-off-by: Tom Kerkhove <kerkhove.tom@gmail.com>
Signed-off-by: guopeng <100843245+guopeng0@users.noreply.github.com>
@guopeng0
Copy link
Contributor Author

guopeng0 commented Apr 18, 2024

@zroubalik
Sure, I’ve resolved the conflict in the Changelog.

@tomkerkhove
Copy link
Member

tomkerkhove commented Apr 18, 2024

/run-e2e fallback
Update: You can check the progress here

@tomkerkhove
Copy link
Member

@guopeng0 Can you fix the linting issue please?

@guopeng0
Copy link
Contributor Author

@tomkerkhove Sure, I’ll make the changes as soon as possible.

CHANGELOG.md Outdated Show resolved Hide resolved
@zroubalik zroubalik merged commit b13e2a4 into kedacore:main Apr 23, 2024
19 of 20 checks passed
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.

Validate fallback configuration during admission process
4 participants