RayCluster integration with Kueue #1520

vicentefb · 2023-12-27T05:27:34Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Support RayCluster as a queue-able workload in Kueue since there are many use-cases and existing workloads that depend on long-lived RayClusters.

Which issue(s) this PR fixes:

Fixes #1272

Special notes for your reviewer:

This implementation is using master's version of kuberay and using v1 api version instead of v1alpha1 because it's using the new suspend API (ray-project/kuberay#1667) for RayCluster

Does this PR introduce a user-facing change?

Support RayCluster as a queue-able workload in Kueue

netlify · 2023-12-27T05:27:40Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`c9080e3`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/65b3f6f0609d1d00082fe61c

k8s-ci-robot · 2023-12-27T05:27:44Z

Hi @vicentefb. 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/test-infra repository.

vicentefb · 2023-12-27T05:28:10Z

Will squash commits once final review is given.

tenzen-y · 2023-12-27T06:47:19Z

/ok-to-test

pkg/controller/jobframework/reconciler.go

pkg/controller/jobs/raycluster/raycluster_webhook.go

test/integration/controller/jobs/raycluster/raycluster_controller_test.go

andrewsykim · 2023-12-28T15:06:09Z

/hold

Holding until there's a kuberay release that has ray-project/kuberay#1711

vicentefb · 2023-12-28T17:54:22Z

/retest

astefanutti · 2024-01-25T17:32:59Z

Did you mean the feature or the test?
Node selectors are fundamental to the way flavors work, so we shouldn't miss the feature. I would be cautious about leaving the unit test for later.

I meant the feature. But I agree having the unit test and the feature in the upcoming v0.6.0 release would be ideal.

alculquicondor · 2024-01-25T17:52:21Z

So it doesn't currently work? I'm rather opposed to merging without it.

/hold

alculquicondor · 2024-01-25T18:05:21Z

test/integration/controller/jobs/raycluster/raycluster_controller_test.go

+		gomega.Expect(len(createdJob.Spec.HeadGroupSpec.Template.Spec.NodeSelector)).Should(gomega.Equal(1))
+		gomega.Expect(createdJob.Spec.HeadGroupSpec.Template.Spec.NodeSelector[instanceKey]).Should(gomega.Equal(onDemandFlavor.Name))
+		gomega.Expect(len(createdJob.Spec.WorkerGroupSpecs[0].Template.Spec.NodeSelector)).Should(gomega.Equal(1))
+		gomega.Expect(createdJob.Spec.WorkerGroupSpecs[0].Template.Spec.NodeSelector[instanceKey]).Should(gomega.Equal(spotFlavor.Name))


ok, it is tested here. It would be useful to have this in unit tests, but it's ok to leave for a follow up

cc @astefanutti

/hold cancel.

astefanutti · 2024-01-25T18:25:49Z

pkg/controller/jobs/raycluster/raycluster_controller_test.go

+	}{
+		"when workload is admitted, cluster is unsuspended": {
+			job: *baseJobWrapper.Clone().
+				NodeSelectorHeadGroup("provisioning", "spot").


Should this should be removed from the test input, as it's expected to be added during the reconciliation?

Also a flavor needs to be defined, because that's where the selectors are obtained from.

pkg/controller/jobs/raycluster/raycluster_controller_test.go

alculquicondor · 2024-01-25T18:33:21Z

pkg/controller/jobs/raycluster/raycluster_controller_test.go

+	}{
+		"when workload is admitted, cluster is unsuspended": {
+			job: *baseJobWrapper.Clone().
+				NodeSelectorHeadGroup("provisioning", "spot").


Also a flavor needs to be defined, because that's where the selectors are obtained from.

pkg/controller/jobs/raycluster/raycluster_controller_test.go

astefanutti

LGTM

k8s-ci-robot · 2024-01-26T09:17:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, astefanutti, vicentefb

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-01-26T16:20:00Z

/lgtm

based on #1520 (review)

k8s-ci-robot · 2024-01-26T16:20:08Z

LGTM label has been added.

Git tree hash: 4ec47e5614815d8b5ed50a02d7a1e3b40c34ba50

vicentefb · 2024-01-26T17:36:29Z

/hold cancel

alculquicondor · 2024-01-26T18:06:35Z

The periodic E2E tests are healthy... so could it be that this PR is breaking them?

/test pull-kueue-test-e2e-main-1-26
/test pull-kueue-test-e2e-main-1-27
/test pull-kueue-test-e2e-main-1-28
/test pull-kueue-test-e2e-main-1-29

updated kubebuilder markers but still workload isnt created updating branch update unit tests working ? changes ray cluster admitted as a workload and running, it has a lot of debug log lines that need to be removed added comma fixed helper method, need to implemente tests changes v1alpha1 to v1 but still workload isnt created pods are not getting suspended :( charts WIP admit RayCluster as kueuable workload updated kubebuilder markers but still workload isnt created updating branch update unit tests working ? changes ray cluster admitted as a workload and running, it has a lot of debug log lines that need to be removed fixed helper method, need to implemente tests changes v1alpha1 to v1 but still workload isnt created updated wrappers not all tests are passing but still working on them debugging podReady test update updated controller and webhook tests, now working updated raycluster webhook test added ray cluster sample yaml updated go modules since this is using kuberay masters version removing diffs from reconciler file removing diffs from reconciler file removing diffs from reconciler file updated role yaml file added TODO comment for autoscaler updated raycluster controller test added scheme for v1 inside register file update rayjob import to reference v1 otherwise tests are not passing in PR changed the order of jobs list updated pod controller api version to be v1 instead of v1alpha1 update go files fixed pull kueue test reverted changes made to rayjob import library version fixed rayjob tests WIP admit RayCluster as kueuable workload updated kubebuilder markers but still workload isnt created updating branch update unit tests working ? changes ray cluster admitted as a workload and running, it has a lot of debug log lines that need to be removed added comma fixed helper method, need to implemente tests changes v1alpha1 to v1 but still workload isnt created pods are not getting suspended :( charts WIP admit RayCluster as kueuable workload updated kubebuilder markers but still workload isnt created updating branch update unit tests working ? changes ray cluster admitted as a workload and running, it has a lot of debug log lines that need to be removed fixed helper method, need to implemente tests changes v1alpha1 to v1 but still workload isnt created updated wrappers not all tests are passing but still working on them debugging podReady test update updated controller and webhook tests, now working updated raycluster webhook test updated go modules since this is using kuberay masters version removing diffs from reconciler file removing diffs from reconciler file removing diffs from reconciler file updated role yaml file added TODO comment for autoscaler updated raycluster controller test added scheme for v1 inside register file update rayjob import to reference v1 otherwise tests are not passing in PR changed the order of jobs list updated pod controller api version to be v1 instead of v1alpha1 update go files fixed pull kueue test reverted changes made to rayjob import library version fixed rayjob tests updated example raycluster nit updated ray cluster controller unit test and wrapper updated tests and charts updated ownerReference for rayJob and rayCluster removed extra configuration for pods and duplicated text generated by script added third argument for reconciler

reverted git tag change moved the sample yaml file to a different branch addressed comments and used generalised method call in reconciler to check ownership updated new reconciler variable addressed comments nit removed register changes revert changes to go dependencies to test something updated go files nit added schema nit added files generated by make verify update charts test test updated register test fixed test added builder n update fix go modules fixed test nit added case for ray cluster completion nit debugging added test for coverage updated the restore node test nit addressed comments update added scheme for rayv1 in tests nit updated test to inject node selector with flavor defined nit nit

alculquicondor · 2024-01-26T18:20:16Z

tests are definitely flaky #1658

alculquicondor · 2024-01-26T18:44:49Z

/lgtm

k8s-ci-robot · 2024-01-26T18:44:55Z

LGTM label has been added.

Git tree hash: 9882d57638bb7418ba59d98bf166f1829eff6ef9

k8s-ci-robot requested review from kerthcet and trasc December 27, 2023 05:27

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

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 27, 2023

vicentefb closed this Dec 27, 2023

vicentefb reopened this Dec 27, 2023

vicentefb force-pushed the KueueRayCluster branch from 32d1e36 to 1fb4a7c Compare December 27, 2023 05:31

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 27, 2023

andrewsykim reviewed Dec 27, 2023

View reviewed changes

vicentefb force-pushed the KueueRayCluster branch 2 times, most recently from c59763f to 79b174a Compare December 27, 2023 20:16

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 28, 2023

vicentefb force-pushed the KueueRayCluster branch 3 times, most recently from 673c99c to 0765c64 Compare January 2, 2024 20:57

vicentefb changed the title ~~[WIP] RayCluster integration with Kueue~~ RayCluster integration with Kueue Jan 3, 2024

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

vicentefb requested a review from andrewsykim January 3, 2024 15:59

vicentefb force-pushed the KueueRayCluster branch from 13b200f to 24e1dd5 Compare January 3, 2024 19:33

alculquicondor reviewed Jan 25, 2024

View reviewed changes

astefanutti reviewed Jan 25, 2024

View reviewed changes

alculquicondor reviewed Jan 25, 2024

View reviewed changes

vicentefb force-pushed the KueueRayCluster branch 4 times, most recently from f8c09fb to 916328e Compare January 25, 2024 22:01

vicentefb requested a review from astefanutti January 26, 2024 07:06

astefanutti approved these changes Jan 26, 2024

View reviewed changes

k8s-ci-robot assigned alculquicondor Jan 26, 2024

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

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

vicentefb added 2 commits January 26, 2024 18:10

vicentefb force-pushed the KueueRayCluster branch from 916328e to c9080e3 Compare January 26, 2024 18:16

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 26, 2024

k8s-ci-robot requested a review from alculquicondor January 26, 2024 18:16

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

k8s-ci-robot merged commit 3b37fbf into kubernetes-sigs:main Jan 26, 2024
14 checks passed

k8s-ci-robot added this to the v0.6 milestone Jan 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RayCluster integration with Kueue #1520

RayCluster integration with Kueue #1520

vicentefb commented Dec 27, 2023

netlify bot commented Dec 27, 2023 •

edited

Loading

k8s-ci-robot commented Dec 27, 2023

vicentefb commented Dec 27, 2023

tenzen-y commented Dec 27, 2023

andrewsykim commented Dec 28, 2023

vicentefb commented Dec 28, 2023

astefanutti commented Jan 25, 2024

alculquicondor commented Jan 25, 2024

alculquicondor Jan 25, 2024

astefanutti Jan 25, 2024

alculquicondor Jan 25, 2024

alculquicondor Jan 25, 2024

astefanutti left a comment

k8s-ci-robot commented Jan 26, 2024

alculquicondor commented Jan 26, 2024

k8s-ci-robot commented Jan 26, 2024

vicentefb commented Jan 26, 2024

alculquicondor commented Jan 26, 2024

alculquicondor commented Jan 26, 2024

alculquicondor commented Jan 26, 2024

k8s-ci-robot commented Jan 26, 2024

RayCluster integration with Kueue #1520

RayCluster integration with Kueue #1520

Conversation

vicentefb commented Dec 27, 2023

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?

netlify bot commented Dec 27, 2023 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

k8s-ci-robot commented Dec 27, 2023

vicentefb commented Dec 27, 2023

tenzen-y commented Dec 27, 2023

andrewsykim commented Dec 28, 2023

vicentefb commented Dec 28, 2023

astefanutti commented Jan 25, 2024

alculquicondor commented Jan 25, 2024

alculquicondor Jan 25, 2024

Choose a reason for hiding this comment

astefanutti Jan 25, 2024

Choose a reason for hiding this comment

alculquicondor Jan 25, 2024

Choose a reason for hiding this comment

alculquicondor Jan 25, 2024

Choose a reason for hiding this comment

astefanutti left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 26, 2024

alculquicondor commented Jan 26, 2024

k8s-ci-robot commented Jan 26, 2024

vicentefb commented Jan 26, 2024

alculquicondor commented Jan 26, 2024

alculquicondor commented Jan 26, 2024

alculquicondor commented Jan 26, 2024

k8s-ci-robot commented Jan 26, 2024

netlify bot commented Dec 27, 2023 •

edited

Loading