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.

Sign up for GitHub

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

Jump to bottom

fix: don't validate workload with only resource limits set #4803

Merged
merged 7 commits into from
Jan 3, 2024
Merged

fix: don't validate workload with only resource limits set #4803

merged 7 commits into from
Jan 3, 2024

Conversation

nettoclaudio
Copy link
Contributor

@nettoclaudio nettoclaudio commented Jul 16, 2023 •
edited

Check if container resource limits were provided before rejecting the ScaledObject creation.

Checklist

Fixes #4802

@github-actions
Copy link

Thank you for your contribution! 🙏 We will review your PR as soon as possible.

🏖️ Over the summer, the response time will be longer than usual due to maintainers taking time off so please bear with us.

While you are waiting, make sure to:

Learn more about:

@nettoclaudio nettoclaudio marked this pull request as ready for review July 17, 2023 20:21
@nettoclaudio nettoclaudio requested a review from a team as a code owner July 17, 2023 20:21
JorTurFer
JorTurFer reviewed Jul 26, 2023
View reviewed changes
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.

Nice catch!
I have left some comments inline.
We have to update also e2e test for covering this new scenario. ScaledObject validations are tested here: https://github.com/kedacore/keda/blob/main/tests/internals/scaled_object_validation/scaled_object_validation_test.go

apis/keda/v1alpha1/scaledobject_webhook.go Outdated Show resolved Hide resolved
apis/keda/v1alpha1/scaledobject_webhook.go Outdated Show resolved Hide resolved
apis/keda/v1alpha1/scaledobject_webhook_test.go Outdated Show resolved Hide resolved
apis/keda/v1alpha1/scaledobject_webhook_test.go Outdated Show resolved Hide resolved
@JorTurFer
Copy link
Member

JorTurFer commented Jul 26, 2023 •
edited by github-actions bot

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

@nettoclaudio nettoclaudio force-pushed the skip-validation-of-workloads-with-only-limits-set branch 2 times, most recently from 2bbe2d7 to 5ac25b7 Compare August 4, 2023 13:58
@JorTurFer
Copy link
Member

JorTurFer commented Aug 15, 2023 •
edited by github-actions bot

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

JorTurFer
JorTurFer reviewed Aug 15, 2023
View reviewed changes
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 nice!
Could you add a test case on e2e tests? We already have a file for admission webhooks, so it's just add a test case there:
main/tests/internals/scaled_object_validation/scaled_object_validation_test.go

zroubalik
zroubalik reviewed Sep 5, 2023
View reviewed changes
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.

Looking good!

@nettoclaudio do you have an update on this please?

@nettoclaudio nettoclaudio force-pushed the skip-validation-of-workloads-with-only-limits-set branch from 5ac25b7 to bba7a77 Compare November 29, 2023 20:56
@tomkerkhove
Copy link
Member

tomkerkhove commented Dec 1, 2023 •
edited by github-actions bot

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

@zroubalik
Copy link
Member

zroubalik commented Dec 21, 2023 •
edited by github-actions bot

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

zroubalik
zroubalik reviewed Dec 21, 2023
View reviewed changes
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.

@nettoclaudio could you please rebase this PR? then we can go ahead and proceed

@nettoclaudio
fix: don't validate workload with only resource limits set
bf214d8 
Signed-off-by: Claudio Netto <nettinhorama@gmail.com>
@nettoclaudio
docs: add resolved issue in changelog
11a7abf 
Signed-off-by: Claudio Netto <nettinhorama@gmail.com>
@nettoclaudio
fix: preventing possible panic due to nil pointer dereferencing
4a43163 
Signed-off-by: Claudio Netto <nettinhorama@gmail.com>
@nettoclaudio
test: set CPU/memory scaler specs properly
13226a6 
Signed-off-by: Claudio Netto <nettinhorama@gmail.com>
@nettoclaudio
refactor: moving the check of resource validation to helper function
1c655d8 
Signed-off-by: Claudio Netto <nettinhorama@gmail.com>
@nettoclaudio nettoclaudio force-pushed the skip-validation-of-workloads-with-only-limits-set branch from bba7a77 to 1c655d8 Compare December 21, 2023 23:10
@zroubalik
Copy link
Member

zroubalik commented Dec 22, 2023 •
edited by github-actions bot

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

@nettoclaudio
test: +e2e to cover that webhook dont validate workload with only lim…
465aebb 
…its set

Signed-off-by: Claudio Netto <nettinhorama@gmail.com>
@nettoclaudio
Copy link
Contributor Author

nettoclaudio commented Dec 22, 2023 •
edited

I apologize for the delay in addressing the change request, @JorTurFer. I've added the e2e that ensures that the webhook validator will validate the underlying workload whenever it only has resource limits provided. Can you please re-run the e2e @zroubalik?

@zroubalik
Copy link
Member

zroubalik commented Dec 22, 2023 •
edited by github-actions bot

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

@zroubalik
Copy link
Member

zroubalik commented Jan 2, 2024 •
edited by github-actions bot

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

@tomkerkhove tomkerkhove merged commit 9c23dae into kedacore:main Jan 3, 2024
19 checks passed
toniiiik pushed a commit to toniiiik/keda that referenced this pull request Jan 15, 2024
@nettoclaudio @toniiiik
fix: don't validate workload with only resource limits set (kedacore#…
63ff63a 
…4803)

Signed-off-by: anton.lysina <alysina@gmail.com>
@wozniakjan wozniakjan mentioned this pull request Jan 17, 2024
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zroubalik zroubalik zroubalik left review comments

@JorTurFer JorTurFer JorTurFer left review comments

@tomkerkhove tomkerkhove tomkerkhove approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Admission webhook should not reject workloads with only resource limits set
4 participants
@nettoclaudio @JorTurFer @tomkerkhove @zroubalik