Merge lp:~thumper/juju-core/better-local-destroy-environ into lp:~go-bot/juju-core/trunk

[Tim Penhey]

Local provider destroy removes upstart scripts forcibly.

During the force destroy part, forcibly remove the
mongo and machine agent upstart scripts.

Tests are added to show that these are removed.
A drive by test was added to show that the containers
are destroyed too.

https://codereview.appspot.com/79690043/

Add some tests for local provider destroy

During the force destroy part, forcably remove the
mongo and machine agent upstart scripts.

Tests are added to show that these are removed.
A drive by test was added to show that the containers
are destroyed too.

https://codereview.appspot.com/79690043/

[Tim Penhey (thumper)]

Reviewers: mp+212544_code.launchpad.net,

Message:
Please take a look.

Description:
Add some tests for local provider destroy

During the force destroy part, forcably remove the
mongo and machine agent upstart scripts.

Tests are added to show that these are removed.
A drive by test was added to show that the containers
are destroyed too.

https://code.launchpad.net/~thumper/juju-core/better-local-destroy-environ/+merge/212544

(do not edit description out of merge proposal)

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

Affected files (+95, -25 lines):
   A [revision details]
   M provider/local/config_test.go
   M provider/local/environ.go
   M provider/local/environ_test.go
   M provider/local/export_test.go

[Andrew Wilkins (axwalk)]

On 2014/03/25 04:19:53, thumper wrote:
> Please take a look.

Lovely, LGTM.

https://codereview.appspot.com/79690043/

[Andrew Wilkins (axwalk)]

Oops, meant to mail this

https://codereview.appspot.com/79690043/diff/1/provider/local/environ.go
File provider/local/environ.go (right):

https://codereview.appspot.com/79690043/diff/1/provider/local/environ.go#newcode443
provider/local/environ.go:443: // Stop the mongo database. It's possible
that the service
// Stop the mongo database and machine agent. It's possible ...

?

https://codereview.appspot.com/79690043/

[Tim Penhey (thumper)]

On 2014/03/25 04:29:55, axw wrote:
> Oops, meant to mail this

https://codereview.appspot.com/79690043/diff/1/provider/local/environ.go
> File provider/local/environ.go (right):

https://codereview.appspot.com/79690043/diff/1/provider/local/environ.go#newcode443
> provider/local/environ.go:443: // Stop the mongo database. It's
possible that
> the service
> // Stop the mongo database and machine agent. It's possible ...

> ?

Done.

https://codereview.appspot.com/79690043/

[Go Bot (go-bot)]

The attempt to merge lp:~thumper/juju-core/better-local-destroy-environ into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core 0.019s
ok launchpad.net/juju-core/agent 1.037s
ok launchpad.net/juju-core/agent/mongo 0.607s
ok launchpad.net/juju-core/agent/tools 0.184s
ok launchpad.net/juju-core/bzr 5.184s
ok launchpad.net/juju-core/cert 2.875s
ok launchpad.net/juju-core/charm 0.419s
? launchpad.net/juju-core/charm/hooks [no test files]
? launchpad.net/juju-core/charm/testing [no test files]
ok launchpad.net/juju-core/cloudinit 0.030s
ok launchpad.net/juju-core/cloudinit/sshinit 0.757s
ok launchpad.net/juju-core/cmd 0.164s
ok launchpad.net/juju-core/cmd/charm-admin 0.759s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 200.061s
ok launchpad.net/juju-core/cmd/jujud 63.958s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 8.824s
? launchpad.net/juju-core/cmd/plugins/juju-restore [no test files]
ok launchpad.net/juju-core/cmd/plugins/local 0.158s
? launchpad.net/juju-core/cmd/plugins/local/juju-local [no test files]
ok launchpad.net/juju-core/constraints 0.026s
ok launchpad.net/juju-core/container 0.052s
ok launchpad.net/juju-core/container/factory 0.046s
ok launchpad.net/juju-core/container/kvm 0.232s
ok launchpad.net/juju-core/container/kvm/mock 0.053s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 4.316s
? launchpad.net/juju-core/container/lxc/mock [no test files]
? launchpad.net/juju-core/container/lxc/testing [no test files]
? launchpad.net/juju-core/container/testing [no test files]
ok launchpad.net/juju-core/downloader 5.213s
ok launchpad.net/juju-core/environs 2.354s
ok launchpad.net/juju-core/environs/bootstrap 10.152s
ok launchpad.net/juju-core/environs/cloudinit 0.441s
ok launchpad.net/juju-core/environs/config 2.806s
ok launchpad.net/juju-core/environs/configstore 0.036s
ok launchpad.net/juju-core/environs/filestorage 0.026s
ok launchpad.net/juju-core/environs/httpstorage 0.744s
ok launchpad.net/juju-core/environs/imagemetadata 0.433s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.051s
ok launchpad.net/juju-core/environs/jujutest 0.154s
ok launchpad.net/juju-core/environs/manual 14.050s
ok launchpad.net/juju-core/environs/simplestreams 0.265s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 0.880s
ok launchpad.net/juju-core/environs/storage 0.841s
ok launchpad.net/juju-core/environs/sync 43.653s
ok launchpad.net/juju-core/environs/testing 0.171s
ok launchpad.net/juju-core/environs/tools 4.443s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.010s
ok launchpad.net/juju-core/instance 0.018s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 18.047s
ok launchpad.net/juju-core/juju/arch...

=== modified file 'provider/local/config_test.go'
--- provider/local/config_test.go	2014-02-13 02:46:58 +0000
+++ provider/local/config_test.go	2014-03-25 06:20:24 +0000
@@ -106,8 +106,7 @@
 }
 
 func (s *configSuite) TestBootstrapAsRoot(c *gc.C) {
-	restore := local.SetRootCheckFunction(func() bool { return true })
-	defer restore()
+	s.PatchValue(local.CheckIfRoot, func() bool { return true })
 	env, err := local.Provider.Prepare(testing.Context(c), minimalConfig(c))
 	c.Assert(err, gc.IsNil)
 	err = env.Bootstrap(testing.Context(c), constraints.Value{})

=== modified file 'provider/local/environ.go'
--- provider/local/environ.go	2014-03-25 02:46:17 +0000
+++ provider/local/environ.go	2014-03-25 06:20:24 +0000
@@ -39,6 +39,7 @@
 	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/state/api"
 	"launchpad.net/juju-core/state/api/params"
+	"launchpad.net/juju-core/upstart"
 	"launchpad.net/juju-core/utils/shell"
 	"launchpad.net/juju-core/version"
 	"launchpad.net/juju-core/worker/terminationworker"
@@ -439,6 +440,12 @@
 			}
 		}
 	}
+	// Stop the mongo database and machine agent. It's possible that the
+	// service doesn't exist or is not running, so don't check the error.
+	upstart.NewService(env.mongoServiceName()).StopAndRemove()
+	upstart.NewService(env.machineAgentServiceName()).StopAndRemove()
+
+	// Finally, remove the data-dir.
 	if err := os.RemoveAll(env.config.rootDir()); err != nil && !os.IsNotExist(err) {
 		return err
 	}

=== modified file 'provider/local/environ_test.go'
--- provider/local/environ_test.go	2014-03-25 02:46:17 +0000
+++ provider/local/environ_test.go	2014-03-25 06:20:24 +0000
@@ -4,6 +4,7 @@
 package local_test
 
 import (
+	"fmt"
 	"io/ioutil"
 	"os"
 	"path/filepath"
@@ -14,6 +15,9 @@
 
 	coreCloudinit "launchpad.net/juju-core/cloudinit"
 	"launchpad.net/juju-core/constraints"
+	"launchpad.net/juju-core/container"
+	"launchpad.net/juju-core/container/lxc"
+	containertesting "launchpad.net/juju-core/container/testing"
 	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/environs/cloudinit"
 	"launchpad.net/juju-core/environs/config"
@@ -25,6 +29,7 @@
 	"launchpad.net/juju-core/provider/local"
 	"launchpad.net/juju-core/state/api/params"
 	coretesting "launchpad.net/juju-core/testing"
+	"launchpad.net/juju-core/upstart"
 )
 
 const echoCommandScript = "#!/bin/sh\necho $0 \"$@\" >> $0.args"
@@ -93,7 +98,6 @@
 type localJujuTestSuite struct {
 	baseProviderSuite
 	jujutest.Tests
-	restoreRootCheck   func()
 	oldUpstartLocation string
 	testPath           string
 	dbServiceName      string
@@ -115,17 +119,20 @@
 
 	// Add in an admin secret
 	s.Tests.TestConfig["admin-secret"] = "sekrit"
-	s.restoreRootCheck = local.SetRootCheckFunction(func() bool { return false })
+	s.PatchValue(local.CheckIfRoot, func() bool { return false })
 	s.Tests.SetUpTest(c)
 
 	cfg, err := config.New(config.NoDefaults, s.TestConfig)
 	c.Assert(err, gc.IsNil)
 	s.dbServiceName = "juju-db-" + local.ConfigNamespace(cfg)
+
+	s.PatchValue(local.FinishBootstrap, func(mcfg *cloudinit.MachineConfig, cloudcfg *coreCloudinit.Config, ctx environs.BootstrapContext) error {
+		return nil
+	})
 }
 
 func (s *localJujuTestSuite) TearDownTest(c *gc.C) {
 	s.Tests.TearDownTest(c)
-	s.restoreRootCheck()
 	s.baseProviderSuite.TearDownTest(c)
 }
 
@@ -179,9 +186,6 @@
 }
 
 func (s *localJujuTestSuite) TestDestroy(c *gc.C) {
-	s.PatchValue(local.FinishBootstrap, func(mcfg *cloudinit.MachineConfig, cloudcfg *coreCloudinit.Config, ctx environs.BootstrapContext) error {
-		return nil
-	})
 	env := s.testBootstrap(c, minimalConfig(c))
 	err := env.Destroy()
 	// Succeeds because there's no "agents" directory,
@@ -191,16 +195,17 @@
 	c.Assert(s.fakesudo+".args", jc.DoesNotExist)
 }
 
-func (s *localJujuTestSuite) TestDestroyCallSudo(c *gc.C) {
-	s.PatchValue(local.FinishBootstrap, func(mcfg *cloudinit.MachineConfig, cloudcfg *coreCloudinit.Config, ctx environs.BootstrapContext) error {
-		return nil
-	})
-	env := s.testBootstrap(c, minimalConfig(c))
+func (s *localJujuTestSuite) makeAgentsDir(c *gc.C, env environs.Environ) {
 	rootDir := env.Config().AllAttrs()["root-dir"].(string)
 	agentsDir := filepath.Join(rootDir, "agents")
 	err := os.Mkdir(agentsDir, 0755)
 	c.Assert(err, gc.IsNil)
-	err = env.Destroy()
+}
+
+func (s *localJujuTestSuite) TestDestroyCallSudo(c *gc.C) {
+	env := s.testBootstrap(c, minimalConfig(c))
+	s.makeAgentsDir(c, env)
+	err := env.Destroy()
 	c.Assert(err, gc.IsNil)
 	data, err := ioutil.ReadFile(s.fakesudo + ".args")
 	c.Assert(err, gc.IsNil)
@@ -217,10 +222,71 @@
 	c.Assert(string(data), gc.Equals, strings.Join(expected, " ")+"\n")
 }
 
+func (s *localJujuTestSuite) makeFakeUpstartScripts(c *gc.C, env environs.Environ,
+) (mongo *upstart.Service, machineAgent *upstart.Service) {
+	upstartDir := c.MkDir()
+	s.PatchValue(&upstart.InitDir, upstartDir)
+	s.MakeTool(c, "start", `echo "some-service start/running, process 123"`)
+
+	namespace := env.Config().AllAttrs()["namespace"].(string)
+	mongo = upstart.NewService(fmt.Sprintf("juju-db-%s", namespace))
+	mongoConf := upstart.Conf{
+		Service: *mongo,
+		Desc:    "fake mongo",
+		Cmd:     "echo FAKE",
+	}
+	err := mongoConf.Install()
+	c.Assert(err, gc.IsNil)
+	c.Assert(mongo.Installed(), jc.IsTrue)
+
+	machineAgent = upstart.NewService(fmt.Sprintf("juju-agent-%s", namespace))
+	agentConf := upstart.Conf{
+		Service: *machineAgent,
+		Desc:    "fake agent",
+		Cmd:     "echo FAKE",
+	}
+	err = agentConf.Install()
+	c.Assert(err, gc.IsNil)
+	c.Assert(machineAgent.Installed(), jc.IsTrue)
+
+	return mongo, machineAgent
+}
+
+func (s *localJujuTestSuite) TestDestroyRemovesUpstartServices(c *gc.C) {
+	env := s.testBootstrap(c, minimalConfig(c))
+	s.makeAgentsDir(c, env)
+	mongo, machineAgent := s.makeFakeUpstartScripts(c, env)
+	s.PatchValue(local.CheckIfRoot, func() bool { return true })
+
+	err := env.Destroy()
+	c.Assert(err, gc.IsNil)
+
+	c.Assert(mongo.Installed(), jc.IsFalse)
+	c.Assert(machineAgent.Installed(), jc.IsFalse)
+}
+
+func (s *localJujuTestSuite) TestDestroyRemovesContainers(c *gc.C) {
+	env := s.testBootstrap(c, minimalConfig(c))
+	s.makeAgentsDir(c, env)
+	s.PatchValue(local.CheckIfRoot, func() bool { return true })
+
+	namespace := env.Config().AllAttrs()["namespace"].(string)
+	manager, err := lxc.NewContainerManager(container.ManagerConfig{
+		container.ConfigName:   namespace,
+		container.ConfigLogDir: "logdir",
+	})
+	c.Assert(err, gc.IsNil)
+
+	machine1 := containertesting.CreateContainer(c, manager, "1")
+
+	err = env.Destroy()
+	c.Assert(err, gc.IsNil)
+
+	container := s.Factory.New(string(machine1.Id()))
+	c.Assert(container.IsConstructed(), jc.IsFalse)
+}
+
 func (s *localJujuTestSuite) TestBootstrapRemoveLeftovers(c *gc.C) {
-	s.PatchValue(local.FinishBootstrap, func(mcfg *cloudinit.MachineConfig, cloudcfg *coreCloudinit.Config, ctx environs.BootstrapContext) error {
-		return nil
-	})
 	cfg := minimalConfig(c)
 	rootDir := cfg.AllAttrs()["root-dir"].(string)
 

=== modified file 'provider/local/export_test.go'
--- provider/local/export_test.go	2014-03-18 15:43:41 +0000
+++ provider/local/export_test.go	2014-03-25 06:20:24 +0000
@@ -10,6 +10,7 @@
 )
 
 var (
+	CheckIfRoot      = &checkIfRoot
 	CheckLocalPort   = &checkLocalPort
 	DetectAptProxies = &detectAptProxies
 	FinishBootstrap  = &finishBootstrap
@@ -19,14 +20,6 @@
 	UserCurrent      = &userCurrent
 )
 
-// SetRootCheckFunction allows tests to override the check for a root user.
-// The return value is the function to restore the old value.
-func SetRootCheckFunction(f func() bool) func() {
-	old := checkIfRoot
-	checkIfRoot = f
-	return func() { checkIfRoot = old }
-}
-
 // ConfigNamespace returns the result of the namespace call on the
 // localConfig.
 func ConfigNamespace(cfg *config.Config) string {