Add support for JobSet DependsOn with PodsReady #7889

MaysaMacedo · 2025-11-25T15:43:38Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

When a dependsOn is defined on a JobSet and the waitForPodsReady
is configured in Kueue, the PodsReady condition should be udpated
only for the Jobs that are started, which is not the case currently,
all the jobs, including the ones with dependsOn that are not started
are being considered when checking if the respective Pods are ready.
This commit fixes the issue by ensuring that whenever there is some
updated in the job, it means the job has started to be managed and
it should be taken into account when defining if all the Pods are
ready.

Which issue(s) this PR fixes:

Fixes #6796

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Adds support for PodsReady when JobSet dependsOn is used.

k8s-ci-robot · 2025-11-25T15:43:41Z

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 · 2025-11-25T15:43:44Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`a82cad0`
🔍 Latest deploy log	https://app.netlify.com/projects/kubernetes-sigs-kueue/deploys/6932eae50bb2070008942e17

kannon92

cc @mimowo

Is this aligned with the idea you have for this feature?

mimowo · 2025-11-25T16:35:42Z

TBH, I don't think this is a feature, it is more of a bugfix after DependsOn was introduced into JobSet.

Is this aligned with the idea you have for this feature?

I will try to check that in more detail tomorrow, but possibly only after release 0.15.0

pkg/controller/jobs/jobset/jobset_controller.go

kannon92 · 2025-11-25T16:45:57Z

It would be great to have a e2e test demonstrating this works for waitForPodsReady.

mimowo · 2025-11-25T16:51:16Z

It would be great to have a e2e test demonstrating this works for waitForPodsReady.

e2e might be possible but tricky because the nature of the feature is timing-based, so I'm worried about flakes. I would start with unit and integration

MaysaMacedo · 2025-12-01T15:31:07Z

/hold

MaysaMacedo · 2025-12-01T17:35:46Z

/test pull-kueue-test-integration-baseline-main

kannon92 · 2025-12-01T17:42:11Z

This should have a release note.

pkg/util/testingjobs/jobset/wrappers.go

test/e2e/customconfigs/waitforpodsready_test.go

MaysaMacedo · 2025-12-04T12:13:13Z

/hold cancel

pkg/controller/jobs/jobset/jobset_controller.go

When a dependsOn is defined on a JobSet and the waitForPodsReady is configured in Kueue, the PodsReady condition should be udpated only for the Jobs that are started, which is not the case currently, all the jobs, including the ones with dependsOn that are not started are being considered when checking if the respective Pods are ready. This commit fixes the issue by ensuring that whenever there is some updated in the job, it means the job has started to be managed and it should be taken into account when defining if all the Pods are ready.

test/e2e/customconfigs/waitforpodsready_test.go

k8s-ci-robot · 2025-12-08T10:32:19Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: MaysaMacedo, olekzabl
Once this PR has been reviewed and has the lgtm label, please assign mimowo for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

mimowo · 2025-12-08T11:31:10Z

pkg/controller/jobs/jobset/jobset_controller.go

+		status, ok := jobsStatus[replicatedJob.Name]
+		// Register the amount of expected replicas from jobs that already has
+		// some status updated or doesn't have a dependsOn.
+		if replicatedJob.DependsOn == nil || (ok && status.Active+status.Failed+status.Ready+status.Succeeded+status.Suspended > 0) {


Here I think we here changes are required:

replicatedJob.DependsOn == nil

replicatedJob.DependsOn == Ready && status.Ready+status.Succeeded >= spec.ReplicatedJobs[idx of the Job]

replicatedJob.DependsOn=Completed && status.Succeeded >= spec.ReplicatedJobs[idx of the Job]

So:

is easy, as before

we delay the check until the downstream job is Ready, which in practice should be quick

we delay the check until the downstream job is Complete, this can take arbitrarily long

I'm not sure I follow your recommendation.
Why do we need to differentiate between a job having DependsOn ready or complete?
isn't it enough to verify if any status has changed for that job and that would mean the dependsOn dependency was satisfied and job is started, consequently that job should be taken into account to define the podsReady?

Ok I might be missing something as well, but this is my understanding.

Consider this example:

JobA dependsOn=Complete JobB

JobB is running for 1h, so JobA hasn't started yet

Now question: should we declare the JobSet as PodsReady=true, or PodsReady=false?

I was initially thinking that in this case PodsReady=true, because the JobSet if making progress. If we declared PodsReady=False in this case, then it would require the administrator to set waitForPodsReady.timeout > 1h.

OTOH, maybe this case, JobB taking 1h is a bit artificial.

If we use my recommendation and declare PodsReady=true for the JobSet, then in this case the JobSet may transition PodsReady=true to PodsReady=false once the JobB is complete. This admin could compensate by increasing the waitForPodsReady.recoveryTimeout, but it is also not indeed the primary use case of the timeout.

The current version of this PR is achieving your idea I think:

Job B is running

Job A dependsOn JobB complete

as the only job running is job B the status is podReady=true, once that is completed jobA starts and podReady condition might transition to False if the PodA taking a bit to be ready, for example downloading image, and then it transitions to podReady when the podA is ready.

The rationale I tried to follow in my implementation is:
The jobs that should be taken into account to set podsReady are the ones that have started.

Hm, can you point me to a test for that?

I see, interesting idea. I will continue tomorrow

This might be an example:
https://github.com/kubernetes-sigs/kueue/pull/7889/files#diff-54b6457ae9b6a5f7dbf838cda78fcce66cab0b097a47cc6adacf72f8ea9c498aR569-R584

The jobs that should be taken into account to set podsReady are the ones that have started.

when I mentioned have started, I meant that the job has any status updates for status.Active+status.Failed+status.Ready+status.Succeeded+status.Suspended.
From what I tested, If the job hasn't started to be handled it would have all as 0 in the status.

k8s-ci-robot requested review from kannon92 and tenzen-y November 25, 2025 15:43

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

kannon92 reviewed Nov 25, 2025

View reviewed changes

mimowo reviewed Nov 25, 2025

View reviewed changes

pkg/controller/jobs/jobset/jobset_controller.go Show resolved Hide resolved

MaysaMacedo force-pushed the support-jobset-waitforpodready branch from 26585c7 to 2ccaac0 Compare December 1, 2025 15:29

MaysaMacedo marked this pull request as ready for review December 1, 2025 15:29

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 Dec 1, 2025

k8s-ci-robot requested a review from kannon92 December 1, 2025 15:30

k8s-ci-robot removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 1, 2025

k8s-ci-robot requested a review from mbobrovskyi December 1, 2025 15:30

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 1, 2025

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 1, 2025

MaysaMacedo force-pushed the support-jobset-waitforpodready branch from 2ccaac0 to 1a70b19 Compare December 1, 2025 23:21

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Dec 2, 2025

MaysaMacedo force-pushed the support-jobset-waitforpodready branch 2 times, most recently from 2bb5d5e to 6457364 Compare December 3, 2025 22:09

olekzabl reviewed Dec 3, 2025

View reviewed changes

pkg/util/testingjobs/jobset/wrappers.go Outdated Show resolved Hide resolved

test/e2e/customconfigs/waitforpodsready_test.go Show resolved Hide resolved

MaysaMacedo force-pushed the support-jobset-waitforpodready branch 6 times, most recently from 0963962 to 56f8699 Compare December 4, 2025 12:12

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

MaysaMacedo force-pushed the support-jobset-waitforpodready branch from 56f8699 to 5f64777 Compare December 4, 2025 12:38

kannon92 reviewed Dec 4, 2025

View reviewed changes

pkg/controller/jobs/jobset/jobset_controller.go Show resolved Hide resolved

MaysaMacedo force-pushed the support-jobset-waitforpodready branch from 5f64777 to a82cad0 Compare December 5, 2025 14:23

olekzabl approved these changes Dec 8, 2025

View reviewed changes

test/e2e/customconfigs/waitforpodsready_test.go Show resolved Hide resolved

mimowo reviewed Dec 8, 2025

View reviewed changes

Add support for JobSet DependsOn with PodsReady #7889

Are you sure you want to change the base?

Add support for JobSet DependsOn with PodsReady #7889

Conversation

MaysaMacedo commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

k8s-ci-robot commented Nov 25, 2025

Uh oh!

netlify bot commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Uh oh!

kannon92 left a comment

Choose a reason for hiding this comment

Uh oh!

mimowo commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

kannon92 commented Nov 25, 2025

Uh oh!

mimowo commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaysaMacedo commented Dec 1, 2025

Uh oh!

MaysaMacedo commented Dec 1, 2025

Uh oh!

kannon92 commented Dec 1, 2025

Uh oh!

Uh oh!

Uh oh!

MaysaMacedo commented Dec 4, 2025

Uh oh!

Uh oh!

Uh oh!

k8s-ci-robot commented Dec 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaysaMacedo Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

MaysaMacedo commented Nov 25, 2025 •

edited

Loading

netlify bot commented Nov 25, 2025 •

edited

Loading

mimowo commented Nov 25, 2025 •

edited

Loading

mimowo commented Nov 25, 2025 •

edited

Loading

MaysaMacedo Dec 9, 2025 •

edited

Loading