Allow updating scheduling directives of suspended jobs that never started #105479

ahg-g · 2021-10-05T02:12:37Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Implements kubernetes/enhancements#2926

Which issue(s) this PR fixes:

Fixes #104714

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Node affinity, node selector and tolerations are now mutable for jobs that are suspended and have never been started

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-scheduling/2926-job-mutable-scheduling-directives

k8s-ci-robot · 2021-10-05T02:12:44Z

@ahg-g: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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/test-infra repository.

ahg-g · 2021-10-05T13:01:32Z

/label api-review

ahg-g · 2021-10-05T13:02:00Z

/assign @liggitt

ahg-g · 2021-10-06T15:50:13Z

/sig scheduling
/sig apps

liggitt · 2021-10-08T15:09:49Z

pkg/apis/batch/validation/validation.go

+		oldTemplate.Spec.NodeSelector = template.Spec.NodeSelector
+		oldTemplate.Spec.Tolerations = template.Spec.Tolerations


@deads2k I thought we had a verification check that would complain about these if there wasn't a +k8s:verify-mutation comment (in this case, it's actually ok because we're working on a copy made above, but I still expected the explicit check / comment to be required)

@ahg-g this PR will need rebasing once #105578 merges, and will need to add +k8s:verify-mutation:reason=clone directives on the lines making cloned assignments to/from the old object

Which tests are supposed to fail if this line isn't added? It seems that the tests are still passing

verify CI is failing the verify: non-mutating-validation test (you can run yourself with hack/verify-non-mutating-validation.sh)

thanks, done.

pkg/apis/batch/validation/validation.go

liggitt · 2021-10-08T15:13:07Z

pkg/apis/batch/validation/validation.go

+	oldTemplate := &oldSpec.Template
+	if opts.AllowMutableSchedulingDirectives {
+		oldTemplate = oldSpec.Template.DeepCopy()
+		if template.Spec.Affinity != nil {


this seems like it misses the case where template.Spec.Affinity is nil and oldTemplate.Spec.Affinity has node affinity directives. if so, add a unit test that would have caught that

Good catch! one thing here to note here, to succeed, the current validation will require having an empty Affinity if NodeAffinity was removed and other affinity types don't exist, removing the whole Affinity spec will fail.

Original

Template { Affinity: { NodeAffinity: { .... } }

Update succeeds

Template: { Affinity: {} }

Update fails

Template: { }

supporting the case above will probably require cloning the new template or do some magic to check if Affinity is empty without actually individually checking its members (because the logic will break if a new member is added and we forget to update it).

@liggitt is the above semantics acceptable?

yeah, something like this, perhaps:

switch { case template.Spec.Affinity == nil && oldTemplate.Spec.Affinity != nil: // allow the Affinity field to be cleared if the old template had no affinity directives other than NodeAffinity oldTemplate.Spec.Affinity.NodeAffinity = nil if (*oldTemplate.Spec.Affinity) == (api.Affinity{}) { oldTemplate.Spec.Affinity = nil } case template.Spec.Affinity != nil && oldTemplate.Spec.Affinity == nil: // allow the NodeAffinity field to skip immutability checking oldTemplate.Spec.Affinity.NodeAffinity = &api.Affinity{NodeAffinity: template.Spec.Affinity.NodeAffinity} case template.Spec.Affinity != nil && oldTemplate.Spec.Affinity != nil: // allow the NodeAffinity field to skip immutability checking oldTemplate.Spec.Affinity.NodeAffinity = template.Spec.Affinity.NodeAffinity }

sg, updated.

pkg/features/kube_features.go

pkg/apis/batch/validation/validation_test.go

liggitt · 2021-10-14T13:43:46Z

/approve

API bits lgtm, go ahead and squash

liggitt · 2021-10-14T13:44:04Z

/hold cancel

k8s-ci-robot · 2021-10-14T13:44:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, liggitt

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/apis/OWNERS~~ [liggitt]
~~pkg/features/OWNERS~~ [ahg-g,liggitt]
~~pkg/registry/OWNERS~~ [liggitt]
~~test/OWNERS~~ [liggitt]

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

alculquicondor · 2021-10-14T13:51:28Z

pkg/apis/batch/validation/validation_test.go

+				Field: "spec.template",
+			},
+		},
+		"mutable node affinity": {


I think we are covering the first and second branch. We could have these tests:

add node affinity

change node affinity

remove node affinity

I added another test to cover the third branch.

…jobs that never started

alculquicondor

/lgtm

liggitt · 2021-10-23T15:52:22Z

test/integration/job/job_test.go

@@ -629,6 +630,81 @@ func TestSuspendJobControllerRestart(t *testing.T) {
 	}, false)
 }

+func TestNodeSelectorUpdate(t *testing.T) {


seeing this flake a lot at HEAD - https://storage.googleapis.com/k8s-triage/index.html?pr=1&test=TestNodeSelectorUpdate

thanks for flagging, we need to wrap the update calls with RetryOnConflict

k8s-ci-robot requested review from aojea and dchen1107 October 5, 2021 02:13

ahg-g force-pushed the ahg-mutable branch from 397e689 to a4b62f6 Compare October 5, 2021 02:16

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 5, 2021

k8s-ci-robot added the api-review Categorizes an issue or PR as actively needing an API review. label Oct 5, 2021

k8s-ci-robot assigned liggitt Oct 5, 2021

liggitt added this to Assigned in API Reviews Oct 5, 2021

liggitt added this to the v1.23 milestone Oct 5, 2021

ahg-g force-pushed the ahg-mutable branch from a4b62f6 to 238d38a Compare October 5, 2021 13:04

ahg-g mentioned this pull request Oct 5, 2021

Mutable scheduling directives for suspended Jobs kubernetes/enhancements#2926

Closed

8 tasks

ahg-g changed the title ~~Allow updating scheduling directives suspended jobs that never started~~ Allow updating scheduling directives for suspended jobs that never started Oct 5, 2021

ahg-g changed the title ~~Allow updating scheduling directives for suspended jobs that never started~~ Allow updating scheduling directives of suspended jobs that never started Oct 5, 2021

k8s-ci-robot added the sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. label Oct 6, 2021

liggitt assigned alculquicondor Oct 6, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 8, 2021

liggitt reviewed Oct 8, 2021

View reviewed changes

liggitt moved this from In progress to Changes requested in API Reviews Oct 8, 2021

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Oct 8, 2021

ahg-g force-pushed the ahg-mutable branch from 4f654d4 to e870374 Compare October 8, 2021 18:50

k8s-ci-robot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 8, 2021

liggitt moved this from Changes requested to In progress in API Reviews Oct 11, 2021

alculquicondor reviewed Oct 12, 2021

View reviewed changes

pkg/apis/batch/validation/validation_test.go Outdated Show resolved Hide resolved

ahg-g force-pushed the ahg-mutable branch from e870374 to 78f8e09 Compare October 13, 2021 22:49

liggitt moved this from In progress to Changes requested in API Reviews Oct 14, 2021

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

liggitt moved this from Changes requested to API review completed, 1.23 in API Reviews Oct 14, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 14, 2021

ahg-g force-pushed the ahg-mutable branch from 772a606 to 7b27fd4 Compare October 14, 2021 13:44

alculquicondor reviewed Oct 14, 2021

View reviewed changes

ahg-g force-pushed the ahg-mutable branch from 7b27fd4 to e4d63a1 Compare October 14, 2021 14:03

Allow updating node affinity, selector and tolerations for suspended …

335817c

…jobs that never started

ahg-g force-pushed the ahg-mutable branch from e4d63a1 to 335817c Compare October 14, 2021 14:04

alculquicondor reviewed Oct 14, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 14, 2021

k8s-ci-robot merged commit dea052c into kubernetes:master Oct 14, 2021

liggitt reviewed Oct 23, 2021

View reviewed changes

ahg-g deleted the ahg-mutable branch October 25, 2021 14:43

ahg-g mentioned this pull request Oct 28, 2021

Make job pod template annotations and labels mutable for suspended jobs that never started #105980

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow updating scheduling directives of suspended jobs that never started #105479

Allow updating scheduling directives of suspended jobs that never started #105479

ahg-g commented Oct 5, 2021 •

edited by liggitt

k8s-ci-robot commented Oct 5, 2021

ahg-g commented Oct 5, 2021

ahg-g commented Oct 5, 2021

ahg-g commented Oct 6, 2021

liggitt Oct 8, 2021

liggitt Oct 8, 2021 •

edited

ahg-g Oct 12, 2021

liggitt Oct 14, 2021 •

edited

ahg-g Oct 14, 2021

liggitt Oct 8, 2021

ahg-g Oct 8, 2021 •

edited

ahg-g Oct 12, 2021

liggitt Oct 14, 2021 •

edited

ahg-g Oct 14, 2021

liggitt commented Oct 14, 2021

liggitt commented Oct 14, 2021

k8s-ci-robot commented Oct 14, 2021

alculquicondor Oct 14, 2021

ahg-g Oct 14, 2021

alculquicondor left a comment

liggitt Oct 23, 2021

ahg-g Oct 25, 2021

		oldTemplate.Spec.NodeSelector = template.Spec.NodeSelector
		oldTemplate.Spec.Tolerations = template.Spec.Tolerations

Allow updating scheduling directives of suspended jobs that never started #105479

Allow updating scheduling directives of suspended jobs that never started #105479

Conversation

ahg-g commented Oct 5, 2021 • edited by liggitt

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?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Oct 5, 2021

ahg-g commented Oct 5, 2021

ahg-g commented Oct 5, 2021

ahg-g commented Oct 6, 2021

Choose a reason for hiding this comment

liggitt Oct 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Oct 14, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahg-g Oct 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Oct 14, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Oct 14, 2021

liggitt commented Oct 14, 2021

k8s-ci-robot commented Oct 14, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahg-g commented Oct 5, 2021 •

edited by liggitt

liggitt Oct 8, 2021 •

edited

liggitt Oct 14, 2021 •

edited

ahg-g Oct 8, 2021 •

edited

liggitt Oct 14, 2021 •

edited