Skip to content

Commit

Permalink
Apply finalizer on rancher cluster to avoid re-create race condition (#…
Browse files Browse the repository at this point in the history 
…597)

Signed-off-by: Danil-Grigorev <[email protected]>
  • Loading branch information
@Danil-Grigorev
Danil-Grigorev authored Jul 11, 2024
1 parent 6ce1c25 commit 8b39e86
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 28 deletions.
53 changes: 33 additions & 20 deletions internal/controllers/import_controller_v3.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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,
Expand All @@ -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{}
}
Expand All @@ -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) {
Expand Down Expand Up @@ -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")

Expand All @@ -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 {
Expand All @@ -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.
Expand Down
29 changes: 21 additions & 8 deletions internal/controllers/import_controller_v3_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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() {
Expand All @@ -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))
Expand All @@ -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-"))
Expand Down Expand Up @@ -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())
})

Expand Down Expand Up @@ -534,4 +546,5 @@ var _ = Describe("reconcile CAPI Cluster", func() {
Expect(rancherClusters.Items).To(HaveLen(0))
}).Should(Succeed())
})

})

0 comments on commit 8b39e86

Please sign in to comment.