add podgroup controller #401

wangyuqing4 · 2019-07-31T04:01:15Z

Fixes #134

if normal pod/job/...... use volcano scheduler, podgroup controller watch pod, can create podgroup.

hzxuzhonghu · 2019-07-31T04:13:04Z

Can u add desc about the relation with #165 and #370 ? I donot know which one to review

wangyuqing4 · 2019-07-31T04:17:24Z

#165 is huge, so split smaller one, #401 is part 1, #370 is part 3. @hzxuzhonghu you can review #401 first.

TravisBuddy · 2019-07-31T04:39:48Z

Hey @wangyuqing4,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: 395cfd80-b34d-11e9-a522-656c855f12dd

hzxuzhonghu · 2019-07-31T06:37:30Z

ok

k82cn · 2019-07-31T06:57:31Z

pkg/controllers/podgroup/pg_controller.go

+				switch obj.(type) {
+				case *v1.Pod:
+					pod := obj.(*v1.Pod)
+					if pod.Spec.SchedulerName == "volcano" &&


We should not hard code scheduler name here.

hzxuzhonghu · 2019-07-31T12:01:15Z

pkg/controllers/podgroup/pg_controller.go

+		queue: workqueue.NewRateLimitingQueue(workqueue.DefaultControllerRateLimiter()),
+	}
+
+	cc.sharedInformers = informers.NewSharedInformerFactory(cc.kubeClients, 0)


One point: we should prevent duplicate informers in different controllers. It is very costly

ok，follow-up pr will optimize the part，gc/queue/job/pg controller will delete same code.

agreed, please file an issue to track

hzxuzhonghu

I think overall lgtm, but some nits

hzxuzhonghu · 2019-08-01T02:07:26Z

pkg/controllers/podgroup/pg_controller.go

+func (cc *Controller) Run(stopCh <-chan struct{}) {
+	go cc.sharedInformers.Start(stopCh)
+	go cc.podInformer.Informer().Run(stopCh)
+	go cc.pgInformer.Informer().Run(stopCh)


We donot need Run informers separately as sharedInformers Start will Run the informers created from informer factory.

Same happens to other controllers

hzxuzhonghu · 2019-08-01T02:11:42Z

pkg/controllers/podgroup/pg_controller_handler.go

+	}
+
+	req := podRequest{
+		pod: pod,


Actually, you just need the pod ns/name

hzxuzhonghu · 2019-08-01T02:32:17Z

pkg/controllers/podgroup/pg_controller.go

+func (cc *Controller) Run(stopCh <-chan struct{}) {
+	go cc.sharedInformers.Start(stopCh)
+	go cc.podInformer.Informer().Run(stopCh)
+	go cc.pgInformer.Informer().Run(stopCh)


cc.sharedInformers.Star will run all its informers.

hzxuzhonghu · 2019-08-01T02:32:35Z

pkg/controllers/podgroup/pg_controller_handler.go

+)
+
+type podRequest struct {
+	pod *v1.Pod


only need pod ns/name

We can not use pointer here: the same pod may have different object in cache.

k82cn · 2019-08-01T02:54:46Z

pkg/controllers/podgroup/pg_controller.go

+// Run start NewPodgroupController
+func (cc *Controller) Run(stopCh <-chan struct{}) {
+	go cc.sharedInformers.Start(stopCh)
+	go cc.podInformer.Informer().Run(stopCh)


if start sharedInformer, it's not necessary to start pod informer again.

same thing happens to other controllers

k82cn · 2019-08-01T03:02:42Z

pkg/controllers/podgroup/pg_controller_handler.go

+	if pod.Annotations[scheduling.GroupNameAnnotationKey] == "" {
+		pod.Annotations[scheduling.GroupNameAnnotationKey] = pgName
+	} else {
+		return nil


log an error message if pod.Annotations[scheduling.GroupNameAnnotationKey] != pgName.

k82cn · 2019-08-01T03:06:35Z

pkg/controllers/podgroup/pg_controller_handler.go

+			ObjectMeta: metav1.ObjectMeta{
+				Namespace:       pod.Namespace,
+				Name:            pgName,
+				OwnerReferences: pod.OwnerReferences,


if pod's ownerReferences is empty, set PodGroup's ower references to this Pod for GC.

k82cn · 2019-08-01T03:22:06Z

pkg/controllers/podgroup/pg_controller_handler.go

+func generatePodgroupName(pod *v1.Pod) string {
+	pgName := vkbatchv1.PodgroupNamePrefix
+	if len(pod.OwnerReferences) != 0 {
+		pgName += string(pod.OwnerReferences[0].UID)


we should find the controlled owner

TravisBuddy · 2019-08-01T09:22:27Z

Hey @wangyuqing4,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: e001cd50-b43d-11e9-a5f8-ad46f5ea24b0

k82cn · 2019-08-01T09:41:50Z

pkg/controllers/podgroup/pg_controller_handler.go

+
+func newPGOwnerReferences(pod *v1.Pod) []metav1.OwnerReference {
+	if len(pod.OwnerReferences) != 0 {
+		return pod.OwnerReferences


there maybe not controlled ownerference there.

hzxuzhonghu · 2019-08-01T10:16:26Z

/lgtm

k82cn · 2019-08-01T12:18:18Z

/approve

volcano-sh-bot · 2019-08-01T12:18:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: k82cn, wangyuqing4

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

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

volcano-sh-bot requested review from hex108 and TommyLike July 31, 2019 04:01

volcano-sh-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jul 31, 2019

k82cn reviewed Jul 31, 2019

View reviewed changes

wangyuqing4 force-pushed the pg-ctrl branch from 0a80073 to 5a8dcc6 Compare July 31, 2019 08:58

hzxuzhonghu reviewed Jul 31, 2019

View reviewed changes

hzxuzhonghu reviewed Aug 1, 2019

View reviewed changes

k82cn reviewed Aug 1, 2019

View reviewed changes

wangyuqing4 force-pushed the pg-ctrl branch from 5a8dcc6 to d7b8df6 Compare August 1, 2019 08:40

k82cn reviewed Aug 1, 2019

View reviewed changes

add podgroup controller

d055c00

wangyuqing4 force-pushed the pg-ctrl branch from d7b8df6 to d055c00 Compare August 1, 2019 10:01

volcano-sh-bot assigned hzxuzhonghu Aug 1, 2019

volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 1, 2019

volcano-sh-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 1, 2019

volcano-sh-bot merged commit 7243054 into volcano-sh:master Aug 1, 2019

wangyuqing4 mentioned this pull request Aug 3, 2019

remove shadowPodgroup in scheduler #406

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add podgroup controller #401

add podgroup controller #401

wangyuqing4 commented Jul 31, 2019

hzxuzhonghu commented Jul 31, 2019

wangyuqing4 commented Jul 31, 2019 •

edited

Loading

TravisBuddy commented Jul 31, 2019

hzxuzhonghu commented Jul 31, 2019

k82cn Jul 31, 2019

wangyuqing4 Jul 31, 2019

hzxuzhonghu Jul 31, 2019

wangyuqing4 Aug 1, 2019

hzxuzhonghu Aug 1, 2019

hzxuzhonghu left a comment

hzxuzhonghu Aug 1, 2019

hzxuzhonghu Aug 1, 2019

hzxuzhonghu Aug 1, 2019

hzxuzhonghu Aug 1, 2019

hzxuzhonghu Aug 1, 2019

k82cn Aug 1, 2019

k82cn Aug 1, 2019

hzxuzhonghu Aug 1, 2019

k82cn Aug 1, 2019

k82cn Aug 1, 2019

k82cn Aug 1, 2019

TravisBuddy commented Aug 1, 2019

k82cn Aug 1, 2019

hzxuzhonghu commented Aug 1, 2019

k82cn commented Aug 1, 2019

volcano-sh-bot commented Aug 1, 2019

add podgroup controller #401

add podgroup controller #401

Conversation

wangyuqing4 commented Jul 31, 2019

hzxuzhonghu commented Jul 31, 2019

wangyuqing4 commented Jul 31, 2019 • edited Loading

TravisBuddy commented Jul 31, 2019

TravisBuddy Request Identifier: 395cfd80-b34d-11e9-a522-656c855f12dd

hzxuzhonghu commented Jul 31, 2019

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

hzxuzhonghu left a 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

TravisBuddy commented Aug 1, 2019

TravisBuddy Request Identifier: e001cd50-b43d-11e9-a5f8-ad46f5ea24b0

Choose a reason for hiding this comment

hzxuzhonghu commented Aug 1, 2019

k82cn commented Aug 1, 2019

volcano-sh-bot commented Aug 1, 2019

wangyuqing4 commented Jul 31, 2019 •

edited

Loading