From e22b268f7f159d4c3f1e77b44697377105ddd5e0 Mon Sep 17 00:00:00 2001 From: Dongsu Park Date: Fri, 24 Feb 2017 13:13:41 +0100 Subject: [PATCH 1/3] pkg: fix data race around HasASessionRequested TestCreateSSHShell fails when running "go test --race", because of concurrent accesses from multiple goroutines. ``` WARNING: DATA RACE Read at 0x00c42025b730 by goroutine 42: k8s.io/minikube/pkg/minikube/cluster.TestCreateSSHShell() k8s.io/minikube/pkg/minikube/cluster/cluster_test.go:523 +0x543 testing.tRunner() /usr/local/golang/src/testing/testing.go:657 +0x107 Previous write at 0x00c42025b730 by goroutine 49: k8s.io/minikube/pkg/minikube/tests.(*SSHServer).Start.func1.1() k8s.io/minikube/pkg/minikube/tests/ssh_mock.go:95 +0x743 ``` To fix that, convert HadASessionRequested to an atomic variable. Callers should run helper functions, SetSessionRequested() and IsSessionRequested() instead of direct access to the variable. --- pkg/minikube/cluster/cluster_test.go | 2 +- pkg/minikube/tests/ssh_mock.go | 25 ++++++++++++++++++++----- 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/pkg/minikube/cluster/cluster_test.go b/pkg/minikube/cluster/cluster_test.go index 42d7129cbf..a6645c852c 100644 --- a/pkg/minikube/cluster/cluster_test.go +++ b/pkg/minikube/cluster/cluster_test.go @@ -520,7 +520,7 @@ func TestCreateSSHShell(t *testing.T) { t.Fatalf("Error running ssh command: %s", err) } - if !s.HadASessionRequested { + if !s.IsSessionRequested() { t.Fatalf("Expected ssh session to be run") } } diff --git a/pkg/minikube/tests/ssh_mock.go b/pkg/minikube/tests/ssh_mock.go index f5c7b11ba7..61b142f9c9 100644 --- a/pkg/minikube/tests/ssh_mock.go +++ b/pkg/minikube/tests/ssh_mock.go @@ -23,6 +23,7 @@ import ( "io" "net" "strconv" + "sync/atomic" "github.com/golang/glog" "github.com/pkg/errors" @@ -33,10 +34,11 @@ import ( type SSHServer struct { Config *ssh.ServerConfig // Commands stores the raw commands executed against the server. - Commands map[string]int - Connected bool - Transfers *bytes.Buffer - HadASessionRequested bool + Commands map[string]int + Connected bool + Transfers *bytes.Buffer + // Only access this with atomic ops + hadASessionRequested int32 // CommandsToOutput can be used to mock what the SSHServer returns for a given command CommandToOutput map[string]string } @@ -59,6 +61,7 @@ func NewSSHServer() (*SSHServer, error) { return nil, errors.Wrap(err, "Error creating signer from key") } s.Config.AddHostKey(signer) + s.SetSessionRequested(false) return s, nil } @@ -92,7 +95,7 @@ func (s *SSHServer) Start() (int, error) { // Service the incoming Channel channel. for newChannel := range chans { if newChannel.ChannelType() == "session" { - s.HadASessionRequested = true + s.SetSessionRequested(true) } channel, requests, err := newChannel.Accept() s.Connected = true @@ -136,3 +139,15 @@ func (s *SSHServer) Start() (int, error) { } return port, nil } + +func (s *SSHServer) SetSessionRequested(b bool) { + var i int32 + if b { + i = 1 + } + atomic.StoreInt32(&s.hadASessionRequested, i) +} + +func (s *SSHServer) IsSessionRequested() bool { + return atomic.LoadInt32(&s.hadASessionRequested) != 0 +} From 6cf5ba7a2851dccfc300f4e9329a92eb1a8227f6 Mon Sep 17 00:00:00 2001 From: Dongsu Park Date: Fri, 24 Feb 2017 15:40:24 +0100 Subject: [PATCH 2/3] pkg: fix data race around CommandToOutput TestGetLocalkubeStatus fails when running "go test --race", because of concurrent accesses from multiple goroutines. ``` WARNING: DATA RACE Read at 0x00c420435378 by goroutine 43: k8s.io/minikube/pkg/minikube/tests.(*SSHServer).Start.func1.1() k8s.io/minikube/pkg/minikube/tests/ssh_mock.go:122 +0x389 Previous write at 0x00c420435378 by goroutine 40: k8s.io/minikube/pkg/minikube/cluster.TestGetLocalkubeStatus() k8s.io/minikube/pkg/minikube/cluster/cluster_test.go:359 +0x540 testing.tRunner() /usr/local/golang/src/testing/testing.go:657 +0x107 ``` To fix that, convert CommandToOutput to an atomic value that stores the map. Callers should run helper functions, SetCommandToOutput() and GetCommandToOutput() instead of direct access to the value. --- pkg/minikube/cluster/cluster_test.go | 12 ++++++------ pkg/minikube/tests/ssh_mock.go | 22 +++++++++++++++++++--- 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/pkg/minikube/cluster/cluster_test.go b/pkg/minikube/cluster/cluster_test.go index a6645c852c..c32867bb90 100644 --- a/pkg/minikube/cluster/cluster_test.go +++ b/pkg/minikube/cluster/cluster_test.go @@ -355,23 +355,23 @@ func TestGetLocalkubeStatus(t *testing.T) { } api.Hosts[constants.MachineName] = &host.Host{Driver: d} - s.CommandToOutput = map[string]string{ + s.SetCommandToOutput(map[string]string{ localkubeStatusCommand: state.Running.String(), - } + }) if _, err := GetLocalkubeStatus(api); err != nil { t.Fatalf("Error getting localkube status: %s", err) } - s.CommandToOutput = map[string]string{ + s.SetCommandToOutput(map[string]string{ localkubeStatusCommand: state.Stopped.String(), - } + }) if _, err := GetLocalkubeStatus(api); err != nil { t.Fatalf("Error getting localkube status: %s", err) } - s.CommandToOutput = map[string]string{ + s.SetCommandToOutput(map[string]string{ localkubeStatusCommand: "Bad Output", - } + }) if _, err := GetLocalkubeStatus(api); err == nil { t.Fatalf("Expected error in getting localkube status as ssh returned bad output") } diff --git a/pkg/minikube/tests/ssh_mock.go b/pkg/minikube/tests/ssh_mock.go index 61b142f9c9..c394186e63 100644 --- a/pkg/minikube/tests/ssh_mock.go +++ b/pkg/minikube/tests/ssh_mock.go @@ -20,6 +20,7 @@ import ( "bytes" "crypto/rand" "crypto/rsa" + "fmt" "io" "net" "strconv" @@ -39,8 +40,9 @@ type SSHServer struct { Transfers *bytes.Buffer // Only access this with atomic ops hadASessionRequested int32 - // CommandsToOutput can be used to mock what the SSHServer returns for a given command - CommandToOutput map[string]string + // commandsToOutput can be used to mock what the SSHServer returns for a given command + // Only access this with atomic ops + commandToOutput atomic.Value } // NewSSHServer returns a NewSSHServer instance, ready for use. @@ -62,6 +64,7 @@ func NewSSHServer() (*SSHServer, error) { } s.Config.AddHostKey(signer) s.SetSessionRequested(false) + s.SetCommandToOutput(map[string]string{}) return s, nil } @@ -115,7 +118,7 @@ func (s *SSHServer) Start() (int, error) { s.Commands[cmd.Command] = 1 // Write specified command output as mocked ssh output - if val, ok := s.CommandToOutput[cmd.Command]; ok { + if val, err := s.GetCommandToOutput(cmd.Command); err == nil { channel.Write([]byte(val)) } channel.SendRequest("exit-status", false, []byte{0, 0, 0, 0}) @@ -140,6 +143,19 @@ func (s *SSHServer) Start() (int, error) { return port, nil } +func (s *SSHServer) SetCommandToOutput(cmdToOutput map[string]string) { + s.commandToOutput.Store(cmdToOutput) +} + +func (s *SSHServer) GetCommandToOutput(cmd string) (string, error) { + cmdMap := s.commandToOutput.Load().(map[string]string) + val, ok := cmdMap[cmd] + if !ok { + return "", fmt.Errorf("unavailable command %s", cmd) + } + return val, nil +} + func (s *SSHServer) SetSessionRequested(b bool) { var i int32 if b { From 4eb9e92dfff5c46e775e3e7dcfa7caead260978b Mon Sep 17 00:00:00 2001 From: Dongsu Park Date: Fri, 24 Feb 2017 17:30:07 +0100 Subject: [PATCH 3/3] pkg: fix data race around KubeConfigFile TestSetupKubeConfig fails when running "go test --race", because of concurrent accesses from multiple goroutines. ``` WARNING: DATA RACE Write at 0x00c4201cc4b8 by goroutine 16: k8s.io/minikube/pkg/minikube/kubeconfig.TestSetupKubeConfig.func1() k8s.io/minikube/pkg/minikube/kubeconfig/config_test.go:103 +0x23a testing.tRunner() /usr/local/golang/src/testing/testing.go:657 +0x107 ``` To fix that, convert KubeConfigFile to an atomic value that stores string. Callers should run helper functions, SetKubeConfigFile() and GetKubeConfigFile() instead of direct access to the value. --- cmd/minikube/cmd/start.go | 3 ++- pkg/minikube/kubeconfig/config.go | 20 +++++++++++++++----- pkg/minikube/kubeconfig/config_test.go | 6 +++--- 3 files changed, 20 insertions(+), 9 deletions(-) diff --git a/cmd/minikube/cmd/start.go b/cmd/minikube/cmd/start.go index eef9202acb..de95b97497 100644 --- a/cmd/minikube/cmd/start.go +++ b/cmd/minikube/cmd/start.go @@ -177,8 +177,9 @@ func runStart(cmd *cobra.Command, args []string) { ClientKey: constants.MakeMiniPath("apiserver.key"), CertificateAuthority: constants.MakeMiniPath("ca.crt"), KeepContext: viper.GetBool(keepContext), - KubeConfigFile: kubeConfigFile, } + kubeCfgSetup.SetKubeConfigFile(kubeConfigFile) + if err := kubeconfig.SetupKubeConfig(kubeCfgSetup); err != nil { glog.Errorln("Error setting up kubeconfig: ", err) cmdUtil.MaybeReportErrorAndExit(err) diff --git a/pkg/minikube/kubeconfig/config.go b/pkg/minikube/kubeconfig/config.go index c186f5b781..c85ba7ac48 100644 --- a/pkg/minikube/kubeconfig/config.go +++ b/pkg/minikube/kubeconfig/config.go @@ -20,6 +20,7 @@ import ( "io/ioutil" "os" "path/filepath" + "sync/atomic" "github.com/golang/glog" "github.com/pkg/errors" @@ -47,18 +48,27 @@ type KubeConfigSetup struct { // Should the current context be kept when setting up this one KeepContext bool - // KubeConfigFile is the path where the kube config is stored - KubeConfigFile string + // kubeConfigFile is the path where the kube config is stored + // Only access this with atomic ops + kubeConfigFile atomic.Value +} + +func (k *KubeConfigSetup) SetKubeConfigFile(kubeConfigFile string) { + k.kubeConfigFile.Store(kubeConfigFile) +} + +func (k *KubeConfigSetup) GetKubeConfigFile() string { + return k.kubeConfigFile.Load().(string) } // SetupKubeconfig reads config from disk, adds the minikube settings, and writes it back. // activeContext is true when minikube is the CurrentContext // If no CurrentContext is set, the given name will be used. func SetupKubeConfig(cfg *KubeConfigSetup) error { - glog.Infoln("Using kubeconfig: ", cfg.KubeConfigFile) + glog.Infoln("Using kubeconfig: ", cfg.GetKubeConfigFile()) // read existing config or create new if does not exist - config, err := ReadConfigOrNew(cfg.KubeConfigFile) + config, err := ReadConfigOrNew(cfg.GetKubeConfigFile()) if err != nil { return err } @@ -89,7 +99,7 @@ func SetupKubeConfig(cfg *KubeConfigSetup) error { } // write back to disk - if err := WriteConfig(config, cfg.KubeConfigFile); err != nil { + if err := WriteConfig(config, cfg.GetKubeConfigFile()); err != nil { return err } return nil diff --git a/pkg/minikube/kubeconfig/config_test.go b/pkg/minikube/kubeconfig/config_test.go index 59ba7ac279..4d5887082a 100644 --- a/pkg/minikube/kubeconfig/config_test.go +++ b/pkg/minikube/kubeconfig/config_test.go @@ -100,9 +100,9 @@ func TestSetupKubeConfig(t *testing.T) { if err != nil { t.Fatalf("Error making temp directory %s", err) } - test.cfg.KubeConfigFile = filepath.Join(tmpDir, "kubeconfig") + test.cfg.SetKubeConfigFile(filepath.Join(tmpDir, "kubeconfig")) if len(test.existingCfg) != 0 { - ioutil.WriteFile(test.cfg.KubeConfigFile, test.existingCfg, 0600) + ioutil.WriteFile(test.cfg.GetKubeConfigFile(), test.existingCfg, 0600) } err = SetupKubeConfig(test.cfg) if err != nil && !test.err { @@ -111,7 +111,7 @@ func TestSetupKubeConfig(t *testing.T) { if err == nil && test.err { t.Errorf("Expected error but got none") } - config, err := ReadConfigOrNew(test.cfg.KubeConfigFile) + config, err := ReadConfigOrNew(test.cfg.GetKubeConfigFile()) if err != nil { t.Errorf("Error reading kubeconfig file: %s", err) }