From 15724773b36432745f90be6649669edaad6a56e8 Mon Sep 17 00:00:00 2001 From: Kubernetes Prow Robot <20407524+k8s-ci-robot@users.noreply.github.com> Date: Wed, 17 Jul 2024 06:09:07 -0700 Subject: [PATCH] :seedling: Make ClusterResourceSet controller more predictable --- .../clusterresourceset_controller.go | 16 ++++++ .../clusterresourceset_controller_test.go | 49 +++++++++++-------- exp/addons/internal/controllers/suite_test.go | 8 +-- internal/test/envtest/environment.go | 2 +- 4 files changed, 51 insertions(+), 24 deletions(-) diff --git a/exp/addons/internal/controllers/clusterresourceset_controller.go b/exp/addons/internal/controllers/clusterresourceset_controller.go index 03482f7d05d2..5aabd8e71f7f 100644 --- a/exp/addons/internal/controllers/clusterresourceset_controller.go +++ b/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) } diff --git a/exp/addons/internal/controllers/clusterresourceset_controller_test.go b/exp/addons/internal/controllers/clusterresourceset_controller_test.go index 8235ef4f4c43..5f13a545e965 100644 --- a/exp/addons/internal/controllers/clusterresourceset_controller_test.go +++ b/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{ diff --git a/exp/addons/internal/controllers/suite_test.go b/exp/addons/internal/controllers/suite_test.go index 2e688f51d611..c41b562dc789 100644 --- a/exp/addons/internal/controllers/suite_test.go +++ b/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)) } diff --git a/internal/test/envtest/environment.go b/internal/test/envtest/environment.go index a4ab7382614e..a40cfadbeb86 100644 --- a/internal/test/envtest/environment.go +++ b/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.