Update wl/job handling for the case, when the job is finished #1383

achernevskii · 2023-11-30T02:02:49Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Workload is not being recreated, when the job is finished.
Job will be stopped if it's not suspended and matching workload is not found. This action doesn't depend on the job.Finished condition anymore.

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Avoid recreating a Workload for a finished Job and finalize a job when the workload is declared finished.

netlify · 2023-11-30T02:02:56Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`9eb11c5`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/656a47ba7af24100089af66b

tenzen-y · 2023-11-30T05:43:12Z

@achernevskii Why do we need this change? I couldn't see what is bug.

achernevskii · 2023-11-30T16:48:36Z

@achernevskii Why do we need this change? I couldn't see what is bug.

It's not a bug. The behavior has changed since v0.4.x in those two PRs:
https://github.com/kubernetes-sigs/kueue/pull/1072/files#diff-552ab657667de70900b502e2617bcb405fbf4203ff08b2a35d7c87f2d637a75bR431
https://github.com/kubernetes-sigs/kueue/pull/1033/files#diff-552ab657667de70900b502e2617bcb405fbf4203ff08b2a35d7c87f2d637a75bR186
And now workload is being recreated after the job is Finished.

After this discussion I've decided to go back to the original reconciler logic:
#1373 (comment)

alculquicondor · 2023-11-30T21:45:14Z

pkg/controller/jobframework/reconciler.go

-			if err := r.stopJob(ctx, job, w, StopReasonNoMatchingWorkload, "No matching Workload"); err != nil {
-				return nil, fmt.Errorf("stopping job with no matching workload: %w", err)
-			}
+		if err := r.stopJob(ctx, job, w, StopReasonNoMatchingWorkload, "No matching Workload"); err != nil {


I think we should only call stop if it's not already finished. Otherwise it's just a waste of API calls.

Is there a case where we need this for pod groups? It doesn't look like it.

Hypothetically, a job could be finished but unsuspended. I don't think it's possible for any of the existing integrations, but it could be possible from the GeneticJob interface perspective.

If we're trying to minimize the number of API calls in this case, shouldn't we call IsSuspended here instead of Finished?

IsSuspended is just about the suspend field. It would still return false for the job if it's finished but the suspend field is false. That's why we were still checking for IsFinished as well.

So Finished==true will always mean that no pods related to the job are active?

For the current reconciler, workload will be recreated. So if a job is finished, but not suspended, and new workload is not admitted, the job will be stopped:
https://github.com/kubernetes-sigs/kueue/blob/main/pkg/controller/jobframework/reconciler.go#L367

With this implementation we have to be sure that the job is stopped when we find out that workload is not found.

So Finished==true will always mean that no pods related to the job are active?

IIUC, Finished==true means that the job has succeeded or failed regardless of being suspended.

So Finished==true will always mean that no pods related to the job are active?

It should, unless the job implemented IsFinished wrong.

tenzen-y · 2023-12-01T06:45:51Z

@achernevskii Why do we need this change? I couldn't see what is bug.

It's not a bug. The behavior has changed since v0.4.x in those two PRs: https://github.com/kubernetes-sigs/kueue/pull/1072/files#diff-552ab657667de70900b502e2617bcb405fbf4203ff08b2a35d7c87f2d637a75bR431 https://github.com/kubernetes-sigs/kueue/pull/1033/files#diff-552ab657667de70900b502e2617bcb405fbf4203ff08b2a35d7c87f2d637a75bR186 And now workload is being recreated after the job is Finished.

After this discussion I've decided to go back to the original reconciler logic: #1373 (comment)

That makes sense. Thanks for the clarification.

/remove-kind-bug
/kind cleanup

tenzen-y · 2023-12-01T06:46:13Z

/remove-kind bug

alculquicondor · 2023-12-01T16:10:44Z

pkg/controller/jobframework/reconciler.go

+			if err != nil {
+				log.Error(err, "Updating workload status")
+			}


🤔 this might have escaped previous reviews.

If the Workload update fails, we should return the error, so it can be reconciled again (except for NotFound errors).

Another thing that escaped:

Finalizing the job should happen after we update the Workload. Otherwise, the job and the Workload (which is a dependent object) could potentially be deleted before it was transitioned to Finished. This could be fine, but it's better to give complete information before removing the Workload, in case a user is watching for the changes.

Regarding the job finalization/wl status update order: #1177

pkg/controller/jobs/job/job_controller_test.go

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

alculquicondor · 2023-12-01T16:21:14Z

pkg/controller/jobs/pod/pod_controller_test.go

@@ -1408,6 +1408,47 @@ func TestReconciler(t *testing.T) {
 			},
 			workloadCmpOpts: defaultWorkloadCmpOpts,
 		},
+		"if a pod group is finished and the wl is deleted, new workload shouldn't be created": {


can we add another one (if it isn't there)?

If a pod group is not finished (for example, it has a non-terminated gated Pod), a workload should be recreated.

We're indirectly checking for this scenario here:
https://github.com/kubernetes-sigs/kueue/blob/main/pkg/controller/jobs/pod/pod_controller_test.go#L1280

But I've added a test with gated pod anyway.

* Workload is not being recreated, when the job is finished. * If related workload is not found, the job will be stopped only if it's not finished.

alculquicondor · 2023-12-01T20:29:35Z

pkg/controller/jobframework/reconciler.go

+		if wl != nil && !apimeta.IsStatusConditionTrue(wl.Status.Conditions, kueue.WorkloadFinished) {
+			err := workload.UpdateStatus(ctx, r.client, wl, condition.Type, condition.Status, condition.Reason, condition.Message, constants.JobControllerName)
+			if err != nil && !apierrors.IsNotFound(err) {
+				log.Error(err, "Updating workload status")


you can remove this line, as returned errors are logged as well.

alculquicondor

/lgtm
/approve

k8s-ci-robot · 2023-12-01T21:44:15Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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]
~~pkg/util/OWNERS~~ [alculquicondor]

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

k8s-ci-robot · 2023-12-01T21:44:15Z

LGTM label has been added.

Git tree hash: e01b5e0a14ad32dc03cc44e76754e9a23505bec4

alculquicondor · 2024-01-11T16:48:47Z

/kind bug

alculquicondor · 2024-01-11T16:48:55Z

/remove-kind cleanup

alculquicondor · 2024-01-11T16:50:09Z

/release-note-edit

Avoid recreating a Workload for a finished Job and finalize a job when the workload is declared finished.

…etes-sigs#1383) * Update wl/job handling for the case, when the job is finished * Workload is not being recreated, when the job is finished. * If related workload is not found, the job will be stopped only if it's not finished. * Add job finalization if wl has a finished condition

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

k8s-ci-robot requested a review from mimowo November 30, 2023 02:02

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

k8s-ci-robot requested a review from trasc November 30, 2023 02:02

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

alculquicondor reviewed Nov 30, 2023

View reviewed changes

k8s-ci-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Dec 1, 2023

k8s-ci-robot removed the kind/bug Categorizes issue or PR as related to a bug. label Dec 1, 2023

alculquicondor reviewed Dec 1, 2023

View reviewed changes

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. 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 1, 2023

achernevskii force-pushed the fix/finished_job_handling branch from 8b89668 to b11d1dc Compare December 1, 2023 19:11

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

Update wl/job handling for the case, when the job is finished

9683b08

* Workload is not being recreated, when the job is finished. * If related workload is not found, the job will be stopped only if it's not finished.

achernevskii force-pushed the fix/finished_job_handling branch from b11d1dc to 9683b08 Compare December 1, 2023 19:30

alculquicondor reviewed Dec 1, 2023

View reviewed changes

Add job finalization if wl has a finished condition

9eb11c5

achernevskii force-pushed the fix/finished_job_handling branch from 5a62b5c to 9eb11c5 Compare December 1, 2023 20:53

alculquicondor reviewed Dec 1, 2023

View reviewed changes

k8s-ci-robot assigned alculquicondor Dec 1, 2023

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

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

k8s-ci-robot merged commit dbca421 into kubernetes-sigs:main Dec 1, 2023

k8s-ci-robot added this to the v0.6 milestone Dec 1, 2023

achernevskii deleted the fix/finished_job_handling branch December 4, 2023 05:28

alculquicondor mentioned this pull request Jan 11, 2024

High churn cluster with pod only causing stuck queue and overcommitment #1450

Closed

k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Jan 11, 2024

k8s-ci-robot removed the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Jan 11, 2024

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

alculquicondor mentioned this pull request Jan 11, 2024

Skip recreating a workload for a finished job #1572

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update wl/job handling for the case, when the job is finished #1383

Update wl/job handling for the case, when the job is finished #1383

achernevskii commented Nov 30, 2023 •

edited by k8s-ci-robot

Loading

netlify bot commented Nov 30, 2023 •

edited

Loading

tenzen-y commented Nov 30, 2023

achernevskii commented Nov 30, 2023

alculquicondor Nov 30, 2023

achernevskii Nov 30, 2023

alculquicondor Nov 30, 2023

achernevskii Nov 30, 2023

tenzen-y Dec 1, 2023

alculquicondor Dec 1, 2023

tenzen-y commented Dec 1, 2023

tenzen-y commented Dec 1, 2023

alculquicondor Dec 1, 2023

achernevskii Dec 1, 2023

alculquicondor Dec 1, 2023

achernevskii Dec 1, 2023

alculquicondor Dec 1, 2023

alculquicondor left a comment

k8s-ci-robot commented Dec 1, 2023

k8s-ci-robot commented Dec 1, 2023

alculquicondor commented Jan 11, 2024

alculquicondor commented Jan 11, 2024

alculquicondor commented Jan 11, 2024

Update wl/job handling for the case, when the job is finished #1383

Update wl/job handling for the case, when the job is finished #1383

Conversation

achernevskii commented Nov 30, 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?

netlify bot commented Nov 30, 2023 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

tenzen-y commented Nov 30, 2023

achernevskii commented Nov 30, 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

tenzen-y commented Dec 1, 2023

tenzen-y commented Dec 1, 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

alculquicondor left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 1, 2023

k8s-ci-robot commented Dec 1, 2023

alculquicondor commented Jan 11, 2024

alculquicondor commented Jan 11, 2024

alculquicondor commented Jan 11, 2024

achernevskii commented Nov 30, 2023 •

edited by k8s-ci-robot

Loading

netlify bot commented Nov 30, 2023 •

edited

Loading