Skip to content

Remove default option for batch scheduler name #2371

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
Merged
merged 4 commits into from
Sep 13, 2024
Merged

Remove default option for batch scheduler name #2371

merged 4 commits into from
Sep 13, 2024

Conversation

yangwwei
Copy link
Contributor

@yangwwei yangwwei commented Sep 10, 2024 •
edited
Loading

Per review comment here: #2300 (comment). Remove the redundant "default" option for the batch scheduler name.

How this was tested?

  • UT (this PR added UT coverage for various cases)
  • Local tests
// normal start up
./ray-operator/bin/manager -leader-election-namespace default -use-kubernetes-proxy

// batchscheduler.enabled=true
// this enables volcano integration
./ray-operator/bin/manager -enable-batch-scheduler -leader-election-namespace default -use-kubernetes-proxy

// batchscheduler.name=volcao
// this enables volcano integration
./ray-operator/bin/manager -batch-scheduler volcano -leader-election-namespace default -use-kubernetes-proxy
{"level":"info","ts":"2024-09-10T16:18:31.104-0700","logger":"setup","msg":"Feature flag batch-scheduler is enabled","scheduler name":"volcano"}
...

// batchscheduler.name=yunikorn
// this enables yunikorn integration
./ray-operator/bin/manager -batch-scheduler yunikorn -leader-election-namespace default -use-kubernetes-proxy
{"level":"info","ts":"2024-09-10T16:18:54.893-0700","logger":"setup","msg":"Feature flag batch-scheduler is enabled","scheduler name":"yunikorn"}
...

@yangwwei yangwwei mentioned this pull request Sep 10, 2024
Merged
@yangwwei
Copy link
Contributor Author

@andrewsykim @kevin85421 pls help to review this follow up PR.

kevin85421
kevin85421 reviewed Sep 10, 2024
View reviewed changes
ray-operator/main.go Outdated
@@ -92,7 +92,7 @@ func main() {
"Encoder to use for logging stdout. Valid values are 'json' and 'console'. Defaults to 'json'")
flag.BoolVar(&enableBatchScheduler, "enable-batch-scheduler", false,
"(Deprecated) Enable batch scheduler. Currently is volcano, which supports gang scheduler policy.")
flag.StringVar(&batchScheduler, "batch-scheduler", "default",
flag.StringVar(&batchScheduler, "batch-scheduler", "",
"Batch scheduler name, supported values are default, volcano, yunikorn.")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"Batch scheduler name, supported values are default, volcano, yunikorn.")
"Batch scheduler name, supported values are volcano and yunikorn.")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Fixed.

@kevin85421 kevin85421 self-assigned this Sep 10, 2024
andrewsykim
andrewsykim reviewed Sep 11, 2024
View reviewed changes
ray-operator/main.go Outdated
@@ -92,7 +92,7 @@ func main() {
"Encoder to use for logging stdout. Valid values are 'json' and 'console'. Defaults to 'json'")
flag.BoolVar(&enableBatchScheduler, "enable-batch-scheduler", false,
"(Deprecated) Enable batch scheduler. Currently is volcano, which supports gang scheduler policy.")
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a note in this flag "Use --batch-scheduler instead"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Fixed.

andrewsykim
andrewsykim reviewed Sep 11, 2024
View reviewed changes
helm-chart/kuberay-operator/values.yaml
# "batchScheduler.enabled=true" at the same time as it will override this option.
name: default
name: ""
Copy link
Member

Choose a reason for hiding this comment

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

Should we update helm-chart/kuberay-operator/templates/deployment.yaml such that --batch-scheduler flag is not set if name is empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is already there:

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

Lines 60 to 67 in 0e1c248

{{- 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 -}}
.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also did some tests:

  1. Default install
helm install kuberay-operator ../helm-chart/kuberay-operator

the args:

   Args:
      --feature-gates=RayClusterStatusConditions=false
      --enable-leader-election=true
  1. Legacy option
helm install kuberay-operator --set batchScheduler.enabled=true ../helm-chart/kuberay-operator

the args in the deployment:

    Args:
      --feature-gates=RayClusterStatusConditions=false
      --enable-batch-scheduler
      --enable-leader-election=true
  1. New option
helm install kuberay-operator  --set batchScheduler.name=yunikorn ../helm-chart/kuberay-operator

the args

   Args:
      --feature-gates=RayClusterStatusConditions=false
      --batch-scheduler=yunikor
      --enable-leader-election=true

@yangwwei
Copy link
Contributor Author

yangwwei commented Sep 11, 2024 •
edited
Loading

Not sure what are the errors in the CI mean:

Error: This request has been automatically failed because it uses a deprecated version of actions/download-artifact: v2. Learn more: https://github.blog/changelog/2024-02-13-deprecation-notice-v1-and-v2-of-the-artifact-actions/

Seems like all PR recently failed with this.

@kevin85421
Copy link
Member

Seems like all PR recently failed with this.

@yangwwei could you rebase with the master branch? Thanks!

andrewsykim
andrewsykim reviewed Sep 11, 2024
View reviewed changes
Copy link
Member

@andrewsykim andrewsykim left a comment

Choose a reason for hiding this comment

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

LGTM

@kevin85421 kevin85421 merged commit 22cc61d into ray-project:master Sep 13, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewsykim andrewsykim andrewsykim left review comments

@kevin85421 kevin85421 kevin85421 approved these changes

Assignees

@kevin85421 kevin85421

Labels
1.3.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yangwwei @kevin85421 @andrewsykim