Issue 6877054: First part of the nova service double. (Closed)
DescriptionFirst part of the nova service double.
As discussed, I'm starting to propose bits of the nova service double in chunks, as I go.
The direct API and its tests are ready. Added a comprehensive comment about RuleInfo struct,
to describe better the types of security group rules.
https://code.launchpad.net/~dimitern/goose/nova-testing-service/+merge/138115
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 87
Patch Set 2 : First part of the nova service double. #
Total comments: 45
Patch Set 3 : First part of the nova service double. #
Total comments: 2
Patch Set 4 : First part of the nova service double. #Patch Set 5 : First part of the nova service double. #Patch Set 6 : First part of the nova service double. #
MessagesTotal messages: 13
Please take a look.
Sign in to reply to this message.
This is coming along nicely. I'd like to see what the answers to the various questions are before I mark as approved. https://codereview.appspot.com/6877054/diff/1/nova/nova.go File nova/nova.go (right): https://codereview.appspot.com/6877054/diff/1/nova/nova.go#newcode274 nova/nova.go:274: // http://docs.openstack.org/developer/nova/nova.concepts.html#concept-security-... Thank you for adding such a comprehensive description, very useful. Minor fix - there are a few typos to clean up. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/novase... File testservices/novaservice/novaservice.go (right): https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/novase... testservices/novaservice/novaservice.go:15: } I don't understand why Entity is included here. When a new flavor is added, wouldn't we require that a full FlavorDetail record be used? https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/novase... testservices/novaservice/novaservice.go:21: } See above comment about the use of Entity here. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/novase... testservices/novaservice/novaservice.go:84: AddServerSecurityGroup(serverId string, groupId int) error The nova client uses groupName rather than groupId. Should this be changed to match? Also applies to other security group methods below. Or perhaps this is just the internal API and the http server part will do the group name lookup? But even so, perhaps it would be nice to try and keep the http server quite simple, and have the service double do the heavy lifting? https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/novase... testservices/novaservice/novaservice.go:108: AddServerFloatingIP(serverId string, ipId int) error The nova client uses address as string rather than ipId as int. Do we need to match that here? See above comment. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... File testservices/novaservice/service.go (right): https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:28: servers: make(map[string]Server), Why not nova.FlavorDetail, nova.ServerDetail? https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:45: return fmt.Errorf("refusing to add a nil flavor") perhaps better worded as "cannot add nil flavour" https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:72: The existing nova client doesn't currently have a GetFlavor() API. Are we planning to add one? Or, if this is just for use by the http server, perhaps we can add a comment to mark which methods are internal only? https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:84: The nova client specifies the expected response format as: var resp struct { Flavors []FlavorDetail } Will the above produce data that will be marshalled to the required format? I'm not sure it will? Don't we want to return a collection of nova.FlavorDetail? https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:125: Same question about data format. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:137: Same question about data format. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:202: return fmt.Errorf("trying to add a rule to unknown security group %d", rule.ParentGroupId) perhaps reword as "cannot add ..." https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... File testservices/novaservice/service_test.go (right): https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service_test.go:23: A general comment - the individual tests seem to do too much. My preference would be for a large number of smaller tests, each test only verifying one specific aspect of the required behaviour. But perhaps we can be more tolerant here since these tests are for an internal tool.
Sign in to reply to this message.
Overall LGTM. Some small bits to cleanup, but a lot of that could be done in a follow up branch. https://codereview.appspot.com/6877054/diff/1/nova/nova.go File nova/nova.go (right): https://codereview.appspot.com/6877054/diff/1/nova/nova.go#newcode258 nova/nova.go:258: // inside the effective groups for the server can change at any time. Typo "restarted". Also, is it "when the server is running" or is it "when the server is starting". Since you mention "changing them won't take effect until ... restarted". Also, the "part of their parent group" doesn't really make sense. Maybe a different writing: Rules can be added and removed while the servers are running. The set of security groups that apply to a server are only evaluated when the server is started. Adding or removing a security group will not take effect until that server is restarted. However, changing the rules of existing groups will take effect immediately. https://codereview.appspot.com/6877054/diff/1/nova/nova.go#newcode271 nova/nova.go:271: // the rule is added. Offhand, it feels like these should be specified on the individual attributes, rather than in the global definition. I could be wrong. If it is hard to put multiline descriptions on individual attributes, it might be better here. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/novase... File testservices/novaservice/novaservice.go (right): https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/novase... testservices/novaservice/novaservice.go:84: AddServerSecurityGroup(serverId string, groupId int) error We might need both a AddServerSecurityGroupById and an AddServerSecurityGroupByName. I think if it is something user-visible, then we want by-name (I want to add group X to server Y would certainly be by name). But if it is internal then by Id makes sense. I suppose it also depends what the actual HTTP API is. I think at this level we want these functions to be pretty thin shims over the HTTP level. (eg, if using name would require an extra round trip to lookup the id, then we should use by-id here) https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/novase... testservices/novaservice/novaservice.go:108: AddServerFloatingIP(serverId string, ipId int) error I don't know the HTTP api, but ip as an int only works in ipv4. Perhaps Openstack is not IPv6 compatible but we would need 128 bit integers there. So it certainly seems like we need a string here. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... File testservices/novaservice/service_test.go (right): https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service_test.go:23: On 2012/12/06 00:26:04, wallyworld wrote: > A general comment - the individual tests seem to do too much. My preference > would be for a large number of smaller tests, each test only verifying one > specific aspect of the required behaviour. But perhaps we can be more tolerant > here since these tests are for an internal tool. I generally agree that you want to shoot for only really 1 real assertion per test. (Sometimes you also have to assert pre and post conditions.) The only caveat is that it is really hard to test Add without Get/Has, and Remove without Add, etc. On that topic, I'm surprised there is a "HasFlavor" vs a "GetFlavor" since the former only returns 1 bit of information, while the latter includes that bit and a lot more. But if it is the OpenStack api, then we need to preserve that. So I would probably say some sort of smoke test of add+has+remove+has. But do more testing of edge cases (adding twice, removing twice, etc) in separate tests.
Sign in to reply to this message.
this is looking nice. lots of comments, but not much substantial. https://codereview.appspot.com/6877054/diff/1/nova/nova.go File nova/nova.go (right): https://codereview.appspot.com/6877054/diff/1/nova/nova.go#newcode252 nova/nova.go:252: // Every rule is essentially an iptables ACCEPT rule, thus a group "essentially" is usually an unnecessary addition to a sentence. is it the same as an iptables ACCEPT rule or not? if not, how does it differ. https://codereview.appspot.com/6877054/diff/1/nova/nova.go#newcode285 nova/nova.go:285: /// CreateSecurityGroupRule creates a security group rule. s/\/// https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/novase... File testservices/novaservice/novaservice.go (right): https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/novase... testservices/novaservice/novaservice.go:25: type NovaService interface { any particular reason this is an interface? the ec2 double works fine with a static type FWIW. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... File testservices/novaservice/service.go (right): https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:42: func (n *Nova) AddFlavor(flavor Flavor) error { given that the plan is for this to act as an http server, i'm slightly concerned about all these public methods acting on the Nova type with no synchronisation, as the server will likewise be acting on this type, presumably in a different goroutine. the approach i took in the ec2 double was to export only methods that gave me functionality i couldn't get via the http API (which meant that for most methods it was easy to use a mutex in a single place). is there a particular reason all these methods need to be exposed? (there's no reason you can leave these methods unexposed and have the current tests access them directly, BTW, as in that case you can make sure there's no other entity making concurrent changes). https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:45: return fmt.Errorf("refusing to add a nil flavor") i'd probably write this as: switch { case flavor.entity != nil: id = flavor.entity.Id case flavor.detail != nil: id = flavor.detail.Id default: return fmt.Errorf("cannot add nil flavour") } one more line, but clearer IMHO. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:58: // HasFlavor verifies the given flavor exists or not. the usual convention for bool-returning comments would have: // HasFlavor returns whether the given flavor exists. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:97: if server.server == nil && server.detail == nil { same switch as suggested above? https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:111: // HasServer verifies the given server exists or not. // HasServer returns whether the given server exists. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:202: return fmt.Errorf("trying to add a rule to unknown security group %d", rule.ParentGroupId) On 2012/12/06 00:26:04, wallyworld wrote: > perhaps reword as "cannot add ..." +1 https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:247: if !ok { just a thought: return ok && (groupId == -1 || n.HasSecurityGroup(groupId) && rule.ParentGroupId == groupId) ? https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:276: idx := -1 given that this code is almost exactly repeated elsewhere, why not abstract it into a function. also, the []int doesn't seem to be exposed anywhere in the public API - might a map[int]bool be more appropriate? https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:303: if ok { d (and the ok can be removed too) https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:321: if !ok { d (and you can remove the ok too) https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:341: if !ok { d (and you can remove the ok too) https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:371: if len(n.floatingIPs) == 0 { d https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:392: // AllFlotingIPs returns a list of all created floating IPs. s/Floting/Floating/ https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:394: if len(n.floatingIPs) == 0 { it seems to me that returning an empty slice should be just fine. no floating ips isn't necessarily an error, is it? https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:423: fips, ok := n.serverIPs[serverId] i think you can dispense with the ok logic entirely here and it'll work fine. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:442: if !ok { d (and you can remove the ok above too) https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:464: if !ok { d (and the ok can go too) https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... File testservices/novaservice/service_http_test.go (right): https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http_test.go:31: func (s *NovaHTTPSuite) TearDownSuite(c *C) { FWIW i usually pair SetUpSuite with TearDownSuite so the teardown context is close to the setup context, but YMMV. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... File testservices/novaservice/service_test.go (right): https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service_test.go:129: if servers[0].server == nil { why not swap the elements in servers instead and save some columns? https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service_test.go:132: c.Assert(expectedServers, DeepEquals, servers) c.Assert(servers, DeepEquals, expectedServers) gocheck wants the thing you're testing to be on the left. same applies to other tests further down. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service_test.go:234: c.Assert(rule.FromPort, Not(IsNil)) s/Not(IsNil)/NotNil/
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6877054/diff/1/nova/nova.go File nova/nova.go (right): https://codereview.appspot.com/6877054/diff/1/nova/nova.go#newcode252 nova/nova.go:252: // Every rule is essentially an iptables ACCEPT rule, thus a group On 2012/12/06 17:32:41, rog wrote: > "essentially" is usually an unnecessary addition to a sentence. is it the same > as an iptables ACCEPT rule or not? if not, how does it differ. So they clain in the docs. Ok, I'll remove the essentially. https://codereview.appspot.com/6877054/diff/1/nova/nova.go#newcode258 nova/nova.go:258: // inside the effective groups for the server can change at any time. On 2012/12/06 06:07:10, jameinel wrote: > Typo "restarted". > > Also, is it "when the server is running" or is it "when the server is starting". > Since you mention "changing them won't take effect until ... restarted". > Also, the "part of their parent group" doesn't really make sense. > > Maybe a different writing: > Rules can be added and removed while the servers are running. The set of > security groups that apply to a server are only evaluated when the server is > started. Adding or removing a security group will not take effect until that > server is restarted. However, changing the rules of existing groups will take > effect immediately. I'll fix the typo, and will use the text you suggested - it looks better, 10x :) https://codereview.appspot.com/6877054/diff/1/nova/nova.go#newcode271 nova/nova.go:271: // the rule is added. On 2012/12/06 06:07:10, jameinel wrote: > Offhand, it feels like these should be specified on the individual attributes, > rather than in the global definition. I could be wrong. If it is hard to put > multiline descriptions on individual attributes, it might be better here. Ok, I'll reformat the comments to be on the individual fields https://codereview.appspot.com/6877054/diff/1/nova/nova.go#newcode274 nova/nova.go:274: // http://docs.openstack.org/developer/nova/nova.concepts.html#concept-security-... On 2012/12/06 00:26:04, wallyworld wrote: > Thank you for adding such a comprehensive description, very useful. Minor fix - > there are a few typos to clean up. Done. https://codereview.appspot.com/6877054/diff/1/nova/nova.go#newcode285 nova/nova.go:285: /// CreateSecurityGroupRule creates a security group rule. On 2012/12/06 17:32:41, rog wrote: > s/\/// Done. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/novase... File testservices/novaservice/novaservice.go (right): https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/novase... testservices/novaservice/novaservice.go:15: } On 2012/12/06 00:26:04, wallyworld wrote: > I don't understand why Entity is included here. When a new flavor is added, > wouldn't we require that a full FlavorDetail record be used? A discussed, I'll change that. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/novase... testservices/novaservice/novaservice.go:21: } On 2012/12/06 00:26:04, wallyworld wrote: > See above comment about the use of Entity here. Done. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/novase... testservices/novaservice/novaservice.go:25: type NovaService interface { On 2012/12/06 17:32:41, rog wrote: > any particular reason this is an interface? the ec2 double works fine with a > static type FWIW. I'll remove it in a follow up, so not to clutter up the CL. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/novase... testservices/novaservice/novaservice.go:84: AddServerSecurityGroup(serverId string, groupId int) error On 2012/12/06 00:26:04, wallyworld wrote: > The nova client uses groupName rather than groupId. Should this be changed to > match? Also applies to other security group methods below. Or perhaps this is > just the internal API and the http server part will do the group name lookup? > But even so, perhaps it would be nice to try and keep the http server quite > simple, and have the service double do the heavy lifting? This is just in the internal API and for convenience in tests. The HTTP API will work as OS does. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/novase... testservices/novaservice/novaservice.go:84: AddServerSecurityGroup(serverId string, groupId int) error On 2012/12/06 06:07:10, jameinel wrote: > We might need both a AddServerSecurityGroupById and an > AddServerSecurityGroupByName. I think if it is something user-visible, then we > want by-name (I want to add group X to server Y would certainly be by name). But > if it is internal then by Id makes sense. I suppose it also depends what the > actual HTTP API is. I think at this level we want these functions to be pretty > thin shims over the HTTP level. (eg, if using name would require an extra round > trip to lookup the id, then we should use by-id here) I don't see any good in adding 2 methods - it's just an internal API used in tests after all. HTTP API will work as expected still. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/novase... testservices/novaservice/novaservice.go:108: AddServerFloatingIP(serverId string, ipId int) error On 2012/12/06 00:26:04, wallyworld wrote: > The nova client uses address as string rather than ipId as int. Do we need to > match that here? See above comment. Again, this is useful for tests only, HTTP API is not affected. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/novase... testservices/novaservice/novaservice.go:108: AddServerFloatingIP(serverId string, ipId int) error On 2012/12/06 06:07:10, jameinel wrote: > I don't know the HTTP api, but ip as an int only works in ipv4. Perhaps > Openstack is not IPv6 compatible but we would need 128 bit integers there. > So it certainly seems like we need a string here. OS is IPv6 capable (I think), this ID here is the the ID of the floating IP, not the IP itself as int32. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... File testservices/novaservice/service.go (right): https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:28: servers: make(map[string]Server), On 2012/12/06 00:26:04, wallyworld wrote: > Why not nova.FlavorDetail, nova.ServerDetail? Done. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:42: func (n *Nova) AddFlavor(flavor Flavor) error { On 2012/12/06 17:32:41, rog wrote: > given that the plan is for this to act as an http server, i'm slightly concerned > about all these public methods acting on the Nova type with no synchronisation, > as the server will likewise be acting on this type, presumably in a different > goroutine. > > the approach i took in the ec2 double was to export only methods that gave me > functionality i couldn't get via the http API (which meant that for most methods > it was easy to use a mutex in a single place). is there a particular reason all > these methods need to be exposed? > > (there's no reason you can leave these methods unexposed and have the current > tests access them directly, BTW, as in that case you can make sure there's no > other entity making concurrent changes). Well, I though about concurrency issues, but couldn't find a good way to test for such sync issues. Any idea how I can force such a conflict to occur reliably? https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:45: return fmt.Errorf("refusing to add a nil flavor") On 2012/12/06 00:26:04, wallyworld wrote: > perhaps better worded as "cannot add nil flavour" Done. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:45: return fmt.Errorf("refusing to add a nil flavor") On 2012/12/06 17:32:41, rog wrote: > i'd probably write this as: > > switch { > case flavor.entity != nil: > id = flavor.entity.Id > case flavor.detail != nil: > id = flavor.detail.Id > default: > return fmt.Errorf("cannot add nil flavour") > } > > one more line, but clearer IMHO. Good to know, though I changed and hopefully simplified the logic here already. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:58: // HasFlavor verifies the given flavor exists or not. On 2012/12/06 17:32:41, rog wrote: > the usual convention for bool-returning comments would have: > > // HasFlavor returns whether the given flavor exists. Done. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:72: On 2012/12/06 00:26:04, wallyworld wrote: > The existing nova client doesn't currently have a GetFlavor() API. Are we > planning to add one? Or, if this is just for use by the http server, perhaps we > can add a comment to mark which methods are internal only? It's only used internally in tests - all of the methods are, the HTTP API is the same. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:84: On 2012/12/06 00:26:04, wallyworld wrote: > The nova client specifies the expected response format as: > var resp struct { > Flavors []FlavorDetail > } > > Will the above produce data that will be marshalled to the required format? I'm > not sure it will? Don't we want to return a collection of nova.FlavorDetail? As discussed I'm changing this. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:97: if server.server == nil && server.detail == nil { On 2012/12/06 17:32:41, rog wrote: > same switch as suggested above? See previous. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:111: // HasServer verifies the given server exists or not. On 2012/12/06 17:32:41, rog wrote: > // HasServer returns whether the given server exists. Done. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:125: On 2012/12/06 00:26:04, wallyworld wrote: > Same question about data format. Done. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:137: On 2012/12/06 00:26:04, wallyworld wrote: > Same question about data format. Done. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:202: return fmt.Errorf("trying to add a rule to unknown security group %d", rule.ParentGroupId) On 2012/12/06 00:26:04, wallyworld wrote: > perhaps reword as "cannot add ..." Done. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:202: return fmt.Errorf("trying to add a rule to unknown security group %d", rule.ParentGroupId) On 2012/12/06 17:32:41, rog wrote: > On 2012/12/06 00:26:04, wallyworld wrote: > > perhaps reword as "cannot add ..." > > +1 Done. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:247: if !ok { On 2012/12/06 17:32:41, rog wrote: > just a thought: > > return ok && (groupId == -1 || n.HasSecurityGroup(groupId) && rule.ParentGroupId > == groupId) > > ? Done. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:276: idx := -1 On 2012/12/06 17:32:41, rog wrote: > given that this code is almost exactly repeated elsewhere, why not abstract it > into a function. also, the []int doesn't seem to be exposed anywhere in the > public API - might a map[int]bool be more appropriate? It is exposed indirectly through HasSecurityGroupRule(), and having map[int]bool won't work here because I need to serialize the group.Rules as well in the response. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:303: if ok { On 2012/12/06 17:32:41, rog wrote: > d (and the ok can be removed too) I didn't get that.. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:321: if !ok { On 2012/12/06 17:32:41, rog wrote: > d (and you can remove the ok too) d? https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:341: if !ok { On 2012/12/06 17:32:41, rog wrote: > d (and you can remove the ok too) d? https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:371: if len(n.floatingIPs) == 0 { On 2012/12/06 17:32:41, rog wrote: > d d? https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:392: // AllFlotingIPs returns a list of all created floating IPs. On 2012/12/06 17:32:41, rog wrote: > s/Floting/Floating/ Done. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:394: if len(n.floatingIPs) == 0 { On 2012/12/06 17:32:41, rog wrote: > it seems to me that returning an empty slice should be just fine. no floating > ips isn't necessarily an error, is it? Well for the internal API and the tests it is, because that seemed more sensible to me. If it feels weird, I can remove it. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:423: fips, ok := n.serverIPs[serverId] On 2012/12/06 17:32:41, rog wrote: > i think you can dispense with the ok logic entirely here and it'll work fine. It won't work, because I want to catch the error when the IP is already added to that server. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:442: if !ok { On 2012/12/06 17:32:41, rog wrote: > d (and you can remove the ok above too) d? why? https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:464: if !ok { On 2012/12/06 17:32:41, rog wrote: > d (and the ok can go too) d? https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... File testservices/novaservice/service_http_test.go (right): https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service_http_test.go:31: func (s *NovaHTTPSuite) TearDownSuite(c *C) { On 2012/12/06 17:32:41, rog wrote: > FWIW i usually pair SetUpSuite with TearDownSuite so the teardown context is > close to the setup context, but YMMV. Done. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... File testservices/novaservice/service_test.go (right): https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service_test.go:23: On 2012/12/06 06:07:10, jameinel wrote: > On 2012/12/06 00:26:04, wallyworld wrote: > > A general comment - the individual tests seem to do too much. My preference > > would be for a large number of smaller tests, each test only verifying one > > specific aspect of the required behaviour. But perhaps we can be more tolerant > > here since these tests are for an internal tool. > > I generally agree that you want to shoot for only really 1 real assertion per > test. (Sometimes you also have to assert pre and post conditions.) > The only caveat is that it is really hard to test Add without Get/Has, and > Remove without Add, etc. > > On that topic, I'm surprised there is a "HasFlavor" vs a "GetFlavor" since the > former only returns 1 bit of information, while the latter includes that bit and > a lot more. But if it is the OpenStack api, then we need to preserve that. > > So I would probably say some sort of smoke test of add+has+remove+has. But do > more testing of edge cases (adding twice, removing twice, etc) in separate > tests. Ok, I can separate the tests more for the edge cases. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service_test.go:23: On 2012/12/06 00:26:04, wallyworld wrote: > A general comment - the individual tests seem to do too much. My preference > would be for a large number of smaller tests, each test only verifying one > specific aspect of the required behaviour. But perhaps we can be more tolerant > here since these tests are for an internal tool. Exactly, these are internal tests, not much different from the same ones in the swift double. It's hard to test only Add without doing Has/Get and Remove in the same test. After spending a few hours however, I split the test cases in a more granular fashion. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service_test.go:129: if servers[0].server == nil { On 2012/12/06 17:32:41, rog wrote: > why not swap the elements in servers instead and save some columns? Done. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service_test.go:132: c.Assert(expectedServers, DeepEquals, servers) On 2012/12/06 17:32:41, rog wrote: > c.Assert(servers, DeepEquals, expectedServers) > > gocheck wants the thing you're testing to be on the left. > same applies to other tests further down. Done. https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service_test.go:234: c.Assert(rule.FromPort, Not(IsNil)) On 2012/12/06 17:32:41, rog wrote: > s/Not(IsNil)/NotNil/ Done.
Sign in to reply to this message.
LGTM with suggested fixes and rogpeppe's issues addressed https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... File testservices/novaservice/service.go (right): https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service.go:88: flavors := []nova.FlavorDetail{} I'm still not sure this is 100% correct? The goose nova client expects data of the form: var resp struct { Flavors []Entity } Maybe same comment for other methods. https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... File testservices/novaservice/service_test.go (right): https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_test.go:58: c.Assert(err, IsNil) I think we also need to assert that the newly added flavor exists in addition to err == nil Same comment for addServer etc
Sign in to reply to this message.
getting there! https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... File testservices/novaservice/service.go (right): https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:42: func (n *Nova) AddFlavor(flavor Flavor) error { On 2012/12/07 00:23:33, dimitern wrote: > On 2012/12/06 17:32:41, rog wrote: > > given that the plan is for this to act as an http server, i'm slightly > concerned > > about all these public methods acting on the Nova type with no > synchronisation, > > as the server will likewise be acting on this type, presumably in a different > > goroutine. > > > > the approach i took in the ec2 double was to export only methods that gave me > > functionality i couldn't get via the http API (which meant that for most > methods > > it was easy to use a mutex in a single place). is there a particular reason > all > > these methods need to be exposed? > > > > (there's no reason you can leave these methods unexposed and have the current > > tests access them directly, BTW, as in that case you can make sure there's no > > other entity making concurrent changes). > > Well, I though about concurrency issues, but couldn't find a good way to test > for such sync issues. Any idea how I can force such a conflict to occur > reliably? you can't - race conditions are difficult to test for (although there is now a race detector in Go tip, which could help in the future). however, if you plan on having concurrent access to a shared data structure, you must protect it in some way. if you're serving web pages, there's a nice single entry point (ServeHTTP) and it's easy to wrap a mutex.Lock/Unlock around it if needed. but in this case, you've got many public methods, all of which will be called concurrently with the web server, so you either need to add a mutex lock to all of them, or unexport them and have them only called from the http server (and tests, possibly). https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:394: if len(n.floatingIPs) == 0 { On 2012/12/07 00:23:33, dimitern wrote: > On 2012/12/06 17:32:41, rog wrote: > > it seems to me that returning an empty slice should be just fine. no floating > > ips isn't necessarily an error, is it? > Well for the internal API and the tests it is, because that seemed more sensible > to me. If it feels weird, I can remove it. yeah, i think it feels a little weird. what's wrong with zero? https://codereview.appspot.com/6877054/diff/1/testservices/novaservice/servic... testservices/novaservice/service.go:423: fips, ok := n.serverIPs[serverId] On 2012/12/07 00:23:33, dimitern wrote: > On 2012/12/06 17:32:41, rog wrote: > > i think you can dispense with the ok logic entirely here and it'll work fine. > > It won't work, because I want to catch the error when the IP is already added to > that server. what i mean is that you can do: fips := n.serverIPs[serverId] for _, fipId := range fips { ... } no need to test whether the serverId is in the map - it'll return an empty slice if it isn't, and the range logic will work fine. same applies to similar comments i've made elsewhere. https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... File testservices/novaservice/service.go (right): https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service.go:100: entities := []nova.Entity{} FYI: var entities []nova.Entity is just as good, has slightly lower runtime cost and is fewer characters (and cleaner IMHO). https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service.go:291: _, err := n.GetSecurityGroup(groupId) if you're going to test whether a security group exists by using GetSecurityGroup here, then perhaps you should be consistent and use it everywhere, instead of looking in n.groups directly. (BTW "GetSecurityGroup" would more usually be phrased as simply "SecurityGroup" - the "Get" is usually omitted for accessors). https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... File testservices/novaservice/service_test.go (right): https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_test.go:166: defer s.removeFlavor(c, flavors[0]) this should be a line earlier. what if the second addFlavor fails? https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_test.go:181: nova.Entity{Id: "fl1", Links: []nova.Link{}}, this looks slightly fishy to me. i don't think we should care if the returned links are nil or not nil. i wouldn't mind testing against nil though, if that made things easier, as that's the usual zero value. (in that case you could just delete the Links initialisation) https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_test.go:190: defer s.removeFlavor(c, flavors[0]) move one line up https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_test.go:208: Links: []nova.Link{}, again, fishy. https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_test.go:220: Links: []nova.Link{}, ditto https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_test.go:290: defer s.removeServer(c, servers[0]) move one line up https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_test.go:340: Links: []nova.Link{}, as above https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_test.go:411: defer s.removeGroup(c, groups[0]) move one line up https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_test.go:498: defer s.removeGroup(c, srcGroup) move one line up https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_test.go:765: defer s.removeIP(c, fips[0]) move one line up
Sign in to reply to this message.
Getting close. Please consider what happens if the service double is accessed concurrently, at the moment there is nothing preventing data races of most of the internal structures. https://codereview.appspot.com/6877054/diff/9001/nova/nova.go File nova/nova.go (right): https://codereview.appspot.com/6877054/diff/9001/nova/nova.go#newcode63 nova/nova.go:63: Swap *string // Can be empty (nil) What does swap refer to ? swap device ? There could be more than one ? Swap amount ? Should this be an int ? https://codereview.appspot.com/6877054/diff/9001/nova/nova_test.go File nova/nova_test.go (right): https://codereview.appspot.com/6877054/diff/9001/nova/nova_test.go#newcode205 nova/nova_test.go:205: c.Check(rule.Group, Not(IsNil)) c.Check(rule.Group, NotNil) https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... File testservices/novaservice/service.go (right): https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service.go:12: type Nova struct { I think you need a sync.Mutex here (and used in all the accessors below) to ensure the various maps are not corrupted with concurrent mutation. https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service.go:143: return nova.ServerDetail{}, fmt.Errorf("no such server %q", serverId) this is a hint that you may want to return a *nova.ServerDetail https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service.go:152: return nova.Entity{}, err same https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service.go:219: return nil, fmt.Errorf("no security groups to return") Is this really an error ?
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6877054/diff/9001/nova/nova.go File nova/nova.go (right): https://codereview.appspot.com/6877054/diff/9001/nova/nova.go#newcode63 nova/nova.go:63: Swap *string // Can be empty (nil) On 2012/12/10 03:33:39, dfc wrote: > What does swap refer to ? swap device ? There could be more than one ? Swap > amount ? Should this be an int ? It seems so yes, although openstack does not always report that one the machines and this is frequently missing. No idea what happens when more than one swap device is present. Anyway, because this is confusing and unhelpful, I'm removing this until we need it. https://codereview.appspot.com/6877054/diff/9001/nova/nova_test.go File nova/nova_test.go (right): https://codereview.appspot.com/6877054/diff/9001/nova/nova_test.go#newcode205 nova/nova_test.go:205: c.Check(rule.Group, Not(IsNil)) On 2012/12/10 03:33:39, dfc wrote: > c.Check(rule.Group, NotNil) Done. https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... File testservices/novaservice/service.go (right): https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service.go:12: type Nova struct { On 2012/12/10 03:33:39, dfc wrote: > I think you need a sync.Mutex here (and used in all the accessors below) to > ensure the various maps are not corrupted with concurrent mutation. As discussed, I'll make all the direct API calls private and add a mutex at ServerHTTP. https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service.go:88: flavors := []nova.FlavorDetail{} On 2012/12/07 11:32:33, wallyworld wrote: > I'm still not sure this is 100% correct? > The goose nova client expects data of the form: > var resp struct { > Flavors []Entity > } > Maybe same comment for other methods. Now it will be alright - the internal state is consistent and only needs to be serialized in the HTTP responses. That's why here is the AllFlavorsAsEntities() etc. https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service.go:100: entities := []nova.Entity{} On 2012/12/07 12:15:35, rog wrote: > FYI: > var entities []nova.Entity > > is just as good, has slightly lower runtime cost and is fewer characters (and > cleaner IMHO). Done. https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service.go:143: return nova.ServerDetail{}, fmt.Errorf("no such server %q", serverId) On 2012/12/10 03:33:39, dfc wrote: > this is a hint that you may want to return a *nova.ServerDetail Done. https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service.go:152: return nova.Entity{}, err On 2012/12/10 03:33:39, dfc wrote: > same Done. https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service.go:219: return nil, fmt.Errorf("no security groups to return") On 2012/12/10 03:33:39, dfc wrote: > Is this really an error ? Well, openstack returns 204 when no items are returned for lists, but I guess I can remove the error and just return an empty slice (the same for the other All* methods) https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service.go:291: _, err := n.GetSecurityGroup(groupId) On 2012/12/07 12:15:35, rog wrote: > if you're going to test whether a security group exists by using > GetSecurityGroup here, then perhaps you should be consistent and use it > everywhere, instead of looking in n.groups directly. > > (BTW "GetSecurityGroup" would more usually be phrased as simply "SecurityGroup" > - the "Get" is usually omitted for accessors). Ok, I'll make this more consistent. But for now, I won't change all Get* methods for now, so not to complicate the CL. I'll do it in a follow up along with removing the interface. https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... File testservices/novaservice/service_test.go (right): https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_test.go:58: c.Assert(err, IsNil) On 2012/12/07 11:32:33, wallyworld wrote: > I think we also need to assert that the newly added flavor exists in addition to > err == nil > Same comment for addServer etc Well, assuming Add*() does its job and returns err == nil, this should be enough, and besides I do check that in TestAddRemove*() in a few places (checking it exists before removing it). https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_test.go:166: defer s.removeFlavor(c, flavors[0]) On 2012/12/07 12:15:35, rog wrote: > this should be a line earlier. what if the second addFlavor fails? Done. https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_test.go:181: nova.Entity{Id: "fl1", Links: []nova.Link{}}, On 2012/12/07 12:15:35, rog wrote: > this looks slightly fishy to me. i don't think we should care if the returned > links are nil or not nil. i wouldn't mind testing against nil though, if that > made things easier, as that's the usual zero value. (in that case you could just > delete the Links initialisation) The idea is, AddFlavor/Server will create the links automatically if nil, and subsequently DeepEquals will fail, unless I really put both links as expected, which I think is unnecessary here - I test the links are created properly in other test cases. Anyway, I changed that now with buildLinks bool passed explicitly as a second argument to addServer/addFlavor. If true the logic for generating links will kick in, otherwise leaving it nil and thus simplifying the logic in a few places. https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_test.go:190: defer s.removeFlavor(c, flavors[0]) On 2012/12/07 12:15:35, rog wrote: > move one line up Done. https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_test.go:208: Links: []nova.Link{}, On 2012/12/07 12:15:35, rog wrote: > again, fishy. Done. https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_test.go:220: Links: []nova.Link{}, On 2012/12/07 12:15:35, rog wrote: > ditto Done. https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_test.go:290: defer s.removeServer(c, servers[0]) On 2012/12/07 12:15:35, rog wrote: > move one line up Done. https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_test.go:340: Links: []nova.Link{}, On 2012/12/07 12:15:35, rog wrote: > as above Done. https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_test.go:411: defer s.removeGroup(c, groups[0]) On 2012/12/07 12:15:35, rog wrote: > move one line up Done. https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_test.go:498: defer s.removeGroup(c, srcGroup) On 2012/12/07 12:15:35, rog wrote: > move one line up Done. https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_test.go:765: defer s.removeIP(c, fips[0]) On 2012/12/07 12:15:35, rog wrote: > move one line up Done.
Sign in to reply to this message.
LGTM with a couple of remarks below. https://codereview.appspot.com/6877054/diff/9001/nova/nova.go File nova/nova.go (right): https://codereview.appspot.com/6877054/diff/9001/nova/nova.go#newcode63 nova/nova.go:63: Swap *string // Can be empty (nil) On 2012/12/10 12:46:09, dimitern wrote: > On 2012/12/10 03:33:39, dfc wrote: > > What does swap refer to ? swap device ? There could be more than one ? Swap > > amount ? Should this be an int ? > It seems so yes, although openstack does not always report that one the machines > and this is frequently missing. No idea what happens when more than one swap > device is present. > Anyway, because this is confusing and unhelpful, I'm removing this until we need > it. i'd still like to see some comments on these fields, or a URL where the information on each field can be found. for instance, is Disk the number of disks? the amount of disk space? the kind of disk available? is RAM measured in MB, GB, KB? https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... File testservices/novaservice/service.go (right): https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service.go:291: _, err := n.GetSecurityGroup(groupId) On 2012/12/10 12:46:09, dimitern wrote: > On 2012/12/07 12:15:35, rog wrote: > > if you're going to test whether a security group exists by using > > GetSecurityGroup here, then perhaps you should be consistent and use it > > everywhere, instead of looking in n.groups directly. > > > > (BTW "GetSecurityGroup" would more usually be phrased as simply > "SecurityGroup" > > - the "Get" is usually omitted for accessors). > > Ok, I'll make this more consistent. But for now, I won't change all Get* methods > for now, so not to complicate the CL. I'll do it in a follow up along with > removing the interface. +1 https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... File testservices/novaservice/service_test.go (right): https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_test.go:181: nova.Entity{Id: "fl1", Links: []nova.Link{}}, On 2012/12/10 12:46:09, dimitern wrote: > On 2012/12/07 12:15:35, rog wrote: > > this looks slightly fishy to me. i don't think we should care if the returned > > links are nil or not nil. i wouldn't mind testing against nil though, if that > > made things easier, as that's the usual zero value. (in that case you could > just > > delete the Links initialisation) > > The idea is, AddFlavor/Server will create the links automatically if nil, and > subsequently DeepEquals will fail, unless I really put both links as expected, > which I think is unnecessary here - I test the links are created properly in > other test cases. > > Anyway, I changed that now with buildLinks bool passed explicitly as a second > argument to addServer/addFlavor. If true the logic for generating links will > kick in, otherwise leaving it nil and thus simplifying the logic in a few > places. i think that makes sense. it makes the logic clearer. another possibility is to build the links if len(Links) == 0 - is it ever reasonable to have a flavor with no links? https://codereview.appspot.com/6877054/diff/20001/testservices/novaservice/se... File testservices/novaservice/service_test.go (right): https://codereview.appspot.com/6877054/diff/20001/testservices/novaservice/se... testservices/novaservice/service_test.go:476: c.Assert(*(ru.FromPort), Equals, *rule.FromPort) not sure why you've added the brackets. without seems fine to me.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6877054/diff/9001/nova/nova.go File nova/nova.go (right): https://codereview.appspot.com/6877054/diff/9001/nova/nova.go#newcode63 nova/nova.go:63: Swap *string // Can be empty (nil) On 2012/12/10 13:02:51, rog wrote: > On 2012/12/10 12:46:09, dimitern wrote: > > On 2012/12/10 03:33:39, dfc wrote: > > > What does swap refer to ? swap device ? There could be more than one ? Swap > > > amount ? Should this be an int ? > > It seems so yes, although openstack does not always report that one the > machines > > and this is frequently missing. No idea what happens when more than one swap > > device is present. > > Anyway, because this is confusing and unhelpful, I'm removing this until we > need > > it. > > i'd still like to see some comments on these fields, or a URL where the > information on each field can be found. for instance, is Disk the number of > disks? the amount of disk space? the kind of disk available? is RAM measured in > MB, GB, KB? Not very clear, but looking at nova source I could assume RAM is in MB, while the disk is the size of the root fs in GB. Added comments. https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... File testservices/novaservice/service_test.go (right): https://codereview.appspot.com/6877054/diff/9001/testservices/novaservice/ser... testservices/novaservice/service_test.go:181: nova.Entity{Id: "fl1", Links: []nova.Link{}}, On 2012/12/10 13:02:51, rog wrote: > On 2012/12/10 12:46:09, dimitern wrote: > > On 2012/12/07 12:15:35, rog wrote: > > > this looks slightly fishy to me. i don't think we should care if the > returned > > > links are nil or not nil. i wouldn't mind testing against nil though, if > that > > > made things easier, as that's the usual zero value. (in that case you could > > just > > > delete the Links initialisation) > > > > The idea is, AddFlavor/Server will create the links automatically if nil, and > > subsequently DeepEquals will fail, unless I really put both links as expected, > > which I think is unnecessary here - I test the links are created properly in > > other test cases. > > > > Anyway, I changed that now with buildLinks bool passed explicitly as a second > > argument to addServer/addFlavor. If true the logic for generating links will > > kick in, otherwise leaving it nil and thus simplifying the logic in a few > > places. > > i think that makes sense. it makes the logic clearer. another possibility is to > build the links if len(Links) == 0 - is > it ever reasonable to have a flavor with no links? Did as discussed on IRC - adding buildServerLinks and buildFlavorLinks, simplifying addServer/addFlavor. https://codereview.appspot.com/6877054/diff/20001/testservices/novaservice/se... File testservices/novaservice/service_test.go (right): https://codereview.appspot.com/6877054/diff/20001/testservices/novaservice/se... testservices/novaservice/service_test.go:476: c.Assert(*(ru.FromPort), Equals, *rule.FromPort) On 2012/12/10 13:02:51, rog wrote: > not sure why you've added the brackets. without seems fine to me. Done.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
*** Submitted: First part of the nova service double. As discussed, I'm starting to propose bits of the nova service double in chunks, as I go. The direct API and its tests are ready. Added a comprehensive comment about RuleInfo struct, to describe better the types of security group rules. R=wallyworld, jameinel, rog, dfc CC= https://codereview.appspot.com/6877054
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
