Trigger the workloads eviction on admission check rejection. #1562

trasc · 2024-01-09T12:53:38Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Allow the workload reconciler to trigger the eviction of a workload that has Rejected admission checks and is Admitted, before making it as Finished.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Trigger an eviction for an admitted Job after an admission check changed state to Rejected.

netlify · 2024-01-09T12:53:45Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`fbca3bf`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/65a13596e0fa3000085df840

trasc · 2024-01-09T13:31:21Z

/cc @alculquicondor

mimowo

Please add a release note if this change makes a user-facing impact (which seems to be the case)

pkg/controller/core/workload_controller.go

alculquicondor · 2024-01-09T16:36:55Z

/approve
/hold
Leaving LGTM to @mimowo

pkg/controller/core/workload_controller.go

alculquicondor

/hold cancel

alculquicondor · 2024-01-10T14:49:59Z

/approve cancel

pkg/workload/workload.go

mimowo · 2024-01-11T11:44:35Z

pkg/controller/core/workload_controller.go

 	cqName, cqOk := r.queues.ClusterQueueForWorkload(&wl)
 	if cqOk {
 		if updated, err := r.reconcileSyncAdmissionChecks(ctx, &wl, cqName); updated || err != nil {
 			return ctrl.Result{}, err
 		}
 	}

-	if workload.SyncAdmittedCondition(&wl) {
+	// If the workload is not admitted update its admitted condition if necessary.
+	if !workload.IsAdmitted(&wl) && workload.SyncAdmittedCondition(&wl) {


Is this change needed, or just a performance optimization? If needed, does the order matter?

Also, is it correct, I'm asking because if the workload was already admitted, then (IIUC) we don't enter the branch, we also skip if workload.HasQuotaReservation(&wl) {, then we enter if rejectedChecks := workload.GetRejectedChecks(&wl); len(rejectedChecks) > 0 {. So we would mark the workload as finished.
I think the idea was to go via eviction rather than marking the workload as finished? Can you explain what is the code path leading to workload eviction in that scenario now?

It is needed, and the order is important since SyncAdmittedCondition can update the Admitted cind. in the wl.

If the workload is admitted, it will skip this and just check if the eviction is needed.

I checked that the integration test passes when the check !workload.IsAdmitted(&wl) is removed. Can we add / extend the integration test to demonstrate its importance?

done

(If we don't keep this the Admitted condition is cleared before the eviction starts)

(If we don't keep this the Admitted condition is cleared before the eviction starts)

Why does that matter?

To be clear I'm fine with having Admitted=True and Evicted=True, I'm also fine with clearing the Admitted before adding Evicted=True. Trying to understand what is the difference, to avoid unnecessary code complications.

comment updated.

To be clear I'm fine with having Admitted=True and Evicted=True, I'm also fine with clearing the Admitted before adding Evicted=True. Trying to understand what is the difference, to avoid unnecessary code complications.

It's more logical in my opinion and is the same flow as in case of premonition based eviction.

sgtm, I buy the argument of consistent flow with preemption-based eviction

test/integration/controller/core/workload_controller_test.go

mimowo · 2024-01-11T16:19:46Z

/lgtm
I don't consider the remaining comments as blocking, but nice-to-haves.

k8s-ci-robot · 2024-01-11T16:19:53Z

LGTM label has been added.

Git tree hash: fd03579ded1ca0a3f9f48e31766aaf8683edb151

pkg/controller/core/workload_controller.go

mimowo

LGTM, I think we also need a release note, I suggest "Fix the bug that a job would continue execution if the admission check state is changed to Rejected during execution."

pkg/controller/core/workload_controller.go

Co-authored-by: Michał Woźniak <mimowo@users.noreply.github.com>

trasc · 2024-01-12T12:51:21Z

LGTM, I think we also need a release note, I suggest "Fix the bug that a job would continue execution if the admission check state is changed to Rejected during execution."

done

mimowo

/lgtm

k8s-ci-robot · 2024-01-12T12:53:58Z

LGTM label has been added.

Git tree hash: dba641e2cb63a7d9e8df71013d46316a634496cc

alculquicondor · 2024-01-12T15:16:34Z

/approve

k8s-ci-robot · 2024-01-12T15:16:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, trasc

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:

~~OWNERS~~ [alculquicondor]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

alculquicondor · 2024-01-12T15:17:50Z

/release-note-edit

Trigger an eviction for an admitted Job after an admission check changed state to Rejected.

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. release-note-none Denotes a PR that doesn't merit a release note. labels Jan 9, 2024

k8s-ci-robot requested review from kerthcet and mimowo January 9, 2024 12:53

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 9, 2024

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 9, 2024

k8s-ci-robot requested a review from alculquicondor January 9, 2024 13:31

mimowo reviewed Jan 9, 2024

View reviewed changes

pkg/controller/core/workload_controller.go Outdated Show resolved Hide resolved

mimowo reviewed Jan 10, 2024

View reviewed changes

pkg/controller/core/workload_controller.go Outdated Show resolved Hide resolved

pkg/controller/core/workload_controller.go Outdated Show resolved Hide resolved

pkg/controller/core/workload_controller.go Outdated Show resolved Hide resolved

alculquicondor reviewed Jan 10, 2024

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 10, 2024

k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 10, 2024

trasc force-pushed the evict-workloads-with-rejected-AC branch 2 times, most recently from d7c44c3 to 89c647e Compare January 11, 2024 08:50

mimowo reviewed Jan 11, 2024

View reviewed changes

trasc force-pushed the evict-workloads-with-rejected-AC branch from c42c52e to 598ba28 Compare January 11, 2024 15:30

mimowo reviewed Jan 11, 2024

View reviewed changes

test/integration/controller/core/workload_controller_test.go Outdated Show resolved Hide resolved

trasc force-pushed the evict-workloads-with-rejected-AC branch from 598ba28 to 683d38c Compare January 11, 2024 15:52

k8s-ci-robot assigned mimowo Jan 11, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 11, 2024

alculquicondor reviewed Jan 11, 2024

View reviewed changes

pkg/controller/core/workload_controller.go Outdated Show resolved Hide resolved

pkg/controller/core/workload_controller.go Outdated Show resolved Hide resolved

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 11, 2024

k8s-ci-robot requested a review from mimowo January 11, 2024 20:31

trasc added 7 commits January 12, 2024 10:19

Trigger the workloads eviction on admission check rejection.

838fa1a

Wait for the eviction to and, add integration test.

b1a07d4

Review remarks.

8725651

Review Remarks

535444a

Review Remarks

8a05225

Review Remarks

d277388

Fix after rebase.

b10409e

trasc force-pushed the evict-workloads-with-rejected-AC branch from 9edd7f2 to b10409e Compare January 12, 2024 09:00

mimowo reviewed Jan 12, 2024

View reviewed changes

pkg/controller/core/workload_controller.go Outdated Show resolved Hide resolved

Update pkg/controller/core/workload_controller.go

fbca3bf

Co-authored-by: Michał Woźniak <mimowo@users.noreply.github.com>

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jan 12, 2024

mimowo reviewed Jan 12, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 12, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 12, 2024

k8s-ci-robot merged commit ea5bf98 into kubernetes-sigs:main Jan 12, 2024
14 checks passed

k8s-ci-robot added this to the v0.6 milestone Jan 12, 2024

trasc deleted the evict-workloads-with-rejected-AC branch January 15, 2024 18:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trigger the workloads eviction on admission check rejection. #1562

Trigger the workloads eviction on admission check rejection. #1562

trasc commented Jan 9, 2024 •

edited by k8s-ci-robot

netlify bot commented Jan 9, 2024 •

edited

trasc commented Jan 9, 2024

mimowo left a comment

alculquicondor commented Jan 9, 2024

alculquicondor left a comment

alculquicondor commented Jan 10, 2024

mimowo Jan 11, 2024

trasc Jan 11, 2024

mimowo Jan 11, 2024 •

edited

trasc Jan 11, 2024

mimowo Jan 11, 2024

trasc Jan 11, 2024

mimowo Jan 12, 2024

mimowo commented Jan 11, 2024

k8s-ci-robot commented Jan 11, 2024

mimowo left a comment

trasc commented Jan 12, 2024

mimowo left a comment

k8s-ci-robot commented Jan 12, 2024

alculquicondor commented Jan 12, 2024

k8s-ci-robot commented Jan 12, 2024

alculquicondor commented Jan 12, 2024

Trigger the workloads eviction on admission check rejection. #1562

Trigger the workloads eviction on admission check rejection. #1562

Conversation

trasc commented Jan 9, 2024 • edited by k8s-ci-robot

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

netlify bot commented Jan 9, 2024 • edited

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

trasc commented Jan 9, 2024

mimowo left a comment

Choose a reason for hiding this comment

alculquicondor commented Jan 9, 2024

alculquicondor left a comment

Choose a reason for hiding this comment

alculquicondor commented Jan 10, 2024

mimowo Jan 11, 2024

Choose a reason for hiding this comment

trasc Jan 11, 2024

Choose a reason for hiding this comment

mimowo Jan 11, 2024 • edited

Choose a reason for hiding this comment

trasc Jan 11, 2024

Choose a reason for hiding this comment

mimowo Jan 11, 2024

Choose a reason for hiding this comment

trasc Jan 11, 2024

Choose a reason for hiding this comment

mimowo Jan 12, 2024

Choose a reason for hiding this comment

mimowo commented Jan 11, 2024

k8s-ci-robot commented Jan 11, 2024

mimowo left a comment

Choose a reason for hiding this comment

trasc commented Jan 12, 2024

mimowo left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 12, 2024

alculquicondor commented Jan 12, 2024

k8s-ci-robot commented Jan 12, 2024

alculquicondor commented Jan 12, 2024

trasc commented Jan 9, 2024 •

edited by k8s-ci-robot

netlify bot commented Jan 9, 2024 •

edited

mimowo Jan 11, 2024 •

edited