Allow updates/patches to pod disruption budgets#69867
Allow updates/patches to pod disruption budgets#69867k8s-ci-robot merged 2 commits intokubernetes:masterfrom
Conversation
|
/kind feature |
|
/sig apps |
|
@davidmccormick |
|
ok have added a release note - does it look ok? |
|
@davidmccormick
the tool that collects release notes also inserts a PR link next to them for better context. i would defer to the maintainers for further comments on the PR itself. |
|
ok thanks, have updated the release note as suggested |
|
@hongchaodeng @janetkuo can you review this PR please? |
|
@mattfarina @janetkuo |
|
@kubernetes/sig-apps-feature-requests @kubernetes/sig-apps-pr-reviews can some review or comment. This seems like we should do ? |
|
@davidmccormick Sorry for the delay. Are you able to pick this back up? I personally want to see this happen too and I'll volunteer to be a reviewer and help track down an approver. If you don't have time to continue this, please let us know so we can open a new PR. |
|
/lgtm |
There was a problem hiding this comment.
what is the purpose of setting and restoring the generation field? it seems like a no-op
There was a problem hiding this comment.
if possible, we should drop that behavior... validation shouldn't mutate objects, even temporarily
|
one question about cleaning up the generation set/restore in ValidatePodDisruptionBudgetUpdate, lgtm otherwise |
|
Hi @liggitt, I couldn't work out why the pdb validation wanted to save and restore the generation number, so I've removed it and re-run the tests and still behaving as expected. Looking better? |
test/e2e/apps/disruption.go
Outdated
There was a problem hiding this comment.
I thought that the time was to allow the pod disruption budget to be processed. The difference is that now we specifically wait for the pdb to be processed when we create or update the pdb in the waitForPdbToBeProcessed() function.
There was a problem hiding this comment.
The difference is that now we specifically wait for the pdb to be processed when we create or update the pdb in the waitForPdbToBeProcessed() function.
That indicates the pdb controller has observed the update. It does not indicate the informer in the apiserver process (used by the eviction REST handler) has observed the update. In practice, the in-process informer is likely to always see the update events first, but that is not guaranteed.
Please keep an eye on this test post-merge to see if this increases flakiness.
|
validation change lgtm. one comment on dropping the timeout on the test. I didn't look at the new test, will defer to @kubernetes/sig-apps-pr-reviews for that |
|
/lgtm |
|
/approve |
… running ValidatePodDisruptionBudget only.
|
/retest |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: davidmccormick, kow3ns, liggitt The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest Review the full test history for this PR. Silence the bot with an |
|
will this be backported to 1.13 and 1.14 ? |
|
No, backports are limited to critical bug fixes and fixes for regressions. Features are not backported. |
What this PR does / why we need it: It removes the immutability of pod disruption budgets. Encouraging teams to use Pod Disruption Budgets is a vital part of us allowing us to effectively perform maintenance on our clusters without inadvertently taking out one of the services running within it. With the present Pod Disruption Budgets being immutable this presents problems incorporating them into application charts (neither kubectl with force or helm can properly handle changing a pdb object - forcing an administrator to delete and re-create it). It has been said in the issue that there is no longer any design reason requiring pdbs to be immutable, so let's remove this control and allow them to be patched.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #45398
#45398
Special notes for your reviewer:
I have removed the DeepEqual check and then updated the tests in line with updates being allowed. In updating the tests I found that the order two of the test parameters had been accidentally transposed so I have corrected that and updated them to show that updates are now successful but pdb validation still applies - i.e. you are still not allowed to mix MaxUnavailable and MinAvailable values.
Release note: