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

Revert changes made to eviction and qos #40025

Closed

Conversation

derekwaynecarr
Copy link
Member

What this PR does / why we need it:
This PR reverts changes that were made to qos and eviction designs that extended the meaning of a critical pod annotation.

#38836
#39059
#39114

Which issue this PR fixes
Addresses #40024 for the master branch.

Special notes for your reviewer:
For details on why this is being done, see #38322 (comment)

Release note:

Revert: Admit critical pods in the kubelet
Revert: Don't evict static pods
Revert: assign -998 as the oom_score_adj for critical pods (e.g. kube-proxy)

…ict"

This reverts commit d04fd1b, reversing
changes made to ae4db79.
…ritical"

This reverts commit 699964c, reversing
changes made to f86c047.
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 17, 2017
@derekwaynecarr
Copy link
Member Author

derekwaynecarr commented Jan 17, 2017

@k8s-cherrypick-bot
Copy link

Removing label cherrypick-candidate because no release milestone was set. This is an invalid state and thus this PR is not being considered for cherry-pick to any release branch. Please add an appropriate release milestone and then re-add the label.

@derekwaynecarr
Copy link
Member Author

This needs to be cherry-picked to release-1.4 and release-1.5.

@derekwaynecarr derekwaynecarr added this to the v1.6 milestone Jan 17, 2017
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jan 17, 2017
@vishh
Copy link
Contributor

vishh commented Jan 17, 2017

Can we instead make these changes gated behind a flag? If we revert them, kube-proxy will get evicted due to very many reasons including that of eviction bugs.

@dchen1107
Copy link
Member

@derekwaynecarr I provided some background context and the rough risk analysis at #38322 (comment)

I am ok with reverting this from main branch which is the initial goal @bprashanth and I discussed. But the production is bleeding if we revert those commits from 1.4 and 1.5 branches unless we have alternative solution.

@derekwaynecarr
Copy link
Member Author

@dchen1107 - i would like to revert master. ideally, i would like to revert 1.4 and 1.5. there are a number of pro/cons in any cluster deployment choice that could impact operators and users in production. the deployment model should not have an impact on the entire community in the manner this PR did. we basically invented a workload priority design really quickly, and rapidly pushed it out to present and prior releases with little to no community discussion or documentation to support one preferred deployment choice. i worry this establishes a precedent that i would not like to see followed in the future.

if reverting in 1.4 and 1.5 is not agreeable, can a member of your team flag-gate the feature behind an experimental flag in 1.4 and 1.5 (default to false) and reduce the scope of the annotation to only be meaningful in the kubelet if the pod is in the kube-system namespace?

as for this pr, i think it should merge as-is in master and the experimental flag should never appear in 1.5.

@k8s-github-robot
Copy link

@derekwaynecarr PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 17, 2017
@derekwaynecarr
Copy link
Member Author

Typo in previous comment, I think experimental flag should not be needed in 1.6... too many releases in my head

@vishh
Copy link
Contributor

vishh commented Jan 18, 2017

@derekwaynecarr A fix for solving static pod evictions will not land in v1.6 since it involves quite a few changes (moving static pods to Daemon Set, supporting daemon set upgrades, rightsizing kube proxy, switching to Guaranteed QoS, etc). Thankfully most of these missing features/bugs are being addressed in the v1.6 time frame.
For v1.6 (and the previous releases), I'd prefer adding a flag gate (disabled by default) for eviction changes including critical pod annotation & static pod eviction policy.

@vishh
Copy link
Contributor

vishh commented Jan 18, 2017

Do we need this revert to go in as-is before the flag gates are added? I'd prefer adding the flag gates directly.

@thockin
Copy link
Member

thockin commented Jan 18, 2017 via email

@liggitt
Copy link
Member

liggitt commented Jan 18, 2017

please let's pursue a flag gate

even with the flag enabled, the effect of the annotation needs to be limited to static pods in the kube-system namespace (or something equally restrictive). users should never be able to end-run existing qos/quota controls via annotations in the API

@thockin
Copy link
Member

thockin commented Jan 18, 2017 via email

@liggitt
Copy link
Member

liggitt commented Jan 18, 2017

then at least limit the power to a privileged namespace (along with the flag gate)

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] Needs approval from an approver in each of these OWNERS Files:

We suggest the following people:
cc @mikedanese @derekwaynecarr
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@derekwaynecarr
Copy link
Member Author

closing in favor of #40655

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. 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.

None yet