luxolus/open-nomad
2
0
Fork 0
Code Issues 1 Pull requests Packages Releases Wiki Activity

csi: NodePublish should not create target_path, only its parent dir (#8505)

Browse source 
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 shall 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.
This commit is contained in:
Tim Gross 2020-07-23 15:52:22 -04:00 committed by GitHub
parent fb21e592cd
commit 56c6dacd38
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
1 changed files with 13 additions and 8 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

21
client/pluginmanager/csimanager/volume.go
View file

@ -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())
}
@ -103,21 +107,22 @@ 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)
// 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
targetPath := v.targetForVolume(v.mountRoot, vol.ID, alloc.ID, usage)
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)
}
return allocPath, !isNotMount, nil
return targetPath, !isNotMount, nil
}
func volumeCapability(vol *structs.CSIVolume, usage *UsageOptions) (*csi.VolumeCapability, error) {
@ -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")
@ -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.
@ -306,7 +311,7 @@ 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.