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

Change eviction logic in NodeController and make it Zone-aware #28897

Merged
merged 1 commit into from
Aug 2, 2016
Merged

Change eviction logic in NodeController and make it Zone-aware #28897

merged 1 commit into from
Aug 2, 2016

Conversation

gmarek
Copy link
Contributor

@gmarek gmarek commented Jul 13, 2016 •
edited
Loading

Ref. #28832

This PR changes the behavior of the NodeController. From now on

Change eviction policies in NodeController:
- add a "partialDisruption" mode, when more than 33% of Nodes in the zone are not Ready
- add "fullDisruption" mode, when all Nodes in the zone are not Ready

Eviction behavior depends on the mode in which NodeController is operating:
- if the new state is "partialDisruption" or "fullDisruption" we call a user defined function that returns a new QPS to use (default 1/10 of the default rate, and the default rate respectively),
- if the new state is "normal" we resume normal operation (go back to default limiter settings),
- if all zones in the cluster are in "fullDisruption" state we stop all evictions.

cc @wojtek-t @smarterclayton @davidopp

@gmarek gmarek added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Jul 13, 2016
@gmarek gmarek added this to the v1.4 milestone Jul 13, 2016
@gmarek gmarek self-assigned this Jul 13, 2016
@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note-label-needed labels Jul 13, 2016
@gmarek gmarek force-pushed the hooks2 branch 2 times, most recently from 9b29ae3 to 1d9937e Compare July 13, 2016 15:35
@gmarek gmarek added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Jul 13, 2016
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 13, 2016
@gmarek gmarek mentioned this pull request Jul 14, 2016
Closed
@k8s-github-robot k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Jul 14, 2016
@gmarek
Copy link
Contributor Author

gmarek commented Jul 14, 2016

@k8s-bot unit test this issue: #28941

@k8s-github-robot k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 14, 2016
@gmarek gmarek changed the title Add a hook in NodeController for handling partial Zone segmentation Change eviction logic in NodeController and make it Zone-aware Jul 14, 2016
@gmarek gmarek removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Jul 14, 2016
@gmarek gmarek assigned wojtek-t and unassigned gmarek Jul 14, 2016
@gmarek gmarek force-pushed the hooks2 branch 3 times, most recently from 24056ad to 6ae8d41 Compare July 14, 2016 16:53
@k8s-github-robot k8s-github-robot removed the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Jul 14, 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 Jul 28, 2016
@gmarek
Copy link
Contributor Author

gmarek commented Jul 28, 2016

@wojtek-t PTAL

wojtek-t
wojtek-t reviewed Aug 2, 2016
View reviewed changes
pkg/controller/node/controller_utils.go
@@ -36,17 +36,28 @@ import (
)

// This function is expected to get a slice of NodeReadyConditions for all Nodes in a given zone.
// The zone is considered:
// - fullyDisrupted if there're no Ready Nodes,
// - partiallyDisrupted if more than 1/3 of Nodes (no less than 3) are not Ready,
Copy link
Member

Choose a reason for hiding this comment

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

nit: s/no less than 3/at least 3/ ?

Copy link
Member

Choose a reason for hiding this comment

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

also maybe:
s/not Ready/NotReady/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First done, second discussed f2f (it's a 3-value logic, so NotReady != not Ready)

@wojtek-t
Copy link
Member

wojtek-t commented Aug 2, 2016

Added a bunch of new comments, but it generally looks ok (please use the new commit to apply them - we will squash at the end).

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 2, 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 Aug 2, 2016
@wojtek-t
Copy link
Member

wojtek-t commented Aug 2, 2016

I think I'm fine with the PR - please squash the commits and I will do quick final pass.

@gmarek
Copy link
Contributor Author

gmarek commented Aug 2, 2016

@wojtek-t - squshed.

@k8s-bot
Copy link

k8s-bot commented Aug 2, 2016

GCE e2e build/test passed for commit 66224ce.

@wojtek-t wojtek-t added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 2, 2016
@wojtek-t
Copy link
Member

wojtek-t commented Aug 2, 2016

LGTM

@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 Aug 2, 2016

GCE e2e build/test passed for commit 66224ce.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 68def06 into kubernetes:master Aug 2, 2016
@gmarek gmarek deleted the hooks2 branch August 30, 2016 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@wojtek-t wojtek-t

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. 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.
5 participants
@gmarek @wojtek-t @k8s-bot @k8s-github-robot @googlebot