✨ Use caching read for bootstrap config owner #8867

nojnhuh · 2023-06-15T20:07:39Z

What this PR does / why we need it: This PR makes the lookup of the KubeadmConfig's owner use a cached client instead of the uncached unstructured client. To make the rest of the adjacent plumbing reusable, the new typed read by GetOwnerByRefCached converts the object into Unstructured to construct a ConfigOwner.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #8849

/cc @fabriziopandini

nojnhuh · 2023-06-15T20:09:00Z

bootstrap/util/configowner_test.go

-				Namespace: metav1.NamespaceDefault,
-				Labels: map[string]string{
-					clusterv1.MachineControlPlaneLabel: "",
+	doTests := func(t *testing.T, getFn func(context.Context, client.Client, metav1.Object) (*ConfigOwner, error)) {


Most of the test changes here are indentation-related, so I recommend ticking the "hide whitespace" checkbox.

nojnhuh · 2023-06-15T20:10:53Z

@fabriziopandini Please let me know if there's any specific testing I can do to show what kind of performance impact this has.

fabriziopandini

Thanks for implementing this PR, just a couple of nits from my side.
Also, what about adding a note about the introduction of the new function in the 1.4 -> 1.5 migration notes?

fabriziopandini · 2023-06-20T09:11:45Z

bootstrap/util/configowner.go

@@ -125,6 +127,16 @@ func (co ConfigOwner) KubernetesVersion() string {

 // GetConfigOwner returns the Unstructured object owning the current resource.


I would add a warning that this can impact performance at scale because it reads unstructured objects which are not cached

Should we deprecate this func?

I would say not without knowing why me made this public, I assume because someone asked for it from providers

Looks like there is one usage in the CAPA controller: https://cs.k8s.io/?q=GetConfigOwner&i=nope&files=&excludeFiles=&repos=

I don't know if they intentionally use the version which triggers an uncached call.

But definitely fine for me to not deprecate in this PR to get the improvement in ASAP.

fabriziopandini · 2023-06-20T09:16:22Z

bootstrap/util/configowner.go

+// GetOwnerByRefCached finds and returns the owner by looking at the object
+// reference. It may return a cached object.


what about

Suggested change

// GetOwnerByRefCached finds and returns the owner by looking at the object

// reference. It may return a cached object.

// GetOwnerByRefFromCache finds and returns the owner by looking at the object

// reference. The implementation ensures a typed client is used, so the objects are read from the cache.

fabriziopandini · 2023-06-20T09:17:51Z

bootstrap/util/configowner.go

+// GetConfigOwnerCached returns the Unstructured object owning the current
+// resource, which may come from a cache.
+func GetConfigOwnerCached(ctx context.Context, c client.Client, obj metav1.Object) (*ConfigOwner, error) {


What about

Suggested change

// GetConfigOwnerCached returns the Unstructured object owning the current

// resource, which may come from a cache.

func GetConfigOwnerCached(ctx context.Context, c client.Client, obj metav1.Object) (*ConfigOwner, error) {

// GetConfigOwnerCached returns the Unstructured object owning the current

// resource. The implementation ensures a typed client is used, so the objects are read from the cache.

func GetConfigOwnerFromCache(ctx context.Context, c client.Client, obj metav1.Object) (*ConfigOwner, error) {

sbueringer · 2023-06-20T14:19:42Z

lgtm pending the godoc nits

sbueringer · 2023-06-20T14:22:38Z

@fabriziopandini Please let me know if there's any specific testing I can do to show what kind of performance impact this has.

I think our scale testing is not in a state yet to just say please rerun those tests and you can see the improvement based on this data. But based on some other changes we made we know that using the cache is a lot more performant than calls against the apiserver. I'll include this PR/commit in the scale testing I'm currently doing locally.

nojnhuh · 2023-06-20T15:27:43Z

Sorry for the extra force-push, almost forgot to update the function name in the migration doc. All feedback should be addressed now.

sbueringer · 2023-06-21T04:47:00Z

Thank you very much!

/lgtm
/approve

k8s-ci-robot · 2023-06-21T04:47:06Z

LGTM label has been added.

Git tree hash: 108045f494f20d713ee38a44c73e84675199917c

k8s-ci-robot · 2023-06-21T04:47:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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~~ [sbueringer]

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

killianmuldoon

/area provider/bootstrap-kubeadm

k8s-ci-robot requested a review from fabriziopandini June 15, 2023 20:07

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 15, 2023

nojnhuh commented Jun 15, 2023

View reviewed changes

nojnhuh force-pushed the cached-get-config-owner branch from 5a71226 to 06e8903 Compare June 15, 2023 20:21

fabriziopandini reviewed Jun 20, 2023

View reviewed changes

nojnhuh force-pushed the cached-get-config-owner branch from 06e8903 to f6777ab Compare June 20, 2023 15:25

✨ Use caching read for bootstrap config owner

426d2ad

nojnhuh force-pushed the cached-get-config-owner branch from f6777ab to 426d2ad Compare June 20, 2023 15:26

k8s-ci-robot assigned sbueringer Jun 21, 2023

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

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

k8s-ci-robot merged commit 869676f into kubernetes-sigs:main Jun 21, 2023
20 checks passed

k8s-ci-robot added this to the v1.5 milestone Jun 21, 2023

sbueringer mentioned this pull request Jun 23, 2023

Scale testing #8814

Open

27 tasks

killianmuldoon reviewed Jun 27, 2023

View reviewed changes

k8s-ci-robot added the area/provider/bootstrap-kubeadm Issues or PRs related to CAPBK label Jun 27, 2023

nojnhuh deleted the cached-get-config-owner branch February 18, 2024 01:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Use caching read for bootstrap config owner #8867

✨ Use caching read for bootstrap config owner #8867

nojnhuh commented Jun 15, 2023

nojnhuh Jun 15, 2023

nojnhuh commented Jun 15, 2023

fabriziopandini left a comment

fabriziopandini Jun 20, 2023

sbueringer Jun 20, 2023

fabriziopandini Jun 20, 2023

sbueringer Jun 20, 2023

fabriziopandini Jun 20, 2023

fabriziopandini Jun 20, 2023

sbueringer commented Jun 20, 2023

sbueringer commented Jun 20, 2023 •

edited

nojnhuh commented Jun 20, 2023

sbueringer commented Jun 21, 2023

k8s-ci-robot commented Jun 21, 2023

k8s-ci-robot commented Jun 21, 2023

killianmuldoon left a comment

		@@ -125,6 +127,16 @@ func (co ConfigOwner) KubernetesVersion() string {

		// GetConfigOwner returns the Unstructured object owning the current resource.

		// GetOwnerByRefCached finds and returns the owner by looking at the object
		// reference. It may return a cached object.

✨ Use caching read for bootstrap config owner #8867

✨ Use caching read for bootstrap config owner #8867

Conversation

nojnhuh commented Jun 15, 2023

nojnhuh Jun 15, 2023

Choose a reason for hiding this comment

nojnhuh commented Jun 15, 2023

fabriziopandini left a comment

Choose a reason for hiding this comment

fabriziopandini Jun 20, 2023

Choose a reason for hiding this comment

sbueringer Jun 20, 2023

Choose a reason for hiding this comment

fabriziopandini Jun 20, 2023

Choose a reason for hiding this comment

sbueringer Jun 20, 2023

Choose a reason for hiding this comment

fabriziopandini Jun 20, 2023

Choose a reason for hiding this comment

fabriziopandini Jun 20, 2023

Choose a reason for hiding this comment

sbueringer commented Jun 20, 2023

sbueringer commented Jun 20, 2023 • edited

nojnhuh commented Jun 20, 2023

sbueringer commented Jun 21, 2023

k8s-ci-robot commented Jun 21, 2023

k8s-ci-robot commented Jun 21, 2023

killianmuldoon left a comment

Choose a reason for hiding this comment

sbueringer commented Jun 20, 2023 •

edited