Merge lp:~thumper/juju-core/ssh-run-on-remote into lp:~go-bot/juju-core/trunk

[Tim Penhey]

Add ssh utility function.

Adds a function that wraps ssh to execute a list of
commands on a remote machine.

The function takes a timeout parameter that kills the
connection if the command takes too long.

https://codereview.appspot.com/49940045/

Add ssh utility function.

Adds a function that wraps ssh to execute a list of
commands on a remote machine.

The function takes a timeout parameter that kills the
connection if the command takes too long.

https://codereview.appspot.com/49940045/

[Tim Penhey (thumper)]

Reviewers: mp+201095_code.launchpad.net,

Message:
Please take a look.

Description:
Add ssh utility function.

Adds a function that wraps ssh to execute a list of
commands on a remote machine.

The function takes a timeout parameter that kills the
connection if the command takes too long.

https://code.launchpad.net/~thumper/juju-core/ssh-run-on-remote/+merge/201095

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/49940045/

Affected files (+204, -0 lines):
   A [revision details]
   A utils/ssh/run.go
   A utils/ssh/run_test.go

[Andrew Wilkins (axwalk)]

https://codereview.appspot.com/49940045/diff/1/utils/ssh/run.go
File utils/ssh/run.go (right):

https://codereview.appspot.com/49940045/diff/1/utils/ssh/run.go#newcode44
utils/ssh/run.go:44: input, err := command.StdinPipe()
command.Stdin = strings.NewReader(params.Command+"\n")

https://codereview.appspot.com/49940045/diff/1/utils/ssh/run.go#newcode51
utils/ssh/run.go:51: var commandDone = make(chan error)
Style nit, I think := is appropriate here.
Also, the channel doesn't need to be created until after Start returns
successfully.

https://codereview.appspot.com/49940045/diff/1/utils/ssh/run.go#newcode59
utils/ssh/run.go:59: close(commandDone)
Maybe a bit paranoid, but I think it's good defensive programming to
defer the close at the top.

https://codereview.appspot.com/49940045/diff/1/utils/ssh/run.go#newcode67
utils/ssh/run.go:67: status :=
ee.ProcessState.Sys().(syscall.WaitStatus)
This is not going to work on the Windows CLI, is it?

You can use ee.ProcessState.Exited(), but I don't think there's a
platform-independent way of getting the exit status. A helper may be in
order.

https://codereview.appspot.com/49940045/diff/1/utils/ssh/run_test.go
File utils/ssh/run_test.go (right):

https://codereview.appspot.com/49940045/diff/1/utils/ssh/run_test.go#newcode105
utils/ssh/run_test.go:105: while read line
cat /dev/stdin?

https://codereview.appspot.com/49940045/

[Tim Penhey (thumper)]

Please take a look.

https://codereview.appspot.com/49940045/diff/1/utils/ssh/run.go
File utils/ssh/run.go (right):

https://codereview.appspot.com/49940045/diff/1/utils/ssh/run.go#newcode44
utils/ssh/run.go:44: input, err := command.StdinPipe()
On 2014/01/10 01:23:58, axw wrote:
> command.Stdin = strings.NewReader(params.Command+"\n")

Done.

https://codereview.appspot.com/49940045/diff/1/utils/ssh/run.go#newcode51
utils/ssh/run.go:51: var commandDone = make(chan error)
On 2014/01/10 01:23:58, axw wrote:
> Style nit, I think := is appropriate here.
> Also, the channel doesn't need to be created until after Start returns
> successfully.

OK.

https://codereview.appspot.com/49940045/diff/1/utils/ssh/run.go#newcode59
utils/ssh/run.go:59: close(commandDone)
On 2014/01/10 01:23:58, axw wrote:
> Maybe a bit paranoid, but I think it's good defensive programming to
defer the
> close at the top.

sure, although this is paranoia as there is no other return options.

https://codereview.appspot.com/49940045/diff/1/utils/ssh/run.go#newcode67
utils/ssh/run.go:67: status :=
ee.ProcessState.Sys().(syscall.WaitStatus)
On 2014/01/10 01:23:58, axw wrote:
> This is not going to work on the Windows CLI, is it?

> You can use ee.ProcessState.Exited(), but I don't think there's a
> platform-independent way of getting the exit status. A helper may be
in order.

Perhaps, but the CLI isn't doing this, the apiserver is.

https://codereview.appspot.com/49940045/

[Andrew Wilkins (axwalk)]

LGTM

https://codereview.appspot.com/49940045/diff/1/utils/ssh/run.go
File utils/ssh/run.go (right):

https://codereview.appspot.com/49940045/diff/1/utils/ssh/run.go#newcode67
utils/ssh/run.go:67: status :=
ee.ProcessState.Sys().(syscall.WaitStatus)
On 2014/01/10 01:57:15, thumper wrote:
> On 2014/01/10 01:23:58, axw wrote:
> > This is not going to work on the Windows CLI, is it?
> >
> > You can use ee.ProcessState.Exited(), but I don't think there's a
> > platform-independent way of getting the exit status. A helper may be
in order.

> Perhaps, but the CLI isn't doing this, the apiserver is.

Ah yes, I misunderstood the point of this. Never mind then.

https://codereview.appspot.com/49940045/

[Go Bot (go-bot)]

The attempt to merge lp:~thumper/juju-core/ssh-run-on-remote into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 1.553s
ok launchpad.net/juju-core/agent/tools 0.236s
ok launchpad.net/juju-core/bzr 7.916s
ok launchpad.net/juju-core/cert 3.278s
ok launchpad.net/juju-core/charm 0.606s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.067s
ok launchpad.net/juju-core/cloudinit/sshinit 1.109s
ok launchpad.net/juju-core/cmd 0.250s
ok launchpad.net/juju-core/cmd/charm-admin 0.818s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 181.973s
ok launchpad.net/juju-core/cmd/jujud 55.014s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 3.113s
? launchpad.net/juju-core/cmd/plugins/juju-restore [no test files]
ok launchpad.net/juju-core/constraints 0.028s
ok launchpad.net/juju-core/container 0.035s
ok launchpad.net/juju-core/container/factory 0.068s
ok launchpad.net/juju-core/container/kvm 0.304s
ok launchpad.net/juju-core/container/kvm/mock 0.037s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 0.421s
? launchpad.net/juju-core/container/lxc/mock [no test files]
? launchpad.net/juju-core/container/lxc/testing [no test files]
? launchpad.net/juju-core/container/testing [no test files]
ok launchpad.net/juju-core/downloader 5.266s
ok launchpad.net/juju-core/environs 3.257s
ok launchpad.net/juju-core/environs/bootstrap 4.600s
ok launchpad.net/juju-core/environs/cloudinit 0.827s
ok launchpad.net/juju-core/environs/config 1.118s
ok launchpad.net/juju-core/environs/configstore 0.043s
ok launchpad.net/juju-core/environs/filestorage 0.043s
ok launchpad.net/juju-core/environs/httpstorage 0.949s
ok launchpad.net/juju-core/environs/imagemetadata 0.499s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.059s
ok launchpad.net/juju-core/environs/jujutest 0.242s
ok launchpad.net/juju-core/environs/manual 11.156s
ok launchpad.net/juju-core/environs/simplestreams 0.366s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 1.332s
ok launchpad.net/juju-core/environs/storage 1.196s
ok launchpad.net/juju-core/environs/sync 30.786s
ok launchpad.net/juju-core/environs/testing 0.216s
ok launchpad.net/juju-core/environs/tools 6.757s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.016s
ok launchpad.net/juju-core/instance 0.023s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 16.437s
ok launchpad.net/juju-core/juju/osenv 0.019s
? launchpad.net/juju-core/juju/testing [no test files]
ok launchpad.net/juju-core/log 0.016s
ok launchpad.net/juju-core/log/syslog 0.025s
ok launchpad.net/juju-core/names 0.029s
? launchpad.net/juju-core/provider [no test files]
? launchpad.net/juju-core/prov...

=== added file 'utils/ssh/run.go'
--- utils/ssh/run.go	1970-01-01 00:00:00 +0000
+++ utils/ssh/run.go	2014-01-10 01:56:08 +0000
@@ -0,0 +1,80 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package ssh
+
+import (
+	"bytes"
+	"fmt"
+	"os/exec"
+	"strings"
+	"syscall"
+	"time"
+
+	"launchpad.net/juju-core/cmd"
+)
+
+// ExecParams are used for the parameters for ExecuteCommandOnMachine.
+type ExecParams struct {
+	IdentityFile string
+	Host         string
+	Command      string
+	Timeout      time.Duration
+}
+
+// ExecuteCommandOnMachine will execute the command passed through on
+// the host specified. This is done using ssh, and passing the commands
+// through /bin/bash.  If the command is not finished within the timeout
+// specified, an error is returned.  Any output captured during that time
+// is also returned in the remote response.
+func ExecuteCommandOnMachine(params ExecParams) (result cmd.RemoteResponse, err error) {
+	// execute bash accepting commands on stdin
+	if params.Host == "" {
+		return result, fmt.Errorf("missing host address")
+	}
+	logger.Debugf("execute on %s", params.Host)
+	options := []Option{NoPasswordAuthentication}
+	if params.IdentityFile != "" {
+		options = append(options, Option{"-i", params.IdentityFile})
+	}
+	command := Command(params.Host, []string{"/bin/bash", "-s"}, options...)
+	// start a go routine to do the actual execution
+	var stdout, stderr bytes.Buffer
+	command.Stdout = &stdout
+	command.Stderr = &stderr
+	command.Stdin = strings.NewReader(params.Command + "\n")
+
+	if err = command.Start(); err != nil {
+		return result, err
+	}
+	commandDone := make(chan error)
+	go func() {
+		defer close(commandDone)
+		err := command.Wait()
+		logger.Debugf("command.Wait finished: %v", err)
+		commandDone <- err
+	}()
+
+	select {
+	case err = <-commandDone:
+		logger.Debugf("select from commandDone channel: %v", err)
+		// command finished and returned us the results
+		if ee, ok := err.(*exec.ExitError); ok && err != nil {
+			status := ee.ProcessState.Sys().(syscall.WaitStatus)
+			if status.Exited() {
+				// A non-zero return code isn't considered an error here.
+				result.Code = status.ExitStatus()
+				err = nil
+			}
+		}
+
+	case <-time.After(params.Timeout):
+		logger.Infof("killing the command due to timeout")
+		err = fmt.Errorf("command timed out")
+		command.Process.Kill()
+	}
+	// In either case, gather as much as we have from stdout and stderr
+	result.Stderr = stderr.Bytes()
+	result.Stdout = stdout.Bytes()
+	return result, err
+}

=== added file 'utils/ssh/run_test.go'
--- utils/ssh/run_test.go	1970-01-01 00:00:00 +0000
+++ utils/ssh/run_test.go	2014-01-10 01:56:08 +0000
@@ -0,0 +1,116 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package ssh_test
+
+import (
+	"io/ioutil"
+	"os"
+	"path/filepath"
+
+	gc "launchpad.net/gocheck"
+
+	"launchpad.net/juju-core/testing"
+	jc "launchpad.net/juju-core/testing/checkers"
+	"launchpad.net/juju-core/testing/testbase"
+	"launchpad.net/juju-core/utils/ssh"
+)
+
+type ExecuteSSHCommandSuite struct {
+	testbase.LoggingSuite
+	testbin string
+	fakessh string
+}
+
+var _ = gc.Suite(&ExecuteSSHCommandSuite{})
+
+func (s *ExecuteSSHCommandSuite) SetUpTest(c *gc.C) {
+	s.LoggingSuite.SetUpTest(c)
+	s.testbin = c.MkDir()
+	s.fakessh = filepath.Join(s.testbin, "ssh")
+	newPath := s.testbin + ":" + os.Getenv("PATH")
+	s.PatchEnvironment("PATH", newPath)
+}
+
+func (s *ExecuteSSHCommandSuite) fakeSSH(c *gc.C, cmd string) {
+	err := ioutil.WriteFile(s.fakessh, []byte(cmd), 0755)
+	c.Assert(err, gc.IsNil)
+}
+
+func (s *ExecuteSSHCommandSuite) TestCaptureOutput(c *gc.C) {
+	s.fakeSSH(c, echoSSH)
+
+	response, err := ssh.ExecuteCommandOnMachine(ssh.ExecParams{
+		Host:    "hostname",
+		Command: "sudo apt-get update\nsudo apt-get upgrade",
+		Timeout: testing.ShortWait,
+	})
+
+	c.Assert(err, gc.IsNil)
+	c.Assert(response.Code, gc.Equals, 0)
+	c.Assert(string(response.Stdout), gc.Equals, "sudo apt-get update\nsudo apt-get upgrade\n")
+	c.Assert(string(response.Stderr), gc.Equals,
+		"-o StrictHostKeyChecking no -o PasswordAuthentication no hostname -- /bin/bash -s\n")
+}
+
+func (s *ExecuteSSHCommandSuite) TestIdentityFile(c *gc.C) {
+	s.fakeSSH(c, echoSSH)
+
+	response, err := ssh.ExecuteCommandOnMachine(ssh.ExecParams{
+		IdentityFile: "identity-file",
+		Host:         "hostname",
+		Timeout:      testing.ShortWait,
+	})
+
+	c.Assert(err, gc.IsNil)
+	c.Assert(string(response.Stderr), jc.Contains, " -i identity-file ")
+}
+
+func (s *ExecuteSSHCommandSuite) TestTimoutCaptureOutput(c *gc.C) {
+	s.fakeSSH(c, slowSSH)
+
+	response, err := ssh.ExecuteCommandOnMachine(ssh.ExecParams{
+		IdentityFile: "identity-file",
+		Host:         "hostname",
+		Command:      "ignored",
+		Timeout:      testing.ShortWait,
+	})
+
+	c.Check(err, gc.ErrorMatches, "command timed out")
+	c.Assert(response.Code, gc.Equals, 0)
+	c.Assert(string(response.Stdout), gc.Equals, "stdout\n")
+	c.Assert(string(response.Stderr), gc.Equals, "stderr\n")
+}
+
+func (s *ExecuteSSHCommandSuite) TestCapturesReturnCode(c *gc.C) {
+	s.fakeSSH(c, passthroughSSH)
+
+	response, err := ssh.ExecuteCommandOnMachine(ssh.ExecParams{
+		IdentityFile: "identity-file",
+		Host:         "hostname",
+		Command:      "echo stdout; exit 42",
+		Timeout:      testing.ShortWait,
+	})
+
+	c.Check(err, gc.IsNil)
+	c.Assert(response.Code, gc.Equals, 42)
+	c.Assert(string(response.Stdout), gc.Equals, "stdout\n")
+	c.Assert(string(response.Stderr), gc.Equals, "")
+}
+
+// echoSSH outputs the command args to stderr, and copies stdin to stdout
+var echoSSH = `#!/bin/bash
+# Write the args to stderr
+echo "$*" >&2
+cat /dev/stdin
+`
+
+// slowSSH sleeps for a while after outputting some text to stdout and stderr
+var slowSSH = `#!/bin/bash
+echo "stderr" >&2
+echo "stdout"
+sleep 5s
+`
+
+// passthroughSSH creates an ssh that executes stdin.
+var passthroughSSH = `#!/bin/bash -s`