Merge lp:~axwalk/juju-core/jujud-uninstall-mongo into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

cmd/jujud: remove mongo and data-dir on teardown

Two new agent.conf keys are introduced:
 - AGENT_SERVICE_NAME, and
 - MONGO_SERVICE_NAME.

The former is always added, the latter only
for state servers. For backwards-compatibility
we fall back to os.Getenv("UPSTART_JOB") for
AGENT_SERVICE_NAME.

https://codereview.appspot.com/28270043/

cmd/jujud: remove mongo and data-dir on teardown

Two new agent.conf keys are introduced:
 - AGENT_SERVICE_NAME, and
 - MONGO_SERVICE_NAME.

The former is always added, the latter only
for state servers. For backwards-compatibility
we fall back to os.Getenv("UPSTART_JOB") for
AGENT_SERVICE_NAME.

https://codereview.appspot.com/28270043/

[Andrew Wilkins (axwalk)]

Reviewers: mp+195565_code.launchpad.net,

Message:
Please take a look.

Description:
cmd/jujud: remove mongo and data-dir on teardown

Note: this does not remove the services when
using the local provider, as it gives the services
non-standard names. It doesn't have any negative
effects, either, unless you're running the local
provider on a machine provisioned with Juju; but
that's not really sane is it?

https://code.launchpad.net/~axwalk/juju-core/jujud-uninstall-mongo/+merge/195565

(do not edit description out of merge proposal)

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

Affected files (+38, -11 lines):
   A [revision details]
   M cmd/jujud/machine.go
   M cmd/jujud/machine_test.go
   M environs/cloudinit/cloudinit.go
   M upstart/service.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-20131118065206-jpp233o069gz8774
+New revision: <email address hidden>

Index: upstart/service.go
=== modified file 'upstart/service.go'
--- upstart/service.go 2013-09-18 01:25:45 +0000
+++ upstart/service.go 2013-11-18 09:36:25 +0000
@@ -13,6 +13,10 @@
  const (
   maxMongoFiles = 65000
   maxAgentFiles = 20000
+
+ // MongoServiceName is the default name of the upstart
+ // service that runs mongod for Juju.
+ MongoServiceName = "juju-db"
  )

  // MongoUpstartService returns the upstart config for the mongo state
service.
@@ -40,6 +44,12 @@
   }
  }

+// MachineAgentServiceName returns the upstart service name for
+// a machine agent with the given tag.
+func MachineAgentServiceName(tag string) string {
+ return "jujud-" + tag
+}
+
  // MachineAgentUpstartService returns the upstart config for a machine
agent
  // based on the tag and machineId passed in.
  func MachineAgentUpstartService(name, toolsDir, dataDir, logDir, tag,
machineId string, env map[string]string) *Conf {

Index: cmd/jujud/machine.go
=== modified file 'cmd/jujud/machine.go'
--- cmd/jujud/machine.go 2013-10-02 23:18:21 +0000
+++ cmd/jujud/machine.go 2013-11-18 09:36:25 +0000
@@ -304,13 +304,23 @@
   return names.MachineTag(a.MachineId)
  }

-func (m *MachineAgent) uninstallAgent() error {
- // TODO(axw) get this from agent config when it's available
- name := os.Getenv("UPSTART_JOB")
- if name != "" {
- return upstart.NewService(name).Remove()
- }
- return nil
+func (a *MachineAgent) uninstallAgent() error {
+ // NOTE: this will not stop/remove upstart services
+ // for the local provider (only), which has different
+ // service names for different users/environments.
+ service :=
upstart.NewService(upstart.MachineAgentServiceName(a.Conf.config.Tag()))
+ if err := service.Remove(); err != nil {
+ return 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
+ // if the upstart config exists.
+ service = upstart.NewService(upstart.MongoServiceName)
+ if err := service.StopAndRemove(); err != nil {
+ return err
+ }
+ return os....

[John A Meinel (jameinel)]

Did you test this works live?

It looks reasonable to me, and seems a lot more straightforward than
creating a script that we then figure out how to run later.

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

https://codereview.appspot.com/28270043/diff/1/cmd/jujud/machine.go#newcode313
cmd/jujud/machine.go:313: return err
I wonder if we actually want to just return here.
As this is a 'cleanup' style function, I would think you'd want to run
all cleanups and then if any of them had errors report them.

https://codereview.appspot.com/28270043/diff/1/upstart/service.go
File upstart/service.go (right):

https://codereview.appspot.com/28270043/diff/1/upstart/service.go#newcode19
upstart/service.go:19: MongoServiceName = "juju-db"
My first thought is that this shouldn't be data in 'upstart'. Because
the upstart library shouldn't know about agent names (juju should know
about it, and communicate that *to* upstart).

Though it is also strange that upstart knows how many MongoFiles to
support, so I may be off base on that.

https://codereview.appspot.com/28270043/diff/1/upstart/service.go#newcode51
upstart/service.go:51: }
It does feel like if we are going to be providing a new Public api, that
we should be adding a test for how that API behaves.

https://codereview.appspot.com/28270043/

[Andrew Wilkins (axwalk)]

> Did you test this works live?

Yes indeed. Works with the null provider. Doesn't break the local
provider (but doesn't remove its services).

I'm going to take a look at adding the upstart service names into
agent.conf again, as I think it may clean things up a bit w.r.t. service
names. We'd solve the local-provider disparity (which is really just an
ugliness, and not a real problem), and do away with the need for
exporting the service names from a package (like upstart).

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

https://codereview.appspot.com/28270043/diff/1/cmd/jujud/machine.go#newcode313
cmd/jujud/machine.go:313: return err
On 2013/11/18 09:56:46, jameinel wrote:
> I wonder if we actually want to just return here.
> As this is a 'cleanup' style function, I would think you'd want to run
all
> cleanups and then if any of them had errors report them.

Fair call. I'll update to do that.

https://codereview.appspot.com/28270043/diff/1/upstart/service.go
File upstart/service.go (right):

https://codereview.appspot.com/28270043/diff/1/upstart/service.go#newcode19
upstart/service.go:19: MongoServiceName = "juju-db"
On 2013/11/18 09:56:46, jameinel wrote:
> My first thought is that this shouldn't be data in 'upstart'. Because
the
> upstart library shouldn't know about agent names (juju should know
about it, and
> communicate that *to* upstart).

So, I agree, but I also want to keep it close to the definition of the
upstart service. Probably the right thing to do is move it all to
environs/cloudinit.

> Though it is also strange that upstart knows how many MongoFiles to
support, so
> I may be off base on that.

I think it's a bit off that there's any notion of Mongo or MachineAgent
in the upstart package at all.

https://codereview.appspot.com/28270043/diff/1/upstart/service.go#newcode51
upstart/service.go:51: }
On 2013/11/18 09:56:46, jameinel wrote:
> It does feel like if we are going to be providing a new Public api,
that we
> should be adding a test for how that API behaves.

Yes, sorry, I'll add one.

https://codereview.appspot.com/28270043/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/28270043/

[Roger Peppe (rogpeppe)]

LGTM with some minor thoughts below.

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

https://codereview.appspot.com/28270043/diff/20001/cmd/jujud/machine.go#newcode316
cmd/jujud/machine.go:316: errors = append(errors, err)
I think that having a bunch of errors without context might prove very
hard for a user to interpret if something goes wrong.

I think things might be more understandable if we added some context to
each one;
e.g.

errors = append(errors, fmt.Errorf("cannot remove service %q: %v",
agentServiceName, err))

https://codereview.appspot.com/28270043/diff/20001/cmd/jujud/machine.go#newcode334
cmd/jujud/machine.go:334: }
if len(errors) == 1 {
     return fmt.Errorf("uninstall failed: %v", errors[0])
}

to save the square brackets when only one thing fails?
probably not worth it, just a thought.

https://codereview.appspot.com/28270043/diff/20001/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/28270043/diff/20001/environs/cloudinit/cloudinit.go#newcode313
environs/cloudinit/cloudinit.go:313:
acfg.SetValue(agent.MongoServiceName, mongoServiceName)
Is there nowhere to test this functionality?

https://codereview.appspot.com/28270043/

[Andrew Wilkins (axwalk)]

Please take a look.

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

https://codereview.appspot.com/28270043/diff/20001/cmd/jujud/machine.go#newcode316
cmd/jujud/machine.go:316: errors = append(errors, err)
On 2013/11/19 09:50:39, rog wrote:
> I think that having a bunch of errors without context might prove very
hard for
> a user to interpret if something goes wrong.

> I think things might be more understandable if we added some context
to each
> one;
> e.g.

> errors = append(errors, fmt.Errorf("cannot remove service %q: %v",
> agentServiceName, err))

Good call. Done.

https://codereview.appspot.com/28270043/diff/20001/cmd/jujud/machine.go#newcode334
cmd/jujud/machine.go:334: }
On 2013/11/19 09:50:39, rog wrote:
> if len(errors) == 1 {
> return fmt.Errorf("uninstall failed: %v", errors[0])
> }

> to save the square brackets when only one thing fails?
> probably not worth it, just a thought.

I concluded it wasn't worthwhile, as it made the code pretty ugly. The
error message will only ever go to the machine log.

https://codereview.appspot.com/28270043/diff/20001/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/28270043/diff/20001/environs/cloudinit/cloudinit.go#newcode313
environs/cloudinit/cloudinit.go:313:
acfg.SetValue(agent.MongoServiceName, mongoServiceName)
On 2013/11/19 09:50:39, rog wrote:
> Is there nowhere to test this functionality?

Gah, I intended to do that, it just slipped my mind. Added a test.

https://codereview.appspot.com/28270043/

[Go Bot (go-bot)]

Attempt to merge into lp:juju-core failed due to conflicts:

text conflict in environs/cloudinit/cloudinit_test.go

=== modified file 'agent/agent.go'
--- agent/agent.go	2013-11-19 13:46:00 +0000
+++ agent/agent.go	2013-11-20 02:15:23 +0000
@@ -28,6 +28,8 @@
 	StorageAddr       = "STORAGE_ADDR"
 	SharedStorageDir  = "SHARED_STORAGE_DIR"
 	SharedStorageAddr = "SHARED_STORAGE_ADDR"
+	AgentServiceName  = "AGENT_SERVICE_NAME"
+	MongoServiceName  = "MONGO_SERVICE_NAME"
 )
 
 // The Config interface is the sole way that the agent gets access to the

=== modified file 'cmd/jujud/machine.go'
--- cmd/jujud/machine.go	2013-11-14 04:28:28 +0000
+++ cmd/jujud/machine.go	2013-11-20 02:15:23 +0000
@@ -337,13 +337,35 @@
 	return names.MachineTag(a.MachineId)
 }
 
-func (m *MachineAgent) uninstallAgent() error {
-	// TODO(axw) get this from agent config when it's available
-	name := os.Getenv("UPSTART_JOB")
-	if name != "" {
-		return upstart.NewService(name).Remove()
-	}
-	return nil
+func (a *MachineAgent) uninstallAgent() error {
+	var errors []error
+	agentServiceName := a.Conf.config.Value(agent.AgentServiceName)
+	if agentServiceName == "" {
+		// For backwards compatibility, handle lack of AgentServiceName.
+		agentServiceName = os.Getenv("UPSTART_JOB")
+	}
+	if agentServiceName != "" {
+		if err := upstart.NewService(agentServiceName).Remove(); err != nil {
+			errors = append(errors, fmt.Errorf("cannot remove service %q: %v", agentServiceName, 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
+	// if the upstart config exists.
+	mongoServiceName := a.Conf.config.Value(agent.MongoServiceName)
+	if mongoServiceName != "" {
+		if err := upstart.NewService(mongoServiceName).StopAndRemove(); err != nil {
+			errors = append(errors, fmt.Errorf("cannot stop/remove service %q: %v", mongoServiceName, err))
+		}
+	}
+	if err := os.RemoveAll(a.Conf.dataDir); err != nil {
+		errors = append(errors, err)
+	}
+	if len(errors) == 0 {
+		return nil
+	}
+	return fmt.Errorf("uninstall failed: %v", errors)
 }
 
 // Below pieces are used for testing,to give us access to the *State opened

=== modified file 'cmd/jujud/machine_test.go'
--- cmd/jujud/machine_test.go	2013-11-11 22:19:10 +0000
+++ cmd/jujud/machine_test.go	2013-11-20 02:15:23 +0000
@@ -149,11 +149,15 @@
 	a := s.newAgent(c, m)
 	err = runWithTimeout(a)
 	c.Assert(err, gc.IsNil)
+}
 
-	// try again with the machine removed.
+func (s *MachineSuite) TestWithRemovedMachine(c *gc.C) {
+	m, _, _ := s.primeAgent(c, state.JobHostUnits, state.JobManageState)
+	err := m.EnsureDead()
+	c.Assert(err, gc.IsNil)
 	err = m.Remove()
 	c.Assert(err, gc.IsNil)
-	a = s.newAgent(c, m)
+	a := s.newAgent(c, m)
 	err = runWithTimeout(a)
 	c.Assert(err, gc.IsNil)
 }

=== modified file 'environs/cloudinit/cloudinit.go'
--- environs/cloudinit/cloudinit.go	2013-11-19 00:18:18 +0000
+++ environs/cloudinit/cloudinit.go	2013-11-20 02:15:23 +0000
@@ -303,6 +303,10 @@
 	if err != nil {
 		return nil, err
 	}
+	acfg.SetValue(agent.AgentServiceName, machineAgentServiceName(tag))
+	if cfg.StateServer {
+		acfg.SetValue(agent.MongoServiceName, mongoServiceName)
+	}
 	cmds, err := acfg.WriteCommands()
 	if err != nil {
 		return nil, err
@@ -311,6 +315,12 @@
 	return acfg, nil
 }
 
+const mongoServiceName = "juju-db"
+
+func machineAgentServiceName(tag string) string {
+	return "jujud-" + tag
+}
+
 func (cfg *MachineConfig) addMachineAgentToBoot(c *cloudinit.Config, tag, machineId string) error {
 	// Make the agent run via a symbolic link to the actual tools
 	// directory, so it can upgrade itself without needing to change
@@ -319,7 +329,7 @@
 	// TODO(dfc) ln -nfs, so it doesn't fail if for some reason that the target already exists
 	c.AddScripts(fmt.Sprintf("ln -s %v %s", cfg.Tools.Version, shquote(toolsDir)))
 
-	name := "jujud-" + tag
+	name := machineAgentServiceName(tag)
 	conf := upstart.MachineAgentUpstartService(name, toolsDir, cfg.DataDir, "/var/log/juju/", tag, machineId, nil)
 	cmds, err := conf.InstallCommands()
 	if err != nil {
@@ -340,7 +350,8 @@
 		"dd bs=1M count=1 if=/dev/zero of="+dbDir+"/journal/prealloc.2",
 	)
 
-	conf := upstart.MongoUpstartService("juju-db", cfg.DataDir, dbDir, cfg.StatePort)
+	name := mongoServiceName
+	conf := upstart.MongoUpstartService(name, cfg.DataDir, dbDir, cfg.StatePort)
 	cmds, err := conf.InstallCommands()
 	if err != nil {
 		return fmt.Errorf("cannot make cloud-init upstart script for the state database: %v", err)

=== modified file 'environs/cloudinit/cloudinit_test.go'
--- environs/cloudinit/cloudinit_test.go	2013-11-19 00:18:18 +0000
+++ environs/cloudinit/cloudinit_test.go	2013-11-20 02:15:23 +0000
@@ -17,9 +17,11 @@
 	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/environs/cloudinit"
 	"launchpad.net/juju-core/environs/config"
+	"launchpad.net/juju-core/names"
 	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/state/api"
 	"launchpad.net/juju-core/testing"
+	jc "launchpad.net/juju-core/testing/checkers"
 	"launchpad.net/juju-core/testing/testbase"
 	"launchpad.net/juju-core/tools"
 	"launchpad.net/juju-core/version"
@@ -324,6 +326,21 @@
 	return tools
 }
 
+func getAgentConfig(c *gc.C, tag string, scripts []string) (cfg string) {
+	re := regexp.MustCompile(`printf '%s\\n' '([^']+)' > .*agents/` + regexp.QuoteMeta(tag) + `/agent\.conf`)
+	found := false
+	for _, s := range scripts {
+		m := re.FindStringSubmatch(s)
+		if m == nil {
+			continue
+		}
+		cfg = m[1]
+		found = true
+	}
+	c.Assert(found, gc.Equals, true)
+	return cfg
+}
+
 // check that any --env-config $base64 is valid and matches t.cfg.Config
 func checkEnvConfig(c *gc.C, cfg *config.Config, x map[interface{}]interface{}, scripts []string) {
 	re := regexp.MustCompile(`--env-config '([^']+)'`)
@@ -376,10 +393,16 @@
 			checkEnvConfig(c, test.cfg.Config, x, scripts)
 		}
 		checkPackage(c, x, "git", true)
+		tag := names.MachineTag(test.cfg.MachineId)
+		acfg := getAgentConfig(c, tag, scripts)
+		c.Assert(acfg, jc.Contains, "AGENT_SERVICE_NAME: jujud-"+tag)
 		if test.cfg.StateServer {
 			checkPackage(c, x, "mongodb-server", true)
 			source := "ppa:juju/stable"
 			checkAptSource(c, x, source, "", test.cfg.NeedMongoPPA())
+			c.Assert(acfg, jc.Contains, "MONGO_SERVICE_NAME: juju-db")
+		} else {
+			c.Assert(acfg, gc.Not(jc.Contains), "MONGO_SERVICE_NAME")
 		}
 		source := "deb http://ubuntu-cloud.archive.canonical.com/ubuntu precise-updates/cloud-tools main"
 		needCloudArchive := test.cfg.Tools.Version.Series == "precise"

=== modified file 'provider/local/environ.go'
--- provider/local/environ.go	2013-11-18 05:41:36 +0000
+++ provider/local/environ.go	2013-11-20 02:15:23 +0000
@@ -507,6 +507,8 @@
 		agent.StorageAddr:       env.config.storageAddr(),
 		agent.SharedStorageDir:  env.config.sharedStorageDir(),
 		agent.SharedStorageAddr: env.config.sharedStorageAddr(),
+		agent.AgentServiceName:  env.machineAgentServiceName(),
+		agent.MongoServiceName:  env.mongoServiceName(),
 	}
 	// NOTE: the state address HAS to be localhost, otherwise the mongo
 	// initialization fails.  There is some magic code somewhere in the mongo