Simplifying the docker plugin mode logic for looking up the host path…

… (#1084)

Simplifying the docker plugin mode logic for looking up the host path

core/orchestrator_core.go

@@ -41,7 +41,9 @@ const (
 )
 
 // recordTiming is used to record in Prometheus the total time taken for an operation as follows:
-//   defer recordTiming("backend_add")()
+//
+//	defer recordTiming("backend_add")()
+//
 // see also: https://play.golang.org/p/6xRXlhFdqBd
 func recordTiming(operation string, err *error) func() {
 	startTime := time.Now()
@@ -3220,6 +3222,11 @@ func (o *TridentOrchestrator) unpublishVolume(ctx context.Context, volumeName, n
 	return nil
 }
 
+// isDockerPluginMode returns true if the ENV variable config.DockerPluginModeEnvVariable is set
+func isDockerPluginMode() bool {
+	return os.Getenv(config.DockerPluginModeEnvVariable) != ""
+}
+
 // AttachVolume mounts a volume to the local host.  This method is currently only used by Docker,
 // and it should be able to accomplish its task using only the data passed in; it should not need to
 // use the storage controller API.  It may be assumed that this method always runs on the host to
@@ -3245,9 +3252,8 @@ func (o *TridentOrchestrator) AttachVolume(
 	}
 
 	hostMountpoint := mountpoint
-	isDockerPluginModeSet := false
-	if os.Getenv(config.DockerPluginModeEnvVariable) != "" {
-		isDockerPluginModeSet = true
+	isDockerPluginModeSet := isDockerPluginMode()
+	if isDockerPluginModeSet {
 		hostMountpoint = filepath.Join("/host", mountpoint)
 	}
 
@@ -3327,25 +3333,34 @@ func (o *TridentOrchestrator) DetachVolume(ctx context.Context, volumeName, moun
 		return utils.VolumeDeletingError(fmt.Sprintf("volume %s is deleting", volumeName))
 	}
 
+	hostMountpoint := mountpoint
+	isDockerPluginModeSet := isDockerPluginMode()
+	if isDockerPluginModeSet {
+		hostMountpoint = filepath.Join("/host", mountpoint)
+	}
+
 	Logc(ctx).WithFields(log.Fields{
-		"volume":     volumeName,
-		"mountpoint": mountpoint,
+		"volume":                volumeName,
+		"mountpoint":            mountpoint,
+		"hostMountpoint":        hostMountpoint,
+		"isDockerPluginModeSet": isDockerPluginModeSet,
 	}).Debug("Unmounting volume.")
 
 	// Check if the mount point exists, so we know that it's attached and must be cleaned up
-	_, err = os.Stat(mountpoint)
+	_, err = os.Stat(hostMountpoint) // trident is not chrooted, therefore we must use the /host if in plugin mode
 	if err != nil {
 		// Not attached, so nothing to do
 		return nil
 	}
 
 	// Unmount the volume
 	if err := utils.Umount(ctx, mountpoint); err != nil {
+		// utils.Unmount is chrooted, therefore it does NOT need the /host
 		return err
 	}
 
 	// Best effort removal of the mount point
-	_ = os.Remove(mountpoint)
+	_ = os.Remove(hostMountpoint) // trident is not chrooted, therefore we must the /host if in plugin mode
 	return nil
 }
 

frontend/docker/plugin.go

@@ -7,7 +7,6 @@ import (
 	"crypto/tls"
 	"encoding/json"
 	"fmt"
-	"io/ioutil"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -66,78 +65,34 @@ func NewPlugin(driverName, driverPort string, orchestrator core.Orchestrator) (*
 	}
 
 	if plugin.isDockerPluginMode {
-
-		// determine the docker data-root
-		var daemonConfig DaemonConfig
-		if _, statErr := os.Stat(filepath.Join("/host", DaemonConfigFile)); os.IsNotExist(statErr) {
-			// this is not a problem, they may not have changed their docker setup, so go with the default
-			Logc(ctx).Debugf("custom docker daemon config '%v' not found\n", DaemonConfigFile)
-		} else {
-			// check to see if they have specified a new value for the data-root
-			file, err := ioutil.ReadFile(filepath.Join("/host", DaemonConfigFile))
-			if err != nil {
-				return nil, err
-			}
-
-			err = json.Unmarshal([]byte(file), &daemonConfig)
-			if err != nil {
-				return nil, err
-			}
-		}
-		if daemonConfig.DataRoot == "" {
-			daemonConfig.DataRoot = DefaultDaemonDataRoot
+		hostMountInfo, err := utils.GetHostMountInfo(ctx)
+		if err != nil {
+			return nil, err
 		}
-		Logc(ctx).Debugf("using data-root: %v\n", daemonConfig.DataRoot)
 
-		mountInfo, err := utils.GetMountInfo(ctx)
+		selfMountInfo, err := utils.GetSelfMountInfo(ctx)
 		if err != nil {
 			return nil, err
 		}
 
-	mountInfoSearch:
-		for _, procMount := range mountInfo {
-			Logc(ctx).Debugf("root: %v, mountPoint: %v\n", procMount.Root, procMount.MountPoint)
-			if procMount.MountPoint == volume.DefaultDockerRootDirectory+"/netapp" {
-				// procMount.Root contains our unique docker plugin id in the string, and usually looks like this:
-				//   /var/lib/docker/plugins/579614f4362d39dcbcc96be69813d1b2c14fc0d409ac517e5d2ff7a64e685bb4/propagated-mount
-				// or more generally
-				//   /daemonConfig.DataRoot/plugins/579614f4362d39dcbcc96be69813d1b2c14fc0d409ac517e5d2ff7a64e685bb4/propagated-mount
-				plugin.hostVolumePath = filepath.Join(procMount.Root)
-				if strings.HasPrefix(plugin.hostVolumePath, "/plugins") {
-					// the path is missing the expected prefix (defaults to '/var/lib/docker') and we must add it back
-					//
-					// this scenario can happen when /var/lib/docker is a loopback device, or does not live directly on /
-					//
-					// in other words, we need to convert from what we found:
-					//    /plugins/579614f4362d39dcbcc96be69813d1b2c14fc0d409ac517e5d2ff7a64e685bb4/propagated-mount
-					//
-					// into what we need:
-					//   /var/lib/docker/plugins/579614f4362d39dcbcc96be69813d1b2c14fc0d409ac517e5d2ff7a64e685bb4/propagated-mount
-					//  or more generally:
-					//   /daemonConfig.DataRoot/plugins/579614f4362d39dcbcc96be69813d1b2c14fc0d409ac517e5d2ff7a64e685bb4/propagated-mount
-					plugin.hostVolumePath = filepath.Join(daemonConfig.DataRoot, plugin.hostVolumePath)
-				}
-
-				// confirm the computed hostVolumePath exists on the /host filesystem
-				if _, err := os.Stat(filepath.Join("/host", plugin.hostVolumePath)); os.IsNotExist(err) {
-					// NOTE: we must add a "/host" prefix here because the Trident binary itself is not "chrooted"
-					// the mount command that we eventually invoke IS "chrooted" and it will not need the "/host" prefix
-					// so, we store it without the "/host" prefix, but in this path we must prepend it to test
-					return nil, fmt.Errorf("plugin.hostVolumePath %v does not exist on /host path", plugin.hostVolumePath)
-				}
-
-				Logc(ctx).WithFields(log.Fields{
-					"plugin.volumePath":     plugin.volumePath,
-					"plugin.hostVolumePath": plugin.hostVolumePath,
-				}).Debugf("Running in Docker plugin mode.")
-				break mountInfoSearch
-			}
+		plugin.hostVolumePath, err = deriveHostVolumePath(ctx, hostMountInfo, selfMountInfo)
+		if err != nil {
+			return nil, err
 		}
 
-		if plugin.hostVolumePath == "" {
-			return nil, fmt.Errorf("could not find proc mount entry for %v", volume.DefaultDockerRootDirectory)
+		// confirm the derived hostVolumePath exists on the /host filesystem
+		if _, err := os.Stat(filepath.Join("/host", plugin.hostVolumePath)); os.IsNotExist(err) {
+			// NOTE: we must add a "/host" prefix here because the Trident binary itself is not "chrooted"
+			// the mount command that we eventually invoke IS "chrooted" and it will not need the "/host" prefix
+			// so, we store it without the "/host" prefix, but in this path we must prepend it to test
+			return nil, fmt.Errorf("plugin.hostVolumePath %v does not exist on /host path", plugin.hostVolumePath)
 		}
 
+		Logc(ctx).WithFields(log.Fields{
+			"plugin.volumePath":     plugin.volumePath,
+			"plugin.hostVolumePath": plugin.hostVolumePath,
+		}).Debugf("Running in Docker plugin mode.")
+
 	} else {
 
 		// Register the plugin with Docker, needed in binary mode (non-plugin mode)
@@ -155,6 +110,53 @@ func NewPlugin(driverName, driverPort string, orchestrator core.Orchestrator) (*
 	return plugin, nil
 }
 
+// deriveHostVolumePath uses /proc/1/mountinfo and /proc/self/mountinfo to determine the host's path for mounting
+func deriveHostVolumePath(ctx context.Context, hostMountInfo, selfMountInfo []utils.MountInfo) (string, error) {
+	/*
+		Given the location /var/lib/docker-volumes/netapp within the docker plugin container, we must determine
+		where this location exists on the host.
+
+		As an example, given the following output we can derive that the container's view of the directory:
+		   /var/lib/docker-volumes/netapp
+		is actually the following directory on the host:
+		   /dev/lib/docker/plugins/9722f031f38b0188233463043f8a76b09d6c8b1d194ef46c0b16191f84ccf8e9/propagated-mount
+
+		# ps -ef | grep -i trident
+		root      9145  9091  0 18:27 pts/11   00:00:00 grep --color=auto -i trident
+		root     25202 25172  0 Sep10 ?        00:00:06 /netapp/trident --address=0.0.0.0 --port=8000 --docker_plugin_mode=true
+
+		# cat /proc/25202/mountinfo | grep " /var/lib/docker-volumes/netapp "
+		437 417 0:6 /lib/docker/plugins/9722f031f38b0188233463043f8a76b09d6c8b1d194ef46c0b16191f84ccf8e9/propagated-mount /var/lib/docker-volumes/netapp rw,nosuid,relatime shared:2 - devtmpfs udev rw,size=8187808k,nr_inodes=2046952,mode=755
+
+		# cat /proc/1/mountinfo | grep propagated-mount
+		263 25 0:6 /lib/docker/plugins/9722f031f38b0188233463043f8a76b09d6c8b1d194ef46c0b16191f84ccf8e9/propagated-mount /dev/lib/docker/plugins/9722f031f38b0188233463043f8a76b09d6c8b1d194ef46c0b16191f84ccf8e9/propagated-mount rw,nosuid,relatime shared:2 - devtmpfs udev rw,size=8187808k,nr_inodes=2046952,mode=755
+	*/
+	if len(hostMountInfo) == 0 {
+		return "", fmt.Errorf("cannot derive host volume path, missing /proc/1/mountinfo data")
+	}
+
+	if len(selfMountInfo) == 0 {
+		return "", fmt.Errorf("cannot derive host volume path, missing /proc/self/mountinfo data")
+	}
+
+	m := make(map[string]string)
+	for _, hostProcMount := range hostMountInfo {
+		Logc(ctx).Debugf("hroot: %v, hmountPoint: %v\n", hostProcMount.Root, hostProcMount.MountPoint)
+		m[hostProcMount.Root] = filepath.Join(hostProcMount.MountPoint)
+	}
+
+	for _, procSelfMount := range selfMountInfo {
+		Logc(ctx).Debugf("root: %v, mountPoint: %v\n", procSelfMount.Root, procSelfMount.MountPoint)
+		if procSelfMount.MountPoint == volume.DefaultDockerRootDirectory+"/netapp" {
+			if hostVolumePath, ok := m[procSelfMount.Root]; ok {
+				return hostVolumePath, nil
+			}
+		}
+	}
+
+	return "", fmt.Errorf("could not find proc mount entry for %v", volume.DefaultDockerRootDirectory)
+}
+
 func registerDockerVolumePlugin(ctx context.Context, root string) error {
 	Logc(ctx).Debugf(">>>> docker.registerDockerVolumePlugin(%s)", root)
 	defer Logc(ctx).Debugf("<<<< docker.registerDockerVolumePlugin(%s)", root)
@@ -515,7 +517,7 @@ func (p *Plugin) mountpoint(name string) string {
 	return filepath.Join(p.volumePath, name)
 }
 
-// hostMountpoint TBD but it's where the plugin's /var/lib/docker-volumes/netapp lives on the host
+// hostMountpoint is where the plugin's /var/lib/docker-volumes/netapp lives on the host
 func (p *Plugin) hostMountpoint(name string) string {
 	if p.isDockerPluginMode {
 		return filepath.Join(p.hostVolumePath, name)

frontend/docker/plugin_test.go

@@ -3,15 +3,74 @@
 package docker
 
 import (
+	"context"
 	"io/ioutil"
 	"os"
+	"path/filepath"
 	"testing"
 
 	log "github.com/sirupsen/logrus"
+	"github.com/stretchr/testify/assert"
+
+	"github.com/netapp/trident/utils"
 )
 
 func TestMain(m *testing.M) {
 	// Disable any standard log output
 	log.SetOutput(ioutil.Discard)
 	os.Exit(m.Run())
 }
+
+func TestDeriveHostVolumePath_negative(t *testing.T) {
+	ctx := context.TODO()
+
+	////////////////////////////////////////////////////////////////////////////////////////////////////////////
+	// negative case:  missing both mountinfos
+	hostMountInfo := []utils.MountInfo{}
+	selfMountInfo := []utils.MountInfo{}
+	_, err := deriveHostVolumePath(ctx, hostMountInfo, selfMountInfo)
+	assert.Error(t, err, "no error")
+	assert.Equal(t, "cannot derive host volume path, missing /proc/1/mountinfo data", err.Error())
+
+	////////////////////////////////////////////////////////////////////////////////////////////////////////////
+	// negative case: no data in selfMountInfo
+	hostMountInfo = append(hostMountInfo, utils.MountInfo{
+		Root:       "/lib/docker/plugins/9722f031f38b0188233463043f8a76b09d6c8b1d194ef46c0b16191f84ccf8e9/propagated-mount",
+		MountPoint: "/dev/lib/docker/plugins/9722f031f38b0188233463043f8a76b09d6c8b1d194ef46c0b16191f84ccf8e9/propagated-mount",
+	})
+	_, err = deriveHostVolumePath(ctx, hostMountInfo, selfMountInfo)
+	assert.Equal(t, "cannot derive host volume path, missing /proc/self/mountinfo data", err.Error())
+
+	////////////////////////////////////////////////////////////////////////////////////////////////////////////
+	// negative case: missing /var/lib/docker-volumes in selfMountInfo
+	selfMountInfo = append(selfMountInfo, utils.MountInfo{
+		Root:       "/",
+		MountPoint: "/dev/shm",
+	})
+	_, err = deriveHostVolumePath(ctx, hostMountInfo, selfMountInfo)
+	assert.Equal(t, "could not find proc mount entry for /var/lib/docker-volumes", err.Error())
+}
+
+func TestDeriveHostVolumePath_positive(t *testing.T) {
+	ctx := context.TODO()
+
+	////////////////////////////////////////////////////////////////////////////////////////////////////////////
+	// positive case: have all the data we need
+	hostMountInfo := []utils.MountInfo{
+		{Root: "/", MountPoint: "/dev/shm"},
+		{
+			Root:       "/lib/docker/plugins/9722f031f38b0188233463043f8a76b09d6c8b1d194ef46c0b16191f84ccf8e9/propagated-mount",
+			MountPoint: "/dev/lib/docker/plugins/9722f031f38b0188233463043f8a76b09d6c8b1d194ef46c0b16191f84ccf8e9/propagated-mount",
+		},
+	}
+	selfMountInfo := []utils.MountInfo{
+		{Root: "/", MountPoint: "/dev/shm"},
+		{
+			Root:       "/lib/docker/plugins/9722f031f38b0188233463043f8a76b09d6c8b1d194ef46c0b16191f84ccf8e9/propagated-mount",
+			MountPoint: "/var/lib/docker-volumes/netapp",
+		},
+	}
+	hostVolumePath, err := deriveHostVolumePath(ctx, hostMountInfo, selfMountInfo)
+	assert.Nil(t, err)
+	assert.Equal(t, filepath.FromSlash("/dev/lib/docker/plugins/9722f031f38b0188233463043f8a76b09d6c8b1d194ef46c0b16191f84ccf8e9/propagated-mount"), hostVolumePath)
+}

utils/bof.go

@@ -428,7 +428,7 @@ func GetMountedLoopDevices(ctx context.Context) ([]string, error) {
 	Logc(ctx).Debug(">>>> bof.GetMountedLoopDevices")
 	defer Logc(ctx).Debug("<<<< bof.GetMountedLoopDevices")
 
-	procSelfMountInfo, err := listProcSelfMountinfo(procSelfMountinfoPath)
+	procSelfMountInfo, err := listProcMountinfo(procSelfMountinfoPath)
 	if err != nil {
 		return nil, err
 	}

utils/devices.go

@@ -427,7 +427,7 @@ func GetMountedISCSIDevices(ctx context.Context) ([]*ScsiDeviceInfo, error) {
 	Logc(ctx).Debug(">>>> devices.GetMountedISCSIDevices")
 	defer Logc(ctx).Debug("<<<< devices.GetMountedISCSIDevices")
 
-	procSelfMountinfo, err := listProcSelfMountinfo(procSelfMountinfoPath)
+	procSelfMountinfo, err := listProcMountinfo(procSelfMountinfoPath)
 	if err != nil {
 		return nil, err
 	}

utils/mount.go

@@ -103,6 +103,8 @@ const (
 	procMountsPath = "/proc/mounts"
 	// Location of the mount file to use
 	procSelfMountinfoPath = "/proc/self/mountinfo"
+	// Location of the host mount file to use
+	procHostMountinfoPath = "/proc/1/mountinfo"
 )
 
 // This represents a single line in /proc/mounts or /etc/fstab.
@@ -129,19 +131,19 @@ type MountInfo struct {
 	SuperOptions []string
 }
 
-// listProcSelfMountinfo (Available since Linux 2.6.26) lists information about mount points
+// listProcMountinfo (Available since Linux 2.6.26) lists information about mount points
 // in the process's mount namespace. Ref: http://man7.org/linux/man-pages/man5/proc.5.html
 // for /proc/[pid]/mountinfo
-func listProcSelfMountinfo(mountFilePath string) ([]MountInfo, error) {
+func listProcMountinfo(mountFilePath string) ([]MountInfo, error) {
 	content, err := ConsistentRead(mountFilePath, maxListTries)
 	if err != nil {
 		return nil, err
 	}
-	return parseProcSelfMountinfo(content)
+	return parseProcMountinfo(content)
 }
 
-// parseProcSelfMountinfo parses the output of /proc/self/mountinfo file into a slice of MountInfo struct
-func parseProcSelfMountinfo(content []byte) ([]MountInfo, error) {
+// parseProcMountinfo parses the output of /proc/self/mountinfo file into a slice of MountInfo struct
+func parseProcMountinfo(content []byte) ([]MountInfo, error) {
 	out := make([]MountInfo, 0)
 	lines := strings.Split(string(content), "\n")
 	for _, line := range lines {

utils/mount_darwin.go

@@ -99,11 +99,18 @@ func RemountDevice(ctx context.Context, mountpoint, options string) (err error)
 	return UnsupportedError("RemountDevice is not supported on non-linux platform")
 }
 
-// GetMountInfo is a dummy added for compilation on non-linux platform.
-func GetMountInfo(ctx context.Context) ([]MountInfo, error) {
-	Logc(ctx).Debug(">>>> mount_darwin.GetMountInfo")
-	defer Logc(ctx).Debug("<<<< mount_darwin.GetMountInfo")
-	return nil, UnsupportedError("GetMountInfo is not supported on non-linux platform")
+// GetSelfMountInfo is a dummy added for compilation on non-linux platform.
+func GetSelfMountInfo(ctx context.Context) ([]MountInfo, error) {
+	Logc(ctx).Debug(">>>> mount_darwin.GetSelfMountInfo")
+	defer Logc(ctx).Debug("<<<< mount_darwin.GetSelfMountInfo")
+	return nil, UnsupportedError("GetSelfMountInfo is not supported on non-linux platform")
+}
+
+// GetHostMountInfo is a dummy added for compilation on non-linux platform.
+func GetHostMountInfo(ctx context.Context) ([]MountInfo, error) {
+	Logc(ctx).Debug(">>>> mount_darwin.GetHostMountInfo")
+	defer Logc(ctx).Debug("<<<< mount_darwin.GetHostMountInfo")
+	return nil, UnsupportedError("GetHostMountInfo is not supported on non-linux platform")
 }
 
 // IsCompatible checks for compatibility of protocol and platform