Implementation of coscheduling based on CRD [Part 1] #52

cwdsuzhou · 2020-09-16T08:36:02Z

Part 1: we implement the controller and crd
.
/cc @Huang-Wei

denkensk · 2020-09-16T08:52:02Z

We need to separate this PR to two @cwdsuzhou @Huang-Wei

CRD Define + Controller
Use the PodGroup in the current coscheduling implemention (instead of deleteing the current coscheduling directly)

denkensk · 2020-09-16T08:54:52Z

cmd/coscheduling/scheduler/main.go

+	_ "sigs.k8s.io/scheduler-plugins/pkg/apis/config/scheme"
+)
+
+func main() {


We don't need to separate main function for coscheduling.

denkensk · 2020-09-16T08:56:00Z

cmd/coscheduling/controller/controller.go

@@ -0,0 +1,38 @@
+/*


The same as scheduler, we don't need separate controller.

denkensk · 2020-09-16T08:57:18Z

pkg/coscheduling/README.md

@@ -1,7 +1,6 @@
 # Overview

-This folder holds the coscheduling plugin implementations based on [Lightweight coscheduling based on back-to-back queue 
-sorting](https://github.com/kubernetes-sigs/scheduler-plugins/tree/master/kep/2-lightweight-coscheduling).
+This folder holds the coscheduling plugin implementations based on [Coscheduling based on PodGroup CRD](https://github.com/kubernetes-sigs/scheduler-plugins/tree/master/kep/42-podgroup-coscheduling).


Please leave the link of kep https://github.com/kubernetes-sigs/scheduler-plugins/tree/master/kep/2-lightweight-coscheduling here.

denkensk · 2020-09-16T09:00:11Z

pkg/coscheduling/apis/podgroup/v1alpha1/doc.go

@@ -0,0 +1,21 @@
+/*


Move the apis from pkg/coscheduling/apis to pkg/apis/

But I thinks this apis are only for coscheduling

PodGroup is a universal concept. It maybe used by other plugins.

denkensk · 2020-09-16T09:07:47Z

pkg/coscheduling/apis/podgroup/v1alpha1/types.go

+	// MinResources defines the minimal resource of members/tasks to run the pod group;
+	// if there's not enough resources to start all tasks, the scheduler
+	// will not start anyone.
+	MinResources *v1.ResourceList `json:"minResources,omitempty"`


As comments before, MinResource was going to removed.
#42 (comment)

#42 (comment), this it the latest.

I'm confused about how to set the MinResource. It's the total minimal resource of the PodGroup or the pod?
yes

Yes？ the PodGroup or the pod？

I'm confused about how to set the MinResource. It's the total minimal resource of the PodGroup or the pod?
yes

Yes？ the PodGroup or the pod？

We have discussed about this question offline.

minimal resource of PodGroup

Ok. It's better to change like this:

// MinResources defines the minimal resource of members/tasks to run the pod group; ---> // MinResources defines the minimal resource to run the pod group;

cwdsuzhou · 2020-09-16T09:47:16Z

We need to separate this PR to two @cwdsuzhou @Huang-Wei

CRD Define + Controller

Use the PodGroup in the current coscheduling implemention (instead of deleteing the current coscheduling directly)

Too many difference between the existing version and this one. The existing version even can not pass the integration test. If you do not like this, we would like move it to an independent pkg named coscheduling-with-crd. Actually we just use similar interface, detailed implementations are absolutely different.

denkensk · 2020-09-16T10:05:19Z

Please separate this PR to two firstly.
CRD Define + Controller
/hold

Huang-Wei

Thanks for the PR @cwdsuzhou !

A high-level comment is that I'd still prefer not to mixin the implementation into this PR, i.e., it's ideal to have 2 PRs - one for CRD and controller, the 2nd one to include the implementation code. Additionally, the code structure needs to be tweaked a bit as we won't generate binaries for each plugin - all plugins will be compiled into particular type of binaries (scheduler, controller, admission, etc.).

With that said, here are my comments:

Retitle the PR to "Implementation of coscheduling based on CRD [Part 1]"
Move cmd/main.go and cmd/main_test.go to a new pkg cmd/scheduler/.
Make cmd/coscheduling/controller a generic controller, i.e., move cmd/coscheduler/controller/* to cmd/controller
Move pkg/coscheduling/apis/podgroup to pkg/apis/podgroup as PodGroup is a generic API, and hence may consumed by multiple plugins. It also follows the upstream code structure.
pkg/coscheduling/controller/* should also go to pkg/controller, and named as podgroup.go.
Move pkg/coscheduling/generated to pkg/generated
IMO pkg/coscheduling/scheduler/*, along with pkg/coscheduling/coscheduler.go can be introduced in the next PR.
Add tests to test PodGroup and controller logic only.
Modify Dockerfile and Makefile based on latest codebase.

FYI: I've exercised some (not all) of the above comments, you can supplement based on it:

Huang-Wei@13a7897 (CRD definition and controller logic)
Huang-Wei@b0ac55e (codegen)

cwdsuzhou · 2020-09-17T01:55:33Z

Thanks for the PR @cwdsuzhou !

A high-level comment is that I'd still prefer not to mixin the implementation into this PR, i.e., it's ideal to have 2 PRs - one for CRD and controller, the 2nd one to include the implementation code. Additionally, the code structure needs to be tweaked a bit as we won't generate binaries for each plugin - all plugins will be compiled into particular type of binaries (scheduler, controller, admission, etc.).

With that said, here are my comments:

Retitle the PR to "Implementation of coscheduling based on CRD [Part 1]"

Move cmd/main.go and cmd/main_test.go to a new pkg cmd/scheduler/.

Make cmd/coscheduling/controller a generic controller, i.e., move cmd/coscheduler/controller/* to cmd/controller

Move pkg/coscheduling/apis/podgroup to pkg/apis/podgroup as PodGroup is a generic API, and hence may consumed by multiple plugins. It also follows the upstream code structure.

pkg/coscheduling/controller/* should also go to pkg/controller, and named as podgroup.go.

Move pkg/coscheduling/generated to pkg/generated

IMO pkg/coscheduling/scheduler/*, along with pkg/coscheduling/coscheduler.go can be introduced in the next PR.

Add tests to test PodGroup and controller logic only.

Modify Dockerfile and Makefile based on latest codebase.

FYI: I've exercised some (not all) of the above comments, you can supplement based on it:

Huang-Wei@13a7897 (CRD definition and controller logic)

Huang-Wei@b0ac55e (codegen)

Thanks!

Huang-Wei

Just ignore the flaky test, #54 is created for tracking that.

cmd/scheduler/main.go

pkg/util/k8s.go

cmd/controller/app/server.go

pkg/controller/podgroup_test.go

cwdsuzhou · 2020-09-18T02:47:53Z

PR updated according to your suggestions @Huang-Wei

Huang-Wei · 2020-09-18T19:56:07Z

pkg/apis/podgroup/v1alpha1/types.go

+	// +optional
+	metav1.ListMeta `json:"metadata,omitempty"`
+
+	// items is the list of PodGroup


Suggested change

// items is the list of PodGroup

// Items is the list of PodGroup

Huang-Wei · 2020-09-18T19:57:49Z

There is one comment unresolved: #52 (comment). LGTM otherwise.

@denkensk do you want to take another round of review?

denkensk · 2020-09-19T15:00:56Z

pkg/util/types.go

+
+const (
+	// PodGroupLabel is the default label of batch scheduler
+	PodGroupLabel = "podgroup.scheduling.sigs.k8s.io"


podgroup.scheduling.sigs.k8s.io --> pod-group.scheduling.sigs.k8s.io

IMO， podgroup is alose OK. WDYT

pod-group conforms to the naming standard of k8s. We talk about it before.

Hope to be consistent with the current implementation of the naming

denkensk · 2020-09-19T15:02:18Z

pkg/util/types.go

+
+var (
+	// ErrorNotMatched means pod does not match batch scheduling
+	ErrorNotMatched = fmt.Errorf("not match batch scheduling")


not match batch scheduling --> not match coscheduling

denkensk · 2020-09-19T15:05:48Z

pkg/apis/podgroup/v1alpha1/types.go

+	Failed uint32 `json:"failed,omitempty"`
+
+	// ScheduleStartTime of the group
+	ScheduleStartTime metav1.Time `json:"scheduleStartTime,omitempty"`


ScheduleStartTime is the create time of PG ?
If So, it's better to rename it to CreateTime?

This is field just record when the group starts scheduling

Thanks, It also need to add the field to record the create time of PG.

MetaData of CRD already includes this field!

https://github.com/kubernetes-sigs/scheduler-plugins/pull/52/files#diff-23885a062c430d62621c160ab8aaaf08R66

Change codes according to round 1 suggestions Update PR based on suggestions

Huang-Wei · 2020-09-21T17:18:55Z

/approve

leaving /lgtm to @denkensk

k8s-ci-robot · 2020-09-21T17:19:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cwdsuzhou, Huang-Wei

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~~ [Huang-Wei]

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

cwdsuzhou · 2020-09-22T02:06:02Z

ping @denkensk

cwdsuzhou · 2020-09-22T03:13:33Z

/unhold

cwdsuzhou · 2020-09-22T03:18:25Z

@denkensk @Huang-Wei

seems not work. May be you should left only /lgtm ?

denkensk · 2020-09-22T03:23:10Z

/lgtm

k8s-ci-robot requested a review from Huang-Wei September 16, 2020 08:36

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 16, 2020

cwdsuzhou force-pushed the master branch from f6aa1d9 to e659239 Compare September 16, 2020 08:39

denkensk reviewed Sep 16, 2020

View reviewed changes

cwdsuzhou force-pushed the master branch from e659239 to ffe6f30 Compare September 16, 2020 09:55

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 16, 2020

Huang-Wei reviewed Sep 16, 2020

View reviewed changes

cwdsuzhou changed the title ~~Implementation of coscheduling based on CRD~~ Implementation of coscheduling based on CRD [Part 1] Sep 17, 2020

cwdsuzhou force-pushed the master branch from ffe6f30 to 752f9a7 Compare September 17, 2020 12:33

cwdsuzhou requested a review from Huang-Wei September 17, 2020 12:33

cwdsuzhou force-pushed the master branch 2 times, most recently from a721279 to b8c5545 Compare September 17, 2020 12:57

Huang-Wei reviewed Sep 18, 2020

View reviewed changes

cwdsuzhou force-pushed the master branch 2 times, most recently from 2c00efe to 0ad3a3a Compare September 18, 2020 02:46

cwdsuzhou requested a review from Huang-Wei September 18, 2020 02:47

Huang-Wei reviewed Sep 18, 2020

View reviewed changes

cwdsuzhou requested a review from Huang-Wei September 18, 2020 23:07

denkensk reviewed Sep 19, 2020

View reviewed changes

cwdsuzhou force-pushed the master branch from b963e4a to b22b66b Compare September 20, 2020 07:01

Implemetaion of coscheduling base on crd[Part 1]: controller and crd

aeb842c

Change codes according to round 1 suggestions Update PR based on suggestions

cwdsuzhou force-pushed the master branch from b22b66b to aeb842c Compare September 21, 2020 06:21

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 21, 2020

cwdsuzhou requested a review from denkensk September 22, 2020 02:05

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 22, 2020

k8s-ci-robot assigned denkensk Sep 22, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 22, 2020

k8s-ci-robot merged commit 96fd011 into kubernetes-sigs:master Sep 22, 2020

Huang-Wei mentioned this pull request Sep 22, 2020

Official Docker Container Image #47

Closed

cwdsuzhou mentioned this pull request Sep 25, 2020

Implementation of coscheduling base on crd[Part 2]: coscheduling refactor-must have #57

Merged

	// items is the list of PodGroup
	// Items is the list of PodGroup

Implementation of coscheduling based on CRD [Part 1] #52

Implementation of coscheduling based on CRD [Part 1] #52

Conversation

cwdsuzhou commented Sep 16, 2020 • edited Loading

denkensk commented Sep 16, 2020

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

cwdsuzhou commented Sep 16, 2020

denkensk commented Sep 16, 2020

Huang-Wei left a comment

Choose a reason for hiding this comment

cwdsuzhou commented Sep 17, 2020

Huang-Wei left a comment

Choose a reason for hiding this comment

cwdsuzhou commented Sep 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Huang-Wei commented Sep 18, 2020

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

Huang-Wei commented Sep 21, 2020

k8s-ci-robot commented Sep 21, 2020

cwdsuzhou commented Sep 22, 2020

cwdsuzhou commented Sep 22, 2020

cwdsuzhou commented Sep 22, 2020

denkensk commented Sep 22, 2020

cwdsuzhou commented Sep 16, 2020 •

edited

Loading