-
Notifications
You must be signed in to change notification settings - Fork 579
[Feature] [scheduler-plugins] Support second scheduler mode #3852
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.
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
Conversation
Signed-off-by: Cheyu Wu <[email protected]>
09c3853
to
ea71807
Compare
Hi @kevin85421, PTAL |
Why do you use single scheduler for manual test? |
cc @troychiu for review |
Hi @kevin85421 labels:
ray.io/scheduler-name: scheduler-plugins and verified via: $ kubectl get pods -o jsonpath='{range .items[*]}{.metadata.name}{"\t"}{.spec.schedulerName}{"\n"}{end}' This setup follows the multi-scheduler setup, where I’ll revise the wording in the PR description to avoid confusion around the "single scheduler" statement. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand it, you deploy scheduler-plugins in "single scheduler" mode to replace the default scheduler. For "second scheduler" mode, you need to use the Helm chart to install scheduler-plugins in a separate Pod.
https://github.com/kubernetes-sigs/scheduler-plugins/blob/93126eabdf526010bf697d5963d849eab7e8e898/doc/install.md#as-a-second-scheduler
Ops, I have a misunderstanding. I will use the second scheduler mode instead. |
Hi @kevin85421 @troychiu, I have updated the manual testing procedure, PTAL |
I have also updated the 100 pods manual testing, and all of them are in pending status |
Have you tested for both single scheduler and second scheduler for this 100 Pods RayCluster CR? |
@@ -90,8 +90,7 @@ func (k *KubeScheduler) AddMetadataToPod(_ context.Context, app *rayv1.RayCluste | |||
if k.isGangSchedulingEnabled(app) { | |||
pod.Labels[kubeSchedulerPodGroupLabelKey] = app.Name | |||
} | |||
// TODO(kevin85421): Currently, we only support "single scheduler" mode. If we want to support | |||
// "second scheduler" mode, we need to add `schedulerName` to the pod spec. | |||
pod.Spec.SchedulerName = k.Name() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we change the name to scheduler-plugins-scheduler
to match the default name in https://github.com/kubernetes-sigs/scheduler-plugins/blob/master/manifests/install/charts/as-a-second-scheduler/values.yaml#L6C9-L6C36?
if cluster.Labels == nil { | ||
cluster.Labels = make(map[string]string) | ||
} | ||
if tt.enableGang { | ||
cluster.Labels["ray.io/gang-scheduling-enabled"] = "true" | ||
} else { | ||
delete(cluster.Labels, "ray.io/gang-scheduling-enabled") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this be cleaner?
if cluster.Labels == nil { | |
cluster.Labels = make(map[string]string) | |
} | |
if tt.enableGang { | |
cluster.Labels["ray.io/gang-scheduling-enabled"] = "true" | |
} else { | |
delete(cluster.Labels, "ray.io/gang-scheduling-enabled") | |
} | |
cluster.Labels = make(map[string]string) | |
if tt.enableGang { | |
cluster.Labels["ray.io/gang-scheduling-enabled"] = "true" | |
} |
scheduler := &KubeScheduler{} | ||
scheduler.AddMetadataToPod(context.TODO(), &cluster, "worker", pod) | ||
|
||
if tt.expectedPodGroup { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we simply use enableGang instead of having another parameter? I think they have similar intention.
As @kevin85421 mentioned, can you also double check if both modes work fine? |
|
@@ -21,7 +21,7 @@ import ( | |||
) | |||
|
|||
const ( | |||
schedulerName string = "scheduler-plugins" | |||
schedulerName string = "scheduler-plugins-scheduler" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mind adding a comment mentioning the source of the name? https://github.com/kubernetes-sigs/scheduler-plugins/blob/master/manifests/install/charts/as-a-second-scheduler/values.yaml#L6C9-L6C36
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is important. I will add the comment.
@@ -69,13 +69,13 @@ logging: | |||
# | |||
# 4. Use PodGroup | |||
# batchScheduler: | |||
# name: scheduler-plugins | |||
# name: scheduler-plugins-scheduler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For user facing config, I am not sure if we should use "scheduler-plugins" or "scheduler-plugins-scheduler". Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, and it's easier to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But, I think this is a little awkward, we cannot directly change GetPluginName
, because
case schedulerplugins.GetPluginName(): |
If we need to change batchScheduler
to scheduler-plugins
, the code will probably be
const (
schedulerName string = "scheduler-plugins"
+ defaultSchedulerName string = "scheduler-plugins-scheduler"
kubeSchedulerPodGroupLabelKey string = "scheduling.x-k8s.io/pod-group"
)
func GetPluginName() string {
return schedulerName
}
func (k *KubeScheduler) Name() string {
return defaultSchedulerName -> Is this fine to change something like this?
}
I am not sure if there is a better idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, user experience is more important so this is fine to me. However, we'll need good variable naming and comments explaining why there are two names and their corresponding responsibility.
Hi @troychiu, PTAL
|
@kevin85421 do you mind taking a look? Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…ect#3852) Signed-off-by: Cheyu Wu <[email protected]>
Why are these changes needed?
Currently, KubeRay only supports scheduler plugins when it is deployed as a single scheduler.
This change adds support for using a secondary scheduler with
scheduler-plugins
Manual Testing
Common Portion
Ray operator setup
Set
helm-chart/kuberay-operator/values.yaml
'sbatchScheduler.name
toscheduler-plugins
Testing YAML file
deploy.yaml
Single scheduler
CoScheduler setup
Follow the instruction - Reference
Log into the control plane node
Backup kube-scheduler.yaml
Install vim in
kube-scheduler-kind-control-plane
Fix the permission problem in
kube-scheduler-kind-control-plane
Create
/etc/kubernetes/sched-cc.yaml
Install all-in-one.yaml (outside the pod)
Apply missing YAML (outside the pod)
Check the deployment (outside the pod)
Install podgroup crds (outside the pod)
Modify
/etc/kubernetes/manifests/kube-scheduler.yaml
You can see the instruction for more detals
Verify that kube-scheduler pod is running properly
Apply
deploy.yaml
Run Cmd to deploy raycluster with
scheduler-plugins-scheduler
andgang-scheduling-enabled
Result
Get Status
Get scheduler Name - ray operator & ray head & ray worker
$ k get pods -o jsonpath='{range .items[*]}{.metadata.name}{"\t"}{.spec.schedulerName}{"\n"}{end}' kuberay-operator-77596879bc-m7csr default-scheduler raycluster-kuberay-head scheduler-plugins-scheduler raycluster-kuberay-workergroup-worker-68ht4 scheduler-plugins-scheduler raycluster-kuberay-workergroup-worker-m8sv9 scheduler-plugins-scheduler raycluster-kuberay-workergroup-worker-x2ghh scheduler-plugins-scheduler
Remove the deployment
Modify the
deploy.yaml
and apply itRun cmd to check is all of the pod in pending status
$ kubectl get pods -A NAMESPACE NAME READY STATUS RESTARTS AGE default kuberay-operator-77596879bc-m7csr 1/1 Running 0 3m32s default raycluster-kuberay-head 0/1 Pending 0 10s default raycluster-kuberay-workergroup-worker-2sl5s 0/1 Pending 0 9s default raycluster-kuberay-workergroup-worker-2svs6 0/1 Pending 0 7s default raycluster-kuberay-workergroup-worker-2xh6d 0/1 Pending 0 7s default raycluster-kuberay-workergroup-worker-4575m 0/1 Pending 0 10s ... -> skip lots of pending worker pods default raycluster-kuberay-workergroup-worker-znhp5 0/1 Pending 0 7s default raycluster-kuberay-workergroup-worker-zntkk 0/1 Pending 0 6s default raycluster-kuberay-workergroup-worker-zvvsh 0/1 Pending 0 6s default raycluster-kuberay-workergroup-worker-zz6tw 0/1 Pending 0 7s kube-system coredns-6f6b679f8f-cngxt 1/1 Running 0 22m kube-system coredns-6f6b679f8f-nprq6 1/1 Running 0 22m kube-system etcd-kind-control-plane 1/1 Running 0 22m kube-system kindnet-hnsvz 1/1 Running 0 22m kube-system kube-apiserver-kind-control-plane 1/1 Running 0 22m kube-system kube-controller-manager-kind-control-plane 1/1 Running 0 22m kube-system kube-proxy-jbzqc 1/1 Running 0 22m kube-system kube-scheduler-kind-control-plane 1/1 Running 0 6m59s local-path-storage local-path-provisioner-57c5987fd4-8k26v 1/1 Running 0 22m scheduler-plugins scheduler-plugins-controller-845cfd89c6-886bv 1/1 Running 1 (11m ago) 13m
Delete cluster for the next testing
Second scheduler
According to the instruction - Reference
Install the
scheduler-plugins
Check the scheduler-plugins is running
Ray operator and apply config
Set
helm-chart/kuberay-operator/values.yaml
batchScheduler.name toscheduler-plugins
Apply
deploy.yaml
Run Cmd to deploy raycluster with
scheduler-plugins-scheduler
andgang-scheduling-enabled
Result
Get Status
Get scheduler Name - ray operator & ray head & ray worker
$ k get pods -o jsonpath='{range .items[*]}{.metadata.name}{"\t"}{.spec.schedulerName}{"\n"}{end}' kuberay-operator-77596879bc-g5zvx default-scheduler raycluster-kuberay-head scheduler-plugins-scheduler raycluster-kuberay-workergroup-worker-fjgjw scheduler-plugins-scheduler raycluster-kuberay-workergroup-worker-fxnvr scheduler-plugins-scheduler raycluster-kuberay-workergroup-worker-t69t2 scheduler-plugins-scheduler scheduler-plugins-controller-845cfd89c6-j4h6b default-scheduler scheduler-plugins-scheduler-5dd667cb77-lr6tg default-scheduler
Remove the deployment
Modify the
deploy.yaml
and apply itRun cmd to check is all of the pod in pending status
$ kubectl get pods -A NAMESPACE NAME READY STATUS RESTARTS AGE default kuberay-operator-77596879bc-g5zvx 1/1 Running 0 12m default raycluster-kuberay-head 0/1 Pending 0 13s default raycluster-kuberay-workergroup-worker-2gqpl 0/1 Pending 0 12s default raycluster-kuberay-workergroup-worker-2qdjs 0/1 Pending 0 12s default raycluster-kuberay-workergroup-worker-2t6d4 0/1 Pending 0 11s default raycluster-kuberay-workergroup-worker-4jwgj 0/1 Pending 0 10s default raycluster-kuberay-workergroup-worker-52vw7 0/1 Pending 0 12s ... -> skip lots of pending worker pods default raycluster-kuberay-workergroup-worker-zckrr 0/1 Pending 0 12s default raycluster-kuberay-workergroup-worker-zfpzb 0/1 Pending 0 12s default raycluster-kuberay-workergroup-worker-zvzhm 0/1 Pending 0 10s default scheduler-plugins-controller-845cfd89c6-j4h6b 1/1 Running 0 4m16s default scheduler-plugins-scheduler-5dd667cb77-lr6tg 1/1 Running 0 4m16s kube-system coredns-6f6b679f8f-p62kv 1/1 Running 0 16m kube-system coredns-6f6b679f8f-xzn42 1/1 Running 0 16m kube-system etcd-kind-control-plane 1/1 Running 0 16m kube-system kindnet-mwhsd 1/1 Running 0 16m kube-system kube-apiserver-kind-control-plane 1/1 Running 0 16m kube-system kube-controller-manager-kind-control-plane 1/1 Running 0 16m kube-system kube-proxy-mt8q4 1/1 Running 0 16m kube-system kube-scheduler-kind-control-plane 1/1 Running 0 16m local-path-storage local-path-provisioner-57c5987fd4-t5dmn 1/1 Running 0 16m
Related issue number
Closes #3769
Checks