Support the coscheduling plugin of scheduler-plugins #538

tenzen-y · 2023-03-20T20:58:28Z

I added the implementation for the coscheduling plugin of kubernetes-sigs/scheduler-plugins.

ref: https://github.com/kubernetes-sigs/scheduler-plugins/tree/master/pkg/coscheduling

So I modified the following:

Added PodGroupControl interface inspired by https://github.com/kubeflow/common/blob/4dbf18c85417b54158914d1d01c1744b4e3542f2/pkg/controller.v1/control/pod_control.go#L53-L65.
Added VolcanoCtrl implemented PodGroupCtrl interface. The logic is the same as before.
Added SchedulerPluginsCtrl implemented PodGroupCtrl interface.

Follow ups:

Add E2E tests

Part-of: #500

tenzen-y · 2023-03-20T21:02:23Z

I will add the integration tests to this PR.

PTAL
/assign @alculquicondor

cmd/mpi-operator/app/server.go

tenzen-y · 2023-03-20T21:06:50Z

go.mod

 	k8s.io/klog v1.0.0
 	k8s.io/kube-openapi v0.0.0-20230109183929-3758b55a6596
 	k8s.io/utils v0.0.0-20220728103510-ee6ede2d64ed
 	sigs.k8s.io/controller-runtime v0.13.1
+	sigs.k8s.io/scheduler-plugins v0.25.7


We use the newest version since this version introduces a new API group, scheduling.x-k8s.io.

there is no 0.26?

Yes, the newest version is 0.25.
Also, according to kubernetes-sigs/scheduler-plugins#503, the scheduler-plugins seems to be released every 3 months.
So 0.26 will be released in June.

Yes, we do. Usually, we cut official releases every ~3 months

alculquicondor · 2023-03-21T19:57:17Z

Makefile

@@ -59,8 +61,8 @@ vet:

 .PHONY: test
 test:
-test: bin/envtest
-	KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) -p path)" go test -covermode atomic -coverprofile=profile.cov $(shell go list ./... | grep -v '/test/e2e')
+test: bin/envtest scheduler-plugins-crd


I guess this means that volcano doesn't have integration tests

Yes, that's right.

cmd/mpi-operator/app/options/options.go

cmd/mpi-operator/app/server.go

alculquicondor · 2023-03-21T20:01:32Z

go.mod

 	k8s.io/klog v1.0.0
 	k8s.io/kube-openapi v0.0.0-20230109183929-3758b55a6596
 	k8s.io/utils v0.0.0-20220728103510-ee6ede2d64ed
 	sigs.k8s.io/controller-runtime v0.13.1
+	sigs.k8s.io/scheduler-plugins v0.25.7


there is no 0.26?

alculquicondor · 2023-03-21T20:02:03Z

manifests/base/cluster-role.yaml

+  verbs:
+    - "*"
+- apiGroups:
+  - scheduling.k8s.io


both groups?

Does that mean the below?

- apiGroups: - scheduling.x-k8s.io - scheduling.k8s.io resources: - podgroups - priorityclasses verbs: - "*"

Sorry. I meant to ask if we really need to support both groups or just the newer one. I would vote for just the newer one.

I see.
I added the scheduling.k8s.io for priorityClass and the scheduling.x-k8s.io for podgroups. This means we support scheduling.x-k8s.io/v1alpha1 PodGroup and scheduling.k8s.io/v1 PriorityClass. So we don't support scheduling.k8s.io/v1alpha1 PodGroup.

Note:

OLD: scheduling.k8s.io/v1alpha1 PodGroup

NEW: scheduling.x-k8s.io/v1alpha1 PodGroup

IIUC, the PriorityClass belongs to scheduling.k8s.io APIgroup, right?

oops, sorry, I didn't notice it was a different kind.

pkg/controller/mpi_job_controller.go

pkg/controller/mpi_job_controller_test.go

alculquicondor · 2023-03-21T20:13:18Z

pkg/controller/podgroup.go

+			continue
+		}
+		for i := int32(0); i < *rp.Replicas; i++ {
+			for _, c := range rp.Template.Spec.Containers {


what about initContainers?

IMO, It is enough to count only containers since Pods don't launch both initContainers and containers simultaneously.

Do you have any concerns?

resource calculation in scheduler is: max(initContainers, sum(containers)) + pod overhead.

Also, this is ignoring any limit ranges or defaulting.

But I guess it's ok if this is just an estimate. I would guess that the scheduler ultimately considers the final pods.

But I guess it's ok if this is just an estimate. I would guess that the scheduler ultimately considers the final pods.

I think so too. It's fine with only containers since the conscheduling plugin isn't quota management software :)

alculquicondor · 2023-03-22T19:45:37Z

I wonder if @denkensk could give a sanity check as one of the authors of the coscheduling plugin.

tenzen-y · 2023-03-22T19:50:22Z

I wonder if @denkensk could give a sanity check as one of the authors of the coscheduling plugin.

@alculquicondor Thank you for the review!
I'd be happy if this PR is reviewed by @denkensk.

alculquicondor · 2023-03-23T19:15:48Z

pkg/controller/podgroup.go

+		if rp.Replicas == nil {
+			continue
+		}
+		for i := int32(0); i < *rp.Replicas; i++ {


Uhm... can you replace this with a multiplication?

Does that mean for i := 0; i < (num(containers) × replicas); i++?

I mean, avoid iterating over and over the pod containers. You should be able to multiply the resources for 1 pod by the number of replicas

I see. Makes sense. I will try.

alculquicondor · 2023-03-23T19:17:02Z

/lgtm
/hold
for @denkensk

alculquicondor · 2023-03-23T21:30:47Z

pkg/controller/podgroup.go

@@ -374,10 +374,11 @@ func addResources(minResources corev1.ResourceList, resources corev1.ResourceReq
 		}
 	}
 	for name, quantity := range merged {
+		addition := resource.MustParse(strconv.Itoa(int(quantity.Value()) * replicas))


Ah, I didn't realize that Quantity doesn't have a multiplication operation.

This is an odd way of calculating it though. Have you verified if this works for something like 1m?

If not, maybe we can revert and follow up with a PR upstream to add multiplication.

FWIW, in kueue we convert everything to int64 before multiplying, using MilliValue for CPU.

Have you verified if this works for something like 1m?

I verified that situation right now. This way doesn't work well :(

FWIW, in kueue we convert everything to int64 before multiplying, using MilliValue for CPU.

I verified the resource.MustParse(strconv.Itoa(int(quantity.MilliValue()) * replicas)) works well.

However, as you say, this way looks odd.

are you sure it gives you the right value?

also, it might not work well with memory.

If we use MilliValue for CPU and Memory, it seems to work well. However, for GPU, it doesn't work well.

However, I’m not sure the above conversion works well in all situations.

So, It might be better to revert this change.

+1

we should follow up with a multiplication method upstream. It will become increasingly useful for batch operators.

I will create an issue in k/k tomorrow or next week.

alculquicondor · 2023-03-23T22:26:40Z

/lgtm

denkensk

Thanks @tenzen-y Some small comments.

denkensk · 2023-03-24T10:53:22Z

pkg/controller/mpi_job_controller.go

-	mpiJobLister    listers.MPIJobLister
-	mpiJobSynced    cache.InformerSynced
+	// podGroupCtrl is a client for PodGroups (volcano and scheduler-plugins).
+	podGroupCtrl PodGroupControl


Sorry. A little confused. Why do we call it xxControl if it's a client?

I don't have the special reasons for calling it "xxControl", and I was inspired by https://github.com/kubeflow/common/tree/83259a047fc47895dab1012728fb829523363a6d/pkg/controller.v1/control. This means we can even name it "xxClient".

This was probably inspired on podControl in KCM:
https://github.com/kubernetes/kubernetes/blob/c3e7eca7fd38454200819b60e58144d5727f1bbc/pkg/controller/job/job_controller.go#L91

denkensk · 2023-03-24T11:11:56Z

cmd/mpi-operator/app/server.go

+		var podGroupCtrl controllersv1.PodGroupControl
+		if opt.GangSchedulingName == options.GangSchedulerVolcano {
+			podGroupCtrl = controllersv1.NewVolcanoCtrl(volcanoClientSet, namespace)
+		} else if len(opt.GangSchedulingName) != 0 {


how about if I set it as "xxx", will it still choose as scheduler-plugins? will we check if the name is invalid like not volcano and scheduler-plugins?

If you set some scheduler name excluding volcano to the gang-scheduling flag, the mpi-operator uses scheduler-plugins as we decided to block collaborating with some custom schedulers in kubeflow/common#209 (comment).

denkensk · 2023-03-24T11:17:23Z

cmd/mpi-operator/app/server.go

+		return nil, nil, nil, nil, nil, err
+	}
+
+	schedClientSet, err := schedclientset.NewForConfig(restclientset.AddUserAgent(config, "scheduler-plugins"))


do we need init clientset both for scheduler-plugins and volcano?

No, we don't. We may be able to create either clientset in L145 - L 152 of this file.

Ah, it might better pass the gangScheulingName to createClientSets func.

denkensk · 2023-03-24T14:44:11Z

pkg/controller/mpi_job_controller_test.go

+	if f.gangSchedulingName == options.GangSchedulerVolcano {
+		podGroupCtrl = NewVolcanoCtrl(f.volcanoClient, metav1.NamespaceAll)
+	} else if len(f.gangSchedulingName) != 0 {
+		podGroupCtrl = NewSchedulerPluginsCtrl(f.schedClient, metav1.NamespaceAll, "default-scheduler")


is it “default-scheduler”?

IIUC, if we set up the scheduler-plugins as single scheduler mode, we can use the scheduler-plugins as a part of the default scheduler.

https://github.com/kubernetes-sigs/scheduler-plugins/blob/master/doc/install.md#install-release-v0257-and-use-coscheduling

alculquicondor · 2023-03-28T17:18:16Z

can you squash?

/approve

Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

tenzen-y · 2023-03-28T18:14:55Z

@alculquicondor Squashed.

alculquicondor · 2023-03-28T18:20:21Z

/approve

leaving lgtm to @denkensk

google-oss-prow · 2023-03-28T18:20:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor

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~~ [alculquicondor]

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

denkensk · 2023-03-29T02:17:54Z

/lgtm

Thanks for your contribution. @tenzen-y

denkensk · 2023-03-29T02:26:06Z

/unhold

google-oss-prow bot added the do-not-merge/work-in-progress label Mar 20, 2023

google-oss-prow bot requested review from gaocegege and zw0610 March 20, 2023 20:58

google-oss-prow bot added the size/XXL label Mar 20, 2023

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

tenzen-y commented Mar 20, 2023

View reviewed changes

cmd/mpi-operator/app/server.go Outdated Show resolved Hide resolved

tenzen-y commented Mar 20, 2023

View reviewed changes

tenzen-y force-pushed the support-scheduler-plugins branch 2 times, most recently from fd79e7d to e5bd5e5 Compare March 21, 2023 13:46

tenzen-y changed the title ~~WIP: Support the coscheduler plugin of scheduler-plugins~~ Support the coscheduler plugin of scheduler-plugins Mar 21, 2023

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

tenzen-y changed the title ~~Support the coscheduler plugin of scheduler-plugins~~ Support the coscheduling plugin of scheduler-plugins Mar 21, 2023

alculquicondor reviewed Mar 21, 2023

View reviewed changes

tenzen-y force-pushed the support-scheduler-plugins branch 5 times, most recently from 0efb6d7 to 9450cdc Compare March 22, 2023 12:59

alculquicondor reviewed Mar 23, 2023

View reviewed changes

google-oss-prow bot added do-not-merge/hold lgtm and removed lgtm labels Mar 23, 2023

tenzen-y force-pushed the support-scheduler-plugins branch from 5945ac5 to 1fb5394 Compare March 23, 2023 21:04

alculquicondor reviewed Mar 23, 2023

View reviewed changes

tenzen-y force-pushed the support-scheduler-plugins branch from 1fb5394 to 9450cdc Compare March 23, 2023 22:20

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

denkensk reviewed Mar 24, 2023

View reviewed changes

google-oss-prow bot removed the lgtm label Mar 24, 2023

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

Support the coscheduling plugin of scheduler-plugins

58f0dfd

Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

tenzen-y force-pushed the support-scheduler-plugins branch from b41aeea to 58f0dfd Compare March 28, 2023 18:14

google-oss-prow bot assigned denkensk Mar 29, 2023

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

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

google-oss-prow bot merged commit 2495860 into kubeflow:master Mar 29, 2023

tenzen-y deleted the support-scheduler-plugins branch March 29, 2023 05:10

tenzen-y mentioned this pull request Mar 31, 2023

Update podgroups once schedulingPolicy of MPIJobs are changed #542

Merged

Support the coscheduling plugin of scheduler-plugins #538

Support the coscheduling plugin of scheduler-plugins #538

Conversation

tenzen-y commented Mar 20, 2023

tenzen-y commented Mar 20, 2023 • edited Loading

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

tenzen-y Mar 22, 2023 • edited Loading

Choose a reason for hiding this comment

tenzen-y Mar 22, 2023 • edited Loading

Choose a reason for hiding this comment

tenzen-y Mar 22, 2023 • edited Loading

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

alculquicondor commented Mar 22, 2023

tenzen-y commented Mar 22, 2023

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

alculquicondor commented Mar 23, 2023

Choose a reason for hiding this comment

alculquicondor Mar 23, 2023 • edited Loading

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

alculquicondor commented Mar 23, 2023

denkensk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tenzen-y Mar 24, 2023 • edited Loading

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

alculquicondor commented Mar 28, 2023

tenzen-y commented Mar 28, 2023

alculquicondor commented Mar 28, 2023

google-oss-prow bot commented Mar 28, 2023

denkensk commented Mar 29, 2023

denkensk commented Mar 29, 2023

tenzen-y commented Mar 20, 2023 •

edited

Loading

tenzen-y Mar 22, 2023 •

edited

Loading

tenzen-y Mar 22, 2023 •

edited

Loading

tenzen-y Mar 22, 2023 •

edited

Loading

alculquicondor Mar 23, 2023 •

edited

Loading

tenzen-y Mar 24, 2023 •

edited

Loading