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

PodDisruptionBudget by percentage doesn't work with StatefulSets #39125

Closed
a-robinson opened this issue Dec 22, 2016 · 5 comments · Fixed by #39454
Closed

PodDisruptionBudget by percentage doesn't work with StatefulSets #39125

a-robinson opened this issue Dec 22, 2016 · 5 comments · Fixed by #39454
Assignees
@foxish
Labels
area/stateful-apps sig/apps Categorizes an issue or PR as relevant to SIG Apps.

Comments

@a-robinson
Copy link
Contributor

Is it intentional that the disruption controller isn't aware of StatefulSets? They appear to work together when the budget is configured with a static minimum number of pods, but not when configured with a percentage because the controller finders don't include StatefulSets:

kubernetes/pkg/controller/disruption/disruption.go

Line 204 in f49442d

return []podControllerFinder{dc.getPodReplicationControllers, dc.getPodDeployments, dc.getPodReplicaSets}

kubernetes/pkg/controller/disruption/disruption.go

Line 557 in f49442d

err = fmt.Errorf("asked for percentage, but found no controllers for pod %q", pod.Name)

The events for the disruption budget resource show the error message from the second link ("Failed to calculate the number of expected pods: asked for percentage, but found no controllers for pod "cockroachdb-0"), leaving it with no idea how many pods are expected.

This is with client and server both at v1.5.1.

@foxish @erictune
@a-robinson a-robinson mentioned this issue Dec 22, 2016
Merged
@foxish
Copy link
Contributor

foxish commented Dec 22, 2016

Thanks @a-robinson ! Looks like we missed this one. The real fix for this issue would be to use controllerRefs instead of hard-coded lists of controllers, which will ensure that even custom/third party controllers with a scale subresource are covered, but that's in the 1.6 timeframe. For now, I'll send a PR to add StatefulSet to the list of controllers it recognizes.

/cc @kow3ns @enisoc

@foxish foxish self-assigned this Dec 22, 2016
@foxish foxish added area/stateful-apps sig/apps Categorizes an issue or PR as relevant to SIG Apps. labels Dec 22, 2016
@a-robinson
Copy link
Contributor Author

Thanks @foxish! Is the simpler fix something that'll be pulled into a 1.5.x patch release?

@foxish foxish mentioned this issue Jan 5, 2017
Merged
@foxish
Copy link
Contributor

foxish commented Jan 5, 2017

@a-robinson Just created the PR. I expect that it should get into the next patch release.
/cc @saad-ali.

k8s-github-robot pushed a commit that referenced this issue Jan 5, 2017
Merge pull request #39454 from foxish/fix-stateful-set-detection
f8b7083 
Automatic merge from submit-queue (batch tested with PRs 39435, 39454)

Fix PDB by percentages for StatefulSet pods

Previously, PDBs defined in terms of percentages would error out with StatefulSet as they did not know how to find the scale associated.
This change teaches the disruption controller to also look at StatefulSets and their scale.

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #39125

**Release note**:
```release-note
Fix issue with PodDisruptionBudgets in which `minAvailable` specified as a percentage did not work with StatefulSet Pods.
```

cc @a-robinson @kow3ns @kubernetes/sig-apps-misc
@foxish
Copy link
Contributor

foxish commented Jan 11, 2017

@mwielgus we need to cherrypick #39454 and #39544 into release-1.5 to have this fixed for the next point release. But there are merge conflicts. I am not familiar enough with the disruption controller to understand the other changes that have gone in. Could you please take a look?

@foxish foxish reopened this Jan 11, 2017
@foxish
Copy link
Contributor

foxish commented Jan 12, 2017

@davidopp, @mwielgus, can someone help cherrypick this into 1.5? I'm sure other folks are going to hit this issue as well as they use StatefulSets with PDB.

@foxish foxish closed this as completed Mar 2, 2017
@cppforlife cppforlife mentioned this issue Feb 15, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@foxish foxish

Labels
area/stateful-apps sig/apps Categorizes an issue or PR as relevant to SIG Apps.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix PDB by percentages for StatefulSet pods foxish/kubernetes
2 participants
@foxish @a-robinson