Support Koordinator as one batch scheduler option #2572

kingeasternsun · 2024-11-26T17:17:25Z

Why are these changes needed?

Koordinator is a QoS-based scheduling for efficient orchestration of microservices, AI, and big data workloads on Kubernetes. It aims to improve the runtime efficiency and reliability of both latency sensitive workloads and batch jobs, simplify the complexity of resource-related configuration tuning, and increase pod deployment density to improve resource utilizations.

The integration is easy, Koordinator support annotation way to support gang scheduling without podgroup CR

gang.scheduling.koordinator.sh/name           
gang.scheduling.koordinator.sh/min-available

Koordinator are compatible with pod-group.scheduling.sigs.k8s.io, pod-group.scheduling.sigs.k8s.io/name and pod-group.scheduling.sigs.k8s.io/min-available in community.

https://koordinator.sh/docs/designs/gang-scheduling

Related issue number

#2573

Checks

I've made sure the tests are passing.
Testing Strategy
- Unit tests
- Manual tests
- This PR is not tested :(

kevin85421 · 2024-12-04T05:03:33Z

Is this PR ready for review? The PR title is still marked as "draft."

andrewsykim · 2024-12-04T15:10:45Z

@kingeasternsun can you please add some tests before marking this ready for review? See other implementation for batch schedulers for reference

Signed-off-by: kingeasternsun <[email protected]>

kingeasternsun · 2024-12-07T13:40:57Z

Is this PR ready for review? The PR title is still marked as "draft."

Thanks for your comment, It's still developing, and now it lacks test code.

kingeasternsun · 2024-12-07T13:41:45Z

@kingeasternsun can you please add some tests before marking this ready for review? See other implementation for batch schedulers for reference

Thanks for your advice, I'll add the tests quickly

Signed-off-by: kingeasternsun <[email protected]>

kingeasternsun · 2024-12-10T03:04:55Z

Is this PR ready for review? The PR title is still marked as "draft."

Hey, everything is fine now.

kingeasternsun · 2024-12-10T03:05:30Z

@kingeasternsun can you please add some tests before marking this ready for review? See other implementation for batch schedulers for reference

Hi, tests had been added .

andrewsykim · 2024-12-10T04:04:31Z

ray-operator/controllers/ray/batchscheduler/koordinator/koordinator_gang_groups.go

+}
+
+func analyzeGangGroupsFromApp(app *rayv1.RayCluster) ([]string, map[string]wokerGroupReplicas) {
+	gangGroups := make([]string, 1+len(app.Spec.WorkerGroupSpecs))


nit: len(app.Spec.WorkerGroupSpecs)+1

nit: len(app.Spec.WorkerGroupSpecs)+1

Thank you for your review! I will fix it as soon as possible.

andrewsykim · 2024-12-10T04:04:45Z

ray-operator/controllers/ray/batchscheduler/koordinator/koordinator_gang_groups.go

+	}
+
+	for i, workerGroupSepc := range app.Spec.WorkerGroupSpecs {
+		gangGroups[1+i] = generateGangGroupName(app, workerGroupSepc.Template.Namespace, workerGroupSepc.GroupName)


nit: gangGroups[i+1]

nit: gangGroups[i+1]

Thank you for your review! I will fix it as soon as possible.

andrewsykim · 2024-12-10T04:05:03Z

ray-operator/controllers/ray/batchscheduler/koordinator/koordinator_gang_groups.go

+	for i, workerGroupSepc := range app.Spec.WorkerGroupSpecs {
+		gangGroups[1+i] = generateGangGroupName(app, workerGroupSepc.Template.Namespace, workerGroupSepc.GroupName)
+		minMemberMap[workerGroupSepc.GroupName] = wokerGroupReplicas{
+			Replicas:    *(workerGroupSepc.Replicas),


nit: the brackets are not needed here

nit: the brackets are not needed here

Thank you for your review! I will fix it as soon as possible.

andrewsykim · 2024-12-10T04:05:07Z

ray-operator/controllers/ray/batchscheduler/koordinator/koordinator_gang_groups.go

+		gangGroups[1+i] = generateGangGroupName(app, workerGroupSepc.Template.Namespace, workerGroupSepc.GroupName)
+		minMemberMap[workerGroupSepc.GroupName] = wokerGroupReplicas{
+			Replicas:    *(workerGroupSepc.Replicas),
+			MinReplicas: *(workerGroupSepc.MinReplicas),


nit: the brackets are not needed here

nit: the brackets are not needed here

Thank you for your review! I will fix it as soon as possible.

andrewsykim · 2024-12-10T04:05:36Z

ray-operator/controllers/ray/batchscheduler/koordinator/koordinator_scheduler.go

+	logger := ctrl.LoggerFrom(ctx).WithName(SchedulerName)
+
+	// when gang scheduling is enabled, extra annotations need to be added to all pods
+	if y.isGangSchedulingEnabled(app) {


if !y.isGangSchedulingEnabled(app) { return } ....

if !y.isGangSchedulingEnabled(app) { return } ....

Thank you for your review! I will fix it as soon as possible.

andrewsykim · 2024-12-10T04:07:02Z

ray-operator/controllers/ray/batchscheduler/koordinator/koordinator_scheduler_test.go

+		},
+	)
+
+	setHeadPodNamespace(rayClusterWithGangScheduling, "ns0")


Why is this call needed? The namespace should be inherited from the RayCluster namespace right?

Why is this call needed? The namespace should be inherited from the RayCluster namespace right?

What you said is absolutely correct. However, to make this module's code more general, I considered that there might be scenarios in the future where the namespace for head pods or worker pods needs to be specifically designated. Therefore, I implemented compatibility here: if the namespace for the head pod or worker pod is empty, it will inherit the namespace of the rayCluster; otherwise, it will be processed according to the specified namespace.

Of course, if you think we don’t need to consider such special cases for now, I completely agree as well.

andrewsykim · 2024-12-10T04:07:57Z

ray-operator/controllers/ray/batchscheduler/koordinator/koordinator_scheduler_test.go

+
+	setHeadPodNamespace(rayClusterWithGangScheduling, "ns0")
+	addWorkerPodSpec(rayClusterWithGangScheduling, "workergroup1", "ns1", 4, 2)
+	addWorkerPodSpec(rayClusterWithGangScheduling, "workergroup2", "ns2", 5, 3)


Does Koordinator support having RayCluster pods spread across different namespaces? I don't think that's really supported with Kuberay

Does Koordinator support having RayCluster pods spread across different namespaces? I don't think that's really supported with Kuberay

As mentioned in the documentation at https://koordinator.sh/docs/designs/gang-scheduling/#annotation-way, the Koordinator scheduler supports highly flexible GangGroup scheduling requirements, including the ability to support different pods in a GangGroup belonging to different namespaces.

Of course, if you believe such special scheduling requirements are unlikely to occur in KubeRay, I fully agree as well.

andrewsykim · 2024-12-10T04:09:00Z

Can you add an example YAML similar to this one https://github.com/ray-project/kuberay/blob/e9f31556c14fae6391fb27a4a96bfbe01f917d46/ray-operator/config/samples/ray-cluster.yunikorn-scheduler.yaml?

andrewsykim · 2024-12-10T04:09:34Z

Please update main.go

kuberay/ray-operator/main.go

Lines 96 to 97 in e9f3155

    
           flag.StringVar(&batchScheduler, "batch-scheduler", "", 
        
           	"Batch scheduler name, supported values are volcano and yunikorn.")

andrewsykim · 2024-12-10T04:10:02Z

Please update batch schedulers in helm chart

kuberay/helm-chart/kuberay-operator/values.yaml

Lines 60 to 87 in e9f3155

    
           # Enable customized Kubernetes scheduler integration. If enabled, Ray workloads will be scheduled 
        
           # by the customized scheduler. 
        
           #  * "enabled" is the legacy option and will be deprecated soon. 
        
           #  * "name" is the standard option, expecting a scheduler name, supported values are 
        
           #    "default", "volcano", and "yunikorn". 
        
           # 
        
           # Note: "enabled" and "name" should not be set at the same time. If both are set, an error will be thrown. 
        
           # 
        
           # Examples: 
        
           #  1. Use volcano (deprecated) 
        
           #       batchScheduler: 
        
           #         enabled: true 
        
           # 
        
           #  2. Use volcano 
        
           #       batchScheduler: 
        
           #         name: volcano 
        
           # 
        
           #  3. Use yunikorn 
        
           #       batchScheduler: 
        
           #         name: yunikorn 
        
           # 
        
           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 
        
             # 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: ""

kingeasternsun · 2024-12-16T02:02:23Z

Can you add an example YAML similar to this one https://github.com/ray-project/kuberay/blob/e9f31556c14fae6391fb27a4a96bfbe01f917d46/ray-operator/config/samples/ray-cluster.yunikorn-scheduler.yaml?

Thank you for your review! I will add it as soon as possible.

kingeasternsun · 2024-12-16T02:02:44Z

Please update main.go

kuberay/ray-operator/main.go

Lines 96 to 97 in e9f3155

flag.StringVar(&batchScheduler, "batch-scheduler", "",

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

Thank you for your review! I will fix it as soon as possible.

kingeasternsun · 2024-12-16T02:03:03Z

Please update batch schedulers in helm chart

kuberay/helm-chart/kuberay-operator/values.yaml

Lines 60 to 87 in e9f3155

# Enable customized Kubernetes scheduler integration. If enabled, Ray workloads will be scheduled

# by the customized scheduler.

# * "enabled" is the legacy option and will be deprecated soon.

# * "name" is the standard option, expecting a scheduler name, supported values are

# "default", "volcano", and "yunikorn".

#

# Note: "enabled" and "name" should not be set at the same time. If both are set, an error will be thrown.

#

# Examples:

# 1. Use volcano (deprecated)

# batchScheduler:

# enabled: true

#

# 2. Use volcano

# batchScheduler:

# name: volcano

#

# 3. Use yunikorn

# batchScheduler:

# name: yunikorn

#

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

# 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: ""

Thank you for your review! I will fix it as soon as possible.

Signed-off-by: kingeasternsun <[email protected]>

:spark: Support Koordinator as one batch scheduler option

f1a1297

kevin85421 assigned kevin85421 and andrewsykim Nov 27, 2024

support koordinator gang group with annotation

b3e2847

Signed-off-by: kingeasternsun <[email protected]>

add koordinator gang group ut

d7690fa

Signed-off-by: kingeasternsun <[email protected]>

kingeasternsun changed the title ~~Draft: Support Koordinator as one batch scheduler option~~ Support Koordinator as one batch scheduler option Dec 7, 2024

andrewsykim reviewed Dec 10, 2024

View reviewed changes

fix the nit, and add the sample of koordinator

440879e

Signed-off-by: kingeasternsun <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Koordinator as one batch scheduler option #2572

Support Koordinator as one batch scheduler option #2572

kingeasternsun commented Nov 26, 2024 •

edited

Loading

kevin85421 commented Dec 4, 2024

andrewsykim commented Dec 4, 2024

kingeasternsun commented Dec 7, 2024

kingeasternsun commented Dec 7, 2024

kingeasternsun commented Dec 10, 2024

kingeasternsun commented Dec 10, 2024

andrewsykim Dec 10, 2024

kingeasternsun Dec 16, 2024

andrewsykim Dec 10, 2024

kingeasternsun Dec 16, 2024

andrewsykim Dec 10, 2024

kingeasternsun Dec 16, 2024

andrewsykim Dec 10, 2024

kingeasternsun Dec 16, 2024

andrewsykim Dec 10, 2024

kingeasternsun Dec 16, 2024

andrewsykim Dec 10, 2024

kingeasternsun Dec 16, 2024

andrewsykim Dec 10, 2024

kingeasternsun Dec 16, 2024

andrewsykim commented Dec 10, 2024

andrewsykim commented Dec 10, 2024

andrewsykim commented Dec 10, 2024

kingeasternsun commented Dec 16, 2024

kingeasternsun commented Dec 16, 2024

kingeasternsun commented Dec 16, 2024

Support Koordinator as one batch scheduler option #2572

Are you sure you want to change the base?

Support Koordinator as one batch scheduler option #2572

Conversation

kingeasternsun commented Nov 26, 2024 • edited Loading

Why are these changes needed?

Related issue number

Checks

kevin85421 commented Dec 4, 2024

andrewsykim commented Dec 4, 2024

kingeasternsun commented Dec 7, 2024

kingeasternsun commented Dec 7, 2024

kingeasternsun commented Dec 10, 2024

kingeasternsun commented Dec 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewsykim commented Dec 10, 2024

andrewsykim commented Dec 10, 2024

andrewsykim commented Dec 10, 2024

kingeasternsun commented Dec 16, 2024

kingeasternsun commented Dec 16, 2024

kingeasternsun commented Dec 16, 2024

kingeasternsun commented Nov 26, 2024 •

edited

Loading