Make scheduler-plugins the default gang scheduler #209

Syulin7 · 2023-01-29T09:13:10Z

What this PR does / why we need it:

Now supports many gang schedulers(volcano, scheduler-plugins). This PR supports koordinator gang scheduler.

Related: kubeflow/training-operator#1746

johnugeorge · 2023-01-29T14:18:47Z

Thanks @Syulin7 We are in the process of training operator release(currently rc0). Since kubeflow/common dependency has not branched out, we have to wait a bit to have the final release and then merge this one.

/cc @tenzen-y Can you review this?

tenzen-y · 2023-01-29T15:31:55Z

Thanks @Syulin7 We are in the process of training operator release(currently rc0). Since kubeflow/common dependency has not branched out, we have to wait a bit to have the final release and then merge this one.

/cc @tenzen-y Can you review this?

Sure.
First, I will ask questions about the Koordinator at kubeflow/training-operator#1746.

tenzen-y · 2023-01-29T15:32:06Z

/hold

pkg/controller.v1/control/podgroup_control.go

pkg/reconciler.v1/common/gang_koordinator.go

Syulin7 · 2023-01-31T03:26:25Z

@tenzen-y This PR is ready for review. PTAL.

pkg/controller.v1/common/job_controller.go

tenzen-y · 2023-01-31T05:24:04Z

Can you squash commits into one?

Syulin7 · 2023-01-31T05:55:34Z

Can you squash commits into one?

Done

tenzen-y

/lgtm
/assign @johnugeorge

alculquicondor

a nit, otherwise lgtm

alculquicondor · 2023-03-15T18:14:15Z

pkg/controller.v1/common/job_controller.go

@@ -72,6 +72,8 @@ const (
 	GangSchedulerNone             GangScheduler = "None"
 	GangSchedulerVolcano          GangScheduler = "volcano"
 	GangSchedulerSchedulerPlugins GangScheduler = "scheduler-plugins"
+	// GangSchedulerDefault Using this scheduler name or any scheduler name different than volcano uses the scheduler-plugins PodGroup
+	GangSchedulerDefault GangScheduler = "default-scheduler"


Remove if this is not being used

alculquicondor · 2023-03-15T18:15:46Z

/retitle Make scheduler-plugins the default gang scheduler

Syulin7 · 2023-03-16T09:19:44Z

@alculquicondor @tenzen-y @terrytangyuan all conversations are done, PTAL.

tenzen-y · 2023-03-16T09:52:31Z

pkg/reconciler.v1/common/gang_scheduler_framework.go

@@ -164,6 +165,10 @@ func (r *SchedulerFrameworkReconciler) DecoratePodForGangScheduling(
 	podTemplate *corev1.PodTemplateSpec,
 	job client.Object,
 ) {
+	if podTemplate.Spec.SchedulerName == "" && r.GetGangSchedulerName() != "default-scheduler" {


Suggested change

if podTemplate.Spec.SchedulerName == "" && r.GetGangSchedulerName() != "default-scheduler" {

if podTemplate.Spec.SchedulerName == "" {

Let's use a logic the same as the podgroup_control.

tenzen-y · 2023-03-16T09:53:22Z

I left a comment for nit. Overall, lgtm.

Signed-off-by: Syulin7 <735122171@qq.com>

tenzen-y · 2023-03-16T10:49:11Z

@Syulin7 Thanks for the great work!
/lgtm

@johnugeorge
Do you decide on the week for releasing the training operator v1.6, although releasing kubeflow v1.7 seems delayed?

Week 23 - Distribution Testing Ends: Wednesday, Mar 22nd 2023

alculquicondor · 2023-03-16T13:22:32Z

pkg/controller.v1/control/podgroup_control.go

 }

 // NewSchedulerPluginsControl returns a SchedulerPluginsControl
-func NewSchedulerPluginsControl(c client.Client) PodGroupControlInterface {
-	return &SchedulerPluginsControl{Client: c}
+func NewSchedulerPluginsControl(c client.Client, schedulerName string) PodGroupControlInterface {


Not about this PR, but I realized that some functions below are unnecessary, like UpdatePodGroup, and CreatePodGroup. They are casting unnecessarily: the controller-runtime client can already handle the casting via reflection. And I think GetPodGroup and DeletePodGroup could be refactored so that we don't need the casting either. Maybe even use generics.

alculquicondor · 2023-03-16T13:22:53Z

/lgtm

terrytangyuan

Thanks!

/lgtm
/approve

google-oss-prow · 2023-03-16T21:04:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: terrytangyuan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [terrytangyuan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

johnugeorge · 2023-03-18T19:02:00Z

@tenzen-y The final release is not yet made due to delay in Kubeflow upstream. Sorry for the delay. This should not happen again in the future as we always branch out for all repos except common. We will target this week end to merge- Friday 24 March

tenzen-y · 2023-03-19T16:24:02Z

@tenzen-y The final release is not yet made due to delay in Kubeflow upstream. Sorry for the delay. This should not happen again in the future as we always branch out for all repos except common. We will target this week end to merge- Friday 24 March

Thanks for letting me know! I appreciate your hard work!

johnugeorge · 2023-03-21T19:55:59Z

Training operator release is made. This can be merged.

Thanks for the patience

tenzen-y · 2023-03-21T20:03:59Z

@Syulin7 Thank you!
/hold cancel

tenzen-y · 2023-03-28T19:23:33Z

@johnugeorge Can we cut a new release on this repo to move forward kubeflow/training-operator#1747?

google-oss-prow bot requested review from Jeffwan and terrytangyuan January 29, 2023 09:13

google-oss-prow bot added the size/L label Jan 29, 2023

Syulin7 force-pushed the koordinator branch from 20fb888 to bed2e1c Compare January 29, 2023 09:33

Syulin7 mentioned this pull request Jan 29, 2023

Make scheduler-plugins the default gang scheduler. kubeflow/training-operator#1747

Merged

1 task

google-oss-prow bot added the do-not-merge/hold label Jan 29, 2023

tenzen-y mentioned this pull request Jan 30, 2023

Support koordinator gang scheduler plugin kubeflow/training-operator#1746

Closed

tenzen-y reviewed Jan 30, 2023

View reviewed changes

pkg/controller.v1/control/podgroup_control.go Outdated Show resolved Hide resolved

pkg/reconciler.v1/common/gang_koordinator.go Outdated Show resolved Hide resolved

Syulin7 force-pushed the koordinator branch from 09f1889 to 383ec8f Compare January 31, 2023 02:13

google-oss-prow bot added size/XS and removed size/L labels Jan 31, 2023

Syulin7 force-pushed the koordinator branch from 383ec8f to b812b46 Compare January 31, 2023 02:33

google-oss-prow bot added size/M size/L and removed size/XS size/M labels Jan 31, 2023

tenzen-y reviewed Jan 31, 2023

View reviewed changes

pkg/controller.v1/common/job_controller.go Outdated Show resolved Hide resolved

Syulin7 force-pushed the koordinator branch from 65a3dc5 to b812b46 Compare January 31, 2023 05:51

google-oss-prow bot added size/M and removed size/L labels Jan 31, 2023

tenzen-y reviewed Jan 31, 2023

View reviewed changes

google-oss-prow bot assigned johnugeorge and tenzen-y Jan 31, 2023

google-oss-prow bot added the lgtm label Jan 31, 2023

Syulin7 force-pushed the koordinator branch 2 times, most recently from fec0139 to 34510ef Compare March 15, 2023 15:30

alculquicondor reviewed Mar 15, 2023

View reviewed changes

Syulin7 changed the title ~~Support Koordinator Gang Scheduler~~ Make scheduler-plugins the default gang scheduler Mar 16, 2023

Syulin7 force-pushed the koordinator branch from 34510ef to 5b4466d Compare March 16, 2023 09:16

tenzen-y reviewed Mar 16, 2023

View reviewed changes

Set SchedulerPluginsControl as the default implementation

5b3c484

Signed-off-by: Syulin7 <735122171@qq.com>

Syulin7 force-pushed the koordinator branch from 5b4466d to 5b3c484 Compare March 16, 2023 10:01

Syulin7 closed this Mar 16, 2023

Syulin7 reopened this Mar 16, 2023

google-oss-prow bot added the lgtm label Mar 16, 2023

alculquicondor reviewed Mar 16, 2023

View reviewed changes

google-oss-prow bot assigned alculquicondor Mar 16, 2023

terrytangyuan approved these changes Mar 16, 2023

View reviewed changes

google-oss-prow bot assigned terrytangyuan Mar 16, 2023

google-oss-prow bot added the approved label Mar 16, 2023

google-oss-prow bot removed the do-not-merge/hold label Mar 21, 2023

google-oss-prow bot merged commit 83259a0 into kubeflow:master Mar 21, 2023

tenzen-y mentioned this pull request Mar 24, 2023

Support the coscheduling plugin of scheduler-plugins kubeflow/mpi-operator#538

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make scheduler-plugins the default gang scheduler #209

Make scheduler-plugins the default gang scheduler #209

Syulin7 commented Jan 29, 2023

johnugeorge commented Jan 29, 2023

tenzen-y commented Jan 29, 2023

tenzen-y commented Jan 29, 2023

Syulin7 commented Jan 31, 2023

tenzen-y commented Jan 31, 2023

Syulin7 commented Jan 31, 2023

tenzen-y left a comment

alculquicondor left a comment

alculquicondor Mar 15, 2023

alculquicondor commented Mar 15, 2023

Syulin7 commented Mar 16, 2023

tenzen-y Mar 16, 2023

tenzen-y commented Mar 16, 2023

tenzen-y commented Mar 16, 2023

alculquicondor Mar 16, 2023

alculquicondor commented Mar 16, 2023

terrytangyuan left a comment

google-oss-prow bot commented Mar 16, 2023

johnugeorge commented Mar 18, 2023

tenzen-y commented Mar 19, 2023

johnugeorge commented Mar 21, 2023

tenzen-y commented Mar 21, 2023

tenzen-y commented Mar 28, 2023

	if podTemplate.Spec.SchedulerName == "" && r.GetGangSchedulerName() != "default-scheduler" {
	if podTemplate.Spec.SchedulerName == "" {

Make scheduler-plugins the default gang scheduler #209

Make scheduler-plugins the default gang scheduler #209

Conversation

Syulin7 commented Jan 29, 2023

johnugeorge commented Jan 29, 2023

tenzen-y commented Jan 29, 2023

tenzen-y commented Jan 29, 2023

Syulin7 commented Jan 31, 2023

tenzen-y commented Jan 31, 2023

Syulin7 commented Jan 31, 2023

tenzen-y left a comment

Choose a reason for hiding this comment

alculquicondor left a comment

Choose a reason for hiding this comment

alculquicondor Mar 15, 2023

Choose a reason for hiding this comment

alculquicondor commented Mar 15, 2023

Syulin7 commented Mar 16, 2023

tenzen-y Mar 16, 2023

Choose a reason for hiding this comment

tenzen-y commented Mar 16, 2023

tenzen-y commented Mar 16, 2023

alculquicondor Mar 16, 2023

Choose a reason for hiding this comment

alculquicondor commented Mar 16, 2023

terrytangyuan left a comment

Choose a reason for hiding this comment

google-oss-prow bot commented Mar 16, 2023

johnugeorge commented Mar 18, 2023

tenzen-y commented Mar 19, 2023

johnugeorge commented Mar 21, 2023

tenzen-y commented Mar 21, 2023

tenzen-y commented Mar 28, 2023