🌱 Make ClusterResourceSet controller more predictable

[fabriziopandini]

What this PR does / why we need it:
ClusterResource set controller doesn't have an ideal implementation for cases where many ClusterResourceSets are targeting the same cluster (might be also the API is not specifically designed for this use case).

This PR is making this case a little bit more predictable, by avoiding a potential impact of exponential backoff delay quickly growing, but it is not solving the underlying issue.

At least, this should help in getting rid of some flakiness in our tests, and there is also a comment providing some more context on what's going on.

Another positive effect of this PR is that it remove a lot of noise from the logs

/area clusterresourceset

Related to #10854

[fabriziopandini]

Together with @sbueringer we did investingate this a little bit more. There are a few things at play:

1. How the test was written

• The test "Should handle applying multiple ClusterResourceSets concurrently to the same cluster" was re-using the same config map and secrets for 10 CRS (which does not makes sense in a real configuration)
• When one of the CRS object reconciles, it adds owner refs to configMap and secrets, and watches on those objects triggers reconciles for all 10 CRS. This generates storm of events making the test unstable.
• I have added a commit to create 1 CM/1 Secret for each CRS

2. code dependencies with Cluster Cache Tracker.

• The reconcile func is getting from Cluster Cache Tracker a remote client to apply CRS
• At the beginning of the tests, when all the CRS objects reconciles toghether on a newly created cluster, it might happen that all the CRS objects except the first will get a lock error when trying to acquire a client from Cluster Cache Tracker
• In case of lock errors, there is a requeue after 1 minute and test will fails (flake)
• The new commit makes this (and other tests) more stable by creating a remote client in the setup phase (before all the reconcile).

3. Test dependency with a remote client performances

• The key to avoid conficts when updating CRS bindings is to keep reconcile duration short.
• By default, the remote client used to apply CRS has a verty small QPS and burst.
• With 10 CRS reconciling in parallel and all using the same remote client, throttling was kicking in, making the test duration unpredictable.
• @sbueringer is working to a separate PR to make this configurable

We should wait for @sbueringer PR to merge before checking again if this fix the issue

[sbueringer]

Just a few nits, otherwise lgtm

[sbueringer]

The separate PR mentioned above is open now (#10880)

[fabriziopandini]

/hold for #10880

[sbueringer]

Thx!

/lgtm
/approve

(keeping the hold for the other PR and thus also until after the beta tag is created)

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: d5f1201126700ccde19cea8527f7cac1a67a8734

[k8s-ci-robot]

[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

[sbueringer]

@fabriziopandini my PR is merged, you have to rebase if you want to run the test locally though

[sbueringer]

/test pull-cluster-api-test-main

[fabriziopandini]

/test pull-cluster-api-test-main

[sbueringer]

https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api/10869/pull-cluster-api-test-main/1813476101129768960 is a new flake independent of your PR (flaky on main, fails 100% on test-mink8s)

[sbueringer]

With the qps changes one run took 6s and another 7s. That is roughly what we expected (and far away from the 1m timeout)

[jimmidyson]

Thank you so much @fabriziopandini and @sbueringer for looking at this! Sorry for introducing this test flakiness while fixing the previous bug 😞

[jimmidyson]

/lgtm

[chrischdi]

/lgtm

[sbueringer]

/hold cancel

Let's keep an eye on the test. I think it should not be flaky anymore (even with the current timeout)

[jimmidyson]

Can we backport this to 1.7 branch too please?

[jimmidyson]

/cherrypick release-1.7

[k8s-infra-cherrypick-robot]

@jimmidyson: #10869 failed to apply on top of branch "release-1.7":

Applying: Make ClusterResourceSet controller more predictable
Applying: Improve TestClusterResourceSetReconciler
Using index info to reconstruct a base tree...
M	exp/addons/internal/controllers/clusterresourceset_controller_test.go
M	exp/addons/internal/controllers/suite_test.go
M	internal/test/envtest/environment.go
Falling back to patching base and 3-way merge...
Auto-merging internal/test/envtest/environment.go
Auto-merging exp/addons/internal/controllers/suite_test.go
CONFLICT (content): Merge conflict in exp/addons/internal/controllers/suite_test.go
Auto-merging exp/addons/internal/controllers/clusterresourceset_controller_test.go
CONFLICT (content): Merge conflict in exp/addons/internal/controllers/clusterresourceset_controller_test.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 Improve TestClusterResourceSetReconciler
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-1.7

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.

[sbueringer]

Can we backport this to 1.7 branch too please?

Yup, that's fine. Just needs a manual cherry-pick. Maybe you can check for other PRs we had in this area and that should probably also be cherry-picked (I think there were only 1-2 since your PR merged)

[jimmidyson]

@sbueringer Looks like we'd need to cherry-pick ##10756, #10880, and this one. Does that sound right to you?

[sbueringer]

Sounds reasonable!