[Feature] Support Kubeflow MPIJob in MultiKueue #2880

mszadkow · 2024-08-23T11:34:49Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

The PR introduces a new MultiKueue adapter to handle MPIJob (Kubeflow).
We want to extend MultiKueue capabilities to satisfy the needs of early adopters.

Which issue(s) this PR fixes:

Fixes #2552

Special notes for your reviewer:

Does this PR introduce a user-facing change?

MultiKueue: Support for the Kubeflow MPIJob

k8s-ci-robot · 2024-08-23T11:34:52Z

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 · 2024-08-23T11:35:06Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`8daa5f7`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/66e1f6c187261800080eb51e

mszadkow · 2024-08-23T11:35:28Z

/ok-to-test

mszadkow · 2024-08-23T13:21:00Z

/retest-required

mszadkow · 2024-08-27T07:42:49Z

/retest-required

mbobrovskyi · 2024-08-27T08:08:41Z

/test pull-kueue-test-integration-main

Due to #2901.

mszadkow · 2024-08-27T09:33:47Z

/retest-required

mszadkow · 2024-08-27T11:01:27Z

/retest-required

mszadkow · 2024-08-27T11:12:27Z

/retest

mszadkow · 2024-08-27T12:25:25Z

/retest

mszadkow · 2024-08-27T13:04:13Z

/retest

Modify training-operator setup to be able to work along side mpi-operator

due to consolidation of MultiKueue adapters for the KFJobs

mbobrovskyi

Overall LGTM. Only this warning left:

# Warning: 'patchesStrategicMerge' is deprecated. Please use 'patches' instead. Run 'kustomize edit fix' to update your Kustomization automatically.

But I think we can fix it on follow-up.

Thanks!

hack/multikueue-e2e-test.sh

alculquicondor · 2024-09-06T16:09:14Z

Makefile-deps.mk

 	mkdir -p $(EXTERNAL_CRDS_DIR)/training-operator/
-	cp -f $(KF_TRAINING_ROOT)/manifests/base/crds/* $(EXTERNAL_CRDS_DIR)/training-operator/
+	cp -prf $(KF_TRAINING_ROOT)/manifests/* $(EXTERNAL_CRDS_DIR)/training-operator/
+	## Removing kubeflow.org_mpijobs.yaml is required as the version of MPIJob is conflicting between training-operator and mpi-operator - in integration tests.


Thanks for the details!

It makes me think that we need a note on this page https://kueue.sigs.k8s.io/docs/tasks/run/kubeflow/mpijobs/ explaining that you need to disable the MPI from training-operator if using both.

hack/e2e-common.sh

hack/multikueue-e2e-test.sh

pkg/controller/jobframework/validation.go

pkg/controller/jobs/mpijob/mpijob_multikueue_adapter.go

pkg/util/testingjobs/mpijob/wrappers_mpijob.go

alculquicondor · 2024-09-06T17:05:13Z

test/e2e/config/multikueue/manager/namespace.yaml

+apiVersion: v1
+kind: Namespace
+metadata:
+  name: kubeflow


Why do we need this namespace? Are the tests creating any objects in it?

test/integration/multikueue/multikueue_test.go

mbobrovskyi · 2024-09-11T06:52:56Z

hack/multikueue-e2e-test.sh

+    # 2. Training-operator deployment is modified to enable all kubeflow jobs except for mpi -  https://github.com/kubeflow/training-operator/issues/1777
+
+    # Modify the `newTag` for the `kubeflow/training-operator` to use the one training-operator version
+    $YQ eval '(.images[] | select(.name == "kubeflow/training-operator").newTag) = env(KUBEFLOW_IMAGE_VERSION)' -i "$KUBEFLOW_MANIFEST_MANAGER/kustomization.yaml"


Are you reverting these changes after testing?

Like here:

kueue/hack/multikueue-e2e-test.sh

Line 109 in 467b4ee

(cd config/components/manager && $KUSTOMIZE edit set image controller="$IMAGE_TAG")

kueue/hack/e2e-common.sh

Lines 97 to 99 in 467b4ee

function restore_managers_image {

(cd config/components/manager && $KUSTOMIZE edit set image controller="$INITIAL_IMAGE")

}

If not, users may accidentally commit unnecessary changes to this file.

I don't, good point I will add the restore

isn't this file outside of the tracked files?

No. It's on the git and yq will edit this file.

mszadkow · 2024-09-11T20:19:42Z

/retest

alculquicondor

/approve

k8s-ci-robot · 2024-09-11T20:24:50Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

alculquicondor · 2024-09-11T20:24:56Z

Leaving LGTM to @mbobrovskyi

mbobrovskyi · 2024-09-12T02:02:26Z

/lgtm

Thanks!

k8s-ci-robot · 2024-09-12T02:02:31Z

LGTM label has been added.

Git tree hash: e6c09ab9b855734fcb0824b61368ab7fe86456f5

mbobrovskyi · 2024-09-12T02:39:16Z

/cherry-pick website

k8s-infra-cherrypick-robot · 2024-09-12T02:39:54Z

@mbobrovskyi: #2880 failed to apply on top of branch "website":

Applying: Introduce MultiKueue adapter for MPIJob
Applying: Add MPIJobs integration tests
Using index info to reconstruct a base tree...
M	test/integration/multikueue/multikueue_test.go
M	test/integration/multikueue/suite_test.go
Falling back to patching base and 3-way merge...
Auto-merging test/integration/multikueue/suite_test.go
CONFLICT (content): Merge conflict in test/integration/multikueue/suite_test.go
Auto-merging test/integration/multikueue/multikueue_test.go
CONFLICT (content): Merge conflict in test/integration/multikueue/multikueue_test.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 Add MPIJobs integration tests
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick website

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.

mbobrovskyi · 2024-09-12T07:06:19Z

@mszadkow IIUC we have changes on site. Could you please prepare cherry-pick to website branch?

tenzen-y · 2024-09-13T11:52:36Z

@mszadkow IIUC we have changes on site. Could you please prepare cherry-pick to website branch?

We should not merge this PR to the website branch since this feature has not been released yet.

* Introduce MultiKueue adapter for MPIJob * Add MPIJobs integration tests * Update access rights for mpijob * Remove v1 MPIJob yaml from training-operator dep-crds * Update MPIJob version to v2beta1 * Introduce mpi-operator to multikueue e2e tests Modify training-operator setup to be able to work along side mpi-operator * Reduce the amount of KFJob e2e multikueue tests due to consolidation of MultiKueue adapters for the KFJobs * Add e2e Multikueue tests for MPIJob * Apply suggestions from verify * Use one makefile target to prep kubeflow traiing-operator manifest and crds * Apply code review changes * Yet another small fix * Rework after code review * move modifications of training-operator deployment and crds to kustomize * Another rework after code review * Cleanup after manager kustomize modification

k8s-ci-robot requested review from alculquicondor and denkensk August 23, 2024 11:34

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 23, 2024

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Aug 23, 2024

mszadkow force-pushed the feature/support-kubeflow-mpijob-in-mk branch from 1182083 to 5537a91 Compare August 23, 2024 13:20

mszadkow force-pushed the feature/support-kubeflow-mpijob-in-mk branch from bfebd01 to e231672 Compare August 27, 2024 10:50

mszadkow force-pushed the feature/support-kubeflow-mpijob-in-mk branch from b27cba7 to 11f84a7 Compare August 27, 2024 12:23

mszadkow force-pushed the feature/support-kubeflow-mpijob-in-mk branch from 4af4abb to 25c5547 Compare August 27, 2024 13:03

mszadkow added 7 commits August 27, 2024 17:24

Introduce MultiKueue adapter for MPIJob

1962437

Add MPIJobs integration tests

422ae06

Update access rights for mpijob

e10f9f7

Remove v1 MPIJob yaml from training-operator dep-crds

edeeb9c

Update MPIJob version to v2beta1

2ae47f5

Introduce mpi-operator to multikueue e2e tests

f4b2402

Modify training-operator setup to be able to work along side mpi-operator

Reduce the amount of KFJob e2e multikueue tests

acb28e2

due to consolidation of MultiKueue adapters for the KFJobs

mbobrovskyi reviewed Sep 6, 2024

View reviewed changes

hack/multikueue-e2e-test.sh Outdated Show resolved Hide resolved

alculquicondor reviewed Sep 6, 2024

View reviewed changes

Another rework after code review

963e44a

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 Sep 9, 2024

mbobrovskyi reviewed Sep 11, 2024

View reviewed changes

mszadkow force-pushed the feature/support-kubeflow-mpijob-in-mk branch from 10f0399 to 37f9a07 Compare September 11, 2024 19:53

Cleanup after manager kustomize modification

8daa5f7

mszadkow force-pushed the feature/support-kubeflow-mpijob-in-mk branch from 37f9a07 to 8daa5f7 Compare September 11, 2024 19:59

alculquicondor reviewed Sep 11, 2024

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 11, 2024

k8s-ci-robot assigned mbobrovskyi Sep 12, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 12, 2024

k8s-ci-robot merged commit fde44ba into kubernetes-sigs:main Sep 12, 2024
16 checks passed

k8s-ci-robot added this to the v0.9 milestone Sep 12, 2024

mbobrovskyi deleted the feature/support-kubeflow-mpijob-in-mk branch September 12, 2024 02:03

mbobrovskyi mentioned this pull request Sep 12, 2024

Fix Warning: 'patchesStrategicMerge' is deprecated. #3027

Merged

mbobrovskyi mentioned this pull request Sep 12, 2024

Flaky Test: MultiKueue when Creating a multikueue admission check Should run a MPIJob on worker if admitted. #3028

Closed

mbobrovskyi mentioned this pull request Sep 26, 2024

Update Kubeflow image tag #2709

Closed

tenzen-y mentioned this pull request Sep 27, 2024

Add mbobrovskyi to the top level reviewer #3156

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Support Kubeflow MPIJob in MultiKueue #2880

[Feature] Support Kubeflow MPIJob in MultiKueue #2880

mszadkow commented Aug 23, 2024

k8s-ci-robot commented Aug 23, 2024

netlify bot commented Aug 23, 2024 •

edited

Loading

mszadkow commented Aug 23, 2024

mszadkow commented Aug 23, 2024

mszadkow commented Aug 27, 2024

mbobrovskyi commented Aug 27, 2024

mszadkow commented Aug 27, 2024

mszadkow commented Aug 27, 2024

mszadkow commented Aug 27, 2024

mszadkow commented Aug 27, 2024

mszadkow commented Aug 27, 2024

mbobrovskyi left a comment

alculquicondor Sep 6, 2024

alculquicondor Sep 6, 2024

mbobrovskyi Sep 11, 2024 •

edited

Loading

mszadkow Sep 11, 2024

alculquicondor Sep 11, 2024

mbobrovskyi Sep 12, 2024 •

edited

Loading

mszadkow commented Sep 11, 2024

alculquicondor left a comment

k8s-ci-robot commented Sep 11, 2024

alculquicondor commented Sep 11, 2024

mbobrovskyi commented Sep 12, 2024

k8s-ci-robot commented Sep 12, 2024

mbobrovskyi commented Sep 12, 2024

k8s-infra-cherrypick-robot commented Sep 12, 2024

mbobrovskyi commented Sep 12, 2024

tenzen-y commented Sep 13, 2024

	function restore_managers_image {
	(cd config/components/manager && $KUSTOMIZE edit set image controller="$INITIAL_IMAGE")
	}

[Feature] Support Kubeflow MPIJob in MultiKueue #2880

[Feature] Support Kubeflow MPIJob in MultiKueue #2880

Conversation

mszadkow commented Aug 23, 2024

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?

k8s-ci-robot commented Aug 23, 2024

netlify bot commented Aug 23, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

mszadkow commented Aug 23, 2024

mszadkow commented Aug 23, 2024

mszadkow commented Aug 27, 2024

mbobrovskyi commented Aug 27, 2024

mszadkow commented Aug 27, 2024

mszadkow commented Aug 27, 2024

mszadkow commented Aug 27, 2024

mszadkow commented Aug 27, 2024

mszadkow commented Aug 27, 2024

mbobrovskyi left a comment

Choose a reason for hiding this comment

alculquicondor Sep 6, 2024

Choose a reason for hiding this comment

alculquicondor Sep 6, 2024

Choose a reason for hiding this comment

mbobrovskyi Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

mszadkow Sep 11, 2024

Choose a reason for hiding this comment

alculquicondor Sep 11, 2024

Choose a reason for hiding this comment

mbobrovskyi Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

mszadkow commented Sep 11, 2024

alculquicondor left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 11, 2024

alculquicondor commented Sep 11, 2024

mbobrovskyi commented Sep 12, 2024

k8s-ci-robot commented Sep 12, 2024

mbobrovskyi commented Sep 12, 2024

k8s-infra-cherrypick-robot commented Sep 12, 2024

mbobrovskyi commented Sep 12, 2024

tenzen-y commented Sep 13, 2024

netlify bot commented Aug 23, 2024 •

edited

Loading

mbobrovskyi Sep 11, 2024 •

edited

Loading

mbobrovskyi Sep 12, 2024 •

edited

Loading