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

janetkuo
Copy link
Member

@janetkuo 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

@janetkuo janetkuo added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Feb 24, 2017
@janetkuo janetkuo added this to the v1.6 milestone Feb 24, 2017
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 24, 2017
@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 24, 2017
@k8s-reviewable
Copy link

This change is Reviewable

Copy link
Contributor

@vishh vishh left a comment

Choose a reason for hiding this comment

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

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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Added TODO

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved them to essential and added TODO for PodFitsHostPorts

// 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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea! Done

@vishh
Copy link
Contributor

vishh commented Feb 24, 2017

one more nit. otherwise lgtm

@janetkuo
Copy link
Member Author

Fixed nit and squashed

@vishh
Copy link
Contributor

vishh commented Feb 24, 2017

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 24, 2017
@vishh vishh added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 24, 2017
@janetkuo
Copy link
Member Author

@k8s-bot kops aws e2e test this

@davidopp
Copy link
Member

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
Copy link
Contributor

vishh commented Feb 24, 2017

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

@davidopp
Copy link
Member

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

@0xmichalis
Copy link
Contributor

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

@janetkuo
Copy link
Member Author

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
Copy link
Member

piosz commented Feb 24, 2017

@janetkuo thanks!

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 26, 2017
@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 27, 2017
@k8s-github-robot
Copy link

[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 "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2017
@janetkuo
Copy link
Member Author

Just rebased. Re-applying tag.

@k8s-github-robot
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make DaemonSet respect "critical pods annotation" when scheduling
8 participants