-
Notifications
You must be signed in to change notification settings - Fork 39.8k
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
Mark Static pods on the Master as critical #47356
Mark Static pods on the Master as critical #47356
Conversation
/release-note-none |
/assign @dchen1107 @vishh |
/lgtm |
@dashpole should this be included in v1.7? |
I would say yes. I would consider not placing the "critical" annotation on critical system pods a bug. |
This PR is good but seems we should also add the three non-static pods? event-exporter-v0.1.0-947590526-h897n (non-static) |
@dashpole Please make the kubeadm static pods critical then as well (cmd/kubeadm/app/master/manifests.go) |
@luxas unless I am missing something, those are the static pods I am changing in this PR |
@davidopp I am not familiar with what those pods do. Making them critical allows them to preempt other pods in the system in order to schedule. |
Another non-static system pod that is not currently marked as critical is the NPD. We plan to leave it non-critical for the time being, as it is not critical for the system to function, and will reschedule when there is enough allocatable resources. |
@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this |
event-exporter is important for debugging so probably yes. l7-default-backend I don't know exactly what it does but it sounds from the name like it may be critical for Ingress so probably yes. monitoring-influxdb-grafana maybe could be skipped. |
alright, Ill add the event-exporter, and the L7-default-backend. |
You are changing the manifest files used by the kube-up.sh scripts, but not the ones used by kubeadm. See https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/master/manifests.go |
f576f5d
to
be7a993
Compare
Ok, I have added the kubeadm static pods as well. This PR now makes all static system pods critical. |
based on #47277 (comment), I am going to leave out non-static pods. |
I also added another comment on static pods at: #47277 (comment) We need to answer that questions. Also release-note is required for this. |
be7a993
to
e223eb9
Compare
/retest /approve |
PTAL @kubernetes/sig-scalability-pr-reviews @kubernetes/sig-node-pr-reviews @kubernetes/kubernetes-release-managers This was already LGTM'd, can we get the label reapplied? |
Damn, can't even tag the right sig, I meant @kubernetes/sig-scheduling-pr-reviews of course |
/lgtm but needs a different owner for final approval. /cc @roberthbailey |
This is clearly in cluster lifecycle territory. /approve |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dashpole, jbeda, luxas, roberthbailey, timothysc, vishh Associated issue: 47277 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
Automatic merge from submit-queue (batch tested with PRs 47669, 40284, 47356, 47458, 47701) |
fixes #47277.
A known issue with static pods is that they do not interact well with evictions. If a static pod is evicted or oom killed, then it will never be recreated. To mitigate this, we do not evict static pods that are critical. In addition, non-critical pods are candidates for preemption if a critical pod is scheduled to the node. If there are not enough allocatable resources on the node, this causes the static pod to be preempted.
This PR marks all static pods in the kube-system namspace as critical.
cc @vishh @dchen1107