Support scale subresource for PDBs#76294
Conversation
9092fb3 to
f98f893
Compare
|
/test pull-kubernetes-integration |
|
/remove-sig api-machinery |
|
thanks for the PR... is there a proposal for this API change? |
|
Yes, a design would be good. It can be short if the semantics are simple and there are few issues. |
f98f893 to
cc916c8
Compare
ab0ae9a to
717e4a5
Compare
4f60537 to
3e82085
Compare
|
@mikedanese @liggitt Can you take a look at this one? I would like to get this into 1.15 and I don't want to get too close to code freeze. |
| } | ||
| gr := mapping.Resource.GroupResource() | ||
|
|
||
| scale, err := dc.scaleNamespacer.Scales(namespace).Get(gr, controllerRef.Name) |
There was a problem hiding this comment.
this API is unfortunate... we're throwing away the version information we just got from our restmapper. if we know the full group/version/resource we want, we should be able to invoke it directly, and the scale client would be way simpler (it wouldn't need a restmapper internally). can you open a follow up issue to improve this?
liggitt
left a comment
There was a problem hiding this comment.
RBAC change looks fine, there's some additional checking that needs to be done on the controllerRef group
| return nil, err | ||
| } | ||
| rs, err := dc.rsLister.ReplicaSets(namespace).Get(controllerRef.Name) | ||
| if err != nil { |
There was a problem hiding this comment.
pre-existing, but the same group check ("apps" or "extensions") should be done when resolving the deployment controllerref below. up to you whether you do that here or in a follow-up
There was a problem hiding this comment.
Updated the PR to include the group test on the deployment controller ref.
| return false, nil | ||
| } | ||
|
|
||
| if controllerRef.Kind == expectedKind { |
There was a problem hiding this comment.
nit: move this first and short-circuit the parse and group search and return false, nil early if this doesn't match
There was a problem hiding this comment.
Yeah, this looks cleaner. I have updated the function.
| groupMatch := false | ||
| for _, group := range expectedGroups { | ||
| if group == gv.Group { | ||
| groupMatch = true |
There was a problem hiding this comment.
if the kind match has already been done, can simply return true, nil here
|
a couple nits in verifyGroupKind, and one other place a group check is needed I'm not that familiar with the test coverage of PDBs... do we have positive and negative tests for each of the controllerref types modified here? |
|
@liggitt I have added additional tests to cover the finder functions. |
|
thanks, looks great /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: janetkuo, kow3ns, liggitt, mortent 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 |
* Support scale subresource for PDBs * Check group in finder functions * Small fixes and more tests
What type of PR is this?
/kind feature
What this PR does / why we need it:
PDBs currently only work with the workload controllers deployment, replicaset, statefulset and replicationcontroller. This change adds support for any controllers that implement the scale subresource.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: