🌱 Cleanup separate unstructuredCachingClient

[sbueringer]

Signed-off-by: Stefan Büringer buringerst@vmware.com

What this PR does / why we need it:
Noticed that we still have the separate unstructuredCachingClient when reviewing Christians CRS PR.

This PR removes the separate client and instead configured the manager default client to cache all unstructured calls.

I've opened a separate PR (#10691) to check if we were still using uncached unstructured get/list calls anywhere (to see if thoes additional usages are safe).

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 #

[sbueringer]

/test pull-cluster-api-e2e-main

[sbueringer]

/hold

to check logs of #10691 e2e-min for uncached unstructured get/list occurences first

[sbueringer]

/hold cancel

Test results from #10691

To summarize. The only places where we used uncached get/list calls for Unstructured on main are:

• MachinePool controller
• Machine controller isDeleteNodeAllowed
• Cluster webhook validateTopologyControlPlaneVersion

I think in all 3 cases it's okay to also use cached calls. So I think it's fine to just always cache unstructured.

[sbueringer]

(I'll take a look at the unit test failures)

[sbueringer]

I think I had to change this because now the tests are also using cached unstructured get/list calls (production code in KCP always did)

[sbueringer]

/test pull-cluster-api-e2e-main

[k8s-ci-robot]

@sbueringer: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-apidiff-main	4714c48	link	false	/test pull-cluster-api-apidiff-main

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-sigs/prow repository. I understand the commands that are listed here.

[enxebre]

/lgtm

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: ff72e4c0092ffbea477e7b93a5a8d3427ce933b7

[enxebre]

night need to handle deprecation https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api/10692/pull-cluster-api-apidiff-main/1795517221561176064

[sbueringer]

I can add the fields here again (with //Deprecated and without using them) if you want.

This would allow folks one more minor release to not set these fields. Not sure if it makes much difference to be honest

[fabriziopandini]

/lgtm

Nice cleanup!
I have only one question: is there a way to check if before this change the regular Client was used for reading unstructured somewhere, and check if this code might be relying to the fact that the read was not cached?

[vincepri]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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 [vincepri]

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

[sbueringer]

/lgtm

Nice cleanup! I have only one question: is there a way to check if before this change the regular Client was used for reading unstructured somewhere, and check if this code might be relying to the fact that the read was not cached?

I basically did the following:

• Create a version of CR that logs to stdout whenever the regular Client does an uncached get/list with Unstructured: https://github.com/kubernetes-sigs/controller-runtime/compare/release-0.18...sbueringer:controller-runtime:pr-panic-unstructured?expand=1
  • I was trying to use panic, but this doesn't work well, because clusterctl was panic'ing all over the place then :)
• Open a PR to use this CR version: [DONT_REVIEW] 🌱 POC: find all uncached unstructured calls #10691
• Go over e2e-main logs and grep for these log outputs

This way I identified the following cases: (see diff on #10691 for the first two)

• MachinePool controller
• Machine controller isDeleteNodeAllowed
• Cluster webhook validateTopologyControlPlaneVersion (

  cluster-api/internal/webhooks/cluster.go

  Line 492 in 7c2d2f5

  cp, err := external.Get(ctx, ctrlClient, oldCluster.Spec.ControlPlaneRef, oldCluster.Namespace)

  )

After that I looked manually at the cases and they all seem safe to me.

I think aparat from that we would have the following options.

To identify those cases:

• I imagine it's possible to write a tool to identify in which cases mgr.GetClient() is used to do unstructured get/list call
  • But it's definitely not trivial as we just pass client.Client's around in our code base
  • Manually assessing every single Client.Get/List call in our entire code base would also work. But this would take a lot of time because we always have to go up the entire stack trace to find out which client is used.

To assess if these cases are problematic:

• I don't think there is an automated way to deduct if the logic behind our get/list calls still works if the object is not coming live from the apiserver or is potentially <1-2ms outdated (which is usually the diff). (even the object we get from the apiserver could already be outdated immediately after we retrieved it)
  • So I think the problematic cases are something like: create a Machine, do a list call very soon after, create another Machine because the first one was not observed
• I think the only good option there is to manually assess.