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

[Tim Penhey]

Implement the uniter parts of juju-run

Add a method to the uniter that has the run listener.
The uniter gets the hook lock, sets up a jujuc server
and calls the commands using the hook context run commands
method.

https://codereview.appspot.com/41470043/

Implement the uniter parts of juju-run

Add a method to the uniter that has the run listener.
The uniter gets the hook lock, sets up a jujuc server
and calls the commands using the hook context run commands
method.

https://codereview.appspot.com/41470043/

[Tim Penhey (thumper)]

Reviewers: mp+198830_code.launchpad.net,

Message:
Please take a look.

Description:
Implement the uniter parts of juju-run

Add a method to the uniter that has the run listener.
The uniter gets the hook lock, sets up a jujuc server
and calls the commands using the hook context run commands
method.

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

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

(do not edit description out of merge proposal)

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

Affected files (+171, -0 lines):
   A [revision details]
   M worker/uniter/uniter.go
   M worker/uniter/uniter_test.go

[Ian Booth (wallyworld)]

LGTM with some tweaks

https://codereview.appspot.com/41470043/diff/1/worker/uniter/uniter_test.go
File worker/uniter/uniter_test.go (right):

https://codereview.appspot.com/41470043/diff/1/worker/uniter/uniter_test.go#newcode853
worker/uniter/uniter_test.go:853: runCommands{fmt.Sprintf("echo juju run
${JUJU_UNIT_NAME} > %s", filepath.Join(testDir, "run.output"))},
There's a lot of duplicated filepath.Join(testDir, "run.output").
Can we not just create a generic file at the top of the method and reuse
that? Assuming it is cleaned up after each test which we can add if it
is not done yet.

https://codereview.appspot.com/41470043/

[Tim Penhey (thumper)]

Please take a look.

https://codereview.appspot.com/41470043/diff/1/worker/uniter/uniter_test.go
File worker/uniter/uniter_test.go (right):

https://codereview.appspot.com/41470043/diff/1/worker/uniter/uniter_test.go#newcode853
worker/uniter/uniter_test.go:853: runCommands{fmt.Sprintf("echo juju run
${JUJU_UNIT_NAME} > %s", filepath.Join(testDir, "run.output"))},
On 2013/12/13 00:47:04, wallyworld wrote:
> There's a lot of duplicated filepath.Join(testDir, "run.output").
> Can we not just create a generic file at the top of the method and
reuse that?
> Assuming it is cleaned up after each test which we can add if it is
not done
> yet.

Added some magic to reduce duplication.

https://codereview.appspot.com/41470043/

[Go Bot (go-bot)]

The prerequisite https://code.launchpad.net/~thumper/juju-core/uniter-context-run-commands has not yet been merged into https://code.launchpad.net/~go-bot/juju-core/trunk.

=== modified file 'worker/uniter/uniter.go'
--- worker/uniter/uniter.go	2013-12-13 03:09:56 +0000
+++ worker/uniter/uniter.go	2013-12-13 03:09:57 +0000
@@ -32,6 +32,14 @@
 
 var logger = loggo.GetLogger("juju.worker.uniter")
 
+const (
+	// These work fine for linux, but should we need to work with windows
+	// workloads in the future, we'll need to move these into a file that is
+	// compiled conditionally for different targets and use tcp (most likely).
+	RunListenerNetType = "unix"
+	RunListenerFile    = "run.socket"
+)
+
 // A UniterExecutionObserver gets the appropriate methods called when a hook
 // is executed and either succeeds or fails.  Missing hooks don't get reported
 // in this way.
@@ -65,6 +73,7 @@
 	sf           *StateFile
 	rand         *rand.Rand
 	hookLock     *fslock.Lock
+	runListener  *RunListener
 
 	ranConfigChanged bool
 	// The execution observer is only used in tests at this stage. Should this
@@ -91,6 +100,7 @@
 	if err = u.init(unitTag); err != nil {
 		return err
 	}
+	defer u.runListener.Close()
 	logger.Infof("unit %q started", u.unit)
 
 	// Start filtering state change events for consumption by modes.
@@ -168,6 +178,17 @@
 	if err != nil {
 		return err
 	}
+
+	runListenerSocketPath := filepath.Join(u.baseDir, RunListenerFile)
+	logger.Debugf("starting juju-run listener on %s:%s", RunListenerNetType, runListenerSocketPath)
+	u.runListener, err = NewRunListener(u, RunListenerNetType, runListenerSocketPath)
+	if err != nil {
+		return err
+	}
+	// The socket needs to have permissions 777 in order for other users to use it.
+	if err := os.Chmod(runListenerSocketPath, 0777); err != nil {
+		return err
+	}
 	u.relationers = map[int]*Relationer{}
 	u.relationHooks = make(chan hook.Info)
 	u.charm = charm.NewGitDir(filepath.Join(u.baseDir, "charm"))
@@ -343,6 +364,33 @@
 	return srv, socketPath, nil
 }
 
+// RunCommands executes the supplied commands in a hook context.
+func (u *Uniter) RunCommands(commands string) (results *cmd.RemoteResponse, err error) {
+	logger.Tracef("run commands: %s", commands)
+	hctxId := fmt.Sprintf("%s:run-commands:%d", u.unit.Name(), u.rand.Int63())
+	lockMessage := fmt.Sprintf("%s: running commands", u.unit.Name())
+	if err = u.acquireHookLock(lockMessage); err != nil {
+		return nil, err
+	}
+	defer u.hookLock.Unlock()
+
+	hctx, err := u.getHookContext(hctxId, -1, "")
+	if err != nil {
+		return nil, err
+	}
+	srv, socketPath, err := u.startJujucServer(hctx)
+	if err != nil {
+		return nil, err
+	}
+	defer srv.Close()
+
+	result, err := hctx.RunCommands(commands, u.charm.Path(), u.toolsDir, socketPath)
+	if result != nil {
+		logger.Tracef("run commands: rc=%v\nstdout:\n%sstderr:\n%s", result.Code, result.Stdout, result.Stderr)
+	}
+	return result, err
+}
+
 func (u *Uniter) notifyHookInternal(hook string, hctx *HookContext, method func(string)) {
 	if r, ok := hctx.HookRelation(); ok {
 		remote, _ := hctx.RemoteUnitName()

=== modified file 'worker/uniter/uniter_test.go'
--- worker/uniter/uniter_test.go	2013-12-13 03:09:56 +0000
+++ worker/uniter/uniter_test.go	2013-12-13 03:09:57 +0000
@@ -10,11 +10,13 @@
 	"fmt"
 	"io"
 	"io/ioutil"
+	"net/rpc"
 	"net/url"
 	"os"
 	"os/exec"
 	"path/filepath"
 	"strconv"
+	"strings"
 	stdtesting "testing"
 	"time"
 
@@ -23,6 +25,7 @@
 
 	"launchpad.net/juju-core/agent/tools"
 	"launchpad.net/juju-core/charm"
+	"launchpad.net/juju-core/cmd"
 	"launchpad.net/juju-core/errors"
 	"launchpad.net/juju-core/juju/testing"
 	"launchpad.net/juju-core/state"
@@ -841,6 +844,50 @@
 	s.runUniterTests(c, upgradeConflictsTests)
 }
 
+func (s *UniterSuite) TestRunCommand(c *gc.C) {
+	testDir := c.MkDir()
+	testFile := func(name string) string {
+		return filepath.Join(testDir, name)
+	}
+	echoUnitNameToFile := func(name string) string {
+		return fmt.Sprintf("echo juju run ${JUJU_UNIT_NAME} > %s", filepath.Join(testDir, name))
+	}
+	tests := []uniterTest{
+		ut(
+			"run commands: environment",
+			quickStart{},
+			runCommands{echoUnitNameToFile("run.output")},
+			verifyFile{filepath.Join(testDir, "run.output"), "juju run u/0\n"},
+		), ut(
+			"run commands: jujuc commands",
+			quickStartRelation{},
+			runCommands{
+				fmt.Sprintf("owner-get tag > %s", testFile("jujuc.output")),
+				fmt.Sprintf("unit-get private-address >> %s", testFile("jujuc.output")),
+				fmt.Sprintf("unit-get public-address >> %s", testFile("jujuc.output")),
+			},
+			verifyFile{
+				testFile("jujuc.output"),
+				"user-admin\nprivate.dummy.address.example.com\npublic.dummy.address.example.com\n",
+			},
+		), ut(
+			"run commands: async using rpc client",
+			quickStart{},
+			asyncRunCommands{echoUnitNameToFile("run.output")},
+			verifyFile{testFile("run.output"), "juju run u/0\n"},
+		), ut(
+			"run commands: waits for lock",
+			quickStart{},
+			acquireHookSyncLock{},
+			asyncRunCommands{echoUnitNameToFile("wait.output")},
+			verifyNoFile{testFile("wait.output")},
+			releaseHookSyncLock,
+			verifyFile{testFile("wait.output"), "juju run u/0\n"},
+		),
+	}
+	s.runUniterTests(c, tests)
+}
+
 var relationsTests = []uniterTest{
 	// Relations.
 	ut(
@@ -1894,3 +1941,83 @@
 	lock := createHookLock(c, ctx.dataDir)
 	c.Assert(lock.IsLocked(), jc.IsTrue)
 }}
+
+type runCommands []string
+
+func (cmds runCommands) step(c *gc.C, ctx *context) {
+	commands := strings.Join(cmds, "\n")
+	result, err := ctx.uniter.RunCommands(commands)
+	c.Assert(err, gc.IsNil)
+	c.Check(result.Code, gc.Equals, 0)
+	c.Check(string(result.Stdout), gc.Equals, "")
+	c.Check(string(result.Stderr), gc.Equals, "")
+}
+
+type asyncRunCommands []string
+
+func (cmds asyncRunCommands) step(c *gc.C, ctx *context) {
+	commands := strings.Join(cmds, "\n")
+	socketPath := filepath.Join(ctx.path, uniter.RunListenerFile)
+
+	go func() {
+		// make sure the socket exists
+		client, err := rpc.Dial("unix", socketPath)
+		c.Assert(err, gc.IsNil)
+		defer client.Close()
+
+		var result cmd.RemoteResponse
+		err = client.Call(uniter.JujuRunEndpoint, commands, &result)
+		c.Assert(err, gc.IsNil)
+		c.Check(result.Code, gc.Equals, 0)
+		c.Check(string(result.Stdout), gc.Equals, "")
+		c.Check(string(result.Stderr), gc.Equals, "")
+	}()
+}
+
+type verifyFile struct {
+	filename string
+	content  string
+}
+
+func (verify verifyFile) fileExists() bool {
+	_, err := os.Stat(verify.filename)
+	return err == nil
+}
+
+func (verify verifyFile) checkContent(c *gc.C) {
+	content, err := ioutil.ReadFile(verify.filename)
+	c.Assert(err, gc.IsNil)
+	c.Assert(string(content), gc.Equals, verify.content)
+}
+
+func (verify verifyFile) step(c *gc.C, ctx *context) {
+	if verify.fileExists() {
+		verify.checkContent(c)
+		return
+	}
+	c.Logf("waiting for file: %s", verify.filename)
+	timeout := time.After(worstCase)
+	for {
+		select {
+		case <-time.After(coretesting.ShortWait):
+			if verify.fileExists() {
+				verify.checkContent(c)
+				return
+			}
+		case <-timeout:
+			c.Fatalf("file does not exist")
+		}
+	}
+}
+
+// verify that the file does not exist
+type verifyNoFile struct {
+	filename string
+}
+
+func (verify verifyNoFile) step(c *gc.C, ctx *context) {
+	c.Assert(verify.filename, jc.DoesNotExist)
+	// Wait a short time and check again.
+	time.Sleep(coretesting.ShortWait)
+	c.Assert(verify.filename, jc.DoesNotExist)
+}