Merge lp:~thumper/juju-core/juju-run-on-machine-with-no-units into lp:~go-bot/juju-core/trunk

[Tim Penhey]

Fix the juju-run on machines with no units.

This branch also adds an upgrade step to make sure
that existing environments also get the goodness.

https://codereview.appspot.com/62390043/

Fix the juju-run on machines with no units.

This branch also adds an upgrade step to make sure
that existing environments also get the goodness.

https://codereview.appspot.com/62390043/

[Tim Penhey (thumper)]

Reviewers: mp+206050_code.launchpad.net,

Message:
Please take a look.

Description:
Fix the juju-run on machines with no units.

This branch also adds an upgrade step to make sure
that existing environments also get the goodness.

https://code.launchpad.net/~thumper/juju-core/juju-run-on-machine-with-no-units/+merge/206050

(do not edit description out of merge proposal)

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

Affected files (+100, -6 lines):
   [revision details]
   environs/cloudinit/cloudinit.go
   environs/cloudinit/cloudinit_test.go
   upgrades/steps118.go
   upgrades/upgrade.go
   upgrades/upgrade_test.go

[Tim Penhey (thumper)]

Please take a look.

https://codereview.appspot.com/62390043/

[Ian Booth (wallyworld)]

LGTM. We need one more test to ensure the upgrade operation is
registered correctly but I'll add that in my next lot of work as I have
to do it anyway.

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

https://codereview.appspot.com/62390043/diff/1/environs/cloudinit/cloudinit.go#newcode282
environs/cloudinit/cloudinit.go:282:
part of me wishes that "locks" were a const somewhere

https://codereview.appspot.com/62390043/

=== modified file 'environs/cloudinit/cloudinit.go'
--- environs/cloudinit/cloudinit.go	2014-02-11 16:39:04 +0000
+++ environs/cloudinit/cloudinit.go	2014-02-13 03:37:16 +0000
@@ -275,8 +275,16 @@
 				shquote(cfg.ProxySettings.AsScriptEnvironment())))
 	}
 
+	// Make the lock dir and change the ownership of the lock dir itself to
+	// ubuntu:ubuntu from root:root so the juju-run command run as the ubuntu
+	// user is able to get access to the hook execution lock (like the uniter
+	// itself does.)
+	lockDir := path.Join(cfg.DataDir, "locks")
 	c.AddScripts(
-		fmt.Sprintf("mkdir -p %s", cfg.DataDir),
+		fmt.Sprintf("mkdir -p %s", lockDir),
+		// We only try to change ownership if there is an ubuntu user
+		// defined, and we determine this by the existance of the home dir.
+		fmt.Sprintf("[ -e /home/ubuntu ] && chown ubuntu:ubuntu %s", lockDir),
 		fmt.Sprintf("mkdir -p %s", cfg.LogDir),
 	)
 

=== modified file 'environs/cloudinit/cloudinit_test.go'
--- environs/cloudinit/cloudinit_test.go	2014-02-11 16:39:04 +0000
+++ environs/cloudinit/cloudinit_test.go	2014-02-13 03:37:16 +0000
@@ -100,7 +100,8 @@
 printf '%s\\n' 'FAKE_NONCE' > '/var/lib/juju/nonce.txt'
 test -e /proc/self/fd/9 \|\| exec 9>&2
 \(\[ ! -e /home/ubuntu/.profile \] \|\| grep -q '.juju-proxy' /home/ubuntu/.profile\) \|\| printf .* >> /home/ubuntu/.profile
-mkdir -p /var/lib/juju
+mkdir -p /var/lib/juju/locks
+\[ -e /home/ubuntu \] && chown ubuntu:ubuntu /var/lib/juju/locks
 mkdir -p /var/log/juju
 echo 'Fetching tools.*
 bin='/var/lib/juju/tools/1\.2\.3-precise-amd64'
@@ -227,7 +228,8 @@
 printf '%s\\n' 'FAKE_NONCE' > '/var/lib/juju/nonce.txt'
 test -e /proc/self/fd/9 \|\| exec 9>&2
 \(\[ ! -e /home/ubuntu/\.profile \] \|\| grep -q '.juju-proxy' /home/ubuntu/.profile\) \|\| printf .* >> /home/ubuntu/.profile
-mkdir -p /var/lib/juju
+mkdir -p /var/lib/juju/locks
+\[ -e /home/ubuntu \] && chown ubuntu:ubuntu /var/lib/juju/locks
 mkdir -p /var/log/juju
 echo 'Fetching tools.*
 bin='/var/lib/juju/tools/1\.2\.3-linux-amd64'

=== modified file 'upgrades/export_test.go'
--- upgrades/export_test.go	2014-02-10 05:34:06 +0000
+++ upgrades/export_test.go	2014-02-13 03:37:16 +0000
@@ -3,4 +3,10 @@
 
 package upgrades
 
-var UpgradeOperations = &upgradeOperations
+var (
+	UpgradeOperations = &upgradeOperations
+
+	UbuntuHome = &ubuntuHome
+
+	EnsureLockDirExistsAndUbuntuWritable = ensureLockDirExistsAndUbuntuWritable
+)

=== modified file 'upgrades/steps118.go'
--- upgrades/steps118.go	2014-02-10 05:34:06 +0000
+++ upgrades/steps118.go	2014-02-13 03:37:16 +0000
@@ -3,9 +3,51 @@
 
 package upgrades
 
+import (
+	"fmt"
+	"path"
+
+	"launchpad.net/juju-core/utils/exec"
+)
+
 // stepsFor118 returns upgrade steps to upgrade to a Juju 1.18 deployment.
 func stepsFor118(context Context) []UpgradeStep {
 	return []UpgradeStep{
-	// Nothing yet.
-	}
+		// Nothing yet.
+		&upgradeStep{
+			description: "make $DATADIR/locks owned by ubuntu:ubuntu",
+			targets:     []UpgradeTarget{HostMachine},
+			run:         ensureLockDirExistsAndUbuntuWritable,
+		},
+	}
+}
+
+var ubuntuHome = "/home/ubuntu"
+
+// Previously the lock directory was created when the uniter started. This
+// allows serialization of all of the hook execution across units running on a
+// single machine.  This lock directory is now also used but the juju-run
+// command on the host machine.  juju-run also gets a lock on the hook
+// execution fslock prior to execution.  However, the lock directory was owned
+// by root, and the juju-run process was being executed by the ubuntu user, so
+// we need to change the ownership of the lock directory to ubuntu:ubuntu.
+// Also we need to make sure that this directory exists on machines with no
+// units.
+func ensureLockDirExistsAndUbuntuWritable(context Context) error {
+	lockDir := path.Join(context.AgentConfig().DataDir(), "locks")
+	// We only try to change ownership if there is an ubuntu user
+	// defined, and we determine this by the existance of the home dir.
+	command := fmt.Sprintf(""+
+		"mkdir -p %s\n"+
+		"[ -e %s ] && chown ubuntu:ubuntu %s\n",
+		lockDir, ubuntuHome, lockDir)
+	logger.Tracef("command: %s", command)
+	result, err := exec.RunCommands(exec.RunParams{
+		Commands: command,
+	})
+	if err != nil {
+		return err
+	}
+	logger.Tracef("stdout: %s", result.Stdout)
+	return nil
 }

=== added file 'upgrades/steps118_test.go'
--- upgrades/steps118_test.go	1970-01-01 00:00:00 +0000
+++ upgrades/steps118_test.go	2014-02-13 03:37:16 +0000
@@ -0,0 +1,117 @@
+// Copyright 2014 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package upgrades_test
+
+import (
+	"fmt"
+	"io/ioutil"
+	"os"
+	"path/filepath"
+
+	"github.com/loggo/loggo"
+	gc "launchpad.net/gocheck"
+
+	"launchpad.net/juju-core/agent"
+	"launchpad.net/juju-core/state/api"
+	"launchpad.net/juju-core/testing"
+	jc "launchpad.net/juju-core/testing/checkers"
+	"launchpad.net/juju-core/upgrades"
+)
+
+type ensureLockDirSuite struct {
+	testing.FakeHomeSuite
+	bin     string
+	home    string
+	datadir string
+	lockdir string
+	ctx     upgrades.Context
+}
+
+var _ = gc.Suite(&ensureLockDirSuite{})
+
+// fakecommand outputs its arguments to stdout for verification
+var fakecommand = `#!/bin/bash
+
+echo $@ | tee $0.args
+`
+
+func (s *ensureLockDirSuite) SetUpTest(c *gc.C) {
+	s.FakeHomeSuite.SetUpTest(c)
+
+	s.bin = c.MkDir()
+	s.PatchEnvironment("PATH", s.bin+":"+os.Getenv("PATH"))
+
+	err := ioutil.WriteFile(
+		filepath.Join(s.bin, "chown"),
+		[]byte(fakecommand), 0777)
+	c.Assert(err, gc.IsNil)
+
+	loggo.GetLogger("juju.upgrade").SetLogLevel(loggo.TRACE)
+
+	s.home = c.MkDir()
+	s.PatchValue(upgrades.UbuntuHome, s.home)
+
+	s.datadir = c.MkDir()
+	s.lockdir = filepath.Join(s.datadir, "locks")
+	s.ctx = &ensureContext{&mockAgentConfig{datadir: s.datadir}}
+}
+
+func (s *ensureLockDirSuite) assertChownCalled(c *gc.C) {
+	bytes, err := ioutil.ReadFile(filepath.Join(s.bin, "chown.args"))
+	c.Assert(err, gc.IsNil)
+	c.Assert(string(bytes), gc.Equals, fmt.Sprintf("ubuntu:ubuntu %s\n", s.lockdir))
+}
+
+func (s *ensureLockDirSuite) assertNoChownCalled(c *gc.C) {
+	c.Assert(filepath.Join(s.bin, "chown.args"), jc.DoesNotExist)
+}
+
+func (s *ensureLockDirSuite) TestLockDirCreated(c *gc.C) {
+	err := upgrades.EnsureLockDirExistsAndUbuntuWritable(s.ctx)
+	c.Assert(err, gc.IsNil)
+
+	c.Assert(s.lockdir, jc.IsDirectory)
+	s.assertChownCalled(c)
+}
+
+func (s *ensureLockDirSuite) TestIdempotent(c *gc.C) {
+	err := upgrades.EnsureLockDirExistsAndUbuntuWritable(s.ctx)
+	c.Assert(err, gc.IsNil)
+
+	err = upgrades.EnsureLockDirExistsAndUbuntuWritable(s.ctx)
+	c.Assert(err, gc.IsNil)
+
+	c.Assert(s.lockdir, jc.IsDirectory)
+	s.assertChownCalled(c)
+}
+
+func (s *ensureLockDirSuite) TestNoChownIfNoHome(c *gc.C) {
+	s.PatchValue(upgrades.UbuntuHome, filepath.Join(s.home, "not-exist"))
+	err := upgrades.EnsureLockDirExistsAndUbuntuWritable(s.ctx)
+	c.Assert(err, gc.IsNil)
+
+	c.Assert(s.lockdir, jc.IsDirectory)
+	s.assertNoChownCalled(c)
+}
+
+type ensureContext struct {
+	agentConfig *mockAgentConfig
+}
+
+func (c *ensureContext) APIState() *api.State {
+	return nil
+}
+
+func (c *ensureContext) AgentConfig() agent.Config {
+	return c.agentConfig
+}
+
+type mockAgentConfig struct {
+	agent.Config
+	datadir string
+}
+
+func (mock *mockAgentConfig) DataDir() string {
+	return mock.datadir
+}

=== modified file 'upgrades/upgrade.go'
--- upgrades/upgrade.go	2014-02-12 10:29:45 +0000
+++ upgrades/upgrade.go	2014-02-13 03:37:16 +0000
@@ -6,10 +6,15 @@
 import (
 	"fmt"
 
+	"github.com/loggo/loggo"
+
+	"launchpad.net/juju-core/agent"
 	"launchpad.net/juju-core/state/api"
 	"launchpad.net/juju-core/version"
 )
 
+var logger = loggo.GetLogger("juju.upgrade")
+
 // UpgradeStep defines an idempotent operation that is run to perform
 // a specific upgrade step.
 type UpgradeStep interface {
@@ -41,6 +46,7 @@
 
 const (
 	// HostMachine is a machine on which units are deployed.
+	// all machines?
 	HostMachine = UpgradeTarget("hostMachine")
 
 	// StateServer is a machine participating in a Juju state server cluster.
@@ -69,6 +75,8 @@
 type Context interface {
 	// APIState returns an API connection to state.
 	APIState() *api.State
+	// AgentConfig returns the agent config for the machine that is being upgraded.
+	AgentConfig() agent.Config
 }
 
 // UpgradeContext is a default Context implementation.
@@ -76,7 +84,8 @@
 	// Work in progress........
 	// Exactly what a context needs is to be determined as the
 	// implementation evolves.
-	st *api.State
+	st          *api.State
+	agentConfig agent.Config
 }
 
 // APIState is defined on the Context interface.
@@ -84,6 +93,11 @@
 	return c.st
 }
 
+// AgentConfig is defined on the Context interface.
+func (c *UpgradeContext) AgentConfig() agent.Config {
+	return c.agentConfig
+}
+
 // upgradeOperation provides base attributes for any upgrade step.
 type upgradeOperation struct {
 	description string
@@ -158,3 +172,24 @@
 	}
 	return nil
 }
+
+type upgradeStep struct {
+	description string
+	targets     []UpgradeTarget
+	run         func(Context) error
+}
+
+// Description is defined on the UpgradeStep interface.
+func (step *upgradeStep) Description() string {
+	return step.description
+}
+
+// Targets is defined on the UpgradeStep interface.
+func (step *upgradeStep) Targets() []UpgradeTarget {
+	return step.targets
+}
+
+// Run is defined on the UpgradeStep interface.
+func (step *upgradeStep) Run(context Context) error {
+	return step.run(context)
+}

=== modified file 'upgrades/upgrade_test.go'
--- upgrades/upgrade_test.go	2014-02-12 07:24:28 +0000
+++ upgrades/upgrade_test.go	2014-02-13 03:37:16 +0000
@@ -10,6 +10,7 @@
 
 	gc "launchpad.net/gocheck"
 
+	"launchpad.net/juju-core/agent"
 	"launchpad.net/juju-core/state/api"
 	jc "launchpad.net/juju-core/testing/checkers"
 	"launchpad.net/juju-core/testing/testbase"
@@ -70,6 +71,10 @@
 	return nil
 }
 
+func (c *mockContext) AgentConfig() agent.Config {
+	return nil
+}
+
 func targets(targets ...upgrades.UpgradeTarget) (upgradeTargets []upgrades.UpgradeTarget) {
 	for _, t := range targets {
 		upgradeTargets = append(upgradeTargets, t)