From 52f83349d84cf42802960284a88f0c6a1eff502b Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Sun, 12 Jul 2026 22:48:35 -0400 Subject: [PATCH 1/4] Implement code for symlink recreation when /mnt/local-storage is wiped --- pkg/common/current_block_device_info.go | 21 ++-- pkg/common/device_link_handler.go | 23 ++++- pkg/common/device_link_handler_test.go | 25 ++++- pkg/common/provisioner_utils.go | 2 +- pkg/common/pv_link_cache_test.go | 102 ++++++++++++------- pkg/diskmaker/controllers/lv/reconcile.go | 2 +- pkg/diskmaker/controllers/lvset/reconcile.go | 2 +- 7 files changed, 123 insertions(+), 54 deletions(-) diff --git a/pkg/common/current_block_device_info.go b/pkg/common/current_block_device_info.go index dcf7409ba..626c8825e 100644 --- a/pkg/common/current_block_device_info.go +++ b/pkg/common/current_block_device_info.go @@ -59,11 +59,11 @@ type CurrentBlockDeviceInfo struct { // // example - /dev/disk/by-id/wwn-0x123432 // -// This function MUST be called only when there is no valid symlinks for `newSymlinkSourcePath` -// in `/mnt/local-storage/` and yet LSO MUST have created a PV for device pointed by `newSymlinkSourcePath` -// in previously. So this function is our last resort to find a valid symlink path for PV. -// Only return valid new SymlinkPath if currentLinkTarget doesn't resolve and user has asked -// for symlinks to be recreated. +// This function MUST be called only when there is no valid symlink for `newSymlinkSourcePath` +// in `/mnt/local-storage/` and yet LSO previously created a PV for the device pointed by +// `newSymlinkSourcePath`. It is our last resort to find the PV symlink path that should be +// recreated. Returns the recovered path only when the LVDL policy is PreferredLinkTarget and +// the PV symlink is missing or no longer points at status.currentLinkTarget. func (c CurrentBlockDeviceInfo) RecoverPVSymlinkPath(ctx context.Context, symlinkDir, newSymlinkSourcePath string, client client.Client) (string, error) { lvdls := c.lvdls if len(lvdls) > 1 { @@ -92,10 +92,13 @@ func (c CurrentBlockDeviceInfo) RecoverPVSymlinkPath(ctx context.Context, symlin return "", PolicyNotPreferredError{SymlinkSource: newSymlinkSourcePath, SymlinkDir: symlinkDir, LVDL: lvdl, PVSymlinkPath: symlinkPath} } - // check if currentLinkTarget resolves to a valid device, if yes then no need to do anything - resolvedCurrent, err := internal.FilePathEvalSymLinks(currentLinkTarget) - if err == nil { - return "", fmt.Errorf("currentSymlink %s still resolves to %s for %s", currentLinkTarget, resolvedCurrent, newSymlinkSourcePath) + // If the PV symlink still points at currentLinkTarget and that target is + // already the source we are recovering for, nothing is broken. + // When newSymlinkSourcePath differs (e.g. stale by-id / sibling fallback), + // keep recovering so PreferredLinkTarget can relink to the new preferred path. + linkTarget, err := internal.Readlink(symlinkPath) + if err == nil && linkTarget == currentLinkTarget && currentLinkTarget == newSymlinkSourcePath { + return "", fmt.Errorf("PV symlink %s still points to currentLinkTarget %s for %s", symlinkPath, currentLinkTarget, newSymlinkSourcePath) } if slices.Contains(validLinkTargets, newSymlinkSourcePath) { diff --git a/pkg/common/device_link_handler.go b/pkg/common/device_link_handler.go index 2371848f7..9d04a1b30 100644 --- a/pkg/common/device_link_handler.go +++ b/pkg/common/device_link_handler.go @@ -263,8 +263,9 @@ func (dl *DeviceLinkHandler) findOrCreateLVDL(ctx context.Context, pvName, names return existing, nil } -// RecreateSymlinkIfNeeded checks the LVDL policy and atomically recreates -// the symlink if policy is PreferredLinkTarget and currentLinkTarget != preferredLinkTarget. +// RecreateSymlinkIfNeeded atomically recreates the symlink at symLinkPath to point at +// the preferred by-id path. Callers should invoke this when HasMismatchingSymlink is true +// (preferred != current, or the PV symlink is missing under PreferredLinkTarget policy). // symLinkPath is the full path under /mnt/local-storage//. // Returns nil if no action is needed or action succeeded, error if action failed. // On error, it sets a failure OperatorCondition on the LVDL object. @@ -289,6 +290,12 @@ func (dl *DeviceLinkHandler) RecreateSymlinkIfNeeded(ctx context.Context, lvdl * } symLinkDir := filepath.Dir(symLinkPath) + // Ensure the parent directory exists (e.g. after an admin removed /mnt/local-storage/). + if err := os.MkdirAll(symLinkDir, 0755); err != nil { + msg := fmt.Sprintf("failed to create symlink dir %s: %v", symLinkDir, err) + condition := getCondition("MkdirFailed", msg, operatorv1.ConditionTrue) + return dl.updateStatus(ctx, lvdl, condition, blockDevice, preferredTarget, currentTarget, symLinkPath) + } entries, err := internal.FilePathGlob(symLinkDir + "/*") if err != nil { msg := fmt.Sprintf("failed to list symlink dir %s: %v", symLinkDir, err) @@ -441,7 +448,10 @@ func (dl *DeviceLinkHandler) setLVDLCondition(lvdl *v1.LocalVolumeDeviceLink, co return lvdl } -func HasMismatchingSymlink(lvdl *v1.LocalVolumeDeviceLink, blockDevice internal.BlockDevice) bool { +// HasMismatchingSymlink reports whether the PV symlink under /mnt/local-storage needs +// recreation under PreferredLinkTarget policy. That is true when the symlink is missing +// on disk, or when currentLinkTarget differs from the preferred by-id path. +func HasMismatchingSymlink(lvdl *v1.LocalVolumeDeviceLink, blockDevice internal.BlockDevice, symlinkPath string) bool { lvdlName := "" if lvdl != nil { lvdlName = lvdl.Name @@ -454,6 +464,13 @@ func HasMismatchingSymlink(lvdl *v1.LocalVolumeDeviceLink, blockDevice internal. return false } + if symlinkPath != "" { + if _, err := os.Lstat(symlinkPath); os.IsNotExist(err) { + klog.InfoS("PV symlink is missing; recreation needed", "symlinkPath", symlinkPath, "lvdl", lvdlName) + return true + } + } + preferredTarget, err := blockDevice.GetPathByID() if err != nil { klog.ErrorS(err, "error getting pathbyid for device", "device", blockDevice.Name) diff --git a/pkg/common/device_link_handler_test.go b/pkg/common/device_link_handler_test.go index 1ccad07ef..b6178ef9d 100644 --- a/pkg/common/device_link_handler_test.go +++ b/pkg/common/device_link_handler_test.go @@ -511,10 +511,15 @@ func assertSingleConditionReason(t *testing.T, lvdl *v1.LocalVolumeDeviceLink, r // TestHasMismatchingSymlink verifies the HasMismatchingSymlink helper for various policies and states. func TestHasMismatchingSymlink(t *testing.T) { + existingSymlink := filepath.Join(t.TempDir(), "existing-symlink") + assert.NoError(t, os.Symlink("/dev/null", existingSymlink)) + missingSymlink := filepath.Join(t.TempDir(), "missing-symlink") + testCases := []struct { name string lvdl *v1.LocalVolumeDeviceLink blockDevice internal.BlockDevice + symlinkPath string expected bool }{ { @@ -527,12 +532,14 @@ func TestHasMismatchingSymlink(t *testing.T) { name: "policy None", lvdl: newLVDLWithPolicy("pv", "ns", v1.DeviceLinkPolicyNone, "/current", "/preferred"), blockDevice: internal.BlockDevice{PathByID: "/preferred"}, + symlinkPath: missingSymlink, expected: false, }, { name: "policy CurrentLinkTarget", lvdl: newLVDLWithPolicy("pv", "ns", v1.DeviceLinkPolicyCurrentLinkTarget, "/current", "/preferred"), blockDevice: internal.BlockDevice{PathByID: "/preferred"}, + symlinkPath: missingSymlink, expected: false, }, { @@ -545,14 +552,30 @@ func TestHasMismatchingSymlink(t *testing.T) { name: "policy PreferredLinkTarget with matching targets", lvdl: newLVDLWithPolicy("pv", "ns", v1.DeviceLinkPolicyPreferredLinkTarget, "/same", "/same"), blockDevice: internal.BlockDevice{PathByID: "/same"}, + symlinkPath: existingSymlink, expected: false, }, { name: "policy PreferredLinkTarget with mismatching targets", lvdl: newLVDLWithPolicy("pv", "ns", v1.DeviceLinkPolicyPreferredLinkTarget, "/current", "/dev/disk/by-id/preferred"), blockDevice: internal.BlockDevice{KName: "sda", PathByID: "/dev/disk/by-id/preferred"}, + symlinkPath: existingSymlink, + expected: true, + }, + { + name: "policy PreferredLinkTarget with missing PV symlink", + lvdl: newLVDLWithPolicy("pv", "ns", v1.DeviceLinkPolicyPreferredLinkTarget, "/same", "/same"), + blockDevice: internal.BlockDevice{PathByID: "/same"}, + symlinkPath: missingSymlink, expected: true, }, + { + name: "policy PreferredLinkTarget with matching targets and empty symlink path", + lvdl: newLVDLWithPolicy("pv", "ns", v1.DeviceLinkPolicyPreferredLinkTarget, "/same", "/same"), + blockDevice: internal.BlockDevice{PathByID: "/same"}, + symlinkPath: "", + expected: false, + }, } for _, tc := range testCases { @@ -561,7 +584,7 @@ func TestHasMismatchingSymlink(t *testing.T) { internal.FilePathEvalSymLinks = func(path string) (string, error) { return "/dev/sda", nil } - assert.Equal(t, tc.expected, HasMismatchingSymlink(tc.lvdl, tc.blockDevice)) + assert.Equal(t, tc.expected, HasMismatchingSymlink(tc.lvdl, tc.blockDevice, tc.symlinkPath)) }) } } diff --git a/pkg/common/provisioner_utils.go b/pkg/common/provisioner_utils.go index a157229a5..27b647455 100644 --- a/pkg/common/provisioner_utils.go +++ b/pkg/common/provisioner_utils.go @@ -126,7 +126,7 @@ func SyncPVAndLVDL(ctx context.Context, args SyncPVAndLVDLArgs) error { // Symlink recreation must happen before PV creation because it fixes the // symlink that the PV will reference. RecreateSymlinkIfNeeded already // updates the LVDL status, so we skip ApplyStatus later. - requiresSymlinkRecreation := HasMismatchingSymlink(lvdl, args.BlockDevice) + requiresSymlinkRecreation := HasMismatchingSymlink(lvdl, args.BlockDevice, symLinkPath) if requiresSymlinkRecreation { if _, err := deviceHandler.RecreateSymlinkIfNeeded(ctx, lvdl, symLinkPath, args.BlockDevice); err != nil { return fmt.Errorf("error recreating symlink: %w", err) diff --git a/pkg/common/pv_link_cache_test.go b/pkg/common/pv_link_cache_test.go index 9d57caee0..0acb49aef 100644 --- a/pkg/common/pv_link_cache_test.go +++ b/pkg/common/pv_link_cache_test.go @@ -32,6 +32,7 @@ func TestCurrentBlockDeviceInfoRecoverPVSymlinkPath(t *testing.T) { name string setup func(t *testing.T, c *LocalVolumeDeviceLinkCache) (CurrentBlockDeviceInfo, string) symlinkEvalFunc func(path string) (string, error) + readlinkFunc func(path string) (string, error) apiObjects []client.Object wantPath string wantErr string @@ -73,35 +74,74 @@ func TestCurrentBlockDeviceInfoRecoverPVSymlinkPath(t *testing.T) { wantErr: "policy is not PreferredLinkTarget", }, { - name: "errors when current link still resolves", + name: "errors when PV symlink still points to currentLinkTarget", setup: func(t *testing.T, c *LocalVolumeDeviceLinkCache) (CurrentBlockDeviceInfo, string) { - lvdl := newCacheLVDL("pv-resolves", cacheLocalNode, "/tmp/current-resolves", v1api.DeviceLinkPolicyPreferredLinkTarget, sourcePath) + lvdl := newCacheLVDL("pv-healthy", cacheLocalNode, "/dev/disk/by-id/wwn-1234", v1api.DeviceLinkPolicyPreferredLinkTarget, sourcePath) + lvdl.Status.PersistentVolumeSymlinkPath = "/mnt/local-storage/sc-a/wwn-1234" c.addOrUpdateLVDL(lvdl) - info := c.localDeviceInfos[sourcePath] - return info, sourcePath + return c.localDeviceInfos[sourcePath], sourcePath }, - symlinkEvalFunc: func(path string) (string, error) { - if path == "/tmp/current-resolves" { - return "/dev/sda", nil + readlinkFunc: func(path string) (string, error) { + if path == "/mnt/local-storage/sc-a/wwn-1234" { + return "/dev/disk/by-id/wwn-1234", nil } return "", os.ErrNotExist }, - apiObjects: []client.Object{ - localPV("pv-resolves", "/mnt/local-storage/sc-a/current-resolves"), + wantErr: "PV symlink /mnt/local-storage/sc-a/wwn-1234 still points to currentLinkTarget", + }, + { + name: "returns symlink path when PV symlink points at current but preferred source changed", + setup: func(t *testing.T, c *LocalVolumeDeviceLinkCache) (CurrentBlockDeviceInfo, string) { + lvdl := newCacheLVDL("pv-sibling", cacheLocalNode, "/dev/disk/by-id/wwn-old", v1api.DeviceLinkPolicyPreferredLinkTarget, "/dev/disk/by-id/wwn-old", "/dev/disk/by-id/scsi-sibling") + lvdl.Status.PersistentVolumeSymlinkPath = "/mnt/local-storage/sc-a/old-symlink-name" + c.addOrUpdateLVDL(lvdl) + return c.localDeviceInfos["/dev/disk/by-id/wwn-old"], "/dev/disk/by-id/wwn-new" + }, + readlinkFunc: func(path string) (string, error) { + if path == "/mnt/local-storage/sc-a/old-symlink-name" { + return "/dev/disk/by-id/wwn-old", nil + } + return "", os.ErrNotExist + }, + wantPath: "/mnt/local-storage/sc-a/old-symlink-name", + }, + { + name: "returns symlink path when PV symlink is missing", + setup: func(t *testing.T, c *LocalVolumeDeviceLinkCache) (CurrentBlockDeviceInfo, string) { + lvdl := newCacheLVDL("pv-missing-link", cacheLocalNode, "/dev/disk/by-id/wwn-1234", v1api.DeviceLinkPolicyPreferredLinkTarget, sourcePath) + lvdl.Status.PersistentVolumeSymlinkPath = "/mnt/local-storage/sc-a/wwn-1234" + c.addOrUpdateLVDL(lvdl) + return c.localDeviceInfos[sourcePath], sourcePath + }, + readlinkFunc: func(path string) (string, error) { + return "", os.ErrNotExist + }, + wantPath: "/mnt/local-storage/sc-a/wwn-1234", + }, + { + name: "returns symlink path when PV symlink points elsewhere", + setup: func(t *testing.T, c *LocalVolumeDeviceLinkCache) (CurrentBlockDeviceInfo, string) { + lvdl := newCacheLVDL("pv-stale", cacheLocalNode, "/dev/disk/by-id/wwn-1234", v1api.DeviceLinkPolicyPreferredLinkTarget, sourcePath) + lvdl.Status.PersistentVolumeSymlinkPath = "/mnt/local-storage/sc-a/wwn-1234" + c.addOrUpdateLVDL(lvdl) + return c.localDeviceInfos[sourcePath], sourcePath }, - wantErr: "currentSymlink /tmp/current-resolves still resolves", + readlinkFunc: func(path string) (string, error) { + if path == "/mnt/local-storage/sc-a/wwn-1234" { + return "/dev/disk/by-id/old-stale", nil + } + return "", os.ErrNotExist + }, + wantPath: "/mnt/local-storage/sc-a/wwn-1234", }, { - name: "returns recomputed symlink path when preferred policy and unresolved current", + name: "returns recomputed symlink path when preferred policy", setup: func(t *testing.T, c *LocalVolumeDeviceLinkCache) (CurrentBlockDeviceInfo, string) { lvdl := newCacheLVDL("pv-success", cacheLocalNode, "/dev/disk/by-id/yyy", v1api.DeviceLinkPolicyPreferredLinkTarget, sourcePath) c.addOrUpdateLVDL(lvdl) info := c.localDeviceInfos[sourcePath] return info, sourcePath }, - symlinkEvalFunc: func(path string) (string, error) { - return "", os.ErrNotExist - }, apiObjects: []client.Object{ localPV("pv-success", "/mnt/local-storage/sc-a/xxx"), }, @@ -116,9 +156,6 @@ func TestCurrentBlockDeviceInfoRecoverPVSymlinkPath(t *testing.T) { c.addOrUpdateLVDL(lvdl) return c.localDeviceInfos["/dev/disk/by-id/wwn-other"], sourcePath }, - symlinkEvalFunc: func(path string) (string, error) { - return "", os.ErrNotExist - }, apiObjects: []client.Object{ localPV("pv-invalid-target", "/mnt/local-storage/sc-a/current-invalid"), }, @@ -132,9 +169,6 @@ func TestCurrentBlockDeviceInfoRecoverPVSymlinkPath(t *testing.T) { c.addOrUpdateLVDL(lvdl) return c.localDeviceInfos[sourcePath], sourcePath }, - symlinkEvalFunc: func(path string) (string, error) { - return "", os.ErrNotExist - }, // pvObjectsForInfo would imply basename "current-status"; status SymlinkPath basename wins in getLVDLAndSymlinkPath. wantPath: filepath.Join(symlinkDir, "from-lvdl-status"), }, @@ -146,9 +180,6 @@ func TestCurrentBlockDeviceInfoRecoverPVSymlinkPath(t *testing.T) { c.addOrUpdateLVDL(lvdl) return c.localDeviceInfos[sourcePath], sourcePath }, - symlinkEvalFunc: func(path string) (string, error) { - return "", os.ErrNotExist - }, apiObjects: []client.Object{ &corev1.PersistentVolume{ ObjectMeta: metav1.ObjectMeta{Name: "pv-local"}, @@ -169,9 +200,6 @@ func TestCurrentBlockDeviceInfoRecoverPVSymlinkPath(t *testing.T) { c.addOrUpdateLVDL(lvdl) return c.localDeviceInfos[sourcePath], sourcePath }, - symlinkEvalFunc: func(path string) (string, error) { - return "", os.ErrNotExist - }, apiObjects: []client.Object{ localPV("pv-local", "/mnt/local-storage/sc-a/from-pv-local"), }, @@ -185,9 +213,6 @@ func TestCurrentBlockDeviceInfoRecoverPVSymlinkPath(t *testing.T) { c.addOrUpdateLVDL(lvdl) return c.localDeviceInfos[sourcePath], sourcePath }, - symlinkEvalFunc: func(path string) (string, error) { - return "", os.ErrNotExist - }, wantErr: "error getting associated pv object pv-missing", }, { @@ -198,9 +223,6 @@ func TestCurrentBlockDeviceInfoRecoverPVSymlinkPath(t *testing.T) { c.addOrUpdateLVDL(lvdl) return c.localDeviceInfos[sourcePath], sourcePath }, - symlinkEvalFunc: func(path string) (string, error) { - return "", os.ErrNotExist - }, apiObjects: []client.Object{ localPV("pv-empty-local", ""), }, @@ -214,9 +236,6 @@ func TestCurrentBlockDeviceInfoRecoverPVSymlinkPath(t *testing.T) { c.addOrUpdateLVDL(lvdl) return c.localDeviceInfos[sourcePath], sourcePath }, - symlinkEvalFunc: func(path string) (string, error) { - return "", os.ErrNotExist - }, apiObjects: []client.Object{ csiPV("pv-csi", "example.com/csi", "volume-handle"), }, @@ -230,9 +249,6 @@ func TestCurrentBlockDeviceInfoRecoverPVSymlinkPath(t *testing.T) { c.addOrUpdateLVDL(lvdl) return c.localDeviceInfos[sourcePath], sourcePath }, - symlinkEvalFunc: func(path string) (string, error) { - return "", os.ErrNotExist - }, apiObjects: []client.Object{}, wantErr: "does not match expected symlink directory", }, @@ -241,8 +257,10 @@ func TestCurrentBlockDeviceInfoRecoverPVSymlinkPath(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { origEval := internal.FilePathEvalSymLinks + origReadlink := internal.Readlink t.Cleanup(func() { internal.FilePathEvalSymLinks = origEval + internal.Readlink = origReadlink }) if tc.symlinkEvalFunc != nil { @@ -252,6 +270,14 @@ func TestCurrentBlockDeviceInfoRecoverPVSymlinkPath(t *testing.T) { return "", os.ErrNotExist } } + if tc.readlinkFunc != nil { + internal.Readlink = tc.readlinkFunc + } else { + // Default: PV symlink missing so recovery can return the path. + internal.Readlink = func(path string) (string, error) { + return "", os.ErrNotExist + } + } cache := NewLocalVolumeDeviceLinkCache(nil, nil, cacheLocalNode) info, source := tc.setup(t, cache) diff --git a/pkg/diskmaker/controllers/lv/reconcile.go b/pkg/diskmaker/controllers/lv/reconcile.go index 6e988499b..0b23a5c1b 100644 --- a/pkg/diskmaker/controllers/lv/reconcile.go +++ b/pkg/diskmaker/controllers/lv/reconcile.go @@ -720,7 +720,7 @@ func (r *LocalVolumeReconciler) processRejectedDevicesForDeviceLinks(ctx context klog.ErrorS(err, "error finding lvdl", "lvdl", lvdlName) } var lvdlError error - if common.HasMismatchingSymlink(lvdl, blockDevice) { + if common.HasMismatchingSymlink(lvdl, blockDevice, symlinkPath) { _, lvdlError = r.deviceLinkHandler.RecreateSymlinkIfNeeded(ctx, lvdl, symlinkPath, blockDevice) } else { // it is possible that symlinkPath has become stale, in which case we must let RecreateSymlinkIfNeeded to fix it. diff --git a/pkg/diskmaker/controllers/lvset/reconcile.go b/pkg/diskmaker/controllers/lvset/reconcile.go index af654ef3c..b5f5023ec 100644 --- a/pkg/diskmaker/controllers/lvset/reconcile.go +++ b/pkg/diskmaker/controllers/lvset/reconcile.go @@ -380,7 +380,7 @@ func (r *LocalVolumeSetReconciler) processRejectedDevicesForDeviceLinks(ctx cont } var lvdlError error - if common.HasMismatchingSymlink(lvdl, blockDevice) { + if common.HasMismatchingSymlink(lvdl, blockDevice, symlinkPath) { // Also attempt symlink recreation for in-use devices with PreferredLinkTarget policy. _, lvdlError = r.deviceLinkHandler.RecreateSymlinkIfNeeded(ctx, lvdl, symlinkPath, blockDevice) } else { From 495ae3775030cfd04c5f037b1f77b7dc4af98beb Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Mon, 13 Jul 2026 00:24:45 -0400 Subject: [PATCH 2/4] Add e2e for symlink recreation --- test/e2e/localvolume_test.go | 47 +++++++++++++++ test/e2e/localvolumeset_test.go | 68 +++++++++++++++++++++ test/e2e/symlink_check.go | 104 ++++++++++++++++++++++++++++++-- 3 files changed, 215 insertions(+), 4 deletions(-) diff --git a/test/e2e/localvolume_test.go b/test/e2e/localvolume_test.go index 0a8696183..2967fb0a2 100644 --- a/test/e2e/localvolume_test.go +++ b/test/e2e/localvolume_test.go @@ -324,6 +324,53 @@ var _ = Describe("LocalVolume", Label("LocalVolume"), Ordered, func() { checkForSymlinks(namespace, nodeEnv, symLinkPath) }) }) + + Context("symlink restoration after wipe", Ordered, func() { + var tc *testContext + + BeforeAll(func() { + tc = &testContext{ + f: f, + namespace: namespace, + nodeEnv: nodeEnv, + } + + selectedNode := nodeEnv[0].node + tc.localVolume = getLocalVolume(selectedNode, selectedDisk.path, namespace) + tc.localVolume.Name = "test-local-disk-wipe" + tc.localVolume.Spec.StorageClassDevices[0].StorageClassName = "test-local-sc-wipe" + + Eventually(func(ctx context.Context) error { + f.Logf("creating localvolume for wipe test") + return f.Client.Create(ctx, tc.localVolume, nil) + }, time.Minute, time.Second*2).WithContext(context.Background()).ShouldNot(HaveOccurred(), "creating localvolume for wipe test") + + DeferCleanup(func() { tc.Cleanup() }) + }) + + It("restores symlinks for PreferredLinkTarget LVDLs after /mnt/local-storage wipe", func() { + err := waitForDaemonSet(f.KubeClient, namespace, nodedaemon.DiskMakerName, retryInterval, hourTimeout) + Expect(err).NotTo(HaveOccurred(), "waiting for diskmaker daemonset") + + tc.pvs = eventuallyFindPVs(f, tc.localVolume.Spec.StorageClassDevices[0].StorageClassName, 1) + + pvNames := make([]string, 0, len(tc.pvs)) + for _, pv := range tc.pvs { + pvNames = append(pvNames, pv.Name) + } + lvdls := eventuallyFindLVDLsForPVs(f, namespace, pvNames) + for i := range lvdls { + lvdl := updateLVDLPolicy(f, &lvdls[i], localv1.DeviceLinkPolicyPreferredLinkTarget) + waitForLVDLLinkTargets(f, lvdl, lvdl.Status.PreferredLinkTarget, lvdl.Status.PreferredLinkTarget) + } + + tc.pvs = eventuallyFindPVs(f, tc.localVolume.Spec.StorageClassDevices[0].StorageClassName, 1) + + f.Logf("TEST: symlink restoration after /mnt/local-storage wipe") + Expect(tc.pvs).To(HaveLen(1)) + tc.pvs[0] = verifySymlinkRestorationAfterWipe(tc, tc.pvs[0]) + }) + }) }) // --- Helper functions --- diff --git a/test/e2e/localvolumeset_test.go b/test/e2e/localvolumeset_test.go index e3066523b..4d47d8414 100644 --- a/test/e2e/localvolumeset_test.go +++ b/test/e2e/localvolumeset_test.go @@ -528,6 +528,74 @@ var _ = Describe("LocalVolumeSet", Label("LocalVolumeSet"), Ordered, func() { eventuallyFindAvailablePVs(f, tc.lvset.Spec.StorageClassName, tc.pvs) }) }) + + Context("symlink restoration after wipe", Ordered, func() { + var tc *testContext + + BeforeAll(func() { + tc = &testContext{ + f: f, + namespace: namespace, + nodeEnv: nodeEnv, + } + + sixtyGi := resource.MustParse("60G") + eightyGi := resource.MustParse("80G") + tc.lvset = &localv1alpha1.LocalVolumeSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "wipe-test-fs", + Namespace: namespace, + }, + Spec: localv1alpha1.LocalVolumeSetSpec{ + StorageClassName: "wipe-test-fs", + VolumeMode: localv1.PersistentVolumeFilesystem, + NodeSelector: &corev1.NodeSelector{NodeSelectorTerms: []corev1.NodeSelectorTerm{ + { + MatchExpressions: []corev1.NodeSelectorRequirement{ + { + Key: corev1.LabelHostname, + Operator: corev1.NodeSelectorOpIn, + Values: []string{nodeEnv[0].node.ObjectMeta.Labels[corev1.LabelHostname]}, + }, + }, + }, + }}, + DeviceInclusionSpec: &localv1alpha1.DeviceInclusionSpec{ + DeviceTypes: []localv1alpha1.DeviceType{localv1alpha1.RawDisk}, + MinSize: &sixtyGi, + MaxSize: &eightyGi, + }, + }, + } + tc.lvSets = append(tc.lvSets, tc.lvset) + + f.Logf("creating localvolumeset for wipe test %q", tc.lvset.GetName()) + err := f.Client.Create(context.TODO(), tc.lvset, nil) + Expect(err).NotTo(HaveOccurred(), "create localvolumeset for wipe test") + + DeferCleanup(func() { tc.Cleanup() }) + }) + + It("restores symlinks for PreferredLinkTarget LVDLs after /mnt/local-storage wipe", func() { + tc.pvs = eventuallyFindPVs(f, tc.lvset.Spec.StorageClassName, 1) + + pvNames := make([]string, 0, len(tc.pvs)) + for _, pv := range tc.pvs { + pvNames = append(pvNames, pv.Name) + } + lvdls := eventuallyFindLVDLsForPVs(f, namespace, pvNames) + for i := range lvdls { + lvdl := updateLVDLPolicy(f, &lvdls[i], localv1.DeviceLinkPolicyPreferredLinkTarget) + waitForLVDLLinkTargets(f, lvdl, lvdl.Status.PreferredLinkTarget, lvdl.Status.PreferredLinkTarget) + } + + tc.pvs = eventuallyFindPVs(f, tc.lvset.Spec.StorageClassName, 1) + + f.Logf("TEST: symlink restoration after /mnt/local-storage wipe for lvset") + Expect(tc.pvs).To(HaveLen(1)) + tc.pvs[0] = verifySymlinkRestorationAfterWipe(tc, tc.pvs[0]) + }) + }) }) func waitForLVSetAndOwnedPVsToDisappear(lvset *localv1alpha1.LocalVolumeSet) { diff --git a/test/e2e/symlink_check.go b/test/e2e/symlink_check.go index 9f31f91b0..14ecbc1dd 100644 --- a/test/e2e/symlink_check.go +++ b/test/e2e/symlink_check.go @@ -335,6 +335,70 @@ func verifySymlinkFallbackOnDisappearingLink( return pv } +// verifySymlinkRestorationAfterWipe deletes all symlinks under /mnt/local-storage +// on the PV's node, then verifies that LSO restores the symlink when the LVDL +// has PreferredLinkTarget policy. +// +// Precondition: the PV's LVDL must have policy=PreferredLinkTarget. +func verifySymlinkRestorationAfterWipe( + tc *testContext, + pv corev1.PersistentVolume, +) corev1.PersistentVolume { + nodeHostname := findNodeHostnameForPV(&pv) + + lvdl := eventuallyGetLVDL(tc.f, tc.namespace, pv.Name) + Expect(lvdl.Spec.Policy).To(Equal(localv1.DeviceLinkPolicyPreferredLinkTarget), + "precondition: LVDL policy must be PreferredLinkTarget") + preferredTarget := lvdl.Status.PreferredLinkTarget + Expect(preferredTarget).ToNot(BeEmpty(), + "precondition: LVDL PreferredLinkTarget must be set before wipe") + + tc.f.Logf("wipe test: wiping /mnt/local-storage on node %s", nodeHostname) + wipeLocalStorageSymlinks(tc.namespace, nodeHostname) + + tc.f.Logf("wipe test: waiting for symlink restoration %s -> %s", pv.Spec.Local.Path, preferredTarget) + eventuallyVerifyNodeSymlinkTarget(tc.namespace, nodeHostname, pv.Spec.Local.Path, preferredTarget) + + tc.f.Logf("wipe test: verifying LVDL state after symlink restoration") + lvdl = waitForLVDLLinkTargets(tc.f, lvdl, preferredTarget, preferredTarget) + Expect(lvdl.Spec.Policy).To(Equal(localv1.DeviceLinkPolicyPreferredLinkTarget), + "LVDL policy must remain PreferredLinkTarget after restoration") + + return pv +} + +func wipeLocalStorageSymlinks(namespace, nodeHostname string) { + f := framework.Global + f.Logf("wiping /mnt/local-storage symlinks on node %s", nodeHostname) + + job, err := newWipeLocalStorageJob(nodeHostname, namespace) + Expect(err).NotTo(HaveOccurred(), "could not create wipe job") + + createOrReplaceJob(namespace, job, fmt.Sprintf("creating wipe job on node: %q", nodeHostname)) + waitForJobCompletion(job, fmt.Sprintf("waiting for wipe job to complete: %q", job.GetName())) + job.TypeMeta.Kind = "Job" + eventuallyDelete(job) +} + +func newWipeLocalStorageJob(nodeHostname, namespace string) (*batchv1.Job, error) { + script := ` +set -eu +set -x +echo "wiping symlinks under /mnt/local-storage" +find /mnt/local-storage -type l -delete -print 2>/dev/null || true +echo "wipe complete" +` + return newNodeJob( + nodeHostname, + namespace, + fmt.Sprintf("wipe-symlinks-%s", nodeHostname), + "deletes all symlinks under /mnt/local-storage", + []string{"/bin/bash", "-c", script}, + &NodeJobOptions{ + ContainerRestartPolicy: corev1.RestartPolicyNever, + }) +} + func newCheckSymlinkTargetJob(nodeHostname, namespace, symlinkPath, expectedTarget string) (*batchv1.Job, error) { script := ` set -eu @@ -343,6 +407,7 @@ set -x actual_target="$(readlink "$SYMLINK_PATH")" [[ "$actual_target" == "$EXPECTED_TARGET" ]] ` + backoffLimit := int32(0) return newNodeJob( nodeHostname, namespace, @@ -350,6 +415,7 @@ actual_target="$(readlink "$SYMLINK_PATH")" "checks that a host symlink points to the expected target", []string{"/bin/bash", "-c", script}, &NodeJobOptions{ + JobBackoffLimit: &backoffLimit, ContainerRestartPolicy: corev1.RestartPolicyNever, Env: []corev1.EnvVar{ {Name: "SYMLINK_PATH", Value: symlinkPath}, @@ -358,12 +424,42 @@ actual_target="$(readlink "$SYMLINK_PATH")" }) } -func verifyNodeSymlinkTarget(namespace, nodeHostname, symlinkPath, expectedTarget string) { +// checkNodeSymlinkTarget runs a one-shot job that verifies the host symlink +// points at expectedTarget. Returns an error if the check job fails (e.g. the +// symlink is missing or points elsewhere), so callers can poll with Eventually. +func checkNodeSymlinkTarget(namespace, nodeHostname, symlinkPath, expectedTarget string) error { + f := framework.Global job, err := newCheckSymlinkTargetJob(nodeHostname, namespace, symlinkPath, expectedTarget) - Expect(err).NotTo(HaveOccurred(), "could not create symlink target check job") + if err != nil { + return err + } + createOrReplaceJob(namespace, job, fmt.Sprintf("checking symlink target on node %s", nodeHostname)) + + j := &batchv1.Job{} + Eventually(func(ctx context.Context) int32 { + if err := f.Client.Get(ctx, types.NamespacedName{Name: job.Name, Namespace: namespace}, j); err != nil { + return 0 + } + return j.Status.Succeeded + j.Status.Failed + }, time.Minute, time.Second*5).WithContext(context.Background()).Should(BeNumerically(">=", 1)) - createOrReplaceJob(namespace, job, fmt.Sprintf("creating symlink target check job on node: %q", nodeHostname)) - waitForJobCompletion(job, fmt.Sprintf("waiting for symlink target check job to complete: %q", job.GetName())) job.TypeMeta.Kind = "Job" eventuallyDelete(job) + + if j.Status.Succeeded == 0 { + return fmt.Errorf("symlink %s does not point to %s on node %s", symlinkPath, expectedTarget, nodeHostname) + } + return nil +} + +func verifyNodeSymlinkTarget(namespace, nodeHostname, symlinkPath, expectedTarget string) { + Expect(checkNodeSymlinkTarget(namespace, nodeHostname, symlinkPath, expectedTarget)).To(Succeed(), + "symlink %s on node %s should point to %s", symlinkPath, nodeHostname, expectedTarget) +} + +func eventuallyVerifyNodeSymlinkTarget(namespace, nodeHostname, symlinkPath, expectedTarget string) { + Eventually(func() error { + return checkNodeSymlinkTarget(namespace, nodeHostname, symlinkPath, expectedTarget) + }, time.Minute*8, time.Second*15).Should(Succeed(), + "waiting for symlink %s on node %s to point to %s", symlinkPath, nodeHostname, expectedTarget) } From c3fff76fed9662c725b02de3464c2c5985c77a06 Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Mon, 13 Jul 2026 00:35:28 -0400 Subject: [PATCH 3/4] Add separate e2e for without and with filesystem signature --- test/e2e/localvolume_test.go | 25 +++++++++---------- test/e2e/localvolumeset_test.go | 24 +++++++++--------- test/e2e/symlink_check.go | 43 +++++++++++++++++++++++++++++++++ 3 files changed, 66 insertions(+), 26 deletions(-) diff --git a/test/e2e/localvolume_test.go b/test/e2e/localvolume_test.go index 2967fb0a2..b82e16ce2 100644 --- a/test/e2e/localvolume_test.go +++ b/test/e2e/localvolume_test.go @@ -339,6 +339,7 @@ var _ = Describe("LocalVolume", Label("LocalVolume"), Ordered, func() { tc.localVolume = getLocalVolume(selectedNode, selectedDisk.path, namespace) tc.localVolume.Name = "test-local-disk-wipe" tc.localVolume.Spec.StorageClassDevices[0].StorageClassName = "test-local-sc-wipe" + tc.localVolume.Spec.StorageClassDevices[0].VolumeMode = localv1.PersistentVolumeFilesystem Eventually(func(ctx context.Context) error { f.Logf("creating localvolume for wipe test") @@ -348,27 +349,25 @@ var _ = Describe("LocalVolume", Label("LocalVolume"), Ordered, func() { DeferCleanup(func() { tc.Cleanup() }) }) - It("restores symlinks for PreferredLinkTarget LVDLs after /mnt/local-storage wipe", func() { + It("restores symlink when device has no filesystem signature", func() { err := waitForDaemonSet(f.KubeClient, namespace, nodedaemon.DiskMakerName, retryInterval, hourTimeout) Expect(err).NotTo(HaveOccurred(), "waiting for diskmaker daemonset") tc.pvs = eventuallyFindPVs(f, tc.localVolume.Spec.StorageClassDevices[0].StorageClassName, 1) + Expect(tc.pvs).To(HaveLen(1)) - pvNames := make([]string, 0, len(tc.pvs)) - for _, pv := range tc.pvs { - pvNames = append(pvNames, pv.Name) - } - lvdls := eventuallyFindLVDLsForPVs(f, namespace, pvNames) - for i := range lvdls { - lvdl := updateLVDLPolicy(f, &lvdls[i], localv1.DeviceLinkPolicyPreferredLinkTarget) - waitForLVDLLinkTargets(f, lvdl, lvdl.Status.PreferredLinkTarget, lvdl.Status.PreferredLinkTarget) - } + lvdl := eventuallyGetLVDL(f, namespace, tc.pvs[0].Name) + lvdl = updateLVDLPolicy(f, lvdl, localv1.DeviceLinkPolicyPreferredLinkTarget) + waitForLVDLLinkTargets(f, lvdl, lvdl.Status.PreferredLinkTarget, lvdl.Status.PreferredLinkTarget) - tc.pvs = eventuallyFindPVs(f, tc.localVolume.Spec.StorageClassDevices[0].StorageClassName, 1) + f.Logf("TEST: symlink restoration after wipe with no filesystem signature") + tc.pvs[0] = verifySymlinkRestorationAfterWipeNoFSSignature(tc, tc.pvs[0]) + }) - f.Logf("TEST: symlink restoration after /mnt/local-storage wipe") + It("restores symlink when device has a filesystem signature", func() { Expect(tc.pvs).To(HaveLen(1)) - tc.pvs[0] = verifySymlinkRestorationAfterWipe(tc, tc.pvs[0]) + f.Logf("TEST: symlink restoration after wipe with filesystem signature (rejected-device path)") + tc.pvs[0] = verifySymlinkRestorationAfterWipeWithFSSignature(tc, tc.pvs[0]) }) }) }) diff --git a/test/e2e/localvolumeset_test.go b/test/e2e/localvolumeset_test.go index 4d47d8414..bff64539f 100644 --- a/test/e2e/localvolumeset_test.go +++ b/test/e2e/localvolumeset_test.go @@ -576,24 +576,22 @@ var _ = Describe("LocalVolumeSet", Label("LocalVolumeSet"), Ordered, func() { DeferCleanup(func() { tc.Cleanup() }) }) - It("restores symlinks for PreferredLinkTarget LVDLs after /mnt/local-storage wipe", func() { + It("restores symlink when device has no filesystem signature", func() { tc.pvs = eventuallyFindPVs(f, tc.lvset.Spec.StorageClassName, 1) + Expect(tc.pvs).To(HaveLen(1)) - pvNames := make([]string, 0, len(tc.pvs)) - for _, pv := range tc.pvs { - pvNames = append(pvNames, pv.Name) - } - lvdls := eventuallyFindLVDLsForPVs(f, namespace, pvNames) - for i := range lvdls { - lvdl := updateLVDLPolicy(f, &lvdls[i], localv1.DeviceLinkPolicyPreferredLinkTarget) - waitForLVDLLinkTargets(f, lvdl, lvdl.Status.PreferredLinkTarget, lvdl.Status.PreferredLinkTarget) - } + lvdl := eventuallyGetLVDL(f, namespace, tc.pvs[0].Name) + lvdl = updateLVDLPolicy(f, lvdl, localv1.DeviceLinkPolicyPreferredLinkTarget) + waitForLVDLLinkTargets(f, lvdl, lvdl.Status.PreferredLinkTarget, lvdl.Status.PreferredLinkTarget) - tc.pvs = eventuallyFindPVs(f, tc.lvset.Spec.StorageClassName, 1) + f.Logf("TEST: symlink restoration after wipe with no filesystem signature for lvset") + tc.pvs[0] = verifySymlinkRestorationAfterWipeNoFSSignature(tc, tc.pvs[0]) + }) - f.Logf("TEST: symlink restoration after /mnt/local-storage wipe for lvset") + It("restores symlink when device has a filesystem signature", func() { Expect(tc.pvs).To(HaveLen(1)) - tc.pvs[0] = verifySymlinkRestorationAfterWipe(tc, tc.pvs[0]) + f.Logf("TEST: symlink restoration after wipe with filesystem signature for lvset (rejected-device path)") + tc.pvs[0] = verifySymlinkRestorationAfterWipeWithFSSignature(tc, tc.pvs[0]) }) }) }) diff --git a/test/e2e/symlink_check.go b/test/e2e/symlink_check.go index 14ecbc1dd..f8b67b25a 100644 --- a/test/e2e/symlink_check.go +++ b/test/e2e/symlink_check.go @@ -367,6 +367,49 @@ func verifySymlinkRestorationAfterWipe( return pv } +// verifySymlinkRestorationAfterWipeNoFSSignature covers the valid-device reconcile +// path: the PV has never been consumed, so LVDL.Status.FilesystemUUID is empty. +func verifySymlinkRestorationAfterWipeNoFSSignature( + tc *testContext, + pv corev1.PersistentVolume, +) corev1.PersistentVolume { + lvdl := eventuallyGetLVDL(tc.f, tc.namespace, pv.Name) + Expect(lvdl.Status.FilesystemUUID).To(BeEmpty(), + "precondition: LVDL FilesystemUUID must be empty when device has no filesystem signature") + return verifySymlinkRestorationAfterWipe(tc, pv) +} + +// verifySymlinkRestorationAfterWipeWithFSSignature covers the rejected-device +// reconcile path. It consumes the filesystem PV (so blkid reports an FS UUID +// and the device is in-use), wipes /mnt/local-storage while the volume is still +// Bound, verifies symlink restoration, then deletes the consumers. +// +// Do not release the PVC before wipe: Delete reclaim clears the filesystem +// signature and would send the device back through the valid-device path. +func verifySymlinkRestorationAfterWipeWithFSSignature( + tc *testContext, + pv corev1.PersistentVolume, +) corev1.PersistentVolume { + tc.f.Logf("wipe+fs test: consuming PV %q to establish filesystem signature", pv.Name) + pvc, job, pod := consumePV(tc.namespace, pv) + verifyLVDLFilesystemUUIDForPVs(tc.f, tc.namespace, []string{pv.Name}) + + lvdl := eventuallyGetLVDL(tc.f, tc.namespace, pv.Name) + Expect(lvdl.Status.FilesystemUUID).ToNot(BeEmpty(), + "precondition: LVDL FilesystemUUID must be set when device has a filesystem signature") + + tc.f.Logf("wipe+fs test: wiping while PV %q is still Bound", pv.Name) + pv = verifySymlinkRestorationAfterWipe(tc, pv) + + lvdl = eventuallyGetLVDL(tc.f, tc.namespace, pv.Name) + Expect(lvdl.Status.FilesystemUUID).ToNot(BeEmpty(), + "FilesystemUUID must remain set after wipe restoration while Bound") + + tc.f.Logf("wipe+fs test: deleting consumers for PV %q", pv.Name) + eventuallyDelete(job, pvc, pod) + return pv +} + func wipeLocalStorageSymlinks(namespace, nodeHostname string) { f := framework.Global f.Logf("wiping /mnt/local-storage symlinks on node %s", nodeHostname) From 07a8638dfb9fe46ab1481e40febab00f430809ae Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Tue, 14 Jul 2026 11:57:05 -0400 Subject: [PATCH 4/4] Address review comments --- pkg/common/current_block_device_info.go | 9 +- pkg/common/device_link_handler.go | 25 ++-- pkg/common/device_link_handler_test.go | 9 +- pkg/common/provisioner_utils.go | 24 ++-- pkg/common/pv_link_cache_test.go | 13 ++ .../controllers/lv/create_pv_test.go | 129 ++++++++++++++++++ pkg/diskmaker/controllers/lv/reconcile.go | 7 +- pkg/diskmaker/controllers/lvset/reconcile.go | 7 +- test/e2e/symlink_check.go | 77 +++++++++-- 9 files changed, 266 insertions(+), 34 deletions(-) diff --git a/pkg/common/current_block_device_info.go b/pkg/common/current_block_device_info.go index 626c8825e..44f3ebdd2 100644 --- a/pkg/common/current_block_device_info.go +++ b/pkg/common/current_block_device_info.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "os" "path/filepath" "slices" @@ -96,8 +97,14 @@ func (c CurrentBlockDeviceInfo) RecoverPVSymlinkPath(ctx context.Context, symlin // already the source we are recovering for, nothing is broken. // When newSymlinkSourcePath differs (e.g. stale by-id / sibling fallback), // keep recovering so PreferredLinkTarget can relink to the new preferred path. + // Only treat a missing symlink (or a successfully read alternate target) as + // recoverable; permission/I/O/non-symlink failures must surface. linkTarget, err := internal.Readlink(symlinkPath) - if err == nil && linkTarget == currentLinkTarget && currentLinkTarget == newSymlinkSourcePath { + if err != nil { + if !os.IsNotExist(err) { + return "", fmt.Errorf("error reading PV symlink %s: %w", symlinkPath, err) + } + } else if linkTarget == currentLinkTarget && currentLinkTarget == newSymlinkSourcePath { return "", fmt.Errorf("PV symlink %s still points to currentLinkTarget %s for %s", symlinkPath, currentLinkTarget, newSymlinkSourcePath) } diff --git a/pkg/common/device_link_handler.go b/pkg/common/device_link_handler.go index 9d04a1b30..52a0dd180 100644 --- a/pkg/common/device_link_handler.go +++ b/pkg/common/device_link_handler.go @@ -451,43 +451,48 @@ func (dl *DeviceLinkHandler) setLVDLCondition(lvdl *v1.LocalVolumeDeviceLink, co // HasMismatchingSymlink reports whether the PV symlink under /mnt/local-storage needs // recreation under PreferredLinkTarget policy. That is true when the symlink is missing // on disk, or when currentLinkTarget differs from the preferred by-id path. -func HasMismatchingSymlink(lvdl *v1.LocalVolumeDeviceLink, blockDevice internal.BlockDevice, symlinkPath string) bool { +// Unexpected Lstat failures (permission, I/O, etc.) are returned so callers can +// requeue instead of treating the link as present. +func HasMismatchingSymlink(lvdl *v1.LocalVolumeDeviceLink, blockDevice internal.BlockDevice, symlinkPath string) (bool, error) { lvdlName := "" if lvdl != nil { lvdlName = lvdl.Name } klog.V(4).Infof("checking for mismatching symlinks, lvdl %s", lvdlName) if lvdl == nil { - return false + return false, nil } if lvdl.Spec.Policy != v1.DeviceLinkPolicyPreferredLinkTarget { - return false + return false, nil } if symlinkPath != "" { - if _, err := os.Lstat(symlinkPath); os.IsNotExist(err) { - klog.InfoS("PV symlink is missing; recreation needed", "symlinkPath", symlinkPath, "lvdl", lvdlName) - return true + if _, err := os.Lstat(symlinkPath); err != nil { + if os.IsNotExist(err) { + klog.InfoS("PV symlink is missing; recreation needed", "symlinkPath", symlinkPath, "lvdl", lvdlName) + return true, nil + } + return false, fmt.Errorf("error checking PV symlink %s: %w", symlinkPath, err) } } preferredTarget, err := blockDevice.GetPathByID() if err != nil { klog.ErrorS(err, "error getting pathbyid for device", "device", blockDevice.Name) - return false + return false, nil } currentTarget := lvdl.Status.CurrentLinkTarget klog.Infof("checking for mismatching symlinks current: %s, preferred: %s", currentTarget, preferredTarget) if preferredTarget == "" { - return false + return false, nil } if currentTarget == preferredTarget { - return false + return false, nil } - return true + return true, nil } func getCondition(reason, msg string, status operatorv1.ConditionStatus) operatorv1.OperatorCondition { diff --git a/pkg/common/device_link_handler_test.go b/pkg/common/device_link_handler_test.go index b6178ef9d..da2c3b05c 100644 --- a/pkg/common/device_link_handler_test.go +++ b/pkg/common/device_link_handler_test.go @@ -584,11 +584,18 @@ func TestHasMismatchingSymlink(t *testing.T) { internal.FilePathEvalSymLinks = func(path string) (string, error) { return "/dev/sda", nil } - assert.Equal(t, tc.expected, HasMismatchingSymlink(tc.lvdl, tc.blockDevice, tc.symlinkPath)) + assert.Equal(t, tc.expected, mustHasMismatchingSymlink(t, tc.lvdl, tc.blockDevice, tc.symlinkPath)) }) } } +func mustHasMismatchingSymlink(t *testing.T, lvdl *v1.LocalVolumeDeviceLink, blockDevice internal.BlockDevice, symlinkPath string) bool { + t.Helper() + got, err := HasMismatchingSymlink(lvdl, blockDevice, symlinkPath) + assert.NoError(t, err) + return got +} + func TestRecreateSymlinkIfNeeded(t *testing.T) { type recreateTestCase struct { name string diff --git a/pkg/common/provisioner_utils.go b/pkg/common/provisioner_utils.go index 27b647455..a8960aa79 100644 --- a/pkg/common/provisioner_utils.go +++ b/pkg/common/provisioner_utils.go @@ -123,10 +123,22 @@ func SyncPVAndLVDL(ctx context.Context, args SyncPVAndLVDLArgs) error { return fmt.Errorf("error finding localvolumedevicelink object %s: %w", pvName, err) } + // Do not recreate symlinks or update LVDL/PV state for released volumes. + existingPV := &corev1.PersistentVolume{ObjectMeta: metav1.ObjectMeta{Name: pvName}} + err = client.Get(ctx, types.NamespacedName{Name: pvName}, existingPV) + if err == nil && existingPV.Status.Phase == corev1.VolumeReleased { + klog.InfoS("PV is still being cleaned, not going to recreate it", "pvName", pvName, "disk", deviceName) + // Caller should try again soon + return ErrTryAgain + } + // Symlink recreation must happen before PV creation because it fixes the // symlink that the PV will reference. RecreateSymlinkIfNeeded already // updates the LVDL status, so we skip ApplyStatus later. - requiresSymlinkRecreation := HasMismatchingSymlink(lvdl, args.BlockDevice, symLinkPath) + requiresSymlinkRecreation, err := HasMismatchingSymlink(lvdl, args.BlockDevice, symLinkPath) + if err != nil { + return fmt.Errorf("error checking symlink mismatch: %w", err) + } if requiresSymlinkRecreation { if _, err := deviceHandler.RecreateSymlinkIfNeeded(ctx, lvdl, symLinkPath, args.BlockDevice); err != nil { return fmt.Errorf("error recreating symlink: %w", err) @@ -154,16 +166,6 @@ func SyncPVAndLVDL(ctx context.Context, args SyncPVAndLVDLArgs) error { return fmt.Errorf("could not read the device's volume mode from the node: %w", err) } - // Do not attempt to create or update existing PV's that have been released - existingPV := &corev1.PersistentVolume{ObjectMeta: metav1.ObjectMeta{Name: pvName}} - err = client.Get(ctx, types.NamespacedName{Name: pvName}, existingPV) - - if err == nil && existingPV.Status.Phase == corev1.VolumeReleased { - klog.InfoS("PV is still being cleaned, not going to recreate it", "pvName", pvName, "disk", deviceName) - // Caller should try again soon - return ErrTryAgain - } - var capacityBytes int64 switch actualVolumeMode { case corev1.PersistentVolumeBlock: diff --git a/pkg/common/pv_link_cache_test.go b/pkg/common/pv_link_cache_test.go index 0acb49aef..ed1656002 100644 --- a/pkg/common/pv_link_cache_test.go +++ b/pkg/common/pv_link_cache_test.go @@ -118,6 +118,19 @@ func TestCurrentBlockDeviceInfoRecoverPVSymlinkPath(t *testing.T) { }, wantPath: "/mnt/local-storage/sc-a/wwn-1234", }, + { + name: "errors when reading PV symlink fails for a non-missing reason", + setup: func(t *testing.T, c *LocalVolumeDeviceLinkCache) (CurrentBlockDeviceInfo, string) { + lvdl := newCacheLVDL("pv-read-error", cacheLocalNode, "/dev/disk/by-id/wwn-1234", v1api.DeviceLinkPolicyPreferredLinkTarget, sourcePath) + lvdl.Status.PersistentVolumeSymlinkPath = "/mnt/local-storage/sc-a/wwn-1234" + c.addOrUpdateLVDL(lvdl) + return c.localDeviceInfos[sourcePath], sourcePath + }, + readlinkFunc: func(path string) (string, error) { + return "", os.ErrPermission + }, + wantErr: "error reading PV symlink /mnt/local-storage/sc-a/wwn-1234", + }, { name: "returns symlink path when PV symlink points elsewhere", setup: func(t *testing.T, c *LocalVolumeDeviceLinkCache) (CurrentBlockDeviceInfo, string) { diff --git a/pkg/diskmaker/controllers/lv/create_pv_test.go b/pkg/diskmaker/controllers/lv/create_pv_test.go index 3bcc156fb..1ed2ef9fd 100644 --- a/pkg/diskmaker/controllers/lv/create_pv_test.go +++ b/pkg/diskmaker/controllers/lv/create_pv_test.go @@ -580,3 +580,132 @@ func TestSyncPVAndLVDL_DeviceLinkLifecycle(t *testing.T) { }) } } + +// TestSyncPVAndLVDL_SkipsReleasedPV verifies that SyncPVAndLVDL returns +// ErrTryAgain for a Released PV and does not recreate the symlink or update +// LVDL/PV state while cleanup is still in progress. +func TestSyncPVAndLVDL_SkipsReleasedPV(t *testing.T) { + reclaimPolicyDelete := corev1.PersistentVolumeReclaimDelete + fakeByIDLink := "/dev/disk/by-id/wwn-preferred" + + tmpDir := diskmakertest.TempDir(t, "create-local-pv-released-") + + lv := localv1.LocalVolume{ + TypeMeta: metav1.TypeMeta{ + Kind: localv1.LocalVolumeKind, + APIVersion: localv1.GroupVersion.String(), + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "lv-released", + Namespace: "openshift-local-storage", + UID: "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee", + }, + } + node := corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node-released", + Labels: map[string]string{corev1.LabelHostname: "node-hostname-released"}, + }, + } + sc := storagev1.StorageClass{ + ObjectMeta: metav1.ObjectMeta{Name: "sc-released"}, + ReclaimPolicy: &reclaimPolicyDelete, + } + + symLinkPath := filepath.Join(tmpDir, sc.Name, "claimed") + pvName := common.GeneratePVName(filepath.Base(symLinkPath), node.Name, sc.Name) + currentTarget := "/dev/disk/by-id/wwn-current" + + assert.NoError(t, os.MkdirAll(filepath.Dir(symLinkPath), 0755)) + assert.NoError(t, os.Symlink(currentTarget, symLinkPath)) + + releasedPV := &corev1.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{Name: pvName}, + Spec: corev1.PersistentVolumeSpec{ + PersistentVolumeReclaimPolicy: reclaimPolicyDelete, + PersistentVolumeSource: corev1.PersistentVolumeSource{ + Local: &corev1.LocalVolumeSource{Path: symLinkPath}, + }, + StorageClassName: sc.Name, + }, + Status: corev1.PersistentVolumeStatus{Phase: corev1.VolumeReleased}, + } + lvdl := &localv1.LocalVolumeDeviceLink{ + ObjectMeta: metav1.ObjectMeta{ + Name: pvName, + Namespace: lv.Namespace, + }, + Spec: localv1.LocalVolumeDeviceLinkSpec{ + PersistentVolumeName: pvName, + NodeName: node.Name, + Policy: localv1.DeviceLinkPolicyPreferredLinkTarget, + }, + Status: localv1.LocalVolumeDeviceLinkStatus{ + CurrentLinkTarget: currentTarget, + PreferredLinkTarget: fakeByIDLink, + PersistentVolumeSymlinkPath: symLinkPath, + }, + } + + r, testConfig := getFakeDiskMaker(t, tmpDir, &lv, &node, &sc, releasedPV, lvdl) + testConfig.runtimeConfig.Node = &node + testConfig.runtimeConfig.Name = common.GetProvisionedByValue(node) + testConfig.runtimeConfig.Namespace = lv.Namespace + testConfig.runtimeConfig.DiscoveryMap[sc.Name] = provCommon.MountConfig{ + VolumeMode: string(localv1.PersistentVolumeBlock), + } + testConfig.fakeVolUtil.AddNewDirEntries(tmpDir, map[string][]*provUtil.FakeDirEntry{ + sc.Name: { + {Name: filepath.Base(symLinkPath), Capacity: 10 * common.GiB, VolumeType: provUtil.FakeEntryBlock}, + }, + }) + + diskmakertest.WithInternalMocks(t, func() { + internal.FilePathGlob = func(pattern string) ([]string, error) { + if pattern == filepath.Join(internal.DiskByIDDir, "*") { + return []string{fakeByIDLink}, nil + } + return filepath.Glob(pattern) + } + internal.FilePathEvalSymLinks = func(path string) (string, error) { + if path == fakeByIDLink || path == currentTarget { + return "/dev/null", nil + } + return filepath.EvalSymlinks(path) + } + }) + + err := common.SyncPVAndLVDL(t.Context(), common.SyncPVAndLVDLArgs{ + LocalVolumeLikeObject: &lv, + RuntimeConfig: r.runtimeConfig, + StorageClass: sc, + MountPointMap: sets.New[string](), + Client: r.Client, + ClientReader: r.ClientReader, + SymLinkPath: symLinkPath, + BlockDevice: internal.BlockDevice{ + Name: "null", + KName: "null", + PathByID: fakeByIDLink, + }, + CacheWriter: r.pvLinkCache, + ExtraLabelsForPV: map[string]string{}, + }) + assert.ErrorIs(t, err, common.ErrTryAgain) + + target, readErr := os.Readlink(symLinkPath) + assert.NoError(t, readErr) + assert.Equal(t, currentTarget, target, + "symlink must not be recreated while PV is Released") + + gotLVDL := &localv1.LocalVolumeDeviceLink{} + assert.NoError(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: pvName, Namespace: lv.Namespace}, gotLVDL)) + assert.Equal(t, currentTarget, gotLVDL.Status.CurrentLinkTarget, + "LVDL CurrentLinkTarget must not change while PV is Released") + assert.Equal(t, fakeByIDLink, gotLVDL.Status.PreferredLinkTarget) + assert.Equal(t, localv1.DeviceLinkPolicyPreferredLinkTarget, gotLVDL.Spec.Policy) + + gotPV := &corev1.PersistentVolume{} + assert.NoError(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: pvName}, gotPV)) + assert.Equal(t, corev1.VolumeReleased, gotPV.Status.Phase) +} diff --git a/pkg/diskmaker/controllers/lv/reconcile.go b/pkg/diskmaker/controllers/lv/reconcile.go index 0b23a5c1b..dcfba1b60 100644 --- a/pkg/diskmaker/controllers/lv/reconcile.go +++ b/pkg/diskmaker/controllers/lv/reconcile.go @@ -720,7 +720,12 @@ func (r *LocalVolumeReconciler) processRejectedDevicesForDeviceLinks(ctx context klog.ErrorS(err, "error finding lvdl", "lvdl", lvdlName) } var lvdlError error - if common.HasMismatchingSymlink(lvdl, blockDevice, symlinkPath) { + requiresSymlinkRecreation, mismatchErr := common.HasMismatchingSymlink(lvdl, blockDevice, symlinkPath) + if mismatchErr != nil { + klog.ErrorS(mismatchErr, "error checking symlink mismatch", "device", blockDevice.Name, "symlinkPath", symlinkPath) + continue + } + if requiresSymlinkRecreation { _, lvdlError = r.deviceLinkHandler.RecreateSymlinkIfNeeded(ctx, lvdl, symlinkPath, blockDevice) } else { // it is possible that symlinkPath has become stale, in which case we must let RecreateSymlinkIfNeeded to fix it. diff --git a/pkg/diskmaker/controllers/lvset/reconcile.go b/pkg/diskmaker/controllers/lvset/reconcile.go index b5f5023ec..5518d1759 100644 --- a/pkg/diskmaker/controllers/lvset/reconcile.go +++ b/pkg/diskmaker/controllers/lvset/reconcile.go @@ -380,7 +380,12 @@ func (r *LocalVolumeSetReconciler) processRejectedDevicesForDeviceLinks(ctx cont } var lvdlError error - if common.HasMismatchingSymlink(lvdl, blockDevice, symlinkPath) { + requiresSymlinkRecreation, mismatchErr := common.HasMismatchingSymlink(lvdl, blockDevice, symlinkPath) + if mismatchErr != nil { + klog.ErrorS(mismatchErr, "error checking symlink mismatch", "device", blockDevice.Name, "symlinkPath", symlinkPath) + continue + } + if requiresSymlinkRecreation { // Also attempt symlink recreation for in-use devices with PreferredLinkTarget policy. _, lvdlError = r.deviceLinkHandler.RecreateSymlinkIfNeeded(ctx, lvdl, symlinkPath, blockDevice) } else { diff --git a/test/e2e/symlink_check.go b/test/e2e/symlink_check.go index f8b67b25a..535e8ee04 100644 --- a/test/e2e/symlink_check.go +++ b/test/e2e/symlink_check.go @@ -340,6 +340,10 @@ func verifySymlinkFallbackOnDisappearingLink( // has PreferredLinkTarget policy. // // Precondition: the PV's LVDL must have policy=PreferredLinkTarget. +// +// A DeferCleanup restores the PV symlink if the test fails or times out after +// the wipe; otherwise diskmaker's PV deleter cannot resolve the device and +// LocalVolume(Set) cleanup hangs on the protection finalizer. func verifySymlinkRestorationAfterWipe( tc *testContext, pv corev1.PersistentVolume, @@ -353,11 +357,18 @@ func verifySymlinkRestorationAfterWipe( Expect(preferredTarget).ToNot(BeEmpty(), "precondition: LVDL PreferredLinkTarget must be set before wipe") + symlinkPath := pv.Spec.Local.Path + DeferCleanup(func() { + tc.f.Logf("cleanup: ensuring PV symlink %s -> %s exists on node %s after wipe test", + symlinkPath, preferredTarget, nodeHostname) + restoreLocalStorageSymlink(tc.namespace, nodeHostname, symlinkPath, preferredTarget) + }) + tc.f.Logf("wipe test: wiping /mnt/local-storage on node %s", nodeHostname) - wipeLocalStorageSymlinks(tc.namespace, nodeHostname) + wipeLocalStorageSymlinks(tc.namespace, nodeHostname, symlinkPath) - tc.f.Logf("wipe test: waiting for symlink restoration %s -> %s", pv.Spec.Local.Path, preferredTarget) - eventuallyVerifyNodeSymlinkTarget(tc.namespace, nodeHostname, pv.Spec.Local.Path, preferredTarget) + tc.f.Logf("wipe test: waiting for symlink restoration %s -> %s", symlinkPath, preferredTarget) + eventuallyVerifyNodeSymlinkTarget(tc.namespace, nodeHostname, symlinkPath, preferredTarget) tc.f.Logf("wipe test: verifying LVDL state after symlink restoration") lvdl = waitForLVDLLinkTargets(tc.f, lvdl, preferredTarget, preferredTarget) @@ -410,11 +421,11 @@ func verifySymlinkRestorationAfterWipeWithFSSignature( return pv } -func wipeLocalStorageSymlinks(namespace, nodeHostname string) { +func wipeLocalStorageSymlinks(namespace, nodeHostname, symlinkPath string) { f := framework.Global - f.Logf("wiping /mnt/local-storage symlinks on node %s", nodeHostname) + f.Logf("wiping /mnt/local-storage symlinks on node %s (expecting %s present)", nodeHostname, symlinkPath) - job, err := newWipeLocalStorageJob(nodeHostname, namespace) + job, err := newWipeLocalStorageJob(nodeHostname, namespace, symlinkPath) Expect(err).NotTo(HaveOccurred(), "could not create wipe job") createOrReplaceJob(namespace, job, fmt.Sprintf("creating wipe job on node: %q", nodeHostname)) @@ -423,12 +434,30 @@ func wipeLocalStorageSymlinks(namespace, nodeHostname string) { eventuallyDelete(job) } -func newWipeLocalStorageJob(nodeHostname, namespace string) (*batchv1.Job, error) { +// restoreLocalStorageSymlink recreates a /mnt/local-storage PV symlink on the +// node. Used by DeferCleanup after wipe tests so PV deletion can still resolve +// the device path if LSO did not restore the link in time. +func restoreLocalStorageSymlink(namespace, nodeHostname, symlinkPath, target string) { + f := framework.Global + f.Logf("restoring PV symlink %s -> %s on node %s", symlinkPath, target, nodeHostname) + + job, err := newRestoreLocalStorageSymlinkJob(nodeHostname, namespace, symlinkPath, target) + Expect(err).NotTo(HaveOccurred(), "could not create restore symlink job") + + createOrReplaceJob(namespace, job, fmt.Sprintf("creating restore symlink job on node: %q", nodeHostname)) + waitForJobCompletion(job, fmt.Sprintf("waiting for restore symlink job to complete: %q", job.GetName())) + job.TypeMeta.Kind = "Job" + eventuallyDelete(job) +} + +func newWipeLocalStorageJob(nodeHostname, namespace, symlinkPath string) (*batchv1.Job, error) { script := ` set -eu set -x +echo "verifying PV symlink exists before wipe: $SYMLINK_PATH" +test -L "$SYMLINK_PATH" echo "wiping symlinks under /mnt/local-storage" -find /mnt/local-storage -type l -delete -print 2>/dev/null || true +find /mnt/local-storage -type l -delete -print echo "wipe complete" ` return newNodeJob( @@ -439,6 +468,34 @@ echo "wipe complete" []string{"/bin/bash", "-c", script}, &NodeJobOptions{ ContainerRestartPolicy: corev1.RestartPolicyNever, + Env: []corev1.EnvVar{ + {Name: "SYMLINK_PATH", Value: symlinkPath}, + }, + }) +} + +func newRestoreLocalStorageSymlinkJob(nodeHostname, namespace, symlinkPath, target string) (*batchv1.Job, error) { + script := ` +set -eu +set -x +echo "restoring PV symlink $SYMLINK_PATH -> $TARGET" +mkdir -p "$(dirname "$SYMLINK_PATH")" +ln -sfn "$TARGET" "$SYMLINK_PATH" +actual_target="$(readlink "$SYMLINK_PATH")" +[[ "$actual_target" == "$TARGET" ]] +` + return newNodeJob( + nodeHostname, + namespace, + fmt.Sprintf("restore-symlink-%s", nodeHostname), + "restores a PV symlink under /mnt/local-storage after wipe tests", + []string{"/bin/bash", "-c", script}, + &NodeJobOptions{ + ContainerRestartPolicy: corev1.RestartPolicyNever, + Env: []corev1.EnvVar{ + {Name: "SYMLINK_PATH", Value: symlinkPath}, + {Name: "TARGET", Value: target}, + }, }) } @@ -484,7 +541,9 @@ func checkNodeSymlinkTarget(namespace, nodeHostname, symlinkPath, expectedTarget return 0 } return j.Status.Succeeded + j.Status.Failed - }, time.Minute, time.Second*5).WithContext(context.Background()).Should(BeNumerically(">=", 1)) + }, time.Minute, time.Second*5).WithContext(context.Background()). + Should(BeNumerically(">=", 1), + "check symlink job %q on node %q for path %q did not complete", job.Name, nodeHostname, symlinkPath) job.TypeMeta.Kind = "Job" eventuallyDelete(job)