Issue 11034044: Azure provider: Destroy() and StopInstances().
DescriptionAzure provider: Destroy() and StopInstances().
This branch implements environ's Destroy() and StopInstances() in the Azure provider. As always with the provider code, most of the non-obvious code is in the tests. Since the testing utilities provided by gwacl return a slice of the fake requests made, it's tempting to make an extensive usage of that information and basically duplicate the unit-tests for the called gwacl methods. Instead of doing that, I chose a middle ground and checked that the requests look right without going too deep; this should be sufficient to test that the provider methods have the expected behavior and get tests to fail if the gwacl methods change significantly.
https://code.launchpad.net/~rvb/juju-core/az-destroy/+merge/173657
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 22
MessagesTotal messages: 6
Please take a look.
Sign in to reply to this message.
I don't think you have to change this, but there are a couple bits that might make it easier to support. 'makeService' is a bit confusing because Juju has a 'service' concept. So clarifying what you are creating here would be nice. Just asserting that we call GET seems a bit weak, vs at least asserting what *type* of thing you are GETing. I do still prefer the double approach to the mocking-HTTP requests. Mostly because when you do change gwacl to fix things, it can easily break the juju-core test suite without the juju-core code itself actually being incorrect. (eg you discover that you know enough that you can just DELETE the services without needing to GET their details first.) If it isn't really possible to assert more about the types of URLs being accessed, then LGTM. https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go File environs/azure/environ_test.go (right): https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go... environs/azure/environ_test.go:351: // When destroying a hosted service, gwacl first issues a Get reques typo: request https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go... environs/azure/environ_test.go:390: responses := buildDestroyServiceResponses(c, services) How does this match up with the 1 instance == 1 service model that JTV has brought up might be changing? Given that Juju has its own notion of what a service is, maybe the helper could be "makeAzureService" to distinguish and avoid confusion? https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go... environs/azure/environ_test.go:403: c.Check((*requests)[3].Method, Equals, "DELETE") It feels like this could use slightly more assertion if we are going to do it this way. Specifically that we are GET and or DELETE a service (somehow the "service1" is in the content, or something). To handle the "I wanted to delete a service, but I actually just deleted an instance" or something along those lines. https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go... environs/azure/environ_test.go:456: c.Check(strings.HasSuffix(transport.Exchanges[2].Request.URL.String(), "blob-2"), IsTrue) Like this one is actually asserting that you are DELETEing something "blob-1" related. (Otherwise this test could have passed if you just DELETE services.) https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go... environs/azure/environ_test.go:496: c.Check((*requests), HasLen, 1+len(services)*2) So I think what I'd like to see is not testing of the formatting of the requests (that belongs in lp:gwacl), but slightly more sanity checking than just GET. So something that shows it is a GET of all instances, and then a GET + DELETE of a given service, etc.
Sign in to reply to this message.
LGTM, thoroughly tested, but as always a few notes still. https://codereview.appspot.com/11034044/diff/1/environs/azure/environ.go File environs/azure/environ.go (right): https://codereview.appspot.com/11034044/diff/1/environs/azure/environ.go#newc... environs/azure/environ.go:134: // Shortcut to exit quickly if 'instances' is an empty slice or nil. I'd prefer it if you didn't do this unless the comment could cite a very good reason, such as breakage or a specific, unacceptable performance overhead. Otherwise it's pointless cyclomatic complexity, which means reduced test coverage. https://codereview.appspot.com/11034044/diff/1/environs/azure/environ.go#newc... environs/azure/environ.go:144: // Shutdown all the instances; If there are errors, return only the Typo: "if" after semicolon, not "If"! Also, "shutdown" is the noun and "shut down" is the verb. (Same in the line below). https://codereview.appspot.com/11034044/diff/1/environs/azure/environ.go#newc... environs/azure/environ.go:247: logger.Debugf("environs/azure: destroying environment %q", env.name) No need to prefix the message with "environs/azure." That's built into the logger. https://codereview.appspot.com/11034044/diff/1/environs/azure/environ.go#newc... environs/azure/environ.go:261: // Stop all instances. You're trying to delete all blobs and *then* stopping the instances. Has Azure fixed the problems with deleting VHDs yet? Because otherwise I imagine you'd just get errors about VHD files being still in use by the VMs that have them mounted. https://codereview.appspot.com/11034044/diff/1/environs/azure/environ.go#newc... environs/azure/environ.go:271: // Add any instances we've been told about but haven't yet shown I think I recognize this code from the ec2/openstack providers... What an ugly piece of code it is. From a Python background, I would say chuck both sources of instances into the same set, then pass that set as an iterable. Unfortunately in Go, that comes out as: combinedInstances := make(map[string]bool) for _, inst := range env.AllInstances() { combinedInstances[inst] = true } for _, inst := range ensureInstances { combinedInstances[inst] = true } instances := make([]string, 0) for inst := range combinedInstances { instances = append(instances, inst) } More regular perhaps, but still an insane amount of work for what it is. https://codereview.appspot.com/11034044/diff/1/environs/azure/environ.go#newc... environs/azure/environ.go:281: if err != nil { Maybe just "return env.StopInstances(insts)"? I just can't take the mindless repetition of error propagation any more! https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go File environs/azure/environ_test.go (right): https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go... environs/azure/environ_test.go:9: "io/ioutil" By the way, there's a new convention for formatting our imports — but as far as I can tell, no cleanup drive. So we're expected to fix this ourselves as we go along, I guess. The convention is: still one import statement, but within its parentheses, standard-library packages come first; then a blank line; then other dependencies from outside the project; then another blank line; and finally juju-core dependencies. The formatter will sort within each section, but not across blank lines. https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go... environs/azure/environ_test.go:348: func buildDestroyServiceResponses(c *C, services []*gwacl.HostedService) []gwacl.DispatcherResponse { Can you document what this function is for? https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go... environs/azure/environ_test.go:351: // When destroying a hosted service, gwacl first issues a Get reques This makes it sound as if gwacl.DeleteHostedService is doing all that, when actually apart from the one mention of a GET request, you're describing what the provider code does. My first reaction was: "why isn't this knowledge kept inside gwacl then?" It would help to make a consistent choice of perspective(s). Frankly I'm not even sure it helps to know that that first request is a GET request — the response looks the same regardless of which method the request used. Maybe the best perspective is to explain in terms of gwacl calls. https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go... environs/azure/environ_test.go:354: // the service does not contain deployments to keep the testing simple) Don't do it the Go way! This limitation may be just an afterthought while you're implementing, and it's tempting to think that "the function does whatever it does." But remember how you felt when Go's standard http library failed with "no renegotiation"? Document the purpose and preconditions up front. https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go... environs/azure/environ_test.go:368: []byte(""), Just "nil" should do. https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go... environs/azure/environ_test.go:375: // return requests Leftover? Vestigial tail? Ghost of Revisions Past? https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go... environs/azure/environ_test.go:394: err := env.StopInstances(instances) It'd be nice to break up the test with a pair of blank lines around the part where you exercise the tested functionality, so you get neat setup, exercise, and verification portions. It's quite a wall of text otherwise. Personally I like to keep the check for nilness of the error return in the "exercise" portion, so that the portion doesn't just consist of the one line. But maybe that's just my petty bourgeois exceptions habit. https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go... environs/azure/environ_test.go:397: // - one GET request to fetch the infos about the service; I suppose it's not completely unacceptable, but "the infos" is a term I normally only hear from Germans who aren't quite fluent in English. It offends my sense of aesthetics somehow. It seems vague with a false pretense of specificity. I have no better reasons than that, but would you consider "the service's properties"? https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go... environs/azure/environ_test.go:406: // makeAzureStorage creates a test azureStorage object that will talk to a I think you can test this at a higher level, with less hassle and less code. Look for setDummyStorage; that will set up your azureEnviron with a fake storage. Then you can test directly for things like "my file gets deleted from storage" without having to muck about with requests and responses. Leave that stuff to the storage tests. https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go... environs/azure/environ_test.go:408: func makeAzureStorageMocking(transport http.RoundTripper, container string, account string) azureStorage { The documentation calls it makeAzureStorage, without the Mocking part. https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go... environs/azure/environ_test.go:417: var blobListWith2BlobsResponse = ` Maybe we can use gwacl's dedent for this stuff?
Sign in to reply to this message.
On 2013/07/09 09:05:34, jtv.canonical wrote: > LGTM, thoroughly tested, but as always a few notes still. Thanks for the review! > > https://codereview.appspot.com/11034044/diff/1/environs/azure/environ.go > File environs/azure/environ.go (right): > > https://codereview.appspot.com/11034044/diff/1/environs/azure/environ.go#newc... > environs/azure/environ.go:134: // Shortcut to exit quickly if 'instances' is an > empty slice or nil. > > I'd prefer it if you didn't do this unless the comment could cite a very good > reason, such as breakage or a specific, unacceptable performance overhead. > Otherwise it's pointless cyclomatic complexity, which means reduced test > coverage. Fixed. > https://codereview.appspot.com/11034044/diff/1/environs/azure/environ.go#newc... > environs/azure/environ.go:144: // Shutdown all the instances; If there are > errors, return only the > > Typo: "if" after semicolon, not "If"! > > Also, "shutdown" is the noun and "shut down" is the verb. (Same in the line > below). Fixed. > https://codereview.appspot.com/11034044/diff/1/environs/azure/environ.go#newc... > environs/azure/environ.go:247: logger.Debugf("environs/azure: destroying > environment %q", env.name) > > No need to prefix the message with "environs/azure." That's built into the > logger. Well spotted, fixed. > https://codereview.appspot.com/11034044/diff/1/environs/azure/environ.go#newc... > environs/azure/environ.go:261: // Stop all instances. > > You're trying to delete all blobs and *then* stopping the instances. Has Azure > fixed the problems with deleting VHDs yet? Because otherwise I imagine you'd > just get errors about VHD files being still in use by the VMs that have them > mounted. I think you're mixing up two different things here: the objects that are in the storage (the tools, the state-info file, etc) and the VHD. > https://codereview.appspot.com/11034044/diff/1/environs/azure/environ.go#newc... > environs/azure/environ.go:271: // Add any instances we've been told about but > haven't yet shown > > I think I recognize this code from the ec2/openstack providers... What an ugly > piece of code it is. It is ugly indeed, this should not be part of the provider's code but until this gets refactored, I'd prefer to keep that an exact copy of the code the other providers use so that this can be easily refactored later. > https://codereview.appspot.com/11034044/diff/1/environs/azure/environ.go#newc... > environs/azure/environ.go:281: if err != nil { > > Maybe just "return env.StopInstances(insts)"? I just can't take the mindless > repetition of error propagation any more! Done. > https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go > File environs/azure/environ_test.go (right): > > https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go... > environs/azure/environ_test.go:9: "io/ioutil" > > By the way, there's a new convention for formatting our imports — but as far as > I can tell, no cleanup drive. So we're expected to fix this ourselves as we go > along, I guess. > > The convention is: still one import statement, but within its parentheses, > standard-library packages come first; then a blank line; then other dependencies > from outside the project; then another blank line; and finally juju-core > dependencies. The formatter will sort within each section, but not across blank > lines. Ah ok, thanks for pointing that out. > https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go... > environs/azure/environ_test.go:348: func buildDestroyServiceResponses(c *C, > services []*gwacl.HostedService) []gwacl.DispatcherResponse { > > Can you document what this function is for? Done. > https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go... > environs/azure/environ_test.go:351: // When destroying a hosted service, gwacl > first issues a Get reques > > This makes it sound as if gwacl.DeleteHostedService is doing all that, when > actually apart from the one mention of a GET request, you're describing what the > provider code does. My first reaction was: "why isn't this knowledge kept > inside gwacl then?" No, that's what DestroyHostedService does. > > https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go... > environs/azure/environ_test.go:368: []byte(""), > > Just "nil" should do. That's right, fixed. > https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go... > environs/azure/environ_test.go:375: // return requests > > Leftover? Vestigial tail? Ghost of Revisions Past? Leftover :). Removed. > https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go... > environs/azure/environ_test.go:394: err := env.StopInstances(instances) > > It'd be nice to break up the test with a pair of blank lines around the part > where you exercise the tested functionality, so you get neat setup, exercise, > and verification portions. It's quite a wall of text otherwise. > > Personally I like to keep the check for nilness of the error return in the > "exercise" portion, so that the portion doesn't just consist of the one line. > But maybe that's just my petty bourgeois exceptions habit. Good suggestion, done. > https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go... > environs/azure/environ_test.go:397: // - one GET request to fetch the infos > about the service; > > I suppose it's not completely unacceptable, but "the infos" is a term I normally > only hear from Germans who aren't quite fluent in English. It offends my sense > of aesthetics somehow. It seems vague with a false pretense of specificity. I > have no better reasons than that, but would you consider "the service's > properties"? Done. > https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go... > environs/azure/environ_test.go:406: // makeAzureStorage creates a test > azureStorage object that will talk to a > > I think you can test this at a higher level, with less hassle and less code. > Look for setDummyStorage; that will set up your azureEnviron with a fake > storage. Then you can test directly for things like "my file gets deleted from > storage" without having to muck about with requests and responses. Leave that > stuff to the storage tests. I don't think that's possible because: a) I need to test the destroying the environment actually deletes all the files. b) The casting statement inside Destroy() prevents me from using setDummyStorage. > https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go... > environs/azure/environ_test.go:408: func makeAzureStorageMocking(transport > http.RoundTripper, container string, account string) azureStorage { > > The documentation calls it makeAzureStorage, without the Mocking part. Fixed. > https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go... > environs/azure/environ_test.go:417: var blobListWith2BlobsResponse = ` > > Maybe we can use gwacl's dedent for this stuff? I indented the XML strings better… I don't really see why I would use dedent, XML does not really care about white spaces between tags.
Sign in to reply to this message.
On 2013/07/09 10:12:41, rvb wrote: > https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go... > > environs/azure/environ_test.go:351: // When destroying a hosted service, gwacl > > first issues a Get reques > > > > This makes it sound as if gwacl.DeleteHostedService is doing all that, when > > actually apart from the one mention of a GET request, you're describing what > the > > provider code does. My first reaction was: "why isn't this knowledge kept > > inside gwacl then?" > > No, that's what DestroyHostedService does. In that case, it's painful to have this detailed knowledge of gwacl's implementation embedded here instead of in gwacl. Can you think of a useful way to move it? > > I think you can test this at a higher level, with less hassle and less code. > > Look for setDummyStorage; that will set up your azureEnviron with a fake > > storage. Then you can test directly for things like "my file gets deleted > from > > storage" without having to muck about with requests and responses. Leave that > > stuff to the storage tests. > > I don't think that's possible because: > a) I need to test the destroying the environment actually deletes all the files. > b) The casting statement inside Destroy() prevents me from using > setDummyStorage. It's a shame about b) because a) is not actually an obstacle — it's exactly the sort of thing I just described you could test more easily this way! This suggests to me that there's a DeleteAll method missing on StorageWriter... > I indented the XML strings better… I don't really see why I would use dedent, > XML does not really care about white spaces between tags. I assumed you did it the ugly way for some good reason that was going to become obvious. :-P
Sign in to reply to this message.
Thanks for the review! On 2013/07/09 08:00:44, jameinel wrote: > I don't think you have to change this, but there are a couple bits that might > make it easier to support. > > 'makeService' is a bit confusing because Juju has a 'service' concept. So > clarifying what you are creating here would be nice. Good point, I've renamed all the helpers (for instance s/makeService/makeAzureService). > Just asserting that we call GET seems a bit weak, vs at least asserting what > *type* of thing you are GETing. You're right, I've improved the tests to check that they target the right objects. > I do still prefer the double approach to the mocking-HTTP requests. Mostly > because when you do change gwacl to fix things, it can easily break the > juju-core test suite without the juju-core code itself actually being incorrect. > (eg you discover that you know enough that you can just DELETE the services > without needing to GET their details first.) This is a bit late for this and we deliberately choose not to do a test double in gwacl. This saves a lot of code in gwacl but the drawback is that some of gwacl's internal leak into the tests of third party applications, such as the juju provider, which use gwacl. Again, this is a tradeoff and the main goal is to allow us to move quickly. It maintenance is easier this way, once this is all stabilized (I'm talking about gwacl here and how we map Juju services onto gwacl objects), it might be good to move to a model where all the tests will be based on a test double. > > https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go... > environs/azure/environ_test.go:351: // When destroying a hosted service, gwacl > first issues a Get reques > typo: request Fixed. > https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go... > environs/azure/environ_test.go:390: responses := buildDestroyServiceResponses(c, > services) > How does this match up with the 1 instance == 1 service model that JTV has > brought up might be changing? This works under that assumption, you're right… I've added a comment in the code (the actual code for StopInstances, not the test code). > Given that Juju has its own notion of what a service is, maybe the helper could > be "makeAzureService" to distinguish and avoid confusion? Done. > https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go... > environs/azure/environ_test.go:403: c.Check((*requests)[3].Method, Equals, > "DELETE") > It feels like this could use slightly more assertion if we are going to do it > this way. > Specifically that we are GET and or DELETE a service (somehow the "service1" is > in the content, or something). To handle the "I wanted to delete a service, but > I actually just deleted an instance" or something along those lines. > > https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go... > environs/azure/environ_test.go:456: > c.Check(strings.HasSuffix(transport.Exchanges[2].Request.URL.String(), > "blob-2"), IsTrue) > Like this one is actually asserting that you are DELETEing something "blob-1" > related. (Otherwise this test could have passed if you just DELETE services.) > > https://codereview.appspot.com/11034044/diff/1/environs/azure/environ_test.go... > environs/azure/environ_test.go:496: c.Check((*requests), HasLen, > 1+len(services)*2) > So I think what I'd like to see is not testing of the formatting of the requests > (that belongs in lp:gwacl), but slightly more sanity checking than just GET. So > something that shows it is a GET of all instances, and then a GET + DELETE of a > given service, etc. Fixed.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
