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.

Sign up for GitHub

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

Jump to bottom

Initial support for pod eviction based on disk #27199

Merged
merged 4 commits into from
Jul 29, 2016
Merged

Initial support for pod eviction based on disk #27199

merged 4 commits into from
Jul 29, 2016

Conversation

derekwaynecarr
Copy link
Member

@derekwaynecarr derekwaynecarr commented Jun 10, 2016 •
edited
Loading

This PR adds the following:

  1. node reports disk pressure condition based on configured thresholds
  2. scheduler does not place pods on nodes reporting disk pressure
  3. kubelet will not admit any pod when it reports disk pressure
  4. kubelet ranks pods for eviction when low on disk
  5. kubelet evicts greediest pod

Follow-on PRs will need to handle:

  1. integrate with new image gc PR (Initial support for pod eviction based on disk #27199)
  2. container gc policy should always run (will not be launched from eviction, tbd who does that)
    1. this means kill pod is fine for all eviction code paths since container gc will remove dead container
  3. min reclaim support will just poll summary provider (derek will do follow-on)
  4. need to know if imagefs is same device as rootfs from summary (derek follow-on)

/cc @vishh @kubernetes/sig-node

@derekwaynecarr derekwaynecarr added do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jun 10, 2016
@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/design Categorizes issue or PR as related to design. kind/old-docs size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 10, 2016
@derekwaynecarr derekwaynecarr assigned vishh and unassigned yujuhong Jun 10, 2016
vreon
vreon reviewed Jun 10, 2016
View reviewed changes
docs/proposals/kubelet-eviction.md

The default `minimum-eviction-threshold` is `0` for all resources.
The default `eviction-miniumum-reclaim` is `0` for all resources.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: minimum

@derekwaynecarr derekwaynecarr force-pushed the disk_eviction branch 2 times, most recently from 8b47e29 to 9cd8401 Compare June 12, 2016 19:59
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 12, 2016
@derekwaynecarr derekwaynecarr added this to the v1.4 milestone Jun 17, 2016
@k8s-github-robot k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 17, 2016
@derekwaynecarr derekwaynecarr changed the title WIP: disk eviction Initial support for pod eviction based on disk Jun 17, 2016
@derekwaynecarr derekwaynecarr added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/old-docs labels Jun 17, 2016
@derekwaynecarr
Copy link
Member Author

@vishh - follow-on PRs from either of us will be needed to complete this feature, but this provides a decent basis for us to collaborate against when master branch reopens for kube 1.4.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 21, 2016
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 21, 2016
@pmorie
Copy link
Member

pmorie commented Jul 27, 2016

@k8s-bot test this issue: #29703

@dchen1107 dchen1107 mentioned this pull request Jul 27, 2016
Closed
@derekwaynecarr
Copy link
Member Author

It appears something is wrong with collecting root container stats on GCE that I was not seeing locally. Will investigate tomorrow.

@k8s-github-robot k8s-github-robot added do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. retest-not-required-docs-only needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed retest-not-required-docs-only labels Jul 28, 2016
@derekwaynecarr derekwaynecarr removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 28, 2016
@derekwaynecarr
Copy link
Member Author

@ronnielai - tests passed, turns out cadvisor is slower to warm on gce, so moved diskinfoprovider call into a one time setup in each synchronize call.

@ronnielai ronnielai added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 28, 2016
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 28, 2016
@derekwaynecarr
Copy link
Member Author

fixed rebase error, retagging.

@derekwaynecarr derekwaynecarr added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 28, 2016
@k8s-github-robot
Copy link

@derekwaynecarr
You must link to the test flake issue which caused you to request this manual re-test.
Re-test requests should be in the form of: k8s-bot test this issue: #<number>
Here is the list of open test flakes.

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 29, 2016
@k8s-bot
Copy link

k8s-bot commented Jul 29, 2016

GCE e2e build/test passed for commit d37710f.

@derekwaynecarr derekwaynecarr added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 29, 2016
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Jul 29, 2016

GCE e2e build/test passed for commit d37710f.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 821ff65 into kubernetes:master Jul 29, 2016
xingzhou pushed a commit to xingzhou/kubernetes that referenced this pull request Dec 15, 2016
Merge pull request kubernetes#29321 from derekwaynecarr/eviction-prop…
b848619 
…osal-update-flags

Automatic merge from submit-queue

Update proposed flag names for kubelet eviction

This PR changes the flag names proposed in kubelet eviction for minimum amount of resource to reclaim when triggering an eviction.

This captures the design change proposed and agreed to in kubernetes#27199 

Having it in a separate PR removes noise from reviewing the core PR.

/cc @vishh @ronnielai PTAL
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@ronnielai ronnielai

Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v1.4
Development

Successfully merging this pull request may close these issues.
10 participants
@derekwaynecarr @dchen1107 @vishh @ronnielai @pmorie @k8s-github-robot @k8s-bot @vreon @googlebot @yujuhong