diff --git a/cmd/minikube/cmd/config/util_test.go b/cmd/minikube/cmd/config/util_test.go index 5a47c1579832..f1627f19fa82 100644 --- a/cmd/minikube/cmd/config/util_test.go +++ b/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) } } } diff --git a/pkg/minikube/bootstrapper/certs.go b/pkg/minikube/bootstrapper/certs.go index 6702a6b1220f..611b5416cce8 100644 --- a/pkg/minikube/bootstrapper/certs.go +++ b/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") } diff --git a/pkg/minikube/config/profile.go b/pkg/minikube/config/profile.go index 3546ec2e9341..22ad511d512d 100644 --- a/pkg/minikube/config/profile.go +++ b/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 } diff --git a/pkg/minikube/kubeconfig/settings.go b/pkg/minikube/kubeconfig/settings.go index 28601d0b6e70..0cd09ec542ec 100644 --- a/pkg/minikube/kubeconfig/settings.go +++ b/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 { diff --git a/pkg/util/lock/lock.go b/pkg/util/lock/lock.go index 513eaaf1c5ec..5083fa0d8cb4 100644 --- a/pkg/util/lock/lock.go +++ b/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 } diff --git a/pkg/util/lock/lock_test.go b/pkg/util/lock/lock_test.go index 8c84321d4e62..8bb97d046578 100644 --- a/pkg/util/lock/lock_test.go +++ b/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) } }) }