Update workload when job changes before admission #1223

yaroslava-serdiuk · 2023-10-18T19:11:20Z

What type of PR is this?

/kind feature

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?

Jobs preserve their position in the queue if the number of pods change before being admitted

netlify · 2023-10-18T19:11:27Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`9df2318`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/653fc8cb0ab3aa0008e041be

k8s-ci-robot · 2023-10-18T19:11:30Z

Hi @yaroslava-serdiuk. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

alculquicondor · 2023-10-18T19:56:38Z

/ok-to-test
/assign @mimowo

mimowo

Generally LGTM, just a question regarding the scenario when the job is running.

pkg/controller/jobframework/reconciler.go

mimowo · 2023-10-19T09:30:04Z

/lgtm
/assign @alculquicondor

alculquicondor

I think we also need to modify the workload_webhook, as it currently has PodSets as immutable.

Also please add a integration test for Job that starts with one that doesn't fit in the quota, (record the creationTimestmap of the Worklad at this point) and gets admitted after being updated to a smaller size (ensure that the creationTimestamp didn't change)

pkg/controller/jobframework/reconciler.go

alculquicondor · 2023-10-19T17:00:53Z

/lgtm cancel

yaroslava-serdiuk · 2023-10-26T10:54:57Z

@alculquicondor
I added a test that modifies parallelism and verify the creation stamp doesn't changed. The pod template requests are immutable fields, so I wanted to write the test you suggested in a way that job doesn't tolerate spot taint, so the job can't fit and with the toleration job can run. However I hit that adding toleration to the job does't update workload (or recreate in the old/current logic): #1264

mimowo

/lgtm

k8s-ci-robot · 2023-10-27T11:59:37Z

LGTM label has been added.

Git tree hash: ae7a2bf2a9c60f07070d2f22969c82beea38bf60

alculquicondor · 2023-10-27T17:04:12Z

/release-note-edit

Jobs preserve their position in the queue if their resource requests change before being admitted

alculquicondor · 2023-10-27T17:11:31Z

pkg/controller/jobs/job/job_controller_test.go

@@ -874,6 +874,26 @@ func TestReconciler(t *testing.T) {
 			},
 			wantErr: jobframework.ErrNoMatchingWorkloads,
 		},
+		"non-matching non-admitted workload is updated": {


Let's add a case for when the Workload has a quota reservation. In that case we would remove the existing Workload, right?

We might want to change what to do with a Workload that is admitted when the requests change, but that's for #77

we have the case for that just above current test ("non-matching admitted workload is deleted")

yaroslava-serdiuk · 2023-10-27T17:22:54Z

Jobs preserve their position in the queue if their resource requests change before being admitted

Resources requests are immutable, the user can't change them

alculquicondor · 2023-10-27T17:27:08Z

/release-note-edit

Jobs preserve their position in the queue if the number of pods change before being admitted

pkg/controller/jobs/job/job_controller_test.go

alculquicondor · 2023-10-27T17:22:07Z

pkg/webhooks/workload_webhook.go

-	allErrs = append(allErrs, apivalidation.ValidateImmutableField(newObj.Spec.PriorityClassSource, oldObj.Spec.PriorityClassSource, specPath.Child("priorityClassSource"))...)
-	allErrs = append(allErrs, apivalidation.ValidateImmutableField(newObj.Spec.PriorityClassName, oldObj.Spec.PriorityClassName, specPath.Child("priorityClassName"))...)
+
+	if workload.HasQuotaReservation(oldObj) {


What is the rationale for only checking oldObj?

I think we want to update the workload that doesn't have a quota reservation and this is oldObj.
I think if the update function is called for new object that has quota reservation and the old doesn't (not sure what the real case for that), it's should be the same as we call update first for new obj without quota reservation and then reserve the quota.
My understanding why we check the quota is that we can't guarantee the quota for new object, so we want to do updates when the quota is not reserved. If the quota is fine for new object, than we are fine.
Do I miss something?

Yeah, I think that's fine.
I just remembered that it is not possible to update Spec and Status at the same time.

pkg/webhooks/workload_webhook_test.go

test/integration/controller/jobs/job/job_controller_test.go

alculquicondor

/approve
/lgtm

k8s-ci-robot · 2023-10-31T16:14:00Z

LGTM label has been added.

Git tree hash: a1a77f08b74d82f96dfe792cdb45de5e582e1ff1

k8s-ci-robot · 2023-10-31T16:14:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, yaroslava-serdiuk

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

kerthcet · 2023-11-03T07:03:38Z

pkg/controller/jobframework/reconciler.go

+		return nil, fmt.Errorf("can't construct workload for update: %w", err)
+	}
+	wl.Spec = newWl.Spec
+	if err = r.client.Update(ctx, wl); err != nil {


So the key point here is we'll replace the unadmitted workload with the newest spec. right?

Correct. However note, that currently we can update only the amount of pods/containers: #1264

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Oct 18, 2023

k8s-ci-robot requested review from ahg-g and denkensk October 18, 2023 19:11

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

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 18, 2023

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

k8s-ci-robot assigned mimowo Oct 18, 2023

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 18, 2023

mimowo reviewed Oct 19, 2023

View reviewed changes

pkg/controller/jobframework/reconciler.go Show resolved Hide resolved

yaroslava-serdiuk force-pushed the workload-update branch from f1f4693 to c6a3d26 Compare October 19, 2023 09:22

k8s-ci-robot assigned alculquicondor Oct 19, 2023

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

alculquicondor reviewed Oct 19, 2023

View reviewed changes

pkg/controller/jobframework/reconciler.go Outdated Show resolved Hide resolved

pkg/controller/jobframework/reconciler.go Show resolved Hide resolved

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 19, 2023

k8s-ci-robot requested review from alculquicondor and mimowo October 19, 2023 17:00

yaroslava-serdiuk force-pushed the workload-update branch from c6a3d26 to aa11027 Compare October 26, 2023 09:26

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 Oct 26, 2023

yaroslava-serdiuk force-pushed the workload-update branch 2 times, most recently from f2debd9 to 07bbe48 Compare October 26, 2023 10:48

yaroslava-serdiuk force-pushed the workload-update branch from 07bbe48 to fa7e92b Compare October 26, 2023 11:19

yaroslava-serdiuk force-pushed the workload-update branch 3 times, most recently from 3e1243f to 4a3546e Compare October 27, 2023 08:41

mimowo reviewed Oct 27, 2023

View reviewed changes

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

alculquicondor reviewed Oct 27, 2023

View reviewed changes

Allow updating workload for suspended job

9df2318

yaroslava-serdiuk force-pushed the workload-update branch from 4a3546e to 9df2318 Compare October 30, 2023 15:16

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 30, 2023

k8s-ci-robot requested review from alculquicondor and mimowo October 30, 2023 15:16

alculquicondor reviewed Oct 31, 2023

View reviewed changes

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

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

k8s-ci-robot merged commit 0adc8c4 into kubernetes-sigs:main Oct 31, 2023

k8s-ci-robot added this to the v0.6 milestone Oct 31, 2023

kerthcet reviewed Nov 3, 2023

View reviewed changes

alculquicondor mentioned this pull request Nov 15, 2023

Add support for pod groups #1319

Merged

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

Update workload when job changes before admission #1223

Update workload when job changes before admission #1223

yaroslava-serdiuk commented Oct 18, 2023 •

edited by k8s-ci-robot

Loading

netlify bot commented Oct 18, 2023 •

edited

Loading

k8s-ci-robot commented Oct 18, 2023

alculquicondor commented Oct 18, 2023

mimowo left a comment

mimowo commented Oct 19, 2023

alculquicondor left a comment

alculquicondor commented Oct 19, 2023

yaroslava-serdiuk commented Oct 26, 2023

mimowo left a comment

k8s-ci-robot commented Oct 27, 2023

alculquicondor commented Oct 27, 2023

alculquicondor Oct 27, 2023 •

edited

Loading

yaroslava-serdiuk Oct 30, 2023

yaroslava-serdiuk commented Oct 27, 2023

alculquicondor commented Oct 27, 2023

alculquicondor Oct 27, 2023

yaroslava-serdiuk Oct 28, 2023

alculquicondor Oct 31, 2023

alculquicondor left a comment

k8s-ci-robot commented Oct 31, 2023

k8s-ci-robot commented Oct 31, 2023

kerthcet Nov 3, 2023

yaroslava-serdiuk Nov 3, 2023

Update workload when job changes before admission #1223

Update workload when job changes before admission #1223

Conversation

yaroslava-serdiuk commented Oct 18, 2023 • edited by k8s-ci-robot Loading

What type of PR is this?

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?

netlify bot commented Oct 18, 2023 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

k8s-ci-robot commented Oct 18, 2023

alculquicondor commented Oct 18, 2023

mimowo left a comment

Choose a reason for hiding this comment

mimowo commented Oct 19, 2023

alculquicondor left a comment

Choose a reason for hiding this comment

alculquicondor commented Oct 19, 2023

yaroslava-serdiuk commented Oct 26, 2023

mimowo left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 27, 2023

alculquicondor commented Oct 27, 2023

alculquicondor Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

yaroslava-serdiuk Oct 30, 2023

Choose a reason for hiding this comment

yaroslava-serdiuk commented Oct 27, 2023

alculquicondor commented Oct 27, 2023

alculquicondor Oct 27, 2023

Choose a reason for hiding this comment

yaroslava-serdiuk Oct 28, 2023

Choose a reason for hiding this comment

alculquicondor Oct 31, 2023

Choose a reason for hiding this comment

alculquicondor left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 31, 2023

k8s-ci-robot commented Oct 31, 2023

kerthcet Nov 3, 2023

Choose a reason for hiding this comment

yaroslava-serdiuk Nov 3, 2023

Choose a reason for hiding this comment

yaroslava-serdiuk commented Oct 18, 2023 •

edited by k8s-ci-robot

Loading

netlify bot commented Oct 18, 2023 •

edited

Loading

alculquicondor Oct 27, 2023 •

edited

Loading