Prefactor works of implementing job interface #589

kerthcet · 2023-02-21T09:07:15Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

prefactor work before #544

The job controller work flow would like:

handling job is finished
handling workload is nil
processing only main job related logics
handling job is suspended
handling job is unsuspended

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

netlify · 2023-02-21T09:07:20Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`e33b886`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/6400112b44a3ab0008bb81df

kerthcet · 2023-02-21T09:08:23Z

cc @mimowo

mimowo

/lgtm
/assign @alculquicondor
Thanks!

alculquicondor · 2023-02-21T18:07:13Z

pkg/controller/workload/job/job_controller.go

@@ -210,39 +207,41 @@ func (r *JobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.R
 	log := ctrl.LoggerFrom(ctx).WithValues("job", klog.KObj(&job))
 	ctx = ctrl.LoggerInto(ctx, log)

-	pwName := parentWorkload(&job)
+	isMainJob := parentWorkloadName(&job) == ""


isStandaloneJob

alculquicondor · 2023-02-21T18:12:48Z

pkg/controller/workload/job/job_controller.go

+
+	// 3. handle workload is nil.
+	if wl == nil {
+		if !isMainJob {
 			return ctrl.Result{}, nil


in this case, shouldn't we should suspend the dependent job?

so maybe it should be if wl == nil && isStandaloneJob and below (step 6) it should say if wl == nil || wl.Spec.Admission == nil

We only create workload here, leaving the suspend logics to another reconciler. Passing a nil workload downwards is somehow dangerous.

After creating the workload:

if dependent job is suspended, jump to step5.

if dependent job is unsuspended, jump to step6.

But one corner case is both parent and child jobs are unsuspended, then we delete the workload manually. Then if the workload:

admitted immediately again with the same assignment: it works well

admitted immediately again with a different assignment: the node affinity may be changed so not working as expected

unadmitted, jobs will be suspended finally.

Then we may have a potential bug here. We can suspend all the jobs here when workload is nil .

it could also be the case that somehow the parent job controller is crashing (and not able to create a workload) and somehow a user set the child job to suspend=false. We don't want that child job to continue running.

open a ticket here #595

@kerthcet can you double check this is an issue currently, we have this integration test:

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

Line 245 in b2a5e38

ginkgo.It("Should suspend a job if the parent workload does not exist", func() {

IIUC the code to ensure that is here: https://github.com/kubernetes-sigs/kueue/blob/main/pkg/controller/workload/job/job_controller.go#L483-L495

Sure, I'll take charge of this.

I agree. This shouldn't be a problem in the existing code.

Yes, I think it will not happen, I just complicated the situation about admitted immediately again with a different assignment: the node affinity may be changed so not working as expected, the admitted workload should be create at first, then at that moment, the job will be suspended.

it could also be the case that somehow the parent job controller is crashing (and not able to create a workload) and somehow a user set the child job to suspend=false. We don't want that child job to continue running.

This will also be covered by https://github.com/kubernetes-sigs/kueue/blob/main/pkg/controller/workload/job/job_controller.go#L483-L495

pkg/controller/workload/job/job_controller.go

alculquicondor

/assign @mimowo

alculquicondor · 2023-02-22T15:58:12Z

pkg/controller/workload/job/job_controller.go

+
+	// 3. handle workload is nil.
+	if wl == nil {
+		if !isMainJob {
 			return ctrl.Result{}, nil


it could also be the case that somehow the parent job controller is crashing (and not able to create a workload) and somehow a user set the child job to suspend=false. We don't want that child job to continue running.

kerthcet · 2023-02-27T14:18:25Z

Updated, PTAL.

pkg/controller/workload/job/job_controller.go

alculquicondor · 2023-02-27T16:52:01Z

pkg/controller/workload/job/job_controller.go

-		log.Error(err, "Unable to list child workloads")
-		return ctrl.Result{}, err
-	}
-
 	// 1. make sure there is only a single existing instance of the workload


Add a comment here that if there was no workload, this will suspend the job.

Although I find it confusing. Maybe we should move that part out of the function and call stopJob here.

That's kind of thing I already done in job interface library, let's visit this part on that one.

alculquicondor · 2023-03-01T15:04:39Z

/approve
@kerthcet just a couple of nits

k8s-ci-robot · 2023-03-01T15:05:10Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

Signed-off-by: Kante Yin <kerthcet@gmail.com>

kerthcet · 2023-03-02T03:08:42Z

ping @mimowo for LGTM

mimowo · 2023-03-02T06:07:23Z

/lgtm
Thanks!

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

k8s-ci-robot requested review from ahg-g and tenzen-y February 21, 2023 09:07

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 21, 2023

mimowo reviewed Feb 21, 2023

View reviewed changes

k8s-ci-robot assigned alculquicondor and mimowo Feb 21, 2023

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

alculquicondor reviewed Feb 21, 2023

View reviewed changes

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

tenzen-y reviewed Feb 22, 2023

View reviewed changes

pkg/controller/workload/job/job_controller.go Outdated Show resolved Hide resolved

alculquicondor reviewed Feb 22, 2023

View reviewed changes

kerthcet mentioned this pull request Feb 27, 2023

Suspend jobs when workload is nil in reconciling #595

Closed

kerthcet force-pushed the cleanup/preparation-to-job-interface branch from 0d25130 to 6e7146c Compare February 27, 2023 14:09

alculquicondor reviewed Feb 27, 2023

View reviewed changes

alculquicondor mentioned this pull request Feb 27, 2023

Add MPIJob controller #578

Merged

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

Make pre-factor of job interface

e33b886

Signed-off-by: Kante Yin <kerthcet@gmail.com>

kerthcet force-pushed the cleanup/preparation-to-job-interface branch from 6e7146c to e33b886 Compare March 2, 2023 02:59

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

k8s-ci-robot merged commit 271e66a into kubernetes-sigs:main Mar 2, 2023

kerthcet deleted the cleanup/preparation-to-job-interface branch March 2, 2023 08:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prefactor works of implementing job interface #589

Prefactor works of implementing job interface #589

kerthcet commented Feb 21, 2023

netlify bot commented Feb 21, 2023 •

edited

Loading

kerthcet commented Feb 21, 2023

mimowo left a comment •

edited

Loading

alculquicondor Feb 21, 2023

kerthcet Feb 22, 2023

alculquicondor Feb 21, 2023

kerthcet Feb 22, 2023 •

edited

Loading

kerthcet Feb 22, 2023 •

edited

Loading

alculquicondor Feb 22, 2023

kerthcet Feb 27, 2023

mimowo Feb 27, 2023 •

edited

Loading

kerthcet Feb 27, 2023

alculquicondor Feb 27, 2023

kerthcet Feb 27, 2023

kerthcet Feb 27, 2023

alculquicondor left a comment

alculquicondor Feb 22, 2023

kerthcet commented Feb 27, 2023

alculquicondor Feb 27, 2023

kerthcet Mar 2, 2023

alculquicondor commented Mar 1, 2023 •

edited

Loading

k8s-ci-robot commented Mar 1, 2023

kerthcet commented Mar 2, 2023

mimowo commented Mar 2, 2023

Prefactor works of implementing job interface #589

Prefactor works of implementing job interface #589

Conversation

kerthcet commented Feb 21, 2023

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:

netlify bot commented Feb 21, 2023 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

kerthcet commented Feb 21, 2023

mimowo left a comment • edited Loading

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

kerthcet Feb 22, 2023 • edited Loading

Choose a reason for hiding this comment

kerthcet Feb 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimowo Feb 27, 2023 • edited Loading

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

Choose a reason for hiding this comment

kerthcet commented Feb 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor commented Mar 1, 2023 • edited Loading

k8s-ci-robot commented Mar 1, 2023

kerthcet commented Mar 2, 2023

mimowo commented Mar 2, 2023

netlify bot commented Feb 21, 2023 •

edited

Loading

mimowo left a comment •

edited

Loading

kerthcet Feb 22, 2023 •

edited

Loading

kerthcet Feb 22, 2023 •

edited

Loading

mimowo Feb 27, 2023 •

edited

Loading

alculquicondor commented Mar 1, 2023 •

edited

Loading