add admitPod and PGController #165

wangyuqing4 · 2019-05-15T02:14:38Z

Which issue(s) this PR fixes :
Fixes #135 #134

Special notes for your reviewer:

new func AdmitPod in admission controller
new PGcontroller in controller
delete Inqueue job phase
fix UT

Release note:


1. add ValidatingWebhookConfiguration volcano-validate-pod, only limit CREATE pods, allow pods to create when:
- pod.spec.schedulerName is default-scheduler
- podgroup phase isn't Pending
- normal job, no podgroup

2. new PGcontroller, create pg for normal job when use kube-batch.

3. if create job, job phase will be Pending->Running...... , so fix UT

k82cn · 2019-05-15T02:51:24Z

/cc @TommyLike @hzxuzhonghu

hzxuzhonghu · 2019-05-15T02:59:07Z

Have not yet look at the pr, serveral question：

new PGcontroller, create pg for normal job when use kube-batch

Is this for other users instead of volcano job controller? If the user do not use volcano job, how should he disable the job controller in volcano？

if create job, job phase will be Pending->Running...... , so fix UT

Missing the background, just remember the inqueue phase is added days ago, why remove it ?

wangyuqing4 · 2019-05-15T03:25:58Z

user can use other controller to create pod
the pr only delete job inqueue phase，pg inqueue phase is retained, can simplify job state convert

wangyuqing4 · 2019-05-15T06:23:50Z

/cc @lminzhw

TommyLike · 2019-05-15T06:57:40Z

In order not to refactor this again, I think we need some clarifications/discussions on why we need do in this way. pasted as below, hope someone can explain this.

Why we need admission to prevent creating pod? that sounds like part of the sync work, and can be easily moved into our controller logic, Place it here will make the controller keeping creating pods and deny, again and again. What's the benefits from using admission service here?
PodGroupController sounds more like (original)JobController here, we need to watch job and create podgroup if needed.

@k82cn @wangyuqing4 @hzxuzhonghu

k82cn · 2019-05-15T07:02:15Z

xref #135

Currently, we delay pod creattion in job controller which make it hard for two scenarios:

vk.job can not work with other scheduler

enqueue can not support other operators

TommyLike · 2019-05-15T07:08:39Z

xref #135

Currently, we delay pod creattion in job controller which make it hard for two scenarios:

vk.job can not work with other scheduler

enqueue can not support other operators

This can be addressed by removing enqueue state plus waiting for podgroup status if it's a volcano job while keep creating for the others. Anything missing?

hzxuzhonghu · 2019-05-15T07:09:17Z

PodGroupController sounds more like (original)JobController here, we need to watch job and create podgroup if needed.

Same as what I said, if this is for users who are only using kube-batch, this should be a separate component. I donot see it is necessary in volcano.

BTW, I guess the first question is also for kube-batch user only.

k82cn · 2019-05-15T09:47:03Z

Same as what I said, if this is for users who are only using kube-batch, this should be a separate component. I donot see it is necessary in volcano.

hm... 'kube-batch' is part of volcano. Volcano should be open for others who only use scheduler part, e.g. kubeflow.

This can be addressed by removing enqueue state plus waiting for podgroup status if it's a volcano job while keep creating for the others. Anything missing?

not sure how that works :(, we should not bind controller & scheduler.

TommyLike · 2019-05-15T10:01:24Z

Same as what I said, if this is for users who are only using kube-batch, this should be a separate component. I donot see it is necessary in volcano.

hm... 'kube-batch' is part of volcano. Volcano should be open for others who only use scheduler part, e.g. kubeflow.

This can be addressed by removing enqueue state plus waiting for podgroup status if it's a volcano job while keep creating for the others. Anything missing?

not sure how that works :(, we should not bind controller & scheduler.

hmm with a quick thought, the pseudo code would be:

function syncJob(job) {
//1. Create job related resources, for example, service, plugin, configmap
//2. Create pods logic as below.
if IsVolcanoJob() && ! IsPodGroupEnqueue(job) {
   // Skip creating pods
} else {
  //Create pods as required.
}
}

hzxuzhonghu · 2019-05-15T11:12:21Z

hm... 'kube-batch' is part of volcano. Volcano should be open for others who only use scheduler part, e.g. kubeflow.

I know, i mean, if others not use Volcano Job, why do we enforce them to deploy all the volcano controllers? We should allow to specify which controller to start.

k82cn · 2019-05-15T11:23:45Z

why do we enforce them to deploy all the volcano controllers? We should allow to specify which controller to start.

I'd like to control the number of binaries; that should be ok if they do not create volcano Job by yaml file :)

k82cn · 2019-05-15T11:36:50Z

hmm with a quick thought, the pseudo code would be:

PodGroupController does not create pods.

TommyLike · 2019-05-15T14:00:20Z

why do we enforce them to deploy all the volcano controllers? We should allow to specify which controller to start.

I'd like to control the number of binaries; that should be ok if they do not create volcano Job by yaml file :)

hmm with a quick thought, the pseudo code would be:

PodGroupController does not create pods.

I was mean in the job controller, but I think I get the point now, the new added logic in admission service is used to block ANY pods that are created with non-default scheduler? This sounds like a pure kube-batch logic but we address this in volcano?

k82cn · 2019-05-15T14:24:55Z

kube-batch is part of volcano!!! * 3 :)

TommyLike

Thanks

pkg/controllers/podgroup/pg_controller_handler.go

pkg/controllers/podgroup/pg_controller.go

cmd/controllers/app/server.go

pkg/controllers/job/job_controller_actions.go

cmd/controllers/app/options/options.go

pkg/admission/admit_pod.go

pkg/apis/helpers/helpers.go

k82cn · 2019-05-17T00:21:19Z

please resolve the conflict.

installer/chart/volcano/templates/admission-config.yaml

TravisBuddy · 2019-07-15T04:46:44Z

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: 8ac87940-a6bb-11e9-a3be-7d74802580dc

TommyLike · 2019-07-16T08:56:56Z

lgtm, but we need other reviews since it's a huge change.

/assign @hzxuzhonghu

k82cn · 2019-07-16T10:05:57Z

cmd/admission/app/configure/configure.go

@@ -44,6 +44,7 @@ type Config struct {
 	PrintVersion              bool
 	AdmissionServiceName      string
 	AdmissionServiceNamespace string
+	SchedulerName             string


it's better to be an label selector; schedulerName is not enough.

hzxuzhonghu · 2019-07-16T12:39:37Z

cmd/admission/app/configure/configure.go

@@ -177,6 +183,42 @@ func RegisterWebhooks(c *Config, clienset *kubernetes.Clientset, cabundle []byte
 		return err
 	}

+	// Prepare validate pods
+	path = "/pods"


suggest /validating-pods, should consider if we need mutating in the future

like job，

cmd/admission/app/server.go

hack/e2e-admission-config.yaml

installer/helm/chart/volcano/values.yaml

pkg/admission/admission_controller.go

hzxuzhonghu · 2019-07-16T15:14:34Z

pkg/controllers/podgroup/pg_controller.go

+						return true
+					}
+					return false
+				default:


~~There is another case, when delete event miss, please refer to DeletedFinalStateUnknown~~

nvm， it only handles pod add

pkg/controllers/podgroup/pg_controller.go

hzxuzhonghu · 2019-07-16T15:21:54Z

pkg/controllers/podgroup/pg_controller_handler.go

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


do not understand why set pg's OwnerReferences same with pod's

show the pg and pod belong the same thing

hzxuzhonghu · 2019-07-16T15:27:33Z

pkg/controllers/podgroup/pg_controller_handler.go

+			},
+		}
+
+		if _, err := cc.kbClients.SchedulingV1alpha1().PodGroups(pod.Namespace).Create(pg); err != nil {


previously PG is created by job controller, right?

Why also create it here? If a pod already created, is there a need to create a PG with MinMember equals 1? The pod maybe already running.

in the queue, isn't vk job but use volcano scheduler, so create pg here.

test/e2e/admission.go

hzxuzhonghu · 2019-07-17T03:58:13Z

overall lgtm

cmd/controllers/app/options/options.go

pkg/admission/admit_pod.go

volcano-sh-bot · 2019-07-17T06:22:19Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: hzxuzhonghu, wangyuqing4
To complete the pull request process, please assign hex108
You can assign the PR to them by writing /assign @hex108 in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

hzxuzhonghu · 2019-07-19T07:30:46Z

@k82cn any other comments

k82cn · 2019-07-20T01:57:02Z

pkg/controllers/job/job_controller_actions.go

 	if job.Status.State.Phase != "" {
-		return job, nil
+		return false, nil


can we remove bool?

the func initJobStatus in createJob need UpdateStatus, if remove bool, will go on UpdateStatus and return err

pkg/controllers/podgroup/pg_controller.go

k82cn · 2019-07-20T02:27:03Z

let split this huge PR into some smaller ones:

PodGroup Controller
Remove ShadowPodGroup in scheduler
Add admission controller
Remove inqueue state in job & add e2e test for deployment

Each PR should include UT for it.

volcano-sh-bot · 2019-07-20T03:09:24Z

@wangyuqing4: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

wangyuqing4 force-pushed the adm branch from 287c2f7 to dfa0428 Compare May 15, 2019 02:31

volcano-sh-bot requested review from hzxuzhonghu and TommyLike May 15, 2019 02:51

wangyuqing4 force-pushed the adm branch from dfa0428 to 11adb0b Compare May 15, 2019 06:17

wangyuqing4 force-pushed the adm branch from 5424a07 to b93f643 Compare May 16, 2019 01:32

TommyLike reviewed May 16, 2019

View reviewed changes

wangyuqing4 force-pushed the adm branch from c8bc560 to 22a1057 Compare May 16, 2019 11:35

k82cn reviewed May 16, 2019

View reviewed changes

cmd/controllers/app/options/options.go Outdated Show resolved Hide resolved

k82cn reviewed May 16, 2019

View reviewed changes

pkg/admission/admit_pod.go Outdated Show resolved Hide resolved

k82cn reviewed May 16, 2019

View reviewed changes

pkg/apis/helpers/helpers.go Outdated Show resolved Hide resolved

wangyuqing4 force-pushed the adm branch from 22a1057 to ec0cb82 Compare May 20, 2019 12:04

wangyuqing4 mentioned this pull request May 20, 2019

remove createShadowPodGroup volcano-retired/scheduler#18

Closed

lminzhw reviewed May 21, 2019

View reviewed changes

installer/chart/volcano/templates/admission-config.yaml Outdated Show resolved Hide resolved

volcano-sh-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 27, 2019

wangyuqing4 force-pushed the adm branch from d157d54 to 9e82c1f Compare July 15, 2019 03:31

volcano-sh-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 15, 2019

wangyuqing4 force-pushed the adm branch from 9e82c1f to a524104 Compare July 15, 2019 04:16

volcano-sh-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 15, 2019

wangyuqing4 force-pushed the adm branch from a524104 to cfc16dc Compare July 16, 2019 06:29

volcano-sh-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 16, 2019

wangyuqing4 force-pushed the adm branch from cfc16dc to 5cf421d Compare July 16, 2019 06:35

volcano-sh-bot assigned hzxuzhonghu Jul 16, 2019

k82cn reviewed Jul 16, 2019

View reviewed changes

hzxuzhonghu suggested changes Jul 16, 2019

View reviewed changes

hzxuzhonghu reviewed Jul 17, 2019

View reviewed changes

test/e2e/admission.go Show resolved Hide resolved

add admitPod and PGController

52d08de

k82cn reviewed Jul 17, 2019

View reviewed changes

cmd/controllers/app/options/options.go Show resolved Hide resolved

k82cn reviewed Jul 17, 2019

View reviewed changes

pkg/admission/admit_pod.go Outdated Show resolved Hide resolved

wangyuqing4 force-pushed the adm branch from 5cf421d to 52d08de Compare July 17, 2019 04:15

hzxuzhonghu approved these changes Jul 17, 2019

View reviewed changes

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

k82cn reviewed Jul 20, 2019

View reviewed changes

pkg/controllers/podgroup/pg_controller.go Show resolved Hide resolved

volcano-sh-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 20, 2019

lminzhw mentioned this pull request Jul 22, 2019

added enqueue action threshold in sched config #371

Closed

hzxuzhonghu mentioned this pull request Jul 31, 2019

add podgroup controller #401

Merged

wangyuqing4 closed this Aug 19, 2019

add admitPod and PGController #165

add admitPod and PGController #165

Conversation

wangyuqing4 commented May 15, 2019 • edited Loading

k82cn commented May 15, 2019

hzxuzhonghu commented May 15, 2019

wangyuqing4 commented May 15, 2019

wangyuqing4 commented May 15, 2019

TommyLike commented May 15, 2019

k82cn commented May 15, 2019

TommyLike commented May 15, 2019 • edited Loading

hzxuzhonghu commented May 15, 2019

k82cn commented May 15, 2019

TommyLike commented May 15, 2019

hzxuzhonghu commented May 15, 2019

k82cn commented May 15, 2019

k82cn commented May 15, 2019

TommyLike commented May 15, 2019

k82cn commented May 15, 2019

TommyLike left a comment

Choose a reason for hiding this comment

k82cn commented May 17, 2019

TravisBuddy commented Jul 15, 2019

TravisBuddy Request Identifier: 8ac87940-a6bb-11e9-a3be-7d74802580dc

TommyLike commented Jul 16, 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

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 commented Jul 17, 2019

volcano-sh-bot commented Jul 17, 2019

hzxuzhonghu commented Jul 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k82cn commented Jul 20, 2019

volcano-sh-bot commented Jul 20, 2019

wangyuqing4 commented May 15, 2019 •

edited

Loading

TommyLike commented May 15, 2019 •

edited

Loading