Avoid concurrent LVM CLI invocations

README.md

@@ -142,35 +142,37 @@ It is expected that the Plugin Supervisor will launch the binary using the appro
 $ ./csilvm --help
 Usage of ./csilvm:
   -default-fs string
-      The default filesystem to format new volumes with (default "xfs")
+    	The default filesystem to format new volumes with (default "xfs")
   -default-volume-size uint
-      The default volume size in bytes (default 10737418240)
+    	The default volume size in bytes (default 10737418240)
   -devices string
-      A comma-seperated list of devices in the volume group
+    	A comma-seperated list of devices in the volume group
+  -lockfile string
+    	The path to the lock file used to prevent concurrent lvm invocation by multiple csilvm instances
   -node-id string
-      The node ID reported via the CSI Node gRPC service
+    	The node ID reported via the CSI Node gRPC service
   -probe-module value
-      Probe checks that the kernel module is loaded
+    	Probe checks that the kernel module is loaded
   -remove-volume-group
-      If set, the volume group will be removed when ProbeNode is called.
+    	If set, the volume group will be removed when ProbeNode is called.
   -request-limit int
-      Limits backlog of pending requests. (default 10)
+    	Limits backlog of pending requests. (default 10)
   -statsd-format string
-      The statsd format to use (one of: classic, datadog) (default "datadog")
+    	The statsd format to use (one of: classic, datadog) (default "datadog")
   -statsd-max-udp-size int
-      The size to buffer before transmitting a statsd UDP packet (default 1432)
+    	The size to buffer before transmitting a statsd UDP packet (default 1432)
   -statsd-udp-host-env-var string
-      The name of the environment variable containing the host where a statsd service is listening for stats over UDP
+    	The name of the environment variable containing the host where a statsd service is listening for stats over UDP
   -statsd-udp-port-env-var string
-      The name of the environment variable containing the port where a statsd service is listening for stats over UDP
+    	The name of the environment variable containing the port where a statsd service is listening for stats over UDP
   -tag value
-      Value to tag the volume group with (can be given multiple times)
+    	Value to tag the volume group with (can be given multiple times)
   -unix-addr string
-      The path to the listening unix socket file
+    	The path to the listening unix socket file
   -unix-addr-env string
-      An optional environment variable from which to read the unix-addr
+    	An optional environment variable from which to read the unix-addr
   -volume-group string
-      The name of the volume group to manage
+    	The name of the volume group to manage
 ```
 
 
@@ -182,6 +184,22 @@ The unix socket path can also be specified using the `-unix-addr-env=<env-var-na
 It is expected that the CO will connect to the plugin through the unix socket and will subsequently communicate with it in accordance with the CSI specification.
 
 
+### Locking
+
+Any command-line invocations executed by the `csilvm` process first acquires a
+lock on a lockfile. The path to the lockfile can be overridden using the
+`-lockfile=` option or the `CSILVM_LOCKFILE_PATH` environment variable. The
+locking behaviour can be disabled by setting the `-lockfile=` option to the
+empty string. The purpose of locking around command-line invocations is to
+prevent multiple `csilvm` processes from executing concurrent `lvm2` commands.
+This works around deadlocks in lvm2. For example,
+https://jira.mesosphere.com/browse/DCOS_OSS-5434 and
+https://github.com/lvmteam/lvm2/issues/23.
+
+By default the lock file is created at `/run/csilvm.lock` so it is assumed that
+the `/run` directory exists and is writable by the `csilvm` process.
+
+
 ### Logging
 
 The plugin emits fairly verbose logs to `STDERR`.

cmd/csilvm/csilvm.go

@@ -59,6 +59,7 @@ func main() {
 	var probeModulesF stringsFlag
 	flag.Var(&probeModulesF, "probe-module", "Probe checks that the kernel module is loaded")
 	nodeIDF := flag.String("node-id", "", "The node ID reported via the CSI Node gRPC service")
+	lockFilePathF := flag.String("lockfile", "/run/csilvm.lock", "The path to the lock file used to prevent concurrent lvm invocation by multiple csilvm instances")
 	// Metrics-related flags
 	statsdUDPHostEnvVarF := flag.String("statsd-udp-host-env-var", "", "The name of the environment variable containing the host where a statsd service is listening for stats over UDP")
 	statsdUDPPortEnvVarF := flag.String("statsd-udp-port-env-var", "", "The name of the environment variable containing the port where a statsd service is listening for stats over UDP")
@@ -71,6 +72,15 @@ func main() {
 	logger := log.New(os.Stderr, logprefix, logflags)
 	csilvm.SetLogger(logger)
 	lvm.SetLogger(logger)
+	// Setup LVM operation lock file.
+	// See
+	// - https://jira.mesosphere.com/browse/DCOS_OSS-5434
+	// - https://github.com/lvmteam/lvm2/issues/23
+	if *lockFilePathF != "" {
+		lvm.SetLockFilePath(*lockFilePathF)
+	} else if path := os.Getenv("CSILVM_LOCKFILE_PATH"); path != "" {
+		lvm.SetLockFilePath(path)
+	}
 	// Determine listen address.
 	if *socketFileF != "" && *socketFileEnvF != "" {
 		logger.Fatalf("cannot specify -unix-addr and -unix-addr-env")
@@ -85,7 +95,7 @@ func main() {
 	// Unlink the domain socket in case it is left lying around from a
 	// previous run. err return is not really interesting because it is
 	// normal for this to fail if the process is starting for the first time.
-	logger.Printf("Unlinking %s", sock)
+	logger.Printf("Unlinking socket file: %q", sock)
 	syscall.Unlink(sock)
 	// Setup socket listener
 	lis, err := net.Listen("unix", sock)

pkg/lvm/lock.go

@@ -0,0 +1,30 @@
+package lvm
+
+import (
+	"fmt"
+
+	"github.com/gofrs/flock"
+)
+
+var lvmlock *flock.Flock
+
+// SetLockFilePath sets the path to the LOCK file to use for preventing
+// concurrent invocations of LVM command-line utilities.
+//
+// See
+// - https://jira.mesosphere.com/browse/DCOS_OSS-5434
+// - https://github.com/lvmteam/lvm2/issues/23
+func SetLockFilePath(filepath string) {
+	log.Printf("using lock file at %q", filepath)
+	lvmlock = flock.New(filepath)
+	log.Printf("checking if lock can be acquired")
+	err := lvmlock.Lock()
+	if err != nil {
+		panic(fmt.Sprintf("cannot acquire lock: %v", err))
+	}
+	log.Printf("lock acquired, releasing lock")
+	if err := lvmlock.Unlock(); err != nil {
+		panic(fmt.Sprintf("cannot release lock: %v", err))
+	}
+	log.Printf("configured lock file")
+}

pkg/lvm/lvm.go

@@ -850,6 +850,18 @@ func LookupPhysicalVolume(name string) (*PhysicalVolume, error) {
 // https://github.com/Jajcus/lvm2/blob/266d6564d7a72fcff5b25367b7a95424ccf8089e/lib/metadata/metadata.c#L983
 
 func run(cmd string, v interface{}, extraArgs ...string) error {
+	// lvmlock can be nil, as it is a global variable that is intended to be
+	// initialized from calling code outside this package. We have no way of
+	// knowing whether the caller performed that initialization and must
+	// defensively check. In the future, we may decide to simply panic with a
+	// nil pointer dereference.
+	if lvmlock != nil {
+		// We use Lock instead of TryLock as we have no alternative way of
+		// making progress. We expect lvm2 command-line utilities invoked by
+		// this package to return within a reasonable amount of time.
+		lvmlock.Lock()
+		defer lvmlock.Unlock()
+	}
 	var args []string
 	if v != nil {
 		args = append(args, "--reportformat=json")

pkg/lvm/lvm_test.go

@@ -4,6 +4,7 @@ package lvm
 
 import (
 	"fmt"
+	"io/ioutil"
 	"reflect"
 	"sort"
 	"strings"
@@ -18,6 +19,16 @@ const (
 	pvsize = 100 << 20
 )
 
+func init() {
+	// Set the lock file to use in tests.
+	file, err := ioutil.TempFile("", "csilvm-test-lock-file")
+	if err != nil {
+		panic(err)
+	}
+	file.Close()
+	SetLockFilePath(file.Name())
+}
+
 func TestCreatePhysicalDevice(t *testing.T) {
 	loop, err := CreateLoopDevice(pvsize)
 	if err != nil {