Issue 7073060: nova double: list server filters and run server. (Closed)
|
Created:
11 years, 3 months ago by dimitern Modified:
11 years, 3 months ago Reviewers:
mp+142758 Visibility:
Public. |
Descriptionnova double: list server filters and run server.
Implemented filters by status and name (others not implemented, but
probably also not needed), as well as creating a server (RunServer).
Added a few needed things: serverByName and generateUUID + tests.
With this, the nova double will be up-to-date with the nova client
and functional.
https://code.launchpad.net/~dimitern/goose/nova-double-list-filters-run-server/+merge/142758
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 49
Patch Set 2 : nova double: list server filters and run server. #
Total comments: 14
Patch Set 3 : nova double: list server filters and run server. #
Total comments: 6
Patch Set 4 : nova double: list server filters and run server. #Patch Set 5 : nova double: list server filters and run server. #
MessagesTotal messages: 11
Please take a look.
Sign in to reply to this message.
I'm a bit uncertain about the order of some of the operations -- expand a little, please? https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... File testservices/novaservice/service_http.go (right): https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:486: n.buildFlavorLinks(&flavor) I'm not quite sure about the ordering here -- can we skip building the links until we know we can fulfil the request? Or are the subtleties I'm not aware of? https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:497: n.buildServerLinks(&server) Ditto. https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:509: if len(req.Server.SecurityGroups) > 0 { Similarly, can we not handle this error before we start messing with local state? https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:523: if sg, err := n.securityGroupByName(groupName); err != nil { Could we not have handled this error much earlier?
Sign in to reply to this message.
looks reasonable. a few comments below (mostly trivials) https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... File testservices/novaservice/service.go (right): https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:187: // filterServer returns true if the given does not match the given s/given/given server/ ? the semantics seem a little odd - returns true if it does *not* match...? how about matchServer(filter, server) bool, returning true if the server is matched by the filter? that would make most of the code a touch simpler. https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:197: found = true you can return here, i think. https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:206: found = true return here. https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... File testservices/novaservice/service_http.go (right): https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:447: n, err := rand.Read(uuid) you should use io.ReadFull here. there's no guarantee that Read always returns the required number of bytes, https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:453: uuid[4] = 0x40 // version 4 Pseudo Random, see page 7 these look like they're resetting bits unnecessarily (not that it probably matters much!) also, by my reading of the RFC, the second should be uuid[6], not uuid[4] (it's the high bits of time_hi_and_version). i'd expect to see something like this: uuid[8] = uuid[8] &^ 0xc0 | 0x80 uuid[6] = uuid[6] &^ 0xf0 | 0x40 for the record, i think it's nicer to have RFC cross-references as section-numbers rather than page numbers. https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:510: errNoGroup := &errorResponse{ i think it would might be better to create this where it's needed (it's only used in one place), or perhaps make a function noGroupError(groupName, tenantId) that returns a suitably crafted error, if you think that clutters the function too much. https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:537: resp.Server.SecurityGroups[0] = groups resp.Server.SecurityGroups = []map[string]string{{"name": "default"}} ? https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... File testservices/novaservice/service_http_test.go (right): https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http_test.go:560: defer s.service.removeServer(server.Id) this defer should be after the assert https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http_test.go:596: nova.ServerDetail{Id: "sr1", Name: "srv1", Status: nova.StatusBuild}, you can omit the type names from these initialisers. https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http_test.go:604: defer s.service.removeServer(server.Id) this defer should be after the assert https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http_test.go:689: defer s.service.removeSecurityGroup(1) this defer should be after the assert. https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http_test.go:692: defer s.service.removeSecurityGroup(2) ditto https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http_test.go:715: defer s.service.removeServer(server.Id) this defer should be after the assert. https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http_test.go:743: defer s.service.removeServer(server.Id) this defer should be after the assert https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http_test.go:773: nova.ServerDetail{Id: "sr1", Name: "srv1", Status: nova.StatusBuild}, lose the type names, as above. https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http_test.go:781: defer s.service.removeServer(server.Id) shouldn't this be after the assert? https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... File testservices/novaservice/service_test.go (right): https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_test.go:299: nova.ServerDetail{Id: "sr1", Name: "test", Status: nova.StatusActive}, lose the type names https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_test.go:344: nova.Entity{Id: "sr1"}, lose the type names https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_test.go:348: nova.ServerDetail{Id: entities[0].Id}, ditto https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_test.go:455: nova.ServerDetail{Id: "sr1", Name: "test"}, lose the type names
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... File testservices/novaservice/service.go (right): https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:187: // filterServer returns true if the given does not match the given On 2013/01/11 12:51:11, rog wrote: > s/given/given server/ ? > > the semantics seem a little odd - returns true if it does *not* match...? > > how about matchServer(filter, server) bool, returning true if the server is > matched by the filter? that would make most of the code a touch simpler. Yeah, it seems more clear this way. I was wondering myself about it - the sense I came up with was "true = filter that server". I'll make it matchServer(). https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:197: found = true On 2013/01/11 12:51:11, rog wrote: > you can return here, i think. Done. https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:206: found = true On 2013/01/11 12:51:11, rog wrote: > return here. Done. https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... File testservices/novaservice/service_http.go (right): https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:447: n, err := rand.Read(uuid) On 2013/01/11 12:51:11, rog wrote: > you should use io.ReadFull here. there's no guarantee that Read always returns > the required number of bytes, Done. https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:453: uuid[4] = 0x40 // version 4 Pseudo Random, see page 7 On 2013/01/11 12:51:11, rog wrote: > these look like they're resetting bits unnecessarily > (not that it probably matters much!) > > also, by my reading of the RFC, the second should be uuid[6], not uuid[4] (it's > the high bits of time_hi_and_version). > > i'd expect to see something like this: > > uuid[8] = uuid[8] &^ 0xc0 | 0x80 > uuid[6] = uuid[6] &^ 0xf0 | 0x40 > > for the record, i think it's nicer to have RFC cross-references as > section-numbers rather than page numbers. I just copied the code from the specified URL and changed it a bit, but my original intention was to have UUIDs like in canonistack, and since it's a testing service, the constraints on randomness and validity with the RFC are of little value. Anyway, I'll change it as you suggest. https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:486: n.buildFlavorLinks(&flavor) On 2013/01/11 12:29:20, fwereade wrote: > I'm not quite sure about the ordering here -- can we skip building the links > until we know we can fulfil the request? Or are the subtleties I'm not aware of? OpenStack seems very lenient to a lot of errors at this stage - even passing an invalid flavor/image ref sometimes does not cause the request to fail (as long as they're not empty). In our case here, I can return an error from addServer, which is not even needed (to follow OS behavior closely, I should ignore any error and still return accepted, but it felt wrong to do that). Anyway the links are needed before we attempt to add the server, otherwise it'll be added with nil links. https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:497: n.buildServerLinks(&server) On 2013/01/11 12:29:20, fwereade wrote: > Ditto. See above - this is usually the last step before addServer. https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:509: if len(req.Server.SecurityGroups) > 0 { On 2013/01/11 12:29:20, fwereade wrote: > Similarly, can we not handle this error before we start messing with local > state? At this point the server was added OK, and adding it or not to a group is not that critical IMHO - it'll anyway default to the default group in OS. Initially I didn't even check the group name or add it to the server, but since OS does this validation, once I handle the groups I decided to add it here. https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:510: errNoGroup := &errorResponse{ On 2013/01/11 12:51:11, rog wrote: > i think it would might be better to create this where it's needed (it's only > used in one place), or perhaps make a function noGroupError(groupName, tenantId) > that returns a suitably crafted error, if you think that clutters the function > too much. Done. https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:523: if sg, err := n.securityGroupByName(groupName); err != nil { On 2013/01/11 12:29:20, fwereade wrote: > Could we not have handled this error much earlier? That's the first time we get to know the group name, so we can check it. And the server has to be added first. https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:537: resp.Server.SecurityGroups[0] = groups On 2013/01/11 12:51:11, rog wrote: > resp.Server.SecurityGroups = []map[string]string{{"name": "default"}} > ? Nice! Done! https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... File testservices/novaservice/service_http_test.go (right): https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http_test.go:560: defer s.service.removeServer(server.Id) On 2013/01/11 12:51:11, rog wrote: > this defer should be after the assert Done. https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http_test.go:596: nova.ServerDetail{Id: "sr1", Name: "srv1", Status: nova.StatusBuild}, On 2013/01/11 12:51:11, rog wrote: > you can omit the type names from these initialisers. Done. https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http_test.go:604: defer s.service.removeServer(server.Id) On 2013/01/11 12:51:11, rog wrote: > this defer should be after the assert Done. https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http_test.go:689: defer s.service.removeSecurityGroup(1) On 2013/01/11 12:51:11, rog wrote: > this defer should be after the assert. Done. https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http_test.go:692: defer s.service.removeSecurityGroup(2) On 2013/01/11 12:51:11, rog wrote: > ditto Done. https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http_test.go:715: defer s.service.removeServer(server.Id) On 2013/01/11 12:51:11, rog wrote: > this defer should be after the assert. Done. https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http_test.go:743: defer s.service.removeServer(server.Id) On 2013/01/11 12:51:11, rog wrote: > this defer should be after the assert Done. https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http_test.go:773: nova.ServerDetail{Id: "sr1", Name: "srv1", Status: nova.StatusBuild}, On 2013/01/11 12:51:11, rog wrote: > lose the type names, as above. Done. https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http_test.go:781: defer s.service.removeServer(server.Id) On 2013/01/11 12:51:11, rog wrote: > shouldn't this be after the assert? Done. https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... File testservices/novaservice/service_test.go (right): https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_test.go:299: nova.ServerDetail{Id: "sr1", Name: "test", Status: nova.StatusActive}, On 2013/01/11 12:51:11, rog wrote: > lose the type names Done. https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_test.go:344: nova.Entity{Id: "sr1"}, On 2013/01/11 12:51:11, rog wrote: > lose the type names Done. https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_test.go:348: nova.ServerDetail{Id: entities[0].Id}, On 2013/01/11 12:51:11, rog wrote: > ditto Done. https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_test.go:455: nova.ServerDetail{Id: "sr1", Name: "test"}, On 2013/01/11 12:51:11, rog wrote: > lose the type names Done.
Sign in to reply to this message.
LGTM with a few trivial suggestions below. https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... File testservices/novaservice/service_http.go (right): https://codereview.appspot.com/7073060/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http.go:453: uuid[4] = 0x40 // version 4 Pseudo Random, see page 7 On 2013/01/11 14:02:11, dimitern wrote: > On 2013/01/11 12:51:11, rog wrote: > > these look like they're resetting bits unnecessarily > > (not that it probably matters much!) > > > > also, by my reading of the RFC, the second should be uuid[6], not uuid[4] > (it's > > the high bits of time_hi_and_version). > > > > i'd expect to see something like this: > > > > uuid[8] = uuid[8] &^ 0xc0 | 0x80 > > uuid[6] = uuid[6] &^ 0xf0 | 0x40 > > > > for the record, i think it's nicer to have RFC cross-references as > > section-numbers rather than page numbers. > > I just copied the code from the specified URL and changed it a bit, but my > original intention was to have UUIDs like in canonistack, and since it's a > testing service, the constraints on randomness and validity with the RFC are of > little value. Anyway, I'll change it as you suggest. if we're going to reference the RFC, we might as well get it right (we, or someone else, may copy the code later and assume it's correct). https://codereview.appspot.com/7073060/diff/8002/testservices/novaservice/ser... File testservices/novaservice/service_http.go (right): https://codereview.appspot.com/7073060/diff/8002/testservices/novaservice/ser... testservices/novaservice/service_http.go:444: // generateUUID generates a random UUID s/UUID/UUID conforming to RFC 4122. and perhaps "newUUID" might be a nicer name. https://codereview.appspot.com/7073060/diff/8002/testservices/novaservice/ser... testservices/novaservice/service_http.go:445: // (taken from: http://www.ashishbanerjee.com/home/go/go-generate-uuid/0 this URL isn't correct. and since we actually share no code from it any more, we may as well remove it. https://codereview.appspot.com/7073060/diff/8002/testservices/novaservice/ser... testservices/novaservice/service_http.go:449: if n != len(uuid) || err != nil { no need to check n here - err will be non-nil if it's not the required length. https://codereview.appspot.com/7073060/diff/8002/testservices/novaservice/ser... testservices/novaservice/service_http.go:452: // see RFC 4122 d https://codereview.appspot.com/7073060/diff/8002/testservices/novaservice/ser... testservices/novaservice/service_http.go:453: uuid[8] = uuid[8]&^0xc0 | 0x80 // variant bits see page 5 // variant bits; see section 4.1.1 https://codereview.appspot.com/7073060/diff/8002/testservices/novaservice/ser... testservices/novaservice/service_http.go:454: uuid[6] = uuid[6]&^0xf0 | 0x40 // version 4 Pseudo Random, see page 7 // version 4; see section 4.1.3 https://codereview.appspot.com/7073060/diff/8002/testservices/novaservice/ser... File testservices/novaservice/service_http_test.go (right): https://codereview.appspot.com/7073060/diff/8002/testservices/novaservice/ser... testservices/novaservice/service_http_test.go:518: nova.FlavorDetail{Id: "fl1", Name: "flavor 1"}, lose type names
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/7073060/diff/8002/testservices/novaservice/ser... File testservices/novaservice/service_http.go (right): https://codereview.appspot.com/7073060/diff/8002/testservices/novaservice/ser... testservices/novaservice/service_http.go:444: // generateUUID generates a random UUID On 2013/01/11 14:35:34, rog wrote: > s/UUID/UUID conforming to RFC 4122. > > and perhaps "newUUID" might be a nicer name. Done. https://codereview.appspot.com/7073060/diff/8002/testservices/novaservice/ser... testservices/novaservice/service_http.go:445: // (taken from: http://www.ashishbanerjee.com/home/go/go-generate-uuid/0 On 2013/01/11 14:35:34, rog wrote: > this URL isn't correct. and since we actually share no code from it any more, we > may as well remove it. Done. https://codereview.appspot.com/7073060/diff/8002/testservices/novaservice/ser... testservices/novaservice/service_http.go:449: if n != len(uuid) || err != nil { On 2013/01/11 14:35:34, rog wrote: > no need to check n here - err will be non-nil if it's not the required length. Done. https://codereview.appspot.com/7073060/diff/8002/testservices/novaservice/ser... testservices/novaservice/service_http.go:452: // see RFC 4122 On 2013/01/11 14:35:34, rog wrote: > d Done. https://codereview.appspot.com/7073060/diff/8002/testservices/novaservice/ser... testservices/novaservice/service_http.go:453: uuid[8] = uuid[8]&^0xc0 | 0x80 // variant bits see page 5 On 2013/01/11 14:35:34, rog wrote: > // variant bits; see section 4.1.1 Done. https://codereview.appspot.com/7073060/diff/8002/testservices/novaservice/ser... testservices/novaservice/service_http.go:454: uuid[6] = uuid[6]&^0xf0 | 0x40 // version 4 Pseudo Random, see page 7 On 2013/01/11 14:35:34, rog wrote: > // version 4; see section 4.1.3 Done. https://codereview.appspot.com/7073060/diff/8002/testservices/novaservice/ser... File testservices/novaservice/service_http_test.go (right): https://codereview.appspot.com/7073060/diff/8002/testservices/novaservice/ser... testservices/novaservice/service_http_test.go:518: nova.FlavorDetail{Id: "fl1", Name: "flavor 1"}, On 2013/01/11 14:35:34, rog wrote: > lose type names I thought I got all of these - I'll fix it, 10x.
Sign in to reply to this message.
LGTM assuming fix or explanation for the below. https://codereview.appspot.com/7073060/diff/12001/testservices/novaservice/se... File testservices/novaservice/service_http.go (right): https://codereview.appspot.com/7073060/diff/12001/testservices/novaservice/se... testservices/novaservice/service_http.go:492: } Surely you can collect the security groups you need here... https://codereview.appspot.com/7073060/diff/12001/testservices/novaservice/se... testservices/novaservice/service_http.go:518: n.buildServerLinks(&server) ...and add them here... https://codereview.appspot.com/7073060/diff/12001/testservices/novaservice/se... testservices/novaservice/service_http.go:530: if len(req.Server.SecurityGroups) > 0 { ...and thus avoid all this near-duplication here.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/7073060/diff/12001/testservices/novaservice/se... File testservices/novaservice/service_http.go (right): https://codereview.appspot.com/7073060/diff/12001/testservices/novaservice/se... testservices/novaservice/service_http.go:492: } On 2013/01/11 15:02:52, fwereade wrote: > Surely you can collect the security groups you need here... Done. https://codereview.appspot.com/7073060/diff/12001/testservices/novaservice/se... testservices/novaservice/service_http.go:518: n.buildServerLinks(&server) On 2013/01/11 15:02:52, fwereade wrote: > ...and add them here... Not here, but once the server is created - server ID has to be existing to add a group Id to it. https://codereview.appspot.com/7073060/diff/12001/testservices/novaservice/se... testservices/novaservice/service_http.go:530: if len(req.Server.SecurityGroups) > 0 { On 2013/01/11 15:02:52, fwereade wrote: > ...and thus avoid all this near-duplication here. Done.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
*** Submitted: nova double: list server filters and run server. Implemented filters by status and name (others not implemented, but probably also not needed), as well as creating a server (RunServer). Added a few needed things: serverByName and generateUUID + tests. With this, the nova double will be up-to-date with the nova client and functional. R=fwereade, rog CC= https://codereview.appspot.com/7073060
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
