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

Remove default option for batch scheduler name #2371

kevin85421 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

yangwwei commented Sep 10, 2024

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

kevin85421
kevin85421 reviewed Sep 10, 2024
View reviewed changes
ray-operator/main.go Outdated
"(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

@kevin85421 kevin85421 Sep 10, 2024

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

@yangwwei yangwwei Sep 11, 2024

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
flag.StringVar(&logStdoutEncoder, "log-stdout-encoder", "json",
"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

@andrewsykim andrewsykim Sep 11, 2024

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

@yangwwei yangwwei Sep 11, 2024

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
# Set the customized scheduler name, supported values are "volcano" or "yunikorn", do not set
# "batchScheduler.enabled=true" at the same time as it will override this option.
name: default
name: ""
Copy link
Member

@andrewsykim andrewsykim Sep 11, 2024

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

@yangwwei yangwwei Sep 11, 2024

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

@yangwwei yangwwei Sep 11, 2024

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

kevin85421 commented Sep 11, 2024

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