Cannot drain node with pod with more than one Pod Disruption Budget

[yaseenhamdulay]

What would you like to be added: When attempting to drain a node that has a pod scheduled on it with more than one PodDisruptionBudget the drain fails with the following message: "Err: This pod has more than one PodDisruptionBudget, which the eviction subresource does not support."

It would be great if this was actually supported.

Why is this needed: It is not possible to upgrade clusters that have any pods with more than one PodDisruptionBudget since they fail on drain.

[yaseenhamdulay]

/sig apps

[mortent]

Can you explain a little bit why you want to do this? As you seem to have found out, I think it is currently considered an error to have a pod covered by more than one PDB.

[fejta-bot]

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta. /lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity. Mark the issue as fresh with /remove-lifecycle rotten. Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta. /lifecycle rotten

[2rs2ts]

I think the path forward here should be to disallow creating pods covered by more than one PDB, so that this becomes a non-issue.

[yaseenisolated]

/remove-lifecycle rotten

[yaseenisolated]

It is very easy for a cluster to get into this state without any feedback that something is potentially wrong.

The first visibility into the problem is that a node drain (and every operation that starts a drain) fails.

I like the idea of disallowing creating pods that are covered by more than one PDB. Additionally PDBs that cause more than one existing pod to be covered by a PDB should be disallowed.

[yaseenhamdulay]

It's a sharp edge that can be a confusing User Experince.

[liggitt]

see also a request to allow forcibly draining nodes (https://github.com/kubernetes/kubernetes/issues/83307) or requesting a drain in a way such that pods that could not be immediately evicted are marked for later eviction/cleanup (https://github.com/kubernetes/kubernetes/issues/66811)

[liggitt]

I think the path forward here should be to disallow creating pods covered by more than one PDB, so that this becomes a non-issue.

That's not workable, a pod create and pdb create request could arrive simultaneously that would violate this invariant (or a second PDB could be created later).

At first glance, this seems similar to a single resource needing to update multiple ResourceQuota objects to be admitted. Is there a reason we cannot honor more than one PDB?

[liggitt]

tentatively labeling this as a bug; it's clear this was intentional, but it's not clear why this behavior is correct

[tedyu]

Looking at the code, it seems we may be able to accommodate more than one PDB. In the first round, pass dry-run being true to EvictionREST#checkAndDecrement and see if there is any non-nill error returned by any PDB.

If there is no non-nil error, we can go through PDBs again without dry-run. If there is non-nil error, just short circuit and return that error.

The body of the above logic is wrapped within retry.RetryOnConflict().

[tedyu]

I have a patch ready implementing the above suggestion.

[mortent]

I think that sounds like a reasonable solution.

My only question is what happens if something fails during the process of updating the PDBs. This would mean that disruptionsAllowed has been decremented on only some of the PDBs. I think this should eventually get resolved as the entries on the DisruptedPods map time out. Also, this should be a similar situation to what currently happens if we fail to delete a pod after we have updated the PDB.

[mml]

It's technically possible to support multiple PDBs, but in practice it may lead to more problems than we anticipate. @davidopp lays it out a bit in this comment.

@tedyu's logic is sound regarding implementation, but the biggest risk is that it could be extremely easy for users to put ~all evictions into gridlock with a "technically allowed" configuration, which can happen two ways:

1. In other than the best case, the odds that all PDBs impacting Pod X are true drops, potentially quite a bit.
2. Due to the lack of transactions, multiple parallel eviction attempts target debiting the same budget. But they race with each other. As @mortent identifies, this results in us "using" the disruption but not actually doing the eviction. The difference compared to today is that this can now happen due to a foreseeable race that won't necessarily ever make progress. It's no longer an uninteresting corner case.

If we offer support for this feature, users will probably expect an effective implementation for draining systems wishing to rely on it and make progress, which I'm not sure we can offer right now.

I do have a couple of thoughts here:

1. Enable this experimentally, guarded by a flag.
2. Enable this only when using a newer version of the PDB API.

[tedyu]

@mml I have added feature gate for the enhancement in #85299

[tedyu]

@yaseenhamdulay There is some question about the necessity for enabling support of multiple PDBs.

Can you chime in ?

[laurentiuspurba]

I have the same issue while draining node. I use Istio v1.4.3 and this is my IstioControlPlane

gateways:
    components:
      ingressGateway:
        enabled: true
        k8s:
          replicaCount: 2
          hpaSpec:
            minReplicas: 2
      egressGateway:
        enabled: true
        k8s:
          replicaCount: 2
          hpaSpec:
            minReplicas: 2

And the same replicCount: 2 and minReplicas: 2 applied to other Istio resources. And I can see all the pods are duplicated while issuing kubectl -n istio-system get pods

▶ kubectl -n istio-system get pods
NAME                                      READY   STATUS    RESTARTS   AGE
istio-citadel-64d4866c95-qgcgj            1/1     Running   0          103m
istio-citadel-64d4866c95-t5l84            1/1     Running   0          103m
istio-egressgateway-85495856db-2n5wn      1/1     Running   0          103m
istio-egressgateway-85495856db-lfgg5      1/1     Running   0          103m
istio-galley-65bd4d4c96-njv8k             2/2     Running   0          47m
istio-galley-65bd4d4c96-tc5rg             2/2     Running   0          24h
istio-ingressgateway-5b694f6978-bf2bh     1/1     Running   0          103m
istio-ingressgateway-5b694f6978-pc4f9     1/1     Running   0          147m
istio-pilot-6777944bc8-btfcc              2/2     Running   0          22h
istio-pilot-6777944bc8-t47lf              2/2     Running   0          103m
istio-policy-77875887d4-sh9vh             2/2     Running   0          47m
istio-policy-77875887d4-wxgrt             2/2     Running   0          103m
istio-sidecar-injector-75ff97b4d8-bvzts   1/1     Running   0          54m
istio-sidecar-injector-75ff97b4d8-mksvr   1/1     Running   0          147m
istio-telemetry-78d4dc589-6wgjf           2/2     Running   0          47m
istio-telemetry-78d4dc589-b6rm7           2/2     Running   0          103m
istio-tracing-cd67ddf8-9xh4s              1/1     Running   0          47m
istiocoredns-5f7546c6f4-hppd2             2/2     Running   0          103m
istiocoredns-5f7546c6f4-wjcw7             2/2     Running   0          47m
kiali-7964898d8c-l8bbk                    1/1     Running   0          47m

The node draining stop and was aborted with this error

error: error when evicting pod "istio-ingressgateway-5b694f6978-pc4f9": This pod has more than one PodDisruptionBudget, which the eviction subresource does not support.

Is there a way that I can drain the node without getting this error?

[michaelgugino]

For me, having multiple PDBs apply to a single pod doesn't make sense. PDBs should be applied to a homogeneous group of pods. IMO, if you have a pod covered by multiple PDBs, it's because you misconfigured one or more of your PDBs.

I don't have any particular problem with the suggested PR implementation, it seems sensible. If we start allowing more than 1, does 1 become 3 or 5 or 80? It seems it could get intractable without some reasonable upper limit, very easy for a user to create a situation where draining is still impossible. TBD if this situation is worse, the same, or better than just denying more than 1 PDB.

I kind of view the situation as one in the same as MaxUnavailable = 0. It's going to require administrator intervention.

https://github.com/kubernetes/kubernetes/issues/66811 as @liggitt mentioned has some interesting ideas.

I think optionally setting taints or labels while using drain (either via CLI or library) can help some of these things along.

[nairb774]

Can you explain a little bit why you want to do this?

I think multiple PDBs applying to a Pod can make sense if you think about disruptions at different aggregations of an application. If you imagine the smallest unit of aggregation of an application being a Deployment, there might be a desire to limit disruptions for that deployment. But then the next level up the aggregation tree could be a Service. It isn't all that far fetched for a Service to blend together one or more deployments, and as a result want to limit disruptions at that level.

It was based on this thinking that I ended up here after experimenting with ways to simplify management of developer applications by automatically creating PDBs for each Deployment and Service in the cluster. The idea being that for each deployment there should be a maximal amount of disruption it could endure at any given time. I tried to cap every Deployment at 25% max unavailable (number pulled from thin air). In a similar idea, I capped the amount of disruption that a Service could endure as well. Since a single service could be made up of one or more constituent parts, each of which are blended together as a single service (an example could be primary+canary deployments), it seemed sensible to apply a disruption budget at this level as well. I picked the number 10% unavailable for services. The expectation was that if a service was made of a single deployment, the 10% rule would be more strict than the 25% rule and would automatically constrain disruptions. If the service ever became more complex, the 10% and 25% rules would enable a sensible level of SLA for each level of aggregation. It is entirely possible that some services or deployments would need to be further tightened - the hope would be that an additional, manual, PDB would be added to restrict the behavior as needed and the automatic ones could be left in place.

This was bore out of an effort to deploy "sensible defaults" to our clusters. The lack of PDBs means that things which expect to be able to lean on them (VPA as an example) can result in fairly large service disruptions when they kick in. I have yet to deploy any sort of chaos monkey tooling, but I'd imagine being able to partially lean on sensible PDBs is going to go a long way towards having chaos without large disasters.

I hope that explains some some reasoning behind having multiple PDBs apply to a single Pod and how one might expect them to interact.

[2rs2ts]

I think @yaseenisolated had it right.

I like the idea of disallowing creating pods that are covered by more than one PDB. Additionally PDBs that cause more than one existing pod to be covered by a PDB should be disallowed.

Rather than letting users shoot themselves in the foot we should protect them.

[michaelgugino]

I think @yaseenisolated had it right.

I like the idea of disallowing creating pods that are covered by more than one PDB. Additionally PDBs that cause more than one existing pod to be covered by a PDB should be disallowed.

Rather than letting users shoot themselves in the foot we should protect them.

The problem with this is the selectors. While it's somewhat tractable to compare the selectors of other PDBs in a given namespace, there is nothing stopping a pod being created in the future that has labels that match multiple PDBs.

[2rs2ts]

there is nothing stopping a pod being created in the future that has labels that match multiple PDBs

Well, our belief is that there should be such a thing to stop it, in much the same way that a Pod that mounts a non-existent volume can't be created.

[yaseenhamdulay]

We could do both. Prevent creation of pods that match multiple PDBs (its an invalid state) and prevent PDBs from being created whose selectors match pods that have another PDB associated with them.

[michaelgugino]

We could do both. Prevent creation of pods that match multiple PDBs (its an invalid state) and prevent PDBs from being created whose selectors match pods that have another PDB associated with them.

For this to be completely implemented, you'd also need to check updates to all pods and all PDBs. Also, you have to compare the selectors, not just if there will be any pods matching both, as there might not be any matching pods at creating of a PDB. Also, need to sort out to do with things like stateful sets and deployments that are not pods, but will attempt to create them.

I'm heavily on the side of requiring users to manage their PDBs and not create overlapping ones. In some limited cases, it might be desirable to have multiple PDBs in some limited circumstances even if the eviction API doesn't allow you to evict pods covered by PDBs. For instance, let's say you have 3 pods covered by PDB "A". At some point, you're going to do some maintenance, but you don't want a certain class of pods to get interrupted. You might create another PDB "B" which is temporary that covers those same 3 pods and many others. At the end of your maintenance, you delete the PDB "B" to allow eviction operations to resume as normal.

My preferences are

1. Maintain the status quo
2. Allow multiple PDBs in eviction
3. Prevent PDB/Pod Creation when a PDB covers multiple pods.

in that order.

[2rs2ts]

It's already common for replication controllers like Deployments and StatefulSets to be "valid" but create pods that are invalid, and to only be able to discover this once you've applied the configuration. So while I think it would be sweet for there to be validation for replication controllers at apply time, it's not necessary since us k8s users are already so used to having to wait to see if the pods get successfully created or not.

The problem with allowing multiple PDBs in eviction is ambiguity. It is much better to be unambiguous when it comes to matters of service availability. And I do not see a problem with comparing selectors, seems easy to me. You merely need to disallow any PDB with a selector that has an intersection with any other PDB's selector at creation time. As far as checking that a pod would match two PDBs at creation time, we already have code somewhere to check that it matches two PDBs at eviction time, so just repurpose that, no? Allowing multiple PDBs in eviction would be much more complex as you would have to decide which PDB gets to decide whether eviction occurs, or if both get "combined" somehow.

In the case you described above, @michaelgugino the right thing to do would be to create PDBs for each of the kinds of the "many other pods" you described rather than deleting PDB A and making a super-PDB B.

[michaelgugino]

It's already common for replication controllers like Deployments and StatefulSets to be "valid" but create pods that are invalid, and to only be able to discover this once you've applied the configuration.

I'm not sure if this is true. What we're talking about here would require an admission controller that would block the creation or update of a pod, rather than the pod object being created and containers failing to do something useful. Is it possible to craft a deployment or replica set that results in 0 pod objects being created intentionally? By 0 pod objects, I mean 'kubectl get pods -n mynamespace' resulting in 0 results.

The problem with allowing multiple PDBs in eviction is ambiguity.

It's only ambiguous if we don't define what the behavior is. Right now, the behavior is multiple PDBs results in eviction requests never succeeding. This could be desirable or undesirable depending on an individual's situation. IMO, we have now released this 'API' to the userbase at large, and they very well may be utilizing it in the manner in which I've described earlier, and this might be very well intended. I think right now having multiple PDBs as a back-stop to prevent draining of certain workloads might be desirable in some cases, though technically this is abusing the implementation.

In any case, it's easy enough to say "each PDB must allow eviction for eviction to occur" aka 'logical and' across PDBs. I don't think that's ambiguous if we were to go that route.

Disallowing creation of multiple PDBs is somewhat orthogonal to how the eviction API works. Someone could write and deploy a webhook to their cluster if they desired this behavior, or an opt-in admission controller could be created. It might be worth creating a dedicated issue to discuss blocking the creation of PDBs.

I think this issue is more or less about if we should allow the eviction API to handle multiple PDBs, and if so, how we should handle that, rather than blocking their creation.

[prydonius]

/assign @mortent /triage accepted

[rbtcollins]

It seems like the behaviour of overlapping PDBs can be well defined mathematically: a disruption is permitted if for all PDBs covering a pod, disrupting the pod will not violate the PDB. The implementation issue I see in the discussion upthread is due to the eventual consistency relationship between PDBs and the tracked resource being exacerbated: rather than a potential skew between one PDB and one un-deleted pod; now there is potential skew between N decremented PDBs and (M undecremented PDBs + an undeleted pod).

Is there some way to reduce that possibility for skew? e.g. a PDBJournal object or something, which would be written first, naming the pdbs to be updated, the pod to be deleted, and which would be deleted afterwards. This would provide a repair mechanism for the control loop. The cost would be 1 write and 1 delete per PDB update.

We could perhaps only do this in the case that there are multiple PDBs in use, in which case there are already N>1 writes to PDBs + 1 delete = 3 operations being sent to etcd anyway, so the overhead would be 40% at worst.

[michaelgugino]

It's certainly possible to implement something that works. The corner cases are going to be quite difficult. Consider simultaneous drains across two nodes. Pod-A and Pod-B. Both pods have their own PDB, as well as PDB-C.

How many PDBs per-pod should we support? 2? 100?

At the end of the day, we still need to build features users can use and understand. While the UX might be better for the small minority of advanced users, it will be more complex and burdensome for most users. People will advance-PDB use cases can build their own locking mechanims for particular pods using one or more PDBs if they so choose.

[rbtcollins]

What I sketched would handle that two-node case with 3 PDBs in a venn diagram situation without it being a corner case, as long as the processing of deletions and pdb evaluation is done in a controller, which I haven't checked the code to see - sorry :/ - and this will be my last comment, as I don't want to make the bug noisy.

I agree that large numbers of PDBs affecting a single pod would be hard to operate, and undesirable; OTOH I don't think anyone is calling for that: the use cases described above all are modest: changing gracefully from one PDB configuration to another without a period where nodes cannot be drained, and modelling complicated multi-component applications a little better (like e.g. FoundationDB, which is what brought me to this bug).

I am in complete agreement that the UX of what is built should be paramount - user centred design matters a great deal. Thank you for centering that. It's not clear to me - as a user of Kubernetes - how having multiple PDBs is any worse than one, given their existence as separate API documents these days. Having a great 'what PDB for this pod' or good error signalling, should make troubleshooting drain / delete situations straight forward, and thats true whether just one, or many, PDBs apply to a pod.

Lastly, I'm not sure how one would build ones own locking mechanism for particular pods using 'one or more PDBs', since the whole point of this bug is that Kubernetes stops permitting any deletion when more than one PDB exists for a pod.

[michaelgugino]

I think having multiple PDBs supported via the eviction API will lead to a worse UX, especially if the number was more than 2. Today, the UX is clear, even if somewhat not initially intuitive or what some users might find agreeable. It's simple to understand and work with.

Here's how one can build a locking system with multiple PDBs today: I have pod-A, it has primary-pod-a-pdb and generic-lock-pdb. It is unevictable. You could build an eviction webhook to signal some other component (or the webhook itself can do the work) to capture the request. The request is forwarded to some system (the webhook might put a CR in some namespace, such as the same namespace as the pod). A system watching the namespace sees the request, validates it's okay to remove the pod, removes the label which matches generic-lock-pdb.

This would allow users to use PDBs in a very flexible way, such as relying on outside information (eg, not pod readiness) to coordinate whether a pod can be evicted or not.

This is only one example. You could also use VWH without multiple PDBs to do something similar.

[paulgmiller]

I actually constraining it to one pdb but I have definitely seen this error confuse some users. Perhaps we could at least spit out the names of the pdbs? Maybe I should create another issue for that.

https://github.com/kubernetes/kubernetes/blob/edac6fce2ad4a1366a73afc8ac9ca50b2fbf7fc7/pkg/registry/core/pod/storage/eviction.go#L216

[k8s-triage-robot]

This issue is labeled with priority/important-soon but has not been updated in over 90 days, and should be re-triaged. Important-soon issues must be staffed and worked on either currently, or very soon, ideally in time for the next release.

You can:

• Confirm that this issue is still relevant with /triage accepted (org members only)
• Deprioritize it with /priority important-longterm or /priority backlog
• Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

[k8s-ci-robot]

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

[chijinxina]

I think multiple PDBs is meaningful. For example, the following scene.

A service is sharded, it has 10 groups and each group has 2 replicas. For each replica under the group I only allow 1 unavailable, and for instances under the all groups, I allow 25% to be unavailable.

pod-0-0
pod-0-1
...
pod-9-0
pod-9-1

Multiple PDBs can solve the above problem, but for prior discussion see 113891 and 75957. The main problem is multiple parallel eviction attempts target debiting the same budget due to the lack of transactions. So if we support a new MultiplePodDisruptionBudget maybe solve this problem, the API may be defined as follows:

kind: MultiplePodDisruptionBudget
metadata:
  name: test-multi-pdb
spec:
  ruleGroups:
    - name: "rule1"
      maxUnavailable: 1
      selector:
        matchLabels:
          app.kubernetes.io/instance: test
          app.kubernetes.io/group-name: "0"
    - name: "rule2"
      maxUnavailable: 25%
      selector:
        matchLabels:
          app.kubernetes.io/instance: test
status:
  ruleGroupStatus:
    - name: "rule1"
      currentHealthy: 1
      desiredHealthy: 1
      disruptionsAllowed: 1
      expectedPods: 1
      observedGeneration: 1
    - name: "rule2"
      currentHealthy: 1
      desiredHealthy: 1
      disruptionsAllowed: 1
      expectedPods: 1
      observedGeneration: 1```

[2rs2ts]

For those reading this thread in 2023, my company turned what I posted here into OPA gatekeeper policies and have been enforcing them for years. While it is memory-consuming to have to catalog every Pod so we can get all their labels, the end result is that we no longer have nodes that can't drain because one of our colleagues didn't read the PDB documentation. https://github.com/kubernetes/kubernetes/issues/75957#issuecomment-654374595

While I'm all for making K8s a more robust platform and I think the above suggestion for multiple PDBs could be fine, FWIW we have extremely similar use cases in our company and we don't find a need for multiple PDBs. We just make sure the selectors for the PDBs are disjoint and we make sure that if all PDBs are respected then our overreaching availability goals are met. So, in @chijinxina's example, we simply add replicas for each group (besides, 2 is never HA, 3 is the bare minimum for HA, but for important things we do even more) such that each group allowing only 1 disruption also leads to the sum of the groups allowing no more than 25% of the overall system to be disrupted. I'm not invalidating that use case completely, of course; I think it's fine to want to have 25% overall maxUnavailable but only 3 replicas per group, which would lead to only being able to achieve overall 33% maxUnavailable in my example. I'm only stating this so that people facing issues like we did have a workaround testimony they can find value in until we get the ability to support multiple PDBs.

In the meantime, I still strongly believe K8s itself should refuse to admit PDBs that would cause label overlap as well as to admit Pods that would draw from multiple PDBs, until we have the ability to define multiple PDBs for a single Pod. OPA Gatekeeper is nice and all but using up several GB of RAM for redundant cataloging is not great, and we'd prefer it is K8s itself took care of this since it can do it much easier, faster, and more efficiently than a separate component with no access to etcd can.

@michaelgugino

Is it possible to craft a deployment or replica set that results in 0 pod objects being created intentionally? By 0 pod objects, I mean 'kubectl get pods -n mynamespace' resulting in 0 results.

I know this comment is years too late, but, define a podSpec that mounts a ConfigMap that does not exist. You can create the Deployment and just fine but the Pods will never launch; I believe they get a CreateContainerConfigError. I have seen user errors like this so many times I've lost count. It's an extremely common problem and I think any cluster admin dealing with multi-tenanted clusters is going to know exactly what I'm talking about. 😅

[mjhoffman65]

For those reading this thread in 2023, my company turned what I posted here into OPA gatekeeper policies and have been enforcing them for years. While it is memory-consuming to have to catalog every Pod so we can get all their labels, the end result is that we no longer have nodes that can't drain because one of our colleagues didn't read the PDB documentation. #75957 (comment)

@2rs2ts Are you able to share the Gatekeeper constraint template your company is using?

[2rs2ts]

@mjhoffman65 wish I could, but, it's not open-sourced and I would have to go through a bunch of legal clearance to share it. It's unlikely to get cleared because it is not a whole product where getting community feedback would benefit our company and it's not remarkable enough to be something we can show off, it's literally just a couple dozen lines of rego.

I can at least say that it only took us like a day to implement since it's really conceptually easy, though we did have the advantage of already having some groundwork laid (like, we were already cataloguing Deployments, StatefulSets, etc.) You just need to not admit anything with a podSpec that would launch Pods that would match multiple PDBs (and not admit any such Pods, too), as well as not admit any PDBs whose selectors are not disjoint with all other PDBs'.

That last bit is super easy to implement, and can already get you pretty far. In fact, after we got everyone on the same page, we actually took out the rule that prevents Pod admission if it would apply to multiple PDBs, since people had gotten used to doing things The Right Way™ (by our standards, anyway) and we didn't need to enforce it as much, and we really needed to stop having to catalogue tens of thousands of Pods per cluster. So, you can build it up piecemeal like that, starting with just the rule to not allow PDBs that overlap to be created, then build up more from there. If you get stuck on writing the rego, I think you should just ask Stack Overflow for help. I still find rego weird to work with, personally, so I don't blame you for wanting to use someone's prior art.