Add a KEP for graduating PodDisruptionBudget to stable by bsalamat · Pull Request #904 · kubernetes/enhancements

bsalamat · 2019-03-19T02:13:03Z

liggitt · 2019-03-19T02:15:14Z

keps/sig-apps/20190318-PodDisruptionBudget-graduation-to-stable.md

+## Summary
+
+[Pod Disruption Budget (PDB)](https://kubernetes.io/docs/tasks/run-application/configure-pdb/)
+is a Kubernete API that limits the number of pods of a collection that are down simultaneously from voluntary disruptions.


typo: Kubernetes

liggitt · 2019-03-19T02:16:26Z

keps/sig-apps/20190318-PodDisruptionBudget-graduation-to-stable.md

+#### Mutable PDBs
+
+A mutable PDB object allows its `MinAvailable` and `MaxUnavailable` fields to be
+modified by clients. Components that use PDB must watch such modifications and


what about selector fields? is there a good reason to limit which spec fields can be modified?

Added the selector as well. I am not closely familiar with the internal logic of Disruption controller to say with confidence whether updating the selector (and/or other fields) could cause any issues there, but I don't see a concern after a quick look at the code.

liggitt · 2019-03-19T02:20:18Z

keps/sig-apps/20190318-PodDisruptionBudget-graduation-to-stable.md

+the rolling update spec and currently does not take PDB into account. We need to
+change the implementation and use
+`min(PDB.MinAvailable, RollingUpdate.MinAvailable)` and
+`max(PDB.MaxUnavailable, RollingUpdate.MaxUnavailable)` instead.


Do we want deployments looking at PDB objects or using the evict subresource? Just looking at the PDB does not take into account PDB status for selectors that match across deployments, right?

Respecting PDB could easily lead to a situation in which a deployment could not make progress. Given that, is respecting PDB still desired? If so, describe how that state will be detected, communicated, and/or resolved (automatically or manually).

What I had in mind was to only look at MinAvailable and MaxUnavailable values and treat those similar to MinAvailable and MaxUnavailable of a rolling update, without making further changes and without using "evict" subresource, but given your points and reading some of the old comments, I think we should drop this requirement.

liggitt · 2019-03-19T02:22:49Z

keps/sig-apps/20190318-PodDisruptionBudget-graduation-to-stable.md

+#### Needed Tests
+
+If this is considered a non-optional feature, there should be a conformance test
+for it (this needs to be an e2e test tagged as conformance).


This seems like a good candidate for conformance testing. With an ack from @kubernetes/cncf-conformance-wg I'd make this a more definitive statement

Changed wording to make the test a requirement.

bsalamat

Thanks, @liggitt for your quick review!

bsalamat · 2019-03-19T17:57:43Z

keps/sig-apps/20190318-PodDisruptionBudget-graduation-to-stable.md

+## Summary
+
+[Pod Disruption Budget (PDB)](https://kubernetes.io/docs/tasks/run-application/configure-pdb/)
+is a Kubernete API that limits the number of pods of a collection that are down simultaneously from voluntary disruptions.


bsalamat · 2019-03-19T18:17:13Z

keps/sig-apps/20190318-PodDisruptionBudget-graduation-to-stable.md

+#### Mutable PDBs
+
+A mutable PDB object allows its `MinAvailable` and `MaxUnavailable` fields to be
+modified by clients. Components that use PDB must watch such modifications and


Added the selector as well. I am not closely familiar with the internal logic of Disruption controller to say with confidence whether updating the selector (and/or other fields) could cause any issues there, but I don't see a concern after a quick look at the code.

bsalamat · 2019-03-19T18:44:29Z

keps/sig-apps/20190318-PodDisruptionBudget-graduation-to-stable.md

+the rolling update spec and currently does not take PDB into account. We need to
+change the implementation and use
+`min(PDB.MinAvailable, RollingUpdate.MinAvailable)` and
+`max(PDB.MaxUnavailable, RollingUpdate.MaxUnavailable)` instead.


What I had in mind was to only look at MinAvailable and MaxUnavailable values and treat those similar to MinAvailable and MaxUnavailable of a rolling update, without making further changes and without using "evict" subresource, but given your points and reading some of the old comments, I think we should drop this requirement.

timothysc · 2019-03-19T20:17:20Z

keps/sig-apps/20190318-PodDisruptionBudget-graduation-to-stable.md

+
+## Summary
+
+[Pod Disruption Budget (PDB)](https://kubernetes.io/docs/tasks/run-application/configure-pdb/)


@bsalamat @liggitt - I've been thinking a lot about this topic when working on ClusterAPI.

Because the object is very loosely coupled with Pods, and only via label selector, is there any reason why we couldn't have it be more generic and remove the the 'Pod' from the resource kind? The link via label denotes the resource that it is tied to.

As a concrete example, we would like to tie disruption budgets to other resources for cluster management (machines).
/cc @ncdc @detiber

for the other scenarios, is delete the standard definition of disruption?

Agree with disruption budgets to machines. There's two orthogonal dimensions and use cases - managing machine upgrades because of organizational or infrastructure policy (and having to create fake pods to represent that with PDB today), and managing impact to applications.

Would just using label selector cause confusion here? Would there be a case where we would want to match both Machines and Pods with the same selector? Or would it be on the consumer to differentiate the type to associate with the label selector?

As to the specific use case around Machines, would it make sense to have the disruption budget exist for the Machines or the underlying Nodes instead? If what we are striving for is to ensure a certain level of capacity is available during an upgrade, then it seems to me that we are more concerned with the Nodes rather than the Machines themselves.

we have protected a pool of machines by running a pod per node with anti-affinity rules setup, and then you can prevent disruption to more than N number of nodes using that pod as a guard. its a bit of a hack, but it works. in general, i would prefer we have pod disruption budget, and machine disruption budgets as separate objects, the use cases may vary slightly in practice. similar to having machine sets and replica sets.

+1 to having different disruption budget for pods and nodes for a few reasons:

PDB is enforced through evict subresource of Pods which is obviously not applicable to nodes.

If we used the same API for both objects, we would need to add a field to the API to specify the type of object that it applies to (node vs pod).

Implementation of Node Disruption Budget shares little with that of PDB. So, we won't save much on the coding side by combining the two APIs either.

I would prefer we have pod disruption budget, and machine disruption budgets as separate objects, the use cases may vary slightly in practice. similar to having machine sets and replica sets

I'd tend to agree. The concepts are similar, but the application, audience, and scope are different.

PDB's are namespaced, and write access can be granted to users with write access in a namespace.

Machine or Node disruption budgets would be cluster-scoped, and should only be writeable by the cluster admin.

Machine or Node disruption budgets would be cluster-scoped, and should only be writeable by the cluster admin.

That's not true, they would be namespace scoped in a mgmt cluster.

FWIW I'm ok if we decide to not rationalize now, but I do think this is an area in the api where there is overlapping concepts and having (N) of the same type is also not good.

FWIW I'm ok if we decide to not rationalize now,

If we think we may change the API at some point, discussing it now is probably better than after the API is GA'ed.

We have work-arounds on our side.

derekwaynecarr · 2019-03-19T20:22:22Z

i have no major issue about graduating pdb in its current form. limitations that we have encountered recently could be handled via a higher level controller writing to a pdb so making them mutable would help a lot. an example scenario was sizing a pdb relative to the number of control plane machines in a cluster which is easier to do if we support mutability. this is useful if you use daemonsets for some types of workloads, but still want to minimize potential disruption to the number of control plane machines.

smarterclayton · 2019-03-19T20:22:39Z

keps/sig-apps/20190318-PodDisruptionBudget-graduation-to-stable.md

+### Goals
+
+* Plan to promote PDB API to stable version.
+* Propose changes to the deployment controller to take PDB into account.


Where is "taking it into account" described?

I removed that section as it turns out to create complexities which are not easy to address. I'll remove this goal.

MaciekPytel · 2019-03-20T12:53:11Z

keps/sig-apps/20190318-PodDisruptionBudget-graduation-to-stable.md

+  - [This test](https://github.com/kubernetes/kubernetes/blob/ac56bd502ab96696682c66ebdff94b6e52471aa3/test/integration/scheduler/preemption_test.go#L731)
+  tests effects of PDB on preemption (PDB is honored in a best effort way)
+* Eviction integration tests
+  - [These tests](https://github.com/kubernetes/kubernetes/blob/master/test/integration/evictions/evictions_test.go) test eviction logic and its interactions with PDB.


A bunch of autoscaling tests: https://k8s-testgrid.appspot.com/sig-autoscaling-cluster-autoscaler#gci-gce-autoscaling&include-filter-by-regex=pdb, code in: https://github.com/kubernetes/kubernetes/blob/master/test/e2e/autoscaling/cluster_size_autoscaling.go

Added the test

MaciekPytel · 2019-03-20T12:54:46Z

keps/sig-apps/20190318-PodDisruptionBudget-graduation-to-stable.md

+- [ ] Ensure that components do not have any logic that relies on immutability
+of PDBs. For example, if component builds a cache of pods that match various
+PDBs, they must add logic to invalid the cache on updates.
+   - Action Item: sweep for in-tree uses to make sure we’re informer driven and


This should include all repos in kubernetes org, not just k/k. I know Cluster Autoscaler has logic to deal with PDBs (I don't think it will be impacted by this change, but it's worth double checking), I suspect VPA may also use it. Not sure about other projects under kubernetes org.

kow3ns · 2019-03-22T18:35:28Z

@janetkuo and @mortent PTAL as well, I'd like your thoughts about this

bsalamat · 2019-04-08T18:31:47Z

ping @janetkuo and @mortent. Is there anything else that you want to see here?
Since PDB is owned by SIG Apps, I would expect someone from your SIG to take the graduation of PDB from here and perform the remaining actions items.

mortent · 2019-04-08T18:41:51Z

I'm looking into adding support for the scale subresource with PDBs, but I'm not sure if this would be a requirement for GA. But I hope this would be ready for the next release of Kubernetes together with mutable PDBs. And then both changes could soak for one release and we can push PDBs to GA in 1.16.

I am interested in taking on the task of getting this to GA.

kow3ns · 2019-07-09T23:18:09Z

So we've discussed this as something we want to do. It passes the criteria for approval.
/approve

janetkuo · 2019-07-30T23:52:03Z

keps/sig-apps/20190318-PodDisruptionBudget-graduation-to-stable.md

+Kubernetes control plane evicts some of the existing pods, but keeps at least 10
+around. The PDB is updated and states that at least 20 replicas must be kept
+alive. It may appear to an observer that the evictions happened before the PDB 
+update were incorrect, if they don’t notice the PDB update.


We already have this problem before PDBs are mutable.

Immutable PDBs can be deleted and re-created with new spec (20 replicas in this example).

janetkuo · 2019-07-30T23:54:50Z

/lgtm

k8s-ci-robot · 2019-07-30T23:55:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, janetkuo, kow3ns

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~keps/sig-apps/OWNERS~~ [kow3ns]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

This is a placeholder PR for the docs for the PDB to GA KEP kubernetes/enhancements#904

seh · 2020-09-26T13:04:42Z

See kubernetes/kubernetes#95083 for a potential problem with the debatable treatment of the PodDisruptionBudget.Selector field.

k8s-ci-robot assigned kow3ns and liggitt Mar 19, 2019

k8s-ci-robot requested review from kow3ns and mattfarina March 19, 2019 02:13

liggitt reviewed Mar 19, 2019

View reviewed changes

bsalamat commented Mar 19, 2019

View reviewed changes

timothysc reviewed Mar 19, 2019

View reviewed changes

k8s-ci-robot requested review from detiber and ncdc March 19, 2019 20:17

smarterclayton reviewed Mar 19, 2019

View reviewed changes

bsalamat force-pushed the pdb_to_stable_kep branch from d2515ef to e77d930 Compare March 19, 2019 22:12

MaciekPytel reviewed Mar 20, 2019

View reviewed changes

Add a KEP for graduating PodDisruptionBudget to stable

aa2d4d4

bsalamat force-pushed the pdb_to_stable_kep branch from e77d930 to aa2d4d4 Compare March 20, 2019 21:57

mortent mentioned this pull request Apr 22, 2019

PDB support for custom resources with scale subresource #981

Closed

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 9, 2019

kacole2 mentioned this pull request Jul 29, 2019

PodDisruptionBudget and /eviction subresource #85

Closed

12 tasks

janetkuo approved these changes Jul 30, 2019

View reviewed changes

k8s-ci-robot assigned janetkuo Jul 30, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 30, 2019

k8s-ci-robot merged commit a206c03 into kubernetes:master Jul 30, 2019

mortent added a commit to mortent/website that referenced this pull request Aug 14, 2019

[WIP] PDB GA documentation

196f9cc

This is a placeholder PR for the docs for the PDB to GA KEP kubernetes/enhancements#904

mortent mentioned this pull request Aug 14, 2019

[WIP] PDB GA documentation kubernetes/website#15858

Closed

mortent added a commit to mortent/website that referenced this pull request Aug 14, 2019

[WIP] PDB GA documentation

88f866b

This is a placeholder PR for the docs for the PDB to GA KEP kubernetes/enhancements#904

This was referenced Aug 14, 2019

[WIP] PDB GA documentation kubernetes/website#15859

Closed

Promote PodDisruptionBudget to GA kubernetes/kubernetes#81571

Closed

mortent mentioned this pull request Nov 4, 2019

Promote PodDisruptionBudget e2e test to Conformance kubernetes/kubernetes#84740

Merged

seh mentioned this pull request Sep 26, 2020

Disruption controller handles PodDisruptionBudget with empty selector incorrectly kubernetes/kubernetes#95083

Closed


		## Summary

		[Pod Disruption Budget (PDB)](https://kubernetes.io/docs/tasks/run-application/configure-pdb/)

Conversation

bsalamat commented Mar 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsalamat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timothysc Mar 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smarterclayton Mar 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timothysc Mar 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derekwaynecarr commented Mar 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kow3ns commented Mar 22, 2019

Uh oh!

bsalamat commented Apr 8, 2019

Uh oh!

mortent commented Apr 8, 2019

Uh oh!

kow3ns commented Jul 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timothysc Mar 19, 2019 •

edited

Loading

smarterclayton Mar 19, 2019 •

edited

Loading

timothysc Mar 20, 2019 •

edited

Loading