fix for 1.10->1.11 upgrade issue

This commit:
 - Implements a fallback Metal3Data retrieval process as part of the
   Metal3DataTemplate controller's Metal3DataClaim and Metal3Data deletion
   process
   -- The new process allows the controller to retrieve the Metal3Data based
      on the Metal3DataClaim's render information that supposed to point
      directly to the Metal3Data
 - Adds related unit test
 - Extends the existing "UpdateDatas" unit test table evaluation process
   to take into consideration that when a claim is deleted the related
   data has to be deleted too

Original issue:

If the templateReference was used before upgrade to 1.11 and Metal3Data was
generated based on the `templateReference name + Metal3DataClaim index`
instead of the `template name + data claim index` then after the upgrade during
the first new KCP or Metal3MachineDeployment rollout CAPM3 was unable to find
the old Metal3Data because after upgrade to 1.11 CAPM3 was only looking
for the Metal3Data based on `template name + data claim index` only while old
Metal3Data  was named based on `templateReference + index`.

In CAPM3 technically Metal3Data is allowed to get orphaned in case
CAPM3 can't find them based on the information in the Metal3DataClaim and
the related Metal3DataTemplate so the Metal3Data with names based on the
deprecated templateRefference got orphaned and then they were holding onto
IPClaims.

The presence of the orphaned Metal3Data in environments where IP
pre-allocation was used resulted in the old Metal3Data holding onto the pre
allocated IP claims thus the during the rollout the newly created Machies got
stuck as they were unable to re-claim the IPs.

Signed-off-by: Adam Rozman <[email protected]>

Author: Rozzii

baremetal/metal3datatemplate_manager.go

@@ -45,6 +45,12 @@ type DataTemplateManagerInterface interface {
 	UpdateDatas(context.Context) (bool, bool, error)
 }
 
+// Klogr log levels.
+const (
+	EXTRAINFO = 3
+	TRACE     = 5
+)
+
 // DataTemplateManager is responsible for performing machine reconciliation.
 type DataTemplateManager struct {
 	client       client.Client
@@ -167,7 +173,7 @@ func (m *DataTemplateManager) UpdateDatas(ctx context.Context) (bool, bool, erro
 	}
 
 	hasClaims := false
-	// Iterate over the Metal3Data objects to find all indexes and objects
+	// Iterate over the Metal3DataClaim objects to find all indexes and objects
 	for _, dataClaim := range dataClaimObjects.Items {
 		// If DataTemplate does not point to this object, discard
 		if dataClaim.Spec.Template.Name != m.DataTemplate.Name {
@@ -179,11 +185,12 @@ func (m *DataTemplateManager) UpdateDatas(ctx context.Context) (bool, bool, erro
 				continue
 			}
 		}
-
+		m.Log.V(TRACE).Info("Initiate updating data of claim", "Metal3DataClaim", dataClaim.Name, "Metal3DataTemplate", m.DataTemplate.Name)
 		indexes, err = m.updateData(ctx, &dataClaim, indexes)
 		if err != nil {
 			return false, false, err
 		}
+		m.Log.V(EXTRAINFO).Info("Success updating data of claim", "Metal3DataClaim", dataClaim.Name, "Metal3DataTemplate", m.DataTemplate.Name)
 	}
 	m.updateStatusTimestamp()
 	return hasData, hasClaims, nil
@@ -212,7 +219,8 @@ func (m *DataTemplateManager) updateData(ctx context.Context,
 			return indexes, err
 		}
 	} else {
-		indexes, err = m.deleteData(ctx, dataClaim, indexes)
+		m.Log.V(TRACE).Info("Attempting to delete data of claim and related data if present", "Metal3DataClaim", dataClaim.Name)
+		indexes, err = m.deleteMetal3DataAndClaim(ctx, dataClaim, indexes)
 		if err != nil {
 			return indexes, err
 		}
@@ -341,40 +349,87 @@ func (m *DataTemplateManager) createData(ctx context.Context,
 	return indexes, nil
 }
 
-// deleteData deletes the Metal3DataClaim and marks the Metal3Data for deletion.
-func (m *DataTemplateManager) deleteData(ctx context.Context,
+// handleRetrieveDataError handles errors generated by Metal3Data retrieval.
+// First return value is true if Metal3Data was retrieved successfully.
+// Second return value contains an error message if an unknown error was encountered.
+func (m *DataTemplateManager) handleRetrieveDataError(err error, filter string, dataClaimName string, dataName string) (bool, string) {
+	if err != nil && apierrors.IsNotFound(err) {
+		m.Log.Error(err, "Metal3Data NOT FOUND", "Metal3Data was not found based on filter:", filter, "Metal3DataClaim", dataClaimName, "Metal3DataTemplate", m.DataTemplate.Name, "Metal3Data", dataName)
+	} else if err != nil && !apierrors.IsNotFound(err) {
+		persistentErrMsg := "Failed to get Metal3Data object for reason OTHER THAN not finding it based on " + filter
+		m.Log.Error(err, persistentErrMsg, "Metal3DataClaim", dataClaimName, "Metal3DataTemplate", m.DataTemplate.Name, "Metal3Data", dataName)
+		return false, persistentErrMsg
+	} else if err == nil {
+		m.Log.V(TRACE).Info("Metal3Data found!", "Metal3DataClaim", dataClaimName, "Metal3DataTemplate", m.DataTemplate.Name, "Metal3Data", dataName)
+		return true, ""
+	}
+	return false, ""
+}
+
+// retrieveData is a utility to retrieve Metal3Data based on name and save the data to specified object.
+func (m *DataTemplateManager) retrieveData(ctx context.Context, dataName string, tmpM3Data *infrav1.Metal3Data) error {
+	key := client.ObjectKey{
+		Name:      dataName,
+		Namespace: m.DataTemplate.Namespace,
+	}
+	return m.client.Get(ctx, key, tmpM3Data)
+}
+
+// deleteMetal3DataAndClaim deletes the Metal3DataClaim and marks the Metal3Data for deletion.
+func (m *DataTemplateManager) deleteMetal3DataAndClaim(ctx context.Context,
 	dataClaim *infrav1.Metal3DataClaim, indexes map[int]string,
 ) (map[int]string, error) {
 	m.Log.Info("Deleting Metal3DataClaim", "Metal3DataClaim", dataClaim.Name)
+	persistentErrMsg := ""
+	m3DataFound := false
+	tmpM3Data := &infrav1.Metal3Data{}
+	dataName := ""
 
 	dataClaimIndex, ok := m.DataTemplate.Status.Indexes[dataClaim.Name]
+
 	if ok {
-		// Try to get the Metal3Data. if it succeeds, delete it
-		tmpM3Data := &infrav1.Metal3Data{}
-		key := client.ObjectKey{
-			Name:      m.DataTemplate.Name + "-" + strconv.Itoa(dataClaimIndex),
-			Namespace: m.DataTemplate.Namespace,
+		// Try to get the Metal3Data, if it succeeds, delete it
+		dataName = m.DataTemplate.Name + "-" + strconv.Itoa(dataClaimIndex)
+		err := m.retrieveData(ctx, dataName, tmpM3Data)
+		m3DataFound, persistentErrMsg = m.handleRetrieveDataError(err, "template name and claim index", dataClaim.Name, dataName)
+	} else {
+		persistentErrMsg = "Index of the claim was not found"
+		m.Log.Error(errors.New(persistentErrMsg), dataClaim.Name, "Metal3DataTemplate", m.DataTemplate.Name)
+	}
+
+	if !m3DataFound {
+		m.Log.V(TRACE).Info("Attempting to retrieve Metal3Data based on Metal3DataClaim render information")
+		if dataClaim != nil && dataClaim.Status.RenderedData != nil {
+			dataName = dataClaim.Status.RenderedData.Name
+			err := m.retrieveData(ctx, dataName, tmpM3Data)
+			errMsg := ""
+			m3DataFound, errMsg = m.handleRetrieveDataError(err, "render data reference", dataClaim.Name, dataName)
+			persistentErrMsg += errMsg
 		}
-		err := m.client.Get(ctx, key, tmpM3Data)
+	}
+
+	if m3DataFound {
+		// Remove the finalizer
+		m.Log.V(TRACE).Info("Attempting to remove finalizer from associated Metal3Data", "Metal3DataClaim", dataClaim.Name, "Metal3DataTemplate", m.DataTemplate.Name, "Metal3Data", dataName)
+		controllerutil.RemoveFinalizer(tmpM3Data, infrav1.DataClaimFinalizer)
+		err := updateObject(ctx, m.client, tmpM3Data)
 		if err != nil && !apierrors.IsNotFound(err) {
-			dataClaim.Status.ErrorMessage = ptr.To("Failed to get associated Metal3Data object")
+			m.Log.Error(errors.New("Unable to remove finalizer from Metal3Data"), tmpM3Data.Name)
+			return indexes, err
+		}
+		// Delete the Metal3Data
+		m.Log.V(EXTRAINFO).Info("Deleting associated Metal3Data", "Metal3DataClaim", dataClaim.Name, "Metal3DataTemplate", m.DataTemplate.Name, "Metal3Data", dataName)
+		err = deleteObject(ctx, m.client, tmpM3Data)
+		if err != nil && !apierrors.IsNotFound(err) {
+			dataClaim.Status.ErrorMessage = ptr.To("Failed to delete associated Metal3Data object")
 			return indexes, err
-		} else if err == nil {
-			// Remove the finalizer
-			controllerutil.RemoveFinalizer(tmpM3Data, infrav1.DataClaimFinalizer)
-			err = updateObject(ctx, m.client, tmpM3Data)
-			if err != nil && !apierrors.IsNotFound(err) {
-				m.Log.Info("Unable to remove finalizer from Metal3Data", "Metal3Data", tmpM3Data.Name)
-				return indexes, err
-			}
-			// Delete the Metal3Data
-			err = deleteObject(ctx, m.client, tmpM3Data)
-			if err != nil && !apierrors.IsNotFound(err) {
-				dataClaim.Status.ErrorMessage = ptr.To("Failed to delete associated Metal3Data object")
-				return indexes, err
-			}
-			m.Log.Info("Deleted Metal3Data", "Metal3Data", tmpM3Data.Name)
 		}
+		m.Log.Info("Deleted Metal3Data", "Metal3Data", tmpM3Data.Name)
+	} else {
+		errMsg := "Failed to retrieve Metal3Data object because it was not found or for other unknown reason."
+		persistentErrMsg += errMsg
+		dataClaim.Status.ErrorMessage = ptr.To(persistentErrMsg)
+		m.Log.Error(errors.New(errMsg), "error added to Metal3DataClaim status", "Metal3DataClaim", dataClaim.Name, "Metal3DataTemplate", m.DataTemplate.Name, "Metal3Data", dataName)
 	}
 
 	dataClaim.Status.RenderedData = nil

baremetal/metal3datatemplate_manager_test.go

@@ -250,9 +250,7 @@ var _ = Describe("Metal3DataTemplate manager", func() {
 				objects = append(objects, claim)
 			}
 			fakeClient := fake.NewClientBuilder().WithScheme(setupSchemeMm()).WithObjects(objects...).WithStatusSubresource(objects...).Build()
-			templateMgr, err := NewDataTemplateManager(fakeClient, tc.template,
-				logr.Discard(),
-			)
+			templateMgr, err := NewDataTemplateManager(fakeClient, tc.template, logr.Discard())
 			Expect(err).NotTo(HaveOccurred())
 
 			hasData, _, err := templateMgr.UpdateDatas(context.TODO())
@@ -270,19 +268,42 @@ var _ = Describe("Metal3DataTemplate manager", func() {
 			Expect(tc.template.Status.LastUpdated.IsZero()).To(BeFalse())
 			Expect(tc.template.Status.Indexes).To(Equal(tc.expectedIndexes))
 
-			// get list of Metal3Data objects
-			dataObjects := infrav1.Metal3DataClaimList{}
+			// get list of Metal3DataClaim objects
+			dataClaimObjects := infrav1.Metal3DataClaimList{}
 			opts := &client.ListOptions{}
-			err = fakeClient.List(context.TODO(), &dataObjects, opts)
+			err = fakeClient.List(context.TODO(), &dataClaimObjects, opts)
 			Expect(err).NotTo(HaveOccurred())
 
-			// Iterate over the Metal3Data objects to find all indexes and objects
-			for _, claim := range dataObjects.Items {
-				if claim.DeletionTimestamp.IsZero() {
+			// Iterate over the Metal3DataClaim objects to check
+			// if the render data points to somewhere on claims that are not
+			// being deleted
+			for _, claim := range dataClaimObjects.Items {
+				if claim.DeletionTimestamp == nil || claim.DeletionTimestamp.IsZero() {
 					Expect(claim.Status.RenderedData).NotTo(BeNil())
 				}
 			}
 
+			// get list of Metal3DataClaim objects
+			dataObjects := infrav1.Metal3DataList{}
+			opts = &client.ListOptions{}
+			err = fakeClient.List(context.TODO(), &dataObjects, opts)
+			Expect(err).NotTo(HaveOccurred())
+
+			for _, data := range dataObjects.Items {
+				if data.DeletionTimestamp == nil || data.DeletionTimestamp.IsZero() {
+					correctDeletion := false
+					parentClaim := data.Spec.Claim.Name
+					Expect(parentClaim).NotTo(BeEmpty())
+					for _, claim := range dataClaimObjects.Items {
+						if parentClaim == claim.ObjectMeta.Name {
+							Expect(claim.DeletionTimestamp.IsZero()).To(BeTrue())
+							Expect(claim.Status.RenderedData).NotTo(BeNil())
+							correctDeletion = true
+						}
+					}
+					Expect(correctDeletion).To(BeTrue())
+				}
+			}
 		},
 		Entry("No Claims", testCaseUpdateDatas{
 			template: &infrav1.Metal3DataTemplate{
@@ -321,7 +342,7 @@ var _ = Describe("Metal3DataTemplate manager", func() {
 					},
 					Status: infrav1.Metal3DataClaimStatus{
 						RenderedData: &corev1.ObjectReference{
-							Name:      "abc-2",
+							Name:      "abc-2", // Doesn't matter because we are not reconciling the "other template"
 							Namespace: namespaceName,
 						},
 					},
@@ -339,7 +360,7 @@ var _ = Describe("Metal3DataTemplate manager", func() {
 					},
 					Status: infrav1.Metal3DataClaimStatus{
 						RenderedData: &corev1.ObjectReference{
-							Name:      "abc-2",
+							Name:      "abc-1",
 							Namespace: namespaceName,
 						},
 					},
@@ -364,6 +385,26 @@ var _ = Describe("Metal3DataTemplate manager", func() {
 						},
 					},
 				},
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:              "deleting-claim-missmatch-name",
+						Namespace:         namespaceName,
+						DeletionTimestamp: &timeNow,
+						Finalizers:        []string{"ipclaim.ipam.metal3.io"},
+					},
+					Spec: infrav1.Metal3DataClaimSpec{
+						Template: corev1.ObjectReference{
+							Name:      templateMeta.Name,
+							Namespace: namespaceName,
+						},
+					},
+					Status: infrav1.Metal3DataClaimStatus{
+						RenderedData: &corev1.ObjectReference{
+							Name:      "cdc-3",
+							Namespace: namespaceName,
+						},
+					},
+				},
 			},
 			datas: []*infrav1.Metal3Data{
 				{
@@ -402,7 +443,7 @@ var _ = Describe("Metal3DataTemplate manager", func() {
 				},
 				{
 					ObjectMeta: metav1.ObjectMeta{
-						Name:      "data-to-delete",
+						Name:      "abc-3",
 						Namespace: namespaceName,
 					},
 					Spec: infrav1.Metal3DataSpec{
@@ -417,6 +458,23 @@ var _ = Describe("Metal3DataTemplate manager", func() {
 						Index: 3,
 					},
 				},
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "cdc-3",
+						Namespace: namespaceName,
+					},
+					Spec: infrav1.Metal3DataSpec{
+						Template: corev1.ObjectReference{
+							Name:      templateMeta.Name,
+							Namespace: namespaceName,
+						},
+						Claim: corev1.ObjectReference{
+							Name:      "deleting-claim-missmatch-name",
+							Namespace: namespaceName,
+						},
+						Index: 40,
+					},
+				},
 			},
 			expectedIndexes: map[string]int{
 				"claim-without-status": 0,
@@ -598,7 +656,7 @@ var _ = Describe("Metal3DataTemplate manager", func() {
 			)
 			Expect(err).NotTo(HaveOccurred())
 
-			allocatedMap, err := templateMgr.deleteData(context.TODO(), tc.dataClaim, tc.indexes)
+			allocatedMap, err := templateMgr.deleteMetal3DataAndClaim(context.TODO(), tc.dataClaim, tc.indexes)
 			if tc.expectError {
 				Expect(err).To(HaveOccurred())
 			} else {
@@ -681,6 +739,42 @@ var _ = Describe("Metal3DataTemplate manager", func() {
 				},
 			},
 		}),
+		Entry("Deletion needed but data name doesn't match template", testCaseDeleteDatas{
+			template: &infrav1.Metal3DataTemplate{
+				ObjectMeta: testObjectMeta("abc", "", ""),
+				Spec:       infrav1.Metal3DataTemplateSpec{},
+				Status: infrav1.Metal3DataTemplateStatus{
+					Indexes: map[string]int{
+						"TestRef": 0,
+					},
+				},
+			},
+			dataClaim: &infrav1.Metal3DataClaim{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "TestRef",
+					Finalizers: []string{
+						infrav1.DataClaimFinalizer,
+					},
+				},
+				Status: infrav1.Metal3DataClaimStatus{
+					RenderedData: &corev1.ObjectReference{
+						Name: "error-42",
+					},
+				},
+			},
+			indexes: map[int]string{
+				0: "TestRef",
+			},
+			expectedMap:     map[int]string{},
+			expectedIndexes: map[string]int{},
+			datas: []*infrav1.Metal3Data{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "error-42",
+					},
+				},
+			},
+		}),
 	)
 
 })

baremetal/metal3machine_manager.go

@@ -1770,9 +1770,11 @@ func (m *MachineManager) DissociateM3Metadata(ctx context.Context) error {
 		if !(errors.As(err, &reconcileError) && reconcileError.IsTransient()) {
 			return err
 		}
+		m.Log.Error(errors.New("related claim can't be retrieved because of unknown error"), "unknown error", "Metal3Machine", m.Metal3Machine.Name)
 		return nil
 	}
 	if metal3DataClaim == nil {
+		m.Log.Info("Related Metal3DataClaim is nil!", "Metal3Machine", m.Metal3Machine.Name)
 		return nil
 	}
 
@@ -1782,7 +1784,7 @@ func (m *MachineManager) DissociateM3Metadata(ctx context.Context) error {
 		m.Log.Info("Unable to remove finalizers from Metal3DataClaim", "Metal3DataClaim", metal3DataClaim.Name)
 		return err
 	}
-
+	m.Log.Info("Finalizers successfully removed. Initiate deletion of related claim.", "Metal3DataClaim", metal3DataClaim.Name)
 	return deleteObject(ctx, m.client, metal3DataClaim)
 }