diff --git a/CHANGELOG.md b/CHANGELOG.md index 4913decf3..4fdd6eaa2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,7 @@ - **Kubernetes:** Improved slow Qtree provisioning on large SVMs (Issue [#1142](https://github.com/NetApp/trident/issues/1142)). - **Kubernetes:** Fixed ControllerPublish to use volume config file system type to avoid using incorrect default file system type. - **Kubernetes:** Fixed an issue where orphan dm devices might cause repeated CSI NodeUnstageVolume failures. +- **Kubernetes:** Fixed NodeUnpublishVolume failing indefinitely when the target path could not be inspected, such as an NFS mount whose export policy no longer admits the node (Issue [#1184](https://github.com/NetApp/trident/issues/1184)). **Enhancements:** diff --git a/core/node/unmount.go b/core/node/unmount.go index 93622d291..5268f579e 100644 --- a/core/node/unmount.go +++ b/core/node/unmount.go @@ -69,7 +69,13 @@ func (c *Core) unmountGeneric(ctx context.Context, volume, targetPath string) er "target path (%s) not found; volume is not mounted.", targetPath) return nil } - return errors.InternalError("could not check if the target path (%s) is a directory; %v", targetPath, err) + // stat can fail on a path that is still mounted: EACCES when an NFS export rule no longer + // admits this node, ESTALE or EIO when the share has gone stale. None of these prevent + // umount(2). Consult the mount table, which does not touch the filesystem behind the + // target path, instead of failing the unpublish before it is attempted. + Logc(ctx).WithFields(LogFields{"targetPath": targetPath, "error": err}).Warning( + "Could not stat target path; checking the mount table instead.") + isDir = false } var notMountPoint bool diff --git a/core/node/unmount_test.go b/core/node/unmount_test.go index b521ca049..0605cd916 100644 --- a/core/node/unmount_test.go +++ b/core/node/unmount_test.go @@ -45,7 +45,7 @@ func TestUnmount_AcquiresVolumeLock_SerializesConcurrentCalls(t *testing.T) { core.volumeLocks.Lock(volume) - mocks.OsUtils.EXPECT().IsLikelyDir("/target").Return(false, errors.New("stat failed")) + mocks.OsUtils.EXPECT().IsLikelyDir("/target").Return(false, os.ErrNotExist) done := make(chan error, 1) go func() { done <- core.Unmount(context.Background(), volume, UnmountRequest{TargetPath: "/target"}) }() @@ -60,8 +60,7 @@ func TestUnmount_AcquiresVolumeLock_SerializesConcurrentCalls(t *testing.T) { select { case err := <-done: - require.Error(t, err) - assert.Contains(t, err.Error(), "stat failed") + require.NoError(t, err) case <-time.After(2 * time.Second): t.Fatal("Unmount did not proceed after volume lock was released") } @@ -88,15 +87,45 @@ func TestUnmountGeneric_TargetPathGone_ReturnsNilSuccess(t *testing.T) { assert.NoError(t, err) } -func TestUnmountGeneric_IsLikelyDirOtherError_Wrapped(t *testing.T) { +func TestUnmountGeneric_StatError_Mounted_FallsBackToMountTableAndUnmounts(t *testing.T) { core, mocks := newTestCore(t) - mocks.OsUtils.EXPECT().IsLikelyDir("/target").Return(false, errors.New("permission denied")) + mocks.OsUtils.EXPECT().IsLikelyDir("/target").Return(false, os.ErrPermission) + mocks.Mount.EXPECT().IsMounted(gomock.Any(), "", "/target", "").Return(true, nil) + // IsLikelyNotMountPoint must not be called: it would stat the path again. + mocks.Mount.EXPECT().Umount(gomock.Any(), "/target").Return(nil) + mocks.OsUtils.EXPECT().DeleteResourceAtPath(gomock.Any(), "/target").Return(nil) + mocks.NodeHelper.EXPECT().RemovePublishedPath(gomock.Any(), "vol1", "/target").Return(nil) + + err := core.unmountGeneric(context.Background(), "vol1", "/target") + assert.NoError(t, err) +} + +func TestUnmountGeneric_StatError_NotMounted_SkipsUmount(t *testing.T) { + core, mocks := newTestCore(t) + + mocks.OsUtils.EXPECT().IsLikelyDir("/target").Return(false, os.ErrPermission) + mocks.Mount.EXPECT().IsMounted(gomock.Any(), "", "/target", "").Return(false, nil) + // IsLikelyNotMountPoint must not be called: it would stat the path again. + // No Umount call expected. + mocks.OsUtils.EXPECT().DeleteResourceAtPath(gomock.Any(), "/target").Return(nil) + mocks.NodeHelper.EXPECT().RemovePublishedPath(gomock.Any(), "vol1", "/target").Return(nil) + + err := core.unmountGeneric(context.Background(), "vol1", "/target") + assert.NoError(t, err) +} + +func TestUnmountGeneric_StatError_MountTableError_Wrapped(t *testing.T) { + core, mocks := newTestCore(t) + + mocks.OsUtils.EXPECT().IsLikelyDir("/target").Return(false, os.ErrPermission) + // IsLikelyNotMountPoint must not be called: it would stat the path again. + mocks.Mount.EXPECT().IsMounted(gomock.Any(), "", "/target", "").Return(false, errors.New("boom")) err := core.unmountGeneric(context.Background(), "vol1", "/target") require.Error(t, err) - assert.Contains(t, err.Error(), "could not check if the target path") - assert.Contains(t, err.Error(), "permission denied") + assert.Contains(t, err.Error(), "unable to check if targetPath") + assert.Contains(t, err.Error(), "boom") } func TestUnmountGeneric_IsDir_UsesIsLikelyNotMountPoint(t *testing.T) {