Issue 10370044: Implement lxc containers
DescriptionImplement 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)
Patch Set 1 #
Total comments: 43
Patch Set 2 : Implement lxc containers #
Total comments: 9
Patch Set 3 : Implement lxc containers #
Total comments: 13
Patch Set 4 : Implement lxc containers #
MessagesTotal messages: 14
Please take a look.
Sign in to reply to this message.
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#newc... 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#newc... 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 the checks in place to test the creation. The method doesn't need to be exported hence the lower case method name. https://codereview.appspot.com/10370044/diff/1/container/lxc/mock-lxc_test.go File container/lxc/mock-lxc_test.go (right): https://codereview.appspot.com/10370044/diff/1/container/lxc/mock-lxc_test.go... container/lxc/mock-lxc_test.go:4: package lxc_test Perhaps a few lines of comment stating what this file does. It's clear after reading the code, but it would be nice nit to have to. 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#newco... testing/checkers/bool.go:16: var IsTrue Checker = &isTrueChecker{ I know we could do Not(IsTrue), but how much beer for IsFalse as well? https://codereview.appspot.com/10370044/diff/1/testing/checkers/relop.go File testing/checkers/relop.go (right): https://codereview.appspot.com/10370044/diff/1/testing/checkers/relop.go#newc... testing/checkers/relop.go:51: return false, "obtained value not supported" please include the value and type of the obtained value in the error message, to make figuring out what's wrong easier
Sign in to reply to this message.
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#newco... 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.
Sign in to reply to this message.
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 testing/checkers/bool.go (right): https://codereview.appspot.com/10370044/diff/1/testing/checkers/bool.go#newco... testing/checkers/bool.go:16: var IsTrue Checker = &isTrueChecker{ On 2013/06/18 12:47:04, mue wrote: > 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. I did consider it, but thought that it would be best to add them when they are used.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
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...)
Sign in to reply to this message.
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#... 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#newco... 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#newco... 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#newco... 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#... 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
Sign in to reply to this message.
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#... > 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#newco... > 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#newco... > 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#newco... > 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#... > 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.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
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#... > > 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#newco... > > 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#newco... > > 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#... > > 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?
Sign in to reply to this message.
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 error that this loop will spin unchecked. i think you should change this function to return an error and return an error if you get a non-nil error that doesn't satisfy os.IsNotExist. https://codereview.appspot.com/10370044/diff/1/container/lxc/lxc.go#newcode285 container/lxc/lxc.go:285: panic("never reached.") "unreachable" is idiomatic. 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#newcode4 testing/checkers/bool.go:4: package checkers didn't i review this code in another CL? https://codereview.appspot.com/10370044/diff/1/testing/checkers/bool.go#newco... testing/checkers/bool.go:25: return false, "obtained value not a bool" fmt.Sprintf("obtained %T not bool", params[0]) ? https://codereview.appspot.com/10370044/diff/1/testing/checkers/relop.go File testing/checkers/relop.go (right): https://codereview.appspot.com/10370044/diff/1/testing/checkers/relop.go#newc... testing/checkers/relop.go:19: var GreaterThan Checker = &greaterThanChecker{ // GreaterThan implements a checker that accepts // two arguments, a and b, and checks that a > b. ? https://codereview.appspot.com/10370044/diff/1/testing/checkers/relop.go#newc... testing/checkers/relop.go:51: return false, "obtained value not supported" On 2013/06/18 05:29:55, wallyworld wrote: > please include the value and type of the obtained value in the error message, to > make figuring out what's wrong easier the value is printed anyway, so doesn't need printing here. the type is useful additional info though. 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#... container/lxc/instance.go:16: // Id returns a provider-generated identifier for the Instance. // Id implements instance.Instance.Id. and etc below.
Sign in to reply to this message.
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.... 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#... container/lxc/instance.go:14: } var _ Instance = (*lxcInstance)(nil)? https://codereview.appspot.com/10370044/diff/21001/container/lxc/instance.go#... 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#... container/lxc/instance.go:36: func (lxc *lxcInstance) OpenPorts(machineId string, ports []instance.Port) error { Man, we need to get these ports methods off the instance and onto the environ... another time. https://codereview.appspot.com/10370044/diff/21001/container/lxc/lxc.go File container/lxc/lxc.go (right): https://codereview.appspot.com/10370044/diff/21001/container/lxc/lxc.go#newco... container/lxc/lxc.go:240: if err := environs.FinishMachineConfig(machineConfig, environConfig, constraints.Value{}); err != nil { There's some degree of need for some refactoring here -- I don't think we should need the environ config if we're not running a state server. https://codereview.appspot.com/10370044/diff/21001/container/lxc/lxc_test.go File container/lxc/lxc_test.go (right): https://codereview.appspot.com/10370044/diff/21001/container/lxc/lxc_test.go#... container/lxc/lxc_test.go:87: c.Assert(filepath.Join(s.containerDir, name, "cloud-init"), IsNonEmptyFile) Would be nice to see a bit more testing of the generated cloud-init, perhaps... hard to know how much detail is appropriate, though. Suspect this is more ammunition for extracting some sort of InstanceConfig type..?
Sign in to reply to this message.
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 attached to this review. Comments added. https://codereview.appspot.com/10370044/diff/1/container/lxc/lxc.go#newcode280 container/lxc/lxc.go:280: _, err := os.Stat(dir) On 2013/06/21 07:13:52, rog wrote: > i'm concerned that if there's some filesystem > error that this loop will spin unchecked. > > i think you should change this function to return an error > and return an error if you get a non-nil error that > doesn't satisfy os.IsNotExist. Now does this. https://codereview.appspot.com/10370044/diff/1/container/lxc/lxc.go#newcode285 container/lxc/lxc.go:285: panic("never reached.") On 2013/06/21 07:13:52, rog wrote: > "unreachable" is idiomatic. Done. https://codereview.appspot.com/10370044/diff/1/container/lxc/mock-lxc_test.go File container/lxc/mock-lxc_test.go (right): https://codereview.appspot.com/10370044/diff/1/container/lxc/mock-lxc_test.go... container/lxc/mock-lxc_test.go:4: package lxc_test On 2013/06/18 05:29:55, wallyworld wrote: > Perhaps a few lines of comment stating what this file does. It's clear after > reading the code, but it would be nice nit to have to. Done. 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#newcode4 testing/checkers/bool.go:4: package checkers On 2013/06/21 07:13:52, rog wrote: > didn't i review this code in another CL? Yes, lbox doesn't like me. 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#... container/lxc/instance.go:51: } On 2013/06/20 02:01:23, wallyworld wrote: > trunk now also defines a Metadata() method but this is being removed asap. But > for now, lxcInstance does not implement Instance Done. 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#newco... container/lxc/lxc.go:56: // NewContainerManager returns an manager object that can start and stop lxc On 2013/06/20 02:01:23, wallyworld wrote: > an manager -> a manager Done. https://codereview.appspot.com/10370044/diff/12001/container/lxc/lxc.go#newco... container/lxc/lxc.go:234: MachineContainerType: "lxc", On 2013/06/20 02:01:23, wallyworld wrote: > can we use the ContainerType const LXC here please Done. 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#... container/lxc/lxc_test.go:86: testing.AssertNonEmptyFileExists(c, filepath.Join(s.containerDir, name, "cloud-init")) On 2013/06/20 02:01:23, wallyworld wrote: > I do think we need to check the contents of the cloud init file like we do in > other cloud init related tests Done. 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.... container/lxc/export_test.go:8: containerDir = dir On 2013/06/21 10:01:00, fwereade wrote: > fwiw, I find `x,y = y, z` quite readable in these situations. YMMV ofc Oh... for some reason I didn't think of this. 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#... container/lxc/instance.go:14: } On 2013/06/21 10:01:00, fwereade wrote: > var _ Instance = (*lxcInstance)(nil)? Sure... I had relied on the compiler making sure it matched in the other file, but this adds a nice level of explicitness. https://codereview.appspot.com/10370044/diff/21001/container/lxc/instance.go#... container/lxc/instance.go:16: // Id returns a provider-generated identifier for the Instance. On 2013/06/21 10:01:00, fwereade wrote: > On 2013/06/21 07:13:52, rog wrote: > > // Id implements instance.Instance.Id. > > > > and etc below. > > +1 Done. https://codereview.appspot.com/10370044/diff/21001/container/lxc/instance.go#... container/lxc/instance.go:36: func (lxc *lxcInstance) OpenPorts(machineId string, ports []instance.Port) error { On 2013/06/21 10:01:00, fwereade wrote: > Man, we need to get these ports methods off the instance and onto the environ... > another time. Yes we do. https://codereview.appspot.com/10370044/diff/21001/container/lxc/lxc.go File container/lxc/lxc.go (right): https://codereview.appspot.com/10370044/diff/21001/container/lxc/lxc.go#newco... container/lxc/lxc.go:240: if err := environs.FinishMachineConfig(machineConfig, environConfig, constraints.Value{}); err != nil { On 2013/06/21 10:01:00, fwereade wrote: > There's some degree of need for some refactoring here -- I don't think we should > need the environ config if we're not running a state server. From FinishMachineConfig environs/cloudinit.go:32: // Everything needs the environment's authorized keys. authKeys := cfg.AuthorizedKeys() https://codereview.appspot.com/10370044/diff/21001/container/lxc/lxc_test.go File container/lxc/lxc_test.go (right): https://codereview.appspot.com/10370044/diff/21001/container/lxc/lxc_test.go#... container/lxc/lxc_test.go:87: c.Assert(filepath.Join(s.containerDir, name, "cloud-init"), IsNonEmptyFile) On 2013/06/21 10:01:00, fwereade wrote: > Would be nice to see a bit more testing of the generated cloud-init, perhaps... > hard to know how much detail is appropriate, though. Suspect this is more > ammunition for extracting some sort of InstanceConfig type..? Added a few checks, making sure that the file starts with #cloud-config, and that the last command is to start jujud.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
