|
|
|
Add support for Availability Zones
- Added ListAvailabilityZones to Nova client
- Added AvailabilityZone field to RunServerOpts
- Added AvailabilityZone field to ServerDetail
- Updated nova test-service to support all of the above
Availability zones are an OpenStack extension,
so I've made it so that ListAvailabilityZones will
ignore any 404 to the os-availability-zone URL.
https://code.launchpad.net/~axwalk/goose/nova-availability-zones/+merge/222275
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 6
Patch Set 2 : Add support for Availability Zones #
Total comments: 5
Patch Set 3 : Add support for Availability Zones #
Total messages: 8
|
axw
Please take a look.
|
11 years, 7 months ago (2014年06月06日 04:14:27 UTC) #1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Please take a look.
https://codereview.appspot.com/103900045/diff/1/nova/local_test.go File nova/local_test.go (right): https://codereview.appspot.com/103900045/diff/1/nova/local_test.go#newcode251 nova/local_test.go:251: c.Assert(err, IsNil) Are these zones guaranteed to be returned in order? Should this be jc.SameContents https://codereview.appspot.com/103900045/diff/1/testservices/novaservice/serv... File testservices/novaservice/service.go (right): https://codereview.appspot.com/103900045/diff/1/testservices/novaservice/serv... testservices/novaservice/service.go:826: sort.Sort(azByName(zones)) Please document the order of az's is stored. https://codereview.appspot.com/103900045/diff/1/testservices/novaservice/serv... File testservices/novaservice/service_http_test.go (right): https://codereview.appspot.com/103900045/diff/1/testservices/novaservice/serv... testservices/novaservice/service_http_test.go:1196: c.Assert(expected.Zones, DeepEquals, zones) jc.SameContents
Please take a look. https://codereview.appspot.com/103900045/diff/1/nova/local_test.go File nova/local_test.go (right): https://codereview.appspot.com/103900045/diff/1/nova/local_test.go#newcode251 nova/local_test.go:251: c.Assert(err, IsNil) On 2014年06月06日 04:33:17, dfc wrote: > Are these zones guaranteed to be returned in order? Should this be > jc.SameContents Yes, they are sorted by the server. https://codereview.appspot.com/103900045/diff/1/testservices/novaservice/serv... File testservices/novaservice/service.go (right): https://codereview.appspot.com/103900045/diff/1/testservices/novaservice/serv... testservices/novaservice/service.go:826: sort.Sort(azByName(zones)) On 2014年06月06日 04:33:17, dfc wrote: > Please document the order of az's is stored. Added to the doc comment of this function. There's no user-facing place to add a comment. https://codereview.appspot.com/103900045/diff/1/testservices/novaservice/serv... File testservices/novaservice/service_http_test.go (right): https://codereview.appspot.com/103900045/diff/1/testservices/novaservice/serv... testservices/novaservice/service_http_test.go:1196: c.Assert(expected.Zones, DeepEquals, zones) On 2014年06月06日 04:33:17, dfc wrote: > jc.SameContents They are sorted by the server. No need to introduce a dependency on juju/testing/checkers here.
Do any of our openstack clouds actually support this extension? Neither canonistack nor HP seem to. I'm not totally up to dat on current plans, but this seemed to be a dead end a year ago, with different mechanisms talked about as fulfilling the role that weren't availabilty zone based.
On 2014年06月07日 14:54:10, gz wrote: > Do any of our openstack clouds actually support this extension? Neither > canonistack nor HP seem to. I'm not totally up to dat on current plans, but this > seemed to be a dead end a year ago, with different mechanisms talked about as > fulfilling the role that weren't availabilty zone based. HP's US West has 4 availability zones. I've tested this live.
Ah, interesting. I wasn't on their 12.12 deployment (which is being retired Monday), so must have been added for the 13.5 one.
LGTM. https://codereview.appspot.com/103900045/diff/20001/nova/local_test.go File nova/local_test.go (right): https://codereview.appspot.com/103900045/diff/20001/nova/local_test.go#newcod... nova/local_test.go:231: s.openstack.Nova.SetAvailabilityZones() This is pretty magical test server behaviour, but you have a nice big comment at least. https://codereview.appspot.com/103900045/diff/20001/nova/nova.go File nova/nova.go (right): https://codereview.appspot.com/103900045/diff/20001/nova/nova.go#newcode695 nova/nova.go:695: return nil, nil I'd prefer we return a specific error that can be checked, but that's not really a job for this proposal, we need better goose-wide handling of extensions and errors. https://codereview.appspot.com/103900045/diff/20001/testservices/novaservice/... File testservices/novaservice/service_http.go (right): https://codereview.appspot.com/103900045/diff/20001/testservices/novaservice/... testservices/novaservice/service_http.go:228: errAvailabilityZoneIsNotAvailable = &errorResponse{ Rather than doing this the old way, you should use the new testservices/errors.go method of creating the error.
Please take a look. https://codereview.appspot.com/103900045/diff/20001/nova/nova.go File nova/nova.go (right): https://codereview.appspot.com/103900045/diff/20001/nova/nova.go#newcode695 nova/nova.go:695: return nil, nil On 2014年06月07日 17:13:03, gz wrote: > I'd prefer we return a specific error that can be checked, but that's not really > a job for this proposal, we need better goose-wide handling of extensions and > errors. I think you're right, and I'd prefer to get the API right to begin with. The error type/method of checking may change later, but at least we should start out with propagating the error. I've created a error code in goose/errors for "not implemented". https://codereview.appspot.com/103900045/diff/20001/testservices/novaservice/... File testservices/novaservice/service_http.go (right): https://codereview.appspot.com/103900045/diff/20001/testservices/novaservice/... testservices/novaservice/service_http.go:228: errAvailabilityZoneIsNotAvailable = &errorResponse{ On 2014年06月07日 17:13:03, gz wrote: > Rather than doing this the old way, you should use the new > testservices/errors.go method of creating the error. Done.