From 8b39e8682b05f8f13e82125361ad681db5e8752c Mon Sep 17 00:00:00 2001 From: Danil Grigorev Date: Thu, 11 Jul 2024 11:49:01 +0200 Subject: [PATCH] Apply finalizer on rancher cluster to avoid re-create race condition (#597) Signed-off-by: Danil-Grigorev --- internal/controllers/import_controller_v3.go | 53 ++++++++++++------- .../controllers/import_controller_v3_test.go | 29 +++++++--- 2 files changed, 54 insertions(+), 28 deletions(-) diff --git a/internal/controllers/import_controller_v3.go b/internal/controllers/import_controller_v3.go index d6106cdd..38936675 100644 --- a/internal/controllers/import_controller_v3.go +++ b/internal/controllers/import_controller_v3.go @@ -150,18 +150,17 @@ func (r *CAPIImportManagementV3Reconciler) Reconcile(ctx context.Context, req ct return ctrl.Result{Requeue: true}, err } + log = log.WithValues("cluster", capiCluster.Name) + if capiCluster.ObjectMeta.DeletionTimestamp.IsZero() && !turtlesannotations.HasClusterImportAnnotation(capiCluster) && - !controllerutil.ContainsFinalizer(capiCluster, managementv3.CapiClusterFinalizer) { - log.Info("CAPI cluster is imported, adding finalizer") - controllerutil.AddFinalizer(capiCluster, managementv3.CapiClusterFinalizer) + controllerutil.AddFinalizer(capiCluster, managementv3.CapiClusterFinalizer) { + log.Info("CAPI cluster is marked for import, adding finalizer") if err := r.Client.Update(ctx, capiCluster); err != nil { return ctrl.Result{}, fmt.Errorf("error adding finalizer: %w", err) } } - log = log.WithValues("cluster", capiCluster.Name) - // Wait for controlplane to be ready. This should never be false as the predicates // do the filtering. if !capiCluster.Status.ControlPlaneReady && !conditions.IsTrue(capiCluster, clusterv1.ControlPlaneReadyCondition) { @@ -231,16 +230,25 @@ func (r *CAPIImportManagementV3Reconciler) reconcile(ctx context.Context, capiCl rancherCluster = &rancherClusterList.Items[0] } + if !rancherCluster.ObjectMeta.DeletionTimestamp.IsZero() { + if err := r.reconcileDelete(ctx, capiCluster); err != nil { + log.Error(err, "Removing CAPI Cluster failed, retrying") + return ctrl.Result{}, err + } + + if controllerutil.RemoveFinalizer(rancherCluster, managementv3.CapiClusterFinalizer) { + if err := r.Client.Update(ctx, rancherCluster); err != nil { + return ctrl.Result{}, fmt.Errorf("error removing rancher cluster finalizer: %w", err) + } + } + } + if !capiCluster.ObjectMeta.DeletionTimestamp.IsZero() { if err := r.deleteDependentRancherCluster(ctx, capiCluster); err != nil { return ctrl.Result{}, fmt.Errorf("error deleting associated managementv3.Cluster resources: %w", err) } } - if !rancherCluster.ObjectMeta.DeletionTimestamp.IsZero() { - return r.reconcileDelete(ctx, capiCluster) - } - return r.reconcileNormal(ctx, capiCluster, rancherCluster) } @@ -270,6 +278,9 @@ func (r *CAPIImportManagementV3Reconciler) reconcileNormal(ctx context.Context, capiClusterOwnerNamespace: capiCluster.Namespace, ownedLabelName: "", }, + Finalizers: []string{ + managementv3.CapiClusterFinalizer, + }, }, Spec: managementv3.ClusterSpec{ DisplayName: capiCluster.Name, @@ -296,9 +307,10 @@ func (r *CAPIImportManagementV3Reconciler) reconcileNormal(ctx context.Context, return ctrl.Result{}, err } - if feature.Gates.Enabled(feature.PropagateLabels) { - patchBase := client.MergeFromWithOptions(rancherCluster.DeepCopy(), client.MergeFromWithOptimisticLock{}) + patchBase := client.MergeFromWithOptions(rancherCluster.DeepCopy(), client.MergeFromWithOptimisticLock{}) + needsFinalizer := controllerutil.AddFinalizer(rancherCluster, managementv3.CapiClusterFinalizer) + if feature.Gates.Enabled(feature.PropagateLabels) { if rancherCluster.Labels == nil { rancherCluster.Labels = map[string]string{} } @@ -312,6 +324,10 @@ func (r *CAPIImportManagementV3Reconciler) reconcileNormal(ctx context.Context, } log.Info("Successfully propagated labels to Rancher cluster") + } else if needsFinalizer { + if err := r.Client.Patch(ctx, rancherCluster, patchBase); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to patch Rancher cluster: %w", err) + } } if conditions.IsTrue(rancherCluster, managementv3.ClusterConditionReady) { @@ -431,7 +447,7 @@ func (r *CAPIImportManagementV3Reconciler) rancherV1ClusterToCapiCluster(ctx con } } -func (r *CAPIImportManagementV3Reconciler) reconcileDelete(ctx context.Context, capiCluster *clusterv1.Cluster) (ctrl.Result, error) { +func (r *CAPIImportManagementV3Reconciler) reconcileDelete(ctx context.Context, capiCluster *clusterv1.Cluster) error { log := log.FromContext(ctx) log.Info("Reconciling rancher cluster deletion") @@ -447,16 +463,13 @@ func (r *CAPIImportManagementV3Reconciler) reconcileDelete(ctx context.Context, annotations[turtlesannotations.ClusterImportedAnnotation] = "true" capiCluster.SetAnnotations(annotations) + controllerutil.RemoveFinalizer(capiCluster, managementv3.CapiClusterFinalizer) - if controllerutil.ContainsFinalizer(capiCluster, managementv3.CapiClusterFinalizer) { - controllerutil.RemoveFinalizer(capiCluster, managementv3.CapiClusterFinalizer) - - if err := r.Client.Update(ctx, capiCluster); err != nil { - return ctrl.Result{}, fmt.Errorf("error removing finalizer: %w", err) - } + if err := r.Client.Update(ctx, capiCluster); err != nil { + return fmt.Errorf("error removing finalizer: %w", err) } - return ctrl.Result{}, nil + return nil } func (r *CAPIImportManagementV3Reconciler) deleteDependentRancherCluster(ctx context.Context, capiCluster *clusterv1.Cluster) error { @@ -471,7 +484,7 @@ func (r *CAPIImportManagementV3Reconciler) deleteDependentRancherCluster(ctx con }, } - return r.RancherClient.DeleteAllOf(ctx, &managementv3.Cluster{}, selectors...) + return client.IgnoreNotFound(r.RancherClient.DeleteAllOf(ctx, &managementv3.Cluster{}, selectors...)) } // verifyV1ClusterMigration verifies if a v1 cluster has been successfully migrated. diff --git a/internal/controllers/import_controller_v3_test.go b/internal/controllers/import_controller_v3_test.go index 5eca51b5..7cfb0764 100644 --- a/internal/controllers/import_controller_v3_test.go +++ b/internal/controllers/import_controller_v3_test.go @@ -170,7 +170,7 @@ var _ = Describe("reconcile CAPI Cluster", func() { }).Should(Succeed()) }) - It("should reconcile a CAPI cluster when rancher cluster doesn't exist", func() { + It("should reconcile a CAPI cluster when rancher cluster doesn't exist, and set finalizers", func() { ns.Labels = map[string]string{} Expect(cl.Update(ctx, ns)).To(Succeed()) capiCluster.Labels = map[string]string{ @@ -195,9 +195,15 @@ var _ = Describe("reconcile CAPI Cluster", func() { Eventually(ctx, func(g Gomega) { g.Expect(cl.List(ctx, rancherClusters, selectors...)).ToNot(HaveOccurred()) g.Expect(rancherClusters.Items).To(HaveLen(1)) + g.Expect(rancherClusters.Items[0].Name).To(ContainSubstring("c-")) + g.Expect(rancherClusters.Items[0].Labels).To(HaveKeyWithValue(testLabelName, testLabelVal)) + g.Expect(rancherClusters.Items[0].Finalizers).To(ContainElement(managementv3.CapiClusterFinalizer)) + }).Should(Succeed()) + + Eventually(ctx, func(g Gomega) { + g.Expect(cl.Get(ctx, client.ObjectKeyFromObject(capiCluster), capiCluster)).ToNot(HaveOccurred()) + g.Expect(capiCluster.Finalizers).To(ContainElement(managementv3.CapiClusterFinalizer)) }).Should(Succeed()) - Expect(rancherClusters.Items[0].Name).To(ContainSubstring("c-")) - Expect(rancherClusters.Items[0].Labels).To(HaveKeyWithValue(testLabelName, testLabelVal)) }) It("should reconcile a CAPI cluster when rancher cluster doesn't exist and annotation is set on the namespace", func() { @@ -223,7 +229,7 @@ var _ = Describe("reconcile CAPI Cluster", func() { Expect(rancherClusters.Items[0].Name).To(ContainSubstring("c-")) }) - It("should reconcile a CAPI cluster when rancher cluster exists", func() { + It("should reconcile a CAPI cluster when rancher cluster exists, and have finalizers set", func() { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) w.Write([]byte(sampleTemplate)) @@ -244,6 +250,7 @@ var _ = Describe("reconcile CAPI Cluster", func() { Eventually(ctx, func(g Gomega) { g.Expect(cl.List(ctx, rancherClusters, selectors...)).ToNot(HaveOccurred()) g.Expect(rancherClusters.Items).To(HaveLen(1)) + g.Expect(rancherClusters.Items[0].Finalizers).ToNot(ContainElement(managementv3.CapiClusterFinalizer)) }).Should(Succeed()) cluster := rancherClusters.Items[0] Expect(cluster.Name).To(ContainSubstring("c-")) @@ -282,11 +289,16 @@ var _ = Describe("reconcile CAPI Cluster", func() { Name: unstructuredObj.GetName(), }, unstructuredObj)).To(Succeed()) - g.Expect(cl.List(ctx, rancherClusters, selectors...)).ToNot(HaveOccurred()) - g.Expect(rancherClusters.Items).To(HaveLen(1)) - g.Expect(rancherClusters.Items[0].Name).To(ContainSubstring("c-")) - g.Expect(rancherClusters.Items[0].Labels).To(HaveKeyWithValue(testLabelName, testLabelVal)) } + + g.Expect(cl.Get(ctx, client.ObjectKeyFromObject(capiCluster), capiCluster)).ToNot(HaveOccurred()) + g.Expect(capiCluster.Finalizers).To(ContainElement(managementv3.CapiClusterFinalizer)) + + g.Expect(cl.List(ctx, rancherClusters, selectors...)).ToNot(HaveOccurred()) + g.Expect(rancherClusters.Items).To(HaveLen(1)) + g.Expect(rancherClusters.Items[0].Name).To(ContainSubstring("c-")) + g.Expect(rancherClusters.Items[0].Labels).To(HaveKeyWithValue(testLabelName, testLabelVal)) + g.Expect(rancherClusters.Items[0].Finalizers).To(ContainElement(managementv3.CapiClusterFinalizer)) }, 10*time.Second).Should(Succeed()) }) @@ -534,4 +546,5 @@ var _ = Describe("reconcile CAPI Cluster", func() { Expect(rancherClusters.Items).To(HaveLen(0)) }).Should(Succeed()) }) + })