Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Feature] Kuberay RayJob MultiKueue adapter #3892

Merged
merged 3 commits into from
Jan 17, 2025
Merged

[Feature] Kuberay RayJob MultiKueue adapter #3892

merged 3 commits into from
Jan 17, 2025

Conversation

mszadkow
Copy link
Contributor

@mszadkow mszadkow commented Dec 19, 2024 •
edited
Loading

What type of PR is this?

/kind feature

What this PR does / why we need it:

We want to be able to run Kuberay workloads RayJob in MultiKueue.
RayCluster will come as separate PR.

Testing:
Unit and multikueue e2e tests are provided.
Multikueue integration tests will be provided in separate PR - agreed with @mimowo.

Which issue(s) this PR fixes:

Relates to #3822

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Support RayJob in MultiKueue, assuming only Ray CRDs are installed on the management cluster.

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. 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 Dec 19, 2024
@k8s-ci-robot
Copy link
Contributor

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

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 19, 2024
@mszadkow
Copy link
Contributor Author

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 19, 2024
@netlify Netlify
Copy link

netlify bot commented Dec 19, 2024 •
edited
Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name Link
🔨 Latest commit 8f174bb
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/678900beccfe650008c92bb0

mszadkow
mszadkow commented Dec 19, 2024
View reviewed changes
Makefile-deps.mk Outdated Show resolved Hide resolved
mszadkow
mszadkow commented Dec 19, 2024
View reviewed changes
Makefile-test.mk
@@ -70,6 +70,7 @@ IMAGE_TAG ?= $(IMAGE_REPO):$(GIT_TAG)
JOBSET_VERSION = $(shell $(GO_CMD) list -m -f "{{.Version}}" sigs.k8s.io/jobset)
KUBEFLOW_VERSION = $(shell $(GO_CMD) list -m -f "{{.Version}}" github.com/kubeflow/training-operator)
KUBEFLOW_MPI_VERSION = $(shell $(GO_CMD) list -m -f "{{.Version}}" github.com/kubeflow/mpi-operator)
KUBERAY_VERSION = $(shell $(GO_CMD) list -m -f "{{.Version}}" github.com/ray-project/kuberay/ray-operator)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far it's the version without managedBy.
We don't have a ray-operator image that supports managedBy, that would have to be a custom one...
latest -> 1.2.2, which is the last release

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, hopefully 1.3 of KubeRay is released before 0.11 of Kueue and the actually released support will rely on managedBy. @andrewsykim do you know the the KubeRay release is planned?

mszadkow
mszadkow commented Dec 19, 2024
View reviewed changes
hack/e2e-common.sh
export KUBERAY_MANIFEST="${ROOT_DIR}/dep-crds/ray-operator/default/"
export KUBERAY_IMAGE=bitnami/kuberay-operator:${KUBERAY_VERSION/#v}
export KUBERAY_RAY_IMAGE=rayproject/ray:2.9.0
export KUBERAY_RAY_IMAGE_ARM=rayproject/ray:2.9.0-aarch64
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one is for us people working on macOS, it's vital for development, not so much for prod - I think it should stay

mszadkow
mszadkow commented Dec 19, 2024
View reviewed changes
hack/e2e-common.sh Outdated Show resolved Hide resolved
@mszadkow mszadkow force-pushed the feature/kuberay-multikueue-adapter branch from d27f12c to ad4df0d Compare December 19, 2024 11:03
mbobrovskyi
mbobrovskyi reviewed Dec 19, 2024
View reviewed changes
test/e2e/multikueue/e2e_test.go Outdated Show resolved Hide resolved
test/integration/multikueue/multikueue_test.go Outdated Show resolved Hide resolved
@mszadkow mszadkow force-pushed the feature/kuberay-multikueue-adapter branch from ad4df0d to b13ce9a Compare December 30, 2024 12:36
@k8s-ci-robot 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 Dec 30, 2024
@mszadkow mszadkow changed the title [Feature] Kuberay MultiKueue adapter [Feature] Kuberay RayJob MultiKueue adapter Dec 30, 2024
@mszadkow
Copy link
Contributor Author

/retest

@mszadkow mszadkow force-pushed the feature/kuberay-multikueue-adapter branch 2 times, most recently from d3974b4 to 3c3a0c7 Compare December 30, 2024 13:38
@mimowo
Copy link
Contributor

mimowo commented Jan 9, 2025

Actually, could you maybe split the PR into smaller parts:

  1. integration tests (which could be helpful to prove this fix works as expected: [WIP] Update Jobeframework's IsActive() for RayJob #3949 (IIUC these changes in test/integration are not MultiKueue-specific anyway)
  2. actual adapter + e2e tests (this PR)

@mszadkow mszadkow force-pushed the feature/kuberay-multikueue-adapter branch from 1801ddc to 106712f Compare January 9, 2025 16:21
@mszadkow
Copy link
Contributor Author

mszadkow commented Jan 9, 2025

Actually, could you maybe split the PR into smaller parts:

  1. integration tests (which could be helpful to prove this fix works as expected: [WIP] Update Jobeframework's IsActive() for RayJob #3949 (IIUC these changes in test/integration are not MultiKueue-specific anyway)
  2. actual adapter + e2e tests (this PR)

I guess it's no longer valid, as there is no fix, rather extension for MK

@mimowo
Copy link
Contributor

mimowo commented Jan 13, 2025

/release-note-edit

Support RayJob in MultiKueue, assuming only Ray CRDs are installed on the management cluster.

We may revisit the release note when the support relies on managedBy.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jan 13, 2025
@mimowo
Copy link
Contributor

mimowo commented Jan 13, 2025

@mszadkow do you know why pull-kueue-test-integration-main is failing?

@mszadkow
Copy link
Contributor Author

@mszadkow do you know why pull-kueue-test-integration-main is failing?

It won't anymore, I have reverted one change to get logs for the fail - #3892 (comment)

@mszadkow mszadkow force-pushed the feature/kuberay-multikueue-adapter branch from d46ed43 to 3cfcc59 Compare January 13, 2025 13:47
mimowo
mimowo reviewed Jan 13, 2025
View reviewed changes
Makefile-test.mk Outdated Show resolved Hide resolved
@mszadkow mszadkow force-pushed the feature/kuberay-multikueue-adapter branch from 3cfcc59 to 11791fb Compare January 14, 2025 14:37
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 15, 2025
@mszadkow mszadkow force-pushed the feature/kuberay-multikueue-adapter branch from 11791fb to 4d84973 Compare January 16, 2025 11:17
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 16, 2025
@mszadkow mszadkow force-pushed the feature/kuberay-multikueue-adapter branch 2 times, most recently from 9f36f9d to 362733f Compare January 16, 2025 11:54
@mszadkow mszadkow force-pushed the feature/kuberay-multikueue-adapter branch from 362733f to 8f174bb Compare January 16, 2025 12:51
mimowo
mimowo reviewed Jan 17, 2025
View reviewed changes
Copy link
Contributor

@mimowo mimowo left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve
Thanks!
Please follow up with integration tests. For now I believe the e2e tests + unit tests are providing enough coverage to merge, but going forward I want to make sure integration tests are added, so that we can test some bugfixes or features without extending the heavy e2e tests.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 17, 2025
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: e24a7a5c8fb7053c6d7dd5c6c84ccba7bcb3821c

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mimowo, 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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 17, 2025
@k8s-ci-robot k8s-ci-robot merged commit 097262a into kubernetes-sigs:main Jan 17, 2025
17 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v0.11 milestone Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mimowo mimowo mimowo left review comments

@PBundyra PBundyra Awaiting requested review from PBundyra

@mbobrovskyi mbobrovskyi Awaiting requested review from mbobrovskyi

Assignees

@mimowo mimowo

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v0.11
Development

Successfully merging this pull request may close these issues.
4 participants
@mszadkow @k8s-ci-robot @mimowo @mbobrovskyi