Merge lp://staging/~wallyworld/goose/rate-limit-retry-tests into lp://staging/~gophers/goose/trunk

Reviewers: mp+144849_code.launchpad.net,

Message:
Please take a look.

Description:
Rewrite rate limit retry tests

This branch uses the new service double control hooks introduced in the
previous branch to rewrite the tests which check that rate limit
exceeded retries are handled properly.
The kludge used previously to induce a retry error has been removed, and
now an additional test can also be easily added to check the behaviour
if too many rate limit retry responses are received.

So that the tests run fast, I've allowed for the Retry-After header
value to be a float (even though a real instance only assigns an int).
This means the tests can specify a really
short retry time (I used 1ms).

I've also re-added a rate limit retry test for use with the live
instance, but improved it so that it exists as soon as the first rate
limit exceeded response is received.

https://code.launchpad.net/~wallyworld/goose/rate-limit-retry-tests/+merge/144849

Requires:
https://code.launchpad.net/~wallyworld/goose/control-test-doubles/+merge/144838

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/7200049/

Affected files:
   A [revision details]
   M http/client.go
   M identity/userpass.go
   M nova/live_test.go
   M nova/local_test.go
   M testservices/novaservice/service.go
   M testservices/novaservice/service_http.go
   M testservices/service.go

Overall LGTM.

https://codereview.appspot.com/7200049/diff/1/http/client.go
File http/client.go (right):

https://codereview.appspot.com/7200049/diff/1/http/client.go#newcode195
http/client.go:195: for i := 0; i <= c.maxRetries; i++ {
<= ? Doesn't that mean we will try 4 times instead of 3?

https://codereview.appspot.com/7200049/diff/1/identity/userpass.go
File identity/userpass.go (right):

https://codereview.appspot.com/7200049/diff/1/identity/userpass.go#newcode109
identity/userpass.go:109: }
I think I have a more complete fix for this:
https://code.launchpad.net/~jameinel/goose/handle-missing-endpoint/+merge/145098

https://codereview.appspot.com/7200049/diff/1/nova/live_test.go
File nova/live_test.go (right):

https://codereview.appspot.com/7200049/diff/1/nova/live_test.go#newcode452
nova/live_test.go:452: return
Is it possible to use your hooks to check for a retry request, rather
than reading the output string? (I realize this probably didn't exist
before, but it seems like a really good use of it.)

https://codereview.appspot.com/7200049/diff/1/testservices/novaservice/service_http.go
File testservices/novaservice/service_http.go (right):

https://codereview.appspot.com/7200049/diff/1/testservices/novaservice/service_http.go#newcode216
testservices/novaservice/service_http.go:216: // Retry-After is usually
an int but we don't want the tests taking too long to run.
Maybe: RFC says that Retry-After should be an int, but we don't want to
wait an entire second during the test suite.

https://codereview.appspot.com/7200049/

Please take a look.

https://codereview.appspot.com/7200049/diff/1/http/client.go
File http/client.go (right):

https://codereview.appspot.com/7200049/diff/1/http/client.go#newcode195
http/client.go:195: for i := 0; i <= c.maxRetries; i++ {
On 2013/01/28 06:44:57, jameinel wrote:
> <= ? Doesn't that mean we will try 4 times instead of 3?

The logic was confusing retries with total attempts. Using "retries"
terminology, the client sends the first request and retries X times.
With "sendAttempts" terminology, the client tries "sendAttempts"
requests in total.

I've change the variable and constant to be maxSendAttempts to remove
the ambiguity.

https://codereview.appspot.com/7200049/diff/1/identity/userpass.go
File identity/userpass.go (right):

https://codereview.appspot.com/7200049/diff/1/identity/userpass.go#newcode109
identity/userpass.go:109: }
On 2013/01/28 06:44:57, jameinel wrote:
> I think I have a more complete fix for this:

https://code.launchpad.net/%7Ejameinel/goose/handle-missing-endpoint/+merge/145098

I had a quick look, the mp was missing the pre-req branch so was very
large. Looked ok at first glance. Perhaps we can land this branch now as
is and follow up with yours?

https://codereview.appspot.com/7200049/diff/1/nova/live_test.go
File nova/live_test.go (right):

https://codereview.appspot.com/7200049/diff/1/nova/live_test.go#newcode452
nova/live_test.go:452: return
On 2013/01/28 06:44:57, jameinel wrote:
> Is it possible to use your hooks to check for a retry request, rather
than
> reading the output string? (I realize this probably didn't exist
before, but it
> seems like a really good use of it.)

Possibly, but the hook stuff has been set up to work with the test
doubles (manipulates the state of the test service back end) rather than
a real live instance. The nova client implementation would need to be
retooled to implement the ServiceControl interface. Perhaps this can be
done in a followup branch.

https://codereview.appspot.com/7200049/diff/1/testservices/novaservice/service_http.go
File testservices/novaservice/service_http.go (right):

https://codereview.appspot.com/7200049/diff/1/testservices/novaservice/service_http.go#newcode216
testservices/novaservice/service_http.go:216: // Retry-After is usually
an int but we don't want the tests taking too long to run.
On 2013/01/28 06:44:57, jameinel wrote:
> Maybe: RFC says that Retry-After should be an int, but we don't want
to wait an
> entire second during the test suite.

Done.

https://codereview.appspot.com/7200049/

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

...
> https://code.launchpad.net/%7Ejameinel/goose/handle-missing-endpoint/+merge/145098
>
> I had a quick look, the mp was missing the pre-req branch so was
> very large. Looked ok at first glance. Perhaps we can land this
> branch now as is and follow up with yours?

Yeah, goose-bot's trunk wasn't up to date with lp:goose, and which I
based my patch on. I tried to re-push so this should be cleaned up.

As for your patch, I think it is an accurate fix, but my concern is
just a more general thing. When we encounter edge cases the code
wasn't covering (a service that is only exported in one region), we
really need a test case for it, because those are the sorts of things
we are going to forget about in 6 months when we go tweaking things
for HP vs Canonistack vs Rackspace, etc.

>
> https://codereview.appspot.com/7200049/diff/1/nova/live_test.go
> File nova/live_test.go (right):
>
> https://codereview.appspot.com/7200049/diff/1/nova/live_test.go#newcode452
>
>
nova/live_test.go:452: return
> On 2013/01/28 06:44:57, jameinel wrote:
>> Is it possible to use your hooks to check for a retry request,
>> rather
> than
>> reading the output string? (I realize this probably didn't exist
> before, but it
>> seems like a really good use of it.)
>
> Possibly, but the hook stuff has been set up to work with the test
> doubles (manipulates the state of the test service back end) rather
> than a real live instance. The nova client implementation would
> need to be retooled to implement the ServiceControl interface.
> Perhaps this can be done in a followup branch.

I guess it is hooking the client-side stuff, vs hooking the service
side stuff.

>
> https://codereview.appspot.com/7200049/diff/1/testservices/novaservice/service_http.go
>
>
File testservices/novaservice/service_http.go (right):
>
> https://codereview.appspot.com/7200049/diff/1/testservices/novaservice/service_http.go#newcode216
>
>
testservices/novaservice/service_http.go:216: // Retry-After is usually
> an int but we don't want the tests taking too long to run. On
> 2013/01/28 06:44:57, jameinel wrote:
>> Maybe: RFC says that Retry-After should be an int, but we don't
>> want
> to wait an
>> entire second during the test suite.
>
> Done.
>
> https://codereview.appspot.com/7200049/
>

LGTM.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (Cygwin)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlEHUQ0ACgkQJdeBCYSNAAMvGQCfRQtVLUas22F84lPzovlpzzZK
p0IAoItwzHDxMx7CSrVW7Q6X7sYFrNfA
=dice
-----END PGP SIGNATURE-----

LGTM

https://codereview.appspot.com/7200049/

*** Submitted:

Rewrite rate limit retry tests

This branch uses the new service double control hooks introduced in the
previous branch to rewrite the tests which check that rate limit
exceeded retries are handled properly.
The kludge used previously to induce a retry error has been removed, and
now an additional test can also be easily added to check the behaviour
if too many rate limit retry responses are received.

So that the tests run fast, I've allowed for the Retry-After header
value to be a float (even though a real instance only assigns an int).
This means the tests can specify a really
short retry time (I used 1ms).

I've also re-added a rate limit retry test for use with the live
instance, but improved it so that it exists as soon as the first rate
limit exceeded response is received.

R=jameinel
CC=
https://codereview.appspot.com/7200049

https://codereview.appspot.com/7200049/