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

[Tim Penhey]

Implement lxc containers

This branch adds a containers interface, and an lxc
implementation of them. A Container is-a Instance, and
uses the interface embedding to copy across the Instance
methods.

At this stage, the containers do nothing special with
networking, and just use the default set up.

/var/log/juju is mounted from the host to the inside of
the container for ease of access to log files.

Some new checks have been added, along with some testing
functions for files.

https://codereview.appspot.com/10370044/

Implement lxc containers

This branch adds a containers interface, and an lxc
implementation of them. A Container is-a Instance, and
uses the interface embedding to copy across the Instance
methods.

At this stage, the containers do nothing special with
networking, and just use the default set up.

/var/log/juju is mounted from the host to the inside of
the container for ease of access to log files.

Some new checks have been added, along with some testing
functions for files.

https://codereview.appspot.com/10370044/

[Tim Penhey (thumper)]

Reviewers: mp+169980_code.launchpad.net,

Message:
Please take a look.

Description:
Implement lxc containers

This branch adds a containers interface, and an lxc
implementation of them. A Container is-a Instance, and
uses the interface embedding to copy across the Instance
methods.

At this stage, the containers do nothing special with
networking, and just use the default set up.

/var/log/juju is mounted from the host to the inside of
the container for ease of access to log files.

Some new checks have been added, along with some testing
functions for files.

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/10370044/

Affected files:
   A [revision details]
   A container/container.go
   A container/lxc/export_test.go
   A container/lxc/lxc.go
   A container/lxc/lxc_test.go
   A container/lxc/mock-lxc_test.go
   A testing/checkers/bool.go
   A testing/checkers/relop.go
   A testing/file.go

[Ian Booth (wallyworld)]

Looks nice but I have some questions.

https://codereview.appspot.com/10370044/diff/1/container/container.go
File container/container.go (right):

https://codereview.appspot.com/10370044/diff/1/container/container.go#newcode21
container/container.go:21: apiInfo *api.Info) error
It could just be my warped thinking, but I'm conceptually having an
issue with a Create() method being on the Container itself. Shouldn't a
foo factory create a foo? And then once created, the container returned
by the factory is started/stopped etc.

If the above is valid, perhaps a comment explaining the method would
help. All exported methods would ideally be commented, especially ones
like this that represent the API contract.

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

https://codereview.appspot.com/10370044/diff/1/container/lxc/lxc.go#newcode33
container/lxc/lxc.go:33: type ContainerFactory interface {
Exported interfaces/structs should have some comments for doc purposes

https://codereview.appspot.com/10370044/diff/1/container/lxc/lxc.go#newcode39
container/lxc/lxc.go:39: lxc golxc.ContainerFactory
I don't see the need for the lxc attribute here. I think the container
factory can just be embedded like so:

type lxcFactory struct {
     golxc.ContainerFactory
}

Then the code that uses it will be:

factory.New(...)

instead of factory.lxc.New(...)

and the former reads much nicer IMHO. The extra lxc bit just confuses
it.

https://codereview.appspot.com/10370044/diff/1/container/lxc/lxc.go#newcode126
container/lxc/lxc.go:126: // create time seems to work fine.
Is there a bug for this behaviour? If so reference it here. If not, do
we know why it's expected behaviour? Can we put the definitive reason
instead of speculation?

https://codereview.appspot.com/10370044/diff/1/container/lxc/lxc.go#newcode162
container/lxc/lxc.go:162: func (lxc *lxcContainer) InternalLogDir()
string {
Does this need to be exported? I would think not.

https://codereview.appspot.com/10370044/diff/1/container/lxc/lxc.go#newcode174
container/lxc/lxc.go:174: func (lxc *lxcContainer) WriteConfig()
(string, error) {
Does this need to be exported? I would think not.

https://codereview.appspot.com/10370044/diff/1/container/lxc/lxc.go#newcode183
container/lxc/lxc.go:183: func (lxc *lxcContainer) WriteUserData(
Does this need to be exported? I would think not.

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

https://codereview.appspot.com/10370044/diff/1/container/lxc/lxc_test.go#newcode38
container/lxc/lxc_test.go:38: var _ = Suite(&LxcSuite{})
Where are the tests for Start() and Stop()?

https://codereview.appspot.com/10370044/diff/1/container/lxc/lxc_test.go#newcode101
container/lxc/lxc_test.go:101: c.Assert(err, IsNil)
Is there anything more that we can check besides just err is nil? I'd
ideally like to see checks using the params passed to Create. Can we
test that the cloud init stuff/user data is correctly generated etc.
Otherwise I can't see where that stuff is tested.
Typically in this case, the pattern in use it to call the method
assertContainerCreate and put th...

[Frank Mueller (themue)]

Some more comments plus what written in
https://codereview.appspot.com/10361045/ (had that one before).

https://codereview.appspot.com/10370044/diff/1/container/container.go
File container/container.go (right):

https://codereview.appspot.com/10370044/diff/1/container/container.go#newcode21
container/container.go:21: apiInfo *api.Info) error
On 2013/06/18 05:29:55, wallyworld wrote:
> It could just be my warped thinking, but I'm conceptually having an
issue with a
> Create() method being on the Container itself. Shouldn't a foo factory
create a
> foo? And then once created, the container returned by the factory is
> started/stopped etc.

> If the above is valid, perhaps a comment explaining the method would
help. All
> exported methods would ideally be commented, especially ones like this
that
> represent the API contract.

Yes, looks like a problem with the naming. I would like

- ContainerManager as the interface to create or open containers
- lxc.ContainerManager/kvm.ContainerManager implementing that interface
- Container representing container instances to start/stop/destroy them
(again an interface)
- lxc.Container/kvm.Container implementing that interface

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

https://codereview.appspot.com/10370044/diff/1/container/lxc/lxc.go#newcode33
container/lxc/lxc.go:33: type ContainerFactory interface {
On 2013/06/18 05:29:55, wallyworld wrote:
> Exported interfaces/structs should have some comments for doc purposes

+1

https://codereview.appspot.com/10370044/diff/1/testing/checkers/bool.go
File testing/checkers/bool.go (right):

https://codereview.appspot.com/10370044/diff/1/testing/checkers/bool.go#newcode16
testing/checkers/bool.go:16: var IsTrue Checker = &isTrueChecker{
On 2013/06/18 05:29:55, wallyworld wrote:
> I know we could do Not(IsTrue), but how much beer for IsFalse as well?

+1, but as already mentioned, extra CL for testing extensions please.

https://codereview.appspot.com/10370044/

[Tim Penhey (thumper)]

https://codereview.appspot.com/10370044/diff/1/container/container.go
File container/container.go (right):

https://codereview.appspot.com/10370044/diff/1/container/container.go#newcode21
container/container.go:21: apiInfo *api.Info) error
On 2013/06/18 12:47:04, mue wrote:
> On 2013/06/18 05:29:55, wallyworld wrote:
> > It could just be my warped thinking, but I'm conceptually having an
issue with
> a
> > Create() method being on the Container itself. Shouldn't a foo
factory create
> a
> > foo? And then once created, the container returned by the factory is
> > started/stopped etc.
> >
> > If the above is valid, perhaps a comment explaining the method would
help. All
> > exported methods would ideally be commented, especially ones like
this that
> > represent the API contract.

> Yes, looks like a problem with the naming. I would like

> - ContainerManager as the interface to create or open containers
> - lxc.ContainerManager/kvm.ContainerManager implementing that
interface
> - Container representing container instances to start/stop/destroy
them (again
> an interface)
> - lxc.Container/kvm.Container implementing that interface

Actually, after talking with william, I think this entire interface is
going to die :-)

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

https://codereview.appspot.com/10370044/diff/1/container/lxc/lxc.go#newcode33
container/lxc/lxc.go:33: type ContainerFactory interface {
On 2013/06/18 12:47:04, mue wrote:
> On 2013/06/18 05:29:55, wallyworld wrote:
> > Exported interfaces/structs should have some comments for doc
purposes

> +1

Yeah, agreed. I have a feeling that this is going to change somewhat :)

https://codereview.appspot.com/10370044/diff/1/container/lxc/lxc.go#newcode39
container/lxc/lxc.go:39: lxc golxc.ContainerFactory
On 2013/06/18 05:29:55, wallyworld wrote:
> I don't see the need for the lxc attribute here. I think the container
factory
> can just be embedded like so:

> type lxcFactory struct {
> golxc.ContainerFactory
> }

> Then the code that uses it will be:

> factory.New(...)

> instead of factory.lxc.New(...)

> and the former reads much nicer IMHO. The extra lxc bit just confuses
it.

*nod*

https://codereview.appspot.com/10370044/diff/1/container/lxc/lxc.go#newcode126
container/lxc/lxc.go:126: // create time seems to work fine.
On 2013/06/18 05:29:55, wallyworld wrote:
> Is there a bug for this behaviour? If so reference it here. If not, do
we know
> why it's expected behaviour? Can we put the definitive reason instead
of
> speculation?

It is due to the definition of the rootfs, as serge mentioned in an
email. Not quite sure how to fix it, or if it is a real problem. But I
can extend the comment.

https://codereview.appspot.com/10370044/diff/1/container/lxc/lxc.go#newcode162
container/lxc/lxc.go:162: func (lxc *lxcContainer) InternalLogDir()
string {
On 2013/06/18 05:29:55, wallyworld wrote:
> Does this need to be exported? I would think not.

It needs to be exported to test it. And since no-one should really have
access to it, it *shouldn't* matter.

https://codereview.appspot.com/10370044/diff/1/testing/checkers/bool.go
File te...

[Tim Penhey (thumper)]

Please take a look.

https://codereview.appspot.com/10370044/

[Tim Penhey (thumper)]

On 2013/06/19 04:01:42, thumper wrote:
> Please take a look.

Now also contains changes from https://codereview.appspot.com/10395044/
as I couldn't create the dependent branch after it already exists (LP
can...)

https://codereview.appspot.com/10370044/

[Ian Booth (wallyworld)]

In summary, looking pretty good but I have a couple of concerns.

https://codereview.appspot.com/10370044/diff/12001/container/lxc/instance.go
File container/lxc/instance.go (right):

https://codereview.appspot.com/10370044/diff/12001/container/lxc/instance.go#newcode51
container/lxc/instance.go:51: }
trunk now also defines a Metadata() method but this is being removed
asap. But for now, lxcInstance does not implement Instance

https://codereview.appspot.com/10370044/diff/12001/container/lxc/lxc.go
File container/lxc/lxc.go (right):

https://codereview.appspot.com/10370044/diff/12001/container/lxc/lxc.go#newcode56
container/lxc/lxc.go:56: // NewContainerManager returns an manager
object that can start and stop lxc
an manager -> a manager

https://codereview.appspot.com/10370044/diff/12001/container/lxc/lxc.go#newcode130
container/lxc/lxc.go:130: container := manager.New(name)
This confuses me. Can we change this to
container := manager.Get(name)

The above is much more aligned with expected semantics of how New() and
Get() would behave. The reader would see the above and reasonably expect
a new container instance to be created which doesn't make sense.

https://codereview.appspot.com/10370044/diff/12001/container/lxc/lxc.go#newcode234
container/lxc/lxc.go:234: MachineContainerType: "lxc",
can we use the ContainerType const LXC here please

https://codereview.appspot.com/10370044/diff/12001/container/lxc/lxc_test.go
File container/lxc/lxc_test.go (right):

https://codereview.appspot.com/10370044/diff/12001/container/lxc/lxc_test.go#newcode86
container/lxc/lxc_test.go:86: testing.AssertNonEmptyFileExists(c,
filepath.Join(s.containerDir, name, "cloud-init"))
I do think we need to check the contents of the cloud init file like we
do in other cloud init related tests

https://codereview.appspot.com/10370044/

[Tim Penhey (thumper)]

On 2013/06/20 02:01:23, wallyworld wrote:
> In summary, looking pretty good but I have a couple of concerns.

https://codereview.appspot.com/10370044/diff/12001/container/lxc/instance.go
> File container/lxc/instance.go (right):

https://codereview.appspot.com/10370044/diff/12001/container/lxc/instance.go#newcode51
> container/lxc/instance.go:51: }
> trunk now also defines a Metadata() method but this is being removed
asap. But
> for now, lxcInstance does not implement Instance

Why is it being removed? And what is replacing it?

https://codereview.appspot.com/10370044/diff/12001/container/lxc/lxc.go
> File container/lxc/lxc.go (right):

https://codereview.appspot.com/10370044/diff/12001/container/lxc/lxc.go#newcode56
> container/lxc/lxc.go:56: // NewContainerManager returns an manager
object that
> can start and stop lxc
> an manager -> a manager

Fixed.

https://codereview.appspot.com/10370044/diff/12001/container/lxc/lxc.go#newcode130
> container/lxc/lxc.go:130: container := manager.New(name)
> This confuses me. Can we change this to
> container := manager.Get(name)

This is in golxc and out of the scope of this branch. The go idiom is
that New will
return you a well defined object, and that it does. It does not however
actually
create you an lxc container.

https://codereview.appspot.com/10370044/diff/12001/container/lxc/lxc.go#newcode234
> container/lxc/lxc.go:234: MachineContainerType: "lxc",
> can we use the ContainerType const LXC here please

https://codereview.appspot.com/10370044/diff/12001/container/lxc/lxc_test.go
> File container/lxc/lxc_test.go (right):

https://codereview.appspot.com/10370044/diff/12001/container/lxc/lxc_test.go#newcode86
> container/lxc/lxc_test.go:86: testing.AssertNonEmptyFileExists(c,
> filepath.Join(s.containerDir, name, "cloud-init"))
> I do think we need to check the contents of the cloud init file like
we do in
> other cloud init related tests

The contents of cloud-init is covered by the cloud-init tests. I didn't
feel any
need to do additional checks here over and above the actual generation
of the file.

This isn't really a cloud-init test, just a test that a file is written.

https://codereview.appspot.com/10370044/

[Tim Penhey (thumper)]

Please take a look.

https://codereview.appspot.com/10370044/

[Ian Booth (wallyworld)]

On 2013/06/20 22:20:24, thumper wrote:
> On 2013/06/20 02:01:23, wallyworld wrote:
> > In summary, looking pretty good but I have a couple of concerns.
> >
> >
https://codereview.appspot.com/10370044/diff/12001/container/lxc/instance.go
> > File container/lxc/instance.go (right):
> >
> >

https://codereview.appspot.com/10370044/diff/12001/container/lxc/instance.go#newcode51
> > container/lxc/instance.go:51: }
> > trunk now also defines a Metadata() method but this is being removed
asap. But
> > for now, lxcInstance does not implement Instance

> Why is it being removed? And what is replacing it?

William didn't like it because it didn't always provide access to the
metadata. For Instance objects returned from startInstance, the metadata
is available (and this is the case where the metadata is actually
required). However, Instance objects returned from querying the running
instances eg via AllInstances() did not have the metadata available
since an extra RPC call for each distinct instancetype/flavour would be
required. And since the metadata is not needed here, I deferred having
to maike the API calls. But William didn't like the inconsistency. So
what is happening instead is that startInstance returns the Instance and
Metadata as separate result objects.

> >

https://codereview.appspot.com/10370044/diff/12001/container/lxc/lxc.go#newcode130
> > container/lxc/lxc.go:130: container := manager.New(name)
> > This confuses me. Can we change this to
> > container := manager.Get(name)

> This is in golxc and out of the scope of this branch. The go idiom is
that New
> will
> return you a well defined object, and that it does. It does not
however
> actually
> create you an lxc container.

Which is very, very, very unintuitive :-(
Can we at least add a comment so the next person reading the code
understands what is going on.

> >

https://codereview.appspot.com/10370044/diff/12001/container/lxc/lxc.go#newcode234
> > container/lxc/lxc.go:234: MachineContainerType: "lxc",
> > can we use the ContainerType const LXC here please
> >
> >
https://codereview.appspot.com/10370044/diff/12001/container/lxc/lxc_test.go
> > File container/lxc/lxc_test.go (right):
> >
> >

https://codereview.appspot.com/10370044/diff/12001/container/lxc/lxc_test.go#newcode86
> > container/lxc/lxc_test.go:86: testing.AssertNonEmptyFileExists(c,
> > filepath.Join(s.containerDir, name, "cloud-init"))
> > I do think we need to check the contents of the cloud init file like
we do in
> > other cloud init related tests

> The contents of cloud-init is covered by the cloud-init tests. I
didn't feel any
> need to do additional checks here over and above the actual generation
of the
> file.

> This isn't really a cloud-init test, just a test that a file is
written.

Understood. But there's a big assumption that the correct calls to the
cloud init apis have been made. What's to say that an empty or corrupt
file hasn't been written. Can we just add a sanity check that a key line
or two exists in the file contents to ensure the correct parameters have
been passed to cloudinit.MachineConfig, environs.FinishMachineConfig
etc?

https://codereview.appspot.com/10370044/

[Roger Peppe (rogpeppe)]

looks good, with some trivial issues and one significant concern around
the uniqueDirectory code.

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

https://codereview.appspot.com/10370044/diff/1/container/lxc/lxc.go#newcode39
container/lxc/lxc.go:39: lxc golxc.ContainerFactory
On 2013/06/18 23:50:16, thumper wrote:
> On 2013/06/18 05:29:55, wallyworld wrote:
> > I don't see the need for the lxc attribute here. I think the
container factory
> > can just be embedded like so:
> >
> > type lxcFactory struct {
> > golxc.ContainerFactory
> > }
> >
> > Then the code that uses it will be:
> >
> > factory.New(...)
> >
> > instead of factory.lxc.New(...)
> >
> > and the former reads much nicer IMHO. The extra lxc bit just
confuses it.

> *nod*

depends if you actually want to export New as a method on the returned
ContainerFactory. i'd suggest you probably don't, and that using a field
to hide it is actually appropriate, but given that it can only be got to
by dynamic type conversion, it probably doesn't matter *that* much.

https://codereview.appspot.com/10370044/diff/1/container/lxc/lxc.go#newcode73
container/lxc/lxc.go:73: apiInfo *api.Info,
this has enough arguments that i might consider making it take a struct
argument.

https://codereview.appspot.com/10370044/diff/1/container/lxc/lxc.go#newcode149
container/lxc/lxc.go:149: if err := os.Rename(lxc.Directory(),
removedDir); err != nil {
can the Destroy method be called concurrently?

if so, i think you should try the rename each time
you come up with a new name, otherwise someone else
might be racing for the same name.

alternatively just use ioutil.TempDir and move into a known
name inside a directory created with that.

https://codereview.appspot.com/10370044/diff/1/container/lxc/lxc.go#newcode162
container/lxc/lxc.go:162: func (lxc *lxcContainer) InternalLogDir()
string {
On 2013/06/18 23:50:16, thumper wrote:
> On 2013/06/18 05:29:55, wallyworld wrote:
> > Does this need to be exported? I would think not.

> It needs to be exported to test it. And since no-one should really
have access
> to it, it *shouldn't* matter.

you could always define it in export_test.go. i think it's nice
to export only those methods that actually need exporting, otherwise
the reader is left wondering whether there's a particular reason
for exporting them. ditto below, and +1 to wallyworld's queries.

https://codereview.appspot.com/10370044/diff/1/container/lxc/lxc.go#newcode233
container/lxc/lxc.go:233: // Id returns a provider-generated identifier
for the Instance.
If the following methods are exported in order to satisfy an interface,
I'd suggest simply documenting as

// Id implements sompackage.SomeInterface.Id.

(potentially with extra comments that are specific to
this particular implementation). The method documentation
can be left to the interface definition, where it can
be seen with godoc.

If they're only exported for tests, I'd suggest exporting
them only in export_test.go.

https://codereview.appspot.com/10370044/diff/1/container/lxc/lxc.go#newcode280
container/lxc/lxc.go:280: _, err := os.Stat(dir)
i'm concerned that if there's some filesystem
...

[William Reade (fwereade)]

I still think it's a little bit smelly to be mixing Instance methods
into the lxc Container type, but I'm ok to explore this direction and
see how it works out. LGTM with the various other comments addressed,
acknowledged, or explained away, but please check with the other
reviewers before you land it.

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

https://codereview.appspot.com/10370044/diff/1/container/lxc/lxc.go#newcode73
container/lxc/lxc.go:73: apiInfo *api.Info,
On 2013/06/21 07:13:52, rog wrote:
> this has enough arguments that i might consider making it take a
struct
> argument.

I think there's a little bit of settling still to do here. There's
definitely some sort of common params struct that'll be useful when
starting instances of whatever stripe... consider something similar to
MachineConfig, perhaps?

https://codereview.appspot.com/10370044/diff/1/container/lxc/lxc.go#newcode126
container/lxc/lxc.go:126: // create time seems to work fine.
On 2013/06/18 23:50:16, thumper wrote:
> On 2013/06/18 05:29:55, wallyworld wrote:
> > Is there a bug for this behaviour? If so reference it here. If not,
do we know
> > why it's expected behaviour? Can we put the definitive reason
instead of
> > speculation?

> It is due to the definition of the rootfs, as serge mentioned in an
email. Not
> quite sure how to fix it, or if it is a real problem. But I can
extend the
> comment.

+1

https://codereview.appspot.com/10370044/diff/1/container/lxc/lxc.go#newcode149
container/lxc/lxc.go:149: if err := os.Rename(lxc.Directory(),
removedDir); err != nil {
On 2013/06/21 07:13:52, rog wrote:
> can the Destroy method be called concurrently?

> if so, i think you should try the rename each time
> you come up with a new name, otherwise someone else
> might be racing for the same name.

> alternatively just use ioutil.TempDir and move into a known
> name inside a directory created with that.

In theory, I think, it can't be; but, well, probably one day it should
be possible to remove N containers in parallel if they're all unwanted.
Seems smart to consider the concurrent case today.

https://codereview.appspot.com/10370044/diff/21001/container/lxc/export_test.go
File container/lxc/export_test.go (right):

https://codereview.appspot.com/10370044/diff/21001/container/lxc/export_test.go#newcode8
container/lxc/export_test.go:8: containerDir = dir
fwiw, I find `x,y = y, z` quite readable in these situations. YMMV ofc

https://codereview.appspot.com/10370044/diff/21001/container/lxc/instance.go
File container/lxc/instance.go (right):

https://codereview.appspot.com/10370044/diff/21001/container/lxc/instance.go#newcode14
container/lxc/instance.go:14: }
var _ Instance = (*lxcInstance)(nil)?

https://codereview.appspot.com/10370044/diff/21001/container/lxc/instance.go#newcode16
container/lxc/instance.go:16: // Id returns a provider-generated
identifier for the Instance.
On 2013/06/21 07:13:52, rog wrote:
> // Id implements instance.Instance.Id.

> and etc below.

+1

https://codereview.appspot.com/10370044/diff/21001/container/lxc/instance.go#newcode36
container/lxc/instance.go:36: func (lxc *lxcInstance)
OpenP...

[Tim Penhey (thumper)]

Please take a look.

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

https://codereview.appspot.com/10370044/diff/1/container/lxc/lxc.go#newcode73
container/lxc/lxc.go:73: apiInfo *api.Info,
On 2013/06/21 10:01:00, fwereade wrote:
> On 2013/06/21 07:13:52, rog wrote:
> > this has enough arguments that i might consider making it take a
struct
> > argument.

> I think there's a little bit of settling still to do here. There's
definitely
> some sort of common params struct that'll be useful when starting
instances of
> whatever stripe... consider something similar to MachineConfig,
perhaps?

I'd rather wait and let the dust settle first.

https://codereview.appspot.com/10370044/diff/1/container/lxc/lxc.go#newcode149
container/lxc/lxc.go:149: if err := os.Rename(lxc.Directory(),
removedDir); err != nil {
On 2013/06/21 10:01:00, fwereade wrote:
> On 2013/06/21 07:13:52, rog wrote:
> > can the Destroy method be called concurrently?
> >
> > if so, i think you should try the rename each time
> > you come up with a new name, otherwise someone else
> > might be racing for the same name.
> >
> > alternatively just use ioutil.TempDir and move into a known
> > name inside a directory created with that.

> In theory, I think, it can't be; but, well, probably one day it should
be
> possible to remove N containers in parallel if they're all unwanted.
Seems smart
> to consider the concurrent case today.

Even if we were going to remove N containers in parallel, the name is
still unique for any running container. The reason we have the .n
suffix is for when we had container 0, then destroyed it, then created
another then destroyed that one.

https://codereview.appspot.com/10370044/diff/1/container/lxc/lxc.go#newcode162
container/lxc/lxc.go:162: func (lxc *lxcContainer) InternalLogDir()
string {
On 2013/06/21 07:13:52, rog wrote:
> On 2013/06/18 23:50:16, thumper wrote:
> > On 2013/06/18 05:29:55, wallyworld wrote:
> > > Does this need to be exported? I would think not.
> >
> > It needs to be exported to test it. And since no-one should really
have
> access
> > to it, it *shouldn't* matter.

> you could always define it in export_test.go. i think it's nice
> to export only those methods that actually need exporting, otherwise
> the reader is left wondering whether there's a particular reason
> for exporting them. ditto below, and +1 to wallyworld's queries.

understood, and not needed in this case.

https://codereview.appspot.com/10370044/diff/1/container/lxc/lxc.go#newcode233
container/lxc/lxc.go:233: // Id returns a provider-generated identifier
for the Instance.
On 2013/06/21 07:13:52, rog wrote:
> If the following methods are exported in order to satisfy an
interface, I'd
> suggest simply documenting as

> // Id implements sompackage.SomeInterface.Id.

> (potentially with extra comments that are specific to
> this particular implementation). The method documentation
> can be left to the interface definition, where it can
> be seen with godoc.

> If they're only exported for tests, I'd suggest exporting
> them only in export_test.go.

These were moved to a different file in a later change set attache...

[Ian Booth (wallyworld)]

LGTM

https://codereview.appspot.com/10370044/

[Go Bot (go-bot)]

The attempt to merge lp:~thumper/juju-core/lxc-container into lp:juju-core failed. Below is the output from the failed tests.

container/lxc/lxc.go:13:2: import "launchpad.net/golxc": cannot find package

[John A Meinel (jameinel)]

I added 'launchpad.nett/golxc' on the bot and re-approved this.

[Go Bot (go-bot)]

The attempt to merge lp:~thumper/juju-core/lxc-container into lp:juju-core failed. Below is the output from the failed tests.

# launchpad.net/juju-core/container/lxc
container/lxc/instance.go:49: undefined: instance.Metadata

=== added directory 'container'
=== added directory 'container/lxc'
=== added file 'container/lxc/export_test.go'
--- container/lxc/export_test.go	1970-01-01 00:00:00 +0000
+++ container/lxc/export_test.go	2013-06-25 21:33:28 +0000
@@ -0,0 +1,19 @@
+// 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
+}

=== added file 'container/lxc/instance.go'
--- container/lxc/instance.go	1970-01-01 00:00:00 +0000
+++ container/lxc/instance.go	2013-06-25 21:33:28 +0000
@@ -0,0 +1,46 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package lxc
+
+import (
+	"fmt"
+
+	"launchpad.net/juju-core/instance"
+)
+
+type lxcInstance struct {
+	id string
+}
+
+var _ instance.Instance = (*lxcInstance)(nil)
+
+// Id implements instance.Instance.Id.
+func (lxc *lxcInstance) Id() instance.Id {
+	return instance.Id(lxc.id)
+}
+
+// DNSName implements instance.Instance.DNSName.
+func (lxc *lxcInstance) DNSName() (string, error) {
+	return "", instance.ErrNoDNSName
+}
+
+// WaitDNSName implements instance.Instance.WaitDNSName.
+func (lxc *lxcInstance) WaitDNSName() (string, error) {
+	return "", instance.ErrNoDNSName
+}
+
+// OpenPorts implements instance.Instance.OpenPorts.
+func (lxc *lxcInstance) OpenPorts(machineId string, ports []instance.Port) error {
+	return fmt.Errorf("not implemented")
+}
+
+// ClosePorts implements instance.Instance.ClosePorts.
+func (lxc *lxcInstance) ClosePorts(machineId string, ports []instance.Port) error {
+	return fmt.Errorf("not implemented")
+}
+
+// Ports implements instance.Instance.Ports.
+func (lxc *lxcInstance) Ports(machineId string) ([]instance.Port, error) {
+	return nil, fmt.Errorf("not implemented")
+}

=== added file 'container/lxc/lxc.go'
--- container/lxc/lxc.go	1970-01-01 00:00:00 +0000
+++ container/lxc/lxc.go	2013-06-25 21:33:28 +0000
@@ -0,0 +1,283 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package lxc
+
+import (
+	"fmt"
+	"io/ioutil"
+	"os"
+	"path/filepath"
+	"strings"
+
+	"launchpad.net/golxc"
+	"launchpad.net/juju-core/constraints"
+	"launchpad.net/juju-core/environs"
+	"launchpad.net/juju-core/environs/cloudinit"
+	"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 logger = loggo.GetLogger("juju.container.lxc")
+
+var (
+	defaultTemplate     = "ubuntu-cloud"
+	containerDir        = "/var/lib/juju/containers"
+	removedContainerDir = "/var/lib/juju/removed-containers"
+	lxcContainerDir     = "/var/lib/lxc"
+)
+
+// ContainerManager is responsible for starting containers, and stopping and
+// listing containers that it has started.  The name of the manager is used to
+// namespace the lxc containers on the machine.
+type ContainerManager interface {
+	// StartContainer creates and starts a new lxc container for the specified machine.
+	StartContainer(
+		machineId, series, nonce string,
+		tools *state.Tools,
+		environConfig *config.Config,
+		stateInfo *state.Info,
+		apiInfo *api.Info) (instance.Instance, error)
+	// StopContainer stops and destroyes the lxc container identified by Instance.
+	StopContainer(instance.Instance) error
+	// ListContainers return a list of containers that have been started by
+	// this manager.
+	ListContainers() ([]instance.Instance, error)
+}
+
+type containerManager struct {
+	lxcObjectFactory golxc.ContainerFactory
+	name             string
+}
+
+// NewContainerManager returns a manager object that can start and stop lxc
+// containers. The containers that are created are namespaced by the name
+// parameter.
+func NewContainerManager(factory golxc.ContainerFactory, name string) ContainerManager {
+	return &containerManager{factory, name}
+}
+
+func (manager *containerManager) StartContainer(
+	machineId, series, nonce string,
+	tools *state.Tools,
+	environConfig *config.Config,
+	stateInfo *state.Info,
+	apiInfo *api.Info) (instance.Instance, error) {
+
+	name := state.MachineTag(machineId)
+	if manager.name != "" {
+		name = fmt.Sprintf("%s-%s", manager.name, name)
+	}
+	// Note here that the lxcObjectFacotry only returns a valid container
+	// object, and doesn't actually construct the underlying lxc container on
+	// disk.
+	container := manager.lxcObjectFactory.New(name)
+
+	// Create the cloud-init.
+	directory := jujuContainerDirectory(name)
+	logger.Tracef("create directory: %s", directory)
+	if err := os.MkdirAll(directory, 0755); err != nil {
+		logger.Errorf("failed to create container directory: %v", err)
+		return nil, err
+	}
+	logger.Tracef("write cloud-init")
+	userDataFilename, err := writeUserData(directory, machineId, nonce, tools, environConfig, stateInfo, apiInfo)
+	if err != nil {
+		logger.Errorf("failed to write user data: %v", err)
+		return nil, err
+	}
+	logger.Tracef("write the lxc.conf file")
+	configFile, err := writeLxcConfig(directory)
+	if err != nil {
+		logger.Errorf("failed to write config file: %v", err)
+		return nil, err
+	}
+	templateParams := []string{
+		"--debug",                      // Debug errors in the cloud image
+		"--userdata", userDataFilename, // Our groovey cloud-init
+		"--hostid", name, // Use the container name as the hostid
+		"-r", series,
+	}
+	// Create the container.
+	logger.Tracef("create the container")
+	if err := container.Create(configFile, defaultTemplate, templateParams...); err != nil {
+		logger.Errorf("lxc container creation failed: %v", err)
+		return nil, err
+	}
+	// Make sure that the mount dir has been created.
+	logger.Tracef("make the mount dir for the shard logs")
+	if err := os.MkdirAll(internalLogDir(name), 0755); err != nil {
+		logger.Errorf("failed to create internal /var/log/juju mount dir: %v", err)
+		return nil, err
+	}
+	logger.Tracef("lxc container created")
+
+	// Start the lxc container with the appropriate settings for grabbing the
+	// console output and a log file.
+	consoleFile := filepath.Join(directory, "console.log")
+	container.SetLogFile(filepath.Join(directory, "container.log"), golxc.LogDebug)
+	logger.Tracef("start the container")
+	// We explicitly don't pass through the config file to the container.Start
+	// method as we have passed it through at container creation time.  This
+	// is necessary to get the appropriate rootfs reference without explicitly
+	// setting it ourselves.
+	if err = container.Start("", consoleFile); err != nil {
+		logger.Errorf("container failed to start: %v", err)
+		return nil, err
+	}
+	logger.Tracef("container started")
+	return &lxcInstance{name}, nil
+}
+
+func (manager *containerManager) StopContainer(instance instance.Instance) error {
+	name := string(instance.Id())
+	container := manager.lxcObjectFactory.New(name)
+	if err := container.Stop(); err != nil {
+		logger.Errorf("failed to stop lxc container: %v", err)
+		return err
+	}
+	if err := container.Destroy(); err != nil {
+		logger.Errorf("failed to destroy lxc container: %v", err)
+		return err
+	}
+	// Move the directory.
+	logger.Tracef("create old container dir: %s", removedContainerDir)
+	if err := os.MkdirAll(removedContainerDir, 0755); err != nil {
+		logger.Errorf("failed to create removed container directory: %v", err)
+		return err
+	}
+	removedDir, err := uniqueDirectory(removedContainerDir, name)
+	if err != nil {
+		logger.Errorf("was not able to generate a unique directory: %v", err)
+		return err
+	}
+	if err := os.Rename(jujuContainerDirectory(name), removedDir); err != nil {
+		logger.Errorf("failed to rename container directory: %v", err)
+		return err
+	}
+	return nil
+}
+
+func (manager *containerManager) ListContainers() (result []instance.Instance, err error) {
+	containers, err := manager.lxcObjectFactory.List()
+	if err != nil {
+		logger.Errorf("failed getting all instances: %v", err)
+		return
+	}
+	managerPrefix := ""
+	if manager.name != "" {
+		managerPrefix = fmt.Sprintf("%s-", manager.name)
+	}
+
+	for _, container := range containers {
+		// Filter out those not starting with our name.
+		name := container.Name()
+		if !strings.HasPrefix(name, managerPrefix) {
+			continue
+		}
+		if container.IsRunning() {
+			result = append(result, &lxcInstance{name})
+		}
+	}
+	return
+}
+
+func jujuContainerDirectory(containerName string) string {
+	return filepath.Join(containerDir, containerName)
+}
+
+const internalLogDirTemplate = "%s/%s/rootfs/var/log/juju"
+
+func internalLogDir(containerName string) string {
+	return fmt.Sprintf(internalLogDirTemplate, lxcContainerDir, containerName)
+}
+
+const localConfig = `
+lxc.network.type = veth
+lxc.network.link = lxcbr0
+lxc.network.flags = up
+
+lxc.mount.entry=/var/log/juju var/log/juju none defaults,bind 0 0
+`
+
+func writeLxcConfig(directory string) (string, error) {
+	// TODO(thumper): support different network settings.
+	configFilename := filepath.Join(directory, "lxc.conf")
+	if err := ioutil.WriteFile(configFilename, []byte(localConfig), 0644); err != nil {
+		return "", err
+	}
+	return configFilename, nil
+}
+
+func writeUserData(
+	directory, machineId, nonce string,
+	tools *state.Tools,
+	environConfig *config.Config,
+	stateInfo *state.Info,
+	apiInfo *api.Info,
+) (string, error) {
+	userData, err := cloudInitUserData(machineId, nonce, tools, environConfig, stateInfo, apiInfo)
+	if err != nil {
+		logger.Errorf("failed to create user data: %v", err)
+		return "", err
+	}
+	userDataFilename := filepath.Join(directory, "cloud-init")
+	if err := ioutil.WriteFile(userDataFilename, userData, 0644); err != nil {
+		logger.Errorf("failed to write user data: %v", err)
+		return "", err
+	}
+	return userDataFilename, nil
+}
+
+func cloudInitUserData(
+	machineId, nonce string,
+	tools *state.Tools,
+	environConfig *config.Config,
+	stateInfo *state.Info,
+	apiInfo *api.Info,
+) ([]byte, error) {
+	machineConfig := &cloudinit.MachineConfig{
+		MachineId:            machineId,
+		MachineNonce:         nonce,
+		MachineContainerType: state.LXC,
+		StateInfo:            stateInfo,
+		APIInfo:              apiInfo,
+		DataDir:              "/var/lib/juju",
+		Tools:                tools,
+	}
+	if err := environs.FinishMachineConfig(machineConfig, environConfig, constraints.Value{}); err != nil {
+		return nil, err
+	}
+	cloudConfig, err := cloudinit.New(machineConfig)
+	if err != nil {
+		return nil, err
+	}
+	data, err := cloudConfig.Render()
+	if err != nil {
+		return nil, err
+	}
+	return data, nil
+}
+
+// uniqueDirectory returns "path/name" if that directory doesn't exist.  If it
+// does, the method starts appending .1, .2, etc until a unique name is found.
+func uniqueDirectory(path, name string) (string, error) {
+	dir := filepath.Join(path, name)
+	_, err := os.Stat(dir)
+	if os.IsNotExist(err) {
+		return dir, nil
+	}
+	for i := 1; ; i++ {
+		dir := filepath.Join(path, fmt.Sprintf("%s.%d", name, i))
+		_, err := os.Stat(dir)
+		if os.IsNotExist(err) {
+			return dir, nil
+		} else if err != nil {
+			return "", err
+		}
+	}
+	panic("unreachable")
+}

=== added file 'container/lxc/lxc_test.go'
--- container/lxc/lxc_test.go	1970-01-01 00:00:00 +0000
+++ container/lxc/lxc_test.go	2013-06-25 21:33:28 +0000
@@ -0,0 +1,167 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package lxc_test
+
+import (
+	"fmt"
+	"io/ioutil"
+	"os"
+	"path/filepath"
+	stdtesting "testing"
+
+	. "launchpad.net/gocheck"
+	"launchpad.net/goyaml"
+	"launchpad.net/juju-core/container/lxc"
+	"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"
+)
+
+func Test(t *stdtesting.T) {
+	TestingT(t)
+}
+
+type LxcSuite struct {
+	testing.LoggingSuite
+	containerDir       string
+	removedDir         string
+	lxcDir             string
+	oldContainerDir    string
+	oldRemovedDir      string
+	oldLxcContainerDir string
+}
+
+var _ = Suite(&LxcSuite{})
+
+func (s *LxcSuite) SetUpSuite(c *C) {
+	s.LoggingSuite.SetUpSuite(c)
+}
+
+func (s *LxcSuite) TearDownSuite(c *C) {
+	s.LoggingSuite.TearDownSuite(c)
+}
+
+func (s *LxcSuite) 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)
+}
+
+func (s *LxcSuite) TearDownTest(c *C) {
+	lxc.SetContainerDir(s.oldContainerDir)
+	lxc.SetLxcContainerDir(s.oldLxcContainerDir)
+	lxc.SetRemovedContainerDir(s.oldRemovedDir)
+	s.LoggingSuite.TearDownTest(c)
+}
+
+func StartContainer(c *C, manager lxc.ContainerManager, machineId string) instance.Instance {
+	config := testing.EnvironConfig(c)
+	stateInfo := jujutesting.FakeStateInfo(machineId)
+	apiInfo := jujutesting.FakeAPIInfo(machineId)
+
+	series := "series"
+	nonce := "fake-nonce"
+	tools := &state.Tools{
+		Binary: version.MustParseBinary("2.3.4-foo-bar"),
+		URL:    "http://tools.example.com/2.3.4-foo-bar.tgz",
+	}
+
+	inst, err := manager.StartContainer(machineId, series, nonce, tools, config, stateInfo, apiInfo)
+	c.Assert(err, IsNil)
+	return inst
+}
+
+func (s *LxcSuite) TestStartContainer(c *C) {
+	manager := lxc.NewContainerManager(MockFactory(), "")
+	instance := StartContainer(c, manager, "1/lxc/0")
+
+	name := string(instance.Id())
+	// Check our container config files.
+	c.Assert(filepath.Join(s.containerDir, name, "lxc.conf"), IsNonEmptyFile)
+	cloudInitFilename := filepath.Join(s.containerDir, name, "cloud-init")
+	c.Assert(cloudInitFilename, IsNonEmptyFile)
+	data, err := ioutil.ReadFile(cloudInitFilename)
+	c.Assert(err, IsNil)
+	c.Assert(string(data), HasPrefix, "#cloud-config\n")
+
+	x := make(map[interface{}]interface{})
+	err = goyaml.Unmarshal(data, &x)
+	c.Assert(err, IsNil)
+
+	var scripts []string
+	for _, s := range x["runcmd"].([]interface{}) {
+		scripts = append(scripts, s.(string))
+	}
+
+	c.Assert(scripts[len(scripts)-1], Equals, "start jujud-machine-1-lxc-0")
+
+	// Check the mount point has been created inside the container.
+	c.Assert(filepath.Join(s.lxcDir, name, "rootfs/var/log/juju"), IsDirectory)
+}
+
+func (s *LxcSuite) TestStopContainer(c *C) {
+	manager := lxc.NewContainerManager(MockFactory(), "")
+	instance := StartContainer(c, manager, "1/lxc/0")
+
+	err := manager.StopContainer(instance)
+	c.Assert(err, IsNil)
+
+	name := string(instance.Id())
+	// Check that the container dir is no longer in the container dir
+	c.Assert(filepath.Join(s.containerDir, name), DoesNotExist)
+	// but instead, in the removed container dir
+	c.Assert(filepath.Join(s.removedDir, name), IsDirectory)
+}
+
+func (s *LxcSuite) TestStopContainerNameClash(c *C) {
+	manager := lxc.NewContainerManager(MockFactory(), "")
+	instance := StartContainer(c, manager, "1/lxc/0")
+
+	name := string(instance.Id())
+	targetDir := filepath.Join(s.removedDir, name)
+	err := os.MkdirAll(targetDir, 0755)
+	c.Assert(err, IsNil)
+
+	err = manager.StopContainer(instance)
+	c.Assert(err, IsNil)
+
+	// Check that the container dir is no longer in the container dir
+	c.Assert(filepath.Join(s.containerDir, name), DoesNotExist)
+	// but instead, in the removed container dir with a ".1" suffix as there was already a directory there.
+	c.Assert(filepath.Join(s.removedDir, fmt.Sprintf("%s.1", name)), IsDirectory)
+}
+
+func (s *LxcSuite) TestNamedManagerPrefix(c *C) {
+	manager := lxc.NewContainerManager(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()
+	foo := lxc.NewContainerManager(factory, "foo")
+	bar := lxc.NewContainerManager(factory, "bar")
+
+	foo1 := StartContainer(c, foo, "1/lxc/0")
+	foo2 := StartContainer(c, foo, "1/lxc/1")
+	foo3 := StartContainer(c, foo, "1/lxc/2")
+
+	bar1 := StartContainer(c, bar, "1/lxc/0")
+	bar2 := StartContainer(c, bar, "1/lxc/1")
+
+	result, err := foo.ListContainers()
+	c.Assert(err, IsNil)
+	testing.MatchInstances(c, result, foo1, foo2, foo3)
+
+	result, err = bar.ListContainers()
+	c.Assert(err, IsNil)
+	testing.MatchInstances(c, result, bar1, bar2)
+}

=== added file 'container/lxc/mock-lxc_test.go'
--- container/lxc/mock-lxc_test.go	1970-01-01 00:00:00 +0000
+++ container/lxc/mock-lxc_test.go	2013-06-25 21:33:28 +0000
@@ -0,0 +1,147 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package lxc_test
+
+import (
+	"fmt"
+
+	"launchpad.net/golxc"
+)
+
+// This file provides a mock implementation of the golxc interfaces
+// ContainerFactory and Container.
+
+type mockFactory struct {
+	instances map[string]golxc.Container
+}
+
+func MockFactory() golxc.ContainerFactory {
+	return &mockFactory{make(map[string]golxc.Container)}
+}
+
+type mockContainer struct {
+	factory  *mockFactory
+	name     string
+	state    golxc.State
+	logFile  string
+	logLevel golxc.LogLevel
+}
+
+// Name returns the name of the container.
+func (mock *mockContainer) Name() string {
+	return mock.name
+}
+
+// Create creates a new container based on the given template.
+func (mock *mockContainer) Create(configFile, template string, templateArgs ...string) error {
+	mock.state = golxc.StateStopped
+	mock.factory.instances[mock.name] = mock
+	return nil
+}
+
+// Start runs the container as a daemon.
+func (mock *mockContainer) Start(configFile, consoleFile string) error {
+	mock.state = golxc.StateRunning
+	return nil
+}
+
+// Stop terminates the running container.
+func (mock *mockContainer) Stop() error {
+	mock.state = golxc.StateStopped
+	return nil
+}
+
+// Clone creates a copy of the container, giving the copy the specified name.
+func (mock *mockContainer) Clone(name string) (golxc.Container, error) {
+	container := &mockContainer{
+		factory:  mock.factory,
+		name:     name,
+		state:    golxc.StateStopped,
+		logLevel: golxc.LogWarning,
+	}
+	mock.factory.instances[name] = container
+	return container, nil
+}
+
+// Freeze freezes all the container's processes.
+func (mock *mockContainer) Freeze() error {
+	return nil
+}
+
+// Unfreeze thaws all frozen container's processes.
+func (mock *mockContainer) Unfreeze() error {
+	return nil
+}
+
+// Destroy stops and removes the container.
+func (mock *mockContainer) Destroy() error {
+	mock.state = golxc.StateUnknown
+	delete(mock.factory.instances, mock.name)
+	return nil
+}
+
+// Wait waits for one of the specified container states.
+func (mock *mockContainer) Wait(states ...golxc.State) error {
+	return nil
+}
+
+// Info returns the status and the process id of the container.
+func (mock *mockContainer) Info() (golxc.State, int, error) {
+	pid := -1
+	if mock.state == golxc.StateRunning {
+		pid = 42
+	}
+	return mock.state, pid, nil
+}
+
+// IsConstructed checks if the container image exists.
+func (mock *mockContainer) IsConstructed() bool {
+	return mock.state != golxc.StateUnknown
+}
+
+// IsRunning checks if the state of the container is 'RUNNING'.
+func (mock *mockContainer) IsRunning() bool {
+	return mock.state == golxc.StateRunning
+}
+
+// String returns information about the container, like the name, state,
+// and process id.
+func (mock *mockContainer) String() string {
+	_, pid, _ := mock.Info()
+	return fmt.Sprintf("<MockContainer %q, state: %s, pid %d>", mock.name, string(mock.state), pid)
+}
+
+// LogFile returns the current filename used for the LogFile.
+func (mock *mockContainer) LogFile() string {
+	return mock.logFile
+}
+
+// LogLevel returns the current logging level (only used if the
+// LogFile is not "").
+func (mock *mockContainer) LogLevel() golxc.LogLevel {
+	return mock.logLevel
+}
+
+// SetLogFile sets both the LogFile and LogLevel.
+func (mock *mockContainer) SetLogFile(filename string, level golxc.LogLevel) {
+	mock.logFile = filename
+	mock.logLevel = level
+}
+
+func (mock *mockFactory) New(name string) golxc.Container {
+	container := &mockContainer{
+		factory:  mock,
+		name:     name,
+		state:    golxc.StateUnknown,
+		logLevel: golxc.LogWarning,
+	}
+	return container
+}
+
+func (mock *mockFactory) List() (result []golxc.Container, err error) {
+	for _, container := range mock.instances {
+		result = append(result, container)
+	}
+	return
+}

=== modified file 'environs/cloudinit/cloudinit.go'
--- environs/cloudinit/cloudinit.go	2013-06-14 02:32:16 +0000
+++ environs/cloudinit/cloudinit.go	2013-06-25 21:33:28 +0000
@@ -76,7 +76,7 @@
 
 	// MachineContainerType specifies the type of container that the machine
 	// is.  If the machine is not a container, then the type is "".
-	MachineContainerType string
+	MachineContainerType state.ContainerType
 
 	// AuthorizedKeys specifies the keys that are allowed to
 	// connect to the machine (see cloudinit.SSHAddAuthorizedKeys)
@@ -121,7 +121,7 @@
 	c.AddPackage("git")
 	// Perfectly reasonable to install lxc on environment instances and kvm
 	// containers.
-	if cfg.MachineContainerType != "lxc" {
+	if cfg.MachineContainerType != state.LXC {
 		c.AddPackage("lxc")
 	}
 

=== added file 'testing/instance.go'
--- testing/instance.go	1970-01-01 00:00:00 +0000
+++ testing/instance.go	2013-06-25 21:33:28 +0000
@@ -0,0 +1,25 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package testing
+
+import (
+	. "launchpad.net/gocheck"
+	"launchpad.net/juju-core/instance"
+)
+
+// MatchInstances uses DeepEquals to check the instances returned.  The lists
+// are first put into a map, so the ordering of the result and expected values
+// is not tested, and duplicates are ignored.
+func MatchInstances(c *C, result []instance.Instance, expected ...instance.Instance) {
+	resultMap := make(map[instance.Id]instance.Instance)
+	for _, i := range result {
+		resultMap[i.Id()] = i
+	}
+
+	expectedMap := make(map[instance.Id]instance.Instance)
+	for _, i := range expected {
+		expectedMap[i.Id()] = i
+	}
+	c.Assert(resultMap, DeepEquals, expectedMap)
+}