Memory pressure shouldn't hard evict static pods

[bprashanth]

If a node runs out of memory, we kill something. Then if there is memory pressure, we don't admit best effort (https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/eviction/eviction_manager.go#L110). Maybe we can admit static-pods, even if they're best effort?

@kubernetes/sig-node if we can't differentiate static from non-static, maybe we can use the scheduler.alpha.kubernetes.io/critical-pod annotation?

[MrHohn]

This issue is now actually causing customer outage. Should bump it up to P0.

@thockin @bowei

[euank]

Is there any reason any static pods are best effort?

Shouldn't, ideally, all static pods have be guaranteed? Rather than including a "scheduler.alpha.kubernetes.io/critical-pod" annotation, I'd rather modify the pod resource request/limits to make them guaranteed.

[euank]

Two possible solutions (of others, I'm sure) and why I'd prefer guaranteed

Make static pods 'special'

Having static pods skip all admittance checks is implemented in #38795.

I don't like this because of the special-casing, security implications, and because I think there's probably a more correct solution.

I think this solution will lead to the chance for static pods to thrash if the node's in a messy state.

Make static pods guaranteed

I think that making all static pods guaranteed might be a better solution. It results in a lower total node-allocatable for users, but the reality was users shouldn't have allocated those resources already because the static pods needed them.

The hardest part of this solution is picking the request/limit number for each static pod correctly, but honestly we need to do that anyways.

[bprashanth]

I think the issue to discuss big picture solutions is #22212

The suggested oneliner to fix this can go right after https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/eviction/eviction_manager.go#L110:

if notBestEffot || criticalPod {
 return lifecycle.PodAdmitResult{Admit: true}
}

where criticalPod is set based on

annotations:
  node.alpha.kubernetes.io/critical-pod: ''

[euank]

@bprashanth I'm not really happy with such an annotation though. The basically oneline change in #38795 is now "don't apply memory-admit failures to static pods", but I'd only see that as a temporary workaround for the proper solutions to this.

[bprashanth]

Yeah I'm fine doing that too. It's basically the recommeded fix but looking at the source instead of an annotation. I think we'll need the annotation though.

To recap varios other bugs, there are actually multiple issues here:

1. Admit all static pods, unless there's either a node condition or some other "hard" conflict (eg: hostport)
2. Set a lower oom score on (some/all) static pods
3. If a static pod fails admission because of node condition, keep retrying

your pr fixes 1

we still need something like an annotation to fix 2 in the short term (obviously fixable with preemption priority). We could use the existing podSource again (but that means something like fluent running in burstable, would also get a low oom score, so may not be the best tradeoff because we know that things is a pig).

3 involves treating file source different from apiserver source in many more places. Right now, failing admission check marks a pod Failed and the kubelet assumes that some other controller will recreate the pod, and the scheduler will re-assign to a healthy node. The problem of course, is that the kubelet is the controller for static-pods, so really, it needs to continuously reconcile static pods that have failed admission checks and process them as new creates instead of terminated pods.

[dchen1107]

@bprashanth I like your idea using using scheduler.alpha.kubernetes.io/critical-pod annotation here, because:

1. This is more align with our long term fix: Preemption priority / scheme #22212 to have a generic schema describing how critical and importance a pod is.

2. Once the annotation is specified for a pod, when the local controller modules(admission at Kubelet, eviction manager, etc.) make the decision, they all can follow the same set of rule. Kubelet also can give critical pods a lower oom score to make them not likely being killed.

3. More important is that later when we enhance scheduler, rescheduler and daemonset_controller, etc. they all can follow the same rules to avoid the ping-pong issue due to conflict protocol (Kubelet keeps reject some pods, while the remote controller / scheduler keep putting those pods back).

4. This proposal also decouples static pods from the importance / criticalness of a pod. Not all static pods should be treated equally same. For example, fluentd is static pod today, but it is known havign memory leakage issue. Are we planning to treat fluentd pod same as kube-proxy? We are considering when node has the resource starvation issue, losing some logging message is equally bad as losing the node availability?

@euank I don't like the treat static pods special because only kubelet knows those pods are file-based. There is no API field indicating it, so very likely we run into the issues described above in 3) and 4).