Merge pull request #5912 from tstromberg/user-mutex2

Make lock names uid and path specific to avoid conflicts

cmd/minikube/cmd/config/util_test.go

@@ -122,17 +122,16 @@ func TestValidateProfile(t *testing.T) {
 			profileName: "82374328742_2974224498",
 		},
 		{
-			profileName: "minikube",
+			profileName: "validate_test",
 		},
 	}
 
 	for _, test := range testCases {
 		profileNam := test.profileName
-		expectedMsg := fmt.Sprintf("profile %q not found", test.profileName)
-
+		expected := fmt.Sprintf("profile %q not found", test.profileName)
 		err, ok := ValidateProfile(profileNam)
-		if !ok && err.Error() != expectedMsg {
-			t.Errorf("Didnt receive expected message")
+		if !ok && err.Error() != expected {
+			t.Errorf("got error %q, expected %q", err, expected)
 		}
 	}
 }

pkg/minikube/bootstrapper/certs.go

@@ -26,7 +26,6 @@ import (
 	"path"
 	"path/filepath"
 	"strings"
-	"time"
 
 	"github.com/golang/glog"
 	"github.com/pkg/errors"
@@ -40,8 +39,8 @@ import (
 	"k8s.io/minikube/pkg/minikube/localpath"
 	"k8s.io/minikube/pkg/minikube/vmpath"
 	"k8s.io/minikube/pkg/util"
+	"k8s.io/minikube/pkg/util/lock"
 
-	"github.com/juju/clock"
 	"github.com/juju/mutex"
 )
 
@@ -61,17 +60,17 @@ var (
 
 // SetupCerts gets the generated credentials required to talk to the APIServer.
 func SetupCerts(cmd command.Runner, k8s config.KubernetesConfig) error {
+
+	localPath := localpath.MiniPath()
+	glog.Infof("Setting up %s for IP: %s\n", localPath, k8s.NodeIP)
+
 	// WARNING: This function was not designed for multiple profiles, so it is VERY racey:
 	//
 	// It updates a shared certificate file and uploads it to the apiserver before launch.
 	//
 	// If another process updates the shared certificate, it's invalid.
 	// TODO: Instead of racey manipulation of a shared certificate, use per-profile certs
-	spec := mutex.Spec{
-		Name:  "setupCerts",
-		Clock: clock.WallClock,
-		Delay: 15 * time.Second,
-	}
+	spec := lock.UserMutexSpec(filepath.Join(localPath, "certs"))
 	glog.Infof("acquiring lock: %+v", spec)
 	releaser, err := mutex.Acquire(spec)
 	if err != nil {
@@ -79,9 +78,6 @@ func SetupCerts(cmd command.Runner, k8s config.KubernetesConfig) error {
 	}
 	defer releaser.Release()
 
-	localPath := localpath.MiniPath()
-	glog.Infof("Setting up %s for IP: %s\n", localPath, k8s.NodeIP)
-
 	if err := generateCerts(k8s); err != nil {
 		return errors.Wrap(err, "Error generating certs")
 	}

pkg/minikube/config/profile.go

@@ -105,7 +105,7 @@ func CreateProfile(name string, cfg *MachineConfig, miniHome ...string) error {
 	}
 	defer os.Remove(tf.Name())
 
-	if err = lock.WriteFile(tf.Name(), data, 0600); err != nil {
+	if err = ioutil.WriteFile(tf.Name(), data, 0600); err != nil {
 		return err
 	}
 

pkg/minikube/kubeconfig/settings.go

@@ -18,14 +18,14 @@ package kubeconfig
 
 import (
 	"io/ioutil"
+	"path/filepath"
 	"sync/atomic"
-	"time"
 
 	"github.com/golang/glog"
-	"github.com/juju/clock"
 	"github.com/juju/mutex"
 	"github.com/pkg/errors"
 	"k8s.io/client-go/tools/clientcmd/api"
+	"k8s.io/minikube/pkg/util/lock"
 )
 
 // Settings is the minikubes settings for kubeconfig
@@ -119,8 +119,7 @@ func PopulateFromSettings(cfg *Settings, apiCfg *api.Config) error {
 // activeContext is true when minikube is the CurrentContext
 // If no CurrentContext is set, the given name will be used.
 func Update(kcs *Settings) error {
-	// Add a lock around both the read, update, and write operations
-	spec := mutex.Spec{Name: "kubeconfigUpdate", Clock: clock.WallClock, Delay: 10 * time.Second}
+	spec := lock.UserMutexSpec(filepath.Join(kcs.filePath(), "settings.Update"))
 	glog.Infof("acquiring lock: %+v", spec)
 	releaser, err := mutex.Acquire(spec)
 	if err != nil {

pkg/util/lock/lock.go

@@ -20,8 +20,8 @@ import (
 	"fmt"
 	"io/ioutil"
 	"os"
-	"path/filepath"
-	"strconv"
+	"os/user"
+	"regexp"
 	"strings"
 	"time"
 
@@ -31,15 +31,17 @@ import (
 	"github.com/pkg/errors"
 )
 
+var (
+	// nonString is characters to strip from lock names
+	nonString = regexp.MustCompile(`[\W_]+`)
+	// forceID is a user id for consistent testing
+	forceID = ""
+)
+
 // WriteFile decorates ioutil.WriteFile with a file lock and retry
 func WriteFile(filename string, data []byte, perm os.FileMode) error {
-	spec := mutex.Spec{
-		Name:  getMutexName(filename),
-		Clock: clock.WallClock,
-		Delay: 13 * time.Second,
-	}
-	glog.Infof("attempting to write to file %q with filemode %v", filename, perm)
-
+	spec := UserMutexSpec(filename)
+	glog.Infof("acquiring lock for %s: %+v", filename, spec)
 	releaser, err := mutex.Acquire(spec)
 	if err != nil {
 		return errors.Wrapf(err, "error acquiring lock for %s", filename)
@@ -50,34 +52,39 @@ func WriteFile(filename string, data []byte, perm os.FileMode) error {
 	if err = ioutil.WriteFile(filename, data, perm); err != nil {
 		return errors.Wrapf(err, "error writing file %s", filename)
 	}
-
 	return err
 }
 
-func getMutexName(filename string) string {
-	// Make the mutex name the file name and its parent directory
-	dir, name := filepath.Split(filename)
-
-	// Replace underscores and periods with dashes, the only valid punctuation for mutex name
-	name = strings.ReplaceAll(name, ".", "-")
-	name = strings.ReplaceAll(name, "_", "-")
-
-	p := strings.ReplaceAll(filepath.Base(dir), ".", "-")
-	p = strings.ReplaceAll(p, "_", "-")
-	mutexName := fmt.Sprintf("%s-%s", p, strings.ReplaceAll(name, ".", "-"))
-
-	// Check if name starts with an int and prepend a string instead
-	if _, err := strconv.Atoi(mutexName[:1]); err == nil {
-		mutexName = "m" + mutexName
+// UserMutexSpec returns a mutex spec that will not collide with other users
+func UserMutexSpec(path string) mutex.Spec {
+	id := forceID
+	if forceID == "" {
+		u, err := user.Current()
+		if err == nil {
+			id = u.Uid
+		}
 	}
-	// There's an arbitrary hard max on mutex name at 40.
-	if len(mutexName) > 40 {
-		mutexName = mutexName[:40]
+
+	s := mutex.Spec{
+		Name:  getMutexNameForPath(fmt.Sprintf("%s-%s", path, id)),
+		Clock: clock.WallClock,
+		// Poll the lock twice a second
+		Delay: 500 * time.Millisecond,
+		// panic after a minute instead of locking infinitely
+		Timeout: 60 * time.Second,
 	}
+	return s
+}
 
-	// Make sure name doesn't start or end with punctuation
-	mutexName = strings.TrimPrefix(mutexName, "-")
-	mutexName = strings.TrimSuffix(mutexName, "-")
+func getMutexNameForPath(path string) string {
+	// juju requires that names match ^[a-zA-Z][a-zA-Z0-9-]*$", and be under 40 chars long.
+	n := strings.Trim(nonString.ReplaceAllString(path, "-"), "-")
+	// we need to always guarantee an alphanumeric prefix
+	prefix := "m"
 
-	return mutexName
+	// Prefer the last 40 chars, as paths tend get more specific toward the end
+	if len(n) >= 40 {
+		return prefix + n[len(n)-39:]
+	}
+	return prefix + n
 }

pkg/util/lock/lock_test.go

@@ -18,7 +18,9 @@ package lock
 
 import "testing"
 
-func TestGetMutexName(t *testing.T) {
+func TestUserMutexSpec(t *testing.T) {
+	forceID = "test"
+
 	var tests = []struct {
 		description string
 		path        string
@@ -27,40 +29,40 @@ func TestGetMutexName(t *testing.T) {
 		{
 			description: "standard",
 			path:        "/foo/bar",
-			expected:    "foo-bar",
+			expected:    "mfoo-bar-test",
 		},
 		{
 			description: "deep directory",
 			path:        "/foo/bar/baz/bat",
-			expected:    "baz-bat",
+			expected:    "mfoo-bar-baz-bat-test",
 		},
 		{
 			description: "underscores",
 			path:        "/foo_bar/baz",
-			expected:    "foo-bar-baz",
+			expected:    "mfoo-bar-baz-test",
 		},
 		{
 			description: "starts with number",
 			path:        "/foo/2bar/baz",
-			expected:    "m2bar-baz",
+			expected:    "mfoo-2bar-baz-test",
 		},
 		{
 			description: "starts with punctuation",
 			path:        "/.foo/bar",
-			expected:    "foo-bar",
+			expected:    "mfoo-bar-test",
 		},
 		{
 			description: "long filename",
 			path:        "/very-very-very-very-very-very-very-very-long/bar",
-			expected:    "very-very-very-very-very-very-very-very",
+			expected:    "m-very-very-very-very-very-long-bar-test",
 		},
 	}
 
 	for _, tc := range tests {
 		t.Run(tc.description, func(t *testing.T) {
-			got := getMutexName(tc.path)
-			if got != tc.expected {
-				t.Errorf("Unexpected mutex name for path %s. got: %s, expected: %s", tc.path, got, tc.expected)
+			got := UserMutexSpec(tc.path)
+			if got.Name != tc.expected {
+				t.Errorf("%s mutex name = %q, expected %q", tc.path, got.Name, tc.expected)
 			}
 		})
 	}