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.

Sign up for GitHub

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

Jump to bottom

🌱 Use Lazy Restmapper #8091

Merged
merged 2 commits into from
Mar 10, 2023
Merged

🌱 Use Lazy Restmapper #8091

merged 2 commits into from
Mar 10, 2023

Conversation

Fedosin
Copy link
Contributor

@Fedosin Fedosin commented Feb 9, 2023 •
edited
Loading

What this PR does / why we need it:

The feature introduced in controller-runtime 0.14.4+ allows to use a lazy restmapper. The main advantage of this restmapper is that it fetches only required api resources in runtime, which significantly reduces the number of http requests to the api server.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 9, 2023
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 9, 2023
@Fedosin
Copy link
Contributor Author

Fedosin commented Feb 9, 2023

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 9, 2023
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 9, 2023
@Fedosin
Copy link
Contributor Author

Fedosin commented Feb 9, 2023

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 9, 2023
@Fedosin
Copy link
Contributor Author

Fedosin commented Feb 9, 2023

/test pull-cluster-api-test-mink8s-main

@Fedosin Fedosin changed the title ✨ Use Lazy Restmapper 🌱 Use Lazy Restmapper Feb 10, 2023
@fabriziopandini
Copy link
Member

cc @sbueringer who is tracking the latest changes in CR and how they impact CAPI; from a quick pass, two comments from my side:

  • I will move the CR bump to another PR, so we can get this merged fast no matter of using the Lazy mapper or not
  • If we are introducing a feature that is considered experimental in CR, we probably need a feature gate in CAPI off by default.

@sbueringer
Copy link
Member

I'll take a closer look at the CR PR (probably tomorrow).

At a first glance I agree with Fabrizio. Would be good to feature-gate it for now (even if it will probably be the default in CR v0.15)

@sbueringer
Copy link
Member

/test pull-cluster-api-e2e-full-main

@sbueringer
Copy link
Member

sbueringer commented Feb 14, 2023 •
edited
Loading

Just fyi. Brought up a question on the CR PR: kubernetes-sigs/controller-runtime#2116 (comment)

If I'm correct the Cluster API controller would have to be restarted e.g. when a new infra provider which introduces CRDs in a new APIGroup is deployed. But (assuming I'm right) I think this is just a small gap that can be addressed in CR.

@sbueringer
Copy link
Member

/retest
flake

@sbueringer
Copy link
Member

sbueringer commented Mar 2, 2023 •
edited
Loading

@Fedosin Your fix in CR is merged, cherry-picked and v0.14.5 has been released. I opened a PR in CAPI to bump to v0.14.5 (#8213).

Would be great if you have time to update this PR once the CR bump is merged.

It would be nice to get a version of this PR where the lazy restmapper is used behind a feature gate into CAPI v1.4.

We might get to investing a bit in scale testing after v1.4 and this would allow us to easily try out the lazy restmapper in those tests.

(just fyi, we released the first v1.4 beta on Tuesday, we roughly have another 1,5 weeks to get this PR in until the first rc, then it's only bugfixes etc (xref: https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/release/releases/release-1.4.md))

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 3, 2023
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 6, 2023
@Fedosin
Copy link
Contributor Author

Fedosin commented Mar 6, 2023

@sbueringer thank you for your review and comments! I updated the PR and now it uses the released 0.14.5 version of controller-runtime.

@sbueringer
Copy link
Member

@Fedosin Change itself looks fine. Let's add a feature gate for this (disabled per default).

Should be:

sbueringer
sbueringer reviewed Mar 9, 2023
View reviewed changes
controlplane/kubeadm/main.go Outdated Show resolved Hide resolved
test/infrastructure/docker/main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Show resolved Hide resolved
@Fedosin
Register new LazyRestmapper featuregate
d249ac4 
This featuregate will be used for enabling lazy restmapper functionality
from controller-runtime.
@Fedosin
Use lazy restmapper
f741716 
The feature was introduced in controller-runtime 0.14.4+ allows to
use lazy restmapper. The main advantage of this restmapper is that it
 fetches only required api resources in runtime, which highly reduces
the number of http calls to the api server.
@Fedosin
Copy link
Contributor Author

Fedosin commented Mar 9, 2023

/test pull-cluster-api-test-main

@sbueringer
Copy link
Member

Thank you very much!

/test pull-cluster-api-e2e-full-main

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 10, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: e88b70568d267715c146b3bb70b81268e7eb6f89

@sbueringer
Copy link
Member

/retest

@fabriziopandini
Copy link
Member

Nice one!
/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 10, 2023
@k8s-ci-robot k8s-ci-robot merged commit f8ec928 into kubernetes-sigs:main Mar 10, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.4 milestone Mar 10, 2023
@fabriziopandini fabriziopandini mentioned this pull request Mar 23, 2023
Closed
@sbueringer sbueringer mentioned this pull request Apr 22, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbueringer sbueringer sbueringer left review comments

@CecileRobertMichon CecileRobertMichon Awaiting requested review from CecileRobertMichon

@ykakarap ykakarap Awaiting requested review from ykakarap

Assignees

@sbueringer sbueringer

@fabriziopandini fabriziopandini

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.4
Development

Successfully merging this pull request may close these issues.
4 participants
@Fedosin @fabriziopandini @sbueringer @k8s-ci-robot