Implement the lxc broker for the provisioner

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)

[thumper, 2013-06-20 22:33:02]

Please take a look.

[wallyworld, 2013-06-21 00:19:32]

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....
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_...
File worker/provisioner/lxc-broker_test.go (right):

https://codereview.appspot.com/10409049/diff/1/worker/provisioner/lxc-broker_...
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_...
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.

[thumper, 2013-06-24 02:13:41]

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....
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_...
File worker/provisioner/lxc-broker_test.go (right):

https://codereview.appspot.com/10409049/diff/1/worker/provisioner/lxc-broker_...
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_...
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.

[wallyworld, 2013-06-24 02:18:33]

LGTM

[fwereade, 2013-06-26 09:04:08]

LGTM, thanks