KEP-672: Implement the DependsOn API #740

andreyvelich · 2024-12-27T13:17:09Z

What type of PR is this?

/kind feature
/kind api-change

What this PR does / why we need it:

I added implementation for the DependsOn API. The majority of validations are implemented using CEL.

TODO list:

Add unit tests for the depends_on.go
Add controller integration tests to verify CEL validation.
Add controller integration tests to verify Job creation.

/cc @ahg-g @kannon92 @danielvegamyhre @tenzen-y @vsoch

Which issue(s) this PR fixes:

Fixes: #672

Does this PR introduce a user-facing change?

Add the DependsOn API to support serial execution of ReplicatedJobs.

k8s-ci-robot · 2024-12-27T13:17:15Z

@andreyvelich: GitHub didn't allow me to request PR reviews from the following users: vsoch.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

What type of PR is this?

/kind feature
/kind api-change

What this PR does / why we need it:

I added implementation for the DependsOn API. The majority of validations are implemented using CEL.

TODO list:

Add unit tests for the depends_on.go

Add controller integration tests to verify CEL validation.

Add controller integration tests to verify Job creation.

/cc @ahg-g @kannon92 @danielvegamyhre @tenzen-y @vsoch

Which issue(s) this PR fixes:

Fixes: #672

Does this PR introduce a user-facing change?
Add the DependsOn API to support serial execution of ReplicatedJobs.

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-sigs/prow repository.

k8s-ci-robot · 2024-12-27T13:17:19Z

Hi @andreyvelich. 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-sigs/prow repository.

netlify · 2024-12-27T13:17:30Z

✅ Deploy Preview for kubernetes-sigs-jobset ready!

Name	Link
🔨 Latest commit	`0df9651`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-jobset/deploys/67800cefca730a0008b978dc
😎 Deploy Preview	https://deploy-preview-740--kubernetes-sigs-jobset.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

andreyvelich · 2024-12-27T13:20:07Z

api/jobset/v1alpha2/jobset_types.go

+
+	// Complete status means the Succeeded counter equals the number of child Jobs.
+	// .spec.replicatedJobs["name==<JOB_NAME>"].replicas == .status.replicatedJobsStatus.name["name==<JOB_NAME>"].succeeded
+	CompleteStatus DependsOnStatus = "Complete"


What do we think about Complete status here, given the discussion in: #723 ?

We probably want to match the Kubernetes Job and use Complete, so keep as is.

Yea I think that is fine.

+1 for consistency w/ Job API

kannon92 · 2024-12-27T14:43:04Z

/ok-to-test

andreyvelich · 2025-01-03T01:12:40Z

I've added integration and unit tests. Let me know if that looks good to you!
/assign @ahg-g @kannon92 @danielvegamyhre @tenzen-y @vsoch @shravan-achar @akshaychitneni

k8s-ci-robot · 2025-01-03T01:12:44Z

@andreyvelich: GitHub didn't allow me to assign the following users: vsoch, shravan-achar, akshaychitneni.

Note that only kubernetes-sigs members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

I've added integration and unit tests. Let me know if that looks good to you!
/assign @ahg-g @kannon92 @danielvegamyhre @tenzen-y @vsoch @shravan-achar @akshaychitneni

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-sigs/prow repository.

vsoch · 2025-01-03T03:49:08Z

api/jobset/v1alpha2/jobset_types.go

+
+	// Complete status means the Succeeded counter equals the number of child Jobs.
+	// .spec.replicatedJobs["name==<JOB_NAME>"].replicas == .status.replicatedJobsStatus.name["name==<JOB_NAME>"].succeeded
+	CompleteStatus DependsOnStatus = "Complete"


We probably want to match the Kubernetes Job and use Complete, so keep as is.

kannon92 · 2025-01-03T16:27:30Z

test/integration/webhook/jobset_webhook_test.go

@@ -417,5 +417,126 @@ var _ = ginkgo.Describe("jobset webhook defaulting", func() {
 			},
 			updateShouldFail: true,
 		}),
+		ginkgo.Entry("DependsOn and StartupPolicy can't be set together", &testCase{


Please add a test case for StartupPolicy set to AnyOrder and allowing DependsOn

kannon92 · 2025-01-03T16:28:34Z

api/jobset/v1alpha2/jobset_types.go

+	// only after the referenced ReplicatedJobs reach their desired state.
+	// The Order of ReplicatedJobs is defined by their enumeration in the slice.
+	// Note, that the first ReplicatedJob in the slice cannot use the DependsOn API.
+	// TODO (andreyvelich): Currently, only a single item is supported in the DependsOn list.


I don't think we should have TODO in user facing APIs.

Sure, I will remove it.

kannon92 · 2025-01-03T16:36:54Z

pkg/controllers/jobset_controller.go

+		rJobsReplicas[replicatedJob.Name] = replicatedJob.Replicas
+
+		// For depends on, the Job is created only after the previous replicatedJob reached the status.
+		if replicatedJob.DependsOn != nil && !isDependsOnJobReachedStatus(replicatedJob.DependsOn[0], rJobsReplicas[replicatedJob.DependsOn[0].Name], replicatedJobStatuses) {


I think this is a bug. You probably want the idx - 1 rather than 0.

@andreyvelich there are some comments in this PR confused about the hardcoded index 0, it surprised me as well at first until I remembered the design discussion about single vs multi-dependency and future proofing.

I think hard coding the index of a specific dependency makes the code a bit surprising/confusing as compared to simply looping through the slice checking all dependencies, and using the CEL validation you've added to enforce the single dependency only.

I see, @danielvegamyhre do you suggest that in this PR we implement the dependencyReachedStatus function in a way that it can accept array and verify that all dependent ReplicatedJobs reach status ?

kannon92

Given how important this is for kubeflow, I am wondering if a e2e test would be useful.

kannon92 · 2025-01-03T16:40:11Z

pkg/webhooks/jobset_webhook_test.go

@@ -1487,10 +1487,152 @@ func TestValidateCreate(t *testing.T) {
 		},
 	}

+	dependsOnTests := []validationTestCase{
+		{
+			name: "dependsOn is valid",


Can we add a case where job-2 waits for job-1 and job-3 waits for job-2?

k8s-ci-robot · 2025-01-06T15:21:57Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: andreyvelich, vsoch
Once this PR has been reviewed and has the lgtm label, please ask for approval from ahg-g. 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

andreyvelich · 2025-01-06T15:43:22Z

Given how important this is for kubeflow, I am wondering if a e2e test would be useful.

Yeah, maybe we could add one e2e test as part of JobSet: https://github.com/kubernetes-sigs/jobset/blob/main/test/e2e/e2e_test.go.
Will add it soon.

andreyvelich · 2025-01-07T14:50:52Z

@kannon92 @tenzen-y I've added the simple E2E test for DependsOn API.

kannon92 · 2025-01-08T16:52:50Z

Can we make sure to add some examples and maybe call this out on our site?

andreyvelich · 2025-01-08T17:16:45Z

Can we make sure to add some examples and maybe call this out on our site?

Sure, I can add simple example with sleep.
Actually, once we have implementation of MPI Runtime in Kubeflow Training V2, I am planning to add example of MPI job that uses DependsOn API. We can link those examples in the JobSet docs as well.
kubeflow/training-operator#2217

kannon92 · 2025-01-08T20:42:09Z

Sure, I can add simple example with sleep.
Actually, once we have implementation of MPI Runtime in Kubeflow Training V2, I am planning to add example of MPI job that uses DependsOn API. We can link those examples in the JobSet docs as well.

We can have sleep for now. I'd prefer to have standalone examples without requiring Kubeflow in our user facing docs.

danielvegamyhre · 2025-01-09T04:53:28Z

api/jobset/v1alpha2/jobset_types.go

+
+	// Complete status means the Succeeded counter equals the number of child Jobs.
+	// .spec.replicatedJobs["name==<JOB_NAME>"].replicas == .status.replicatedJobsStatus.name["name==<JOB_NAME>"].succeeded
+	CompleteStatus DependsOnStatus = "Complete"


+1 for consistency w/ Job API

danielvegamyhre · 2025-01-09T05:02:31Z

pkg/controllers/jobset_controller.go

+		rJobsReplicas[replicatedJob.Name] = replicatedJob.Replicas
+
+		// For depends on, the Job is created only after the previous replicatedJob reached the status.
+		if replicatedJob.DependsOn != nil && !isDependsOnJobReachedStatus(replicatedJob.DependsOn[0], rJobsReplicas[replicatedJob.DependsOn[0].Name], replicatedJobStatuses) {


@andreyvelich there are some comments in this PR confused about the hardcoded index 0, it surprised me as well at first until I remembered the design discussion about single vs multi-dependency and future proofing.

I think hard coding the index of a specific dependency makes the code a bit surprising/confusing as compared to simply looping through the slice checking all dependencies, and using the CEL validation you've added to enforce the single dependency only.

danielvegamyhre · 2025-01-09T05:07:30Z

api/jobset/v1alpha2/jobset_types.go

+
+const (
+	// Ready status means the Ready + Succeeded + Failed counter equals the number of child Jobs.
+	ReadyStatus DependsOnStatus = "Ready"


I think we should rename these status variables something like DependencyReady and DependencyComplete (or ReplicatedJobReady and ReplicatedJobComplete) since they are specific to a replicatedJob dependency and not the status of the JobSet itself.

With the current naming, in the other places we're importing the api as jobset and using these variables, they sound like they are referring to the status of the JobSet itself (e.g. jobset.ReadyStatus, jobset.CompletedStatus, etc), which is confusing/misleading.

Yeah, that is good point!

danielvegamyhre · 2025-01-09T05:09:16Z

pkg/controllers/depends_on.go

+)
+
+// isDependsOnJobReachedStatus checks if depends on ReplicatedJob reaches Ready or Complete status.
+func isDependsOnJobReachedStatus(dependsOnJob jobset.DependsOn, dependsOnJobReplicas int32, rJobsStatuses []jobset.ReplicatedJobStatus) bool {


The function name is a bit confusing to me, maybe we can name it something like dependencyReachedStatus or replicatedJobReachedStatus?

andreyvelich · 2025-01-09T17:00:45Z

@danielvegamyhre @kannon92 What do you think about example that I added ?

Improve API docs

Rename func to dependencyReachedStatus

Signed-off-by: Andrey Velichkevich <[email protected]>

kannon92 · 2025-01-09T17:26:34Z

site/static/examples/depends-on/depends-on.yaml

+                    - /bin/sh
+                    - -c
+                    - "echo 'initializer runs for 10 seconds' && sleep 10"
+    - name: launcher


For the launcher maybe we should have some kind of readiness probe that can explain how this would be used for ready?

Right now it would go to active/running very quick due to no readiness probe.

Sure, I changed it to startupProbe for launcher.

Right now it would go to active/running very quick due to no readiness probe.

Actually, the Job will be Ready only after all InitContainers are complete: https://kubernetes.io/docs/concepts/workloads/pods/pod-lifecycle/#:~:text=Ready%3A%20the%20Pod%20is%20able%20to%20serve%20requests%20and%20should%20be%20added%20to%20the%20load%20balancing%20pools%20of%20all%20matching%20Services.

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 27, 2024

k8s-ci-robot requested a review from ahg-g December 27, 2024 13:17

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Dec 27, 2024

k8s-ci-robot requested review from kannon92 and danielvegamyhre December 27, 2024 13:17

k8s-ci-robot requested a review from tenzen-y December 27, 2024 13:17

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 27, 2024

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Dec 27, 2024

andreyvelich commented Dec 27, 2024

View reviewed changes

k8s-ci-robot assigned ahg-g, danielvegamyhre, kannon92 and tenzen-y Jan 3, 2025

andreyvelich changed the title ~~[WIP] KEP-672: Implement the DependsOn API~~ KEP-672: Implement the DependsOn API Jan 3, 2025

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 Jan 3, 2025

vsoch approved these changes Jan 3, 2025

View reviewed changes

kannon92 reviewed Jan 3, 2025

View reviewed changes

andreyvelich force-pushed the issue-672-implement-depends-on branch from 76d53e5 to bf743bb Compare January 6, 2025 15:27

andreyvelich force-pushed the issue-672-implement-depends-on branch from c0b8760 to d76b286 Compare January 6, 2025 22:39

danielvegamyhre reviewed Jan 9, 2025

View reviewed changes

andreyvelich added 13 commits January 9, 2025 17:11

Add DependsOn API

718087c

Add JobSet controller changes

6f58d62

Add integration tests for DependsOn CEL validation

7463e32

Add unit tests for DependsOn

3024bf4

Add controller integration tests for DependsOn

ee80b1c

Fix go lint

fd02136

Add test case with DependsOn and StartupPolicy: AnyOrder

7d06e74

Improve API docs

Test case when job-2 depends on job-1 and job-3 depends on job-2

3f2a576

Add manifests to the make generate

ac32d7c

Add E2E test for the DependsOn API

b6889b7

Rename var to DependencyReady and DependencyComplete

4394179

Rename func to dependencyReachedStatus

Update docs and add DependsOn example

c7cbe97

Update to 1.32

13e133c

Signed-off-by: Andrey Velichkevich <[email protected]>

andreyvelich force-pushed the issue-672-implement-depends-on branch from 386fad0 to 13e133c Compare January 9, 2025 17:12

kannon92 reviewed Jan 9, 2025

View reviewed changes

Use startupProbe for launcher

0df9651

andreyvelich force-pushed the issue-672-implement-depends-on branch from ffd4715 to 0df9651 Compare January 9, 2025 17:52

KEP-672: Implement the DependsOn API #740

Are you sure you want to change the base?

KEP-672: Implement the DependsOn API #740

Conversation

andreyvelich commented Dec 27, 2024 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Dec 27, 2024

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Dec 27, 2024

netlify bot commented Dec 27, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-jobset ready!

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

kannon92 commented Dec 27, 2024

andreyvelich commented Jan 3, 2025

k8s-ci-robot commented Jan 3, 2025

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

Choose a reason for hiding this comment

kannon92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 6, 2025

andreyvelich commented Jan 6, 2025

andreyvelich commented Jan 7, 2025

kannon92 commented Jan 8, 2025

andreyvelich commented Jan 8, 2025

kannon92 commented Jan 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielvegamyhre Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreyvelich commented Jan 9, 2025

Choose a reason for hiding this comment

andreyvelich Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

andreyvelich commented Dec 27, 2024 •

edited

Loading

netlify bot commented Dec 27, 2024 •

edited

Loading

danielvegamyhre Jan 9, 2025 •

edited

Loading

andreyvelich Jan 9, 2025 •

edited

Loading