Merge lp:~axwalk/juju-core/lp1263586-force-symlink-jujurun into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

environs/cloudinit: force symlink of juju-run

Manual provisioning fails if the juju-run symlink
exists. We should always recreate the symlink if
it exists, by passing -f to ln.

Also: moved symlinking of juju-run to the machine
agent, so that upgrades do the right thing.

Fixes #1263586

https://codereview.appspot.com/47380044/

environs/cloudinit: force symlink of juju-run

Manual provisioning fails if the juju-run symlink
exists. We should always recreate the symlink if
it exists, by passing -f to ln.

Also: moved symlinking of juju-run to the machine
agent, so that upgrades do the right thing.

Fixes #1263586

https://codereview.appspot.com/47380044/

[Andrew Wilkins (axwalk)]

Reviewers: mp+200368_code.launchpad.net,

Message:
Please take a look.

Description:
environs/cloudinit: force symlink of juju-run

Manual provisioning fails if the juju-run symlink
exists. We should always recreate the symlink if
it exists, by passing -f to ln.

Fixes #1263586

https://code.launchpad.net/~axwalk/juju-core/lp1263586-force-symlink-jujurun/+merge/200368

(do not edit description out of merge proposal)

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

Affected files (+5, -3 lines):
   A [revision details]
   M environs/cloudinit/cloudinit.go
   M environs/cloudinit/cloudinit_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20140102083548-er3lr2qx0y8nwd2x
+New revision: <email address hidden>

Index: environs/cloudinit/cloudinit.go
=== modified file 'environs/cloudinit/cloudinit.go'
--- environs/cloudinit/cloudinit.go 2013-12-18 15:15:23 +0000
+++ environs/cloudinit/cloudinit.go 2014-01-03 06:09:31 +0000
@@ -261,7 +261,7 @@
    // 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),
+ fmt.Sprintf("ln -f -s %s/tools/%s/jujud /usr/local/bin/juju-run",
cfg.DataDir, machineTag),
   )

   // Add the cloud archive cloud-tools pocket to apt sources

Index: environs/cloudinit/cloudinit_test.go
=== modified file 'environs/cloudinit/cloudinit_test.go'
--- environs/cloudinit/cloudinit_test.go 2013-12-17 12:13:50 +0000
+++ environs/cloudinit/cloudinit_test.go 2014-01-03 06:09:31 +0000
@@ -111,7 +111,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
+ln -f -s /var/lib/juju/tools/machine-0/jujud /usr/local/bin/juju-run
  install -D -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
@@ -224,7 +224,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 -f -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/juju...

[William Reade (fwereade)]

not your fault, but NOT LGTM -- I'm pretty sure we need to make this
symlink the machine agent's responsibility, not the cloudinit script's
(otherwise, where will it come from in an upgraded environment?).

So the correct fix is, I think, just a move of the functionality. Sane?

https://codereview.appspot.com/47380044/

[Andrew Wilkins (axwalk)]

On 2014/01/03 13:16:11, fwereade wrote:
> not your fault, but NOT LGTM -- I'm pretty sure we need to make this
symlink the
> machine agent's responsibility, not the cloudinit script's (otherwise,
where
> will it come from in an upgraded environment?).

> So the correct fix is, I think, just a move of the functionality.
Sane?

Yes, very much. Fix incoming soon.

https://codereview.appspot.com/47380044/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/47380044/

[John A Meinel (jameinel)]

I won't block this if we really need to have it, but it is part of why I
feel having a 'juju-run' command that is executed via SSH is the wrong
way about it. (Rather, the running agent should fire of the command to
execute, rather than have it driven via SSH from the API server.)

The actual code seems ok to me, though I wonder if we need to patch a
couple more places to avoid accidentally trying to write to
/usr/local/bin in the test suite.

https://codereview.appspot.com/47380044/diff/20001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/47380044/diff/20001/cmd/jujud/machine.go#newcode354
cmd/jujud/machine.go:354: return os.Symlink(jujud, jujuRun)
So this is something that concerned me a lot with our methodology. When
we upgrade a jujud, is it going to fix this symlink? It really seems
wrong to have a global 'juju-run' binary that points into a
version-specific tools directory. (Presumably we can only have one
machine agent, but we might have multiple unit agents, and the unit
agents could be running different versions. If the idea is to run in a
'hook context' should it run in the context the agent is running?)

I'm personally opposed to the idea of having a /usr/local/bin/juju-run
symlink, but if it has been decided that we must, the code looks sane.

As a different think, is this going to get us in trouble when running
the test suite? My local user certainly doesn't have rights to create
/usr/local/bin/juju-run on my machine.

https://codereview.appspot.com/47380044/diff/20001/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/47380044/diff/20001/cmd/jujud/machine_test.go#newcode65
cmd/jujud/machine_test.go:65: s.PatchValue(&jujuRun,
filepath.Join(c.MkDir(), "juju-run"))
This seems dangerous, in that anything test suite that creates a Machine
job needs to add this patch.

Is this really the only test suite that creates machine agents? I would
expect to see a lot more agents being run. Doesn't agent_test also need
to create them?

https://codereview.appspot.com/47380044/

[Andrew Wilkins (axwalk)]

On 2014/01/06 12:53:47, jameinel wrote:
> I won't block this if we really need to have it, but it is part of why
I feel
> having a 'juju-run' command that is executed via SSH is the wrong way
about it.
> (Rather, the running agent should fire of the command to execute,
rather than
> have it driven via SSH from the API server.)

Since we do have it already, it needs to not break things :)

> The actual code seems ok to me, though I wonder if we need to patch a
couple
> more places to avoid accidentally trying to write to /usr/local/bin in
the test
> suite.

"make check" passes, and I don't run my tests as root.

https://codereview.appspot.com/47380044/diff/20001/cmd/jujud/machine.go
> File cmd/jujud/machine.go (right):

https://codereview.appspot.com/47380044/diff/20001/cmd/jujud/machine.go#newcode354
> cmd/jujud/machine.go:354: return os.Symlink(jujud, jujuRun)
> So this is something that concerned me a lot with our methodology.
When we
> upgrade a jujud, is it going to fix this symlink? It really seems
wrong to have
> a global 'juju-run' binary that points into a version-specific tools
directory.
> (Presumably we can only have one machine agent, but we might have
multiple unit
> agents, and the unit agents could be running different versions. If
the idea is
> to run in a 'hook context' should it run in the context the agent is
running?)

> I'm personally opposed to the idea of having a /usr/local/bin/juju-run
symlink,
> but if it has been decided that we must, the code looks sane.

> As a different think, is this going to get us in trouble when running
the test
> suite? My local user certainly doesn't have rights to create
> /usr/local/bin/juju-run on my machine.

Not with the current tests. I don't think this is really any different
from the tests that modify "$HOME/...", after having changed $HOME. That
scares me much more than this change, but they're working.

https://codereview.appspot.com/47380044/diff/20001/cmd/jujud/machine_test.go
> File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/47380044/diff/20001/cmd/jujud/machine_test.go#newcode65
> cmd/jujud/machine_test.go:65: s.PatchValue(&jujuRun,
filepath.Join(c.MkDir(),
> "juju-run"))
> This seems dangerous, in that anything test suite that creates a
Machine job
> needs to add this patch.

> Is this really the only test suite that creates machine agents? I
would expect
> to see a lot more agents being run. Doesn't agent_test also need to
create them?

Yes. agent_test doesn't actually define any tests, it's just for
embedding, and the unit agent doesn't exercise the new code.

https://codereview.appspot.com/47380044/

[William Reade (fwereade)]

LGTM, but I share jam's concerns about that jujuRun var. We've had (and
still have) problems with isolation for the jujud tests, and I'd prefer
to over-isolate than to under-isolate, at least until we break the
package up a bit.

https://codereview.appspot.com/47380044/diff/20001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/47380044/diff/20001/cmd/jujud/machine.go#newcode354
cmd/jujud/machine.go:354: return os.Symlink(jujud, jujuRun)
On 2014/01/06 12:53:48, jameinel wrote:
> So this is something that concerned me a lot with our methodology.
When we
> upgrade a jujud, is it going to fix this symlink? It really seems
wrong to have
> a global 'juju-run' binary that points into a version-specific tools
directory.
> (Presumably we can only have one machine agent, but we might have
multiple unit
> agents, and the unit agents could be running different versions. If
the idea is
> to run in a 'hook context' should it run in the context the agent is
running?)

> I'm personally opposed to the idea of having a /usr/local/bin/juju-run
symlink,
> but if it has been decided that we must, the code looks sane.

We do sadly at this stage need to do this. Fixing this will I think
involve putting the machine agent in charge of the tools running on that
machine, so that it can download the tools for all agents, update a
single symlink, and then bounce them all; it's a good idea regardless,
but it probably makes most sense to do it once we've got a `juju-reboot`
mechanism for hooks (that'll likely involve setting up coordinated
shutdowns of agents anyway).

Sane?

> As a different think, is this going to get us in trouble when running
the test
> suite? My local user certainly doesn't have rights to create
> /usr/local/bin/juju-run on my machine.

I too would rather see more defensive patching of that value, I think --
maybe even in the package test func?

Although what I'd *really* like is for this module to be decomposed a
bit so we can test things like jobs independent of the workers they run,
and initialization independently from running, and so on.

https://codereview.appspot.com/47380044/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/47380044/diff/20001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/47380044/diff/20001/cmd/jujud/machine.go#newcode354
cmd/jujud/machine.go:354: return os.Symlink(jujud, jujuRun)
On 2014/01/13 08:39:20, william.reade wrote:
> On 2014/01/06 12:53:48, jameinel wrote:
> > So this is something that concerned me a lot with our methodology.
When we
> > upgrade a jujud, is it going to fix this symlink? It really seems
wrong to
> have
> > a global 'juju-run' binary that points into a version-specific tools
> directory.
> > (Presumably we can only have one machine agent, but we might have
multiple
> unit
> > agents, and the unit agents could be running different versions. If
the idea
> is
> > to run in a 'hook context' should it run in the context the agent is
running?)
> >
> > I'm personally opposed to the idea of having a
/usr/local/bin/juju-run
> symlink,
> > but if it has been decided that we must, the code looks sane.

> We do sadly at this stage need to do this. Fixing this will I think
involve
> putting the machine agent in charge of the tools running on that
machine, so
> that it can download the tools for all agents, update a single
symlink, and then
> bounce them all; it's a good idea regardless, but it probably makes
most sense
> to do it once we've got a `juju-reboot` mechanism for hooks (that'll
likely
> involve setting up coordinated shutdowns of agents anyway).

> Sane?

Yes, I think so.

> > As a different think, is this going to get us in trouble when
running the test
> > suite? My local user certainly doesn't have rights to create
> > /usr/local/bin/juju-run on my machine.

> I too would rather see more defensive patching of that value, I think
-- maybe
> even in the package test func?

I was going to do an init(), but either way has the desired effect. I
see TestPackage has other initialisation code already, so I'll tack it
on.

> Although what I'd *really* like is for this module to be decomposed a
bit so we
> can test things like jobs independent of the workers they run, and
> initialization independently from running, and so on.

https://codereview.appspot.com/47380044/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/47380044/

=== modified file 'cmd/jujud/machine.go'
--- cmd/jujud/machine.go	2014-01-12 19:48:01 +0000
+++ cmd/jujud/machine.go	2014-01-14 05:46:46 +0000
@@ -53,6 +53,8 @@
 
 var retryDelay = 3 * time.Second
 
+var jujuRun = "/usr/local/bin/juju-run"
+
 // MachineAgent is a cmd.Command responsible for running a machine agent.
 type MachineAgent struct {
 	cmd.CommandBase
@@ -111,6 +113,9 @@
 		return err
 	}
 	charm.CacheDir = filepath.Join(a.Conf.dataDir, "charmcache")
+	if err := a.initAgent(); err != nil {
+		return err
+	}
 
 	// ensureStateWorker ensures that there is a worker that
 	// connects to the state that runs within itself all the workers
@@ -351,6 +356,14 @@
 	return names.MachineTag(a.MachineId)
 }
 
+func (a *MachineAgent) initAgent() error {
+	if err := os.Remove(jujuRun); err != nil && !os.IsNotExist(err) {
+		return err
+	}
+	jujud := filepath.Join(a.Conf.dataDir, "tools", a.Tag(), "jujud")
+	return os.Symlink(jujud, jujuRun)
+}
+
 func (a *MachineAgent) uninstallAgent() error {
 	var errors []error
 	agentServiceName := a.Conf.config.Value(agent.AgentServiceName)
@@ -363,6 +376,10 @@
 			errors = append(errors, fmt.Errorf("cannot remove service %q: %v", agentServiceName, err))
 		}
 	}
+	// Remove the juju-run symlink.
+	if err := os.Remove(jujuRun); err != nil && !os.IsNotExist(err) {
+		errors = append(errors, err)
+	}
 	// The machine agent may terminate without knowing its jobs,
 	// for example if the machine's entry in state was removed.
 	// Thus, we do not rely on jobs here, and instead just check

=== modified file 'cmd/jujud/machine_test.go'
--- cmd/jujud/machine_test.go	2013-12-23 05:18:58 +0000
+++ cmd/jujud/machine_test.go	2014-01-14 05:46:46 +0000
@@ -4,6 +4,7 @@
 package main
 
 import (
+	"os"
 	"path/filepath"
 	"reflect"
 	"strings"
@@ -61,6 +62,7 @@
 func (s *MachineSuite) SetUpTest(c *gc.C) {
 	s.agentSuite.SetUpTest(c)
 	s.TestSuite.SetUpTest(c)
+	os.Remove(jujuRun) // ignore error; may not exist
 }
 
 func (s *MachineSuite) TearDownTest(c *gc.C) {
@@ -651,3 +653,43 @@
 		s.assertCanOpenState(c, conf.Tag(), conf.DataDir())
 	})
 }
+
+func (s *MachineSuite) TestMachineAgentSymlinkJujuRun(c *gc.C) {
+	_, err := os.Stat(jujuRun)
+	c.Assert(err, jc.Satisfies, os.IsNotExist)
+	s.assertJobWithAPI(c, state.JobManageState, func(conf agent.Config, st *api.State) {
+		// juju-run should have been created
+		_, err := os.Stat(jujuRun)
+		c.Assert(err, gc.IsNil)
+	})
+}
+
+func (s *MachineSuite) TestMachineAgentSymlinkJujuRunExists(c *gc.C) {
+	err := os.Symlink("/nowhere/special", jujuRun)
+	c.Assert(err, gc.IsNil)
+	_, err = os.Stat(jujuRun)
+	c.Assert(err, jc.Satisfies, os.IsNotExist)
+	s.assertJobWithAPI(c, state.JobManageState, func(conf agent.Config, st *api.State) {
+		// juju-run should have been recreated
+		_, err := os.Stat(jujuRun)
+		c.Assert(err, gc.IsNil)
+		link, err := os.Readlink(jujuRun)
+		c.Assert(err, gc.IsNil)
+		c.Assert(link, gc.Not(gc.Equals), "/nowhere/special")
+	})
+}
+
+func (s *MachineSuite) TestMachineAgentUninstall(c *gc.C) {
+	m, ac, _ := s.primeAgent(c, state.JobHostUnits, state.JobManageState)
+	err := m.EnsureDead()
+	c.Assert(err, gc.IsNil)
+	a := s.newAgent(c, m)
+	err = runWithTimeout(a)
+	c.Assert(err, gc.IsNil)
+	// juju-run should have been removed on termination
+	_, err = os.Stat(jujuRun)
+	c.Assert(err, jc.Satisfies, os.IsNotExist)
+	// data-dir should have been removed on termination
+	_, err = os.Stat(ac.DataDir())
+	c.Assert(err, jc.Satisfies, os.IsNotExist)
+}

=== modified file 'cmd/jujud/main_test.go'
--- cmd/jujud/main_test.go	2013-09-13 14:48:13 +0000
+++ cmd/jujud/main_test.go	2014-01-14 05:46:46 +0000
@@ -25,18 +25,27 @@
 
 var caCertFile string
 
+func mktemp(prefix string, content string) string {
+	f, err := ioutil.TempFile("", prefix)
+	if err != nil {
+		panic(err)
+	}
+	_, err = f.WriteString(content)
+	if err != nil {
+		panic(err)
+	}
+	f.Close()
+	return f.Name()
+}
+
 func TestPackage(t *stdtesting.T) {
+	// Change the path to "juju-run", so that the
+	// tests don't try to write to /usr/local/bin.
+	jujuRun = mktemp("juju-run", "")
+	defer os.Remove(jujuRun)
+
 	// Create a CA certificate available for all tests.
-	f, err := ioutil.TempFile("", "juju-test-cert")
-	if err != nil {
-		panic(err)
-	}
-	_, err = f.WriteString(testing.CACert)
-	if err != nil {
-		panic(err)
-	}
-	f.Close()
-	caCertFile = f.Name()
+	caCertFile = mktemp("juju-test-cert", testing.CACert)
 	defer os.Remove(caCertFile)
 
 	testing.MgoTestPackage(t)

=== modified file 'environs/cloudinit/cloudinit.go'
--- environs/cloudinit/cloudinit.go	2014-01-05 22:20:24 +0000
+++ environs/cloudinit/cloudinit.go	2014-01-14 05:46:46 +0000
@@ -263,14 +263,6 @@
 	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-20 01:52:22 +0000
+++ environs/cloudinit/cloudinit_test.go	2014-01-14 05:46:46 +0000
@@ -112,7 +112,6 @@
 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 -D -m 600 /dev/null '/var/lib/juju/system-identity'
 printf '%s\\n' '.*' > '/var/lib/juju/system-identity'
 install -D -m 600 /dev/null '/var/lib/juju/server\.pem'
@@ -228,7 +227,6 @@
 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