From c4fbd7acdbb98f5d59f2807b539554ca671069ee Mon Sep 17 00:00:00 2001 From: Bulat Zamalutdinov <13433317+qJkee@users.noreply.github.com> Date: Mon, 2 Mar 2026 11:32:13 +0400 Subject: [PATCH] feat(node-deletion): remove stale LVMVolumeGroup finalizers remove LVMVolumeGroup node finalizer when node is being deleted --- .../controllers/node/removal/controller.go | 31 +++++--- .../node/removal/controller_test.go | 75 +++++++++++++++++++ 2 files changed, 94 insertions(+), 12 deletions(-) diff --git a/internal/controllers/node/removal/controller.go b/internal/controllers/node/removal/controller.go index 20c66fca1..2c9fb46ea 100644 --- a/internal/controllers/node/removal/controller.go +++ b/internal/controllers/node/removal/controller.go @@ -6,12 +6,14 @@ import ( lvmv1alpha1 "github.com/openshift/lvm-operator/v4/api/v1alpha1" "github.com/openshift/lvm-operator/v4/internal/controllers/constants" + "github.com/openshift/lvm-operator/v4/internal/controllers/vgmanager" v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -64,8 +66,8 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu } } - if err := r.removeWipeAnnotationsForNode(ctx, nodeStatus.GetName()); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to remove wipe annotations for Node %s: %w", nodeStatus.GetName(), err) + if err := r.cleanupVolumeGroupsForNode(ctx, nodeStatus.GetName()); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to clean up VolumeGroups for Node %s: %w", nodeStatus.GetName(), err) } return ctrl.Result{}, nil @@ -120,9 +122,13 @@ func removeDeleteProtectionFinalizer(status *lvmv1alpha1.LVMVolumeGroupNodeStatu return false } -// removeWipeAnnotationsForNode removes wipe annotations for the given node from all LVMVolumeGroup objects. -func (r *Reconciler) removeWipeAnnotationsForNode(ctx context.Context, nodeName string) error { +// cleanupVolumeGroupsForNode removes the node cleanup finalizer and wipe annotation +// for the given node from all LVMVolumeGroup objects. +// This prevents stale finalizers from blocking VolumeGroup deletion +// after a node is removed from the cluster. +func (r *Reconciler) cleanupVolumeGroupsForNode(ctx context.Context, nodeName string) error { logger := log.FromContext(ctx) + finalizer := fmt.Sprintf("%s/%s", vgmanager.NodeCleanupFinalizer, nodeName) annotationKey := constants.DevicesWipedAnnotationPrefix + nodeName volumeGroups := &lvmv1alpha1.LVMVolumeGroupList{} @@ -130,19 +136,20 @@ func (r *Reconciler) removeWipeAnnotationsForNode(ctx context.Context, nodeName return fmt.Errorf("failed to list LVMVolumeGroups: %w", err) } - for i := range volumeGroups.Items { - vg := &volumeGroups.Items[i] - if vg.Annotations == nil { - continue + for _, vg := range volumeGroups.Items { + needsUpdate := controllerutil.RemoveFinalizer(&vg, finalizer) + + if _, exists := vg.Annotations[annotationKey]; exists { + delete(vg.Annotations, annotationKey) + needsUpdate = true } - if _, exists := vg.Annotations[annotationKey]; !exists { + if !needsUpdate { continue } - logger.Info("removing wipe annotation from LVMVolumeGroup", "lvmVolumeGroup", vg.Name, "node", nodeName) - delete(vg.Annotations, annotationKey) - if err := r.Update(ctx, vg); err != nil { + logger.Info("cleaning up LVMVolumeGroup for removed node", "lvmVolumeGroup", vg.Name, "node", nodeName) + if err := r.Update(ctx, &vg); err != nil { return fmt.Errorf("failed to update LVMVolumeGroup %s: %w", vg.Name, err) } } diff --git a/internal/controllers/node/removal/controller_test.go b/internal/controllers/node/removal/controller_test.go index 6b6d954c7..ad857546d 100644 --- a/internal/controllers/node/removal/controller_test.go +++ b/internal/controllers/node/removal/controller_test.go @@ -6,6 +6,7 @@ import ( lvmv1alpha1 "github.com/openshift/lvm-operator/v4/api/v1alpha1" "github.com/openshift/lvm-operator/v4/internal/controllers/node/removal" + "github.com/openshift/lvm-operator/v4/internal/controllers/vgmanager" "github.com/stretchr/testify/assert" v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" @@ -183,3 +184,77 @@ func TestNodeRemovalController_Reconcile(t *testing.T) { }) } } + +func TestNodeRemovalController_CleansUpVolumeGroupsOnNodeDeletion(t *testing.T) { + const ( + namespace = "test" + nodeName = "test-node" + otherNodeName = "other-node" + ) + nodeFinalizer := vgmanager.NodeCleanupFinalizer + "/" + nodeName + otherNodeFinalizer := vgmanager.NodeCleanupFinalizer + "/" + otherNodeName + nodeWipeAnnotation := "wiped.devices.lvms.openshift.io/" + nodeName + otherNodeWipeAnnotation := "wiped.devices.lvms.openshift.io/" + otherNodeName + + req := controllerruntime.Request{NamespacedName: types.NamespacedName{ + Name: nodeName, + Namespace: namespace, + }} + + objs := []client.Object{ + &lvmv1alpha1.LVMVolumeGroupNodeStatus{ + ObjectMeta: metav1.ObjectMeta{Name: nodeName, Namespace: namespace}, + }, + // VG with both the deleted node's finalizer/annotation and another node's + &lvmv1alpha1.LVMVolumeGroup{ + ObjectMeta: metav1.ObjectMeta{ + Name: "vg-both-nodes", + Namespace: namespace, + Finalizers: []string{nodeFinalizer, otherNodeFinalizer}, + Annotations: map[string]string{ + nodeWipeAnnotation: "true", + otherNodeWipeAnnotation: "true", + }, + }, + }, + // VG with only the other node's finalizer — should not be modified + &lvmv1alpha1.LVMVolumeGroup{ + ObjectMeta: metav1.ObjectMeta{ + Name: "vg-other-node-only", + Namespace: namespace, + Finalizers: []string{otherNodeFinalizer}, + }, + }, + } + + newScheme := runtime.NewScheme() + assert.NoError(t, lvmv1alpha1.AddToScheme(newScheme)) + assert.NoError(t, v1.AddToScheme(newScheme)) + + clnt := fake.NewClientBuilder(). + WithObjects(objs...). + WithScheme(newScheme). + WithIndex(&lvmv1alpha1.LVMVolumeGroupNodeStatus{}, "metadata.name", func(object client.Object) []string { + return []string{object.GetName()} + }). + Build() + + r := removal.NewReconciler(clnt, namespace) + _, err := r.Reconcile(context.Background(), req) + assert.NoError(t, err) + + ctx := context.Background() + + // VG that had both nodes' data: only the deleted node's finalizer and annotation should be gone + vgBoth := &lvmv1alpha1.LVMVolumeGroup{} + assert.NoError(t, clnt.Get(ctx, types.NamespacedName{Name: "vg-both-nodes", Namespace: namespace}, vgBoth)) + assert.NotContains(t, vgBoth.Finalizers, nodeFinalizer) + assert.Contains(t, vgBoth.Finalizers, otherNodeFinalizer) + assert.NotContains(t, vgBoth.Annotations, nodeWipeAnnotation) + assert.Contains(t, vgBoth.Annotations, otherNodeWipeAnnotation) + + // VG unrelated to the deleted node: should be untouched + vgOther := &lvmv1alpha1.LVMVolumeGroup{} + assert.NoError(t, clnt.Get(ctx, types.NamespacedName{Name: "vg-other-node-only", Namespace: namespace}, vgOther)) + assert.Contains(t, vgOther.Finalizers, otherNodeFinalizer) +}