Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle error scenarios gracefully in NodeUnpublishVolume #161

Merged
merged 4 commits into from
Jan 23, 2024
Merged

Handle error scenarios gracefully in NodeUnpublishVolume #161

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9be4602
handle unpublish gracefully for volumes that are unmounted already
Jan 18, 2024
4c629a8
Merge branch 'main' into main
Jan 18, 2024
d6f91f6
implement unit test for unmount failure as well valid mount checks
Jan 18, 2024
629d5e7
fixing lint errors
Jan 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions pkg/driver/driver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,10 +159,17 @@ func (d *Driver) NodeUnpublishVolume(ctx context.Context, req *csi.NodeUnpublish
return nil, status.Error(codes.InvalidArgument, "request missing required target path")
}

if err := unmount(req.TargetPath); err != nil {
return nil, status.Errorf(codes.Internal, "unable to unmount %q: %v", req.TargetPath, err)
// Check if target is a valid mount and issue unmount request
if ok, err := isMountPoint(req.TargetPath); err != nil {
return nil, status.Errorf(codes.Internal, "unable to verify mount point %q: %v", req.TargetPath, err)
} else if ok {
if err := unmount(req.TargetPath); err != nil {
return nil, status.Errorf(codes.Internal, "unable to unmount %q: %v", req.TargetPath, err)
}
}
if err := os.Remove(req.TargetPath); err != nil {

// Check and remove the mount path if present, report an error otherwise
if err := os.Remove(req.TargetPath); err != nil && !errors.Is(err, os.ErrNotExist) {
return nil, status.Errorf(codes.Internal, "unable to remove target path %q: %v", req.TargetPath, err)
}

Expand Down
31 changes: 29 additions & 2 deletions pkg/driver/driver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,13 @@ import (
)

const (
testNodeID = "nodeID"
testNodeID = "nodeID"
unmountFailureTest = "unmount failure"
isMountFailureTest = "isMount failure"
)

var (
testDescription string
)

func init() {
Expand All @@ -34,6 +40,17 @@ func init() {
unmount = func(dst string) error {
return os.Remove(metaPath(dst))
}
isMountPoint = func(dst string) (bool, error) {
if testDescription == unmountFailureTest {
return true, nil
}

if testDescription == isMountFailureTest {
return false, fmt.Errorf("mock invalid mount point")
}

return true, nil
}
}

func TestNew(t *testing.T) {
Expand Down Expand Up @@ -323,7 +340,12 @@ func TestNodeUnpublishVolume(t *testing.T) {
expectMsgPrefix: "request missing required target path",
},
{
desc: "unmount failure",
desc: isMountFailureTest,
expectCode: codes.Internal,
expectMsgPrefix: "unable to verify mount point",
},
{
desc: unmountFailureTest,
mungeTargetPath: func(t *testing.T, targetPath string) {
// Removing the meta file to simulate that it wasn't mounted
require.NoError(t, os.Remove(metaPath(targetPath)))
Expand Down Expand Up @@ -365,6 +387,7 @@ func TestNodeUnpublishVolume(t *testing.T) {
if tt.mutateReq != nil {
tt.mutateReq(req)
}
registerTestDescription(tt.desc)
dumpIt(t, "BEFORE", targetPathBase)
resp, err := client.NodeUnpublishVolume(context.Background(), req)
dumpIt(t, "AFTER", targetPathBase)
Expand All @@ -379,6 +402,10 @@ func TestNodeUnpublishVolume(t *testing.T) {
}
}

func registerTestDescription(desc string) {
testDescription = desc
}

func requireGRPCStatusPrefix(tb testing.TB, err error, code codes.Code, msgPrefix string, msgAndArgs ...interface{}) {
st := status.Convert(err)
if code != st.Code() || !strings.HasPrefix(st.Message(), msgPrefix) {
Expand Down