Repository: topolvm/topolvm (go)

Issue:
added writecache causes topolvm to be unable to find an LV it has created
**Describe the bug**

Created a raid1 LV via topolvm for a PVC. Later on, manually added a writecache to that LV outside of topolvm. After adding the writecache, topolvm can no longer find the LV. After removing the writecache, topolvm can find the LV again.

**Environments**
- Version: `ghcr.io/topolvm/topolvm-with-sidecar:0.36.0`
- OS: `PRETTY_NAME="Ubuntu 24.04.1 LTS"`

**To Reproduce**
Steps to reproduce the behavior:

1. configure `--type raid 1` LVs to be created (e.g., https://github.com/guppy0130/kubernetes/blob/99063376b1232b90164c31137f5db627838dd7be/topolvm/values.yaml#L138)
2. create the PVC, which in turn creates a new LV
3. `systemctl stop k3s`
4. add the writecache to the LV outside of topolvm
5. `systemctl start k3s`
6. attempt to mount the now-writecache'd LV into a pod (topolvm says it can't find the LV at this point)
7. stop the kubelet
8. remove the writecache
9. start the kubelet
10. attempt to mount the LV into a pod (succeeds now?)

**Expected behavior**
Since the name of the LV didn't change, topolvm should be able to find the LV and mount it at step 6

**Additional context**

this is what I did for step 4:

```console
root@k3s:~# vgextend hdd-vg /dev/nvme1n1p1
  Volume group "hdd-vg" successfully extended
root@k3s:~# lvcreate -n nvme-cache-for-hdd-vg -L 64G hdd-vg /dev/nvme1n1p1
  Logical volume "nvme-cache-for-hdd-vg" created.
root@k3s:~# lvchange -an /dev/hdd-vg/nvme-cache-for-hdd-vg
root@k3s:~# lvconvert --type writecache --cachevol nvme-cache-for-hdd-vg hdd-vg/5bd73b55-6ee4-4f0a-9af0-0d29c4cabc4d
Erase all existing data on hdd-vg/nvme-cache-for-hdd-vg? [y/n]: y
  Logical volume hdd-vg/5bd73b55-6ee4-4f0a-9af0-0d29c4cabc4d now has writecache.
```

this is after step 4:

```console
root@k3s:~# lvs -a
  LV                                                     VG        Attr       LSize    Pool                         Origin                                        Data%  Meta%  Move Log Cpy%Sync Convert
  5bd73b55-6ee4-4f0a-9af0-0d29c4cabc4d                   hdd-vg    Cwi-aoC---   13.00t [nvme-cache-for-hdd-vg_cvol] [5bd73b55-6ee4-4f0a-9af0-0d29c4cabc4d_wcorig] 0.00
  [5bd73b55-6ee4-4f0a-9af0-0d29c4cabc4d_wcorig]          hdd-vg    rwi-aoC---   13.00t                                                                                                   100.00
  [5bd73b55-6ee4-4f0a-9af0-0d29c4cabc4d_wcorig_rimage_0] hdd-vg    iwi-aor---   13.00t
  [5bd73b55-6ee4-4f0a-9af0-0d29c4cabc4d_wcorig_rimage_1] hdd-vg    iwi-aor---   13.00t
  [5bd73b55-6ee4-4f0a-9af0-0d29c4cabc4d_wcorig_rmeta_0]  hdd-vg    ewi-aor---    4.00m
  [5bd73b55-6ee4-4f0a-9af0-0d29c4cabc4d_wcorig_rmeta_1]  hdd-vg    ewi-aor---    4.00m
  [nvme-cache-for-hdd-vg_cvol]                           hdd-vg    Cwi-aoC---   64.00g
```

topoLVM should be able to see it though; assuming that topoLVM runs this to get all the LVs:

```console
root@k3s:~# lvm fullreport --reportformat json --units b --nosuffix --configreport vg -o vg_name,vg_uuid,vg_size,vg_free --configreport lv -o lv_uuid,lv_name,lv_full_name,lv_path,lv_size,lv_kernel_major,lv_kernel_minor,origin,origin_size,pool_lv,lv_tags,lv_attr,vg_name,data_percent,metadata_percent,pool_lv --configreport pv -o, --configreport pvseg -o, --configreport seg -o, | jq .
```

this JSON blob is inside that report (I've truncated it because the other LVs aren't relevant):

```json
{
  "lv_uuid": "Hfpar5-AE3Q-NBDD-h7Jk-ITzm-S6hc-HR4xvc",
  "lv_name": "5bd73b55-6ee4-4f0a-9af0-0d29c4cabc4d",
  "lv_full_name": "hdd-vg/5bd73b55-6ee4-4f0a-9af0-0d29c4cabc4d",
  "lv_path": "/dev/hdd-vg/5bd73b55-6ee4-4f0a-9af0-0d29c4cabc4d",
  "lv_size": "14293651161088",
  "lv_kernel_major": "252",
  "lv_kernel_minor": "16",
  "origin": "[5bd73b55-6ee4-4f0a-9af0-0d29c4cabc4d_wcorig]",
  "origin_size": "",
  "pool_lv": "[nvme-cache-for-hdd-vg_cvol]",
  "lv_tags": "",
  "lv_attr": "Cwi-aoC---",
  "vg_name": "hdd-vg",
  "data_percent": "0.00",
  "metadata_percent": ""
}
```

the log from my `topolvm-node-*` container:

```jsonc
{
  "level": "error",
  "ts": "2025-01-02T04:00:42Z",
  "msg": "error on grpc call",
  "method": "/csi.v1.Node/NodePublishVolume",
  "error": "rpc error: code = NotFound desc = failed to find LV: 5bd73b55-6ee4-4f0a-9af0-0d29c4cabc4d",
  "stacktrace": ""  // extracted into next code block
}
```

the stacktrace, extracted from the JSON blob and formatted a little better:

```go
github.com/topolvm/topolvm/cmd/topolvm-node/app.ErrorLoggingInterceptor
  /workdir/cmd/topolvm-node/app/run.go:202
github.com/container-storage-interface/spec/lib/go/csi._Node_NodePublishVolume_Handler
  /go/pkg/mod/github.com/container-storage-interface/spec@v1.9.0/lib/go/csi/csi.pb.go:7261
google.golang.org/grpc.(*Server).processUnaryRPC
  /go/pkg/mod/google.golang.org/grpc@v1.58.3/server.go:1374
google.golang.org/grpc.(*Server).handleStream
  /go/pkg/mod/google.golang.org/grpc@v1.58.3/server.go:1751
google.golang.org/grpc.(*Server).serveStreams.func1.1
  /go/pkg/mod/google.golang.org/grpc@v1.58.3/server.go:986
```

and some stuff from journal (which says more of the same stuff, not finding the LV)

```log
Jan 03 21:28:23 k3s k3s[452759]: E0103 21:28:23.704752  452759 nestedpendingoperations.go:348] Operation for "{volumeName:kubernetes.io/csi/topolvm.io^5bd73b55-6ee4-4f0a-9af0-0d29c4cabc4d podName: nodeName:}" failed. No retries permitted until 2025-01-03 21:28:27.70471428 -0800 PST m=+11.818499352 (durationBeforeRetry 4s). Error: MountVolume.SetUp failed for volume "pvc-1d9b46ff-ac68-4a0a-90bb-cf1844653762" (UniqueName: "kubernetes.io/csi/topolvm.io^5bd73b55-6ee4-4f0a-9af0-0d29c4cabc4d") pod "radarr-6b577679d6-pspcp" (UID: "b45cd810-d9c9-4e16-9168-9a533e1cfc0a") : rpc error: code = NotFound desc = failed to find LV: 5bd73b55-6ee4-4f0a-9af0-0d29c4cabc4d
```

after I un-writecache the LV, the output of the previous `lvm fullreport` (similarly truncated and formatted):

```json
{
  "lv_uuid": "Hfpar5-AE3Q-NBDD-h7Jk-ITzm-S6hc-HR4xvc",
  "lv_name": "5bd73b55-6ee4-4f0a-9af0-0d29c4cabc4d",
  "lv_full_name": "hdd-vg/5bd73b55-6ee4-4f0a-9af0-0d29c4cabc4d",
  "lv_path": "/dev/hdd-vg/5bd73b55-6ee4-4f0a-9af0-0d29c4cabc4d",
  "lv_size": "14293651161088",
  "lv_kernel_major": "252",
  "lv_kernel_minor": "16",
  "origin": "",
  "origin_size": "",
  "pool_lv": "",
  "lv_tags": "",
  "lv_attr": "rwi-aor---",
  "vg_name": "hdd-vg",
  "data_percent": "",
  "metadata_percent": ""
}
```

I suspect it might be due to the missing `C` in this rune: https://github.com/topolvm/topolvm/blob/00c6d96c10c9e476e4cbe076ff0fab63620b0a94/internal/lvmd/command/lvm_lv_attr.go#L32-L54

related docs: https://github.com/lvmteam/lvm2/blob/main/man/lvs.8_end#L6
related source: https://github.com/lvmteam/lvm2/blob/5ef958704c82c45a6bd8215d920e4366c0c5e1bd/lib/metadata/lv.c#L1333-L1334

happy to file a PR to add to the rune (?) + related tests, just let me know if that's desirable

Pull request:
support cached volumes, fixes #1003
* add `C` to VolumeType and OpenTarget consts
* `lv.IsThin` should check LV attrs for thin (e.g., cached LVs have a pool but are not necessarily thin)
* add tests for cached LVs

Patch:
diff --git a/internal/lvmd/command/lvm.go b/internal/lvmd/command/lvm.go
index 3a2fe627..82a38668 100644
--- a/internal/lvmd/command/lvm.go
+++ b/internal/lvmd/command/lvm.go
@@ -474,7 +474,7 @@ func (l *LogicalVolume) Origin(ctx context.Context) (*LogicalVolume, error) {
 
 // IsThin checks if the volume is thin volume or not.
 func (l *LogicalVolume) IsThin() bool {
-	return l.pool != nil
+	return l.attr[0] == byte(VolumeTypeThinVolume)
 }
 
 // Pool returns thin pool if this is a thin pool, or nil if not.
diff --git a/internal/lvmd/command/lvm_lv_attr.go b/internal/lvmd/command/lvm_lv_attr.go
index 81d2f2f6..4e2eee97 100644
--- a/internal/lvmd/command/lvm_lv_attr.go
+++ b/internal/lvmd/command/lvm_lv_attr.go
@@ -32,6 +32,7 @@ var (
 type VolumeType rune
 
 const (
+	VolumeTypeCached                     VolumeType = 'C'
 	VolumeTypeMirrored                   VolumeType = 'm'
 	VolumeTypeMirroredNoInitialSync      VolumeType = 'M'
 	VolumeTypeOrigin                     VolumeType = 'o'
@@ -114,6 +115,7 @@ const (
 type OpenTarget rune
 
 const (
+	OpenTargetCache    OpenTarget = 'C'
 	OpenTargetMirror   OpenTarget = 'm'
 	OpenTargetRaid     OpenTarget = 'r'
 	OpenTargetSnapshot OpenTarget = 's'


Test patch:
diff --git a/internal/lvmd/command/lvm_command_test.go b/internal/lvmd/command/lvm_command_test.go
index be3b7f00..3444b9ca 100644
--- a/internal/lvmd/command/lvm_command_test.go
+++ b/internal/lvmd/command/lvm_command_test.go
@@ -192,5 +192,39 @@ func Test_lvm_command(t *testing.T) {
 				t.Fatalf("expected error to be %v, got %v", ErrNoMultipleOfSectorSize, err)
 			}
 		})
+
+		t.Run("create cached volume and it should not classified as thin volume.", func(t *testing.T) {
+			// create the cachedevice
+			cache_vg_name := "CACHEDEVICE"
+			cache_loop, err := testutils.MakeLoopbackDevice(ctx, cache_vg_name)
+			if err != nil {
+				t.Fatal(err)
+			}
+			err = testutils.MakeLoopbackVG(ctx, cache_vg_name, cache_loop)
+			if err != nil {
+				t.Fatal(err)
+			}
+			// ensure cache device cleanup
+			defer func() { _ = testutils.CleanLoopbackVG(cache_vg_name, []string{cache_loop}, []string{cache_vg_name}) }()
+			vg, err := FindVolumeGroup(ctx, cache_vg_name)
+			if err != nil {
+				t.Fatal(err)
+			}
+			// create cached LV
+			err = vg.CreateVolume(ctx, "test1", uint64(topolvm.MinimumSectorSize), []string{"tag"}, 0, "", []string{"--type", "writecache", "--cachesize", "10M", "--cachedevice", cache_loop})
+			if err != nil {
+				t.Fatal(err)
+			}
+			vol, err := vg.FindVolume(ctx, "test1")
+			if err != nil {
+				t.Fatal(err)
+			}
+			if vol.IsThin() {
+				t.Fatal("Expected test1 to not be thin (eval lv_attr instead of pool)")
+			}
+			if vol.attr[0] != byte(VolumeTypeCached) {
+				t.Fatal("Created a LV but without writecache?")
+			}
+		})
 	})
 }
diff --git a/internal/lvmd/command/lvm_lv_attr_test.go b/internal/lvmd/command/lvm_lv_attr_test.go
index 04c0c47b..42518dc2 100644
--- a/internal/lvmd/command/lvm_lv_attr_test.go
+++ b/internal/lvmd/command/lvm_lv_attr_test.go
@@ -81,6 +81,23 @@ func TestParsedLVAttr(t *testing.T) {
 			},
 			noError,
 		},
+		{
+			"Cache",
+			args{raw: "Cwi-aoC---"},
+			LVAttr{
+				VolumeType:       VolumeTypeCached,
+				Permissions:      PermissionsWriteable,
+				AllocationPolicy: AllocationPolicyInherited,
+				Minor:            MinorFalse,
+				State:            StateActive,
+				Open:             OpenTrue,
+				OpenTarget:       OpenTargetCache,
+				Zero:             ZeroFalse,
+				VolumeHealth:     VolumeHealthOK,
+				SkipActivation:   SkipActivationFalse,
+			},
+			noError,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {