Skip to content
This repository has been archived by the owner on Jul 30, 2021. It is now read-only.

pkg/assets/internal: Remove critical pod annotations #777

Merged
merged 1 commit into from Nov 17, 2017
Merged

pkg/assets/internal: Remove critical pod annotations #777

merged 1 commit into from Nov 17, 2017

Conversation

dghubble
Copy link
Contributor

@dghubble dghubble commented Nov 17, 2017

Supercedes #728 and should remove all critical annotations on the control plane as requested.

Broader discussion and rationale:

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 17, 2017
@dghubble dghubble requested review from squat and diegs November 17, 2017 18:23
@dghubble
Copy link
Contributor Author

Rebased.

@dghubble dghubble merged commit a905d0e into kubernetes-retired:master Nov 17, 2017
@dghubble dghubble deleted the remove-critical-annotations branch November 17, 2017 22:39
dghubble added a commit to poseidon/terraform-render-bootstrap that referenced this pull request Nov 29, 2017
@redbaron
Copy link
Contributor

redbaron commented Dec 5, 2017

Do I read it right, that annotations are removed just to make rescheduler happier? It is confusing, bootkube and rescheduler are different projects, bootkube doesn't install rescheduler into cluster by default, why bootkube is making changes, not rescheduler is improving?

I am concerned, because removed annotations are used by kuberneted itself, when ExperimentalCriticalPodAnnotation featuregate is enabled. How one would prevent these very important pods to be evicted by kubelet lets say under disk pressure? Is there a safe way to run self hosted setup in 1.8.x Kubernetes without these annotations at all?

@diegs
Copy link
Contributor

diegs commented Dec 5, 2017

@redbaron as discussed in #519 (comment), the rescheduler is actually dangerous in self-hosted clusters, and it will be replaced by priorities anyway (see comments from bsalamat in that thread).

Since bootkube has never shipped the rescheduler or enabled experimental features it was deemed prudent to remove the annotations. If you are deciding to run the rescheduler and accept the risks associated with it you are of course welcome to add the annotations to your deployment.

@redbaron
Copy link
Contributor

redbaron commented Dec 5, 2017

That is what I thought too, I am not running rescheduller. My point is, that removed annotations are used by kubernetes itself, not rescheduler. These anontations are taken into account when kubelet evicts pods when it is under disk space pressure. So these annotations are useful and their removal makes self hosted setup less safe

@Quentin-M
Copy link
Contributor

@diegs @dghubble @redbaron @aaronlevy

The scheduler.alpha.kubernetes.io/critical-pod annotation is used by Kubernetes itself (i.e. eviction, admission, daemonset, preemption controllers) if the ExperimentalCriticalPodAnnotation feature flag is enabled, in order to avoid evicting pods, and/or guarantee admission that are a/ static or b/ on resource-pressured nodes - regardless of priorities and whether the rescheduler is used or not.

This comment and code block is particularly relevant and shows that without the feature flag and without the annotation, static pods could be evicted, and will not be re-admitted.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants