Make DaemonSet respect critical pods annotation when scheduling #42028
Conversation
janetkuo
added
the
release-note
k8s-ci-robot
added
the
cncf-cla: yes
k8s-github-robot
added
the
size/L
|
This change is |
| @@ -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 { | |||
There was a problem hiding this comment.
not for this PR: There are other too that the DaemonSet should ideally respect - MemoryPressure, DiskPressure for example are some of them.
| predicateFails = append(predicateFails, reasons...) | ||
| } | ||
| } else { | ||
| fit, reasons, err = predicates.GeneralPredicates(pod, nil, nodeInfo) |
There was a problem hiding this comment.
You can also run a GeneralPredicates check and ignore all resource requirement failures.
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Moved them to essential and added TODO for PodFitsHostPorts
janetkuo
force-pushed
the
ds-critical-pods
branch
from
1634a0c to
e0fcc78
Compare
| // 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 { |
There was a problem hiding this comment.
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.
|
one more nit. otherwise lgtm |
janetkuo
force-pushed
the
ds-critical-pods
branch
from
e0fcc78 to
8ffeb49
Compare
|
Fixed nit and squashed |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
vishh
added
the
approved
|
@k8s-bot kops aws e2e test this |
|
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. |
|
@davidopp apologies. I was trying to shepherd it through in time for v1.6 code freeze. |
|
No problem; that's why I said "in the future" |
|
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 |
|
@janetkuo thanks! |
k8s-github-robot
added
the
needs-rebase
janetkuo
force-pushed
the
ds-critical-pods
branch
from
8ffeb49 to
4c88247
Compare
k8s-github-robot
removed
lgtm
|
[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: |
janetkuo
added
the
lgtm
|
Just rebased. Re-applying tag. |
|
Automatic merge from submit-queue (batch tested with PRs 41234, 42186, 41615, 42028, 41788) |
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 #41612Special notes for your reviewer:
Release note:
cc @kubernetes/sig-apps-feature-requests @erictune @vishh @liggitt @Kargakis @lukaszo @piosz @davidopp