Add CommandRunner interface #1844

r2d4 · 2017-08-18T23:57:42Z

Two implementations, SSHRunner and ExecRunner allow commands to be run
either through SSH or os.Exec respectively. This allows the cluster
bootstrappers to be unaware of how they are actually executing the
commands they need. Copy and Remove functions provide nice
convenience functions for running commands that copy and remove files
respectively.

codecov-io · 2017-08-19T00:23:41Z

Codecov Report

Merging #1844 into master will decrease coverage by 0.68%.
The diff coverage is 17.91%.

@@            Coverage Diff             @@
##           master    #1844      +/-   ##
==========================================
- Coverage    32.7%   32.02%   -0.69%     
==========================================
  Files          66       66              
  Lines        4112     3966     -146     
==========================================
- Hits         1345     1270      -75     
+ Misses       2572     2528      -44     
+ Partials      195      168      -27

Impacted Files	Coverage Δ
pkg/minikube/sshutil/sshutil.go	`54.28% <ø> (+8.92%)`	⬆️
cmd/minikube/cmd/logs.go	`12.5% <0%> (-6.25%)`	⬇️
pkg/minikube/machine/client.go	`44.66% <0%> (-2.52%)`	⬇️
cmd/minikube/cmd/config/util.go	`20.73% <0%> (+2.33%)`	⬆️
cmd/minikube/cmd/status.go	`7.93% <0%> (-1.33%)`	⬇️
cmd/minikube/cmd/start.go	`10.1% <0%> (-0.14%)`	⬇️
pkg/minikube/cluster/cluster.go	`40.94% <60%> (-2.3%)`	⬇️
pkg/minikube/cluster/localkube_caching.go	`43.13% <0%> (-11.77%)`	⬇️
pkg/util/kubeconfig/config.go	`47.61% <0%> (+0.68%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4e211c...cac45c3. Read the comment docs.

dlorenc · 2017-08-19T01:20:06Z

pkg/minikube/cluster/cluster.go

-		return errors.Wrap(err, "Error getting ip from driver")
-	}
-	glog.Infoln("Setting up certificates for IP: %s", ipStr)
+	ip := net.ParseIP(k8s.NodeIP)


Why did this change?

A little duplication here,
We already call host.GetIP() in start.go https://github.com/kubernetes/minikube/blob/master/cmd/minikube/cmd/start.go#L143-L147

and populate the KubernetesConfig with that value
https://github.com/kubernetes/minikube/blob/master/cmd/minikube/cmd/start.go#L179

dlorenc · 2017-08-19T01:22:08Z

pkg/minikube/cluster/cluster.go

@@ -232,6 +232,7 @@ func UpdateCluster(cmd bootstrapper.CommandRunner, config KubernetesConfig) erro
 	}

 	for _, f := range copyableFiles {
+		// fmt.Println(f.GetAssetName())


dlorenc · 2017-08-19T01:22:22Z

pkg/minikube/bootstrapper/fake_runner.go

@@ -103,3 +103,11 @@ func (f *FakeCommandRunner) GetFileToContents(fpath string) (string, error) {
 func (f *FakeCommandRunner) SetFileToContents(fileToContents map[string]string) {
 	f.fileToContents.Store(fileToContents)
 }
+
+func (f *FakeCommandRunner) DumpMaps(w io.Writer) {


I just added this as a nice debugging function for the fake runner, since I didn't want to actually make cmdMap and fileMap public, but it might be helpful to dump their contents on test failures.

aaron-prindle · 2017-08-21T20:46:29Z

cmd/minikube/cmd/config/disable_test.go


 func TestDisableUnknownAddon(t *testing.T) {
 	if err := Set("InvalidAddon", "false"); err == nil {
 		t.Fatalf("Disable did not return error for unknown addon")
 	}
 }
-
-func TestDisableValidAddonLocal(t *testing.T) {


Are these addon tests reimplemented somewhere else?

No, a pretty big part of theses tests were checking if the file itself was transferred, which should be covered by the SSHRunner tests.

There's probably a bit of coverage that got lost about writing the config and making sure that the addon gets configured properly.

dlorenc · 2017-08-23T17:42:23Z

pkg/minikube/bootstrapper/exec_runner.go

+
+// Copy copies a file and its permissions
+func (*ExecRunner) Copy(f assets.CopyableFile) error {
+	if err := os.MkdirAll(f.GetTargetDir(), os.ModePerm); err != nil {


What happens here if the directory already exists?

No errror, it behaviors like mkdir -p
https://golang.org/pkg/os/#MkdirAll

MkdirAll creates a directory named path, along with any necessary parents, and returns nil, or else returns an error. The permission bits perm are used for all directories that MkdirAll creates. If path is already a directory, MkdirAll does nothing and returns nil.

dlorenc · 2017-08-23T17:43:29Z

pkg/minikube/bootstrapper/fake_runner.go

+//
+// It implements the CommandRunner interface and is used for testing.
+type FakeCommandRunner struct {
+	commandToOutput atomic.Value


You might be able to use sync.Map instead?

nice. that simplifies things a lot. done

dlorenc · 2017-08-23T17:44:59Z

cmd/minikube/cmd/config/util.go

-			return err
+	if enable {
+		for _, addon := range addon.Assets {
+			cmd.Copy(addon)


Do we need to check for errors here?

dlorenc · 2017-08-23T17:45:06Z

cmd/minikube/cmd/config/util.go

-		if err = os.Remove(filepath.Join(f.GetTargetDir(), f.GetTargetName())); err != nil {
-			return err
+		for _, addon := range addon.Assets {
+			cmd.Remove(addon)


dlorenc · 2017-08-23T17:46:26Z

cmd/minikube/cmd/status.go

@@ -62,7 +62,15 @@ var statusCmd = &cobra.Command{
 		cs := state.None.String()
 		ks := state.None.String()
 		if ms == state.Running.String() {
-			cs, err = cluster.GetLocalkubeStatus(api)
+			h, err := api.Load(config.GetMachineName())


This is weird, but it looks like the permissions of this file changed to 755?

Permissions of what file?

Change the file permissions back.

dlorenc · 2017-08-23T17:46:48Z

cmd/minikube/cmd/status.go

-			cs, err = cluster.GetLocalkubeStatus(api)
+			h, err := api.Load(config.GetMachineName())
+			if err != nil {
+				glog.Exitln("Error getting host")


Should these be MaybeReportErrorandExit'ing?

Two implementations, SSHRunner and ExecRunner allow commands to be run either through SSH or os.Exec respectively. This allows the cluster bootstrappers to be unaware of how they are actually executing the commands they need. Copy and Remove functions provide a nice convenience function for running commands that copy and remove files respectively.

r2d4 · 2017-08-24T21:16:49Z

cmd/minikube/cmd/start.go

@@ -192,21 +192,26 @@ func runStart(cmd *cobra.Command, args []string) {
 		glog.Errorln("Error saving profile cluster configuration: ", err)
 	}

+	cmdRunner, err := machine.GetCommandRunner(host)
+	if err != nil {
+		glog.Errorln("Error getting command runner interface")


add maybeReportErrorAndExit

aaron-prindle · 2017-08-24T22:04:33Z

pkg/minikube/cluster/cluster.go

 	for _, f := range copyableFiles {
-		if err := sshutil.TransferFile(f, client); err != nil {
+		// fmt.Println(f.GetAssetName())


nit: remove comment

aaron-prindle · 2017-08-24T22:07:04Z

LGTM

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 18, 2017

r2d4 force-pushed the command-runner branch from cb9b4fc to aba7b99 Compare August 18, 2017 23:58

dlorenc reviewed Aug 19, 2017

View reviewed changes

r2d4 force-pushed the command-runner branch 2 times, most recently from 49942a4 to 7d8fe62 Compare August 21, 2017 05:11

aaron-prindle reviewed Aug 21, 2017

View reviewed changes

r2d4 force-pushed the command-runner branch 4 times, most recently from d9f2acf to 644b88e Compare August 23, 2017 00:13

dlorenc reviewed Aug 23, 2017

View reviewed changes

r2d4 force-pushed the command-runner branch from 737e718 to c5612f2 Compare August 23, 2017 19:16

r2d4 added 6 commits August 23, 2017 14:33

Move tests over to use FakeCommandRunner

7f558f1

Add comments for exported CommandRunner functions

8d4e1d4

Move setupCerts to use CommandRunners

fd37e6d

Review feedback

fd921b3

Use syncmap instead of sync.Atomic

cac45c3

r2d4 force-pushed the command-runner branch from 44f835f to cac45c3 Compare August 23, 2017 22:08

r2d4 commented Aug 24, 2017

View reviewed changes

aaron-prindle reviewed Aug 24, 2017

View reviewed changes

aaron-prindle approved these changes Aug 24, 2017

View reviewed changes

r2d4 merged commit 3a0d03f into kubernetes:master Aug 24, 2017

r2d4 deleted the command-runner branch August 24, 2017 22:07

r2d4 mentioned this pull request Nov 1, 2017

minikube ssh doesn't recognize the end of transmission signal from standard input #1957

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CommandRunner interface #1844

Add CommandRunner interface #1844

r2d4 commented Aug 18, 2017 •

edited

Loading

codecov-io commented Aug 19, 2017 •

edited

Loading

dlorenc Aug 19, 2017

r2d4 Aug 19, 2017

dlorenc Aug 19, 2017

r2d4 Aug 19, 2017

dlorenc Aug 19, 2017

r2d4 Aug 19, 2017

aaron-prindle Aug 21, 2017 •

edited

Loading

r2d4 Aug 21, 2017

dlorenc Aug 23, 2017

r2d4 Aug 23, 2017

dlorenc Aug 23, 2017

r2d4 Aug 23, 2017

dlorenc Aug 23, 2017

r2d4 Aug 23, 2017

dlorenc Aug 23, 2017

r2d4 Aug 23, 2017

dlorenc Aug 23, 2017

r2d4 Aug 23, 2017

r2d4 Aug 24, 2017

dlorenc Aug 23, 2017

r2d4 Aug 23, 2017

r2d4 Aug 24, 2017

aaron-prindle Aug 24, 2017

aaron-prindle commented Aug 24, 2017

Add CommandRunner interface #1844

Add CommandRunner interface #1844

Conversation

r2d4 commented Aug 18, 2017 • edited Loading

codecov-io commented Aug 19, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaron-prindle Aug 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaron-prindle commented Aug 24, 2017

r2d4 commented Aug 18, 2017 •

edited

Loading

codecov-io commented Aug 19, 2017 •

edited

Loading

aaron-prindle Aug 21, 2017 •

edited

Loading