Skip to content

Implement DisruptionController.#25921

Merged
k8s-github-robot merged 1 commit intokubernetes:masterfrom
mml:db.controller
Aug 17, 2016
Merged

Implement DisruptionController.#25921
k8s-github-robot merged 1 commit intokubernetes:masterfrom
mml:db.controller

Conversation

@mml
Copy link
Copy Markdown
Contributor

@mml mml commented May 19, 2016 •
edited by k8s-oncall
Loading

Part of #12611

This currently also includes a pending commit from #25895

This change is Reviewable

@mml mml added this to the v1.3 milestone May 19, 2016
@mml mml added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels May 19, 2016
@k8s-github-robot k8s-github-robot added kind/new-api size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 19, 2016
@mml mml force-pushed the db.controller branch from e7b823e to bf1f3d3 Compare May 20, 2016 00:43
@davidopp davidopp assigned davidopp and unassigned deads2k May 24, 2016
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 26, 2016
@mml mml force-pushed the db.controller branch from bf1f3d3 to 7ff56fd Compare June 2, 2016 23:36
@k8s-github-robot k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 2, 2016
@goltermann
Copy link
Copy Markdown
Contributor

goltermann commented Jun 6, 2016

@davidopp @mml Is this still targeted for v1.3?

@davidopp
Copy link
Copy Markdown
Contributor

davidopp commented Jun 6, 2016

Sorry, I didn't review the 1.3 milestone on PRs, only issues. This is not for 1.3.

@davidopp davidopp removed this from the v1.3 milestone Jun 6, 2016
@mml mml force-pushed the db.controller branch 5 times, most recently from 5bf1872 to 1b7f7a8 Compare June 14, 2016 01:41
@fejta
Copy link
Copy Markdown
Contributor

fejta commented Jun 14, 2016

@k8s-bot node e2e test this issue #24479

@mml mml force-pushed the db.controller branch 3 times, most recently from 39fe115 to b99ad67 Compare June 21, 2016 01:09
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2016
@mml mml force-pushed the db.controller branch from b99ad67 to 639ab00 Compare June 23, 2016 16:43
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2016
@mml mml removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Jul 28, 2016
@mml
Copy link
Copy Markdown
Contributor Author

mml commented Aug 10, 2016

@davidopp PTAL

Review status: 0 of 6 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

@timothysc
Copy link
Copy Markdown
Contributor

timothysc commented Aug 12, 2016

@davidopp if we're going to land this for 1.4 we should do it soon...

davidopp
davidopp reviewed Aug 14, 2016
View reviewed changes
pkg/client/cache/listers.go Outdated
Store
}

// GetPodPodDisruptionBudgets returns a list of PodDisruptionBudgets matching a pod. Returns an error only if no match PodDisruptionBudgets are found.
Copy link
Copy Markdown
Contributor

@davidopp davidopp Aug 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/match/matching/

@mml
Copy link
Copy Markdown
Contributor Author

mml commented Aug 16, 2016

Review status: 0 of 6 files reviewed at latest revision, 8 unresolved discussions.

pkg/client/cache/listers.go, line 719 [r1] (raw file):

Previously, davidopp (David Oppenheimer) wrote…

s/match/matching/

Done.

pkg/client/cache/listers.go, line 740 [r1] (raw file):

Previously, davidopp (David Oppenheimer) wrote…

maybe continue instead?

Done. I left a TODO that we should add an event to the PDB when this happens. For now it's just a warning log.

pkg/controller/disruption/disruption.go, line 76 [r1] (raw file):

Previously, davidopp (David Oppenheimer) wrote…

I think comments about types, functions, etc. are supposed to start with the name of the thing (rather than "This"). You used the "This" style at least one other time (immediately below) so maybe best to check the whole PR.

Done.

pkg/controller/disruption/disruption.go, line 83 [r1] (raw file):

Previously, davidopp (David Oppenheimer) wrote…

maybe "function type" rather than "function interface" (just to avoid confusion with an interface type...); also s/This/podControllerFinder/

Done.

pkg/controller/disruption/disruption.go, line 230 [r1] (raw file):

Previously, davidopp (David Oppenheimer) wrote…

Why not continue? If a pod is managed by some RS's that have Deployments and some RS's that don't have Deployments, I assume we want this function to return all of the Deployments (just like getPodReplicaSets() returns all of the Deployment-less ReplicaSets, by virtue of having a "continue" if it finds a Deployment)?

Good catch. Fixed.

pkg/controller/disruption/disruption.go, line 462 [r1] (raw file):
If MinAvailable is specified as an integer, why do we care if there are 0 controllers? In that case, I assume the user knew what they were doing.

1 controller is arguably an error, I agree. But is this (evaluating a PDB with an absolute ready count) the spot to police that? I only object here because I need to unequivocally determine the number to multiply the percentage by.

Put another way, specifying a percentage implies one and only one controller per pod, and specifying an integer does not.

Re: expectedCount, that's correct, but I'm not sure what you're proposing I change there.

pkg/controller/disruption/disruption.go, line 517 [r1] (raw file):

Previously, davidopp (David Oppenheimer) wrote…

This function takes pdb as an argument but doesn't use it. It also doesn't use dc so I guess it technically doesn't need to be a member function, but that doesn't seem like a big deal.

I think that was an artifact of me factoring that out of a different function. Reduced it to a normal function of one argument.

test/e2e/disruption.go, line 70 [r1] (raw file):

Previously, davidopp (David Oppenheimer) wrote…

Should you delete the other PDB you just created first?

You mean the one in the prior It() block? Each It block run in its own namespace. Nothing to delete.

Comments from Reviewable

@mml
Copy link
Copy Markdown
Contributor Author

mml commented Aug 16, 2016

@davidopp PTAL

Review status: 0 of 6 files reviewed at latest revision, 8 unresolved discussions.

Comments from Reviewable

@mml mml mentioned this pull request Aug 16, 2016
Closed
@mml
Copy link
Copy Markdown
Contributor Author

mml commented Aug 16, 2016

This is weird.

GitHub pull request #25921 of commit 7469e31b6e2f884b0e78b6d612dfc45938e8d3cb, no merge conflicts.
[...]
Seen 55 remote branches
Checking out Revision 785feb748883b0ae2b939157f05f30c9ea2b92b3 (origin/pr/30686/merge)
[...]
Caused by: hudson.plugins.git.GitException: Command "/usr/bin/git checkout -f 785feb748883b0ae2b939157f05f30c9ea2b92b3" returned status code 128:
stdout: 
stderr: fatal: reference is not a tree: 785feb748883b0ae2b939157f05f30c9ea2b92b3

Why is it trying to checkout 785feb748883b0ae2b939157f05f30c9ea2b92b3 from #30686 ?

@mml
Copy link
Copy Markdown
Contributor Author

mml commented Aug 16, 2016

@k8s-bot test this issue #30691

@mml
Copy link
Copy Markdown
Contributor Author

mml commented Aug 16, 2016

@davidopp squashed and as discussed I'll address open issues in a followup.

@davidopp
Copy link
Copy Markdown
Contributor

davidopp commented Aug 16, 2016

LGTM

Just squash the commits and I'll add the label.

@mml and I discussed two followups:

  • consider making the way expectedCount and rules about permitted controller configurations (specifically, considering it an error if a pod covered by a PDB has 0 controllers or > 1 controller) should be handled the same way for integer and percentage minAvailable
  • generate an event (tied to the PDB) for the 0 controllers or > 1 controllers error conditions

@mml
Copy link
Copy Markdown
Contributor Author

mml commented Aug 16, 2016

@k8s-bot integration test this #30462

@mml
Copy link
Copy Markdown
Contributor Author

mml commented Aug 16, 2016

@k8s-bot unit test this issue: #30462

@davidopp davidopp added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 17, 2016
@k8s-bot
Copy link
Copy Markdown

k8s-bot commented Aug 17, 2016

GCE e2e build/test passed for commit d60ba3c.

@k8s-github-robot
Copy link
Copy Markdown

k8s-github-robot commented Aug 17, 2016

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit d574082 into kubernetes:master Aug 17, 2016
@davidopp davidopp mentioned this pull request Aug 30, 2016
Closed
12 tasks
@seh seh mentioned this pull request Sep 26, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@seh seh seh left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@davidopp davidopp

Labels

lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.