🌱 Make ClusterResourceSet controller more predictable

exp/addons/internal/controllers/clusterresourceset_controller.go

@@ -161,6 +161,22 @@ func (r *ClusterResourceSetReconciler) Reconcile(ctx context.Context, req ctrl.R
 
 	// Return an aggregated error if errors occurred.
 	if len(errs) > 0 {
+		// When there are more than one ClusterResourceSet targeting the same cluster,
+		// there might be conflict when reconciling those ClusterResourceSet in parallel because they all try to
+		// patch the same ClusterResourceSetBinding Object.
+		// In case of patching conflicts we don't want to go on exponential backoff, otherwise it might take an
+		// arbitrary long time to get to stable state due to the backoff delay quickly growing.
+		// Instead, we are requeueing with an interval to make the system a little bit more predictable (and stabilize tests).
+		// NOTE: Conflicts happens mostly when ClusterResourceSetBinding is initialized / an entry is added for each
+		// cluster resource set targeting the same cluster.
+		for _, err := range errs {
+			if aggregate, ok := err.(kerrors.Aggregate); ok {
+				if len(aggregate.Errors()) == 1 && apierrors.IsConflict(aggregate.Errors()[0]) {
+					log.Info("Conflict in patching a ClusterResourceSetBinding that is updated by more than one ClusterResourceSet, requeueing")
+					return ctrl.Result{RequeueAfter: 100 * time.Millisecond}, nil
+				}
+			}
+		}
 		return ctrl.Result{}, kerrors.NewAggregate(errs)
 	}
 

exp/addons/internal/controllers/clusterresourceset_controller_test.go

@@ -56,23 +56,7 @@ func TestClusterResourceSetReconciler(t *testing.T) {
 		resourceConfigMapsNamespace = "default"
 	)
 
-	setup := func(t *testing.T, g *WithT) *corev1.Namespace {
-		t.Helper()
-
-		clusterResourceSetName = fmt.Sprintf("clusterresourceset-%s", util.RandomString(6))
-		labels = map[string]string{clusterResourceSetName: "bar"}
-
-		ns, err := env.CreateNamespace(ctx, namespacePrefix)
-		g.Expect(err).ToNot(HaveOccurred())
-
-		clusterName = fmt.Sprintf("cluster-%s", util.RandomString(6))
-		testCluster = &clusterv1.Cluster{ObjectMeta: metav1.ObjectMeta{Name: clusterName, Namespace: ns.Name}}
-
-		t.Log("Creating the Cluster")
-		g.Expect(env.Create(ctx, testCluster)).To(Succeed())
-		t.Log("Creating the remote Cluster kubeconfig")
-		g.Expect(env.CreateKubeconfigSecret(ctx, testCluster)).To(Succeed())
-
+	createConfigMapAndSecret := func(g Gomega, namespaceName, configmapName, secretName string) {
 		resourceConfigMap1Content := fmt.Sprintf(`metadata:
  name: %s
  namespace: %s
@@ -82,7 +66,7 @@ apiVersion: v1`, resourceConfigMap1Name, resourceConfigMapsNamespace)
 		testConfigmap := &corev1.ConfigMap{
 			ObjectMeta: metav1.ObjectMeta{
 				Name:      configmapName,
-				Namespace: ns.Name,
+				Namespace: namespaceName,
 			},
 			Data: map[string]string{
 				"cm": resourceConfigMap1Content,
@@ -98,7 +82,7 @@ metadata:
 		testSecret := &corev1.Secret{
 			ObjectMeta: metav1.ObjectMeta{
 				Name:      secretName,
-				Namespace: ns.Name,
+				Namespace: namespaceName,
 			},
 			Type: "addons.cluster.x-k8s.io/resource-set",
 			StringData: map[string]string{
@@ -108,7 +92,28 @@ metadata:
 		t.Log("Creating a Secret and a ConfigMap with ConfigMap in their data field")
 		g.Expect(env.Create(ctx, testConfigmap)).To(Succeed())
 		g.Expect(env.Create(ctx, testSecret)).To(Succeed())
+	}
 
+	setup := func(t *testing.T, g *WithT) *corev1.Namespace {
+		t.Helper()
+
+		clusterResourceSetName = fmt.Sprintf("clusterresourceset-%s", util.RandomString(6))
+		labels = map[string]string{clusterResourceSetName: "bar"}
+
+		ns, err := env.CreateNamespace(ctx, namespacePrefix)
+		g.Expect(err).ToNot(HaveOccurred())
+
+		clusterName = fmt.Sprintf("cluster-%s", util.RandomString(6))
+		testCluster = &clusterv1.Cluster{ObjectMeta: metav1.ObjectMeta{Name: clusterName, Namespace: ns.Name}}
+
+		t.Log("Creating the Cluster")
+		g.Expect(env.CreateAndWait(ctx, testCluster)).To(Succeed())
+		t.Log("Creating the remote Cluster kubeconfig")
+		g.Expect(env.CreateKubeconfigSecret(ctx, testCluster)).To(Succeed())
+		_, err = tracker.GetClient(ctx, client.ObjectKeyFromObject(testCluster))
+		g.Expect(err).ToNot(HaveOccurred())
+
+		createConfigMapAndSecret(g, ns.Name, configmapName, secretName)
 		return ns
 	}
 
@@ -1013,9 +1018,13 @@ metadata:
 
 		t.Log("Creating ClusterResourceSet instances that have same labels as selector")
 		for i := 0; i < 10; i++ {
+			configmapName := fmt.Sprintf("%s-%d", configmapName, i)
+			secretName := fmt.Sprintf("%s-%d", secretName, i)
+			createConfigMapAndSecret(g, ns.Name, configmapName, secretName)
+
 			clusterResourceSetInstance := &addonsv1.ClusterResourceSet{
 				ObjectMeta: metav1.ObjectMeta{
-					Name:      fmt.Sprintf("clusterresourceset-%s", util.RandomString(6)),
+					Name:      fmt.Sprintf("clusterresourceset-%d", i),
 					Namespace: ns.Name,
 				},
 				Spec: addonsv1.ClusterResourceSetSpec{

exp/addons/internal/controllers/suite_test.go

@@ -34,8 +34,9 @@ import (
 )
 
 var (
-	env *envtest.Environment
-	ctx = ctrl.SetupSignalHandler()
+	env     *envtest.Environment
+	tracker *remote.ClusterCacheTracker
+	ctx     = ctrl.SetupSignalHandler()
 )
 
 func TestMain(m *testing.M) {
@@ -46,7 +47,8 @@ func TestMain(m *testing.M) {
 	}
 
 	setupReconcilers := func(ctx context.Context, mgr ctrl.Manager) {
-		tracker, err := remote.NewClusterCacheTracker(mgr, remote.ClusterCacheTrackerOptions{})
+		var err error
+		tracker, err = remote.NewClusterCacheTracker(mgr, remote.ClusterCacheTrackerOptions{})
 		if err != nil {
 			panic(fmt.Sprintf("Failed to create new cluster cache tracker: %v", err))
 		}

internal/test/envtest/environment.go

@@ -384,7 +384,7 @@ func (e *Environment) waitForWebhooks() {
 
 // CreateKubeconfigSecret generates a new Kubeconfig secret from the envtest config.
 func (e *Environment) CreateKubeconfigSecret(ctx context.Context, cluster *clusterv1.Cluster) error {
-	return e.Create(ctx, kubeconfig.GenerateSecret(cluster, kubeconfig.FromEnvTestConfig(e.Config, cluster)))
+	return e.CreateAndWait(ctx, kubeconfig.GenerateSecret(cluster, kubeconfig.FromEnvTestConfig(e.Config, cluster)))
 }
 
 // Cleanup deletes all the given objects.