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.

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

Make DaemonSet respect critical pods annotation when scheduling #42028

Merged
merged 1 commit into from Feb 28, 2017

Conversation

Projects
None yet
8 participants
@janetkuo
Copy link
Member

janetkuo commented Feb 24, 2017

What this PR does / why we need it: #41612

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #41612

Special notes for your reviewer:

Release note:

Make DaemonSet respect critical pods annotation when scheduling. 

cc @kubernetes/sig-apps-feature-requests @erictune @vishh @liggitt @kargakis @lukaszo @piosz @davidopp

@k8s-reviewable

This comment has been minimized.

Copy link

k8s-reviewable commented Feb 24, 2017

This change is Reviewable

@vishh
Copy link
Member

vishh left a comment

Added a few comments. Thanks for the PR!

@@ -741,6 +749,9 @@ func (dsc *DaemonSetsController) nodeShouldRunDaemonPod(node *v1.Node, ds *exten

// TODO: Move it to the predicates
for _, c := range node.Status.Conditions {
if critical {
break
}
if c.Type == v1.NodeOutOfDisk && c.Status == v1.ConditionTrue {

This comment has been minimized.

@vishh

vishh Feb 24, 2017

Member

not for this PR: There are other too that the DaemonSet should ideally respect - MemoryPressure, DiskPressure for example are some of them.

This comment has been minimized.

@janetkuo

janetkuo Feb 24, 2017

Author Member

Added TODO

predicateFails = append(predicateFails, reasons...)
}
} else {
fit, reasons, err = predicates.GeneralPredicates(pod, nil, nodeInfo)

This comment has been minimized.

@vishh

vishh Feb 24, 2017

Member

You can also run a GeneralPredicates check and ignore all resource requirement failures.

This comment has been minimized.

@janetkuo

janetkuo Feb 24, 2017

Author Member

Yeah, thought about it but it's easier to manage predicates this way than whitelisting failures in DS controller

}

// noncriticalPredicates are the predicates that only non-critical pods need
func noncriticalPredicates(pod *v1.Pod, meta interface{}, nodeInfo *schedulercache.NodeInfo) (bool, []algorithm.PredicateFailureReason, error) {

This comment has been minimized.

@vishh

vishh Feb 24, 2017

Member

PodFitsHost is essential and PodFitsHostPorts might be essential too for now. The former is a basic requirement to admit pods and preemptions will not work around that.
The latter is a resource that kubelet does not support. I see it as a bug since Kubelet should ideally be prempting pods to free up host ports too. cc @dashpole

This comment has been minimized.

@janetkuo

janetkuo Feb 24, 2017

Author Member

Moved them to essential and added TODO for PodFitsHostPorts

@janetkuo janetkuo force-pushed the janetkuo:ds-critical-pods branch from 1634a0c to e0fcc78 Feb 24, 2017

// If the pod is marked as critical and support for critical pod annotations is enabled,
// check predicates for critical pods only.
fit, reasons, err = predicates.EssentialPredicates(pod, nil, nodeInfo)
if err != nil {

This comment has been minimized.

@vishh

vishh Feb 24, 2017

Member

nit: The error handling logic seems to be the same for both the cases here. Why not combine them into one segment?

if critical {
 fit, reasons, err = predicates.EssentialPredicates(...)
else {
 fit, reasons, err = predicates.GeneralPredicates(...)
}
// Handle errors.

This comment has been minimized.

@janetkuo

janetkuo Feb 24, 2017

Author Member

Good idea! Done

@vishh

This comment has been minimized.

Copy link
Member

vishh commented Feb 24, 2017

one more nit. otherwise lgtm

@janetkuo janetkuo force-pushed the janetkuo:ds-critical-pods branch from e0fcc78 to 8ffeb49 Feb 24, 2017

@janetkuo

This comment has been minimized.

Copy link
Member Author

janetkuo commented Feb 24, 2017

Fixed nit and squashed

@vishh

This comment has been minimized.

Copy link
Member

vishh commented Feb 24, 2017

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 24, 2017

@vishh vishh added the approved label Feb 24, 2017

@janetkuo

This comment has been minimized.

Copy link
Member Author

janetkuo commented Feb 24, 2017

@k8s-bot kops aws e2e test this

@davidopp

This comment has been minimized.

Copy link
Member

davidopp commented Feb 24, 2017

Sigh; this PR shows why the critical pods thing is such an awful hack. I'm looking forward to getting rid of it ASAP.

BTW in the future please try to get an approve from me or @timothysc for changes to the scheduler. We would have approved this change anyway, but it's easy to miss @ mentions so that's unfortunately the only way to ensure that between him and me we know what's going on.

@vishh

This comment has been minimized.

Copy link
Member

vishh commented Feb 24, 2017

@davidopp apologies. I was trying to shepherd it through in time for v1.6 code freeze.

@davidopp

This comment has been minimized.

Copy link
Member

davidopp commented Feb 24, 2017

No problem; that's why I said "in the future"

@kargakis

This comment has been minimized.

Copy link
Member

kargakis commented Feb 24, 2017

lgtm but I hope we are going to proceed with #42002 and remove all this madness from the DS controller.

@janetkuo

This comment has been minimized.

Copy link
Member Author

janetkuo commented Feb 24, 2017

lgtm but I hope we are going to proceed with #42002 and remove all this madness from the DS controller.

Yes that's the plan

@piosz

This comment has been minimized.

Copy link
Member

piosz commented Feb 24, 2017

@janetkuo thanks!

@janetkuo janetkuo force-pushed the janetkuo:ds-critical-pods branch from 8ffeb49 to 4c88247 Feb 27, 2017

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Feb 27, 2017

[APPROVALNOTIFIER] This PR is NOT APPROVED

The following people have approved this PR: janetkuo, k8s-merge-robot, vishh

Needs approval from an approver in each of these OWNERS Files:

We suggest the following people:
cc @mikedanese @timothysc
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@janetkuo janetkuo added the lgtm label Feb 27, 2017

@janetkuo

This comment has been minimized.

Copy link
Member Author

janetkuo commented Feb 27, 2017

Just rebased. Re-applying tag.

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Feb 28, 2017

Automatic merge from submit-queue (batch tested with PRs 41234, 42186, 41615, 42028, 41788)

@k8s-github-robot k8s-github-robot merged commit 2b2c04e into kubernetes:master Feb 28, 2017

15 checks passed

Jenkins Bazel Build Build succeeded.
Details
Jenkins GCE Node e2e Build succeeded.
Details
Jenkins GCE e2e Build succeeded.
Details
Jenkins GCE etcd3 e2e Build succeeded.
Details
Jenkins GCI GCE e2e Build succeeded.
Details
Jenkins GCI GKE smoke e2e Build succeeded.
Details
Jenkins GKE smoke e2e Build succeeded.
Details
Jenkins Kubemark GCE e2e Build succeeded.
Details
Jenkins kops AWS e2e Build succeeded.
Details
Jenkins non-CRI GCE Node e2e Build succeeded.
Details
Jenkins non-CRI GCE e2e Build succeeded.
Details
Jenkins unit/integration Build succeeded.
Details
Jenkins verification Build succeeded.
Details
Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation janetkuo authorized
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.