-
Notifications
You must be signed in to change notification settings - Fork 262
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
Reset all AdmissionChecks on Eviction to Pending #3323
Conversation
/assign @mimowo |
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
856cb00
to
7fd0b67
Compare
7fd0b67
to
1251a44
Compare
1251a44
to
8a96e88
Compare
c5e1385
to
0f86c45
Compare
0f86c45
to
99bc312
Compare
I think we actually need to reset the checks regardless of the reason of eviction. For example, if the checks passed, but the workload is evicted due to WaitForPodsReady? |
0e6dab2
to
1451b4e
Compare
When deactivating (evicting with deactivation reason) a workload we reset the kueue/pkg/controller/core/workload_controller.go Lines 203 to 206 in 42c7b47
I think for consistency we should do the same with checks, but we can re-visit this as a follow up. /lgtm |
LGTM label has been added. Git tree hash: 2c3dd1a32282d6a662b4c6c8192d1f63d446f7e9
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mimowo, PBundyra The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
test/integration/controller/admissionchecks/provisioning/provisioning_test.go
Show resolved
Hide resolved
It seems that this is obviously bug. |
@tenzen-y: #3323 failed to apply on top of branch "release-0.8":
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
I guess we could cherry-pick but then we also need to cherry-pick the forced-ownership PR which might be more that we want. |
…ernetes-sigs#3323) * Make AdmissionChecks Pending after one is in Retry state * Reduce number of patch requests * Improve message, fix e2e test * Improve naming and message on retry * Reset check on different evictions
…ernetes-sigs#3323) * Make AdmissionChecks Pending after one is in Retry state * Reduce number of patch requests * Improve message, fix e2e test * Improve naming and message on retry * Reset check on different evictions
…) (#3395) * Make AdmissionChecks Pending after one is in Retry state * Reduce number of patch requests * Improve message, fix e2e test * Improve naming and message on retry * Reset check on different evictions Co-authored-by: Patryk Bundyra <[email protected]>
…ernetes-sigs#3323) * Make AdmissionChecks Pending after one is in Retry state * Reduce number of patch requests * Improve message, fix e2e test * Improve naming and message on retry * Reset check on different evictions
…ernetes-sigs#3323) * Make AdmissionChecks Pending after one is in Retry state * Reduce number of patch requests * Improve message, fix e2e test * Improve naming and message on retry * Reset check on different evictions
What type of PR is this?
/kind bug
What this PR does / why we need it:
Currently, when AdmissionCheck is in
Retry
state, the Workloads gets evicted and there's no actual retry. The changes improves that flow, changing all AdmissionChecks toPending
if one of them is inRetry
state (unless there's aRejected
AdmissionCheck).Which issue(s) this PR fixes:
Fixes #3322
Special notes for your reviewer:
Does this PR introduce a user-facing change?