From 4b8b2bc6dbe5e03e598237f0a9efc543ed27c16f Mon Sep 17 00:00:00 2001 From: Yanshu Zhao Date: Sat, 9 Jan 2021 18:46:40 +0000 Subject: [PATCH 1/8] Fix flackness of docker-env in parrallel --- cmd/minikube/cmd/delete.go | 9 +++++++++ cmd/minikube/cmd/docker-env.go | 2 +- pkg/provision/provision.go | 36 +++++++++++++++++++++++++++++++++- 3 files changed, 45 insertions(+), 2 deletions(-) diff --git a/cmd/minikube/cmd/delete.go b/cmd/minikube/cmd/delete.go index 7bfe9bfda21e..ac300620ab90 100644 --- a/cmd/minikube/cmd/delete.go +++ b/cmd/minikube/cmd/delete.go @@ -495,6 +495,15 @@ func deleteProfileDirectory(profile string) { exit.Error(reason.GuestProfileDeletion, "Unable to remove machine directory", err) } } + + certDir := filepath.Join(localpath.MiniPath(), profile) + if _, err := os.Stat(certDir); err == nil { + out.Step(style.DeletingHost, `Removing {{.directory}} ...`, out.V{"directory": certDir}) + err := os.RemoveAll(certDir) + if err != nil { + exit.Error(reason.GuestProfileDeletion, "Unable to remove machine directory", err) + } + } } func deleteMachineDirectories(cc *config.ClusterConfig) { diff --git a/cmd/minikube/cmd/docker-env.go b/cmd/minikube/cmd/docker-env.go index 3cfced47bc5a..d79928a3575f 100644 --- a/cmd/minikube/cmd/docker-env.go +++ b/cmd/minikube/cmd/docker-env.go @@ -306,7 +306,7 @@ var dockerEnvCmd = &cobra.Command{ ssh: sshHost, hostIP: hostIP, port: port, - certsDir: localpath.MakeMiniPath("certs"), + certsDir: localpath.MakeMiniPath(cname), noProxy: noProxy, username: d.GetSSHUsername(), hostname: hostname, diff --git a/pkg/provision/provision.go b/pkg/provision/provision.go index 20fa0ddb65bd..72f3d2279356 100644 --- a/pkg/provision/provision.go +++ b/pkg/provision/provision.go @@ -40,6 +40,7 @@ import ( "k8s.io/minikube/pkg/minikube/assets" "k8s.io/minikube/pkg/minikube/command" "k8s.io/minikube/pkg/minikube/config" + "k8s.io/minikube/pkg/minikube/localpath" ) // generic interface for minikube provisioner @@ -102,7 +103,11 @@ func configureAuth(p miniProvisioner) error { return errors.Wrap(err, "error getting ssh hostname during provisioning") } - if err := copyHostCerts(authOptions); err != nil { + //if err := copyHostCerts(authOptions); err != nil { + // return err + //} + + if err := copyCertsForDockEnv(authOptions, machineName); err != nil { return err } @@ -161,6 +166,35 @@ func copyHostCerts(authOptions auth.Options) error { return nil } +func copyCertsForDockEnv(authOptions auth.Options, machineName string) error { + klog.Infof("copyCertsForDockEnv") + + storePath := localpath.MakeMiniPath(machineName) + err := os.MkdirAll(storePath, 0700) + if err != nil { + klog.Errorf("mkdir failed: %v", err) + } + + hostCerts := map[string]string{ + authOptions.CaCertPath: path.Join(storePath, "ca.pem"), + authOptions.ClientCertPath: path.Join(storePath, "cert.pem"), + authOptions.ClientKeyPath: path.Join(storePath, "key.pem"), + } + + execRunner := command.NewExecRunner(false) + for src, dst := range hostCerts { + f, err := assets.NewFileAsset(src, path.Dir(dst), filepath.Base(dst), "0777") + if err != nil { + return errors.Wrapf(err, "open cert file: %s", src) + } + if err := execRunner.Copy(f); err != nil { + return errors.Wrapf(err, "transferring file: %+v", f) + } + } + + return nil +} + func copyRemoteCerts(authOptions auth.Options, driver drivers.Driver) error { klog.Infof("copyRemoteCerts") From baf929b14e7fca3f25407e5b97f2b5dae791ce4d Mon Sep 17 00:00:00 2001 From: Yanshu Zhao Date: Wed, 13 Jan 2021 07:21:31 +0000 Subject: [PATCH 2/8] Add file lock for protect certs generation --- cmd/minikube/cmd/delete.go | 9 --------- cmd/minikube/cmd/docker-env.go | 2 +- go.mod | 1 + go.sum | 2 ++ pkg/minikube/machine/client.go | 14 ++++++++++++- pkg/provision/provision.go | 36 +--------------------------------- 6 files changed, 18 insertions(+), 46 deletions(-) diff --git a/cmd/minikube/cmd/delete.go b/cmd/minikube/cmd/delete.go index ac300620ab90..7bfe9bfda21e 100644 --- a/cmd/minikube/cmd/delete.go +++ b/cmd/minikube/cmd/delete.go @@ -495,15 +495,6 @@ func deleteProfileDirectory(profile string) { exit.Error(reason.GuestProfileDeletion, "Unable to remove machine directory", err) } } - - certDir := filepath.Join(localpath.MiniPath(), profile) - if _, err := os.Stat(certDir); err == nil { - out.Step(style.DeletingHost, `Removing {{.directory}} ...`, out.V{"directory": certDir}) - err := os.RemoveAll(certDir) - if err != nil { - exit.Error(reason.GuestProfileDeletion, "Unable to remove machine directory", err) - } - } } func deleteMachineDirectories(cc *config.ClusterConfig) { diff --git a/cmd/minikube/cmd/docker-env.go b/cmd/minikube/cmd/docker-env.go index d79928a3575f..3cfced47bc5a 100644 --- a/cmd/minikube/cmd/docker-env.go +++ b/cmd/minikube/cmd/docker-env.go @@ -306,7 +306,7 @@ var dockerEnvCmd = &cobra.Command{ ssh: sshHost, hostIP: hostIP, port: port, - certsDir: localpath.MakeMiniPath(cname), + certsDir: localpath.MakeMiniPath("certs"), noProxy: noProxy, username: d.GetSSHUsername(), hostname: hostname, diff --git a/go.mod b/go.mod index 0ecc3be78e1a..eb451d571ee0 100644 --- a/go.mod +++ b/go.mod @@ -46,6 +46,7 @@ require ( github.com/johanneswuerbach/nfsexports v0.0.0-20200318065542-c48c3734757f github.com/juju/clock v0.0.0-20190205081909-9c5c9712527c github.com/juju/errors v0.0.0-20190806202954-0232dcc7464d // indirect + github.com/juju/fslock v0.0.0-20160525022230-4d5c94c67b4b github.com/juju/loggo v0.0.0-20190526231331-6e530bcce5d8 // indirect github.com/juju/mutex v0.0.0-20180619145857-d21b13acf4bf github.com/juju/retry v0.0.0-20180821225755-9058e192b216 // indirect diff --git a/go.sum b/go.sum index 1ce4cf5266c0..3a67a7831bdf 100644 --- a/go.sum +++ b/go.sum @@ -720,6 +720,8 @@ github.com/juju/clock v0.0.0-20190205081909-9c5c9712527c h1:3UvYABOQRhJAApj9MdCN github.com/juju/clock v0.0.0-20190205081909-9c5c9712527c/go.mod h1:nD0vlnrUjcjJhqN5WuCWZyzfd5AHZAC9/ajvbSx69xA= github.com/juju/errors v0.0.0-20190806202954-0232dcc7464d h1:hJXjZMxj0SWlMoQkzeZDLi2cmeiWKa7y1B8Rg+qaoEc= github.com/juju/errors v0.0.0-20190806202954-0232dcc7464d/go.mod h1:W54LbzXuIE0boCoNJfwqpmkKJ1O4TCTZMetAt6jGk7Q= +github.com/juju/fslock v0.0.0-20160525022230-4d5c94c67b4b h1:FQ7+9fxhyp82ks9vAuyPzG0/vVbWwMwLJ+P6yJI5FN8= +github.com/juju/fslock v0.0.0-20160525022230-4d5c94c67b4b/go.mod h1:HMcgvsgd0Fjj4XXDkbjdmlbI505rUPBs6WBMYg2pXks= github.com/juju/loggo v0.0.0-20190526231331-6e530bcce5d8 h1:UUHMLvzt/31azWTN/ifGWef4WUqvXk0iRqdhdy/2uzI= github.com/juju/loggo v0.0.0-20190526231331-6e530bcce5d8/go.mod h1:vgyd7OREkbtVEN/8IXZe5Ooef3LQePvuBm9UWj6ZL8U= github.com/juju/mutex v0.0.0-20180619145857-d21b13acf4bf h1:2d3cilQly1OpAfZcn4QRuwDOdVoHsM4cDTkcKbmO760= diff --git a/pkg/minikube/machine/client.go b/pkg/minikube/machine/client.go index bbc5397e22dd..1f55955f5523 100644 --- a/pkg/minikube/machine/client.go +++ b/pkg/minikube/machine/client.go @@ -40,6 +40,7 @@ import ( "github.com/docker/machine/libmachine/state" "github.com/docker/machine/libmachine/swarm" "github.com/docker/machine/libmachine/version" + "github.com/juju/fslock" "github.com/pkg/errors" "k8s.io/klog/v2" "k8s.io/minikube/pkg/minikube/command" @@ -71,6 +72,7 @@ func NewAPIClient(miniHome ...string) (libmachine.API, error) { storePath: storePath, Filestore: persist.NewFilestore(storePath, certsDir, certsDir), legacyClient: NewRPCClient(storePath, certsDir), + flock: fslock.New(localpath.MakeMiniPath("fileLock.txt")), }, nil } @@ -81,6 +83,7 @@ type LocalClient struct { storePath string *persist.Filestore legacyClient libmachine.API + flock *fslock.Lock } // NewHost creates a new Host @@ -183,7 +186,16 @@ func (api *LocalClient) Create(h *host.Host) error { }{ { "bootstrapping certificates", - func() error { return cert.BootstrapCertificates(h.AuthOptions()) }, + func() error { + // CA cert and client cert should be generated atomically, otherwise might cause bad certificate error + lockErr := api.flock.LockWithTimeout(time.Second * 5) + if lockErr != nil { + return fmt.Errorf("falied to acquire lock > " + lockErr.Error()) + } + certErr := cert.BootstrapCertificates(h.AuthOptions()) + api.flock.Unlock() + return certErr + }, }, { "precreate", diff --git a/pkg/provision/provision.go b/pkg/provision/provision.go index 72f3d2279356..20fa0ddb65bd 100644 --- a/pkg/provision/provision.go +++ b/pkg/provision/provision.go @@ -40,7 +40,6 @@ import ( "k8s.io/minikube/pkg/minikube/assets" "k8s.io/minikube/pkg/minikube/command" "k8s.io/minikube/pkg/minikube/config" - "k8s.io/minikube/pkg/minikube/localpath" ) // generic interface for minikube provisioner @@ -103,11 +102,7 @@ func configureAuth(p miniProvisioner) error { return errors.Wrap(err, "error getting ssh hostname during provisioning") } - //if err := copyHostCerts(authOptions); err != nil { - // return err - //} - - if err := copyCertsForDockEnv(authOptions, machineName); err != nil { + if err := copyHostCerts(authOptions); err != nil { return err } @@ -166,35 +161,6 @@ func copyHostCerts(authOptions auth.Options) error { return nil } -func copyCertsForDockEnv(authOptions auth.Options, machineName string) error { - klog.Infof("copyCertsForDockEnv") - - storePath := localpath.MakeMiniPath(machineName) - err := os.MkdirAll(storePath, 0700) - if err != nil { - klog.Errorf("mkdir failed: %v", err) - } - - hostCerts := map[string]string{ - authOptions.CaCertPath: path.Join(storePath, "ca.pem"), - authOptions.ClientCertPath: path.Join(storePath, "cert.pem"), - authOptions.ClientKeyPath: path.Join(storePath, "key.pem"), - } - - execRunner := command.NewExecRunner(false) - for src, dst := range hostCerts { - f, err := assets.NewFileAsset(src, path.Dir(dst), filepath.Base(dst), "0777") - if err != nil { - return errors.Wrapf(err, "open cert file: %s", src) - } - if err := execRunner.Copy(f); err != nil { - return errors.Wrapf(err, "transferring file: %+v", f) - } - } - - return nil -} - func copyRemoteCerts(authOptions auth.Options, driver drivers.Driver) error { klog.Infof("copyRemoteCerts") From cbf602e7f08b2feb758b383497c8d1e990f72da2 Mon Sep 17 00:00:00 2001 From: Yanshu Zhao Date: Wed, 13 Jan 2021 07:37:23 +0000 Subject: [PATCH 3/8] change unlock to defer --- pkg/minikube/machine/client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/minikube/machine/client.go b/pkg/minikube/machine/client.go index 1f55955f5523..25cd16cb4df4 100644 --- a/pkg/minikube/machine/client.go +++ b/pkg/minikube/machine/client.go @@ -192,8 +192,8 @@ func (api *LocalClient) Create(h *host.Host) error { if lockErr != nil { return fmt.Errorf("falied to acquire lock > " + lockErr.Error()) } + defer api.flock.Unlock() certErr := cert.BootstrapCertificates(h.AuthOptions()) - api.flock.Unlock() return certErr }, }, From 948c1b5f090a6ca895b6f92bc25bb4cf3381227c Mon Sep 17 00:00:00 2001 From: Yanshu Zhao Date: Wed, 13 Jan 2021 07:52:27 +0000 Subject: [PATCH 4/8] check unlock return value --- pkg/minikube/machine/client.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/pkg/minikube/machine/client.go b/pkg/minikube/machine/client.go index 25cd16cb4df4..aaf84359eee3 100644 --- a/pkg/minikube/machine/client.go +++ b/pkg/minikube/machine/client.go @@ -192,7 +192,12 @@ func (api *LocalClient) Create(h *host.Host) error { if lockErr != nil { return fmt.Errorf("falied to acquire lock > " + lockErr.Error()) } - defer api.flock.Unlock() + defer func() { + lockErr = api.flock.Unlock() + if lockErr != nil { + klog.Errorf("falied to release lock > %s", lockErr.Error()) + } + }() certErr := cert.BootstrapCertificates(h.AuthOptions()) return certErr }, From 376399cc32e5678c142d20016b63c83a678872e3 Mon Sep 17 00:00:00 2001 From: Yanshu Zhao Date: Wed, 13 Jan 2021 17:37:48 +0000 Subject: [PATCH 5/8] Change naming of lock and log format --- pkg/minikube/machine/client.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/minikube/machine/client.go b/pkg/minikube/machine/client.go index aaf84359eee3..73db9e0de5e1 100644 --- a/pkg/minikube/machine/client.go +++ b/pkg/minikube/machine/client.go @@ -72,7 +72,7 @@ func NewAPIClient(miniHome ...string) (libmachine.API, error) { storePath: storePath, Filestore: persist.NewFilestore(storePath, certsDir, certsDir), legacyClient: NewRPCClient(storePath, certsDir), - flock: fslock.New(localpath.MakeMiniPath("fileLock.txt")), + flock: fslock.New(localpath.MakeMiniPath("machine_client.lock")), }, nil } @@ -190,12 +190,12 @@ func (api *LocalClient) Create(h *host.Host) error { // CA cert and client cert should be generated atomically, otherwise might cause bad certificate error lockErr := api.flock.LockWithTimeout(time.Second * 5) if lockErr != nil { - return fmt.Errorf("falied to acquire lock > " + lockErr.Error()) + return fmt.Errorf("failed to acquire bootstrap client lock: %v " + lockErr.Error()) } defer func() { lockErr = api.flock.Unlock() if lockErr != nil { - klog.Errorf("falied to release lock > %s", lockErr.Error()) + klog.Errorf("falied to release bootstrap cert client lock %v", lockErr.Error()) } }() certErr := cert.BootstrapCertificates(h.AuthOptions()) From bbbf0d9bb6fd34985e4bb5f930e29453341dbba4 Mon Sep 17 00:00:00 2001 From: Yanshu Zhao Date: Wed, 13 Jan 2021 18:39:23 +0000 Subject: [PATCH 6/8] Add issue link and comment --- pkg/minikube/machine/client.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/minikube/machine/client.go b/pkg/minikube/machine/client.go index 73db9e0de5e1..59f134ece2f8 100644 --- a/pkg/minikube/machine/client.go +++ b/pkg/minikube/machine/client.go @@ -187,7 +187,9 @@ func (api *LocalClient) Create(h *host.Host) error { { "bootstrapping certificates", func() error { - // CA cert and client cert should be generated atomically, otherwise might cause bad certificate error + // Due to https://github.com/docker/machine/issues/3634, we need to have lock for certs generation, otherwise + // it will break Docker-Env test in parallel tesing. + // CA cert and client cert should be generated atomically, otherwise might cause bad certificate error. lockErr := api.flock.LockWithTimeout(time.Second * 5) if lockErr != nil { return fmt.Errorf("failed to acquire bootstrap client lock: %v " + lockErr.Error()) @@ -195,7 +197,7 @@ func (api *LocalClient) Create(h *host.Host) error { defer func() { lockErr = api.flock.Unlock() if lockErr != nil { - klog.Errorf("falied to release bootstrap cert client lock %v", lockErr.Error()) + klog.Errorf("falied to release bootstrap cert client lock: %v", lockErr.Error()) } }() certErr := cert.BootstrapCertificates(h.AuthOptions()) From 4e368f0c40fe18970707b3e49677b7340cba8d07 Mon Sep 17 00:00:00 2001 From: Yanshu Zhao Date: Wed, 13 Jan 2021 18:45:09 +0000 Subject: [PATCH 7/8] Modify comment --- pkg/minikube/machine/client.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/minikube/machine/client.go b/pkg/minikube/machine/client.go index 59f134ece2f8..221c54af4e16 100644 --- a/pkg/minikube/machine/client.go +++ b/pkg/minikube/machine/client.go @@ -187,8 +187,7 @@ func (api *LocalClient) Create(h *host.Host) error { { "bootstrapping certificates", func() error { - // Due to https://github.com/docker/machine/issues/3634, we need to have lock for certs generation, otherwise - // it will break Docker-Env test in parallel tesing. + // because issue #10107 lock is needed to avoid race conditiion minikube in paralel Docker-Env test. // CA cert and client cert should be generated atomically, otherwise might cause bad certificate error. lockErr := api.flock.LockWithTimeout(time.Second * 5) if lockErr != nil { From e933166cf2923c0df528b964222dd6ff39602935 Mon Sep 17 00:00:00 2001 From: Yanshu Zhao Date: Wed, 13 Jan 2021 18:47:12 +0000 Subject: [PATCH 8/8] Modify again --- pkg/minikube/machine/client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/minikube/machine/client.go b/pkg/minikube/machine/client.go index 221c54af4e16..1ad2263ae5d7 100644 --- a/pkg/minikube/machine/client.go +++ b/pkg/minikube/machine/client.go @@ -187,7 +187,7 @@ func (api *LocalClient) Create(h *host.Host) error { { "bootstrapping certificates", func() error { - // because issue #10107 lock is needed to avoid race conditiion minikube in paralel Docker-Env test. + // Lock is needed to avoid race conditiion in parallel Docker-Env test because issue #10107. // CA cert and client cert should be generated atomically, otherwise might cause bad certificate error. lockErr := api.flock.LockWithTimeout(time.Second * 5) if lockErr != nil {