Fix non terminating Data/DataTemplate deletion

Some Metal3Data and Metal3DataTemplates were not destroyed
in rare circumstances due to missing reconcile events caused
by two distinct problems.

DataTemplate deletion has a watch over DataClaims but deletion
can only be performed when Data are completely removed (finalizer
executed) because Data resource requires the template.

We keep the facts that Data and DataClaims are still existing.
* When DataClaims exist, we do not remove the finalizer and wait
  for reconcile events from the watch.
* If we have neither DataClaims or Data, we can safely
  remove the finalizer. Deletion is complete.
* Otherwise we have to make a busy wait on Data deletion
  as there is no more claim to generate event but we still
  need to wait until the Data finalizers have been executed.

Actively wait for DataClaim deletion: If Metal3Data is deleted
before the Metal3DataClaim, we must actively wait for the claim
deletion, as no other reconciliation event will be triggered.

Fixes: #1994
Co-authored-by: Pierre Crégut <[email protected]>
Co-authored-by: Thomas Morin <[email protected]>
Signed-off-by: Pierre Crégut <[email protected]>

baremetal/metal3datatemplate_manager.go

@@ -40,8 +40,8 @@ type DataTemplateManagerInterface interface {
 	UnsetFinalizer()
 	SetClusterOwnerRef(*clusterv1.Cluster) error
 	// UpdateDatas handles the Metal3DataClaims and creates or deletes Metal3Data accordingly.
-	// It returns the number of current allocations.
-	UpdateDatas(context.Context) (int, error)
+	// It returns if there are still Data object and undeleted DataClaims objects.
+	UpdateDatas(context.Context) (bool, bool, error)
 }
 
 // DataTemplateManager is responsible for performing machine reconciliation.
@@ -135,12 +135,7 @@ func (m *DataTemplateManager) getIndexes(ctx context.Context) (map[int]string, e
 			continue
 		}
 
-		// Get the claim Name, if unset use empty string, to still record the
-		// index being used, to avoid conflicts
-		claimName := ""
-		if dataObject.Spec.Claim.Name != "" {
-			claimName = dataObject.Spec.Claim.Name
-		}
+		claimName := dataObject.Spec.Claim.Name
 		m.DataTemplate.Status.Indexes[claimName] = dataObject.Spec.Index
 		indexes[dataObject.Spec.Index] = claimName
 	}
@@ -170,12 +165,13 @@ func (m *DataTemplateManager) updateStatusTimestamp() {
 }
 
 // UpdateDatas handles the Metal3DataClaims and creates or deletes Metal3Data accordingly.
-// It returns the number of current allocations.
-func (m *DataTemplateManager) UpdateDatas(ctx context.Context) (int, error) {
+// It returns if there are still Data object and undeleted DataClaims objects.
+func (m *DataTemplateManager) UpdateDatas(ctx context.Context) (bool, bool, error) {
 	indexes, err := m.getIndexes(ctx)
 	if err != nil {
-		return 0, err
+		return false, false, err
 	}
+	hasData := len(indexes) > 0
 
 	// get list of Metal3DataClaim objects
 	dataClaimObjects := infrav1.Metal3DataClaimList{}
@@ -186,28 +182,31 @@ func (m *DataTemplateManager) UpdateDatas(ctx context.Context) (int, error) {
 
 	err = m.client.List(ctx, &dataClaimObjects, opts)
 	if err != nil {
-		return 0, err
+		return false, false, err
 	}
 
+	hasClaims := false
 	// Iterate over the Metal3Data objects to find all indexes and objects
 	for _, dataClaim := range dataClaimObjects.Items {
 		dataClaim := dataClaim
 		// If DataTemplate does not point to this object, discard
 		if dataClaim.Spec.Template.Name != m.DataTemplate.Name {
 			continue
 		}
-
-		if dataClaim.Status.RenderedData != nil && dataClaim.DeletionTimestamp.IsZero() {
-			continue
+		if dataClaim.DeletionTimestamp.IsZero() {
+			hasClaims = true
+			if dataClaim.Status.RenderedData != nil {
+				continue
+			}
 		}
 
 		indexes, err = m.updateData(ctx, &dataClaim, indexes)
 		if err != nil {
-			return 0, err
+			return false, false, err
 		}
 	}
 	m.updateStatusTimestamp()
-	return len(indexes), nil
+	return hasData, hasClaims, nil
 }
 
 func (m *DataTemplateManager) updateData(ctx context.Context,

baremetal/metal3datatemplate_manager_test.go

@@ -256,7 +256,7 @@ var _ = Describe("Metal3DataTemplate manager", func() {
 			)
 			Expect(err).NotTo(HaveOccurred())
 
-			nbIndexes, err := templateMgr.UpdateDatas(context.TODO())
+			hasData, _, err := templateMgr.UpdateDatas(context.TODO())
 			if tc.expectRequeue || tc.expectError {
 				Expect(err).To(HaveOccurred())
 				if tc.expectRequeue {
@@ -267,7 +267,7 @@ var _ = Describe("Metal3DataTemplate manager", func() {
 			} else {
 				Expect(err).NotTo(HaveOccurred())
 			}
-			Expect(nbIndexes).To(Equal(tc.expectedNbIndexes))
+			Expect(hasData).To(Equal(tc.expectedNbIndexes > 0))
 			Expect(tc.template.Status.LastUpdated.IsZero()).To(BeFalse())
 			Expect(tc.template.Status.Indexes).To(Equal(tc.expectedIndexes))
 

controllers/metal3data_controller.go

@@ -113,11 +113,14 @@ func (r *Metal3DataReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 	if !metal3Data.ObjectMeta.DeletionTimestamp.IsZero() {
 		// Check if the Metal3DataClaim is gone. We cannot clean up until it is.
 		err := r.Client.Get(ctx, types.NamespacedName{Name: metal3Data.Spec.Claim.Name, Namespace: metal3Data.Spec.Claim.Namespace}, &infrav1.Metal3DataClaim{})
-		if err != nil && apierrors.IsNotFound(err) {
-			return r.reconcileDelete(ctx, metadataMgr)
+		if err != nil {
+			if apierrors.IsNotFound(err) {
+				return r.reconcileDelete(ctx, metadataMgr)
+			}
+			return ctrl.Result{}, err
 		}
-		// We were unable to determine if the Metal3DataClaim is gone
-		return ctrl.Result{}, err
+		// Requeue until Metal3DataClaim is gone.
+		return ctrl.Result{Requeue: true, RequeueAfter: requeueAfter}, nil
 	}
 
 	// Handle non-deleted machines

controllers/metal3datatemplate_controller.go

@@ -140,7 +140,7 @@ func (r *Metal3DataTemplateReconciler) reconcileNormal(ctx context.Context,
 	// If the Metal3DataTemplate doesn't have finalizer, add it.
 	dataTemplateMgr.SetFinalizer()
 
-	_, err := dataTemplateMgr.UpdateDatas(ctx)
+	_, _, err := dataTemplateMgr.UpdateDatas(ctx)
 	if err != nil {
 		return checkReconcileError(err, "Failed to recreate the status")
 	}
@@ -150,17 +150,20 @@ func (r *Metal3DataTemplateReconciler) reconcileNormal(ctx context.Context,
 func (r *Metal3DataTemplateReconciler) reconcileDelete(ctx context.Context,
 	dataTemplateMgr baremetal.DataTemplateManagerInterface,
 ) (ctrl.Result, error) {
-	allocationsCount, err := dataTemplateMgr.UpdateDatas(ctx)
+	hasData, hasClaims, err := dataTemplateMgr.UpdateDatas(ctx)
 	if err != nil {
 		return checkReconcileError(err, "Failed to recreate the status")
 	}
 
-	if allocationsCount == 0 {
-		// metal3datatemplate is marked for deletion and ready to be deleted,
-		// so remove the finalizer.
-		dataTemplateMgr.UnsetFinalizer()
+	if hasClaims {
+		return ctrl.Result{}, nil
+	}
+	if hasData {
+		return ctrl.Result{Requeue: true, RequeueAfter: requeueAfter}, nil
 	}
 
+	dataTemplateMgr.UnsetFinalizer()
+
 	return ctrl.Result{}, nil
 }
 

controllers/metal3datatemplate_controller_test.go

@@ -83,19 +83,19 @@ var _ = Describe("Metal3DataTemplate manager", func() {
 				}
 			}
 			if tc.m3dt != nil && !tc.m3dt.DeletionTimestamp.IsZero() && tc.reconcileDeleteError {
-				m.EXPECT().UpdateDatas(context.Background()).Return(0, errors.New(""))
+				m.EXPECT().UpdateDatas(context.Background()).Return(false, false, errors.New(""))
 			} else if tc.m3dt != nil && !tc.m3dt.DeletionTimestamp.IsZero() {
-				m.EXPECT().UpdateDatas(context.Background()).Return(0, nil)
+				m.EXPECT().UpdateDatas(context.Background()).Return(false, false, nil)
 				m.EXPECT().UnsetFinalizer()
 			}
 
 			if tc.m3dt != nil && tc.m3dt.DeletionTimestamp.IsZero() &&
 				tc.reconcileNormal {
 				m.EXPECT().SetFinalizer()
 				if tc.reconcileNormalError {
-					m.EXPECT().UpdateDatas(context.Background()).Return(0, errors.New(""))
+					m.EXPECT().UpdateDatas(context.Background()).Return(false, false, errors.New(""))
 				} else {
-					m.EXPECT().UpdateDatas(context.Background()).Return(1, nil)
+					m.EXPECT().UpdateDatas(context.Background()).Return(true, true, nil)
 				}
 			}
 
@@ -232,9 +232,9 @@ var _ = Describe("Metal3DataTemplate manager", func() {
 			m.EXPECT().SetFinalizer()
 
 			if !tc.UpdateError {
-				m.EXPECT().UpdateDatas(context.TODO()).Return(1, nil)
+				m.EXPECT().UpdateDatas(context.TODO()).Return(true, true, nil)
 			} else {
-				m.EXPECT().UpdateDatas(context.TODO()).Return(0, errors.New(""))
+				m.EXPECT().UpdateDatas(context.TODO()).Return(false, false, errors.New(""))
 			}
 
 			res, err := r.reconcileNormal(context.TODO(), m)
@@ -284,12 +284,12 @@ var _ = Describe("Metal3DataTemplate manager", func() {
 			m := baremetal_mocks.NewMockDataTemplateManagerInterface(gomockCtrl)
 
 			if !tc.DeleteError && tc.DeleteReady {
-				m.EXPECT().UpdateDatas(context.TODO()).Return(0, nil)
+				m.EXPECT().UpdateDatas(context.TODO()).Return(false, false, nil)
 				m.EXPECT().UnsetFinalizer()
 			} else if !tc.DeleteError {
-				m.EXPECT().UpdateDatas(context.TODO()).Return(1, nil)
+				m.EXPECT().UpdateDatas(context.TODO()).Return(true, true, nil)
 			} else {
-				m.EXPECT().UpdateDatas(context.TODO()).Return(0, errors.New(""))
+				m.EXPECT().UpdateDatas(context.TODO()).Return(false, false, errors.New(""))
 			}
 
 			res, err := r.reconcileDelete(context.TODO(), m)