From fe486bbe7a1993cd91b99facfc883d1b8859f064 Mon Sep 17 00:00:00 2001 From: Lennart Jern Date: Mon, 20 May 2024 15:29:13 +0300 Subject: [PATCH 1/2] Protect Metal3Data and Metal3DataClaim with Finalizers Signed-off-by: Lennart Jern Co-authored-by: Max Rantil --- baremetal/metal3datatemplate_manager.go | 36 ++++++++++++++++--------- baremetal/metal3machine_manager.go | 12 +++++++++ baremetal/utils.go | 2 +- 3 files changed, 36 insertions(+), 14 deletions(-) diff --git a/baremetal/metal3datatemplate_manager.go b/baremetal/metal3datatemplate_manager.go index 3e98ca35b9..856670760e 100644 --- a/baremetal/metal3datatemplate_manager.go +++ b/baremetal/metal3datatemplate_manager.go @@ -18,7 +18,6 @@ package baremetal import ( "context" - "fmt" "strconv" "github.com/go-logr/logr" @@ -306,16 +305,17 @@ func (m *DataTemplateManager) createData(ctx context.Context, m.Log.Info("Index", "Claim", dataClaim.Name, "index", claimIndex) // Create the Metal3Data object, with an Owner ref to the Metal3Machine - // (curOwnerRef) and to the Metal3DataTemplate + // (curOwnerRef) and to the Metal3DataTemplate. Also add a finalizer. dataObject := &infrav1.Metal3Data{ TypeMeta: metav1.TypeMeta{ Kind: "Metal3Data", APIVersion: infrav1.GroupVersion.String(), }, ObjectMeta: metav1.ObjectMeta{ - Name: dataName, - Namespace: m.DataTemplate.Namespace, - Labels: dataClaim.Labels, + Name: dataName, + Namespace: m.DataTemplate.Namespace, + Finalizers: []string{infrav1.DataClaimFinalizer}, + Labels: dataClaim.Labels, OwnerReferences: []metav1.OwnerReference{ { Controller: ptr.To(true), @@ -374,18 +374,18 @@ func (m *DataTemplateManager) createData(ctx context.Context, return indexes, nil } -// DeleteDatas deletes old secrets. +// deleteData deletes the Metal3DataClaim and marks the Metal3Data for deletion. func (m *DataTemplateManager) deleteData(ctx context.Context, dataClaim *infrav1.Metal3DataClaim, indexes map[int]string, ) (map[int]string, error) { - var dataName string - m.Log.Info("Deleting Claim", "Metal3DataClaim", dataClaim.Name) + m.Log.Info("Deleting Metal3DataClaim", "Metal3DataClaim", dataClaim.Name) dataClaimIndex, ok := m.DataTemplate.Status.Indexes[dataClaim.Name] if ok { // Try to get the Metal3Data. if it succeeds, delete it tmpM3Data := &infrav1.Metal3Data{} + var dataName string if m.DataTemplate.Spec.TemplateReference != "" { dataName = m.DataTemplate.Spec.TemplateReference + "-" + strconv.Itoa(dataClaimIndex) } else { @@ -401,26 +401,36 @@ func (m *DataTemplateManager) deleteData(ctx context.Context, dataClaim.Status.ErrorMessage = ptr.To("Failed to get associated Metal3Data object") return indexes, err } else if err == nil { - // Delete the secret with metadata - fmt.Println(tmpM3Data.Name) - err = m.client.Delete(ctx, tmpM3Data) + // Remove the finalizer + tmpM3Data.Finalizers = Filter(tmpM3Data.Finalizers, + 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) } } + dataClaim.Status.RenderedData = nil dataClaim.Finalizers = Filter(dataClaim.Finalizers, infrav1.DataClaimFinalizer, ) - m.Log.Info("Deleted Claim", "Metal3DataClaim", dataClaim.Name) - if ok { delete(m.DataTemplate.Status.Indexes, dataClaim.Name) delete(indexes, dataClaimIndex) } + + m.Log.Info("Deleted Metal3DataClaim", "Metal3DataClaim", dataClaim.Name) m.updateStatusTimestamp() return indexes, nil } diff --git a/baremetal/metal3machine_manager.go b/baremetal/metal3machine_manager.go index 8e85bd6d96..7c4a0ee2b6 100644 --- a/baremetal/metal3machine_manager.go +++ b/baremetal/metal3machine_manager.go @@ -1523,6 +1523,9 @@ func (m *MachineManager) AssociateM3Metadata(ctx context.Context) error { ObjectMeta: metav1.ObjectMeta{ Name: m.Metal3Machine.Name, Namespace: m.Metal3Machine.Namespace, + Finalizers: []string{ + infrav1.MachineFinalizer, + }, OwnerReferences: []metav1.OwnerReference{ { APIVersion: m.Metal3Machine.APIVersion, @@ -1652,6 +1655,15 @@ func (m *MachineManager) DissociateM3Metadata(ctx context.Context) error { return nil } + metal3DataClaim.Finalizers = Filter(metal3DataClaim.Finalizers, + infrav1.MachineFinalizer, + ) + err = updateObject(ctx, m.client, metal3DataClaim) + if err != nil && !apierrors.IsNotFound(err) { + m.Log.Info("Unable to remove finalizers from Metal3DataClaim", "Metal3DataClaim", metal3DataClaim.Name) + return err + } + return deleteObject(ctx, m.client, metal3DataClaim) } diff --git a/baremetal/utils.go b/baremetal/utils.go index 7c87ccd24b..2db3d05114 100644 --- a/baremetal/utils.go +++ b/baremetal/utils.go @@ -40,7 +40,7 @@ const ( metal3SecretType corev1.SecretType = "infrastructure.cluster.x-k8s.io/secret" ) -// Filter filters a list for a string. +// Filter filters out occurrences of strToFilter from list and returns the new list. func Filter(list []string, strToFilter string) (newList []string) { for _, item := range list { if item != strToFilter { From acb35f35dd581441c579efccc0861edd00e33ab5 Mon Sep 17 00:00:00 2001 From: Lennart Jern Date: Tue, 21 May 2024 11:08:00 +0300 Subject: [PATCH 2/2] Ensure M3DC is gone before cleaning up M3D Before cleaning up Metal3Data, check that the Metal3DataClaim (consumer) is gone. This commit also renames some of the test objects and variables to make them easier to understand and adds a constant for "Metal3Machine". Signed-off-by: Lennart Jern --- baremetal/metal3data_manager.go | 2 +- baremetal/metal3datatemplate_manager.go | 4 +-- baremetal/metal3datatemplate_manager_test.go | 34 ++++++++++---------- baremetal/utils.go | 2 ++ controllers/metal3data_controller.go | 30 ++++++++++------- 5 files changed, 40 insertions(+), 32 deletions(-) diff --git a/baremetal/metal3data_manager.go b/baremetal/metal3data_manager.go index 0440538f07..3636a32080 100644 --- a/baremetal/metal3data_manager.go +++ b/baremetal/metal3data_manager.go @@ -1437,7 +1437,7 @@ func (m *DataManager) getM3Machine(ctx context.Context, m3dt *infrav1.Metal3Data } // not matching on UID since when pivoting it might change // Not matching on API version as this might change - if ownerRef.Kind == "Metal3Machine" && + if ownerRef.Kind == metal3MachineKind && oGV.Group == infrav1.GroupVersion.Group { metal3MachineName = ownerRef.Name break diff --git a/baremetal/metal3datatemplate_manager.go b/baremetal/metal3datatemplate_manager.go index 856670760e..6c8701618f 100644 --- a/baremetal/metal3datatemplate_manager.go +++ b/baremetal/metal3datatemplate_manager.go @@ -271,7 +271,7 @@ func (m *DataTemplateManager) createData(ctx context.Context, if err != nil { return indexes, err } - if ownerRef.Kind == "Metal3Machine" && + if ownerRef.Kind == metal3MachineKind && aGV.Group == infrav1.GroupVersion.Group { m3mUID = ownerRef.UID m3mName = ownerRef.Name @@ -332,7 +332,7 @@ func (m *DataTemplateManager) createData(ctx context.Context, }, { APIVersion: dataClaim.APIVersion, - Kind: "Metal3Machine", + Kind: metal3MachineKind, Name: m3mName, UID: m3mUID, }, diff --git a/baremetal/metal3datatemplate_manager_test.go b/baremetal/metal3datatemplate_manager_test.go index 617107f2b9..38979c1e92 100644 --- a/baremetal/metal3datatemplate_manager_test.go +++ b/baremetal/metal3datatemplate_manager_test.go @@ -299,24 +299,24 @@ var _ = Describe("Metal3DataTemplate manager", func() { dataClaims: []*infrav1.Metal3DataClaim{ { ObjectMeta: metav1.ObjectMeta{ - Name: "abc", + Name: "claim-without-status", Namespace: namespaceName, }, Spec: infrav1.Metal3DataClaimSpec{ Template: corev1.ObjectReference{ - Name: "abc", + Name: templateMeta.Name, Namespace: namespaceName, }, }, }, { ObjectMeta: metav1.ObjectMeta{ - Name: "abcd", + Name: "orphaned-claim", Namespace: namespaceName, }, Spec: infrav1.Metal3DataClaimSpec{ Template: corev1.ObjectReference{ - Name: "abcd", + Name: "other-template", Namespace: namespaceName, }, }, @@ -329,12 +329,12 @@ var _ = Describe("Metal3DataTemplate manager", func() { }, { ObjectMeta: metav1.ObjectMeta{ - Name: "abce", + Name: "claim-with-status", Namespace: namespaceName, }, Spec: infrav1.Metal3DataClaimSpec{ Template: corev1.ObjectReference{ - Name: "abc", + Name: templateMeta.Name, Namespace: namespaceName, }, }, @@ -347,14 +347,14 @@ var _ = Describe("Metal3DataTemplate manager", func() { }, { ObjectMeta: metav1.ObjectMeta{ - Name: "abcf", + Name: "deleting-claim", Namespace: namespaceName, DeletionTimestamp: &timeNow, Finalizers: []string{"ipclaim.ipam.metal3.io"}, }, Spec: infrav1.Metal3DataClaimSpec{ Template: corev1.ObjectReference{ - Name: "abc", + Name: templateMeta.Name, Namespace: namespaceName, }, }, @@ -374,11 +374,11 @@ var _ = Describe("Metal3DataTemplate manager", func() { }, Spec: infrav1.Metal3DataSpec{ Template: corev1.ObjectReference{ - Name: "abc", + Name: templateMeta.Name, Namespace: namespaceName, }, Claim: corev1.ObjectReference{ - Name: "abc", + Name: "claim-without-status", Namespace: namespaceName, }, Index: 0, @@ -391,11 +391,11 @@ var _ = Describe("Metal3DataTemplate manager", func() { }, Spec: infrav1.Metal3DataSpec{ Template: corev1.ObjectReference{ - Name: "abc", + Name: templateMeta.Name, Namespace: namespaceName, }, Claim: corev1.ObjectReference{ - Name: "abce", + Name: "claim-with-status", Namespace: namespaceName, }, Index: 1, @@ -403,16 +403,16 @@ var _ = Describe("Metal3DataTemplate manager", func() { }, { ObjectMeta: metav1.ObjectMeta{ - Name: "abc-3", + Name: "data-to-delete", Namespace: namespaceName, }, Spec: infrav1.Metal3DataSpec{ Template: corev1.ObjectReference{ - Name: "abc", + Name: templateMeta.Name, Namespace: namespaceName, }, Claim: corev1.ObjectReference{ - Name: "abcf", + Name: "deleting-claim", Namespace: namespaceName, }, Index: 3, @@ -420,8 +420,8 @@ var _ = Describe("Metal3DataTemplate manager", func() { }, }, expectedIndexes: map[string]int{ - "abc": 0, - "abce": 1, + "claim-without-status": 0, + "claim-with-status": 1, }, expectedNbIndexes: 2, }), diff --git a/baremetal/utils.go b/baremetal/utils.go index 2db3d05114..ccd84100dd 100644 --- a/baremetal/utils.go +++ b/baremetal/utils.go @@ -38,6 +38,8 @@ import ( const ( // metal3SecretType defines the type of secret created by metal3. metal3SecretType corev1.SecretType = "infrastructure.cluster.x-k8s.io/secret" + // metal3MachineKind is the Kind of the Metal3Machine. + metal3MachineKind = "Metal3Machine" ) // Filter filters out occurrences of strToFilter from list and returns the new list. diff --git a/controllers/metal3data_controller.go b/controllers/metal3data_controller.go index e5e0d35fe0..077e89bdc4 100644 --- a/controllers/metal3data_controller.go +++ b/controllers/metal3data_controller.go @@ -60,29 +60,29 @@ func (r *Metal3DataReconciler) Reconcile(ctx context.Context, req ctrl.Request) metadataLog := r.Log.WithName(dataControllerName).WithValues("metal3-data", req.NamespacedName) // Fetch the Metal3Data instance. - capm3Metadata := &infrav1.Metal3Data{} + metal3Data := &infrav1.Metal3Data{} - if err := r.Client.Get(ctx, req.NamespacedName, capm3Metadata); err != nil { + if err := r.Client.Get(ctx, req.NamespacedName, metal3Data); err != nil { if apierrors.IsNotFound(err) { return ctrl.Result{}, nil } return ctrl.Result{}, err } - helper, err := patch.NewHelper(capm3Metadata, r.Client) + helper, err := patch.NewHelper(metal3Data, r.Client) if err != nil { return ctrl.Result{}, errors.Wrap(err, "failed to init patch helper") } - // Always patch capm3Data exiting this function so we can persist any Metal3Data changes. + // Always patch Metal3Data exiting this function so we can persist any changes. defer func() { - err := helper.Patch(ctx, capm3Metadata) + err := helper.Patch(ctx, metal3Data) if err != nil { metadataLog.Info("failed to Patch Metal3Data") } }() // Fetch the Cluster. - cluster, err := util.GetClusterFromMetadata(ctx, r.Client, capm3Metadata.ObjectMeta) - if capm3Metadata.ObjectMeta.DeletionTimestamp.IsZero() { + cluster, err := util.GetClusterFromMetadata(ctx, r.Client, metal3Data.ObjectMeta) + if metal3Data.ObjectMeta.DeletionTimestamp.IsZero() { if err != nil { metadataLog.Info("Metal3Data is missing cluster label or cluster does not exist") return ctrl.Result{}, nil @@ -97,21 +97,27 @@ func (r *Metal3DataReconciler) Reconcile(ctx context.Context, req ctrl.Request) metadataLog = metadataLog.WithValues("cluster", cluster.Name) // Return early if the Metadata or Cluster is paused. - if annotations.IsPaused(cluster, capm3Metadata) { + if annotations.IsPaused(cluster, metal3Data) { metadataLog.Info("reconciliation is paused for this object") return ctrl.Result{Requeue: true, RequeueAfter: requeueAfter}, nil } } // Create a helper for managing the metadata object. - metadataMgr, err := r.ManagerFactory.NewDataManager(capm3Metadata, metadataLog) + metadataMgr, err := r.ManagerFactory.NewDataManager(metal3Data, metadataLog) if err != nil { return ctrl.Result{}, errors.Wrapf(err, "failed to create helper for managing the Metal3Data") } - // Handle deleted metadata - if !capm3Metadata.ObjectMeta.DeletionTimestamp.IsZero() { - return r.reconcileDelete(ctx, metadataMgr) + // Handle deletion of Metal3Data + 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) + } + // We were unable to determine if the Metal3DataClaim is gone + return ctrl.Result{}, err } // Handle non-deleted machines