Add batch-scheduler option, deprecate enable-batch-scheduler option #2300

yangwwei · 2024-08-10T06:12:13Z

Summary

See proposal: https://github.com/ray-project/enhancements/blob/main/reps/2024-06-16-support-apache-yunikorn-scheduler.md.

For better batch scheduler integration, this PR adds another option batch-scheduler and user needs to explicitly provide a scheduler name, currently supported: volcano and yunikorn. The old flag enable-batch-scheduler will be deprecated but still be supported. Main changes:

Removed the global var: EnableBatchScheduler in raycluster_controller.go
Pass the configs to SchedulerManager, based on the configs, initiaize just one schedulerinterface.BatchScheduler and schedulerinterface.BatchSchedulerFactory, no need to keep a list, there will be just one scheduler support at runtime.
Simplify the logic in raycluster_controller.go, simply call scheduler manager to do scheduler related operations without checking the flag.

Test

Build the operator:

make -C ray-operator build

Test the old flag:

The old flag is marked as deprecated, prompts the message to ask using the new flag instead. But the flag is still supported.

 ./ray-operator/bin/manager -enable-batch-scheduler -leader-election-namespace default -use-kubernetes-proxy 

{"level":"info","ts":"2024-08-11T11:54:04.935-0700","logger":"setup","msg":"Feature flag enable-batch-scheduler is deprecated and will not be supported soon. Use batch-scheduler instead. "}
...

if PodGroup CRD is not installed, it will fail fast with the following error message:

panic: podGroup CRD is required to exist in current cluster. error: the server could not find the requested resource
goroutine 1 [running]:
github.com/ray-project/kuberay/ray-operator/controllers/ray.NewReconciler({0x105c2d858, _}, {_, _}, {{_, _, _}, {_, _, _}}, ...)
        /Users/weiweiyang/workspace/github/forks/kuberay/ray-operator/controllers/ray/raycluster_controller.go:114 +0x278

Test the new config

batch-scheduler=volcano

./ray-operator/bin/manager -batch-scheduler volcano -leader-election-namespace default -use-kubernetes-proxy

logs (some logs omitted). The log indicates "volcano" is used as the scheduler, and it loads the v1beta1.PodGroup resource, requires that CRD to be pre-installed.


{"level":"info","ts":"2024-08-11T11:56:31.688-0700","logger":"setup","msg":"Feature flag batch-scheduler is enabled","scheduler name":"volcano"}
...
{"level":"info","ts":"2024-08-11T11:58:34.756-0700","logger":"controllers.RayCluster","msg":"Starting EventSource","source":"kind source: *v1.RayCluster"}
{"level":"info","ts":"2024-08-11T11:58:34.756-0700","logger":"controllers.RayCluster","msg":"Starting EventSource","source":"kind source: *v1.Pod"}
{"level":"info","ts":"2024-08-11T11:58:34.756-0700","logger":"controllers.RayCluster","msg":"Starting EventSource","source":"kind source: *v1.Service"}
{"level":"info","ts":"2024-08-11T11:58:34.756-0700","logger":"controllers.RayCluster","msg":"Starting EventSource","source":"kind source: *v1beta1.PodGroup"}
...
{"level":"info","ts":"2024-08-11T11:58:34.757-0700","logger":"controllers.RayJob","msg":"Starting Controller"}
{"level":"info","ts":"2024-08-11T11:58:34.873-0700","logger":"controllers.RayCluster","msg":"Starting workers","worker count":1}
{"level":"info","ts":"2024-08-11T11:58:34.874-0700","logger":"controllers.RayService","msg":"Starting workers","worker count":1}
{"level":"info","ts":"2024-08-11T11:58:34.874-0700","logger":"controllers.RayJob","msg":"Starting workers","worker count":1}
...

Test new config

batch-scheduler=yunikorn

./ray-operator/bin/manager -batch-scheduler yunikorn -leader-election-namespace default -use-kubernetes-proxy

logs (some logs omitted), note, no podGroup (from Volcano) related resource loaded in the controller:

{"level":"info","ts":"2024-08-11T12:00:22.148-0700","logger":"setup","msg":"Feature flag batch-scheduler is enabled","scheduler name":"yunikorn"}
...
{"level":"info","ts":"2024-08-11T12:00:39.359-0700","logger":"controllers.RayCluster","msg":"Starting EventSource","source":"kind source: *v1.RayCluster"}
{"level":"info","ts":"2024-08-11T12:00:39.359-0700","logger":"controllers.RayCluster","msg":"Starting EventSource","source":"kind source: *v1.Pod"}
{"level":"info","ts":"2024-08-11T12:00:39.359-0700","logger":"controllers.RayCluster","msg":"Starting EventSource","source":"kind source: *v1.Service"}
..
{"level":"info","ts":"2024-08-11T12:00:39.360-0700","logger":"controllers.RayService","msg":"Starting Controller"}
{"level":"info","ts":"2024-08-11T12:00:39.463-0700","logger":"controllers.RayCluster","msg":"Starting workers","worker count":1}
{"level":"info","ts":"2024-08-11T12:00:39.464-0700","logger":"controllers.RayJob","msg":"Starting workers","worker count":1}
{"level":"info","ts":"2024-08-11T12:00:39.470-0700","logger":"controllers.RayService","msg":"Starting workers","worker count":1}

If use both old and new option (not supported)

./ray-operator/bin/manager -enable-batch-scheduler -batch-scheduler yunikorn -leader-election-namespace default -use-kubernetes-proxy

failed with the following error message:

{"level":"error","ts":"2024-08-11T12:02:04.914-0700","logger":"setup","msg":"do not use both options together: \"batch-scheduler\" and \"enable-batch-scheduler\".","error":"invalid configuration found","stacktrace":"main.exitOnError\n\t/Users/weiweiyang/workspace/github/forks/kuberay/ray-operator/main.go:271\nmain.main\n\t/Users/weiweiyang/workspace/github/forks/kuberay/ray-operator/main.go:165\nruntime.main\n\t/Users/weiweiyang/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.22.3.darwin-arm64/src/runtime/proc.go:271"}

yangwwei · 2024-08-11T19:06:38Z

@kevin85421 can you help to review this draft? I will start to add more UTs once the overall PR looks good to you. Thanks!

kevin85421 · 2024-08-12T03:46:30Z

There are a lot of CI failures. Would you mind fixing the errors? I will review after I release v1.2.0.

yangwwei · 2024-08-12T04:17:45Z

Got to know the problem, fixing the issues.

kevin85421

Sorry for the delay. The KubeRay v1.2.0 release is taking longer than I expected. I'm back to reviewing PRs.

kevin85421 · 2024-09-03T16:33:57Z

helm-chart/kuberay-operator/templates/deployment.yaml

@@ -60,6 +60,11 @@ spec:
            {{- if .Values.batchScheduler.enabled -}}
            {{- $argList = append $argList "--enable-batch-scheduler" -}}
            {{- end -}}
+            {{- if .Values.batchScheduler -}}


Maybe we can move {{- if .Values.batchScheduler -}} above. That is,

{{- if .Values.batchScheduler -}} {{- if .Values.batchScheduler.enabled -}} {{- $argList = append $argList "--enable-batch-scheduler" -}} {{- end -}} {{- if .Values.batchScheduler.name -}} {{- $argList = append $argList (printf "--batch-scheduler=%s" .Values.batchScheduler.name) -}} {{- end -}} {{- end -}}

Good suggestion. Changes made. Thanks!

kevin85421 · 2024-09-03T16:37:44Z

helm-chart/kuberay-operator/values.yaml

@@ -57,6 +57,7 @@ readinessProbe:

 batchScheduler:


Can you add some comments for batchScheduler to explain how to configure Volcano, Yunikorn, and the default scheduler?

Sure, added some comments. Will add some docs too in the coming PR.

kevin85421 · 2024-09-03T16:44:14Z

ray-operator/controllers/ray/raycluster_controller.go

+	// init the batch scheduler manager
+	schedulerMgr, err := batchscheduler.NewSchedulerManager(rayConfigs, mgr.GetConfig())
+	if err != nil {
+		// fail fast if the scheduler plugin is failed to init


Suggested change

// fail fast if the scheduler plugin is failed to init

// fail fast if the scheduler plugin fails to init

kevin85421 · 2024-09-03T16:44:46Z

ray-operator/controllers/ray/raycluster_controller.go

+	schedulerMgr, err := batchscheduler.NewSchedulerManager(rayConfigs, mgr.GetConfig())
+	if err != nil {
+		// fail fast if the scheduler plugin is failed to init
+		// prevent running the controller in vague state


Suggested change

// prevent running the controller in vague state

// prevent running the controller in an undefined state

kevin85421 · 2024-09-03T16:52:50Z

ray-operator/main.go

@@ -160,8 +160,19 @@ func main() {
 	if forcedClusterUpgrade {
 		setupLog.Info("Deprecated feature flag forced-cluster-upgrade is enabled, which has no effect.")
 	}
-	if ray.EnableBatchScheduler {
-		setupLog.Info("Feature flag enable-batch-scheduler is enabled.")
+	if config.EnableBatchScheduler {


How about creating a new function validateConfig and move L163 - L176 to the function instead?

Yeah, good idea. Changes made.

ray-operator/main.go

ray-operator/controllers/ray/raycluster_controller.go

kevin85421 · 2024-09-03T17:03:15Z

ray-operator/controllers/ray/raycluster_controller.go

@@ -51,7 +52,6 @@ type reconcileFunc func(context.Context, *rayv1.RayCluster) error

 var (
 	DefaultRequeueDuration = 2 * time.Second
-	EnableBatchScheduler   bool


Happy to see that we removed this global variable!

kevin85421 · 2024-09-03T17:24:26Z

ray-operator/controllers/ray/batchscheduler/schedulermanager.go

-	// no scheduler provided
-	return &schedulerinterface.DefaultBatchScheduler{}, nil
-}
+	batch.Lock()


why adding the lock here?

Double checked the code, seems like there is no risk of running into concurrent issues. Since this will be called in the reconcile logic, which is streamlined. I think we can remove the lock.

kevin85421 · 2024-09-03T17:32:10Z

cc @andrewsykim Would you mind reviewing this PR since it also updates the config API?

andrewsykim · 2024-09-09T04:06:45Z

Sorry I missed the review on this! My only comment would be that the "default" batch scheduler option seems redundant. It seems like that should just be "not set" instead

yangwwei · 2024-09-09T05:33:39Z

Sorry I missed the review on this! My only comment would be that the "default" batch scheduler option seems redundant. It seems like that should just be "not set" instead

Hi @andrewsykim just want to confirm. you mean we do not need a "batchScheduler.name=default" as the default value. By default, we do not set this option. If set, the only valid options are "volcano" and "yunikorn" for now. Which means in values.yaml, we will have a by default commented out option, something like this?

batchScheduler:

  # Deprecated. This option will be removed in the future.
  # Note, for backwards compatibility. When it sets to true, it enables volcano scheduler integration.
  enabled: false

  # Name of the scheduler, currently supported "volcano" and "yunikorn", do not set
  # "batchScheduler.enabled=true" at the same time as it will override this option.
  # name:  volcano | yunikorn

andrewsykim · 2024-09-09T14:09:14Z

Yes, I think that would be better. The "default" value is misleading because it implies there is default logic for batch scheduling, but the default implementation is a no-op right?

yangwwei · 2024-09-09T16:41:00Z

Yes, I think that would be better. The "default" value is misleading because it implies there is default logic for batch scheduling, but the default implementation is a no-op right?

Sounds good. I will submit a follow up PR to address this. Thanks @andrewsykim
BTW, what's the time you guys think is the best to remove the old option "batchScheduler.enabled", we can keep it for a few releases, but need to be clear about when it will be removed. Thanks!

yangwwei · 2024-09-10T23:20:39Z

@andrewsykim pls see #2371

Add batch-scheduler option, deprecate enable-batch-scheduler option

30ae0b4

yangwwei marked this pull request as draft August 10, 2024 06:12

kevin85421 self-assigned this Aug 12, 2024

kevin85421 added the 1.3.0 label Aug 12, 2024

minor comments update

f1b66d0

yangwwei added 5 commits August 12, 2024 19:37

Merge remote-tracking branch 'upstream/master'

cf70cef

update helm charts

5ae36e7

Fix UT

0607ab2

fix UT

c53926f

Merge remote-tracking branch 'upstream/master'

ff939df

yangwwei marked this pull request as ready for review August 21, 2024 20:13

kevin85421 reviewed Sep 3, 2024

View reviewed changes

yangwwei added 6 commits September 7, 2024 15:54

Merge branch 'master' into batchscheduler

07d9464

fix lint errors

4a72dd7

fix yaml file format

ae21671

Add UT for scheduler manager, validate init options

6be4012

Fix ut

12d9ee3

Minor comments fix

a231c11

kevin85421 approved these changes Sep 8, 2024

View reviewed changes

kevin85421 merged commit e1edb4c into ray-project:master Sep 8, 2024
27 checks passed

yangwwei deleted the batchscheduler branch September 8, 2024 07:13

yangwwei mentioned this pull request Sep 10, 2024

Remove default option for batch scheduler name #2371

Merged

andrewsykim mentioned this pull request Oct 11, 2024

[Bug] Kuberay can not start when enableBatchScheduler=true #2430

Open

2 tasks

	// fail fast if the scheduler plugin is failed to init
	// fail fast if the scheduler plugin fails to init

	// prevent running the controller in vague state
	// prevent running the controller in an undefined state

Add batch-scheduler option, deprecate enable-batch-scheduler option #2300

Add batch-scheduler option, deprecate enable-batch-scheduler option #2300

Uh oh!

Conversation

yangwwei commented Aug 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test

Uh oh!

yangwwei commented Aug 11, 2024

Uh oh!

kevin85421 commented Aug 12, 2024

Uh oh!

yangwwei commented Aug 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kevin85421 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevin85421 commented Sep 3, 2024

Uh oh!

Uh oh!

andrewsykim commented Sep 9, 2024

Uh oh!

yangwwei commented Sep 9, 2024

Uh oh!

andrewsykim commented Sep 9, 2024

Uh oh!

yangwwei commented Sep 9, 2024

Uh oh!

yangwwei commented Sep 10, 2024

Uh oh!

Uh oh!

yangwwei commented Aug 10, 2024 •

edited

Loading

yangwwei commented Aug 12, 2024 •

edited

Loading