Merge pull request #2873 from metal3-io-bot/cherry-pick-2868-to-release-1.11

🐛 Fix for 1.10->1.11 upgrade issue

Author: metal3-io-bot

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)
 }