[release-4.20] OCPBUGS-78348: feat(node-deletion): remove stale LVMVolumeGroup finalizers

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,29 +122,34 @@ 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{}
 	if err := r.List(ctx, volumeGroups, client.InNamespace(r.Namespace)); err != nil {
 		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)
 		}
 	}

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