🐛 Use a different object name for each ClusterResultSet test case

[fabriziopandini]

What this PR does / why we need it:
Fix ClusterResultSet test flakes by ensuring that each test case uses a different CRS object name (instead of reusing always the same name).

Which issue(s) this PR fixes:
Fixes #4075

[fabriziopandini]

/test pull-cluster-api-test-main

[fabriziopandini]

/test pull-cluster-api-test-main

[fabriziopandini]

/test pull-cluster-api-test-main

[fabriziopandini]

/test pull-cluster-api-test-main

[fabriziopandini]

/test pull-cluster-api-test-main

[fabriziopandini]

/test pull-cluster-api-test-main

[fabriziopandini]

/test pull-cluster-api-test-main

[fabriziopandini]

/test pull-cluster-api-test-main

[fabriziopandini]

@vincepri @jsturtevant @furkatgofurov7 PTAL

With this change, the ClusterResultSet test seems to be consistently passing

[jsturtevant]

I pulled your change into my branch and ran it locally via in goland and still see errors:

Summarizing 1 Failure:

[Fail] ClusterResourceSet Reconciler [It] Should reconcile a ClusterResourceSet when a resource is created that is part of ClusterResourceSet resources 
/home/jstur/projects/cluster-api/exp/addons/controllers/clusterresourceset_controller_test.go:312

Ran 5 of 5 Specs in 16.096 seconds
FAIL! -- 4 Passed | 1 Failed | 0 Pending | 0 Skipped

From my investigation what seems to be happening is the test client doesn't see the configMap right away (my custom logging):

I0115 09:11:02.883895 1738907 clusterresourceset_controller.go:292] controller-runtime/manager/controller/clusterresourceset "msg"="############## can't find the object!" "cluster"="cluster-84jeuv" "name"="test-clusterresourceset" "namespace"="default" "reconciler group"="addons.cluster.x-k8s.io" "reconciler kind"="ClusterResourceSet" "ns"="default" "resource"={"name":"test-configmap3","kind":"ConfigMap"}

and it quietly returns ctrl.Result{} so the controller doesn't reconcile again. As a test, I updated to return and re-run when the ConfigMap isn't found and it passes consistently:

cluster-api/exp/addons/controllers/clusterresourceset_controller.go

Lines 280 to 283 in 1ac7896

	// Continue without adding the error to the aggregate if we can't find the resource.
	if apierrors.IsNotFound(err) {
	continue
	}

to

if apierrors.IsNotFound(err) {
					log.Info("############## can't find the object!", "resource", resource, "ns", cluster.GetNamespace())
					return err
					//continue
				}

Seems like there are two things happening... The test controller doesn't get the value correctly (initially) and second if the the resource isn't there (maybe because of delay or caching) the reconciliation might not happen.

[jsturtevant]

looks like the test failed in prow as well :-(

I saw this comment: // It applies resources best effort and continue on scenarios like: unsupported resource types, failure during creation, missing resources.

Currently if the resource is not found it doesn't re-trigger reconciliation unless something else re-triggers it later. Should we consider adding this as an error and retrying reconciliation with a back-off in cases where the resource is missing?

[fabriziopandini]

@jsturtevant the prow failure is in the machinepool test, and it is tracked in #4068
I will investigate your note above, thanks for the detailed repo

[jsturtevant]

@jsturtevant the prow failure is in the machinepool test, and it is tracked in #4068

I see that now. looked too quick.

These are the rough changes with the logging for debugging and retry: https://github.com/kubernetes-sigs/cluster-api/compare/master...jsturtevant:crs-objects-not-found-retry?expand=1. Seems that this is mainly caused by something in the test client caching (since retries fix it) but it might make the CRS more resilient in the cases that something wasn't on the cluster when the reconciliation happened for what ever reason.

I also found that in this case where the ConfigMap object isn't found the conditions.MarkTrue(clusterResourceSet, addonsv1.ResourcesAppliedCondition) is applied to the cluster which is a bit strange to me.

[fabriziopandini]

/retest

[fabriziopandini]

/test pull-cluster-api-test-main

[fabriziopandini]

/test pull-cluster-api-test-main

[fabriziopandini]

/test pull-cluster-api-test-main

[fabriziopandini]

@jsturtevant thanks!
I have incorporated also the wait for the config map object to be created, and now this flake does not happen anymore both in prow and locally for me. If everyone agrees, I suggest to merge these changes and then make another round of optimization after keeping an eye on flakes for a few days

[fabriziopandini]

/milestone v0.4.0

[vincepri]

Could we use generate name instead?

[vincepri]

/milestone v0.4.0

[vincepri]

/approve
/lgtm

[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