Repository: topolvm/topolvm (go)

Issue:
TopoLVM ignores lvremove errors
**Describe the bug**

lvmd considers `lvremove` exit code 5 as "not found" error ([ref](https://github.com/topolvm/topolvm/blob/v0.28.0/internal/lvmd/command/lvm.go#L581-L585)). But lvm uses this code and for other reasons, at least:

```
root@node1:~# /sbin/lvm lvremove -f topolvm/cad639ab-1870-4399-a560-4db494b84017
  Logical volume topolvm/cad639ab-1870-4399-a560-4db494b84017 in use.
root@node1:~# echo $?
5
```
```
root@node1:~# /sbin/lvm lvremove -f topolvm/49e01e31-550a-428c-9c0d-247214420af4
  Logical volume topolvm/49e01e31-550a-428c-9c0d-247214420af4 contains a filesystem in use.
root@node1:~# echo $?
5
```

As a result topolvm-node ignores the error and abandons the volume on the node.

Related logs:

* lvmd:
  ```
  {"level":"info","ts":"2024-04-18T11:36:03Z","msg":"invoking command","args":["/usr/bin/nsenter","-m","-u","-i","-n","-p","-t","1","/sbin/lvm","lvremove","-f","topolvm/cad639ab-1870-4399-a560-4db494b84017"]}
  ```
* topolvm-node:
  ```
  {"level":"info","ts":"2024-04-18T11:35:50Z","logger":"driver.node","msg":"NodeUnpublishVolume called","volume_id":"cad639ab-1870-4399-a560-4db494b84017","target_path":"/var/lib/kubelet/pods/72864676-e737-464c-be06-8555e986541e/volumes/kubernetes.io~csi/pvc-3759b8b1-e5df-4101-8d72-4b19ef44f06d/mount"}
  {"level":"info","ts":"2024-04-18T11:35:50Z","logger":"driver.node","msg":"NodeUnpublishVolume(fs) is succeeded","volume_id":"cad639ab-1870-4399-a560-4db494b84017","target_path":"/var/lib/kubelet/pods/72864676-e737-464c-be06-8555e986541e/volumes/kubernetes.io~csi/pvc-3759b8b1-e5df-4101-8d72-4b19ef44f06d/mount"}
  {"level":"info","ts":"2024-04-18T11:36:02Z","msg":"start finalizing LogicalVolume","controller":"logicalvolume","controllerGroup":"topolvm.cybozu.com","controllerKind":"LogicalVolume","LogicalVolume":{"name":"pvc-3759b8b1-e5df-4101-8d72-4b19ef44f06d"},"namespace":"","name":"pvc-3759b8b1-e5df-4101-8d72-4b19ef44f06d","reconcileID":"0da5acdc-ac0b-43ec-b801-9ad0bfd7d03d","name":"pvc-3759b8b1-e5df-4101-8d72-4b19ef44f06d"}
  {"level":"info","ts":"2024-04-18T11:36:08Z","msg":"LV already removed","controller":"logicalvolume","controllerGroup":"topolvm.cybozu.com","controllerKind":"LogicalVolume","LogicalVolume":{"name":"pvc-3759b8b1-e5df-4101-8d72-4b19ef44f06d"},"namespace":"","name":"pvc-3759b8b1-e5df-4101-8d72-4b19ef44f06d","reconcileID":"0da5acdc-ac0b-43ec-b801-9ad0bfd7d03d","name":"pvc-3759b8b1-e5df-4101-8d72-4b19ef44f06d","uid":"cad639ab-1870-4399-a560-4db494b84017"}
  ```

**Environments**

- Version: 0.28.0
- OS: Ubuntu 22.04

**To Reproduce**

Steps to reproduce the behavior:

1. Create a Pod with generic ephemeral volume. Something like this:
    ```yaml
    apiVersion: v1
    kind: Pod
    metadata:
      name: test-pod
    spec:
      containers:
      - command:
        - sleep
        - infinity
        image: ubuntu:22.04
        name: test-container
        volumeMounts:
        - mountPath: /mnt/ephemeral-pv
          name: ephemeral-pv
      volumes:
      - ephemeral:
          volumeClaimTemplate:
            spec:
              accessModes:
              - ReadWriteOnce
              resources:
                requests:
                  storage: 1Gi
              storageClassName: topolvm-ext4
              volumeMode: Filesystem
        name: ephemeral-pv
    ```
2. Go to the k8s node and manually mount the corresponding volume elsewhere. For example:
    ```
    root@node1:~# mount /dev/mapper/topolvm-49e01e31--550a--428c--9c0d--247214420af4 /tmp/test-volume/
    root@node1:~#
    ```
3. Delete the pod.
4. Check topolvm-node logs for `LV already removed` message.
5. Check that the volume remains on the node after manually unmounting it (which resolves the issue with `lvmremove`).

**Expected behavior**

topolvm should continue to retry until a volume successfully removed (ever after node reboot).

**Additional context**

None.

Pull request description:
fix: use stricter not found matching for lvm calls
fix #889 by changing the simple exitcode==5 check to a check that also verifies the returned stderr for the regex `Volume group "(.*?)" not found|Failed to find logical volume "(.*?)` so that errors that have exit code 5 but are not about the volume not being present do not cause TopoLVM to ignore the volume.

Since the lvm error handling got a bit too big for my taste I moved it into a separate file. I also added a test case to cover this regex style parsing.

Patch:
diff --git a/internal/lvmd/command/lvm.go b/internal/lvmd/command/lvm.go
index 5dbb0123..2501188d 100644
--- a/internal/lvmd/command/lvm.go
+++ b/internal/lvmd/command/lvm.go
@@ -578,9 +578,7 @@ func (l *LogicalVolume) Resize(ctx context.Context, newSize uint64) error {
 func (vg *VolumeGroup) RemoveVolume(ctx context.Context, name string) error {
 	err := callLVM(ctx, "lvremove", "-f", fullName(name, vg))
 
-	if lvmErr, ok := AsLVMError(err); ok && lvmErr.ExitCode() == 5 {
-		// lvremove returns 5 if the volume does not exist, so we can convert this to ErrNotFound
-		// join it to the original error so that the caller can still see the stderr output.
+	if IsLVMNotFound(err) {
 		return errors.Join(ErrNotFound, err)
 	}
 
diff --git a/internal/lvmd/command/lvm_command.go b/internal/lvmd/command/lvm_command.go
index f7524d70..fbf18136 100644
--- a/internal/lvmd/command/lvm_command.go
+++ b/internal/lvmd/command/lvm_command.go
@@ -2,7 +2,6 @@ package command
 
 import (
 	"bufio"
-	"bytes"
 	"context"
 	"encoding/json"
 	"errors"
@@ -115,47 +114,3 @@ func (p commandReadCloser) Close() error {
 	}
 	return nil
 }
-
-// AsLVMError returns the LVMError from the error if it exists and a bool indicating if is an LVMError or not.
-func AsLVMError(err error) (LVMError, bool) {
-	var lvmErr LVMError
-	ok := errors.As(err, &lvmErr)
-	return lvmErr, ok
-}
-
-// LVMError is an error that wraps the original error and the stderr output of the lvm command if found.
-// It also provides an exit code if present that can be used to determine the type of error from LVM.
-// Regular inaccessible errors will have an exit code of 5.
-type LVMError interface {
-	error
-	ExitCode() int
-	Unwrap() error
-}
-
-type lvmErr struct {
-	err    error
-	stderr []byte
-}
-
-func (e *lvmErr) Error() string {
-	if e.stderr != nil {
-		return fmt.Sprintf("%v: %v", e.err, string(bytes.TrimSpace(e.stderr)))
-	}
-	return e.err.Error()
-}
-
-func (e *lvmErr) Unwrap() error {
-	return e.err
-}
-
-func (e *lvmErr) ExitCode() int {
-	type exitError interface {
-		ExitCode() int
-		error
-	}
-	var err exitError
-	if errors.As(e.err, &err) {
-		return err.ExitCode()
-	}
-	return -1
-}
diff --git a/internal/lvmd/command/lvm_error.go b/internal/lvmd/command/lvm_error.go
new file mode 100644
index 00000000..e3ef6a80
--- /dev/null
+++ b/internal/lvmd/command/lvm_error.go
@@ -0,0 +1,71 @@
+package command
+
+import (
+	"bytes"
+	"errors"
+	"fmt"
+	"regexp"
+)
+
+var (
+	// NotFoundPattern is a regular expression that matches the error message when a volume group or logical volume is not found.
+	// The volume group might not be present or the logical volume might not be present in the volume group.
+	NotFoundPattern = regexp.MustCompile(`Volume group "(.*?)" not found|Failed to find logical volume "(.*?)"`)
+)
+
+// IsLVMNotFound returns true if the error is a LVM recognized error and it determined that either
+// the underlying volume group or logical volume is not found.
+func IsLVMNotFound(err error) bool {
+	lvmErr, ok := AsLVMError(err)
+
+	// If the exit code is not 5, it is guaranteed that the error is not a not found error.
+	if !ok || lvmErr.ExitCode() != 5 {
+		return false
+	}
+
+	return NotFoundPattern.Match([]byte(lvmErr.Error()))
+}
+
+// AsLVMError returns the LVMError from the error if it exists and a bool indicating if is an LVMError or not.
+func AsLVMError(err error) (LVMError, bool) {
+	var lvmErr LVMError
+	ok := errors.As(err, &lvmErr)
+	return lvmErr, ok
+}
+
+// LVMError is an error that wraps the original error and the stderr output of the lvm command if found.
+// It also provides an exit code if present that can be used to determine the type of error from LVM.
+// Regular inaccessible errors will have an exit code of 5.
+type LVMError interface {
+	error
+	ExitCode() int
+	Unwrap() error
+}
+
+type lvmErr struct {
+	err    error
+	stderr []byte
+}
+
+func (e *lvmErr) Error() string {
+	if e.stderr != nil {
+		return fmt.Sprintf("%v: %v", e.err, string(bytes.TrimSpace(e.stderr)))
+	}
+	return e.err.Error()
+}
+
+func (e *lvmErr) Unwrap() error {
+	return e.err
+}
+
+func (e *lvmErr) ExitCode() int {
+	type exitError interface {
+		ExitCode() int
+		error
+	}
+	var err exitError
+	if errors.As(e.err, &err) {
+		return err.ExitCode()
+	}
+	return -1
+}
diff --git a/internal/lvmd/command/lvm_lvs.go b/internal/lvmd/command/lvm_lvs.go
index be60e073..7cc1bd65 100644
--- a/internal/lvmd/command/lvm_lvs.go
+++ b/internal/lvmd/command/lvm_lvs.go
@@ -3,6 +3,7 @@ package command
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"strconv"
 	"strings"
 )
@@ -124,10 +125,8 @@ func getLVReport(ctx context.Context, name string) (map[string]lv, error) {
 	}
 	err := callLVMInto(ctx, res, args...)
 
-	if lvmErr, ok := AsLVMError(err); ok && lvmErr.ExitCode() == 5 {
-		// lvs returns 5 if the volume does not exist, so we can convert this to ErrNotFound
-		// join it to the original error so that the caller can still see the stderr output.
-		return nil, ErrNotFound
+	if IsLVMNotFound(err) {
+		return nil, errors.Join(ErrNotFound, err)
 	}
 
 	if err != nil {
diff --git a/internal/lvmd/command/lvm_vgs.go b/internal/lvmd/command/lvm_vgs.go
index 61a8bc79..d6baa47c 100644
--- a/internal/lvmd/command/lvm_vgs.go
+++ b/internal/lvmd/command/lvm_vgs.go
@@ -3,6 +3,7 @@ package command
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"strconv"
 )
 
@@ -54,11 +55,10 @@ func getVGReport(ctx context.Context, name string) (vg, error) {
 	}
 	err := callLVMInto(ctx, res, args...)
 
-	if lvmErr, ok := AsLVMError(err); ok && lvmErr.ExitCode() == 5 {
-		// vgs returns 5 if the volume group does not exist, so we can convert this to ErrNotFound
-		// join it to the original error so that the caller can still see the stderr output.
-		return vg{}, ErrNotFound
+	if IsLVMNotFound(err) {
+		return vg{}, errors.Join(ErrNotFound, err)
 	}
+
 	if err != nil {
 		return vg{}, err
 	}


Test patch:
diff --git a/internal/lvmd/command/lvm_command_test.go b/internal/lvmd/command/lvm_command_test.go
index c200df3e..db47f732 100644
--- a/internal/lvmd/command/lvm_command_test.go
+++ b/internal/lvmd/command/lvm_command_test.go
@@ -40,6 +40,39 @@ func Test_lvm_command(t *testing.T) {
 		}
 	})
 
+	t.Run("simple lvm vgs should return not found but other failures should not", func(t *testing.T) {
+		var messages []string
+		funcLog := funcr.New(func(_, args string) {
+			messages = append(messages, args)
+		}, funcr.Options{
+			Verbosity: 9,
+		})
+
+		ctx := log.IntoContext(context.Background(), funcLog)
+
+		err := callLVM(ctx, "vgs", "non-existing-vg")
+		if err == nil {
+			t.Fatal(err, "vg should not exist")
+		}
+
+		if !IsLVMNotFound(err) {
+			t.Fatal("error should be not found")
+		}
+
+		if len(messages) != 1 || !strings.Contains(messages[0], "invoking command") {
+			t.Fatal("there should be nothing in stdout except the invoking command log")
+		}
+
+		err = callLVM(ctx, "foobar")
+		if err == nil {
+			t.Fatal(err, "command should not be recognized")
+		}
+
+		if IsLVMNotFound(err) {
+			t.Fatal("error should not be not found")
+		}
+	})
+
 	t.Run("simple lvm vgcreate with non existing device should fail and show logs", func(t *testing.T) {
 		// fakeDeviceName is a string that does not exist on the system (or rather is highly unlikely to exist)
 		// it is used to test the error handling of the callLVMStreamed function