Azure provider: delete Services concurrently

Azure provider: delete Services concurrently

When destroying instances, delete the Windows Azure concurrently.  It takes 2 minutes to take down a Service so will save lots of time when destroying big environments.

The actual concurrency is currently disabled because of a bug in Azure (that we found during testing this branch): deleting Services concurrently sometimes causes a —badly reported— conflict error.  We think this should be fixed in Azure and have reported the problem.  In the meantime I think it's better to land that code than to let it rot.

I deliberately didn't try to recover panics that could happen in the spawn goroutines because I think that if this happens, the whole app should be taken down (similar to what would happen now if one call to DestroyHostedService were to panic).

I'm interested to know what people more versed than me in Go concurrency patterns will have to say about this branch.

https://code.launchpad.net/~rvb/juju-core/concurrent-deletion/+merge/178091

(do not edit description out of merge proposal)

[rvb, 2013-08-01 15:07:22]

Please take a look.

[rvb, 2013-08-01 15:42:43]

Please take a look.

[dimitern, 2013-08-07 13:37:03]

LGTM with a few suggestions

https://codereview.appspot.com/12251044/diff/3001/environs/azure/environ.go
File environs/azure/environ.go (right):

https://codereview.appspot.com/12251044/diff/3001/environs/azure/environ.go#n...
environs/azure/environ.go:11: "launchpad.net/juju-core/agent/tools"

Blank line after gwacl please.

https://codereview.appspot.com/12251044/diff/3001/environs/azure/environ_test.go
File environs/azure/environ_test.go (right):

https://codereview.appspot.com/12251044/diff/3001/environs/azure/environ_test...
environs/azure/environ_test.go:19: "launchpad.net/juju-core/environs"

Here as well.

https://codereview.appspot.com/12251044/diff/3001/environs/azure/environ_test...
environs/azure/environ_test.go:726: c.Error(fmt.Sprintf("None of the requests
matches: Method=%v, URL pattern=%v", method, urlPattern))

s/None/none/
Error messages do not start with an uppercase letter.

[rvb, 2013-08-07 13:59:31]

Thanks for the review!

> LGTM with a few suggestions
>
https://codereview.appspot.com/12251044/diff/3001/environs/azure/environ.go#n...
> environs/azure/environ.go:11: "launchpad.net/juju-core/agent/tools"
> 
> Blank line after gwacl please.

Done.

>
https://codereview.appspot.com/12251044/diff/3001/environs/azure/environ_test...
> environs/azure/environ_test.go:19: "launchpad.net/juju-core/environs"
> 
> Here as well.

Done.

>
https://codereview.appspot.com/12251044/diff/3001/environs/azure/environ_test...
> environs/azure/environ_test.go:726: c.Error(fmt.Sprintf("None of the requests
> matches: Method=%v, URL pattern=%v", method, urlPattern))
> 
> s/None/none/
> Error messages do not start with an uppercase letter.

I thought that rule was to allow error messages to be concatenated as they
bubble up the call stack… and here, it's only a test failure message, not a real
error message.  Why do you think the message should start with an uppercase
letter in this case?

[rvb, 2013-08-07 14:01:28]

Please take a look.

[dimitern, 2013-08-07 14:15:00]

>
https://codereview.appspot.com/12251044/diff/3001/environs/azure/environ_test...
> environs/azure/environ_test.go:726: c.Error(fmt.Sprintf("None of the requests
> matches: Method=%v, URL pattern=%v", method, urlPattern))
> 
> s/None/none/
> Error messages do not start with an uppercase letter.

I thought that rule was to allow error messages to be concatenated as they
bubble up the call stack… and here, it's only a test failure message, not a real
error message.  Why do you think the message should start with an uppercase
letter in this case?

I'm not saying the message should start with an uppercase letter, I'm saying the
opposite :)

https://codereview.appspot.com/12251044/diff/11001/environs/azure/environ.go
File environs/azure/environ.go (right):

https://codereview.appspot.com/12251044/diff/11001/environs/azure/environ.go#...
environs/azure/environ.go:565: // TODO: Ought to have this only for state
servers.
// TODO(rvba) ..

https://codereview.appspot.com/12251044/diff/11001/environs/azure/environ.go#...
environs/azure/environ.go:572: // TODO: Ought to have this only for API servers.
Here as well.

[rvb, 2013-08-07 14:24:49]

On 2013/08/07 14:15:00, dimitern wrote:
> >
>
https://codereview.appspot.com/12251044/diff/3001/environs/azure/environ_test...
> > environs/azure/environ_test.go:726: c.Error(fmt.Sprintf("None of the
requests
> > matches: Method=%v, URL pattern=%v", method, urlPattern))
> > 
> > s/None/none/
> > Error messages do not start with an uppercase letter.
> 
> I thought that rule was to allow error messages to be concatenated as they
> bubble up the call stack… and here, it's only a test failure message, not a
real
> error message.  Why do you think the message should start with an uppercase
> letter in this case?
> 
> I'm not saying the message should start with an uppercase letter, I'm saying
the
> opposite :)

Right, sorry, I got mixed up… but why?

[rvb, 2013-08-07 15:20:21]

> Right, sorry, I got mixed up… but why?

All right, thanks for explaining the reason why on IRC.  The main reason to do
this is consistency with the existing code base as the messages don't get
chained like error messages.

[rvb, 2013-08-07 15:22:50]

Please take a look.

[jtvjtv, 2013-08-08 07:57:51]

LGTM as far as the actual code is concerned, although as you know I see no point
in having the extra goroutine for generating the instance IDs; I'd just give the
channel enough buffer and do that part inline.

One thing that more urgently needs work IMO is the testing.  Concurrency is a
liability, so make sure it's tested thoroughly:
 - How does the code deal with an error halfway through?
 - Is the zero-input case covered?
 - Do you know (both now and as others mess with the code later) that
concurrency really is limited to the given number?
 - Do you know that a saturated worker pool behaves in the way you want?

Testing this stuff is a lot of work, but it can be a stitch in time.

https://codereview.appspot.com/12251044/diff/20001/environs/azure/environ.go
File environs/azure/environ.go (right):

https://codereview.appspot.com/12251044/diff/20001/environs/azure/environ.go#...
environs/azure/environ.go:674: go func() (err error) {

The return value is pointless -- it doesn't go anywhere!

https://codereview.appspot.com/12251044/diff/20001/environs/azure/environ.go#...
environs/azure/environ.go:683: return nil

(And so this is pointless as well)

[rvb, 2013-08-08 09:30:23]

On 2013/08/08 07:57:51, jtvjtv wrote:
> LGTM as far as the actual code is concerned, although as you know I see no
point
> in having the extra goroutine for generating the instance IDs; I'd just give
the
> channel enough buffer and do that part inline.
> 
> One thing that more urgently needs work IMO is the testing.  Concurrency is a
> liability, so make sure it's tested thoroughly:
>  - How does the code deal with an error halfway through?
>  - Is the zero-input case covered?
>  - Do you know (both now and as others mess with the code later) that
> concurrency really is limited to the given number?
>  - Do you know that a saturated worker pool behaves in the way you want?
> 
> Testing this stuff is a lot of work, but it can be a stitch in time.

You're definitely right, I'll add more tests for this.

> https://codereview.appspot.com/12251044/diff/20001/environs/azure/environ.go
> File environs/azure/environ.go (right):
> 
>
https://codereview.appspot.com/12251044/diff/20001/environs/azure/environ.go#...
> environs/azure/environ.go:674: go func() (err error) {
> 
> The return value is pointless -- it doesn't go anywhere!

Good point, fixed.

>
https://codereview.appspot.com/12251044/diff/20001/environs/azure/environ.go#...
> environs/azure/environ.go:683: return nil
> 
> (And so this is pointless as well)

True, fixed.

[rog, 2013-08-08 12:24:10]

Looks reasonable, but how about the suggestion below,
which makes it easy for other code to do the same
thing?

https://codereview.appspot.com/12251044/diff/20001/environs/azure/environ.go
File environs/azure/environ.go (right):

https://codereview.appspot.com/12251044/diff/20001/environs/azure/environ.go#...
environs/azure/environ.go:672: errc := make(chan error, len(instances))

Just a thought - at some point in the past I wanted
to do something similar to this inside the ec2 provider.
I wrote some helper code to do it (although I ended up not
using it) which looks like it would be a good fit here.

The package is currently at code.google.com/p/rog-go/parallel;
how about we suck it into utils/parallel and use it here.

The code would look roughly like this:

run := parallel.NewRun(maxConcurrentDeletes)
for _, instance := range instances {
    instance := instance
    run.Do(func() error {
         request := &gwacl.DestroyHostedServiceRequest{ServiceName: serviceName}
         return context.DestroyHostedService(request)
    })
}
return run.Wait()

[rvb, 2013-08-08 13:43:00]

On 2013/08/08 12:24:10, rog wrote:
> Looks reasonable, but how about the suggestion below,
> which makes it easy for other code to do the same
> thing?
> 
> https://codereview.appspot.com/12251044/diff/20001/environs/azure/environ.go
> File environs/azure/environ.go (right):
> 
>
https://codereview.appspot.com/12251044/diff/20001/environs/azure/environ.go#...
> environs/azure/environ.go:672: errc := make(chan error, len(instances))
> 
> Just a thought - at some point in the past I wanted
> to do something similar to this inside the ec2 provider.
> I wrote some helper code to do it (although I ended up not
> using it) which looks like it would be a good fit here.
> 
> The package is currently at code.google.com/p/rog-go/parallel;
> how about we suck it into utils/parallel and use it here.
> 
> The code would look roughly like this:
> 
> run := parallel.NewRun(maxConcurrentDeletes)
> for _, instance := range instances {
>     instance := instance
>     run.Do(func() error {
>          request := &gwacl.DestroyHostedServiceRequest{ServiceName:
serviceName}
>          return context.DestroyHostedService(request)
>     })
> }
> return run.Wait()

That sounds like an excellent suggestion.  Such a generic tool would be very
useful, especially for providers.  Let me look into it first and then I'll see
about doing that in a follow-up branch.