environs/ec2: handle 409 errors from PutBucket

environs/ec2: handle 409 errors from PutBucket

PutBucket can return a 409/BucketAlreadyOwnedByYou if the bucket already exists. This does not appear to happen in the original s3 region, but will occur in the newer regions.

Filter out this error codition as it's not really an error.

Requires support from goamz/s3/s3test, https://code.launchpad.net/~dave-cheney/goamz/004-send-409-conflict

http://docs.amazonwebservices.com/AmazonS3/2006-03-01/API/ErrorResponses.html

https://code.launchpad.net/~dave-cheney/juju-core/036-environs-ec2-make-non-us-east-1-work-again/+merge/133006

(do not edit description out of merge proposal)

[dave_cheney.net, 2012-11-06 02:13:05]

Please take a look.

[rog, 2012-11-06 13:59:57]

LGTM with one possible consideration below.

https://codereview.appspot.com/6810086/diff/1/environs/ec2/storage.go
File environs/ec2/storage.go (right):

https://codereview.appspot.com/6810086/diff/1/environs/ec2/storage.go#newcode32
environs/ec2/storage.go:32: if err := s.bucket.PutBucket(s3.Private); err != nil
{
I'd prefer to define s3ErrCode similarly to ec2ErrCode and write something like
this:

if err := s.bucket.PutBucket(s3.Private); err != nil  && s3ErrCode(err) !=
"BucketAlreadyOwnedByYou" {

I'm not sure that checking the numeric status code is that important here, as
the string code is suitably descriptive.

[fwereade, 2012-11-06 18:30:17]

Change is fine, but it needs a test.

[dave_cheney.net, 2012-11-06 19:21:50]

What if I changed livetests to run against us-west-1, that should expose all the
stuff that us-east-1 hides from us?

On 07/11/2012, at 5:30, fwereade@gmail.com wrote:

> Change is fine, but it needs a test.
> 
> https://codereview.appspot.com/6810086/

[dave_cheney.net, 2012-11-07 05:40:10]

Please take a look.

[dave_cheney.net, 2012-11-07 09:01:57]

Please take a look.

[fwereade, 2012-11-09 07:00:48]

ISTM that this is just switching an integration test around, causing the most
popular region to become untested. I don't think that's a good direction.

Can we not extend the local s3 server to be able to return both possible
results, and test each of them in local_test.go?

https://codereview.appspot.com/6810086/diff/8001/environs/ec2/live_test.go
File environs/ec2/live_test.go (right):

https://codereview.appspot.com/6810086/diff/8001/environs/ec2/live_test.go#ne...
environs/ec2/live_test.go:27: // * region should not be us-east-1 to avoid
unusual us-east-1 compatbility behavior
But now us-east-1 is untested, right?

https://codereview.appspot.com/6810086/diff/8001/environs/ec2/live_test.go#ne...
environs/ec2/live_test.go:28: // * default-series should be precise until we
public mongodb for other series
s/public/publish/?

[rog, 2012-11-09 10:30:06]

On 2012/11/09 07:00:48, fwereade wrote:
> ISTM that this is just switching an integration test around, causing the most
> popular region to become untested. I don't think that's a good direction.

i agree.
i think we should be able to run the live tests against *all* the regions. or
perhaps, given that it takes a while, we could take the desired region from an
environment variable or command-line flag when running the live tests.

> Can we not extend the local s3 server to be able to return both possible
> results, and test each of them in local_test.go?

the issue of getting the test servers to produce different failure modes has
been punted until now, but this seems reasonable.
perhaps s3test.NewServer could take an "emulation mode" argument - each mode
would imply a set of properties that we've seen in one server.
perhaps the emulation mode could just be a region name.

[TheMue, 2012-11-09 11:39:25]

To ensure the compatibility with all regions an extra test started with an extra
option should cover them.

https://codereview.appspot.com/6810086/diff/8001/environs/ec2/live_test.go
File environs/ec2/live_test.go (right):

https://codereview.appspot.com/6810086/diff/8001/environs/ec2/live_test.go#ne...
environs/ec2/live_test.go:27: // * region should not be us-east-1 to avoid
unusual us-east-1 compatbility behavior
On 2012/11/09 07:00:48, fwereade wrote:
> But now us-east-1 is untested, right?

Where do we have a test of all regions?

[niemeyer, 2012-11-14 16:30:45]

NOT LGTM due to the switch of default region in live tests.

The test for this is better put by tweaking our own fake s3test to do what we
want.

https://codereview.appspot.com/6810086/diff/8001/environs/ec2/live_test.go
File environs/ec2/live_test.go (right):

https://codereview.appspot.com/6810086/diff/8001/environs/ec2/live_test.go#ne...
environs/ec2/live_test.go:27: // * region should not be us-east-1 to avoid
unusual us-east-1 compatbility behavior
On 2012/11/09 07:00:48, fwereade wrote:
> But now us-east-1 is untested, right?

Yes, and us-east-1 is actually a lot more weird than all the others for
historical reasons (things are *more* inconsistent there). Let's please keep the
default pointing there.

https://codereview.appspot.com/6810086/diff/8001/environs/ec2/live_test.go#ne...
environs/ec2/live_test.go:27: // * region should not be us-east-1 to avoid
unusual us-east-1 compatbility behavior
On 2012/11/09 11:39:25, TheMue wrote:
> On 2012/11/09 07:00:48, fwereade wrote:
> > But now us-east-1 is untested, right?
> 
> Where do we have a test of all regions?

We definitely don't want to be running these super-heavy tests for all regions.

[TheMue, 2012-11-14 16:34:43]

> > Where do we have a test of all regions?
> 
> We definitely don't want to be running these super-heavy tests for all
regions.

No, definitely don't want it here. Only thought about a kind of integration test
to ensure our compatibility with all region before a release.

[dave_cheney.net, 2012-12-10 05:40:27]

Please take a look.

[dave_cheney.net, 2012-12-10 05:44:24]

Please take a look.

[dave_cheney.net, 2012-12-10 05:55:24]

Please take a look.

[dave_cheney.net, 2012-12-11 00:11:48]

Please take a look.

[fwereade, 2012-12-11 09:10:22]

Wondering if I've missed something...

https://codereview.appspot.com/6810086/diff/24001/environs/ec2/local_test.go
File environs/ec2/local_test.go (right):

https://codereview.appspot.com/6810086/diff/24001/environs/ec2/local_test.go#...
environs/ec2/local_test.go:110: Send409Conflict: true,
I'm still a little bit sad that we don't appear to be testing both responses.
Can we perhaps duplicate the beginning of the actual test case such that we do
actually hit both code paths here?

[dave_cheney.net, 2012-12-11 09:21:35]

Can I parry that the logic is tested inside goamz/s3 itself. 

On 11/12/2012, at 20:10, cfwereade@gmail.com wrote:

> Wondering if I've missed something...
> 
> 
> https://codereview.appspot.com/6810086/diff/24001/environs/ec2/local_test.go
> File environs/ec2/local_test.go (right):
> 
>
https://codereview.appspot.com/6810086/diff/24001/environs/ec2/local_test.go#...
> environs/ec2/local_test.go:110: Send409Conflict: true,
> I'm still a little bit sad that we don't appear to be testing both
> responses. Can we perhaps duplicate the beginning of the actual test
> case such that we do actually hit both code paths here?
> 
> https://codereview.appspot.com/6810086/

[dave_cheney.net, 2012-12-11 12:25:54]

https://codereview.appspot.com/6810086/diff/24001/environs/ec2/local_test.go
File environs/ec2/local_test.go (right):

https://codereview.appspot.com/6810086/diff/24001/environs/ec2/local_test.go#...
environs/ec2/local_test.go:110: Send409Conflict: true,
On 2012/12/11 09:10:22, fwereade wrote:
> I'm still a little bit sad that we don't appear to be testing both responses.
> Can we perhaps duplicate the beginning of the actual test case such that we do
> actually hit both code paths here?

I am sorry about this as well. I hold it as an artifact that we cannot always
test against a live ec2 instance.

I cannot duplicate the tests, this has already been rejected in a prior CL.

[niemeyer, 2012-12-11 12:34:12]

On 2012/12/11 12:25:54, dfc wrote:
> https://codereview.appspot.com/6810086/diff/24001/environs/ec2/local_test.go
> File environs/ec2/local_test.go (right):
> 
>
https://codereview.appspot.com/6810086/diff/24001/environs/ec2/local_test.go#...
> environs/ec2/local_test.go:110: Send409Conflict: true,
> On 2012/12/11 09:10:22, fwereade wrote:
> > I'm still a little bit sad that we don't appear to be testing both
responses.
> > Can we perhaps duplicate the beginning of the actual test case such that we
do
> > actually hit both code paths here?
> 
> I am sorry about this as well. I hold it as an artifact that we cannot always
> test against a live ec2 instance.
> 
> I cannot duplicate the tests, this has already been rejected in a prior CL.

I think what Williams is suggesting is to have a specific test, such as
TestPutBucket409 that tests the specific situation, and leave the rest of the
tests running through the usual 200 (or vice-versa). This is the same kind of
problem we've discussed and agreed in past branches, and the same kind of
direction too.

[niemeyer, 2012-12-11 12:34:54]

On 2012/12/11 12:34:12, niemeyer wrote:
> I think what Williams is suggesting is to have a specific test, such as

s/Williams is/William's/

[fwereade, 2012-12-11 12:56:32]

On 2012/12/11 12:34:12, niemeyer wrote:
> I think what Williams is suggesting is to have a specific test, such as
> TestPutBucket409 that tests the specific situation, and leave the rest of the
> tests running through the usual 200 (or vice-versa). This is the same kind of
> problem we've discussed and agreed in past branches, and the same kind of
> direction too.

+1, sorry I wasn't clearer

[dave_cheney.net, 2012-12-13 05:06:14]

Please take a look.

[dave_cheney.net, 2012-12-13 18:52:06]

On 2012/12/13 05:06:14, dfc wrote:
> Please take a look.

PTAL.

[dave_cheney.net, 2012-12-15 01:43:27]

Please take a look.

[dave_cheney.net, 2012-12-16 00:06:35]

*** Submitted:

environs/ec2: handle 409 errors from PutBucket

PutBucket can return a 409/BucketAlreadyOwnedByYou if the bucket already exists.
This does not appear to happen in the original s3 region, but will occur in the
newer regions.

Filter out this error codition as it's not really an error.

Requires support from goamz/s3/s3test,
https://code.launchpad.net/~dave-cheney/goamz/004-send-409-conflict

http://docs.amazonwebservices.com/AmazonS3/2006-03-01/API/ErrorResponses.html

R=rog, fwereade, TheMue, niemeyer
CC=
https://codereview.appspot.com/6810086