Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add admitPod #370

Merged
merged 1 commit into from
Aug 8, 2019
Merged

add admitPod #370

merged 1 commit into from
Aug 8, 2019

Conversation

wangyuqing4
Copy link
Contributor

@wangyuqing4 wangyuqing4 commented Jul 20, 2019 •
edited

Fixes: #135

Add admission controller(admitPod)

add ValidatingWebhookConfiguration volcano-validate-pod, only limit CREATE pods, allow pods to create when:

  • pod.spec.schedulerName is not volcano
  • podgroup phase isn't Pending
  • normal job, no podgroup

@volcano-sh-bot volcano-sh-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jul 20, 2019
@TommyLike
Copy link
Contributor

@k82cn @hzxuzhonghu is this ready to go?

@volcano-sh-bot volcano-sh-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 24, 2019
@hzxuzhonghu
Copy link
Collaborator

We need description about what it does. So it is easy for us to trace new features on release note.

@hzxuzhonghu hzxuzhonghu mentioned this pull request Jul 31, 2019
Merged
@volcano-sh-bot volcano-sh-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 5, 2019
@wangyuqing4 wangyuqing4 force-pushed the admission-ctrl branch 2 times, most recently from 069593a to 43606fb Compare August 6, 2019 01:24
@k82cn
Copy link
Member

k82cn commented Aug 6, 2019

@hzxuzhonghu @TommyLike , please help to review :)

hzxuzhonghu
hzxuzhonghu reviewed Aug 6, 2019
View reviewed changes
cmd/admission/main.go
admissioncontroller.KubeBatchClientSet = app.GetKubeBatchClient(restConfig)
admissioncontroller.VolcanoClientSet = app.GetVolcanoClient(restConfig)

servePods(config)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why donot you keep consistent with L54-55?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because pods need client and parameter。

hzxuzhonghu
hzxuzhonghu reviewed Aug 6, 2019
View reviewed changes
pkg/admission/admit_pod.go Outdated

// allow pods to create when
// 1. pod.spec.schedulerName != volcano
// 2. Podgroup phase isn't Pending
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add more comments why do this?

@hzxuzhonghu
Copy link
Collaborator

Actually, i am not sure why need to delay pod creation? Can you elaborate a little bit more?

TommyLike
TommyLike reviewed Aug 6, 2019
View reviewed changes
pkg/admission/admit_pod.go Outdated

// normal pod, SN == volcano
pgName = helpers.GeneratePodgroupName(&pod)
if pgName != "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when this will happen?

if pgName != "" {
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

em，i will delete this if

TommyLike
TommyLike reviewed Aug 6, 2019
View reviewed changes
pkg/admission/admit_pod.go Outdated
if pg.Status.Phase != v1alpha2.PodGroupPending {
return nil
}
return fmt.Errorf("Failed to create pod for pod <%s/%s>, because the podgroup phase is Pending",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

Failed to create pod <%s/%s>

@TravisBuddy
Copy link

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: 05199e50-b845-11e9-af6e-3b363318fc0d

@k82cn
Copy link
Member

k82cn commented Aug 8, 2019

/lgtm
/approve

@volcano-sh-bot volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 8, 2019
@volcano-sh-bot
Copy link
Contributor

[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:

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 volcano-sh-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 8, 2019
@volcano-sh-bot volcano-sh-bot merged commit 566309a into volcano-sh:master Aug 8, 2019
@wangyuqing4 wangyuqing4 mentioned this pull request Aug 19, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TommyLike TommyLike TommyLike left review comments

@hzxuzhonghu hzxuzhonghu hzxuzhonghu left review comments

@quinton-hoole quinton-hoole Awaiting requested review from quinton-hoole

Assignees

@k82cn k82cn

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor Delay Pod Creation by admission controller
6 participants
@wangyuqing4 @TommyLike @hzxuzhonghu @k82cn @TravisBuddy @volcano-sh-bot