Skip to content

Commit

Permalink
Use correct volumeID to lookup private mount for ephemeral volumes in…
Browse files Browse the repository at this point in the history
… NodeUnpublishVolume (#395)

* Use storage system volume ID for unmounting ephemeral volumes.

* Enhanced an ephemeral volume unpublish test with a check for mount points being removed.
  • Loading branch information
alexemc authored Jan 13, 2025
1 parent 26a55d3 commit c9fe976
Show file tree
Hide file tree
Showing 4 changed files with 14 additions and 18 deletions.
1 change: 1 addition & 0 deletions service/features/ephemeral.feature
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ Scenario Outline: Node publish and unpublish ephemeral volume
And I call NodePublishVolume "SDC_GUID"
And I call NodeUnpublishVolume "SDC_GUID"
Then the error contains <errormsg>
And there are no remaining mounts

Examples:
| name | size | storagepool | systemName | errormsg |
Expand Down
22 changes: 10 additions & 12 deletions service/mount.go
Original file line number Diff line number Diff line change
Expand Up @@ -599,14 +599,12 @@ func mkdir(path string) (bool, error) {
// It determines this by checking to see if the volume is mounted anywhere else
// other than the private mount.
func unpublishVolume(
req *csi.NodeUnpublishVolumeRequest,
volumeID, targetPath string,
privDir, device string, reqID string,
) error {
ctx := context.Background()
id := req.GetVolumeId()

target := req.GetTargetPath()
if target == "" {
if targetPath == "" {
return status.Error(codes.InvalidArgument,
"target path required")
}
Expand All @@ -616,17 +614,17 @@ func unpublishVolume(
if err != nil {
return status.Errorf(codes.Internal,
"error getting block device for volume: %s, err: %s",
id, err.Error())
volumeID, err.Error())
}

// Path to mount device to
privTgt := getPrivateMountPoint(privDir, id)
privTgt := getPrivateMountPoint(privDir, volumeID)

f := logrus.Fields{
"device": sysDevice.RealDev,
"privTgt": privTgt,
"CSIRequestID": reqID,
"target": target,
"target": targetPath,
}

mnts, err := gofsutil.GetMounts(ctx)
Expand All @@ -644,10 +642,10 @@ func unpublishVolume(
if m.Path == privTgt {
privMntExist = true
Log.Printf("Found private mount for device %#v, private mount path: %s .", sysDevice, privTgt)
} else if m.Path == target {
} else if m.Path == targetPath {
tgtMntExist = true
deviceMount = m
Log.Printf("Found target mount for device %#v, target mount path: %s .", sysDevice, target)
Log.Printf("Found target mount for device %#v, target mount path: %s .", sysDevice, targetPath)
}
}
}
Expand All @@ -656,12 +654,12 @@ func unpublishVolume(
}

if tgtMntExist {
Log.WithFields(f).Debug(fmt.Sprintf("Unmounting %s", target))
if err := gofsutil.Unmount(ctx, target); err != nil {
Log.WithFields(f).Debug(fmt.Sprintf("Unmounting %s", targetPath))
if err := gofsutil.Unmount(ctx, targetPath); err != nil {
return status.Errorf(codes.Internal,
"Error unmounting target: %s", err.Error())
}
if err := removeWithRetry(target); err != nil {
if err := removeWithRetry(targetPath); err != nil {
return status.Errorf(codes.Internal,
"Error remove target folder: %s", err.Error())
}
Expand Down
2 changes: 1 addition & 1 deletion service/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ func (s *service) NodeUnpublishVolume(
return &csi.NodeUnpublishVolumeResponse{}, nil
}

if err := unpublishVolume(req, s.privDir, sdcMappedVol.SdcDevice, reqID); err != nil {
if err := unpublishVolume(csiVolID, req.GetTargetPath(), s.privDir, sdcMappedVol.SdcDevice, reqID); err != nil {
return nil, err
}

Expand Down
7 changes: 2 additions & 5 deletions service/step_defs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2854,15 +2854,12 @@ func (f *feature) iCallMountPublishVolume() error {
}

func (f *feature) iCallMountUnpublishVolume() error {
req := new(csi.NodeUnpublishVolumeRequest)
req.TargetPath = ""
err := unpublishVolume(req, "", "/bad/device", "1")
err := unpublishVolume("", "", "", "/bad/device", "1")
if err != nil {
fmt.Printf("NodeUnpublishVolume bad targetPath: %s\n", err.Error())
f.err = errors.New("error in unpublishVolume")
}
req.TargetPath = "/badpath"
err = unpublishVolume(req, "", "/bad/device", "1")
err = unpublishVolume("", "/badpath", "", "/bad/device", "1")
if err != nil {
fmt.Printf("NodeUnpublishVolume bad device : %s\n", err.Error())
f.err = errors.New("error in unpublishVolume")
Expand Down

0 comments on commit c9fe976

Please sign in to comment.