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.

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

Separate pod priority from preemption #62243

Merged
merged 2 commits into from Apr 19, 2018

Conversation

@resouer
Member

resouer commented Apr 8, 2018

What this PR does / why we need it:
Users request to split priority and preemption feature gate so they can use priority separately.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #62068

Special notes for your reviewer:

I kept use ENABLE_POD_PRIORITY as ENV name for gce cluster scripts for backward compatibility reason. Please let me know if other approach is preffered.

This is a potential break change as existing clusters will be affected, we may need to include this in 1.11 maybe?

TODO: update this doc https://kubernetes.io/docs/concepts/configuration/pod-priority-preemption/

[Update] Usage: in config file for scheduler:

apiVersion: componentconfig/v1alpha1
kind: KubeSchedulerConfiguration
...
disablePreemption: true

Release note:

Split PodPriority and PodPreemption feature gate
@CaoShuFeng

This comment has been minimized.

Member

CaoShuFeng commented Apr 10, 2018

This is a potential break change as existing clusters will be affected, we may need to include this in 1.11 maybe?

What about we do not introduce a new featrue gate, but add a new argument to kube-controller-manager kube-scheduler, to disable preemption?
like

--disable-pod-preemption
// alpha: v1.8
//
// Allow preemption of pods with lower priority. Only work with PodPriority is enabled.
PodPreemption utilfeature.Feature = "PodPreemption"

This comment has been minimized.

@bsalamat

bsalamat Apr 10, 2018

Contributor

Enabling preemption without enabling priority is wrong configuration. I think we should check and throw error if it happens. I don't know if we have any standard location in our codebase to check for such dependencies of feature gates.

This comment has been minimized.

@resouer

resouer Apr 11, 2018

Member

Thanks, I will make it a scheduler configuration.

@bsalamat

This comment has been minimized.

Contributor

bsalamat commented Apr 10, 2018

What about we do not introduce a new featrue gate, but add a new argument to kube-controller-manager, to disable preemption?

I like this idea. We can then consider preemption enabled only when pod priority is enabled and preemption is not explicitly disabled. It prevents bad configurations where priority is disabled and preemption is enabled.

@resouer

This comment has been minimized.

Member

resouer commented Apr 10, 2018

What about we do not introduce a new feature gate, but add a new argument to kube-controller-manager, to disable preemption?

Sorry, but ... why kube-controller-manager? shouldn't this be part of scheduler configure?

@CaoShuFeng

This comment has been minimized.

Member

CaoShuFeng commented Apr 11, 2018

Sorry, but ... why kube-controller-manager? shouldn't this be part of scheduler configure?

kube-scheduler, I wrote it incorrectly. 😄

@bsalamat

This comment has been minimized.

Contributor

bsalamat commented Apr 13, 2018

Thanks, Harry. Looks generally good, but could you please add a test for the case that preemption is disabled?

@resouer

This comment has been minimized.

Member

resouer commented Apr 13, 2018

@bsalamat Thanks for review, will do.

@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Apr 15, 2018

@resouer

This comment has been minimized.

Member

resouer commented Apr 15, 2018

/test pull-kubernetes-e2e-kops-aws

if err != nil {
t.Fatalf("Error creating nodes: %v", err)
}
nodeLabels := map[string]string{"node": node.Name}

This comment has been minimized.

@bsalamat

bsalamat Apr 16, 2018

Contributor

Please remove lines 337 to 343. We don't need node labels here.

}
// Ensure no pod is preempted.
for _, p := range pods {
if err = wait.Poll(time.Second, wait.ForeverTestTimeout, podIsGettingEvicted(cs, p.Namespace, p.Name)); err == nil {

This comment has been minimized.

@bsalamat

bsalamat Apr 16, 2018

Contributor

This causes the test to take a long time (30 seconds for each pod) before succeeding and has the potential of timing out when there are more pods in the test. Please use waitForPodUnschedulable to check that preemptor is marked unschedulable and then you can use waitForNominatedNodeName on the preemptor to ensure that its NominatedNodeName is never set. I'd suggest creating a variation of waitForNominatedNodeName that takes a timeout. Then you can use the new variation and wait only several seconds to ensure that the nominated node name is not set.

This comment has been minimized.

@resouer

resouer Apr 17, 2018

Member

Thanks for pointing out. Just fixed.

@bsalamat

Thanks, @resouer! Looks good. Just a minor comment.

@@ -63,7 +63,8 @@ type TestContext struct {
scheduler *scheduler.Scheduler
}
// createConfiguratorWithPodInformer create a configurator for scheduler with given informer factory, custom name and pod informer.
// createConfiguratorWithPodInformer create a configurator for scheduler with given informer factory,

This comment has been minimized.

@bsalamat

bsalamat Apr 18, 2018

Contributor

s/create/creates/
Please remove "with given informer factory, custom name and pod informer" from the comment. They don't add any value to the comment and need to be updated when function args change.

@k8s-ci-robot k8s-ci-robot added the lgtm label Apr 18, 2018

@resouer

This comment has been minimized.

Member

resouer commented Apr 18, 2018

/assign @timothysc

for test approval

@resouer

This comment has been minimized.

Member

resouer commented Apr 18, 2018

/assign @mikedanese

For component config approval

@resouer resouer changed the title from Separate pod priority from preemption in feature gate to Separate pod priority from preemption Apr 19, 2018

@mikedanese

This comment has been minimized.

Member

mikedanese commented Apr 19, 2018

/approve

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Apr 19, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, mikedanese, resouer

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Apr 19, 2018

Automatic merge from submit-queue (batch tested with PRs 59592, 62308, 62523, 62635, 62243). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 1e39d68 into kubernetes:master Apr 19, 2018

15 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation resouer authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@resouer resouer deleted the resouer:fix-62068 branch Apr 19, 2018

@bsalamat bsalamat added this to the v1.11 milestone Apr 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment