eviction doesn't work with cronjob pods if PodDisruptionBudget specifies percentage minAvailable/maxUnavailable

[rajusharma]

What happened:
PodDisruptionBudget is not working when CronJob and Deployment have same labels
Error:

kubectl get pdb
NAME                  MIN AVAILABLE   MAX UNAVAILABLE   ALLOWED DISRUPTIONS   AGE
{PDB name}   50%             N/A               0                     3m

kubectl describe pdb
Events:
  Type     Reason                           Age   From               Message
  ----     ------                           ----  ----               -------
  Warning  NoControllers                    16s   controllermanager  found no controllers for pod {CronJob pod name}
  Warning  CalculateExpectedPodCountFailed  16s   controllermanager  Failed to calculate the number of expected pods: found no controllers for pod {CronJob pod name}

What you expected to happen:
PDB should run

How to reproduce it (as minimally and precisely as possible):

• Create CronJob and Deployment with same labels
• Use those labels in PDB selector

Anything else we need to know?:

Environment:

• Kubernetes version (use kubectl version):

kubectl version
Client Version: version.Info{Major:"1", Minor:"13", GitVersion:"v1.13.2", GitCommit:"cff46ab41ff0bb44d8584413b598ad8360ec1def", GitTreeState:"clean", BuildDate:"2019-01-13T23:15:13Z", GoVersion:"go1.11.4", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"11+", GitVersion:"v1.11.8-gke.6", GitCommit:"394ee507d00f15a63cef577a14026096c310698e", GitTreeState:"clean", BuildDate:"2019-03-30T19:31:43Z", GoVersion:"go1.10.8b4", Compiler:"gc", Platform:"linux/amd64"}

• Cloud provider or hardware configuration:
  GKE
• OS (e.g: cat /etc/os-release):
• Kernel (e.g. uname -a):
• Install tools:
• Network plugin and version (if this is a network-related bug):
• Others:

[rajusharma]

@kubernetes/sig-scheduling-bugs

[k8s-ci-robot]

@rajusharma: Reiterating the mentions to trigger a notification:
@kubernetes/sig-scheduling-bugs

In response to this:

@kubernetes/sig-scheduling-bugs

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[tedyu]

From DisruptionController#getExpectedScale:

		for _, finder := range dc.finders() {
			var controllerNScale *controllerAndScale
			controllerNScale, err = finder(pod)

Looks like all the finders returned nil, nil

[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

[rajusharma]

/remove-lifecycle stale

[mortent]

/assign

[mortent]

So the issue here is that the PDB use minAvailable: 50% and also matches pods that belong to a controller that doesn't have a concept of scale. Removing the deployment from the example will result in the same error.

When a PDB specifies either maxUnavailable or minAvailable in percent, the disruption controller needs to look up the "expected" number of pods:

kubernetes/pkg/controller/disruption/disruption.go

Lines 580 to 612 in 29a2b20

	if pdb.Spec.MaxUnavailable != nil {
	expectedCount, err = dc.getExpectedScale(pdb, pods)
	if err != nil {
	return
	}
	var maxUnavailable int
	maxUnavailable, err = intstr.GetValueFromIntOrPercent(pdb.Spec.MaxUnavailable, int(expectedCount), true)
	if err != nil {
	return
	}
	desiredHealthy = expectedCount - int32(maxUnavailable)
	if desiredHealthy < 0 {
	desiredHealthy = 0
	}
	} else if pdb.Spec.MinAvailable != nil {
	if pdb.Spec.MinAvailable.Type == intstr.Int {
	desiredHealthy = pdb.Spec.MinAvailable.IntVal
	expectedCount = int32(len(pods))
	} else if pdb.Spec.MinAvailable.Type == intstr.String {
	expectedCount, err = dc.getExpectedScale(pdb, pods)
	if err != nil {
	return
	}
	var minAvailable int
	minAvailable, err = intstr.GetValueFromIntOrPercent(pdb.Spec.MinAvailable, int(expectedCount), true)
	if err != nil {
	return
	}
	desiredHealthy = int32(minAvailable)
	}
	}
	return

This is not possible with cronjob, only with replicationcontrollers, deployments, replicasets, statefulsets and custom resources that implement the scale subresource.

If you want to set a PDB on a cronjob, it needs to use minAvailable with the actual number and not a percentage.

[rajusharma]

If you want to set a PDB on a cronjob, it needs to use minAvailable with the actual number and not a percentage.

We don't want to set PDB on cronjobs. The issue is, we have some deployments and cronjobs with same labels and PDB doesn't work for those deployments which have same label as cronjobs.

[mortent]

Ah, I see. I think your best option here is to make sure the pods created by the deployments have a label set that makes it possible to target them in the PDB without also capturing the pods from the cronjobs.

[rajusharma]

Yeah we are doing that now by setting different labels for deployment pods and job pods.
Do you think this will remain same or considering it as bug it will be fixed in near future?

[mortent]

I think this is working as intended. Using different labels is the right way to handle this.

[rajusharma]

That means if by accident any cronjob have same labels like deployment, then PodDisruptionBudget for that deployment will not run. Even though there was no change on deployment side, cronjob can break the PDB of deployment. Is this intended 🤔

[mortent]

I don't think we want to prevent PDBs from working with pods with different controllers or prevent PDBs from working with CronJobs. But I agree that it could be useful to provide better feedback if the disruption controller for some reason (this issue highlights one way it can fail) are unable to reconcile a PDB resource.

[liggitt]

what is the benefit to PDBs allowing a controller object they don't understand (and which doesn't support a generic scale subresource) to block eviction?

[mortent]

I can't think of many good reasons why someone would want to set PDBs on cronjob (or any other resource that doesn't support scale). Maybe there are some use cases around batch workloads? Are you suggesting we could remove support for PDBs on pods that doesn't have a known controller or the scale subresource?
For the particular situation in this issue this would mean that we could ignore the pods created by cronjob/job and the PDB will work as expected. The more general problem of PDBs capturing pods belonging to multiple controllers could still be an issue since it might lead to unexpected results.

[mortent]

After looking at the disruption controller in more detail, I think we can handle this better. In the situation described in this issue the controller can ignore the pods from the CronJob when computing the allowed disruptions, but create an event to notify the user that something isn't quite right. I think events are better than conditions for reporting this kind of issues to the user. I have created #85553 for this. More eyes on this would be great to make sure there aren't any scenarios I haven't considered.

The disruption controller also has a failsafe functionality where the allowedDisruptions are set to 0 if the controller encounters any issues. I think we can use conditions to make it clearer to users that this happened. Currently, there isn't any good way to see from the resource itself that allowedDisruptions are 0 because of this. I will try to address this in a separate PR.

[digitalronin]

I can't think of many good reasons why someone would want to set PDBs on cronjob

How about setting a PDB on a cronjob to ensure that the job runs to completion in the event that another process tries to drain the node on which it's running, before it's finished?

[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

[michaelgugino]

/remove-lifecycle rotten

[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

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

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

[k8s-ci-robot]

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

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

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.