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.

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

Allow updates/patches to pod disruption budgets #69867

Merged
merged 2 commits into from May 15, 2019

Conversation

@davidmccormick
Copy link
Contributor

commented Oct 16, 2018

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:

Allow updates/patches to pod disruption budgets
@neolit123

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

/kind feature
/sig api-machinery
/ok-to-test

@neolit123

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

/sig apps
/remove-sig api-machinery

@neolit123

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

@davidmccormick
please add a release note explaining the change.

@davidmccormick

This comment has been minimized.

Copy link
Contributor Author

commented Oct 18, 2018

ok have added a release note - does it look ok?

@neolit123

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

@davidmccormick
normally RN are kept down to a sentence or two.
the title seems like a good one.

Allow updates/patches to pod disruption budgets

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.
thanks.

@davidmccormick

This comment has been minimized.

Copy link
Contributor Author

commented Oct 19, 2018

ok thanks, have updated the release note as suggested

@davidmccormick davidmccormick changed the title WIP: Allow updates/patches to pod disruption budgets Allow updates/patches to pod disruption budgets Nov 2, 2018

@davidmccormick

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2018

@hongchaodeng @janetkuo can you review this PR please?

@nrvnrvn

This comment has been minimized.

Copy link

commented Jan 12, 2019

@mattfarina @janetkuo
hey folks,
any chance to have a look at it?

@krmayankk

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2019

@kubernetes/sig-apps-feature-requests @kubernetes/sig-apps-pr-reviews can some review or comment. This seems like we should do ?

@enisoc

This comment has been minimized.

Copy link
Member

commented Feb 4, 2019

@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.

@mortent

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

/lgtm
/assign @liggitt

@@ -43,14 +42,10 @@ func ValidatePodDisruptionBudget(pdb *policy.PodDisruptionBudget) field.ErrorLis
}

func ValidatePodDisruptionBudgetUpdate(pdb, oldPdb *policy.PodDisruptionBudget) field.ErrorList {
allErrs := field.ErrorList{}

restoreGeneration := pdb.Generation

This comment has been minimized.

Copy link
@liggitt

liggitt Apr 26, 2019

Member

what is the purpose of setting and restoring the generation field? it seems like a no-op

This comment has been minimized.

Copy link
@liggitt

liggitt Apr 26, 2019

Member

if possible, we should drop that behavior... validation shouldn't mutate objects, even temporarily

@liggitt

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

one question about cleaning up the generation set/restore in ValidatePodDisruptionBudgetUpdate, lgtm otherwise

@k8s-ci-robot k8s-ci-robot removed the lgtm label Apr 26, 2019

@davidmccormick

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

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?

@liggitt liggitt removed the sig/auth label May 2, 2019

@@ -189,10 +175,6 @@ var _ = SIGDescribe("DisruptionController", func() {
}

if c.shouldDeny {
// Since disruptionAllowed starts out false, wait at least 60s hoping that
// this gives the controller enough time to have truly set the status.
time.Sleep(timeout)

This comment has been minimized.

Copy link
@liggitt

liggitt May 2, 2019

Member

won't this make the test flaky?

This comment has been minimized.

Copy link
@davidmccormick

davidmccormick May 14, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@liggitt

liggitt May 14, 2019

Member

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.

@liggitt

This comment has been minimized.

Copy link
Member

commented May 2, 2019

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

@mortent

This comment has been minimized.

Copy link
Member

commented May 7, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label May 7, 2019

@kow3ns

This comment has been minimized.

Copy link
Member

commented May 13, 2019

/approve

Remove the generation altering code - validate an update for a PDB by…
… running ValidatePodDisruptionBudget only.

@davidmccormick davidmccormick force-pushed the HotelsDotCom:allow-updates-to-pdbs branch from 03a1b36 to 3537eed May 14, 2019

@k8s-ci-robot k8s-ci-robot added sig/auth and removed lgtm labels May 14, 2019

@davidmccormick

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

had to rebase @mortent @liggitt @kow3ns can you re-approve please?

@davidmccormick

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

/retest

@liggitt

This comment has been minimized.

Copy link
Member

commented May 14, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label May 14, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

[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

Needs approval from an approver in each of these files:

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

@fejta-bot

This comment has been minimized.

Copy link

commented May 14, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit de6ce5e into kubernetes:master May 15, 2019

19 of 20 checks passed

pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
cla/linuxfoundation davidmccormick authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.