-
Notifications
You must be signed in to change notification settings - Fork 228
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
Provide support for ProvisioningRequest's CapacityRevoked condition #2196
Conversation
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
/assign @yaroslava-serdiuk |
please, fix the tests |
5726dba
to
8fe6b4f
Compare
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.
Minor comment
Please, rebase the PR to resolve merge conflict |
cff252f
to
cf6330a
Compare
/lgtm |
LGTM label has been added. Git tree hash: 22bd86626a7137e9e4aebc737e06cc5dcebd29d5
|
/assign @tenzen-y |
Please also add to the release note that ProvisioningRequests objects persist until the Job or the Workload is deleted, IIUC. |
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.
One more comment / question regarding sending workload updates per AC
/lgtm |
LGTM label has been added. Git tree hash: 4d47a984051746c6a582c663b021d272b2b094fc
|
Let me see this. |
Please update description and release note. |
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.
/approve
/hold
for nit and description updates.
Co-authored-by: Aldo Culquicondor <1299064+alculquicondor@users.noreply.github.com>
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.
Basically, lgtm
test/integration/controller/admissionchecks/provisioning/provisioning_test.go
Outdated
Show resolved
Hide resolved
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.
Thank you!
/lgtm
/approve
LGTM label has been added. Git tree hash: 8c067e1612f2fd8f0620a2824677ee2fa7bf2e0f
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, PBundyra, tenzen-y 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 |
/hold |
/hold cancel |
…ubernetes-sigs#2196) * Provide support for ProvisioningRequest's CapacityRevoked condition * Clean-up * Improve naming * Add an unit test case, cleanup logs, slight logic changes to provisioning controller * Clean up, improve integration tests for provisioning request, set Evicted condition for Workload if the capacity for a ProvisioningRequest has been revoked * Add controllers to ProvisioningRequest integration tests, check evicted metric in integration tests, do not set evicted condition in the provisioning controller * Fix integration tests * Delete unit test that checks if that Evicted conditions stays the same * Discard deactivating workload in the provisioning controller * Batch workload status updates * Update pkg/controller/admissionchecks/provisioning/controller.go Co-authored-by: Aldo Culquicondor <1299064+alculquicondor@users.noreply.github.com> * Use IsActive function, clean up provisioning integration tests ---------
…ubernetes-sigs#2196) * Provide support for ProvisioningRequest's CapacityRevoked condition * Clean-up * Improve naming * Add an unit test case, cleanup logs, slight logic changes to provisioning controller * Clean up, improve integration tests for provisioning request, set Evicted condition for Workload if the capacity for a ProvisioningRequest has been revoked * Add controllers to ProvisioningRequest integration tests, check evicted metric in integration tests, do not set evicted condition in the provisioning controller * Fix integration tests * Delete unit test that checks if that Evicted conditions stays the same * Discard deactivating workload in the provisioning controller * Batch workload status updates * Update pkg/controller/admissionchecks/provisioning/controller.go Co-authored-by: Aldo Culquicondor <1299064+alculquicondor@users.noreply.github.com> * Use IsActive function, clean up provisioning integration tests --------- Co-authored-by: Aldo Culquicondor <1299064+alculquicondor@users.noreply.github.com>
/release-note-edit
|
What type of PR is this?
/kind feature
What this PR does / why we need it:
Provide support for the new ProvisioningRequest's condition
CapacityRevoked
. When user uses a job that allows retries and sets.spec.backOffLimit
> 0 and the job gets evicted because ofMaxRunDuration
an according AdmissionCheck gets rejected.After a Workload is finished we no longer delete corresponding ProvisioningRequest. This is because Workload might be finished due to
MaxRunDuration
timeout, before ClusterAutoscaler updates ProvisioningRequest's status toCapacityRevoked
and the Kueue's controller has no opportunity to react to that.Additionally I did a small cleanup.
Which issue(s) this PR fixes:
Part of #2041
Special notes for your reviewer:
I've tested it e2e manually
Does this PR introduce a user-facing change?