Merge lp:~thumper/juju-core/lxc-broker into lp:~go-bot/juju-core/trunk

[Tim Penhey]

Implement the lxc broker for the provisioner

Not hooked up yet.

https://codereview.appspot.com/10409049/

Implement the lxc broker for the provisioner

Had to make some test functions and helpers exported.

https://codereview.appspot.com/10409049/

[Tim Penhey (thumper)]

Reviewers: mp+170713_code.launchpad.net,

Message:
Please take a look.

Description:
Implement the lxc broker for the provisioner

Had to make some test functions and helpers exported.

https://code.launchpad.net/~thumper/juju-core/lxc-broker/+merge/170713

Requires:
https://code.launchpad.net/~thumper/juju-core/lxc-container/+merge/169980

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   D container/lxc/export_test.go
   M container/lxc/lxc_test.go
   M container/lxc/mock/mock-lxc.go
   A container/lxc/test.go
   A worker/provisioner/lxc-broker.go
   A worker/provisioner/lxc-broker_test.go
   M worker/provisioner/provisioner.go
   M worker/provisioner/provisioner_task.go

[Ian Booth (wallyworld)]

Looks good with a few suggestions.

https://codereview.appspot.com/10409049/diff/1/container/lxc/test.go
File container/lxc/test.go (right):

https://codereview.appspot.com/10409049/diff/1/container/lxc/test.go#newcode4
container/lxc/test.go:4: package lxc
This limitation of Go really annoys me.
Could we add a big file level comment that these exported methods must
only be used for tests and not for normal business logic. I know each
method mentions the use of tests but I think a global comment would be
good too.

https://codereview.appspot.com/10409049/diff/1/worker/provisioner/lxc-broker.go
File worker/provisioner/lxc-broker.go (right):

https://codereview.appspot.com/10409049/diff/1/worker/provisioner/lxc-broker.go#newcode27
worker/provisioner/lxc-broker.go:27:
To help with understanding what interface lxcBroker implements, and to
cause a compile time error if the contract is not met, the idiom we have
been using for other things is to add a line like this above the struct
definition:

var _ Broker = (*lxcBroker)(nil)

(rinse and repeat for any other implemented interfaces).

https://codereview.appspot.com/10409049/diff/1/worker/provisioner/lxc-broker_test.go
File worker/provisioner/lxc-broker_test.go (right):

https://codereview.appspot.com/10409049/diff/1/worker/provisioner/lxc-broker_test.go#newcode80
worker/provisioner/lxc-broker_test.go:80: c.Assert(lxc.Id(), Equals,
instance.Id("juju-machine-1-lxc-0"))
Can we test more than this? Perhaps ensure the started instance is in
the list of running instances? Anything else we can check? Like
files/dirs created to show the instance is started?

https://codereview.appspot.com/10409049/diff/1/worker/provisioner/lxc-broker_test.go#newcode88
worker/provisioner/lxc-broker_test.go:88: c.Assert(err, IsNil)
There's more we could do here - we haven't verified the stop actually
worked. The method could be empty and just return nil and the test would
still pass. Could we perhaps list the running instances and verify it's
empty? Oh, and invoke stop on just one instance and see that the others
remain.

https://codereview.appspot.com/10409049/

[Tim Penhey (thumper)]

Please take a look.

https://codereview.appspot.com/10409049/diff/1/container/lxc/test.go
File container/lxc/test.go (right):

https://codereview.appspot.com/10409049/diff/1/container/lxc/test.go#newcode4
container/lxc/test.go:4: package lxc
On 2013/06/21 00:19:32, wallyworld wrote:
> This limitation of Go really annoys me.
> Could we add a big file level comment that these exported methods must
only be
> used for tests and not for normal business logic. I know each method
mentions
> the use of tests but I think a global comment would be good too.

Done.

https://codereview.appspot.com/10409049/diff/1/worker/provisioner/lxc-broker.go
File worker/provisioner/lxc-broker.go (right):

https://codereview.appspot.com/10409049/diff/1/worker/provisioner/lxc-broker.go#newcode27
worker/provisioner/lxc-broker.go:27:
On 2013/06/21 00:19:32, wallyworld wrote:
> To help with understanding what interface lxcBroker implements, and to
cause a
> compile time error if the contract is not met, the idiom we have been
using for
> other things is to add a line like this above the struct definition:

> var _ Broker = (*lxcBroker)(nil)

> (rinse and repeat for any other implemented interfaces).

Done.

https://codereview.appspot.com/10409049/diff/1/worker/provisioner/lxc-broker_test.go
File worker/provisioner/lxc-broker_test.go (right):

https://codereview.appspot.com/10409049/diff/1/worker/provisioner/lxc-broker_test.go#newcode80
worker/provisioner/lxc-broker_test.go:80: c.Assert(lxc.Id(), Equals,
instance.Id("juju-machine-1-lxc-0"))
On 2013/06/21 00:19:32, wallyworld wrote:
> Can we test more than this? Perhaps ensure the started instance is in
the list
> of running instances? Anything else we can check? Like files/dirs
created to
> show the instance is started?

Are we getting into duplication here? Perhaps we could check that there
is a container directory with this instance id.

https://codereview.appspot.com/10409049/diff/1/worker/provisioner/lxc-broker_test.go#newcode88
worker/provisioner/lxc-broker_test.go:88: c.Assert(err, IsNil)
On 2013/06/21 00:19:32, wallyworld wrote:
> There's more we could do here - we haven't verified the stop actually
worked.
> The method could be empty and just return nil and the test would still
pass.
> Could we perhaps list the running instances and verify it's empty? Oh,
and
> invoke stop on just one instance and see that the others remain.

Done.

https://codereview.appspot.com/10409049/

[Ian Booth (wallyworld)]

LGTM

https://codereview.appspot.com/10409049/

[William Reade (fwereade)]

LGTM, thanks

https://codereview.appspot.com/10409049/

[William Reade (fwereade)]

review: Approve

=== removed file 'container/lxc/export_test.go'
--- container/lxc/export_test.go	2013-06-24 00:35:22 +0000
+++ container/lxc/export_test.go	1970-01-01 00:00:00 +0000
@@ -1,19 +0,0 @@
-// Copyright 2013 Canonical Ltd.
-// Licensed under the AGPLv3, see LICENCE file for details.
-
-package lxc
-
-func SetContainerDir(dir string) (old string) {
-	old, containerDir = containerDir, dir
-	return
-}
-
-func SetLxcContainerDir(dir string) (old string) {
-	old, lxcContainerDir = lxcContainerDir, dir
-	return
-}
-
-func SetRemovedContainerDir(dir string) (old string) {
-	old, removedContainerDir = removedContainerDir, dir
-	return
-}

=== modified file 'container/lxc/lxc_test.go'
--- container/lxc/lxc_test.go	2013-06-24 00:57:19 +0000
+++ container/lxc/lxc_test.go	2013-06-25 22:06:25 +0000
@@ -13,6 +13,7 @@
 	. "launchpad.net/gocheck"
 	"launchpad.net/goyaml"
 	"launchpad.net/juju-core/container/lxc"
+	"launchpad.net/juju-core/container/lxc/mock"
 	"launchpad.net/juju-core/instance"
 	jujutesting "launchpad.net/juju-core/juju/testing"
 	"launchpad.net/juju-core/state"
@@ -80,7 +81,7 @@
 }
 
 func (s *LxcSuite) TestStartContainer(c *C) {
-	manager := lxc.NewContainerManager(MockFactory(), "")
+	manager := lxc.NewContainerManager(mock.MockFactory(), "")
 	instance := StartContainer(c, manager, "1/lxc/0")
 
 	name := string(instance.Id())
@@ -108,7 +109,7 @@
 }
 
 func (s *LxcSuite) TestStopContainer(c *C) {
-	manager := lxc.NewContainerManager(MockFactory(), "")
+	manager := lxc.NewContainerManager(mock.MockFactory(), "")
 	instance := StartContainer(c, manager, "1/lxc/0")
 
 	err := manager.StopContainer(instance)
@@ -122,7 +123,7 @@
 }
 
 func (s *LxcSuite) TestStopContainerNameClash(c *C) {
-	manager := lxc.NewContainerManager(MockFactory(), "")
+	manager := lxc.NewContainerManager(mock.MockFactory(), "")
 	instance := StartContainer(c, manager, "1/lxc/0")
 
 	name := string(instance.Id())
@@ -140,13 +141,13 @@
 }
 
 func (s *LxcSuite) TestNamedManagerPrefix(c *C) {
-	manager := lxc.NewContainerManager(MockFactory(), "eric")
+	manager := lxc.NewContainerManager(mock.MockFactory(), "eric")
 	instance := StartContainer(c, manager, "1/lxc/0")
 	c.Assert(string(instance.Id()), Equals, "eric-machine-1-lxc-0")
 }
 
 func (s *LxcSuite) TestListContainers(c *C) {
-	factory := MockFactory()
+	factory := mock.MockFactory()
 	foo := lxc.NewContainerManager(factory, "foo")
 	bar := lxc.NewContainerManager(factory, "bar")
 

=== added directory 'container/lxc/mock'
=== renamed file 'container/lxc/mock-lxc_test.go' => 'container/lxc/mock/mock-lxc.go'
--- container/lxc/mock-lxc_test.go	2013-06-19 03:36:33 +0000
+++ container/lxc/mock/mock-lxc.go	2013-06-25 22:06:25 +0000
@@ -1,7 +1,7 @@
 // Copyright 2013 Canonical Ltd.
 // Licensed under the AGPLv3, see LICENCE file for details.
 
-package lxc_test
+package mock
 
 import (
 	"fmt"

=== added file 'container/lxc/test.go'
--- container/lxc/test.go	1970-01-01 00:00:00 +0000
+++ container/lxc/test.go	2013-06-25 22:06:25 +0000
@@ -0,0 +1,29 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+// Functions defined in this file should *ONLY* be used for testing.  These
+// functions are exported for testing purposes only, and shouldn't be called
+// from code that isn't in a test file.
+
+package lxc
+
+// SetContainerDir allows tests in other packages to override the
+// containerDir.
+func SetContainerDir(dir string) (old string) {
+	old, containerDir = containerDir, dir
+	return
+}
+
+// SetLxcContainerDir allows tests in other packages to override the
+// lxcContainerDir.
+func SetLxcContainerDir(dir string) (old string) {
+	old, lxcContainerDir = lxcContainerDir, dir
+	return
+}
+
+// SetRemovedContainerDir allows tests in other packages to override the
+// removedContainerDir.
+func SetRemovedContainerDir(dir string) (old string) {
+	old, removedContainerDir = removedContainerDir, dir
+	return
+}

=== added file 'worker/provisioner/lxc-broker.go'
--- worker/provisioner/lxc-broker.go	1970-01-01 00:00:00 +0000
+++ worker/provisioner/lxc-broker.go	2013-06-25 22:06:25 +0000
@@ -0,0 +1,64 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package provisioner
+
+import (
+	"launchpad.net/golxc"
+	"launchpad.net/juju-core/constraints"
+	"launchpad.net/juju-core/container/lxc"
+	"launchpad.net/juju-core/environs/config"
+	"launchpad.net/juju-core/instance"
+	"launchpad.net/juju-core/state"
+	"launchpad.net/juju-core/state/api"
+	"launchpad.net/loggo"
+)
+
+var lxcLogger = loggo.GetLogger("juju.provisioner.lxc")
+
+var _ Broker = (*lxcBroker)(nil)
+
+func NewLxcBroker(factory golxc.ContainerFactory, config *config.Config, tools *state.Tools) Broker {
+	return &lxcBroker{
+		golxc:   factory,
+		manager: lxc.NewContainerManager(factory, "juju"),
+		config:  config,
+		tools:   tools,
+	}
+}
+
+type lxcBroker struct {
+	golxc   golxc.ContainerFactory
+	manager lxc.ContainerManager
+	config  *config.Config
+	tools   *state.Tools
+}
+
+func (broker *lxcBroker) StartInstance(machineId, machineNonce string, series string, cons constraints.Value, info *state.Info, apiInfo *api.Info) (instance.Instance, *instance.HardwareCharacteristics, error) {
+	lxcLogger.Infof("starting lxc container for machineId: %s", machineId)
+
+	inst, err := broker.manager.StartContainer(machineId, series, machineNonce, broker.tools, broker.config, info, apiInfo)
+	if err != nil {
+		lxcLogger.Errorf("failed to start container: %v", err)
+		return nil, nil, err
+	}
+	return inst, nil, nil
+}
+
+// StopInstances shuts down the given instances.
+func (broker *lxcBroker) StopInstances(instances []instance.Instance) error {
+	// TODO: potentially parallelise.
+	for _, instance := range instances {
+		lxcLogger.Infof("stopping lxc container for instance: %s", instance.Id())
+		if err := broker.manager.StopContainer(instance); err != nil {
+			lxcLogger.Errorf("container did not stop: %v", err)
+			return err
+		}
+	}
+	return nil
+}
+
+// AllInstances only returns running containers.
+func (broker *lxcBroker) AllInstances() (result []instance.Instance, err error) {
+	return broker.manager.ListContainers()
+}

=== added file 'worker/provisioner/lxc-broker_test.go'
--- worker/provisioner/lxc-broker_test.go	1970-01-01 00:00:00 +0000
+++ worker/provisioner/lxc-broker_test.go	2013-06-25 22:06:25 +0000
@@ -0,0 +1,127 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package provisioner_test
+
+import (
+	"path/filepath"
+
+	. "launchpad.net/gocheck"
+	"launchpad.net/golxc"
+	"launchpad.net/juju-core/constraints"
+	"launchpad.net/juju-core/container/lxc"
+	"launchpad.net/juju-core/container/lxc/mock"
+	"launchpad.net/juju-core/instance"
+	jujutesting "launchpad.net/juju-core/juju/testing"
+	"launchpad.net/juju-core/state"
+	"launchpad.net/juju-core/testing"
+	. "launchpad.net/juju-core/testing/checkers"
+	"launchpad.net/juju-core/version"
+	"launchpad.net/juju-core/worker/provisioner"
+)
+
+type lxcBrokerSuite struct {
+	testing.LoggingSuite
+	golxc              golxc.ContainerFactory
+	broker             provisioner.Broker
+	containerDir       string
+	removedDir         string
+	lxcDir             string
+	oldContainerDir    string
+	oldRemovedDir      string
+	oldLxcContainerDir string
+}
+
+var _ = Suite(&lxcBrokerSuite{})
+
+func (s *lxcBrokerSuite) SetUpSuite(c *C) {
+	s.LoggingSuite.SetUpSuite(c)
+}
+
+func (s *lxcBrokerSuite) TearDownSuite(c *C) {
+	s.LoggingSuite.TearDownSuite(c)
+}
+
+func (s *lxcBrokerSuite) SetUpTest(c *C) {
+	s.LoggingSuite.SetUpTest(c)
+	s.containerDir = c.MkDir()
+	s.oldContainerDir = lxc.SetContainerDir(s.containerDir)
+	s.removedDir = c.MkDir()
+	s.oldRemovedDir = lxc.SetRemovedContainerDir(s.removedDir)
+	s.lxcDir = c.MkDir()
+	s.oldLxcContainerDir = lxc.SetLxcContainerDir(s.lxcDir)
+	s.golxc = mock.MockFactory()
+	tools := &state.Tools{
+		Binary: version.MustParseBinary("2.3.4-foo-bar"),
+		URL:    "http://tools.example.com/2.3.4-foo-bar.tgz",
+	}
+	s.broker = provisioner.NewLxcBroker(s.golxc, testing.EnvironConfig(c), tools)
+}
+
+func (s *lxcBrokerSuite) TearDownTest(c *C) {
+	lxc.SetContainerDir(s.oldContainerDir)
+	lxc.SetLxcContainerDir(s.oldLxcContainerDir)
+	lxc.SetRemovedContainerDir(s.oldRemovedDir)
+	s.LoggingSuite.TearDownTest(c)
+}
+
+func (s *lxcBrokerSuite) startInstance(c *C, machineId string) instance.Instance {
+	stateInfo := jujutesting.FakeStateInfo(machineId)
+	apiInfo := jujutesting.FakeAPIInfo(machineId)
+
+	series := "series"
+	nonce := "fake-nonce"
+	cons := constraints.Value{}
+	lxc, _, err := s.broker.StartInstance(machineId, nonce, series, cons, stateInfo, apiInfo)
+	c.Assert(err, IsNil)
+	return lxc
+}
+
+func (s *lxcBrokerSuite) TestStartInstance(c *C) {
+	machineId := "1/lxc/0"
+	lxc := s.startInstance(c, machineId)
+	c.Assert(lxc.Id(), Equals, instance.Id("juju-machine-1-lxc-0"))
+	c.Assert(s.lxcContainerDir(lxc), IsDirectory)
+	s.assertInstances(c, lxc)
+}
+
+func (s *lxcBrokerSuite) TestStopInstance(c *C) {
+	lxc0 := s.startInstance(c, "1/lxc/0")
+	lxc1 := s.startInstance(c, "1/lxc/1")
+	lxc2 := s.startInstance(c, "1/lxc/2")
+
+	err := s.broker.StopInstances([]instance.Instance{lxc0})
+	c.Assert(err, IsNil)
+	s.assertInstances(c, lxc1, lxc2)
+	c.Assert(s.lxcContainerDir(lxc0), DoesNotExist)
+	c.Assert(s.lxcRemovedContainerDir(lxc0), IsDirectory)
+
+	err = s.broker.StopInstances([]instance.Instance{lxc1, lxc2})
+	c.Assert(err, IsNil)
+	s.assertInstances(c)
+}
+
+func (s *lxcBrokerSuite) TestAllInstances(c *C) {
+	lxc0 := s.startInstance(c, "1/lxc/0")
+	lxc1 := s.startInstance(c, "1/lxc/1")
+	s.assertInstances(c, lxc0, lxc1)
+
+	err := s.broker.StopInstances([]instance.Instance{lxc1})
+	c.Assert(err, IsNil)
+	lxc2 := s.startInstance(c, "1/lxc/2")
+	s.assertInstances(c, lxc0, lxc2)
+}
+
+func (s *lxcBrokerSuite) assertInstances(c *C, inst ...instance.Instance) {
+	results, err := s.broker.AllInstances()
+	c.Assert(err, IsNil)
+	testing.MatchInstances(c, results, inst...)
+}
+
+func (s *lxcBrokerSuite) lxcContainerDir(inst instance.Instance) string {
+	return filepath.Join(s.containerDir, string(inst.Id()))
+}
+
+func (s *lxcBrokerSuite) lxcRemovedContainerDir(inst instance.Instance) string {
+	return filepath.Join(s.removedDir, string(inst.Id()))
+}

=== modified file 'worker/provisioner/provisioner.go'
--- worker/provisioner/provisioner.go	2013-06-20 11:03:10 +0000
+++ worker/provisioner/provisioner.go	2013-06-25 22:06:25 +0000
@@ -80,7 +80,7 @@
 	// to the environment config.
 	machinesWatcher := p.st.WatchEnvironMachines()
 	environmentBroker := newEnvironBroker(p.environ)
-	environmentProvisioner := newProvisionerTask(
+	environmentProvisioner := NewProvisionerTask(
 		p.machineId,
 		p.st,
 		machinesWatcher,

=== modified file 'worker/provisioner/provisioner_task.go'
--- worker/provisioner/provisioner_task.go	2013-06-20 04:27:32 +0000
+++ worker/provisioner/provisioner_task.go	2013-06-25 22:06:25 +0000
@@ -34,7 +34,7 @@
 	Machine(id string) (*state.Machine, error)
 }
 
-func newProvisionerTask(
+func NewProvisionerTask(
 	machineId string,
 	machineGetter MachineGetter,
 	watcher Watcher,