✨ Provide a truly lazy restmapper #2116

Fedosin · 2023-01-02T12:59:11Z

This PR adds a rest mapper that will lazily query the provided client for discovery information to do REST mappings. This allows you to significantly reduce the number of requests to the API server, as currently we do a request for each registered GroupVersion at startup. It also makes the startup time of the controller more predictable and independent of the number of CRDs installed in the system.

Fixes: #1603

pkg/client/apiutil/dynamicrestmapper.go

Fedosin · 2023-01-02T16:46:40Z

/hold

JoelSpeed

I think this is a worthwile improvement to the controller-runtime project, with large numbers of CRDs, we've seen rest mapping take a long time and cache large amounts of data that isn't really necessary.

I think there are a few improvements that could be made to make this a little cleaner, and one logical optimisation, see my comments and let me know what you think

pkg/client/apiutil/lazyrestmapper.go

lobziik

I mainly agree with @JoelSpeed comments. Have just an extra nit about docstring for filteredServerGroupsAndResources.

pkg/client/apiutil/lazyrestmapper.go

pkg/client/apiutil/lazyrestmapper/lazyrestmapper.go

pkg/client/apiutil/lazyrestmapper/lazyrestmapper_test.go

Fedosin · 2023-01-09T22:36:19Z

/hold cancel

vincepri · 2023-01-09T23:48:00Z

pkg/client/apiutil/lazyrestmapper/lazyrestmapper.go

+// LazyRESTMapper is a RESTMapper that will lazily query the provided
+// client for discovery information to do REST mappings.
+type LazyRESTMapper struct {


Curious if we though actually about making this RESTMapper the default implementation, truthfully it sounds like that the benefit of making less calls, and only making them when needed are something that a lot of users might want.

If we decide to go down this path, we should however provide a feature gate of sorts that enables this as an experiment first (opt in) -> on by default -> then enable it and remove the gate.

Thoughts?

Hey! Initially I didn't plan to make it default but rather to use it either for clusters with a slow API server, or for very crd-pumped ones (ahem, OpenShift), when we have to do more than a hundred requests to initialize the mapper.

But after multiple optimizations I could say there is no worsening relative to the current implementation, only improvements. So, it's reasonable to make it default after some testing.

Do you have an example of such feature gating in this project, so I can mark this feature as experimental?

btw, we test this implementation with OpenShift here: openshift/cluster-cloud-controller-manager-operator#213

We'd need to introduce a feature gate, which we currently don't have support for. For this PR, we can focus immediately to do some sort of split-logic between how things were done in the default RESTMapper, and this way.

If we feel comfortable though and have enough testing coverage all around, we can mark this a breaking change (⚠️) and add extensive release notes and ask for feedback.

@alvaroaleman wdyt?

I'd suggest to simply add this restmapper for now and backport it into 0.14. 0.15 onwards (or later, depending on if we find issues) we can simply make it the default but leave the option to provide a custom one.

Could we have the implementation of this PR inlined with the current one, but all of it behind a feature gate?

How would you featuregate this?

A new DynamicRESTMapperOption that sets the flag could work?

Something like: WithExperimentalLazyMapper

alvaroaleman · 2023-01-10T16:50:10Z

Sorry, this is on my queue but I am a bit swamped right now

alvaroaleman

@Fedosin one comment to optimize this even further, other than that this looks good to me. Thanks a ton for your work and sorry it took so long.

pkg/client/apiutil/lazyrestmapper/lazyrestmapper.go

alvaroaleman · 2023-01-29T18:37:59Z

pkg/client/apiutil/lazyrestmapper/lazyrestmapper.go

+// LazyRESTMapper is a RESTMapper that will lazily query the provided
+// client for discovery information to do REST mappings.
+type LazyRESTMapper struct {


Merging the two for next release sounds good to me.

I think the new lazy restmapper does everything the current dynamic one does btw. The current dynamic one will on a resource not found error reload the entire restmapping (with some ratelimiting). Since the new one will simply try to load the restmapping on-demand if it doesn't have it cached, that should solve the same problem just as well.

alvaroaleman · 2023-01-29T18:46:33Z

/cherrypick release-0.14

k8s-infra-cherrypick-robot · 2023-01-29T18:46:34Z

@alvaroaleman: once the present PR merges, I will cherry-pick it on top of release-0.14 in a new PR and assign it to you.

In response to this:

/cherrypick release-0.14

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.

This commit adds a rest mapper that will lazily query the provided client for discovery information to do REST mappings.

Instead of creating the instance of the mapper directly, we will use WithExperimentalLazyMapper option for Dynamic Restmapper.

Fedosin · 2023-02-06T00:18:53Z

@alvaroaleman @vincepri hey folks! I updated this PR as you asked - now there is a "featuregate" called WithExperimentalLazyMapper that enables the Lazy. PTAL :)

alvaroaleman

Thanks you so much!

k8s-ci-robot · 2023-02-06T01:08:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, Fedosin

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:

~~OWNERS~~ [alvaroaleman]

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

k8s-infra-cherrypick-robot · 2023-02-06T01:09:12Z

@alvaroaleman: new pull request created: #2179

In response to this:

/cherrypick release-0.14

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.

sbueringer · 2023-02-13T17:32:12Z

pkg/client/apiutil/lazyrestmapper.go

+func (m *lazyRESTMapper) findAPIGroupByName(groupName string) (metav1.APIGroup, error) {
+	// Ensure that required info about existing API groups is received and stored in the mapper.
+	// It will make 2 API calls to /api and /apis, but only once.
+	if m.apiGroups == nil {


Sorry very late :)

Q: We only ever seem to write m.apiGroups here. What if the apiGroups change after that? Or do we assume that's not relevant as the CRDs a controller uses must be deployed before the controller?

cc @Fedosin

+1 on the question above, as far as I can see the apiGroups is never refreshed which might cause issues for groups that were installed after the mapper gets created AND if someone calls RESTMapping without a version

Good catch!
Q: does it make sense to track this in an issue and makes sure this has an answer before pushing further the adoption of this mapper?

Not sure, the code should work for 97% of uses within controller runtime; but the above is definitely an issue we should fix

For reference this has been addressed in #2208 and released in v0.14.5

provider-kubernetes initialized a new kubernetes client for each reconcile. The REST mapper in controller-runtime used to fetch information about every CRD in the cluster. controller-runtime introduced a lazy restmapper which means we don't have to introduce any complex caching to get a significant performance boost in provider-kubernetes: kubernetes-sigs/controller-runtime#2116 This seems to become the default in the next release: kubernetes-sigs/controller-runtime#2296 But this is so significant that we want to update now: * CPU reduced from constant throttling at 0.4 cores to 0.04 cores * CloudWatch / EKS audit log costs reduced significantly (55% for our cluster, with a lot of provider-kubernetes resources) Signed-off-by: Carl Henrik Lunde <chlunde@ifi.uio.no>

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 2, 2023

k8s-ci-robot requested review from alvaroaleman and vincepri January 2, 2023 12:59

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 2, 2023

alvaroaleman reviewed Jan 2, 2023

View reviewed changes

pkg/client/apiutil/dynamicrestmapper.go Outdated Show resolved Hide resolved

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 2, 2023

Fedosin force-pushed the lazy_loading branch from 611684e to ac5cb85 Compare January 3, 2023 12:15

JoelSpeed reviewed Jan 3, 2023

View reviewed changes

lobziik reviewed Jan 3, 2023

View reviewed changes

pkg/client/apiutil/lazyrestmapper.go Outdated Show resolved Hide resolved

pkg/client/apiutil/lazyrestmapper.go Outdated Show resolved Hide resolved

alvaroaleman reviewed Jan 3, 2023

View reviewed changes

pkg/client/apiutil/lazyrestmapper.go Show resolved Hide resolved

pkg/client/apiutil/lazyrestmapper.go Outdated Show resolved Hide resolved

Fedosin force-pushed the lazy_loading branch 2 times, most recently from d06b559 to 2967f3b Compare January 4, 2023 15:42

JoelSpeed reviewed Jan 4, 2023

View reviewed changes

pkg/client/apiutil/lazyrestmapper/lazyrestmapper.go Outdated Show resolved Hide resolved

pkg/client/apiutil/lazyrestmapper/lazyrestmapper.go Outdated Show resolved Hide resolved

Fedosin force-pushed the lazy_loading branch 3 times, most recently from 7a302ea to cc59ac4 Compare January 5, 2023 15:43

alvaroaleman reviewed Jan 5, 2023

View reviewed changes

pkg/client/apiutil/lazyrestmapper/lazyrestmapper_test.go Outdated Show resolved Hide resolved

Fedosin force-pushed the lazy_loading branch from cc59ac4 to 3f3c8e0 Compare January 9, 2023 22:35

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 9, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 9, 2023

Fedosin force-pushed the lazy_loading branch from 3f3c8e0 to 5f60696 Compare January 9, 2023 22:45

vincepri reviewed Jan 9, 2023

View reviewed changes

Fedosin force-pushed the lazy_loading branch from 5f60696 to f77f9b3 Compare January 10, 2023 16:25

Fedosin force-pushed the lazy_loading branch from f77f9b3 to cf746c2 Compare January 16, 2023 11:33

alvaroaleman reviewed Jan 29, 2023

View reviewed changes

alvaroaleman mentioned this pull request Jan 29, 2023

Lot's of discovery cache throttling with many CRDs #2084

Closed

Fedosin force-pushed the lazy_loading branch from cf746c2 to 23b3137 Compare February 3, 2023 00:19

Provide a truly lazy restmapper

636b1eb

This commit adds a rest mapper that will lazily query the provided client for discovery information to do REST mappings.

Fedosin force-pushed the lazy_loading branch from 23b3137 to 636b1eb Compare February 5, 2023 23:29

Use DynamicRESTMapperOption to enable Lazy Restmapper

d2fc349

Instead of creating the instance of the mapper directly, we will use WithExperimentalLazyMapper option for Dynamic Restmapper.

alvaroaleman approved these changes Feb 6, 2023

View reviewed changes

k8s-ci-robot assigned alvaroaleman Feb 6, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 6, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 6, 2023

k8s-ci-robot merged commit f127c11 into kubernetes-sigs:master Feb 6, 2023

k8s-infra-cherrypick-robot mentioned this pull request Feb 6, 2023

✨ Provide a truly lazy restmapper #2179

Merged

Fedosin mentioned this pull request Feb 9, 2023

🌱 Use Lazy Restmapper kubernetes-sigs/cluster-api#8091

Merged

sbueringer reviewed Feb 13, 2023

View reviewed changes

sbueringer mentioned this pull request Apr 22, 2023

🐛 make clusterctl REST client throttling configurable kubernetes-sigs/cluster-api#8411

Closed

chlunde mentioned this pull request May 8, 2023

Use lazy rest mapper for performance and less audit crossplane-contrib/provider-kubernetes#116

Closed

2 tasks

tiraboschi mentioned this pull request May 22, 2023

Bump k8s to v0.27.2 kubevirt/hyperconverged-cluster-operator#2333

Merged

11 tasks

blakebarnett mentioned this pull request May 25, 2023

Update to controller-runtime v0.16+ kudobuilder/kuttl#473

Closed

ellistarn mentioned this pull request Jun 7, 2023

chore: Upgrade to controller-runtime 1.15 aws/amazon-vpc-resource-controller-k8s#260

Closed

Fedosin deleted the lazy_loading branch July 15, 2023 18:46

jaydeokar mentioned this pull request Aug 1, 2023

Update Container-Runtime to 0.15 aws/amazon-vpc-resource-controller-k8s#286

Merged

OliverMKing mentioned this pull request Aug 31, 2023

update all dependencies Azure/aks-app-routing-operator#92

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Provide a truly lazy restmapper #2116

✨ Provide a truly lazy restmapper #2116

Fedosin commented Jan 2, 2023 •

edited

Fedosin commented Jan 2, 2023

JoelSpeed left a comment

lobziik left a comment •

edited

Fedosin commented Jan 9, 2023

vincepri Jan 9, 2023

Fedosin Jan 10, 2023

Fedosin Jan 10, 2023

vincepri Jan 10, 2023

alvaroaleman Jan 10, 2023

vincepri Jan 30, 2023

alvaroaleman Jan 30, 2023

vincepri Jan 30, 2023

vincepri Jan 30, 2023

alvaroaleman Feb 1, 2023

alvaroaleman commented Jan 10, 2023

alvaroaleman left a comment •

edited

alvaroaleman Jan 29, 2023

alvaroaleman commented Jan 29, 2023

k8s-infra-cherrypick-robot commented Jan 29, 2023

Fedosin commented Feb 6, 2023

alvaroaleman left a comment

k8s-ci-robot commented Feb 6, 2023

k8s-infra-cherrypick-robot commented Feb 6, 2023

sbueringer Feb 13, 2023

sbueringer Feb 13, 2023

vincepri Feb 14, 2023

fabriziopandini Feb 14, 2023

vincepri Feb 14, 2023

sbueringer Apr 12, 2023

✨ Provide a truly lazy restmapper #2116

✨ Provide a truly lazy restmapper #2116

Conversation

Fedosin commented Jan 2, 2023 • edited

Fedosin commented Jan 2, 2023

JoelSpeed left a comment

Choose a reason for hiding this comment

lobziik left a comment • edited

Choose a reason for hiding this comment

Fedosin commented Jan 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvaroaleman commented Jan 10, 2023

alvaroaleman left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvaroaleman commented Jan 29, 2023

k8s-infra-cherrypick-robot commented Jan 29, 2023

Fedosin commented Feb 6, 2023

alvaroaleman left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 6, 2023

k8s-infra-cherrypick-robot commented Feb 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fedosin commented Jan 2, 2023 •

edited

lobziik left a comment •

edited

alvaroaleman left a comment •

edited