Add MPIJob controller #578

mimowo · 2023-02-16T09:16:02Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #65

Special notes for your reviewer:

The follow ups:

extract the code common code into a library
use the released version of MPI operator 0.4.0 (Release 0.4.0 kubeflow/mpi-operator#507)

k8s-ci-robot · 2023-02-16T09:16:03Z

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 · 2023-02-16T09:16:06Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`2d435c8`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/640207eb0657800008633f57

tenzen-y · 2023-02-16T09:26:12Z

@mimowo Can we use a job interface (#537)? Or would you like to switch implementation using a job interface after #544 is completed?

mimowo · 2023-02-16T09:31:35Z

@mimowo Can we use a job interface (#537)? Or would you like to switch implementation using a job interface after #544 is completed?

As mentioned here: #65 (comment). For now, I'm doing a rough prototype by copy-paste to see what the obstacles are. I guess once it is working we could also abstract out the library based on the two implementations. This could also give us better clues of how the final interface should look like. I guess we need to sync up on this.

tenzen-y

I added the first feedback.

tenzen-y · 2023-03-01T15:44:03Z

config/components/rbac/mpijob_editor_role.yaml

@@ -0,0 +1,27 @@
+# permissions for end users to edit jobs.


For the long term, we may want to prepare a separate kustomization.yaml for each integration in the following:

config/ ├── components ├── default ├── default_with_mpijob ├── default_with_training-operator ├── default_with_rayjob ├── default_with_all_integration ...

pkg/util/testing/wrappers_mpijob.go

tenzen-y · 2023-03-01T16:10:15Z

pkg/controller/workload/mpijob/mpijob_controller.go

+// SetupWithManager sets up the controller with the Manager. It indexes workloads
+// based on the owning jobs.
+func (r *MPIJobReconciler) SetupWithManager(mgr ctrl.Manager) error {
+	return ctrl.NewControllerManagedBy(mgr).
+		For(&kubeflow.MPIJob{}).
+		Owns(&kueue.Workload{}).
+		Complete(r)
+}


If there are no CRDs for MPIJob in the cluster, the kueue-controller always says logs in the following, although kueue works fine:

{"level":"error","ts":"2023-03-01T16:01:40.4642038Z","logger":"controller-runtime.source","caller":"source/source.go:143","msg":"if kind is a CRD, it should be installed before calling Start","kind":"MPIJob.kubeflow.org","error":"no matches for kind \"MPIJob\" in version \"kubeflow.org/v2beta1\"","stacktrace":"sigs.k8s.io/controller-runtime/pkg/source.(*Kind).Start.func1.1\n\t/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.14.2/pkg/source/source.go:143\nk8s.io/apimachinery/pkg/util/wait.runConditionWithCrashProtectionWithContext\n\t/go/pkg/mod/k8s.io/apimachinery@v0.26.1/pkg/util/wait/wait.go:235\nk8s.io/apimachinery/pkg/util/wait.WaitForWithContext\n\t/go/pkg/mod/k8s.io/apimachinery@v0.26.1/pkg/util/wait/wait.go:662\nk8s.io/apimachinery/pkg/util/wait.poll\n\t/go/pkg/mod/k8s.io/apimachinery@v0.26.1/pkg/util/wait/wait.go:596\nk8s.io/apimachinery/pkg/util/wait.PollImmediateUntilWithContext\n\t/go/pkg/mod/k8s.io/apimachinery@v0.26.1/pkg/util/wait/wait.go:547\nsigs.k8s.io/controller-runtime/pkg/source.(*Kind).Start.func1\n\t/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.14.2/pkg/source/source.go:136"}

Maybe we can skip to watch MPIJob using following library if there are no CRDs for MPIJob:

// If err is not nil, there are no CRDs for MPIJob in the cluster. _, err := mgr.GetRESTMapper().RESTMapping(schema.GroupKind{Group: kubeflow.SchemeGroupVersion.Group, Kind: "MPIJob"})

WDYT?

I guess it's an overkill for now, as we plan to move this code to a separate binary

I guess it's an overkill for now, as we plan to move this code to a separate binary

Does that mean we will run controllers for CustomJobs as sidecar containers?

Either sidecars or separate deployments.

I suggest this as a future improvement that may not be needed if we move the code out to a separate binary (probably next release though).

test/integration/controller/mpijob/suite_test.go

test/integration/controller/mpijob/mpijob_controller_test.go

tenzen-y

Can we use MPIJob or mpijob in messages and comments instead of Job or job?

pkg/controller/workload/mpijob/mpijob_controller.go

mimowo · 2023-03-01T17:43:31Z

Can we use MPIJob or mpijob in messages and comments instead of Job or job?

I have applied the suggestions. I think this will be easier to maintain once we commonize the code, which is planned, based on the interface: https://github.com/kubernetes-sigs/kueue/tree/main/keps/369-job-interface.

tenzen-y · 2023-03-02T02:08:48Z

Can we use MPIJob or mpijob in messages and comments instead of Job or job?

I have applied the suggestions. I think this will be easier to maintain once we commonize the code, which is planned, based on the interface: https://github.com/kubernetes-sigs/kueue/tree/main/keps/369-job-interface.

Ah, I see. sgtm

mimowo · 2023-03-02T06:19:51Z

/assign @kerthcet
Please complete the review and lgtm.

kerthcet · 2023-03-02T09:22:35Z

I'll take a look but maybe later(tomorrow I think), stuck with other things. Hope you don't mind. ^_^

pkg/controller/workload/mpijob/mpijob_controller.go

kerthcet · 2023-03-03T09:21:31Z

pkg/controller/workload/job/job_controller.go

@@ -519,7 +520,7 @@ func ConstructWorkloadFor(ctx context.Context, client client.Client,
 	job *batchv1.Job, scheme *runtime.Scheme) (*kueue.Workload, error) {
 	w := &kueue.Workload{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      job.Name,
+			Name:      GetWorkloadNameForJob(job.Name),


This is a user facing change we should highlight. But seems we can't do as in Kubernetes currently. Just to notify that this may be involved in the changeLog.

Kind of, workloads are generally API objects that should be "technical details" to the users. But, it might be worth mentioning, I will ask to add the note when we prepare release notes for 0.3.0.

test/integration/controller/job/job_controller_test.go

kerthcet · 2023-03-03T09:39:25Z

pkg/controller/workload/jobframework/workload_names.go

+limitations under the License.
+*/
+
+package jobframework


Maybe we should rename the folder workload to job for Workload is a specific object in Kueue, then the dir tree looks like below, but that can be a separate commit.

job/ ├── batchjob ├── mpijob ├── framework

Potentially, but this would make the diff under this PR unnecessarily big. If we decide on the folder rename this should be a dedicated PR.

Personally, I don't have a strong opinion on the layout. Maybe it can be discussed further when we commonize the code.

I'm ok with the proposal by @kerthcet, for another PR

Let me finish the job library first, or I need to resolve the conflicts again.

kerthcet · 2023-03-03T10:33:27Z

pkg/controller/workload/mpijob/mpijob_controller.go

+
+// podsReady checks if all pods are ready or succeeded
+func podsReady(job *kubeflow.MPIJob) bool {
+	for _, c := range job.Status.Conditions {


Seems like when MPIJob is running, then it's ready. Not quite familiar with MPIJob operator, what will happen before job running?

Yes, the Running condition is set when all workers are running and the launcher: https://github.com/kubeflow/mpi-operator/blob/5946ef4157599a474ab82ff80e780d5c2546c9ee/pkg/controller/mpi_job_controller.go#L1041-L1045.
Before that, the MPIJobCreated is set, and MPIJobSuspended (if it is suspended).

pkg/controller/workload/mpijob/mpijob_webhook.go

test/integration/controller/mpijob/mpijob_controller_test.go

kerthcet · 2023-03-03T11:31:19Z

config/components/rbac/mpijob_editor_role.yaml

@@ -0,0 +1,27 @@
+# permissions for end users to edit jobs.


BatchJob is an in-tree object, so I think it's ok to maintain the yaml here. And we can better organize it like we do in test/e2e/config(I haven't tried it yet). But I don't want to block this significant feature.

mimowo · 2023-03-03T12:37:03Z

@kerthcet thank you for the review! I think I have addressed the remarks. Let me know if something more is needed.

alculquicondor · 2023-03-03T14:38:20Z

/lgtm

alculquicondor · 2023-03-03T14:47:30Z

/lgtm

mimowo · 2023-03-03T14:48:03Z

Rebased and squashed. @alculquicondor please renew lgtm :)

terrytangyuan

This is great! We should add more docs and examples around this.

kerthcet · 2023-03-04T09:29:28Z

Thanks @mimowo that's a great start!

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. labels Feb 16, 2023

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

k8s-ci-robot requested review from ahg-g and denkensk February 16, 2023 09:16

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Feb 16, 2023

mimowo marked this pull request as ready for review February 16, 2023 09:20

k8s-ci-robot requested a review from tenzen-y February 16, 2023 09:20

mimowo mentioned this pull request Feb 16, 2023

Support kubeflow's MPIJob #65

Closed

mimowo marked this pull request as draft February 16, 2023 09:32

This was referenced Feb 16, 2023

Implement genericjob interface on batchv1.Job #544

Closed

Release 0.4.0 kubeflow/mpi-operator#507

Closed

mimowo force-pushed the mpijob-controller branch from 2e73c42 to e3f5c67 Compare February 17, 2023 17:21

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 17, 2023

mimowo force-pushed the mpijob-controller branch from e3f5c67 to bed51ad Compare February 17, 2023 21:13

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 17, 2023

mimowo force-pushed the mpijob-controller branch from bed51ad to 7b46172 Compare February 20, 2023 07:44

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 20, 2023

mimowo marked this pull request as ready for review February 20, 2023 08:05

mimowo force-pushed the mpijob-controller branch 4 times, most recently from 2f8b14b to c4db71a Compare February 21, 2023 11:34

k8s-ci-robot assigned tenzen-y Mar 1, 2023

tenzen-y reviewed Mar 1, 2023

View reviewed changes

mimowo force-pushed the mpijob-controller branch from 2eec0fd to e78db60 Compare March 1, 2023 17:37

mimowo force-pushed the mpijob-controller branch from e78db60 to facd7e2 Compare March 2, 2023 06:10

k8s-ci-robot assigned kerthcet Mar 2, 2023

mimowo force-pushed the mpijob-controller branch 2 times, most recently from b400105 to 015e75f Compare March 2, 2023 08:25

kerthcet reviewed Mar 3, 2023

View reviewed changes

mimowo force-pushed the mpijob-controller branch 2 times, most recently from 82b8f32 to 4dc53e4 Compare March 3, 2023 12:06

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

Add MPIJob controller

2d435c8

mimowo force-pushed the mpijob-controller branch from 4dc53e4 to 2d435c8 Compare March 3, 2023 14:44

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

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

k8s-ci-robot merged commit 6a956f8 into kubernetes-sigs:main Mar 3, 2023

terrytangyuan reviewed Mar 3, 2023

View reviewed changes

This was referenced Mar 13, 2023

Commonize reconciler code by generic Job reconciler #627

Merged

Reorganize directory structure for jobs and the framework #631

Merged

mimowo deleted the mpijob-controller branch March 18, 2023 18:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MPIJob controller #578

Add MPIJob controller #578

mimowo commented Feb 16, 2023 •

edited

Loading

k8s-ci-robot commented Feb 16, 2023

netlify bot commented Feb 16, 2023 •

edited

Loading

tenzen-y commented Feb 16, 2023

mimowo commented Feb 16, 2023

tenzen-y left a comment

tenzen-y Mar 1, 2023

tenzen-y Mar 1, 2023

alculquicondor Mar 1, 2023

tenzen-y Mar 1, 2023 •

edited

Loading

alculquicondor Mar 1, 2023

tenzen-y Mar 1, 2023

mimowo Mar 2, 2023

tenzen-y left a comment

mimowo commented Mar 1, 2023

tenzen-y commented Mar 2, 2023

mimowo commented Mar 2, 2023

kerthcet commented Mar 2, 2023

kerthcet Mar 3, 2023

mimowo Mar 3, 2023

kerthcet Mar 3, 2023

mimowo Mar 3, 2023

mimowo Mar 3, 2023

alculquicondor Mar 3, 2023

kerthcet Mar 4, 2023

kerthcet Mar 3, 2023

mimowo Mar 3, 2023

kerthcet Mar 3, 2023 •

edited

Loading

mimowo commented Mar 3, 2023

alculquicondor commented Mar 3, 2023

alculquicondor commented Mar 3, 2023

mimowo commented Mar 3, 2023

terrytangyuan left a comment

kerthcet commented Mar 4, 2023

Add MPIJob controller #578

Add MPIJob controller #578

Conversation

mimowo commented Feb 16, 2023 • edited 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:

k8s-ci-robot commented Feb 16, 2023

netlify bot commented Feb 16, 2023 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

tenzen-y commented Feb 16, 2023

mimowo commented Feb 16, 2023

tenzen-y left a 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 Mar 1, 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

tenzen-y left a comment

Choose a reason for hiding this comment

mimowo commented Mar 1, 2023

tenzen-y commented Mar 2, 2023

mimowo commented Mar 2, 2023

kerthcet commented Mar 2, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kerthcet Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

mimowo commented Mar 3, 2023

alculquicondor commented Mar 3, 2023

alculquicondor commented Mar 3, 2023

mimowo commented Mar 3, 2023

terrytangyuan left a comment

Choose a reason for hiding this comment

kerthcet commented Mar 4, 2023

mimowo commented Feb 16, 2023 •

edited

Loading

netlify bot commented Feb 16, 2023 •

edited

Loading

tenzen-y Mar 1, 2023 •

edited

Loading

kerthcet Mar 3, 2023 •

edited

Loading