Add disruption admin guide. #1244

mml · 2016-09-16T17:23:37Z

This change is

devin-donnelly · 2016-09-16T17:58:21Z

Can you please retarget this PR to the release-1.4 branch?

googlebot · 2016-09-16T18:00:54Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

mml · 2016-09-16T18:00:58Z

Can you please retarget this PR to the release-1.4 branch?

Done.

mml · 2016-09-16T18:03:17Z

No idea why the bot freaked out.

% git log upstream/release-1.4..disruption
commit e5c144b7cde8b92308a59fc3d7cd4b154ea2ad6a
Author: Matt Liggett <[email protected]>
Date:   Fri Sep 16 10:21:21 2016 -0700

    Add disruption admin guide.

mml · 2016-09-16T18:03:45Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@googlebot I authored all these commits.

devin-donnelly · 2016-09-16T21:58:22Z

docs/admin/disruptions.md

+
+## Rationale
+
+Various cluster management operations may "voluntarily" evict pods. By


Avoid using "we" as a general principle. Suggested rephrase:

"In a "voluntary" eviction , a pod is removed from a node in a controlled manner, such as draining the node for maintenance (kubectl drain) or autoscaling a cluster downward. In the future, the [rescheduler] may also perform voluntary evictions. By contrast, a non-voluntary eviction removes a pod because a node has become unreachable or reports NotReady."

I went more directly definitional for the first sentence.

For the second one, the given example is just that, an example. It's not the definition of non-voluntary and only illustrates the principle.

devin-donnelly · 2016-09-16T21:58:48Z

docs/admin/disruptions.md

+have the
+[rescheduler](https://github.com/kubernetes/kubernetes/blob/master/docs/proposals/rescheduling.md)
+and possibly other examples. (In contrast, something like evicting pods because
+a node has become unreachable or reports NotReady, is not "voluntary.") For


I'd start a new paragraph here.

"For voluntary evictions, it can be useful..."

googlebot · 2016-09-19T18:42:11Z

CLAs look good, thanks!

mml · 2016-09-19T18:42:35Z

@davidopp think I just need a technical LG on this.

davidopp · 2016-09-19T19:09:23Z

docs/admin/disruptions.md

+because a node has become unreachable or reports NotReady, is not "voluntary."
+
+For voluntary evictions, it can be useful for applications to be able to limit
+the number of pods that are down, or the rate with which pods are evicted.  For


Since we didn't implement it yet, I would just remove the "or the rate with which pods are evicted"

davidopp · 2016-09-19T19:09:31Z

docs/admin/disruptions.md

+temporarily. Or a web front end might want to ensure that the number of replicas
+serving load never falls below a certain percentage of the total, even briefly.
+`PodDisruptionBudget` is an API object that specifies the minimum number or
+percentage of replicas of a collection that must be up at a time, and/or the


remove "and/or the maximum eviction rate across the collection."

davidopp · 2016-09-19T19:09:34Z

docs/admin/disruptions.md

+serving load never falls below a certain percentage of the total, even briefly.
+`PodDisruptionBudget` is an API object that specifies the minimum number or
+percentage of replicas of a collection that must be up at a time, and/or the
+maximum eviction rate across the collection. Components that wish to eviction a


s/eviction/evict/

davidopp · 2016-09-19T19:10:30Z

docs/admin/disruptions.md

+
+## Specifying a PodDisruptionBudget
+
+A `PodDisruptionBudget` has two components: a selector to specify the set of


I would rewrite this as

Currently a PodDisruptionBudget has two components: a selector to specify a set of pods, and a description of the minimum number of pods from that set that must be available (i.e. an eviction will not be allowed if it will cause the number of available pods to fall below this threshold).

The "i.e." is not how it's implemented. This may be a simple off-by-one error, or it may be a place where we didn't think carefully enough about the spec.

As it stands, if you say "100%", we will allow evictions if all the pods are ready. If we want "100%" to mean "never permit a 'voluntary' eviction", I will clarify the docs, and changing the code and tests will be easy.

Or maybe the right thing is to open an issue to discuss and track this, and for now document the way it works?

Yes sorry I didn't catch this sooner but I think the semantics you described are not what people are going to expect. I now see that the comment in the proto, "the minimum number of pods that must be available simultaneously" is ambiguous. And I guess I wasn't thinking about this when I was looking at the code and tests. In any event, I do think we want people to be able to say "don't allow any voluntary evictions". Can you send a PR to change the behavior and tests, and change the documentation to say something like what I suggested? Very sorry about that. Thanks for noting the discrepancy with my suggestion!

Also can you update the comment on the proto to describe the new behavior.

Yes to all of these. Alright if I address the semantic change in a followup when I actually change the code? It'll be sent today, but I would like to get this merged in some form, and it is correct as-is.

davidopp · 2016-09-19T19:10:34Z

docs/admin/disruptions.md

+pods, and a description of the minimum number of available pods for a disruption
+to be allowed.  The latter can be either an absolute number or a percentage.  In
+typical usage, a single budget would be used for a collection of pods managed by
+a controller.


add ", for example the pods in a single ReplicaSet"

davidopp · 2016-09-19T19:14:11Z

docs/admin/disruptions.md

+ 3. If there is some kind of misconfiguration, like multiple budgets pointing at
+    the same pod, you will get `500 Internal Server Error`.
+
+For a given eviction, there are two cases.


s/eviction/eviction request/

davidopp · 2016-09-19T19:18:23Z

docs/admin/disruptions.md

+to be allowed.  The latter can be either an absolute number or a percentage.  In
+typical usage, a single budget would be used for a collection of pods managed by
+a controller.
+


I would add something at the end that says "Note that DisruptionBudget does not truly guarantee that the specified number/percentage of pods will always be up -- for example, a node that hosts a pod from the collection may fail when the collection is at the minimum size specified in the PodDisruptionBudget, thus bringing the number of available pods from the collection below the specified size. DisruptionBudget can only protect against voluntary evictions, not all causes of unavailability."

mml · 2016-09-19T20:23:57Z

@davidopp PTAL

davidopp · 2016-09-20T18:28:21Z

Sure, LGTM.

…tes#1290) This back-ports the install instructions from the 2.12 branch (kubernetes#1244) into the 2.11 branch, specifically for the edge helm charts. Co-authored-by: cpretzer <[email protected]>

Fixes linkerd/linkerd2#7745 Copied `grafana.md` doc from kubernetes#1244 from the 2.12 docs into 2.11, for folks that are on edge releases, that already have stripped-off the Grafana instance.

mml assigned davidopp Sep 16, 2016

googlebot added the cla: yes label Sep 16, 2016

mml force-pushed the disruption branch from 6d8fadd to 9992311 Compare September 16, 2016 17:24

devin-donnelly added needs-rebase labels Sep 16, 2016

mml force-pushed the disruption branch from 9992311 to e5c144b Compare September 16, 2016 18:00

mml changed the base branch from master to release-1.4 September 16, 2016 18:00

googlebot added cla: no and removed cla: yes labels Sep 16, 2016

devin-donnelly removed the needs-rebase label Sep 16, 2016

devin-donnelly added this to the 1.4 milestone Sep 16, 2016

devin-donnelly suggested changes Sep 16, 2016

View reviewed changes

devin-donnelly added Docs Review: Open Issues and removed Needs Docs Review labels Sep 16, 2016

mml force-pushed the disruption branch from e5c144b to b4f3385 Compare September 19, 2016 18:42

googlebot added cla: yes and removed cla: no labels Sep 19, 2016

davidopp suggested changes Sep 19, 2016

View reviewed changes

Add disruption admin guide.

1117f1a

mml force-pushed the disruption branch from b4f3385 to 1117f1a Compare September 19, 2016 20:23

devin-donnelly approved these changes Sep 19, 2016

View reviewed changes

devin-donnelly added Docs LGTM and removed Docs Review: Open Issues labels Sep 19, 2016

davidopp mentioned this pull request Sep 19, 2016

Requirements for PodDisruptionBudget, /eviction subresource, and PreferAvoidPods to graduate to Beta and then v1 kubernetes/kubernetes#25321

Closed

10 tasks

davidopp added the Tech Review LGTM label Sep 20, 2016

devin-donnelly removed the Tech Review: Open Issues label Sep 20, 2016

devin-donnelly merged commit 67fb745 into kubernetes:release-1.4 Sep 20, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add disruption admin guide. #1244

Add disruption admin guide. #1244

mml commented Sep 16, 2016 •

edited by thockin

Loading

devin-donnelly commented Sep 16, 2016

googlebot commented Sep 16, 2016

mml commented Sep 16, 2016

mml commented Sep 16, 2016

mml commented Sep 16, 2016

devin-donnelly Sep 16, 2016

mml Sep 19, 2016

devin-donnelly Sep 16, 2016

mml Sep 19, 2016

googlebot commented Sep 19, 2016

mml commented Sep 19, 2016

davidopp Sep 19, 2016

mml Sep 19, 2016

davidopp Sep 19, 2016

mml Sep 19, 2016

davidopp Sep 19, 2016

mml Sep 19, 2016

davidopp Sep 19, 2016

mml Sep 19, 2016

davidopp Sep 20, 2016 •

edited

Loading

davidopp Sep 20, 2016

mml Sep 20, 2016

davidopp Sep 19, 2016 •

edited

Loading

mml Sep 19, 2016

davidopp Sep 19, 2016

mml Sep 19, 2016

davidopp Sep 19, 2016

mml Sep 19, 2016

mml commented Sep 19, 2016

davidopp commented Sep 20, 2016


		## Rationale

		Various cluster management operations may "voluntarily" evict pods. By


		## Specifying a PodDisruptionBudget

		A `PodDisruptionBudget` has two components: a selector to specify the set of

Add disruption admin guide. #1244

Add disruption admin guide. #1244

Conversation

mml commented Sep 16, 2016 • edited by thockin Loading

devin-donnelly commented Sep 16, 2016

googlebot commented Sep 16, 2016

mml commented Sep 16, 2016

mml commented Sep 16, 2016

mml commented Sep 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Sep 19, 2016

mml commented Sep 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidopp Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidopp Sep 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mml commented Sep 19, 2016

davidopp commented Sep 20, 2016

mml commented Sep 16, 2016 •

edited by thockin

Loading

davidopp Sep 20, 2016 •

edited

Loading

davidopp Sep 19, 2016 •

edited

Loading