Repository: topolvm/topolvm (go)

Issue:
LogicalVolumes "leak" and slow down provisioning
**Describe the bug**

`LogicalVolume` objects tied to deleted nodes are not cleaned up and cannot be cleaned up, because the `finalizer` field is cleared by the `topolvm-node` on the node itself (which is no longer running).

Over time this results in the number of LV objects building up in Kubernetes.  The controller is constantly trying to process these and that leads to a general slowdown in that and thus in the provisioning of new volumes (as it's spending a lot of time/threads trying to delete LVs which can't be deleted)

According to [the docs](https://github.com/topolvm/topolvm/blob/6ed49a173e07fdfe48fc98af3ea02a1e2ea2903c/docs/crd-logical-volume.md?plain=1#L54):

> `LogicalVolume` is created with a [finalizer](https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definitions/#finalizers).
When a `LogicalVolume` is being deleted, `topolvm-node` on the target node deletes
the corresponding LVM logical volume and clears the finalizer.

I've seen this happening on two of my own clusters today: one had 400+ stale LVs and another 1200+, built up over a year or so (through multiple topolvm upgrades but there were stale LVs created today - I upgraded to 0.18.1 weeks ago). On the larger cluster it was taking many hours to do a full rolling redeploy of 170 pods over 22 deployments

Note that LVs actually seem to get into two states:

1. They are marked for deletion (have a deletionTimestamp), but the finalizer blocks it
2. They belong to an invalid node (`.spec.nodeName` is no longer a valid node) but _are not_ marked for deletion (and still have the finalizer, so once they're deleted 1 will become an issue)

**Environments**
- Version: 0.18.1
- OS: EKS v0.23

**To Reproduce**
Steps to reproduce the behavior:

I haven't figured out a surefire way to reproduce this - I can delete a node and the `topolvm-node` process _usually_ seems to pick that up. I suspect that given certain conditions that process is either slow or the pod isn't running or gets terminated too quickly, and it doesn't get to removing the finalizer

**Expected behavior**
If the node that a LogicalVolume belongs to no longer exists, the controller should remove the finalizer itself before asking for its deletion

**Additional context**

These two scripts tidy up the LVs for me, in the short term I'll keep running them on an ad-hoc basis:

1. Delete LVs tied to non-existant nodes:
```
kubectl get logicalvolumes -ojson | jq -c .items[] | grep -vf <(kubectl get no -ojson | jq .items[].metadata.name -r) | jq -r .metadata.name | xargs kubectl delete logicalvolume
```

2. Remove finalizer on deleted LVs to allow deletion to complete:
```
kubectl get LogicalVolume -ojson | jq .items[] -c | grep deletionTimestamp | jq -r .metadata.name | xargs -I{} kubectl patch LogicalVolume {} -p '{"metadata":{"finalizers":null}}' --type merge
```

Pull request description:
Don't re-add finalizer to LogicalVolumes that are about to be deleted
This fixes a race-condition between node_controller and logicalvolume_controller where node_controller removes the finalizer from the LogicalVolume, then the lv_controller re-adds it before the node_controller can delete the LV.

If this happens then the LogicalVolume gets 'stuck' in an undeleteable state, because the node is gone so topolvm-node isn't around to remove the finalizer again.

Over time this can result ina  cluster having many undeleteable LogicalVolumes, which slows down topolvm to the point where it can't provision new volumes at all

Closes #715

Patch:
diff --git a/constants.go b/constants.go
index e84b8da0..29252513 100644
--- a/constants.go
+++ b/constants.go
@@ -55,6 +55,11 @@ func GetResizeRequestedAtKey() string {
 	return fmt.Sprintf("%s/resize-requested-at", GetPluginName())
 }
 
+// GetPendingDeletionKey returns the name of the pending-deletion annotation
+func GetLVPendingDeletionKey() string {
+	return fmt.Sprintf("%s/pendingdeletion", GetPluginName())
+}
+
 // GetLogicalVolumeFinalizer returns the name of LogicalVolume finalizer
 func GetLogicalVolumeFinalizer() string {
 	return fmt.Sprintf("%s/logicalvolume", GetPluginName())
diff --git a/controllers/logicalvolume_controller.go b/controllers/logicalvolume_controller.go
index a6899bd2..dd9d2926 100644
--- a/controllers/logicalvolume_controller.go
+++ b/controllers/logicalvolume_controller.go
@@ -42,6 +42,14 @@ func NewLogicalVolumeReconciler(client client.Client, nodeName string, conn *grp
 		lvService: proto.NewLVServiceClient(conn),
 	}
 }
+func NewLogicalVolumeReconcilerWithServices(client client.Client, nodeName string, vgService proto.VGServiceClient, lvService proto.LVServiceClient) *LogicalVolumeReconciler {
+	return &LogicalVolumeReconciler{
+		client:    client,
+		nodeName:  nodeName,
+		vgService: vgService,
+		lvService: lvService,
+	}
+}
 
 // Reconcile creates/deletes LVM logical volume for a LogicalVolume.
 func (r *LogicalVolumeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
@@ -60,6 +68,18 @@ func (r *LogicalVolumeReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 		return ctrl.Result{}, nil
 	}
 
+	if lv.Annotations != nil {
+		_, pendingDeletion := lv.Annotations[topolvm.GetLVPendingDeletionKey()]
+		if pendingDeletion {
+			if controllerutil.ContainsFinalizer(lv, topolvm.GetLogicalVolumeFinalizer()) {
+				log.Error(nil, "logical volume was pending deletion but still has finalizer", "name", lv.Name)
+			} else {
+				log.Info("skipping finalizer for logical volume due to its pending deletion", "name", lv.Name)
+			}
+			return ctrl.Result{}, nil
+		}
+	}
+
 	if lv.ObjectMeta.DeletionTimestamp == nil {
 		if !controllerutil.ContainsFinalizer(lv, topolvm.GetLogicalVolumeFinalizer()) {
 			lv2 := lv.DeepCopy()
diff --git a/controllers/node_controller.go b/controllers/node_controller.go
index 36d70840..2bb2ffbd 100644
--- a/controllers/node_controller.go
+++ b/controllers/node_controller.go
@@ -144,6 +144,11 @@ func (r *NodeReconciler) doFinalize(ctx context.Context, log logr.Logger, node *
 func (r *NodeReconciler) cleanupLogicalVolume(ctx context.Context, log logr.Logger, lv *topolvmv1.LogicalVolume) error {
 	if controllerutil.ContainsFinalizer(lv, topolvm.GetLogicalVolumeFinalizer()) {
 		lv2 := lv.DeepCopy()
+		if lv2.Annotations == nil {
+			lv2.Annotations = make(map[string]string)
+		}
+		// Flag the LV as pending deletion, so the LogicalVolumeReconciler doesn't re-add the finalizer before it sees the deletion
+		lv2.Annotations[topolvm.GetLVPendingDeletionKey()] = "true"
 		controllerutil.RemoveFinalizer(lv2, topolvm.GetLogicalVolumeFinalizer())
 		patch := client.MergeFrom(lv)
 		if err := r.client.Patch(ctx, lv2, patch); err != nil {


Test patch:
diff --git a/constants_test.go b/constants_test.go
index f502b14e..9973753b 100644
--- a/constants_test.go
+++ b/constants_test.go
@@ -93,6 +93,11 @@ func TestGetResizeRequestedAtKey(t *testing.T) {
 	doContainTest(t, GetResizeRequestedAtKey)
 }
 
+func TestGetLVPendingDeletionKey(t *testing.T) {
+	testingutil.DoEnvCheck(t)
+	doContainTest(t, GetLVPendingDeletionKey)
+}
+
 func TestGetLogicalVolumeFinalizer(t *testing.T) {
 	testingutil.DoEnvCheck(t)
 	doContainTest(t, GetLogicalVolumeFinalizer)
diff --git a/controllers/logicalvolume_controller_test.go b/controllers/logicalvolume_controller_test.go
new file mode 100644
index 00000000..f313d6a2
--- /dev/null
+++ b/controllers/logicalvolume_controller_test.go
@@ -0,0 +1,223 @@
+package controllers
+
+import (
+	"context"
+	"time"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+	"github.com/topolvm/topolvm"
+	topolvmv1 "github.com/topolvm/topolvm/api/v1"
+	proto "github.com/topolvm/topolvm/lvmd/proto"
+	"google.golang.org/grpc"
+	corev1 "k8s.io/api/core/v1"
+	storegev1 "k8s.io/api/storage/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
+)
+
+var volumes = &[]*proto.LogicalVolume{}
+
+type MockVGServiceClient struct {
+}
+
+// GetFreeBytes implements proto.VGServiceClient.
+func (MockVGServiceClient) GetFreeBytes(ctx context.Context, in *proto.GetFreeBytesRequest, opts ...grpc.CallOption) (*proto.GetFreeBytesResponse, error) {
+	panic("unimplemented")
+}
+
+// GetLVList implements proto.VGServiceClient.
+func (c MockVGServiceClient) GetLVList(ctx context.Context, in *proto.GetLVListRequest, opts ...grpc.CallOption) (*proto.GetLVListResponse, error) {
+	return &proto.GetLVListResponse{
+		Volumes: *volumes,
+	}, nil
+}
+
+// Watch implements proto.VGServiceClient.
+func (MockVGServiceClient) Watch(ctx context.Context, in *proto.Empty, opts ...grpc.CallOption) (proto.VGService_WatchClient, error) {
+	panic("unimplemented")
+}
+
+type MockLVServiceClient struct {
+}
+
+// CreateLV implements proto.LVServiceClient.
+func (c MockLVServiceClient) CreateLV(ctx context.Context, in *proto.CreateLVRequest, opts ...grpc.CallOption) (*proto.CreateLVResponse, error) {
+	lv := proto.LogicalVolume{
+		Name:   in.Name,
+		SizeGb: in.SizeGb,
+	}
+	*volumes = append(*volumes, &lv)
+	createResponse := proto.CreateLVResponse{
+		Volume: &lv,
+	}
+	return &createResponse, nil
+}
+
+// CreateLVSnapshot implements proto.LVServiceClient.
+func (MockLVServiceClient) CreateLVSnapshot(ctx context.Context, in *proto.CreateLVSnapshotRequest, opts ...grpc.CallOption) (*proto.CreateLVSnapshotResponse, error) {
+	panic("unimplemented")
+}
+
+// RemoveLV implements proto.LVServiceClient.
+func (MockLVServiceClient) RemoveLV(ctx context.Context, in *proto.RemoveLVRequest, opts ...grpc.CallOption) (*proto.Empty, error) {
+	panic("unimplemented")
+}
+
+// ResizeLV implements proto.LVServiceClient.
+func (MockLVServiceClient) ResizeLV(ctx context.Context, in *proto.ResizeLVRequest, opts ...grpc.CallOption) (*proto.Empty, error) {
+	panic("unimplemented")
+}
+
+var _ = Describe("LogicalVolume controller", func() {
+	ctx := context.Background()
+	var stopFunc func()
+	errCh := make(chan error)
+	var vgService MockVGServiceClient
+	var lvService MockLVServiceClient
+
+	startReconciler := func(suffix string) {
+		mgr, err := ctrl.NewManager(cfg, ctrl.Options{
+			Scheme: scheme,
+		})
+		Expect(err).ToNot(HaveOccurred())
+
+		vgService = MockVGServiceClient{}
+		lvService = MockLVServiceClient{}
+
+		reconciler := NewLogicalVolumeReconcilerWithServices(mgr.GetClient(), "node"+suffix, vgService, lvService)
+		err = reconciler.SetupWithManager(mgr)
+		Expect(err).NotTo(HaveOccurred())
+
+		ctx, cancel := context.WithCancel(ctx)
+		stopFunc = cancel
+		go func() {
+			errCh <- mgr.Start(ctx)
+		}()
+		time.Sleep(100 * time.Millisecond)
+	}
+
+	AfterEach(func() {
+		stopFunc()
+		Expect(<-errCh).NotTo(HaveOccurred())
+	})
+
+	setupResources := func(ctx context.Context, suffix string) topolvmv1.LogicalVolume {
+		node := corev1.Node{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "node" + suffix,
+				Finalizers: []string{
+					topolvm.GetNodeFinalizer(),
+				},
+			},
+		}
+		err := k8sClient.Create(ctx, &node)
+		Expect(err).NotTo(HaveOccurred())
+
+		sc := storegev1.StorageClass{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "sc" + suffix,
+			},
+			Provisioner: topolvm.GetPluginName(),
+		}
+		err = k8sClient.Create(ctx, &sc)
+		Expect(err).NotTo(HaveOccurred())
+
+		ns := createNamespace()
+		pvc := corev1.PersistentVolumeClaim{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "pvc" + suffix,
+				Namespace: ns,
+				Annotations: map[string]string{
+					AnnSelectedNode: node.Name,
+				},
+			},
+			Spec: corev1.PersistentVolumeClaimSpec{
+				StorageClassName: &sc.Name,
+				AccessModes: []corev1.PersistentVolumeAccessMode{
+					corev1.ReadWriteOnce,
+				},
+				Resources: corev1.ResourceRequirements{
+					Requests: corev1.ResourceList{
+						corev1.ResourceStorage: *resource.NewQuantity(1, resource.BinarySI),
+					},
+				},
+			},
+		}
+		err = k8sClient.Create(ctx, &pvc)
+		Expect(err).NotTo(HaveOccurred())
+
+		lv := topolvmv1.LogicalVolume{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "lv" + suffix,
+			},
+			Spec: topolvmv1.LogicalVolumeSpec{
+				NodeName: node.Name,
+			},
+		}
+		err = k8sClient.Create(ctx, &lv)
+		Expect(err).NotTo(HaveOccurred())
+
+		return lv
+	}
+
+	It("should add finalizer to LogicalVolume", func() {
+		startReconciler("-add-finalizer")
+
+		ctx := context.Background()
+
+		// Setup
+		lv := setupResources(ctx, "-add-finalizer")
+
+		// Verify
+		// ensure LV has finalizer
+		Eventually(func(g Gomega) bool {
+			err := k8sClient.Get(ctx, client.ObjectKeyFromObject(&lv), &lv)
+			if err != nil {
+				return false
+			}
+			return controllerutil.ContainsFinalizer(&lv, topolvm.GetLogicalVolumeFinalizer())
+		}).Should(BeTrue())
+	})
+
+	It("should not add finalizer to LogicalVolume when volume has pendingdeletion annotation", func() {
+		startReconciler("-pendingdeletion")
+
+		ctx := context.Background()
+
+		// Setup
+		lv := setupResources(ctx, "-pendingdeletion")
+
+		// ensure LV gets finalizer
+		Eventually(func(g Gomega) bool {
+			err := k8sClient.Get(ctx, client.ObjectKeyFromObject(&lv), &lv)
+			if err != nil {
+				return false
+			}
+			return controllerutil.ContainsFinalizer(&lv, topolvm.GetLogicalVolumeFinalizer())
+		}).Should(BeTrue())
+
+		// Add pending deletion key & remove finalizer
+		lv2 := lv.DeepCopy()
+		lv2.Annotations = map[string]string{
+			topolvm.GetLVPendingDeletionKey(): "true",
+		}
+		controllerutil.RemoveFinalizer(lv2, topolvm.GetLogicalVolumeFinalizer())
+
+		patch := client.MergeFrom(&lv)
+		err := k8sClient.Patch(ctx, lv2, patch)
+		Expect(err).NotTo(HaveOccurred())
+
+		// ensure LV finalizer is removed
+		Consistently(func(g Gomega) bool {
+			err := k8sClient.Get(ctx, client.ObjectKeyFromObject(&lv), &lv)
+			if err != nil {
+				return false
+			}
+			return !controllerutil.ContainsFinalizer(&lv, topolvm.GetLogicalVolumeFinalizer())
+		}, "2s").Should(BeTrue())
+	})
+})