Adding toleration to the job doesn't trigger workload change #1304

stuton · 2023-11-02T14:13:59Z

What type of PR is this?

/kind cleanup

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?

Changing tolerations in an inadmissible job triggers an admission retry with the updated tolerations.

k8s-ci-robot · 2023-11-02T14:14:02Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

netlify · 2023-11-02T14:14:07Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

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

alculquicondor · 2023-11-13T18:51:32Z

pkg/util/equality/podset.go

@@ -26,30 +26,33 @@ import (

 // TODO: Revisit this, maybe we should extend the check to everything that could potentially impact
 // the workload scheduling (priority, nodeSelectors(when suspended), tolerations and maybe more)
-func comparePodTemplate(a, b *corev1.PodSpec) bool {
+func comparePodTemplate(a, b *corev1.PodSpec, checkCount, changePodSpecFields bool) bool {
+	if changePodSpecFields && !equality.Semantic.DeepEqual(a.Tolerations, b.Tolerations) {


why do we need this boolean?

the tolerations could change when the job is unsuspended

alculquicondor

Let's add a unit test for the job controller where we can see that the Workload object is updated when you change the toleration.

And another one where if you change the toleration for a Job that is admitted, the Workload gets evicted.

trasc · 2023-12-15T14:46:24Z

And another one where if you change the toleration for a Job that is admitted, the Workload gets evicted.

Is this a very common use-case, I think is easier to make the tolerations immutable while the job is not suspended.

alculquicondor · 2023-12-15T15:18:45Z

Is this a very common use-case, I think is easier to make the tolerations immutable while the job is not suspended.

Tolerations are already immutable for k8s Jobs when not suspended https://kubernetes.io/docs/concepts/workloads/controllers/job/#mutable-scheduling-directives

But that's not true for all CRDs. We might be able to add the restriction as webhooks for jobs we have integrations for, but we can't control other implementations. So worth having the safe guard in the reconciler.

trasc · 2023-12-19T14:27:07Z

Let's add a unit test for the job controller where we can see that the Workload object is updated when you change the toleration.

And another one where if you change the toleration for a Job that is admitted, the Workload gets evicted.

Done, however the overall experience for the case when the tolerations are changed while admitted is at least strange, the job is suspended and the workload is removed but the tolerations from the workload are restored in the job ...

alculquicondor · 2023-12-27T18:57:47Z

pkg/controller/jobs/job/job_controller_test.go

+					Obj(),
+			},
+		},
+		"the workload is admitted and tolerations change": {


Is there already a case where the workload is admitted, but the job is still suspended?

"suspended job with matching admitted workload is unsuspended" .. why?

We need to cover what I describe above: "a suspended job should match either the running or base specs".

If it doesn't match either, I guess it's pretty much treated as this case "the workload is admitted and tolerations change", correct?

alculquicondor · 2023-12-27T18:58:42Z

pkg/controller/jobs/job/job_controller_test.go

+					PodSets(
+						*utiltesting.MakePodSet("main", 10).
+							Toleration(corev1.Toleration{
+								Key:      "tolarationkey1",


Suggested change

Key: "tolarationkey1",

Key: "tolerationkey1",

pkg/controller/jobs/job/job_controller_test.go

pkg/controller/jobs/kubeflow/jobs/mxjob/mxjob_controller_test.go

pkg/util/equality/podset.go

trasc · 2024-01-11T20:51:00Z

/assign

pkg/controller/jobframework/reconciler.go

alculquicondor · 2024-01-12T16:35:56Z

pkg/controller/jobframework/reconciler.go

+		if canBePartiallyAdmitted && ps.MinCount != nil {
+			// update the expected running count
+			ps.Count = psi.Count
+		}


can this just be:

psi.Count = psi.Count

?

If the value is set, admission already determined if partial admission was possible.

no, if we are not using partial admission we need to be strict about the count

alculquicondor · 2024-01-12T16:46:00Z

pkg/controller/jobframework/reconciler.go

-	// If the workload is admitted but the job is suspended, ignore counts.
-	// This might allow some violating jobs to pass equivalency checks, but their
-	// workloads would be invalidated in the next sync after unsuspending.


a version of this comment should be left before return job.IsSuspended() && equality.ComparePodSetSlices(jobPodSets, wl.Spec.PodSets)

I guess we are saying that, if the job is suspended, it can match either the running or the base specs.

alculquicondor · 2024-01-12T16:51:56Z

pkg/controller/jobs/job/job_controller_test.go

+					Obj(),
+			},
+		},
+		"the workload is admitted and tolerations change": {


We need to cover what I describe above: "a suspended job should match either the running or base specs".

If it doesn't match either, I guess it's pretty much treated as this case "the workload is admitted and tolerations change", correct?

pkg/util/equality/podset.go

tenzen-y

overall lgtm

tenzen-y · 2024-01-15T18:02:08Z

pkg/util/equality/podset_test.go

+				Effect:   corev1.TaintEffectNoSchedule,
+			}).Obj()},
+			wantEqual: false,
+		},
 		"different count when checked": {


Suggested change

"different count when checked": {

"different count": {

tenzen-y · 2024-01-15T18:02:32Z

pkg/controller/jobframework/reconciler.go

@@ -607,8 +613,39 @@ func (r *JobReconciler) ensurePrebuiltWorkloadInSync(ctx context.Context, wl *ku
 	return true, nil
 }

+// get the expected podsets during the job execution, returns nil if the workload has no reservation or


Suggested change

// get the expected podsets during the job execution, returns nil if the workload has no reservation or

// expectedRunningPodSets gets the expected podsets during the job execution, returns nil if the workload has no reservation or

alculquicondor

/approve
/hold
for nits

trasc · 2024-01-16T05:29:20Z

nits covered
/unhold

tenzen-y

Thanks!
/lgtm
/approve

k8s-ci-robot · 2024-01-16T05:32:17Z

LGTM label has been added.

Git tree hash: 480a555650ad54d8cfc4e0105aa398e13eb0b32c

k8s-ci-robot · 2024-01-16T05:32:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, stuton, 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:

~~pkg/controller/OWNERS~~ [alculquicondor,tenzen-y]
~~pkg/util/OWNERS~~ [alculquicondor,tenzen-y]

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

alculquicondor · 2024-02-13T20:18:23Z

/remove-kind cleanup
/kind feature

alculquicondor · 2024-02-13T20:20:46Z

/release-note-edit

Changing tolerations in an inadmissible job triggers an admission retry.

alculquicondor · 2024-02-13T20:21:14Z

/release-note-edit

Changing tolerations in an inadmissible job triggers an admission retry with the updated tolerations.

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Nov 2, 2023

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Nov 2, 2023

k8s-ci-robot requested review from denkensk and tenzen-y November 2, 2023 14:14

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 2, 2023

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 2, 2023

stuton marked this pull request as ready for review November 3, 2023 13:48

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 3, 2023

stuton changed the title ~~Adding toleration to the job doesn't trigger workload change~~ [WIP] Adding toleration to the job doesn't trigger workload change Nov 3, 2023

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 3, 2023

stuton force-pushed the update-workload-when-add-toleration branch from d446ad9 to 1a99a26 Compare November 13, 2023 12:03

stuton changed the title ~~[WIP] Adding toleration to the job doesn't trigger workload change~~ Adding toleration to the job doesn't trigger workload change Nov 13, 2023

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 13, 2023

alculquicondor reviewed Nov 13, 2023

View reviewed changes

alculquicondor reviewed Nov 24, 2023

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 19, 2023

trasc force-pushed the update-workload-when-add-toleration branch from b3cd54d to 2a88403 Compare December 19, 2023 14:22

trasc force-pushed the update-workload-when-add-toleration branch from d3fb537 to 2a88403 Compare December 19, 2023 15:05

alculquicondor reviewed Dec 27, 2023

View reviewed changes

k8s-ci-robot assigned trasc Jan 11, 2024

trasc force-pushed the update-workload-when-add-toleration branch from 2a88403 to 536fb42 Compare January 12, 2024 08:08

alculquicondor reviewed Jan 12, 2024

View reviewed changes

Anton Stuchinskii and others added 5 commits January 15, 2024 09:38

Adding toleration to the job doesn't trigger workload change

dcffea0

[jobframework] Construct the running podsets during equivalency check

cb80a72

[controller/job] Tolerations change test.

eee3522

Review remarks, after rebase fix.

61bd8f9

Review Remarks

df14fd3

trasc force-pushed the update-workload-when-add-toleration branch from 536fb42 to df14fd3 Compare January 15, 2024 08:34

tenzen-y reviewed Jan 15, 2024

View reviewed changes

alculquicondor reviewed Jan 15, 2024

View reviewed changes

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 15, 2024

Review Remarks

ed5c81f

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 16, 2024

tenzen-y reviewed Jan 16, 2024

View reviewed changes

k8s-ci-robot assigned tenzen-y Jan 16, 2024

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

k8s-ci-robot merged commit 8a6296f into kubernetes-sigs:main Jan 16, 2024
14 checks passed

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

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. and removed kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Feb 13, 2024

trasc deleted the update-workload-when-add-toleration branch March 12, 2024 08:55

alculquicondor mentioned this pull request May 17, 2024

Plain Pod gets deleted once admitted via ProvisioningRequest (DWS) #2213

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding toleration to the job doesn't trigger workload change #1304

Adding toleration to the job doesn't trigger workload change #1304

stuton commented Nov 2, 2023 •

edited by k8s-ci-robot

Loading

k8s-ci-robot commented Nov 2, 2023

netlify bot commented Nov 2, 2023 •

edited

Loading

alculquicondor Nov 13, 2023

trasc Dec 15, 2023

alculquicondor left a comment

trasc commented Dec 15, 2023

alculquicondor commented Dec 15, 2023

trasc commented Dec 19, 2023

alculquicondor Dec 27, 2023

trasc Jan 11, 2024

alculquicondor Jan 12, 2024

trasc Jan 15, 2024

alculquicondor Dec 27, 2023

trasc Jan 11, 2024

trasc commented Jan 11, 2024

alculquicondor Jan 12, 2024

trasc Jan 15, 2024

alculquicondor Jan 12, 2024

trasc Jan 15, 2024

alculquicondor Jan 12, 2024

tenzen-y left a comment

tenzen-y Jan 15, 2024

trasc Jan 16, 2024

tenzen-y Jan 15, 2024

trasc Jan 16, 2024

alculquicondor left a comment

trasc commented Jan 16, 2024

tenzen-y left a comment

k8s-ci-robot commented Jan 16, 2024

k8s-ci-robot commented Jan 16, 2024

alculquicondor commented Feb 13, 2024

alculquicondor commented Feb 13, 2024

alculquicondor commented Feb 13, 2024

	// get the expected podsets during the job execution, returns nil if the workload has no reservation or
	// expectedRunningPodSets gets the expected podsets during the job execution, returns nil if the workload has no reservation or

Adding toleration to the job doesn't trigger workload change #1304

Adding toleration to the job doesn't trigger workload change #1304

Conversation

stuton commented Nov 2, 2023 • edited by k8s-ci-robot Loading

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?

k8s-ci-robot commented Nov 2, 2023

netlify bot commented Nov 2, 2023 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor left a comment

Choose a reason for hiding this comment

trasc commented Dec 15, 2023

alculquicondor commented Dec 15, 2023

trasc commented Dec 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trasc commented Jan 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tenzen-y left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor left a comment

Choose a reason for hiding this comment

trasc commented Jan 16, 2024

tenzen-y left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 16, 2024

k8s-ci-robot commented Jan 16, 2024

alculquicondor commented Feb 13, 2024

alculquicondor commented Feb 13, 2024

alculquicondor commented Feb 13, 2024

stuton commented Nov 2, 2023 •

edited by k8s-ci-robot

Loading

netlify bot commented Nov 2, 2023 •

edited

Loading