Skip to content

Commit

Permalink
csi: NodePublish should not create target_path, only its parent dir
Browse files Browse the repository at this point in the history
The NodePublish workflow currently creates the target path and its parent
directory. However, the CSI specification says that the CO shall ensure the
parent directory of the target path exists, and that the SP shell place the
block device or mounted directory at the target path. Much of our testing has
been with CSI plugins that are more forgiving, but our behavior breaks
spec-compliant CSI plugins.

This changeset ensures we only create the parent directory.
  • Loading branch information
tgross committed Jul 22, 2020
1 parent 8241a02 commit 985b7c2
Showing 1 changed file with 13 additions and 7 deletions.
20 changes: 13 additions & 7 deletions client/pluginmanager/csimanager/volume.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,11 @@ func (v *volumeManager) stagingDirForVolume(root string, volID string, usage *Us
return filepath.Join(root, StagingDirName, volID, usage.ToFS())
}

func (v *volumeManager) allocDirForVolume(root string, volID, allocID string, usage *UsageOptions) string {
func (v *volumeManager) allocDirForVolume(root string, volID, allocID string) string {
return filepath.Join(root, AllocSpecificDirName, allocID, volID)
}

func (v *volumeManager) targetForVolume(root string, volID, allocID string, usage *UsageOptions) string {
return filepath.Join(root, AllocSpecificDirName, allocID, volID, usage.ToFS())
}

Expand Down Expand Up @@ -103,16 +107,17 @@ func (v *volumeManager) ensureStagingDir(vol *structs.CSIVolume, usage *UsageOpt
// Returns whether the directory is a pre-existing mountpoint, the publish path,
// and any errors that occurred.
func (v *volumeManager) ensureAllocDir(vol *structs.CSIVolume, alloc *structs.Allocation, usage *UsageOptions) (string, bool, error) {
allocPath := v.allocDirForVolume(v.mountRoot, vol.ID, alloc.ID, usage)
allocPath := v.allocDirForVolume(v.mountRoot, vol.ID, alloc.ID)
targetPath := v.targetForVolume(v.mountRoot, vol.ID, alloc.ID, usage)

// Make the alloc path, owned by the Nomad User
if err := os.MkdirAll(allocPath, 0700); err != nil && !os.IsExist(err) {
return "", false, fmt.Errorf("failed to create allocation directory for volume (%s): %v", vol.ID, err)
}

// Validate that it is not already a mount point
// Validate that the target is not already a mount point
m := mount.New()
isNotMount, err := m.IsNotAMountPoint(allocPath)
isNotMount, err := m.IsNotAMountPoint(targetPath)
if err != nil {
return "", false, fmt.Errorf("mount point detection failed for volume (%s): %v", vol.ID, err)
}
Expand Down Expand Up @@ -192,7 +197,7 @@ func (v *volumeManager) publishVolume(ctx context.Context, vol *structs.CSIVolum
if err != nil {
return nil, err
}
pluginTargetPath := v.allocDirForVolume(v.containerMountPoint, vol.ID, alloc.ID, usage)
pluginTargetPath := v.targetForVolume(v.containerMountPoint, vol.ID, alloc.ID, usage)

if isMount {
logger.Debug("Re-using existing published volume for allocation")
Expand Down Expand Up @@ -296,7 +301,7 @@ func combineErrors(maybeErrs ...error) error {
}

func (v *volumeManager) unpublishVolume(ctx context.Context, volID, remoteID, allocID string, usage *UsageOptions) error {
pluginTargetPath := v.allocDirForVolume(v.containerMountPoint, volID, allocID, usage)
pluginTargetPath := v.targetForVolume(v.containerMountPoint, volID, allocID, usage)

// CSI NodeUnpublishVolume errors for timeout, codes.Unavailable and
// codes.ResourceExhausted are retried; all other errors are fatal.
Expand All @@ -306,7 +311,8 @@ func (v *volumeManager) unpublishVolume(ctx context.Context, volID, remoteID, al
grpc_retry.WithBackoff(grpc_retry.BackoffExponential(100*time.Millisecond)),
)

hostTargetPath := v.allocDirForVolume(v.mountRoot, volID, allocID, usage)
hostTargetPath := v.targetForVolume(v.mountRoot, volID, allocID, usage)

if _, err := os.Stat(hostTargetPath); os.IsNotExist(err) {
if rpcErr != nil && strings.Contains(rpcErr.Error(), "no mount point") {
// host target path was already destroyed, nothing to do here.
Expand Down

0 comments on commit 985b7c2

Please sign in to comment.