Merge lp:~thumper/juju-core/jujuc-run into lp:~go-bot/juju-core/trunk

[Tim Penhey]

Implement the juju-run symlink in jujud

Adds a command in the jujud executable so if the executable is called as
juju-run, it will attempt to connect to the rpc server being run by the unit
specified as the first arg.

In order to get the return code passed out of the command infrastructure I
added a new error type in the cmd package called "rcPassthroughError", which
has a return code in it.

We use the jujud from the machine agent as the target of the juju-run symlink
as we can add the symlink using cloud-init and we know there is only one
machine agent.

https://codereview.appspot.com/41660044/

Implement the juju-run symlink in jujud

Adds a command in the jujud executable so if the executable is called as
juju-run, it will attempt to connect to the rpc server being run by the unit
specified as the first arg.

In order to get the return code passed out of the command infrastructure I
added a new error type in the cmd package called "rcPassthroughError", which
has a return code in it.

We use the jujud from the machine agent as the target of the juju-run symlink
as we can add the symlink using cloud-init and we know there is only one
machine agent.

https://codereview.appspot.com/41660044/

[Tim Penhey (thumper)]

Reviewers: mp+198865_code.launchpad.net,

Message:
Please take a look.

Description:
Implement the juju-run symlink in jujud

Adds a command in the jujud executable so if the executable is called as
juju-run, it will attempt to connect to the rpc server being run by the
unit
specified as the first arg.

In order to get the return code passed out of the command infrastructure
I
added a new error type in the cmd package called "rcPassthroughError",
which
has a return code in it.

We use the jujud from the machine agent as the target of the juju-run
symlink
as we can add the symlink using cloud-init and we know there is only one
machine agent.

https://code.launchpad.net/~thumper/juju-core/jujuc-run/+merge/198865

Requires:
https://code.launchpad.net/~thumper/juju-core/uniter-run-commands/+merge/198830

(do not edit description out of merge proposal)

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

Affected files (+269, -0 lines):
   A [revision details]
   M cmd/cmd.go
   M cmd/jujud/main.go
   A cmd/jujud/run.go
   A cmd/jujud/run_test.go
   M environs/cloudinit/cloudinit.go
   M environs/cloudinit/cloudinit_test.go

[Ian Booth (wallyworld)]

LGTM with some tweaks and a suggestion

https://codereview.appspot.com/41660044/diff/1/cmd/cmd.go
File cmd/cmd.go (right):

https://codereview.appspot.com/41660044/diff/1/cmd/cmd.go#newcode20
cmd/cmd.go:20: type rcPassthroughError struct {
Why not use a type alias?

https://codereview.appspot.com/41660044/diff/1/cmd/cmd.go#newcode33
cmd/cmd.go:33: func NewRcPassthroughError(code int) error {
Need doc here and/or the struct definition to say what it is for

https://codereview.appspot.com/41660044/diff/1/cmd/jujud/main.go
File cmd/jujud/main.go (right):

https://codereview.appspot.com/41660044/diff/1/cmd/jujud/main.go#newcode118
cmd/jujud/main.go:118: code = cmd.Main(&RunCommand{},
cmd.DefaultContext(), args[1:])
Should we follow the pattern used for jujuc and jujud and have a
jujurunMain method?

https://codereview.appspot.com/41660044/diff/1/cmd/jujud/run.go
File cmd/jujud/run.go (right):

https://codereview.appspot.com/41660044/diff/1/cmd/jujud/run.go#newcode67
cmd/jujud/run.go:67: c.unit = names.UnitTag(c.unit)
can we have a comment here explaining what is being done ie unit can be
either a name or tag?

https://codereview.appspot.com/41660044/diff/1/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/41660044/diff/1/environs/cloudinit/cloudinit.go#newcode242
environs/cloudinit/cloudinit.go:242: fmt.Sprintf("ln -s
%s/tools/%s/jujud /usr/local/bin/juju-run", cfg.DataDir, machineTag),
please use cfg.jujuTools()

https://codereview.appspot.com/41660044/

[Tim Penhey (thumper)]

Please take a look.

https://codereview.appspot.com/41660044/diff/1/cmd/cmd.go
File cmd/cmd.go (right):

https://codereview.appspot.com/41660044/diff/1/cmd/cmd.go#newcode20
cmd/cmd.go:20: type rcPassthroughError struct {
On 2013/12/13 04:17:33, wallyworld wrote:
> Why not use a type alias?

I think because I want a real struct here. A type alias to int is just
weird.

https://codereview.appspot.com/41660044/diff/1/cmd/cmd.go#newcode33
cmd/cmd.go:33: func NewRcPassthroughError(code int) error {
On 2013/12/13 04:17:33, wallyworld wrote:
> Need doc here and/or the struct definition to say what it is for

Done.

https://codereview.appspot.com/41660044/diff/1/cmd/jujud/main.go
File cmd/jujud/main.go (right):

https://codereview.appspot.com/41660044/diff/1/cmd/jujud/main.go#newcode118
cmd/jujud/main.go:118: code = cmd.Main(&RunCommand{},
cmd.DefaultContext(), args[1:])
On 2013/12/13 04:17:33, wallyworld wrote:
> Should we follow the pattern used for jujuc and jujud and have a
jujurunMain
> method?

We could, but it would just be one line... so I chose not to.

https://codereview.appspot.com/41660044/diff/1/cmd/jujud/run.go
File cmd/jujud/run.go (right):

https://codereview.appspot.com/41660044/diff/1/cmd/jujud/run.go#newcode67
cmd/jujud/run.go:67: c.unit = names.UnitTag(c.unit)
On 2013/12/13 04:17:33, wallyworld wrote:
> can we have a comment here explaining what is being done ie unit can
be either a
> name or tag?

Done.

https://codereview.appspot.com/41660044/diff/1/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/41660044/diff/1/environs/cloudinit/cloudinit.go#newcode242
environs/cloudinit/cloudinit.go:242: fmt.Sprintf("ln -s
%s/tools/%s/jujud /usr/local/bin/juju-run", cfg.DataDir, machineTag),
On 2013/12/13 04:17:33, wallyworld wrote:
> please use cfg.jujuTools()

No, but comment added to explain why.

https://codereview.appspot.com/41660044/

=== modified file 'cmd/cmd.go'
--- cmd/cmd.go	2013-12-11 09:21:56 +0000
+++ cmd/cmd.go	2013-12-13 04:41:06 +0000
@@ -17,6 +17,26 @@
 	"launchpad.net/gnuflag"
 )
 
+type rcPassthroughError struct {
+	code int
+}
+
+func (e *rcPassthroughError) Error() string {
+	return fmt.Sprintf("rc: %v", e.code)
+}
+
+func IsRcPassthroughError(err error) bool {
+	_, ok := err.(*rcPassthroughError)
+	return ok
+}
+
+// NewRcPassthroughError creates an error that will have the code used at the
+// return code from the cmd.Main function rather than the default of 1 if
+// there is an error.
+func NewRcPassthroughError(code int) error {
+	return &rcPassthroughError{code}
+}
+
 func init() {
 	// Don't replace the default transport as other init blocks
 	// register protocols.
@@ -173,6 +193,9 @@
 		return rc
 	}
 	if err := c.Run(ctx); err != nil {
+		if IsRcPassthroughError(err) {
+			return err.(*rcPassthroughError).code
+		}
 		if err != ErrSilent {
 			fmt.Fprintf(ctx.Stderr, "error: %v\n", err)
 		}

=== modified file 'cmd/jujud/main.go'
--- cmd/jujud/main.go	2013-12-12 04:31:01 +0000
+++ cmd/jujud/main.go	2013-12-13 04:41:06 +0000
@@ -114,6 +114,8 @@
 		fmt.Fprint(os.Stderr, jujudDoc)
 		code = 2
 		err = fmt.Errorf("jujuc should not be called directly")
+	} else if commandName == "juju-run" {
+		code = cmd.Main(&RunCommand{}, cmd.DefaultContext(), args[1:])
 	} else {
 		code, err = jujuCMain(commandName, args)
 	}

=== added file 'cmd/jujud/run.go'
--- cmd/jujud/run.go	1970-01-01 00:00:00 +0000
+++ cmd/jujud/run.go	2013-12-13 04:41:06 +0000
@@ -0,0 +1,108 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package main
+
+import (
+	"fmt"
+	"net/rpc"
+	"os"
+	"path/filepath"
+
+	"launchpad.net/gnuflag"
+
+	"launchpad.net/juju-core/cmd"
+	"launchpad.net/juju-core/names"
+	"launchpad.net/juju-core/worker/uniter"
+)
+
+var AgentDir = "/var/lib/juju/agents"
+
+type RunCommand struct {
+	cmd.CommandBase
+	unit     string
+	commands string
+	showHelp bool
+}
+
+const runCommandDoc = `
+Run the specified commands in the hook context for the unit.
+
+unit-name can be either the unit tag:
+ i.e.  unit-ubuntu-0
+or the unit id:
+ i.e.  ubuntu/0
+
+The commands are executed with '/bin/bash -s', and the output returned.
+`
+
+// Info returns usage information for the command.
+func (c *RunCommand) Info() *cmd.Info {
+	return &cmd.Info{
+		Name:    "juju-run",
+		Args:    "<unit-name> <commands>",
+		Purpose: "run commands in a unit's hook context",
+		Doc:     runCommandDoc,
+	}
+}
+
+func (c *RunCommand) SetFlags(f *gnuflag.FlagSet) {
+	f.BoolVar(&c.showHelp, "h", false, "show help on juju-run")
+	f.BoolVar(&c.showHelp, "help", false, "")
+}
+
+func (c *RunCommand) Init(args []string) error {
+	// make sure we aren't in an existing hook context
+	if contextId, err := getenv("JUJU_CONTEXT_ID"); err == nil && contextId != "" {
+		return fmt.Errorf("juju-run cannot be called from within a hook, have context %q", contextId)
+	}
+	if len(args) < 1 {
+		return fmt.Errorf("missing unit-name")
+	}
+	if len(args) < 2 {
+		return fmt.Errorf("missing commands")
+	}
+	c.unit, args = args[0], args[1:]
+	// If the command line param is a unit id (like service/2) we need to
+	// change it to the unit tag as that is the format of the agent directory
+	// on disk (unit-service-2).
+	if names.IsUnit(c.unit) {
+		c.unit = names.UnitTag(c.unit)
+	}
+	c.commands, args = args[0], args[1:]
+	return cmd.CheckEmpty(args)
+}
+
+func (c *RunCommand) Run(ctx *cmd.Context) error {
+	if c.showHelp {
+		return gnuflag.ErrHelp
+	}
+
+	unitDir := filepath.Join(AgentDir, c.unit)
+	logger.Debugf("looking for unit dir %s", unitDir)
+	// make sure the unit exists
+	_, err := os.Stat(unitDir)
+	if os.IsNotExist(err) {
+		return fmt.Errorf("unit %q not found on this machine", c.unit)
+	} else if err != nil {
+		return err
+	}
+
+	socketPath := filepath.Join(unitDir, uniter.RunListenerFile)
+	// make sure the socket exists
+	client, err := rpc.Dial(uniter.RunListenerNetType, socketPath)
+	if err != nil {
+		return err
+	}
+	defer client.Close()
+
+	var result cmd.RemoteResponse
+	err = client.Call(uniter.JujuRunEndpoint, c.commands, &result)
+	if err != nil {
+		return err
+	}
+
+	ctx.Stdout.Write(result.Stdout)
+	ctx.Stderr.Write(result.Stderr)
+	return cmd.NewRcPassthroughError(result.Code)
+}

=== added file 'cmd/jujud/run_test.go'
--- cmd/jujud/run_test.go	1970-01-01 00:00:00 +0000
+++ cmd/jujud/run_test.go	2013-12-13 04:41:06 +0000
@@ -0,0 +1,135 @@
+// Copyright 2012, 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package main
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+
+	gc "launchpad.net/gocheck"
+	"launchpad.net/loggo"
+
+	"launchpad.net/juju-core/cmd"
+	"launchpad.net/juju-core/testing"
+	jc "launchpad.net/juju-core/testing/checkers"
+	"launchpad.net/juju-core/testing/testbase"
+	"launchpad.net/juju-core/worker/uniter"
+)
+
+type RunTestSuite struct {
+	testbase.LoggingSuite
+}
+
+var _ = gc.Suite(&RunTestSuite{})
+
+func (*RunTestSuite) TestWrongArgs(c *gc.C) {
+	for i, test := range []struct {
+		title    string
+		args     []string
+		errMatch string
+		unit     string
+		commands string
+	}{{
+		title:    "no args",
+		errMatch: "missing unit-name",
+	}, {
+		title:    "one arg",
+		args:     []string{"foo"},
+		errMatch: "missing commands",
+	}, {
+		title:    "more than two arg",
+		args:     []string{"foo", "bar", "baz"},
+		errMatch: `unrecognized args: \["baz"\]`,
+	}, {
+		title:    "unit and command assignment",
+		args:     []string{"unit-name", "command"},
+		unit:     "unit-name",
+		commands: "command",
+	}, {
+		title:    "unit id converted to tag",
+		args:     []string{"foo/1", "command"},
+		unit:     "unit-foo-1",
+		commands: "command",
+	},
+	} {
+		c.Log(fmt.Sprintf("\n%d: %s", i, test.title))
+		runCommand := &RunCommand{}
+		err := runCommand.Init(test.args)
+		if test.errMatch == "" {
+			c.Assert(err, gc.IsNil)
+			c.Assert(runCommand.unit, gc.Equals, test.unit)
+			c.Assert(runCommand.commands, gc.Equals, test.commands)
+		} else {
+			c.Assert(err, gc.ErrorMatches, test.errMatch)
+		}
+	}
+}
+
+func (s *RunTestSuite) TestInsideContext(c *gc.C) {
+	s.PatchEnvironment("JUJU_CONTEXT_ID", "fake-id")
+	runCommand := &RunCommand{}
+	err := runCommand.Init([]string{"foo", "bar"})
+	c.Assert(err, gc.ErrorMatches, "juju-run cannot be called from within a hook.*")
+}
+
+func (s *RunTestSuite) TestMissingAgent(c *gc.C) {
+	agentDir := c.MkDir()
+	s.PatchValue(&AgentDir, agentDir)
+
+	_, err := testing.RunCommand(c, &RunCommand{}, []string{"foo", "bar"})
+	c.Assert(err, gc.ErrorMatches, `unit "foo" not found on this machine`)
+}
+
+func (s *RunTestSuite) TestMissingSocket(c *gc.C) {
+	s.PatchValue(&AgentDir, c.MkDir())
+	testAgentDir := filepath.Join(AgentDir, "foo")
+	err := os.Mkdir(testAgentDir, 0755)
+	c.Assert(err, gc.IsNil)
+
+	_, err = testing.RunCommand(c, &RunCommand{}, []string{"foo", "bar"})
+	c.Assert(err, gc.ErrorMatches, `dial unix .*/run.socket: no such file or directory`)
+}
+
+func (s *RunTestSuite) TestRunning(c *gc.C) {
+	loggo.GetLogger("worker.uniter").SetLogLevel(loggo.TRACE)
+	s.runListenerForAgent(c, "foo")
+
+	ctx, err := testing.RunCommand(c, &RunCommand{}, []string{"foo", "bar"})
+	c.Check(cmd.IsRcPassthroughError(err), jc.IsTrue)
+	c.Assert(err, gc.ErrorMatches, "rc: 42")
+	c.Assert(testing.Stdout(ctx), gc.Equals, "bar stdout")
+	c.Assert(testing.Stderr(ctx), gc.Equals, "bar stderr")
+}
+
+func (s *RunTestSuite) runListenerForAgent(c *gc.C, agent string) {
+	s.PatchValue(&AgentDir, c.MkDir())
+
+	testAgentDir := filepath.Join(AgentDir, agent)
+	err := os.Mkdir(testAgentDir, 0755)
+	c.Assert(err, gc.IsNil)
+
+	socketPath := filepath.Join(testAgentDir, uniter.RunListenerFile)
+	listener, err := uniter.NewRunListener(&mockRunner{c}, "unix", socketPath)
+	c.Assert(err, gc.IsNil)
+	c.Assert(listener, gc.NotNil)
+	s.AddCleanup(func(*gc.C) {
+		listener.Close()
+	})
+}
+
+type mockRunner struct {
+	c *gc.C
+}
+
+var _ uniter.CommandRunner = (*mockRunner)(nil)
+
+func (r *mockRunner) RunCommands(commands string) (results *cmd.RemoteResponse, err error) {
+	r.c.Log("mock runner: " + commands)
+	return &cmd.RemoteResponse{
+		Code:   42,
+		Stdout: []byte(commands + " stdout"),
+		Stderr: []byte(commands + " stderr"),
+	}, nil
+}

=== modified file 'environs/cloudinit/cloudinit.go'
--- environs/cloudinit/cloudinit.go	2013-12-04 04:36:32 +0000
+++ environs/cloudinit/cloudinit.go	2013-12-13 04:41:06 +0000
@@ -238,6 +238,14 @@
 	if err != nil {
 		return err
 	}
+	c.AddScripts(
+		// We specifically make the symlink here to the machine's current
+		// tools, not to the specific version tool directory (from
+		// cfg.jujuTools()), as we want the jujud that is linked to in
+		// /usr/local/bin to also upgrade when the machine agent upgrades its
+		// tools and changes the tools directory that it is using.
+		fmt.Sprintf("ln -s %s/tools/%s/jujud /usr/local/bin/juju-run", cfg.DataDir, machineTag),
+	)
 
 	// Add the cloud archive cloud-tools pocket to apt sources
 	// for series that need it. This gives us up-to-date LXC,

=== modified file 'environs/cloudinit/cloudinit_test.go'
--- environs/cloudinit/cloudinit_test.go	2013-12-03 06:04:43 +0000
+++ environs/cloudinit/cloudinit_test.go	2013-12-13 04:41:06 +0000
@@ -109,6 +109,7 @@
 printf '%s\\n' '.*' > '/var/lib/juju/agents/machine-0/format'
 install -m 600 /dev/null '/var/lib/juju/agents/machine-0/agent\.conf'
 printf '%s\\n' '.*' > '/var/lib/juju/agents/machine-0/agent\.conf'
+ln -s /var/lib/juju/tools/machine-0/jujud /usr/local/bin/juju-run
 install -m 600 /dev/null '/var/lib/juju/server\.pem'
 printf '%s\\n' 'SERVER CERT\\n[^']*SERVER KEY\\n[^']*' > '/var/lib/juju/server\.pem'
 mkdir -p /var/lib/juju/db/journal
@@ -219,6 +220,7 @@
 printf '%s\\n' '.*' > '/var/lib/juju/agents/machine-99/format'
 install -m 600 /dev/null '/var/lib/juju/agents/machine-99/agent\.conf'
 printf '%s\\n' '.*' > '/var/lib/juju/agents/machine-99/agent\.conf'
+ln -s /var/lib/juju/tools/machine-99/jujud /usr/local/bin/juju-run
 ln -s 1\.2\.3-linux-amd64 '/var/lib/juju/tools/machine-99'
 echo 'Starting Juju machine agent \(jujud-machine-99\)'.*
 cat >> /etc/init/jujud-machine-99\.conf << 'EOF'\\ndescription "juju machine-99 agent"\\nauthor "Juju Team <juju@lists\.ubuntu\.com>"\\nstart on runlevel \[2345\]\\nstop on runlevel \[!2345\]\\nrespawn\\nnormal exit 0\\n\\nlimit nofile 20000 20000\\n\\nexec /var/lib/juju/tools/machine-99/jujud machine --data-dir '/var/lib/juju' --machine-id 99 --debug >> /var/log/juju/machine-99\.log 2>&1\\nEOF\\n