From e538f6624def6b57f945cc01887a4bf97819cbb1 Mon Sep 17 00:00:00 2001 From: Eddie Torres Date: Tue, 21 Mar 2023 18:21:36 +0000 Subject: [PATCH] Fix staging / unmounting volume operations on Windows - `GetDeviceNameFromMount` now returns the disk number for a mount path on Windows - Cleanup the stage path via Rmdir in `proxyMounter.Unmount()` - Remove unnecessary call to `proxyMounter.WriteVolumeCache()` when unpublishing a volume - Return a nil error in `GetDeviceNameFromMount` if the target path does not exist Signed-off-by: Eddie Torres --- pkg/driver/mount_windows.go | 9 +------- pkg/driver/node.go | 8 ++++--- pkg/mounter/safe_mounter_windows.go | 34 +++++++++++++++++++++-------- 3 files changed, 31 insertions(+), 20 deletions(-) diff --git a/pkg/driver/mount_windows.go b/pkg/driver/mount_windows.go index c9a9e1e46f..8824f3dba4 100644 --- a/pkg/driver/mount_windows.go +++ b/pkg/driver/mount_windows.go @@ -56,7 +56,7 @@ func (m NodeMounter) GetDeviceNameFromMount(mountPath string) (string, int, erro // internal Get-Item cmdlet didn't fail but no item/device was found at the // path so we should return empty string and nil error just like the Linux // implementation would. - pattern := `(Get-Item -Path \S+).Target, output: , error: ` + pattern := `(Get-Item -Path \S+).Target, output: , error: |because it does not exist` matched, matchErr := regexp.MatchString(pattern, err.Error()) if matched { return "", 0, nil @@ -128,8 +128,6 @@ func (m *NodeMounter) Unpublish(target string) error { if !ok { return fmt.Errorf("failed to cast mounter to csi proxy mounter") } - // WriteVolumeCache before unmount - proxyMounter.WriteVolumeCache(target) // Remove symlink err := proxyMounter.Rmdir(target) if err != nil { @@ -150,11 +148,6 @@ func (m *NodeMounter) Unstage(target string) error { if err != nil { return err } - // Cleanup stage path - err = proxyMounter.Rmdir(target) - if err != nil { - return err - } return nil } diff --git a/pkg/driver/node.go b/pkg/driver/node.go index 3afff717ca..ad27136f35 100644 --- a/pkg/driver/node.go +++ b/pkg/driver/node.go @@ -220,13 +220,14 @@ func (d *nodeService) NodeStageVolume(ctx context.Context, req *csi.NodeStageVol // This operation (NodeStageVolume) MUST be idempotent. // If the volume corresponding to the volume_id is already staged to the staging_target_path, // and is identical to the specified volume_capability the Plugin MUST reply 0 OK. + klog.V(4).InfoS("NodeStageVolume: checking if volume is already staged", "device", device, "source", source, "target", target) if device == source { klog.V(4).InfoS("NodeStageVolume: volume already staged", "volumeID", volumeID) return &csi.NodeStageVolumeResponse{}, nil } // FormatAndMount will format only if needed - klog.V(4).InfoS("NodeStageVolume: formatting and mounting with fstype", "source", source, "target", target, "fstype", fsType) + klog.V(4).InfoS("NodeStageVolume: formatting and mounting with fstype", "source", source, "volumeID", volumeID, "target", target, "fstype", fsType) formatOptions := []string{} if len(blockSize) > 0 { if fsType == FSTypeXfs { @@ -255,6 +256,7 @@ func (d *nodeService) NodeStageVolume(ctx context.Context, req *csi.NodeStageVol return nil, status.Errorf(codes.Internal, "Could not resize volume %q (%q): %v", volumeID, source, err) } } + klog.V(4).InfoS("NodeStageVolume: successfully formatted and mounted volume", "source", source, "volumeID", volumeID, "target", target, "fstype", fsType) return &csi.NodeStageVolumeResponse{}, nil } @@ -283,7 +285,7 @@ func (d *nodeService) NodeUnstageVolume(ctx context.Context, req *csi.NodeUnstag // returns the device name, reference count, and error code dev, refCount, err := d.mounter.GetDeviceNameFromMount(target) if err != nil { - msg := fmt.Sprintf("failed to check if volume is mounted: %v", err) + msg := fmt.Sprintf("failed to check if target %q is a mount point: %v", target, err) return nil, status.Error(codes.Internal, msg) } @@ -304,7 +306,7 @@ func (d *nodeService) NodeUnstageVolume(ctx context.Context, req *csi.NodeUnstag if err != nil { return nil, status.Errorf(codes.Internal, "Could not unmount target %q: %v", target, err) } - + klog.V(4).InfoS("NodeUnStageVolume: successfully unstaged volume", "volumeID", volumeID, "target", target) return &csi.NodeUnstageVolumeResponse{}, nil } diff --git a/pkg/mounter/safe_mounter_windows.go b/pkg/mounter/safe_mounter_windows.go index a8b6163eda..81ef3aa6bc 100644 --- a/pkg/mounter/safe_mounter_windows.go +++ b/pkg/mounter/safe_mounter_windows.go @@ -97,11 +97,13 @@ func (mounter *CSIProxyMounter) Unmount(target string) error { TargetPath: normalizeWindowsPath(target), } volumeIdResponse, err := mounter.VolumeClient.GetVolumeIDFromTargetPath(context.Background(), getVolumeIdRequest) - volumeId := volumeIdResponse.GetVolumeId() + if err != nil { + return err + } // Call UnmountVolume CSI proxy function which flushes data cache to disk and removes the global staging path unmountVolumeRequest := &volume.UnmountVolumeRequest{ - VolumeId: volumeId, + VolumeId: volumeIdResponse.VolumeId, TargetPath: normalizeWindowsPath(target), } _, err = mounter.VolumeClient.UnmountVolume(context.Background(), unmountVolumeRequest) @@ -109,25 +111,31 @@ func (mounter *CSIProxyMounter) Unmount(target string) error { return err } + // Cleanup stage path + err = mounter.Rmdir(target) + if err != nil { + return err + } + // Get disk number getDiskNumberRequest := &volume.GetDiskNumberFromVolumeIDRequest{ - VolumeId: volumeId, + VolumeId: volumeIdResponse.VolumeId, } getDiskNumberResponse, err := mounter.VolumeClient.GetDiskNumberFromVolumeID(context.Background(), getDiskNumberRequest) - diskNumber := getDiskNumberResponse.GetDiskNumber() if err != nil { return err } // Offline the disk setDiskStateRequest := &disk.SetDiskStateRequest{ - DiskNumber: diskNumber, + DiskNumber: getDiskNumberResponse.DiskNumber, IsOnline: false, } _, err = mounter.DiskClient.SetDiskState(context.Background(), setDiskStateRequest) if err != nil { return err } + klog.V(4).InfoS("Successfully unmounted volume", "diskNumber", getDiskNumberResponse.DiskNumber, "volumeId", volumeIdResponse.VolumeId, "target", target) return nil } @@ -208,15 +216,23 @@ func (mounter *CSIProxyMounter) DeviceOpened(pathname string) (bool, error) { return false, fmt.Errorf("DeviceOpened not implemented for CSIProxyMounter") } -// GetDeviceNameFromMount returns the volume ID for a mount path. -func (mounter *CSIProxyMounter) GetDeviceNameFromMount(mountPath, pluginMountDir string) (string, error) { +// GetDeviceNameFromMount returns the disk number for a mount path. +func (mounter *CSIProxyMounter) GetDeviceNameFromMount(mountPath, _ string) (string, error) { req := &volume.GetVolumeIDFromTargetPathRequest{TargetPath: normalizeWindowsPath(mountPath)} resp, err := mounter.VolumeClient.GetVolumeIDFromTargetPath(context.Background(), req) if err != nil { return "", err } - - return resp.VolumeId, nil + // Get disk number + getDiskNumberRequest := &volume.GetDiskNumberFromVolumeIDRequest{ + VolumeId: resp.VolumeId, + } + getDiskNumberResponse, err := mounter.VolumeClient.GetDiskNumberFromVolumeID(context.Background(), getDiskNumberRequest) + if err != nil { + return "", err + } + klog.V(4).InfoS("GetDeviceNameFromMount called", "diskNumber", getDiskNumberResponse.DiskNumber, "volumeID", resp.VolumeId, "mountPath", mountPath) + return fmt.Sprint(getDiskNumberResponse.DiskNumber), nil } func (mounter *CSIProxyMounter) MakeRShared(path string) error {