Fix non terminating DataTemplate deletion

DataTemplate deletion has a watch over DataClaims but deletion
can only be performed when Data are completely removed (finalizer
executed) because Data need 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.

Fixes: metal3-io#1994
Co-authored-by: Pierre Crégut <pierre.cregut@orange.com>
Co-authored-by: Thomas Morin <thomas.morin@orange.com>
Signed-off-by: Pierre Crégut <pierre.cregut@orange.com>

baremetal/metal3datatemplate_manager.go

@@ -41,7 +41,7 @@ type DataTemplateManagerInterface interface {
 	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)
+	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/metal3datatemplate_controller.go

@@ -142,7 +142,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")
 	}
@@ -152,17 +152,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)