Issue 6867073: Implement OpenStack start/stop instance (Closed)
DescriptionImplement OpenStack start/stop instance
This branch provides an initial implementation of the StartInstance() and StopInstances() APIs.
Not everythng is done - for example, the image to use is hard coded instead of being looked up, no tools are set up etc.
More of the juju live tests pass eg TestStartStop.
This work relies on the goose work to implement Duplicate and NotFound errors (cuurently under review).
Some previous temporary code to manually start instances and stand alone tests for AllInstances() and Instances() has been removed.
https://code.launchpad.net/~wallyworld/juju-core/openstack-provider-startstopinstance/+merge/138634
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 19
Patch Set 2 : Implement OpenStack start/stop instance #
Total comments: 18
Patch Set 3 : Implement OpenStack start/stop instance #
Total comments: 45
Patch Set 4 : Implement OpenStack start/stop instance #Patch Set 5 : Implement OpenStack start/stop instance #Patch Set 6 : Implement OpenStack start/stop instance #
MessagesTotal messages: 18
Please take a look.
Sign in to reply to this message.
It feels like the code you wrote is good, but you are removing tests without adding new ones to cover the new functionality. Am I missing something? https://codereview.appspot.com/6867073/diff/1/environs/openstack/live_test.go File environs/openstack/live_test.go (left): https://codereview.appspot.com/6867073/diff/1/environs/openstack/live_test.go... environs/openstack/live_test.go:89: c.Check(err, IsNil) These feels like something we might want to keep. Rather than starting them up in SetUp, we could just update the list as we create items. It isn't required, but it would be nice if TearDown always made sure there wasn't anything left running. https://codereview.appspot.com/6867073/diff/1/environs/openstack/provider.go File environs/openstack/provider.go (left): https://codereview.appspot.com/6867073/diff/1/environs/openstack/provider.go#... environs/openstack/provider.go:184: Docs on StartInstance? https://codereview.appspot.com/6867073/diff/1/environs/openstack/provider.go File environs/openstack/provider.go (right): https://codereview.appspot.com/6867073/diff/1/environs/openstack/provider.go#... environs/openstack/provider.go:207: func (e *environ) userData(scfg *startInstanceParams) (*string, error) { Why *string vs just returning ""? https://codereview.appspot.com/6867073/diff/1/environs/openstack/provider.go#... environs/openstack/provider.go:263: log.Debugf("environs/OpenStack: OpenStack user data: %q", userData) Is this actually helpful? It seems the user data has already been baked, so it might not be human readable. Also, you did seem to create userData the function, but you are commenting it out here. Maybe at least a comment that userData isn't complete yet? https://codereview.appspot.com/6867073/diff/1/environs/openstack/provider.go#... environs/openstack/provider.go:280: ImageId: "0f602ea9-c09e-440c-9e29-cfae5635afa3", Maybe we should have a separate file of in-progress constants. In case we have to upgrade them, etc. https://codereview.appspot.com/6867073/diff/1/environs/openstack/provider.go#... environs/openstack/provider.go:408: }, Do we actually open the mgoPort on all machines? I would have thought that would only be necessary on the state servers. (aka the first bootstrap node). Beyond that, it feels like the security group logic should be outside a specific environment, but maybe this is just a step towards that? https://codereview.appspot.com/6867073/diff/1/environs/openstack/provider.go#... environs/openstack/provider.go:442: var zeroGroup nova.SecurityGroup Is it possible to avoid global state here? Or is 'zero' meaning 'nil'? If it is just nil, why not return a '*nova.SecurityGroup' in the next function, and then return the actual 'nil' pointer. Note that you can compare (x == y) to the nil group, but you can't check for identity. Returning the zeroGroup creates a new struct and fills it with empty content. https://codereview.appspot.com/6867073/diff/1/environs/openstack/provider.go#... environs/openstack/provider.go:452: return zeroGroup, err Put another way, this is equivalent to: return nova.SecurityGroup{} https://codereview.appspot.com/6867073/diff/1/environs/openstack/provider.go#... environs/openstack/provider.go:455: // not support group filtering, so we need to load them all and manually search by name. Would it be nicer to put this into a GetSecurityGroupByName sort of function, and we can hopefully improve it in the future? https://codereview.appspot.com/6867073/diff/1/environs/openstack/provider.go#... environs/openstack/provider.go:493: err := nova.DeleteServer(string(id)) Is there a multi-request Openstack API? (there may not be).
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6867073/diff/1/environs/openstack/live_test.go File environs/openstack/live_test.go (left): https://codereview.appspot.com/6867073/diff/1/environs/openstack/live_test.go... environs/openstack/live_test.go:89: c.Check(err, IsNil) On 2012/12/09 07:28:09, jameinel wrote: > These feels like something we might want to keep. Rather than starting them up > in SetUp, we could just update the list as we create items. > It isn't required, but it would be nice if TearDown always made sure there > wasn't anything left running. The above was used to clean up instances created out of band. None are created this way anymore. The existing ec2 tests delete some storage related artifacts which are specifically created by the live tests, but any test instances are assumed to be managed by the core juju tests themselves from what I can tell. https://codereview.appspot.com/6867073/diff/1/environs/openstack/provider.go File environs/openstack/provider.go (left): https://codereview.appspot.com/6867073/diff/1/environs/openstack/provider.go#... environs/openstack/provider.go:184: On 2012/12/09 07:28:09, jameinel wrote: > Docs on StartInstance? I used the implementation in the existing ec2 provider as the basis for this work. There were no docs in the ec2 implementation either, however the StartInstance method as declared on the Environ interface is documented. https://codereview.appspot.com/6867073/diff/1/environs/openstack/provider.go File environs/openstack/provider.go (right): https://codereview.appspot.com/6867073/diff/1/environs/openstack/provider.go#... environs/openstack/provider.go:207: func (e *environ) userData(scfg *startInstanceParams) (*string, error) { On 2012/12/09 07:28:09, jameinel wrote: > Why *string vs just returning ""? The underlying nova RunServerOpts struct uses a *string for its userData atrribute. As an aside, I really dislike the notion of using a potentially legitimate string value "" to also represent the null value. Same as using an int value of 0 to mean "a value for this int isn't specified". Is the use of "" for nil string a Go idiom? https://codereview.appspot.com/6867073/diff/1/environs/openstack/provider.go#... environs/openstack/provider.go:263: log.Debugf("environs/OpenStack: OpenStack user data: %q", userData) On 2012/12/09 07:28:09, jameinel wrote: > Is this actually helpful? It seems the user data has already been baked, so it > might not be human readable. > > Also, you did seem to create userData the function, but you are commenting it > out here. Maybe at least a comment that userData isn't complete yet? This code, including the logging and commented out stuff, was copied from the ec2 implementation. There is a TODO saying that the userData stuff needs to be implemented. Is this not sufficient? https://codereview.appspot.com/6867073/diff/1/environs/openstack/provider.go#... environs/openstack/provider.go:280: ImageId: "0f602ea9-c09e-440c-9e29-cfae5635afa3", On 2012/12/09 07:28:09, jameinel wrote: > Maybe we should have a separate file of in-progress constants. In case we have > to upgrade them, etc. Sure, sounds like a plan. https://codereview.appspot.com/6867073/diff/1/environs/openstack/provider.go#... environs/openstack/provider.go:408: }, On 2012/12/09 07:28:09, jameinel wrote: > Do we actually open the mgoPort on all machines? I would have thought that would > only be necessary on the state servers. (aka the first bootstrap node). > Beyond that, it feels like the security group logic should be outside a specific > environment, but maybe this is just a step towards that? I copied the ec2 implementation. It would be nice to try and extract out this sort of logic and only retain the vendor specific implementation details in the provider stuff. Not just here but elsewhere also. I think we should look at this now that there's more than just the single ec2 provider to worry about. https://codereview.appspot.com/6867073/diff/1/environs/openstack/provider.go#... environs/openstack/provider.go:442: var zeroGroup nova.SecurityGroup On 2012/12/09 07:28:09, jameinel wrote: > Is it possible to avoid global state here? Or is 'zero' meaning 'nil'? If it is > just nil, why not return a '*nova.SecurityGroup' in the next function, and then > return the actual 'nil' pointer. > Note that you can compare (x == y) to the nil group, but you can't check for > identity. Returning the zeroGroup creates a new struct and fills it with empty > content. Agree with the above. But I just copied the ec2 implementation and wanted to be consistent. https://codereview.appspot.com/6867073/diff/1/environs/openstack/provider.go#... environs/openstack/provider.go:455: // not support group filtering, so we need to load them all and manually search by name. On 2012/12/09 07:28:09, jameinel wrote: > Would it be nicer to put this into a GetSecurityGroupByName sort of function, > and we can hopefully improve it in the future? Sure. I can add it directly to this OpenStack provider but perhaps it's best moved down into goose next time some work is done in this area. https://codereview.appspot.com/6867073/diff/1/environs/openstack/provider.go#... environs/openstack/provider.go:493: err := nova.DeleteServer(string(id)) On 2012/12/09 07:28:09, jameinel wrote: > Is there a multi-request Openstack API? (there may not be). Not that I could see. I did look for one and it was with sadness I had to resort to doing things one server at a time.
Sign in to reply to this message.
Getting very close, thanks. https://codereview.appspot.com/6867073/diff/4001/environs/openstack/provider.go File environs/openstack/provider.go (right): https://codereview.appspot.com/6867073/diff/4001/environs/openstack/provider.... environs/openstack/provider.go:25: const mgoPort = 37017 This constant has been repeated a bunch of times in the codebase. This is not your fault, but it would be awesome if on the day the mgoPort does change, we don't have to chase down the 1/2 dozen places this is hard coded atm. https://codereview.appspot.com/6867073/diff/4001/environs/openstack/provider.... environs/openstack/provider.go:214: ProviderType: "ec2", orly ? https://codereview.appspot.com/6867073/diff/4001/environs/openstack/provider.... environs/openstack/provider.go:236: // TODO: implement tools lookup Please remove the large commented out blocks. https://codereview.appspot.com/6867073/diff/4001/environs/openstack/provider.... environs/openstack/provider.go:263: log.Debugf("environs/OpenStack: OpenStack user data: %q", userData) lowercase openstack, environs/ec2 set the tradition. https://codereview.appspot.com/6867073/diff/4001/environs/openstack/provider.... environs/openstack/provider.go:292: log.Printf("environs/OpenStack: started instance %q", inst.Id()) ditto https://codereview.appspot.com/6867073/diff/4001/environs/openstack/provider.... environs/openstack/provider.go:299: ids[i] = inst.(*instance).Id() What happens if insts contains something other than a *instance ?, maybe var ids []state.InstanceId for _, inst := range insts { i, ok := inst.(*instance).Id() if !ok { return errors.New("Incompatible environs.Instance supplied") } ids = append(ids, i) } return e.terminateInstances(ids) https://codereview.appspot.com/6867073/diff/4001/environs/openstack/provider.... environs/openstack/provider.go:444: func (e *environ) getSecurityGroupByName(name string) (*nova.SecurityGroup, error) { return the value, not the pointer to the SecurityGroup https://codereview.appspot.com/6867073/diff/4001/environs/openstack/provider.... environs/openstack/provider.go:484: return *group, nil then you don't have to deference the pointer you just passed. https://codereview.appspot.com/6867073/diff/4001/environs/openstack/provider.... environs/openstack/provider.go:488: if len(ids) == 0 { you can skip this, range over an empty slice is a no op
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6867073/diff/4001/environs/openstack/provider.go File environs/openstack/provider.go (right): https://codereview.appspot.com/6867073/diff/4001/environs/openstack/provider.... environs/openstack/provider.go:25: const mgoPort = 37017 On 2012/12/10 02:50:37, dfc wrote: > This constant has been repeated a bunch of times in the codebase. This is not > your fault, but it would be awesome if on the day the mgoPort does change, we > don't have to chase down the 1/2 dozen places this is hard coded atm. Agreed. I was looking to progress the OpenStack provider and wanted to bring up these sorts of issues separately the the team meeting to get consensus on how to proceed. https://codereview.appspot.com/6867073/diff/4001/environs/openstack/provider.... environs/openstack/provider.go:214: ProviderType: "ec2", On 2012/12/10 02:50:37, dfc wrote: > orly ? Bah, stupid cut'n'paste. https://codereview.appspot.com/6867073/diff/4001/environs/openstack/provider.... environs/openstack/provider.go:236: // TODO: implement tools lookup On 2012/12/10 02:50:37, dfc wrote: > Please remove the large commented out blocks. Done. https://codereview.appspot.com/6867073/diff/4001/environs/openstack/provider.... environs/openstack/provider.go:263: log.Debugf("environs/OpenStack: OpenStack user data: %q", userData) On 2012/12/10 02:50:37, dfc wrote: > lowercase openstack, environs/ec2 set the tradition. Done. https://codereview.appspot.com/6867073/diff/4001/environs/openstack/provider.... environs/openstack/provider.go:292: log.Printf("environs/OpenStack: started instance %q", inst.Id()) On 2012/12/10 02:50:37, dfc wrote: > ditto Done. https://codereview.appspot.com/6867073/diff/4001/environs/openstack/provider.... environs/openstack/provider.go:299: ids[i] = inst.(*instance).Id() On 2012/12/10 02:50:37, dfc wrote: > What happens if insts contains something other than a *instance ?, maybe > > var ids []state.InstanceId > for _, inst := range insts { > i, ok := inst.(*instance).Id() > if !ok { return errors.New("Incompatible environs.Instance supplied") } > ids = append(ids, i) > } > return e.terminateInstances(ids) Done. https://codereview.appspot.com/6867073/diff/4001/environs/openstack/provider.... environs/openstack/provider.go:444: func (e *environ) getSecurityGroupByName(name string) (*nova.SecurityGroup, error) { On 2012/12/10 02:50:37, dfc wrote: > return the value, not the pointer to the SecurityGroup I thought the Go convention was to typically return pointers to structs? ie pass by value for data structures can be expensive? Also, not returning a pointer means I need to return the zeroGroup thing which seems kludgy when a pointer return value averts the need for such "hacks"? Returning a pointer matches the semantics used by the nova client CreateSecurityGroup() API and makes the code easier to write as a result. It would mess things up changing the getSecurityGroupByName() return type to be different. https://codereview.appspot.com/6867073/diff/4001/environs/openstack/provider.... environs/openstack/provider.go:484: return *group, nil On 2012/12/10 02:50:37, dfc wrote: > then you don't have to deference the pointer you just passed. See above. The dereference is needed anyway because CreateSecurityGroup returns a pointer https://codereview.appspot.com/6867073/diff/4001/environs/openstack/provider.... environs/openstack/provider.go:488: if len(ids) == 0 { On 2012/12/10 02:50:37, dfc wrote: > you can skip this, range over an empty slice is a no op I did it because I didn't want to go the the expense if creating a nova client instance unless needed. nova := e.nova()
Sign in to reply to this message.
LGTM modulo minor comments. https://codereview.appspot.com/6867073/diff/9001/environs/openstack/const.go File environs/openstack/const.go (right): https://codereview.appspot.com/6867073/diff/9001/environs/openstack/const.go#... environs/openstack/const.go:2: nit: i've traditionally called the 'util' or 'kitchen sink' file in the package after the name of the package, ie 'openstack.go', rather than 'const.go'. This is very much personal taste, feel free to disregard. https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.go File environs/openstack/provider.go (right): https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:207: func (e *environ) userData(scfg *startInstanceParams) (*string, error) { a pointer to a string ?, you can just return "", err in erroneous cases. https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:236: // TODO: implement tools lookup nit: TODO(wallyworld) is used on other projects, the godoc tool can pick these up. Putting your name inside the TODO doesn't imply you'll actually do the work, just names you as the point man as you probably know the most about why the TODO wasn't done. https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:352: return "juju-" + e.name why not fmt.Sprintf("juju-%s", e.name) for consistency ?
Sign in to reply to this message.
https://codereview.appspot.com/6867073/diff/9001/environs/openstack/const.go File environs/openstack/const.go (right): https://codereview.appspot.com/6867073/diff/9001/environs/openstack/const.go#... environs/openstack/const.go:2: On 2012/12/12 01:10:21, dfc wrote: > nit: i've traditionally called the 'util' or 'kitchen sink' file in the package > after the name of the package, ie 'openstack.go', rather than 'const.go'. This > is very much personal taste, feel free to disregard. It doesn't bother me either way, so I renamed it to 'openstack.go' https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.go File environs/openstack/provider.go (right): https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:207: func (e *environ) userData(scfg *startInstanceParams) (*string, error) { On 2012/12/12 01:10:21, dfc wrote: > a pointer to a string ?, you can just return "", err in erroneous cases. Ingrained habits / best practice in other languages tell me that conflating "" and nil is evil. I guess if it's the Go way then I can unlearn those things :-) I've changed the type to []byte and will fix the Goose API in a separate branch. https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:236: // TODO: implement tools lookup On 2012/12/12 01:10:21, dfc wrote: > nit: TODO(wallyworld) is used on other projects, the godoc tool can pick these > up. Putting your name inside the TODO doesn't imply you'll actually do the work, > just names you as the point man as you probably know the most about why the TODO > wasn't done. Done. https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:352: return "juju-" + e.name On 2012/12/12 01:10:21, dfc wrote: > why not fmt.Sprintf("juju-%s", e.name) for consistency ? I copied the ec2 code verbatim for this bit :-) Fixed here though.
Sign in to reply to this message.
Looks like a great direction. Mostly minor details, and perhaps a couple of relevant points. https://codereview.appspot.com/6867073/diff/9001/environs/openstack/const.go File environs/openstack/const.go (right): https://codereview.appspot.com/6867073/diff/9001/environs/openstack/const.go#... environs/openstack/const.go:2: On 2012/12/12 02:35:54, wallyworld wrote: > It doesn't bother me either way, so I renamed it to 'openstack.go' I'm not sure I get what that means since it wasn't pushed yet, but it feels like we don't need a file for these two constants. Having them in provider.go (or openstack.go, if you've renamed the existing provider.go) would be fine. https://codereview.appspot.com/6867073/diff/9001/environs/openstack/live_test.go File environs/openstack/live_test.go (left): https://codereview.appspot.com/6867073/diff/9001/environs/openstack/live_test... environs/openstack/live_test.go:71: t.testServers, err = t.createInstances(2) Nice to see this going away. Does that mean the real live tests are actually working now? https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.go File environs/openstack/provider.go (right): https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:9: "launchpad.net/goose/errors" FWIW, slightly atypical to have a package specific for errors. In general they are declared close to the logic that generates them, in the respective package. Not a blocker for me, though. https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:207: func (e *environ) userData(scfg *startInstanceParams) (*string, error) { On 2012/12/12 02:35:54, wallyworld wrote: > Ingrained habits / best practice in other languages tell me that conflating "" > and nil is evil. Just as a convention hint, the pattern here is the same we've discussed before: zero value on errors. The zero value of strings is "". https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:240: //TODO - implement spec lookup Extra space after //, plus same formatting convention as above here and below. https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:242: var userData *string = nil s/*// https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:264: if err == nil { Repeating blindly on errors isn't great. It means that the real error may get obscured by issues coming out of the repetition, that any kind of failure introduces artificial delays, and that the real action coming out of this statement is entirely dependent on factors out of our control. We may as well end up with 20 machines instead of one, for example, just because we failed to parse the response for whatever reason. To avoid those issues, we should be clear about *when* we want to repeat. The ec2 provider, for example, is actually spelled like this: if err == nil || ec2ErrCode(err) != "InvalidGroup.NotFound" { break } This reads as "If there are no errors, or *any error I don't recognize as repeatable*, bail out.". We need something like that here as well. https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:428: func (e *environ) getSecurityGroupByName(name string) (*nova.SecurityGroup, error) { s/getSecurityGroupByName/securityGroup/, please. https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:446: func (e *environ) ensureGroup(name string, rules []nova.RuleInfo) (nova.SecurityGroup, error) { Why this method returns a value while the one above returns a pointer? I'd personally put it as a pointer in both myself, but that's arguably personal taste. That said, returning the same type on both seems adequate, in whatever direction you decide. https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:478: err := nova.DeleteServer(string(id)) Is there a call we could use that terminates more than one instance at a time? Would be beneficial to use it, to avoid a (roundtrip_time * N) cost. Not a blocker for this CL, though.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6867073/diff/9001/environs/openstack/const.go File environs/openstack/const.go (right): https://codereview.appspot.com/6867073/diff/9001/environs/openstack/const.go#... environs/openstack/const.go:2: On 2012/12/12 17:50:19, niemeyer wrote: > On 2012/12/12 02:35:54, wallyworld wrote: > > It doesn't bother me either way, so I renamed it to 'openstack.go' > > I'm not sure I get what that means since it wasn't pushed yet, but it feels like > we don't need a file for these two constants. Having them in provider.go (or > openstack.go, if you've renamed the existing provider.go) would be fine. I had them in provider.go and a reviewer asked for a constants.go file to be introduced. The file was called const.go and then it was suggested I rename to openstack.go. Since it's there now, it would be nice to leave it in place perhaps. https://codereview.appspot.com/6867073/diff/9001/environs/openstack/live_test.go File environs/openstack/live_test.go (left): https://codereview.appspot.com/6867073/diff/9001/environs/openstack/live_test... environs/openstack/live_test.go:71: t.testServers, err = t.createInstances(2) On 2012/12/12 17:50:19, niemeyer wrote: > Nice to see this going away. Does that mean the real live tests are actually > working now? Yes, the above was only temporary till more implementation work was done. Some of the live tests do indeed pass with this branch, specifically the TestStartStop one. Other fail due to missing implementation but as we do more, more and more tests will start to pass. Use -live flag is used to trigger the running of the tests against the live Canonistack instance. https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.go File environs/openstack/provider.go (right): https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:9: "launchpad.net/goose/errors" On 2012/12/12 17:50:19, niemeyer wrote: > FWIW, slightly atypical to have a package specific for errors. In general they > are declared close to the logic that generates them, in the respective package. > Not a blocker for me, though. We are introducing generic error handling logic to deal with nested errors / error causes, hence the package. Also, thing like NotFound or DuplicateValue errors which are generic across a number of different packages go here. https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:240: //TODO - implement spec lookup On 2012/12/12 17:50:19, niemeyer wrote: > Extra space after //, plus same formatting convention as above here and below. Done. https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:240: //TODO - implement spec lookup On 2012/12/12 17:50:19, niemeyer wrote: > Extra space after //, plus same formatting convention as above here and below. Done. https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:242: var userData *string = nil On 2012/12/12 17:50:19, niemeyer wrote: > s/*// I've already reworked this in a later version of this branch. https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:264: if err == nil { On 2012/12/12 17:50:19, niemeyer wrote: > Repeating blindly on errors isn't great. It means that the real error may get > obscured by issues coming out of the repetition, that any kind of failure > introduces artificial delays, and that the real action coming out of this > statement is entirely dependent on factors out of our control. We may as well > end up with 20 machines instead of one, for example, just because we failed to > parse the response for whatever reason. > > To avoid those issues, we should be clear about *when* we want to repeat. The > ec2 provider, for example, is actually spelled like this: > > if err == nil || ec2ErrCode(err) != "InvalidGroup.NotFound" { > break > } > > This reads as "If there are no errors, or *any error I don't recognize as > repeatable*, bail out.". > > We need something like that here as well. Yes, I had meant to come back to this when the Goose error framework stuff had been done and forgot. Thanks for picking it up. Fixed now. I used NotFound as a recognised repeatable error like ec2 does but am not sure what others also should be included, if any. https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:428: func (e *environ) getSecurityGroupByName(name string) (*nova.SecurityGroup, error) { On 2012/12/12 17:50:19, niemeyer wrote: > s/getSecurityGroupByName/securityGroup/, please. I've traditionally used a descriptive method name in the past. Clearly there a Go idiom for method naming I need to read up on, any pointers? I think "securityGroup" doesn't really convey well enough what the method does, so I'd love to understand the rational behind the naming convention and why it's different to what tends to be the norm elsewhere. Actually, if I remove the "ByName" part, what happens if I want to look up a security group filtering on something else besides name? (sadly, OpenStack does not have built in support for filtering security groups). It can't be done using just "securityGroup" as the method name since we can't have 2 methods with the same name. So would "securityGroupByName" be suitable for that reason? https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:446: func (e *environ) ensureGroup(name string, rules []nova.RuleInfo) (nova.SecurityGroup, error) { On 2012/12/12 17:50:19, niemeyer wrote: > Why this method returns a value while the one above returns a pointer? I'd > personally put it as a pointer in both myself, but that's arguably personal > taste. That said, returning the same type on both seems adequate, in whatever > direction you decide. I prefer pointer too (hence the method I wrote uses that). But the ensureSecurityGroup method was copied directly from the ec2 implementation, and a fair bit of other business logic assumes a value. The getSecurityGroupByName method is being moved to Goose itself (the code is only here temporarily) where it will be returning a pointer and so I made it that way here to make the migration easier (it has already been done in a downstream branch). So given the above, it is ok to leave things as is? Since the code that returns a pointer will effectively be in a 3rd party library, and existing business logic in the provider assumes a value? https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:478: err := nova.DeleteServer(string(id)) On 2012/12/12 17:50:19, niemeyer wrote: > Is there a call we could use that terminates more than one instance at a time? > Would be beneficial to use it, to avoid a (roundtrip_time * N) cost. Not a > blocker for this CL, though. You are the 2nd person to ask that :-) I would loved to have used such a call but sadly OpenStack does not support it. The OpenStack API is proving to be quite limited compared with ec2 in places.
Sign in to reply to this message.
https://codereview.appspot.com/6867073/diff/9001/environs/openstack/const.go File environs/openstack/const.go (right): https://codereview.appspot.com/6867073/diff/9001/environs/openstack/const.go#... environs/openstack/const.go:2: On 2012/12/13 03:22:42, wallyworld wrote: > On 2012/12/12 17:50:19, niemeyer wrote: > > On 2012/12/12 02:35:54, wallyworld wrote: > > > It doesn't bother me either way, so I renamed it to 'openstack.go' > > > > I'm not sure I get what that means since it wasn't pushed yet, but it feels > like > > we don't need a file for these two constants. Having them in provider.go (or > > openstack.go, if you've renamed the existing provider.go) would be fine. > > I had them in provider.go and a reviewer asked for a constants.go file to be > introduced. The file was called const.go and then it was suggested I rename to > openstack.go. Since it's there now, it would be nice to leave it in place > perhaps. John's also getting used to the conventions of the project and of Go itself. I apologize for making things go back and forth, but it doesn't make sense to have a file for two constants. Please move them back to provider.go and let's keep that convention strong: unless we have a significant number of constants/variables (see the stock unicode package), they don't belong in their own file, but instead close to where they're used. Thanks, and sorry for the trouble. This shall be an initial phase where we get used to working with each other, and shall smooth out over time. https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.go File environs/openstack/provider.go (right): https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:242: var userData *string = nil On 2012/12/13 03:22:42, wallyworld wrote: > On 2012/12/12 17:50:19, niemeyer wrote: > > s/*// > > I've already reworked this in a later version of this branch. You have to push the later version with "lbox propose", otherwise people can't see the changes nor re-review the proposal to approve it. https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:264: if err == nil { On 2012/12/13 03:22:42, wallyworld wrote: > On 2012/12/12 17:50:19, niemeyer wrote: > > Repeating blindly on errors isn't great. It means that the real error may get > > obscured by issues coming out of the repetition, that any kind of failure > > introduces artificial delays, and that the real action coming out of this > > statement is entirely dependent on factors out of our control. We may as well > > end up with 20 machines instead of one, for example, just because we failed to > > parse the response for whatever reason. > > > > To avoid those issues, we should be clear about *when* we want to repeat. The > > ec2 provider, for example, is actually spelled like this: > > > > if err == nil || ec2ErrCode(err) != "InvalidGroup.NotFound" { > > break > > } > > > > This reads as "If there are no errors, or *any error I don't recognize as > > repeatable*, bail out.". > > > > We need something like that here as well. > > Yes, I had meant to come back to this when the Goose error framework stuff had > been done and forgot. Thanks for picking it up. Fixed now. I used NotFound as a > recognised repeatable error like ec2 does but am not sure what others also > should be included, if any. Just NotFound should be fine for now. We can learn over time. https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:428: func (e *environ) getSecurityGroupByName(name string) (*nova.SecurityGroup, error) { On 2012/12/13 03:22:42, wallyworld wrote: > On 2012/12/12 17:50:19, niemeyer wrote: > > s/getSecurityGroupByName/securityGroup/, please. > > I've traditionally used a descriptive method name in the past. Clearly there a I understand, and that's exactly why I'm picking that up for discussion now that you're joining on board. We need to develop a shared idea of what conventions the project takes. > "securityGroup" doesn't really convey well enough what the method does It actually does well enough. This is a method name, not its documentation. If we had 10 other methods that retrieved groups in different ways, maybe that would be necessary, but we don't. It's prototype is clear.. the method name is clear, the result is clear also. As a proof, here is the only use of this method in this file: group, err = e.securityGroup(name) I don't personally need anything else to understand what's going on, and appreciate having to read less. Regarding global conventions, indeed getting rid of "get" is a convention you'll find often in Go. See the API of the state package as well.. it'll give some hints on common conventions for getters and setters that we're using. https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:446: func (e *environ) ensureGroup(name string, rules []nova.RuleInfo) (nova.SecurityGroup, error) { On 2012/12/13 03:22:42, wallyworld wrote: > On 2012/12/12 17:50:19, niemeyer wrote: > > Why this method returns a value while the one above returns a pointer? I'd > > personally put it as a pointer in both myself, but that's arguably personal > > taste. That said, returning the same type on both seems adequate, in whatever > > direction you decide. > > I prefer pointer too (hence the method I wrote uses that). But the > ensureSecurityGroup method was copied directly from the ec2 implementation, and > a fair bit of other business logic assumes a value. All I'm asking for is consistency in this case. Probably because a fair bit of the business logic uses them as values as you point out, the ec2 package returns values for good reason, and it does so consistently. https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:478: err := nova.DeleteServer(string(id)) On 2012/12/13 03:22:42, wallyworld wrote: > On 2012/12/12 17:50:19, niemeyer wrote: > > Is there a call we could use that terminates more than one instance at a time? > > > Would be beneficial to use it, to avoid a (roundtrip_time * N) cost. Not a > > blocker for this CL, though. > > You are the 2nd person to ask that :-) > I would loved to have used such a call but sadly OpenStack does not support it. > The OpenStack API is proving to be quite limited compared with ec2 in places. Aw.. sucks. Well, we can implement/recommend implementing it once we have 100 thousand users claiming for it. :-)
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.go File environs/openstack/provider.go (right): https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:242: var userData *string = nil On 2012/12/13 13:21:59, niemeyer wrote: > On 2012/12/13 03:22:42, wallyworld wrote: > > On 2012/12/12 17:50:19, niemeyer wrote: > > > s/*// > > > > I've already reworked this in a later version of this branch. > > You have to push the later version with "lbox propose", otherwise people can't > see the changes nor re-review the proposal to approve it. Sorry. I pushed to lp. Still getting used to using 2 systems. Habits are hard to break :-) https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:264: if err == nil { On 2012/12/13 13:21:59, niemeyer wrote: > On 2012/12/13 03:22:42, wallyworld wrote: > > On 2012/12/12 17:50:19, niemeyer wrote: > > > Repeating blindly on errors isn't great. It means that the real error may > get > > > obscured by issues coming out of the repetition, that any kind of failure > > > introduces artificial delays, and that the real action coming out of this > > > statement is entirely dependent on factors out of our control. We may as > well > > > end up with 20 machines instead of one, for example, just because we failed > to > > > parse the response for whatever reason. > > > > > > To avoid those issues, we should be clear about *when* we want to repeat. > The > > > ec2 provider, for example, is actually spelled like this: > > > > > > if err == nil || ec2ErrCode(err) != "InvalidGroup.NotFound" > { > > > break > > > } > > > > > > This reads as "If there are no errors, or *any error I don't recognize as > > > repeatable*, bail out.". > > > > > > We need something like that here as well. > > > > Yes, I had meant to come back to this when the Goose error framework stuff had > > been done and forgot. Thanks for picking it up. Fixed now. I used NotFound as > a > > recognised repeatable error like ec2 does but am not sure what others also > > should be included, if any. > > Just NotFound should be fine for now. We can learn over time. Done. https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:428: func (e *environ) getSecurityGroupByName(name string) (*nova.SecurityGroup, error) { On 2012/12/13 13:21:59, niemeyer wrote: > On 2012/12/13 03:22:42, wallyworld wrote: > > On 2012/12/12 17:50:19, niemeyer wrote: > > > s/getSecurityGroupByName/securityGroup/, please. > > > > I've traditionally used a descriptive method name in the past. Clearly there a > > I understand, and that's exactly why I'm picking that up for discussion now that > you're joining on board. We need to develop a shared idea of what conventions > the project takes. > > > "securityGroup" doesn't really convey well enough what the method does > > It actually does well enough. This is a method name, not its documentation. If > we had 10 other methods that retrieved groups in different ways, maybe that > would be necessary, but we don't. It's prototype is clear.. the method name is > clear, the result is clear also. > > As a proof, here is the only use of this method in this file: > > group, err = e.securityGroup(name) > > I don't personally need anything else to understand what's going on, and > appreciate having to read less. > > Regarding global conventions, indeed getting rid of "get" is a convention you'll > find often in Go. See the API of the state package as well.. it'll give some > hints on common conventions for getters and setters that we're using. > This method is now in Goose (and removed in a downstream juju-core branch). We discussed the naming convention on the Blue standup and disagree that the ByName bit should be removed, since adding a search by id for example requires the method names to be disambiguated. Perhaps we can discuss further at the next team meeting. https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:446: func (e *environ) ensureGroup(name string, rules []nova.RuleInfo) (nova.SecurityGroup, error) { On 2012/12/13 13:21:59, niemeyer wrote: > On 2012/12/13 03:22:42, wallyworld wrote: > > On 2012/12/12 17:50:19, niemeyer wrote: > > > Why this method returns a value while the one above returns a pointer? I'd > > > personally put it as a pointer in both myself, but that's arguably personal > > > taste. That said, returning the same type on both seems adequate, in > whatever > > > direction you decide. > > > > I prefer pointer too (hence the method I wrote uses that). But the > > ensureSecurityGroup method was copied directly from the ec2 implementation, > and > > a fair bit of other business logic assumes a value. > > All I'm asking for is consistency in this case. Probably because a fair bit of > the business logic uses them as values as you point out, the ec2 package returns > values for good reason, and it does so consistently. Since the called method has been moved to Goose, and using a pointer is consistent with the rest of the Goose stuff, and Goose is effectively a separate 3rd party library, I'd prefer to keep it as a pointer so that everything works the same in the downstream branch when the Goose API call is used in place of the (removed) local method.
Sign in to reply to this message.
https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.go File environs/openstack/provider.go (right): https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:428: func (e *environ) getSecurityGroupByName(name string) (*nova.SecurityGroup, error) { On 2012/12/13 23:52:56, wallyworld wrote: > On 2012/12/13 13:21:59, niemeyer wrote: > > On 2012/12/13 03:22:42, wallyworld wrote: > > > On 2012/12/12 17:50:19, niemeyer wrote: > > > > s/getSecurityGroupByName/securityGroup/, please. > > > > > > I've traditionally used a descriptive method name in the past. Clearly there > a > > > > I understand, and that's exactly why I'm picking that up for discussion now > that > > you're joining on board. We need to develop a shared idea of what conventions > > the project takes. > > > > > "securityGroup" doesn't really convey well enough what the method does > > > > It actually does well enough. This is a method name, not its documentation. If > > we had 10 other methods that retrieved groups in different ways, maybe that > > would be necessary, but we don't. It's prototype is clear.. the method name is > > clear, the result is clear also. > > > > As a proof, here is the only use of this method in this file: > > > > group, err = e.securityGroup(name) > > > > I don't personally need anything else to understand what's going on, and > > appreciate having to read less. > > > > Regarding global conventions, indeed getting rid of "get" is a convention > you'll > > find often in Go. See the API of the state package as well.. it'll give some > > hints on common conventions for getters and setters that we're using. > > > > This method is now in Goose (and removed in a downstream juju-core branch). We > discussed the naming convention on the Blue standup and disagree that the ByName > bit should be removed, since adding a search by id for example requires the > method names to be disambiguated. Perhaps we can discuss further at the next > team meeting. Hmm.. you could potentially want in the future a method that only returns groups that have computers in them, so perhaps the current method should be called getSecurityGroupWithOrWithoutComputersByName. Then, what about ensureGroup? It feels very vague.. it should be something like getSecurityGroupByNameAndRulesAndCreateIfMissing, of course. You see the point? This is/was a private method, used once, with a very clear signature. Method names should be clear and unambiguous, but there's no need to move the documentation and the parameter names for methods into their name, unless you have a public API that must take relevant variation on the parameters and results (see the regexp package for the counterexample). https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:446: func (e *environ) ensureGroup(name string, rules []nova.RuleInfo) (nova.SecurityGroup, error) { On 2012/12/13 23:52:56, wallyworld wrote: > On 2012/12/13 13:21:59, niemeyer wrote: > > On 2012/12/13 03:22:42, wallyworld wrote: > > > On 2012/12/12 17:50:19, niemeyer wrote: > > > > Why this method returns a value while the one above returns a pointer? I'd > > > > personally put it as a pointer in both myself, but that's arguably > personal > > > > taste. That said, returning the same type on both seems adequate, in > > whatever > > > > direction you decide. > > > > > > I prefer pointer too (hence the method I wrote uses that). But the > > > ensureSecurityGroup method was copied directly from the ec2 implementation, > > and > > > a fair bit of other business logic assumes a value. > > > > All I'm asking for is consistency in this case. Probably because a fair bit of > > the business logic uses them as values as you point out, the ec2 package > returns > > values for good reason, and it does so consistently. > > Since the called method has been moved to Goose, and using a pointer is > consistent with the rest of the Goose stuff, and Goose is effectively a separate > 3rd party library, I'd prefer to keep it as a pointer so that everything works > the same in the downstream branch when the Goose API call is used in place of > the (removed) local method. Sure. Where's that logic? The latest CL still has the method here.
Sign in to reply to this message.
https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.go File environs/openstack/provider.go (right): https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:428: func (e *environ) getSecurityGroupByName(name string) (*nova.SecurityGroup, error) { On 2012/12/14 12:19:40, niemeyer wrote: > On 2012/12/13 23:52:56, wallyworld wrote: > > On 2012/12/13 13:21:59, niemeyer wrote: > > > On 2012/12/13 03:22:42, wallyworld wrote: > > > > On 2012/12/12 17:50:19, niemeyer wrote: > > > > > s/getSecurityGroupByName/securityGroup/, please. > > > > > > > > I've traditionally used a descriptive method name in the past. Clearly > there > > a > > > > > > I understand, and that's exactly why I'm picking that up for discussion now > > that > > > you're joining on board. We need to develop a shared idea of what > conventions > > > the project takes. > > > > > > > "securityGroup" doesn't really convey well enough what the method does > > > > > > It actually does well enough. This is a method name, not its documentation. > If > > > we had 10 other methods that retrieved groups in different ways, maybe that > > > would be necessary, but we don't. It's prototype is clear.. the method name > is > > > clear, the result is clear also. > > > > > > As a proof, here is the only use of this method in this file: > > > > > > group, err = e.securityGroup(name) > > > > > > I don't personally need anything else to understand what's going on, and > > > appreciate having to read less. > > > > > > Regarding global conventions, indeed getting rid of "get" is a convention > > you'll > > > find often in Go. See the API of the state package as well.. it'll give some > > > hints on common conventions for getters and setters that we're using. > > > > > > > This method is now in Goose (and removed in a downstream juju-core branch). We > > discussed the naming convention on the Blue standup and disagree that the > ByName > > bit should be removed, since adding a search by id for example requires the > > method names to be disambiguated. Perhaps we can discuss further at the next > > team meeting. > > Hmm.. you could potentially want in the future a method that only returns groups > that have computers in them, so perhaps the current method should be called > getSecurityGroupWithOrWithoutComputersByName. Then, what about ensureGroup? It > feels very vague.. it should be something like > getSecurityGroupByNameAndRulesAndCreateIfMissing, of course. > I think you're just being silly now. The naming convention that's been adopted is very common in other environments and works very well in every other situation I've come across it. This discussion has now just become a bike shedding exercise and I'd really like to get back to looking at the bigger picture and get this branch landed instead of quibbling over a method name. Especially since the code in question is deleted from juju-core in a downstream branch. https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:446: func (e *environ) ensureGroup(name string, rules []nova.RuleInfo) (nova.SecurityGroup, error) { On 2012/12/14 12:19:40, niemeyer wrote: > On 2012/12/13 23:52:56, wallyworld wrote: > > On 2012/12/13 13:21:59, niemeyer wrote: > > > On 2012/12/13 03:22:42, wallyworld wrote: > > > > On 2012/12/12 17:50:19, niemeyer wrote: > > > > > Why this method returns a value while the one above returns a pointer? > I'd > > > > > personally put it as a pointer in both myself, but that's arguably > > personal > > > > > taste. That said, returning the same type on both seems adequate, in > > > whatever > > > > > direction you decide. > > > > > > > > I prefer pointer too (hence the method I wrote uses that). But the > > > > ensureSecurityGroup method was copied directly from the ec2 > implementation, > > > and > > > > a fair bit of other business logic assumes a value. > > > > > > All I'm asking for is consistency in this case. Probably because a fair bit > of > > > the business logic uses them as values as you point out, the ec2 package > > returns > > > values for good reason, and it does so consistently. > > > > Since the called method has been moved to Goose, and using a pointer is > > consistent with the rest of the Goose stuff, and Goose is effectively a > separate > > 3rd party library, I'd prefer to keep it as a pointer so that everything works > > the same in the downstream branch when the Goose API call is used in place of > > the (removed) local method. > > Sure. Where's that logic? The latest CL still has the method here. The Goose branch is https://codereview.appspot.com/6920045 The juju-core branch is https://codereview.appspot.com/6929055
Sign in to reply to this message.
https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.go File environs/openstack/provider.go (right): https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:428: func (e *environ) getSecurityGroupByName(name string) (*nova.SecurityGroup, error) { On 2012/12/14 13:08:14, wallyworld wrote: > I think you're just being silly now. The naming convention I'm taking the time to explain to you the implicit guidelines that exist in the project, so that you can be productive. If you're unable to talk about that without going down to personal attacks, it'll be difficult to collaborate appropriately. You're free to do whatever you want in your other environments, though. https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:446: func (e *environ) ensureGroup(name string, rules []nova.RuleInfo) (nova.SecurityGroup, error) { On 2012/12/14 13:08:14, wallyworld wrote: > > Sure. Where's that logic? The latest CL still has the method here. > > The Goose branch is https://codereview.appspot.com/6920045 > The juju-core branch is https://codereview.appspot.com/6929055 The CL that is being approved is this one. The return types are inconsistent and they must be fixed if you want this in, or the method must be removed as suggested.
Sign in to reply to this message.
https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.go File environs/openstack/provider.go (right): https://codereview.appspot.com/6867073/diff/9001/environs/openstack/provider.... environs/openstack/provider.go:428: func (e *environ) getSecurityGroupByName(name string) (*nova.SecurityGroup, error) { The discussion as I remember it is that roughly: 1) Name does not appear to actually be the primary key for how security groups are addressed. It might be the primary user visible one, but the PK appears to be ID. 2) Thus the 'securityGroup(ID)' seems like if we are going to have a method without a qualifier, then it should use the ID for that method. 3) It is a little assymetric to have "securityGroup(ID)" and "securityGroupByName(name)". vs "securityGroupById(ID)". 4) If this were python code, we would have probably done: "getSecurityGroup(name=None,id=None)". I'm not positive on that, as it is a case where you only want to specify one of them. Which is clearer in the Go case, because you'd always have to pass 'nil' explicitly for the other one. 5) The other option could be: securityGroup(nameOrId) but I really don't prefer APIs that try to guess what you are asking of it. securityGroup(searchKey, keyType) for: securityGroup(name, "name") Might be a way, but it starts to get messy if you ever want something that uses a different type (eg by int). 6) "securityGroupByName()" seems to be a decent compromise at the end of the discussion.
Sign in to reply to this message.
On 2012/12/16 08:20:25, jameinel wrote: > 6) "securityGroupByName()" seems to be a decent compromise at the end of the > discussion. I'm obviously not going to block that CL based on such a private method name detail. All I've been saying is that securityGroup(name) is a perfectly fine method name, and we have tons of similar cases. If you want to keep ByName in that case, that's certainly fine. The lack of consistency on return ought to be fixed, though. Even because it's extremely trivial.
Sign in to reply to this message.
This LGTM, although I would like to stress my support for (thoughtful and judicious) future factoring-out of the bits that really are common with providers in general (I'm not 100% sure the ec2 similarity justifies that work quite yet: I suspect we'll do it a lot better if we wait until have two very dissimilar providers to compare... not that that doesn't itself have risks).
Sign in to reply to this message.
*** Submitted: Implement OpenStack start/stop instance This branch provides an initial implementation of the StartInstance() and StopInstances() APIs. Not everythng is done - for example, the image to use is hard coded instead of being looked up, no tools are set up etc. More of the juju live tests pass eg TestStartStop. This work relies on the goose work to implement Duplicate and NotFound errors (cuurently under review). Some previous temporary code to manually start instances and stand alone tests for AllInstances() and Instances() has been removed. R=jameinel, dfc, niemeyer, fwereade CC= https://codereview.appspot.com/6867073
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
