juju-core

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

  1. lxc-broker
  2. Merge into trunk
Proposed by Tim Penhey
Status: Merged
Approved by: Tim Penhey
Approved revision: no longer in the source branch.
Merged at revision: 1330
Proposed branch: lp:~thumper/juju-core/lxc-broker
Merge into: lp:~go-bot/juju-core/trunk
Prerequisite: lp:~thumper/juju-core/lxc-container
Diff against target: 353 lines (+229/-27)
8 files modified
container/lxc/export_test.go (+0/-19)
container/lxc/lxc_test.go (+6/-5)
container/lxc/mock/mock-lxc.go (+1/-1)
container/lxc/test.go (+29/-0)
worker/provisioner/lxc-broker.go (+64/-0)
worker/provisioner/lxc-broker_test.go (+127/-0)
worker/provisioner/provisioner.go (+1/-1)
worker/provisioner/provisioner_task.go (+1/-1)
To merge this branch: bzr merge lp:~thumper/juju-core/lxc-broker
Reviewer Review Type Date Requested Status
William Reade (community) Approve
Review via email: mp+170713@code.launchpad.net

Commit message

Implement the lxc broker for the provisioner

Not hooked up yet.

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

Description of the change

Implement the lxc broker for the provisioner

Had to make some test functions and helpers exported.

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

To post a comment you must log in.
Revision history for this message
Tim Penhey (thumper) wrote :

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

Revision history for this message
Ian Booth (wallyworld) wrote :

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/

Revision history for this message
Tim Penhey (thumper) wrote :

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/

Revision history for this message
Ian Booth (wallyworld) wrote :

LGTM

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

Revision history for this message
William Reade (fwereade) wrote :

LGTM, thanks

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

Revision history for this message
William Reade (fwereade) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
1// Copyright 2013 Canonical Ltd.
2// Licensed under the AGPLv3, see LICENCE file for details.
3
4package lxc
5
6func SetContainerDir(dir string) (old string) {
7 old, containerDir = containerDir, dir
8 return
9}
10
11func SetLxcContainerDir(dir string) (old string) {
12 old, lxcContainerDir = lxcContainerDir, dir
13 return
14}
15
16func SetRemovedContainerDir(dir string) (old string) {
17 old, removedContainerDir = removedContainerDir, dir
18 return
19}
200
=== 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 @@
13 . "launchpad.net/gocheck"13 . "launchpad.net/gocheck"
14 "launchpad.net/goyaml"14 "launchpad.net/goyaml"
15 "launchpad.net/juju-core/container/lxc"15 "launchpad.net/juju-core/container/lxc"
16 "launchpad.net/juju-core/container/lxc/mock"
16 "launchpad.net/juju-core/instance"17 "launchpad.net/juju-core/instance"
17 jujutesting "launchpad.net/juju-core/juju/testing"18 jujutesting "launchpad.net/juju-core/juju/testing"
18 "launchpad.net/juju-core/state"19 "launchpad.net/juju-core/state"
@@ -80,7 +81,7 @@
80}81}
8182
82func (s *LxcSuite) TestStartContainer(c *C) {83func (s *LxcSuite) TestStartContainer(c *C) {
83 manager := lxc.NewContainerManager(MockFactory(), "")84 manager := lxc.NewContainerManager(mock.MockFactory(), "")
84 instance := StartContainer(c, manager, "1/lxc/0")85 instance := StartContainer(c, manager, "1/lxc/0")
8586
86 name := string(instance.Id())87 name := string(instance.Id())
@@ -108,7 +109,7 @@
108}109}
109110
110func (s *LxcSuite) TestStopContainer(c *C) {111func (s *LxcSuite) TestStopContainer(c *C) {
111 manager := lxc.NewContainerManager(MockFactory(), "")112 manager := lxc.NewContainerManager(mock.MockFactory(), "")
112 instance := StartContainer(c, manager, "1/lxc/0")113 instance := StartContainer(c, manager, "1/lxc/0")
113114
114 err := manager.StopContainer(instance)115 err := manager.StopContainer(instance)
@@ -122,7 +123,7 @@
122}123}
123124
124func (s *LxcSuite) TestStopContainerNameClash(c *C) {125func (s *LxcSuite) TestStopContainerNameClash(c *C) {
125 manager := lxc.NewContainerManager(MockFactory(), "")126 manager := lxc.NewContainerManager(mock.MockFactory(), "")
126 instance := StartContainer(c, manager, "1/lxc/0")127 instance := StartContainer(c, manager, "1/lxc/0")
127128
128 name := string(instance.Id())129 name := string(instance.Id())
@@ -140,13 +141,13 @@
140}141}
141142
142func (s *LxcSuite) TestNamedManagerPrefix(c *C) {143func (s *LxcSuite) TestNamedManagerPrefix(c *C) {
143 manager := lxc.NewContainerManager(MockFactory(), "eric")144 manager := lxc.NewContainerManager(mock.MockFactory(), "eric")
144 instance := StartContainer(c, manager, "1/lxc/0")145 instance := StartContainer(c, manager, "1/lxc/0")
145 c.Assert(string(instance.Id()), Equals, "eric-machine-1-lxc-0")146 c.Assert(string(instance.Id()), Equals, "eric-machine-1-lxc-0")
146}147}
147148
148func (s *LxcSuite) TestListContainers(c *C) {149func (s *LxcSuite) TestListContainers(c *C) {
149 factory := MockFactory()150 factory := mock.MockFactory()
150 foo := lxc.NewContainerManager(factory, "foo")151 foo := lxc.NewContainerManager(factory, "foo")
151 bar := lxc.NewContainerManager(factory, "bar")152 bar := lxc.NewContainerManager(factory, "bar")
152153
153154
=== 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 @@
1// Copyright 2013 Canonical Ltd.1// Copyright 2013 Canonical Ltd.
2// Licensed under the AGPLv3, see LICENCE file for details.2// Licensed under the AGPLv3, see LICENCE file for details.
33
4package lxc_test4package mock
55
6import (6import (
7 "fmt"7 "fmt"
88
=== 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 @@
1// Copyright 2013 Canonical Ltd.
2// Licensed under the AGPLv3, see LICENCE file for details.
3
4// Functions defined in this file should *ONLY* be used for testing. These
5// functions are exported for testing purposes only, and shouldn't be called
6// from code that isn't in a test file.
7
8package lxc
9
10// SetContainerDir allows tests in other packages to override the
11// containerDir.
12func SetContainerDir(dir string) (old string) {
13 old, containerDir = containerDir, dir
14 return
15}
16
17// SetLxcContainerDir allows tests in other packages to override the
18// lxcContainerDir.
19func SetLxcContainerDir(dir string) (old string) {
20 old, lxcContainerDir = lxcContainerDir, dir
21 return
22}
23
24// SetRemovedContainerDir allows tests in other packages to override the
25// removedContainerDir.
26func SetRemovedContainerDir(dir string) (old string) {
27 old, removedContainerDir = removedContainerDir, dir
28 return
29}
030
=== 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 @@
1// Copyright 2013 Canonical Ltd.
2// Licensed under the AGPLv3, see LICENCE file for details.
3
4package provisioner
5
6import (
7 "launchpad.net/golxc"
8 "launchpad.net/juju-core/constraints"
9 "launchpad.net/juju-core/container/lxc"
10 "launchpad.net/juju-core/environs/config"
11 "launchpad.net/juju-core/instance"
12 "launchpad.net/juju-core/state"
13 "launchpad.net/juju-core/state/api"
14 "launchpad.net/loggo"
15)
16
17var lxcLogger = loggo.GetLogger("juju.provisioner.lxc")
18
19var _ Broker = (*lxcBroker)(nil)
20
21func NewLxcBroker(factory golxc.ContainerFactory, config *config.Config, tools *state.Tools) Broker {
22 return &lxcBroker{
23 golxc: factory,
24 manager: lxc.NewContainerManager(factory, "juju"),
25 config: config,
26 tools: tools,
27 }
28}
29
30type lxcBroker struct {
31 golxc golxc.ContainerFactory
32 manager lxc.ContainerManager
33 config *config.Config
34 tools *state.Tools
35}
36
37func (broker *lxcBroker) StartInstance(machineId, machineNonce string, series string, cons constraints.Value, info *state.Info, apiInfo *api.Info) (instance.Instance, *instance.HardwareCharacteristics, error) {
38 lxcLogger.Infof("starting lxc container for machineId: %s", machineId)
39
40 inst, err := broker.manager.StartContainer(machineId, series, machineNonce, broker.tools, broker.config, info, apiInfo)
41 if err != nil {
42 lxcLogger.Errorf("failed to start container: %v", err)
43 return nil, nil, err
44 }
45 return inst, nil, nil
46}
47
48// StopInstances shuts down the given instances.
49func (broker *lxcBroker) StopInstances(instances []instance.Instance) error {
50 // TODO: potentially parallelise.
51 for _, instance := range instances {
52 lxcLogger.Infof("stopping lxc container for instance: %s", instance.Id())
53 if err := broker.manager.StopContainer(instance); err != nil {
54 lxcLogger.Errorf("container did not stop: %v", err)
55 return err
56 }
57 }
58 return nil
59}
60
61// AllInstances only returns running containers.
62func (broker *lxcBroker) AllInstances() (result []instance.Instance, err error) {
63 return broker.manager.ListContainers()
64}
065
=== 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 @@
1// Copyright 2013 Canonical Ltd.
2// Licensed under the AGPLv3, see LICENCE file for details.
3
4package provisioner_test
5
6import (
7 "path/filepath"
8
9 . "launchpad.net/gocheck"
10 "launchpad.net/golxc"
11 "launchpad.net/juju-core/constraints"
12 "launchpad.net/juju-core/container/lxc"
13 "launchpad.net/juju-core/container/lxc/mock"
14 "launchpad.net/juju-core/instance"
15 jujutesting "launchpad.net/juju-core/juju/testing"
16 "launchpad.net/juju-core/state"
17 "launchpad.net/juju-core/testing"
18 . "launchpad.net/juju-core/testing/checkers"
19 "launchpad.net/juju-core/version"
20 "launchpad.net/juju-core/worker/provisioner"
21)
22
23type lxcBrokerSuite struct {
24 testing.LoggingSuite
25 golxc golxc.ContainerFactory
26 broker provisioner.Broker
27 containerDir string
28 removedDir string
29 lxcDir string
30 oldContainerDir string
31 oldRemovedDir string
32 oldLxcContainerDir string
33}
34
35var _ = Suite(&lxcBrokerSuite{})
36
37func (s *lxcBrokerSuite) SetUpSuite(c *C) {
38 s.LoggingSuite.SetUpSuite(c)
39}
40
41func (s *lxcBrokerSuite) TearDownSuite(c *C) {
42 s.LoggingSuite.TearDownSuite(c)
43}
44
45func (s *lxcBrokerSuite) SetUpTest(c *C) {
46 s.LoggingSuite.SetUpTest(c)
47 s.containerDir = c.MkDir()
48 s.oldContainerDir = lxc.SetContainerDir(s.containerDir)
49 s.removedDir = c.MkDir()
50 s.oldRemovedDir = lxc.SetRemovedContainerDir(s.removedDir)
51 s.lxcDir = c.MkDir()
52 s.oldLxcContainerDir = lxc.SetLxcContainerDir(s.lxcDir)
53 s.golxc = mock.MockFactory()
54 tools := &state.Tools{
55 Binary: version.MustParseBinary("2.3.4-foo-bar"),
56 URL: "http://tools.example.com/2.3.4-foo-bar.tgz",
57 }
58 s.broker = provisioner.NewLxcBroker(s.golxc, testing.EnvironConfig(c), tools)
59}
60
61func (s *lxcBrokerSuite) TearDownTest(c *C) {
62 lxc.SetContainerDir(s.oldContainerDir)
63 lxc.SetLxcContainerDir(s.oldLxcContainerDir)
64 lxc.SetRemovedContainerDir(s.oldRemovedDir)
65 s.LoggingSuite.TearDownTest(c)
66}
67
68func (s *lxcBrokerSuite) startInstance(c *C, machineId string) instance.Instance {
69 stateInfo := jujutesting.FakeStateInfo(machineId)
70 apiInfo := jujutesting.FakeAPIInfo(machineId)
71
72 series := "series"
73 nonce := "fake-nonce"
74 cons := constraints.Value{}
75 lxc, _, err := s.broker.StartInstance(machineId, nonce, series, cons, stateInfo, apiInfo)
76 c.Assert(err, IsNil)
77 return lxc
78}
79
80func (s *lxcBrokerSuite) TestStartInstance(c *C) {
81 machineId := "1/lxc/0"
82 lxc := s.startInstance(c, machineId)
83 c.Assert(lxc.Id(), Equals, instance.Id("juju-machine-1-lxc-0"))
84 c.Assert(s.lxcContainerDir(lxc), IsDirectory)
85 s.assertInstances(c, lxc)
86}
87
88func (s *lxcBrokerSuite) TestStopInstance(c *C) {
89 lxc0 := s.startInstance(c, "1/lxc/0")
90 lxc1 := s.startInstance(c, "1/lxc/1")
91 lxc2 := s.startInstance(c, "1/lxc/2")
92
93 err := s.broker.StopInstances([]instance.Instance{lxc0})
94 c.Assert(err, IsNil)
95 s.assertInstances(c, lxc1, lxc2)
96 c.Assert(s.lxcContainerDir(lxc0), DoesNotExist)
97 c.Assert(s.lxcRemovedContainerDir(lxc0), IsDirectory)
98
99 err = s.broker.StopInstances([]instance.Instance{lxc1, lxc2})
100 c.Assert(err, IsNil)
101 s.assertInstances(c)
102}
103
104func (s *lxcBrokerSuite) TestAllInstances(c *C) {
105 lxc0 := s.startInstance(c, "1/lxc/0")
106 lxc1 := s.startInstance(c, "1/lxc/1")
107 s.assertInstances(c, lxc0, lxc1)
108
109 err := s.broker.StopInstances([]instance.Instance{lxc1})
110 c.Assert(err, IsNil)
111 lxc2 := s.startInstance(c, "1/lxc/2")
112 s.assertInstances(c, lxc0, lxc2)
113}
114
115func (s *lxcBrokerSuite) assertInstances(c *C, inst ...instance.Instance) {
116 results, err := s.broker.AllInstances()
117 c.Assert(err, IsNil)
118 testing.MatchInstances(c, results, inst...)
119}
120
121func (s *lxcBrokerSuite) lxcContainerDir(inst instance.Instance) string {
122 return filepath.Join(s.containerDir, string(inst.Id()))
123}
124
125func (s *lxcBrokerSuite) lxcRemovedContainerDir(inst instance.Instance) string {
126 return filepath.Join(s.removedDir, string(inst.Id()))
127}
0128
=== 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 @@
80 // to the environment config.80 // to the environment config.
81 machinesWatcher := p.st.WatchEnvironMachines()81 machinesWatcher := p.st.WatchEnvironMachines()
82 environmentBroker := newEnvironBroker(p.environ)82 environmentBroker := newEnvironBroker(p.environ)
83 environmentProvisioner := newProvisionerTask(83 environmentProvisioner := NewProvisionerTask(
84 p.machineId,84 p.machineId,
85 p.st,85 p.st,
86 machinesWatcher,86 machinesWatcher,
8787
=== 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 @@
34 Machine(id string) (*state.Machine, error)34 Machine(id string) (*state.Machine, error)
35}35}
3636
37func newProvisionerTask(37func NewProvisionerTask(
38 machineId string,38 machineId string,
39 machineGetter MachineGetter,39 machineGetter MachineGetter,
40 watcher Watcher,40 watcher Watcher,

Subscribers

People subscribed via source and target branches

to status/vote changes: