Migrate cluster role aggregator to apply #99462

jpbetz · 2021-02-25T16:41:30Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Migrates one controller over to use the client-go support for server apply. This is something we had agreed to do as part of the KEP for Apply for client-go's typed client

umbrella issue: kubernetes/enhancements#2155

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Release note will be added when all the last change in the umbrella issue is merged

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[KEP]: KEP 2155

TODO

/sig api-machinery
/wg api-expression

fedebongio · 2021-02-25T21:10:57Z

/triage accepted

fejta-bot · 2021-02-25T21:56:23Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

apelisse

Looks good to me.

pkg/controller/clusterroleaggregation/clusterroleaggregation_controller.go

k8s-ci-robot · 2021-03-05T14:46:13Z

@jpbetz: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-aks-engine-azure	10e4339209548c9e2a9a0345c65047ec0cd14cb7	link	`/test pull-kubernetes-e2e-aks-engine-azure`
pull-kubernetes-e2e-azure-disk-windows	10e4339209548c9e2a9a0345c65047ec0cd14cb7	link	`/test pull-kubernetes-e2e-azure-disk-windows`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

jpbetz · 2021-03-05T17:40:35Z

/test pull-kubernetes-unit

jpbetz · 2021-03-05T19:04:03Z

/retest

pkg/controller/clusterroleaggregation/clusterroleaggregation_controller.go

deads2k · 2021-03-08T17:27:28Z

pkg/controller/clusterroleaggregation/clusterroleaggregation_controller_test.go

This looks like a problem worth fixing.

Yeah, this is an interesting problem I hit. I chatted with a few people out-of-band about this.

Because server-side apply is implemented in the apiserver (see fieldmanager), this looks really difficult to full address (intractable?). It is maybe slightly more tractable now that we have the SMD schemas loaded into the client, but still a large problem. I've opened an issue to track: #99953 with some notes about what we might be able to do (and what makes full support so difficult). Okay to look at this as a follow up to GA? I don't think we're going to be able to do much in the short term :(

cc @apelisse @liggitt

It's difficult to recommend people use a shiny new client for their controllers if they are unable to easily unit test the code that they author. If we cannot recommend that people use the new GA feature, is it truly GA? Is there a pattern they can use in the interim?

I've spent some time reviewing use cases and trying to come up with a recommendation. See client-go Apply Fake Client: Review of use cases for a lot more detail. There seem to be 4 main uses cases to consider:

"Simulate responses to requests (via Reactors)" - works fine

"Validate the requests made using the client" - I think this is where we should focus short term efforts

"Simulate external changes to objects" - works fine

"Using objectTracker as a pretend apiserver" - looks like an anti-pattern, found very few tests doing this

I think we can get the "Validate the requests made using the client" fixed with a relatively small change in the near term.

I'd like to have the apply fake client be at least as good as the update fake client is today. It certainly needs to detect when cases aren't working right as a minimum bar.

I thought we were reserving at least a release in case we find a significant usability problem for which we want to break compatibility. I think it's OK if the fake client isn't feature-complete while in this state, but the comments for the generated fakes should make it clear what works and what doesn't.

pkg/controller/clusterroleaggregation/clusterroleaggregation_controller.go

deads2k · 2021-03-09T17:03:47Z

/lgtm
/approve

k8s-ci-robot · 2021-03-09T17:04:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, jpbetz

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:

~~pkg/controller/OWNERS~~ [deads2k]

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

k8s-ci-robot requested review from caesarxuchao and davidopp February 25, 2021 16:43

jpbetz force-pushed the apply-cluster-role-aggregator branch from ab14069 to c1e72d2 Compare February 25, 2021 17:51

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Feb 25, 2021

jpbetz force-pushed the apply-cluster-role-aggregator branch from c1e72d2 to 7c64054 Compare February 25, 2021 21:26

jpbetz changed the title ~~WIP - Migrate cluster role aggregator to apply~~ Migrate cluster role aggregator to apply Feb 25, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 25, 2021

jpbetz force-pushed the apply-cluster-role-aggregator branch from 7c64054 to bb3737d Compare February 25, 2021 21:40

jpbetz force-pushed the apply-cluster-role-aggregator branch from bb3737d to 4d08f96 Compare February 25, 2021 22:26

apelisse reviewed Feb 26, 2021

View reviewed changes