environs/jujutest: use AttemptStrategy

environs/jujutest: use AttemptStrategy

Since AttemptStrategy is now defined externally,
it makes it easy to use between packages
(not the case when this code was written).

We also use the strategy in TestFile, which
should remove the most failure that I have
most observed when running live tests.

We also use it in environs/ec2 itself where appropriate.

https://code.launchpad.net/~rogpeppe/juju-core/132-fix-TestFile/+merge/129690

(do not edit description out of merge proposal)

[rog, 2012-10-15 15:26:16]

Please take a look.

[rog, 2012-10-15 15:53:59]

Please take a look.

[rog, 2012-10-15 16:20:23]

Please take a look.

[niemeyer, 2012-10-16 18:07:36]

https://codereview.appspot.com/6696043/diff/5006/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/6696043/diff/5006/environs/jujutest/livetests....
environs/jujutest/livetests.go:275: if err = t.Env.Bootstrap(false); err != nil
{
These changes don't look good. We should implement careful retrying where
appropriate within the environment implementation itself, and it should take
into account what the error really is before retrying. Bootstrapping the same
environments N times ignoring any and all errors is really bad. People will be
building new providers thinking it's fine to have things exploding all the time
arbitrarily, which is definitely not true.

The errors we see in live tests are real bugs, that must be fixed.

[rog, 2012-10-17 08:33:58]

https://codereview.appspot.com/6696043/diff/5006/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/6696043/diff/5006/environs/jujutest/livetests....
environs/jujutest/livetests.go:275: if err = t.Env.Bootstrap(false); err != nil
{
On 2012/10/16 18:07:36, niemeyer wrote:
> These changes don't look good. We should implement careful retrying where
> appropriate within the environment implementation itself, and it should take
> into account what the error really is before retrying.

I think we've been through this before. I agree that the environment should
retry when there's an error. However I'm not sure how it can know to retry when
there's *no* error, as above, where we're hoping that the method will return an
error, but eventual consistency means that sometimes it will not.

A similar thing is true of the other places that this CL touches - the call in
question is succeeding, but it's either succeeding when we expect an error, or
it's succeeding and returning stale data.

If you have a better solution than what I'm doing in this CL, I'd like to hear
it. Two (worse) alternatives that I can see are:
- delay every call by 5 seconds to let eventual consistency propagate.
- remove these tests.

[niemeyer, 2012-10-17 11:59:11]

https://codereview.appspot.com/6696043/diff/5006/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/6696043/diff/5006/environs/jujutest/livetests....
environs/jujutest/livetests.go:275: if err = t.Env.Bootstrap(false); err != nil
{
On 2012/10/17 08:33:58, rog wrote:
> I think we've been through this before. I agree that the environment should
> retry when there's an error. However I'm not sure how it can know to retry
when
> there's *no* error, as above, where we're hoping that the method will return
an
> error, but eventual consistency means that sometimes it will not.

I don't think I understand the problem then. You mean that Bootstrap succeeds
every time N times in a row for the same environment?  That surely doesn't look
right.

[rog, 2012-10-17 14:01:22]

Please take a look.

https://codereview.appspot.com/6696043/diff/5006/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/6696043/diff/5006/environs/jujutest/livetests....
environs/jujutest/livetests.go:275: if err = t.Env.Bootstrap(false); err != nil
{
On 2012/10/17 11:59:11, niemeyer wrote:
> On 2012/10/17 08:33:58, rog wrote:
> > I think we've been through this before. I agree that the environment should
> > retry when there's an error. However I'm not sure how it can know to retry
> when
> > there's *no* error, as above, where we're hoping that the method will return
> an
> > error, but eventual consistency means that sometimes it will not.
> 
> I don't think I understand the problem then. You mean that Bootstrap succeeds
> every time N times in a row for the same environment?  That surely doesn't
look
> right.

As discussed on line, we make Bootstrap wait for eventual consistency to resolve
before returning an error.

[niemeyer, 2012-10-17 22:27:19]

https://codereview.appspot.com/6696043/diff/19001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6696043/diff/19001/environs/ec2/ec2.go#newcode214
environs/ec2/ec2.go:214: // up yet, so we retry in case that's what's happening.
s/in case/to verify if/?

https://codereview.appspot.com/6696043/diff/19001/environs/ec2/export_test.go
File environs/ec2/export_test.go (right):

https://codereview.appspot.com/6696043/diff/19001/environs/ec2/export_test.go...
environs/ec2/export_test.go:99: var ShortAttempt = &shortAttempt
That feels like hooking into the implementation internals for very little
benefit.

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

https://codereview.appspot.com/6696043/diff/19001/environs/ec2/live_test.go#n...
environs/ec2/live_test.go:235: // Check that bucket exists, so we can be sure
s/buck/the buck/

https://codereview.appspot.com/6696043/diff/19001/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/6696043/diff/19001/environs/jujutest/livetests...
environs/jujutest/livetests.go:126: // Stopping may not be noticed at first due
to eventual
That actually doesn't seem true. In this case it's simply not an atomic
operation, and it's expected that the instance will take a while to stop.

https://codereview.appspot.com/6696043/diff/19001/environs/jujutest/livetests...
environs/jujutest/livetests.go:589: for a := t.Attempt.Start(); a.Next(); {
Did you get a failure out of this? I'd expect it to be somewhat reliable given
the above checks.

[niemeyer, 2012-10-17 22:27:33]

LGTM considering these.

[rog, 2012-10-18 10:34:43]

Please take a look.

https://codereview.appspot.com/6696043/diff/19001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6696043/diff/19001/environs/ec2/ec2.go#newcode214
environs/ec2/ec2.go:214: // up yet, so we retry in case that's what's happening.
On 2012/10/17 22:27:19, niemeyer wrote:
> s/in case/to verify if/?

Done.

https://codereview.appspot.com/6696043/diff/19001/environs/ec2/export_test.go
File environs/ec2/export_test.go (right):

https://codereview.appspot.com/6696043/diff/19001/environs/ec2/export_test.go...
environs/ec2/export_test.go:99: var ShortAttempt = &shortAttempt
On 2012/10/17 22:27:19, niemeyer wrote:
> That feels like hooking into the implementation internals for very little
> benefit.

It actually gives more value than one might think, because the live tests are
also used in the non-live context, and for the non-live tests, shortAttempt has
a much shorter time out (see ShortTimeouts above). So by using this value
directly, we speed up the tests. I hope it's OK to leave it as is.

An alternative would be to define shortAttempt and ShortTimeouts in the test
code too, so we've got two instances of it. If you think that's better, I'll do
that, but given that the testing code already has knowledge of shortAttempt, I
can't see that it would be worth it.

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

https://codereview.appspot.com/6696043/diff/19001/environs/ec2/live_test.go#n...
environs/ec2/live_test.go:235: // Check that bucket exists, so we can be sure
On 2012/10/17 22:27:19, niemeyer wrote:
> s/buck/the buck/

Done.

https://codereview.appspot.com/6696043/diff/19001/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/6696043/diff/19001/environs/jujutest/livetests...
environs/jujutest/livetests.go:126: // Stopping may not be noticed at first due
to eventual
On 2012/10/17 22:27:19, niemeyer wrote:
> That actually doesn't seem true. In this case it's simply not an atomic
> operation, and it's expected that the instance will take a while to stop.

I thought the instance went into shutting-down mode instantly, but it may well
be that it's waiting for
the machine to react. I've changed the comment.

https://codereview.appspot.com/6696043/diff/19001/environs/jujutest/livetests...
environs/jujutest/livetests.go:589: for a := t.Attempt.Start(); a.Next(); {
On 2012/10/17 22:27:19, niemeyer wrote:
> Did you get a failure out of this? I'd expect it to be somewhat reliable given
> the above checks.

I believe I have seen failures in this code, and that's why it's there (in
general I've only inserted eventual-consistency code where things have gone
wrong).

"Somewhat" reliable is right. Just because we've seen name with the given
contents does not mean we will see it when the files are listed.

I've just tried running this test (without the loop) repeatedly, and I see a
test failure once every 20 or so attempts. I'd prefer to leave it more robust,
if that's ok.

[niemeyer, 2012-10-18 19:15:14]

Still LGTM

https://codereview.appspot.com/6696043/diff/19001/environs/ec2/export_test.go
File environs/ec2/export_test.go (right):

https://codereview.appspot.com/6696043/diff/19001/environs/ec2/export_test.go...
environs/ec2/export_test.go:99: var ShortAttempt = &shortAttempt
On 2012/10/18 10:34:44, rog wrote:
> It actually gives more value than one might think, because the live tests are
> also used in the non-live context, and for the non-live tests, shortAttempt
has
> a much shorter time out (see ShortTimeouts above). So by using this value
> directly, we speed up the tests. I hope it's OK to leave it as is.

This benefit doesn't come from using internal details.

> An alternative would be to define shortAttempt and ShortTimeouts in the test
> code too, so we've got two instances of it. If you think that's better, I'll
do
> that, but given that the testing code already has knowledge of shortAttempt, I
> can't see that it would be worth it.

It doesn't use it. The line above is new.

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

https://codereview.appspot.com/6696043/diff/19001/environs/ec2/live_test.go#n...
environs/ec2/live_test.go:57: Attempt:        *ec2.ShortAttempt,
Attempt: whateverWeWant

[rog, 2012-10-19 12:40:30]

*** Submitted:

environs/jujutest: use AttemptStrategy

Since AttemptStrategy is now defined externally,
it makes it easy to use between packages
(not the case when this code was written).

We also use the strategy in TestFile, which
should remove the most failure that I have
most observed when running live tests.

We also use it in environs/ec2 itself where appropriate.

R=niemeyer
CC=
https://codereview.appspot.com/6696043

https://codereview.appspot.com/6696043/diff/19001/environs/ec2/export_test.go
File environs/ec2/export_test.go (right):

https://codereview.appspot.com/6696043/diff/19001/environs/ec2/export_test.go...
environs/ec2/export_test.go:99: var ShortAttempt = &shortAttempt
On 2012/10/18 19:15:14, niemeyer wrote:
> On 2012/10/18 10:34:44, rog wrote:
> > It actually gives more value than one might think, because the live tests
are
> > also used in the non-live context, and for the non-live tests, shortAttempt
> has
> > a much shorter time out (see ShortTimeouts above). So by using this value
> > directly, we speed up the tests. I hope it's OK to leave it as is.
> 
> This benefit doesn't come from using internal details.
> 
> > An alternative would be to define shortAttempt and ShortTimeouts in the test
> > code too, so we've got two instances of it. If you think that's better, I'll
> do
> > that, but given that the testing code already has knowledge of shortAttempt,
I
> > can't see that it would be worth it.
> 
> It doesn't use it. The line above is new.

but the ShortTimeouts code is not new - that's the testing code I was referring
to that uses shortAttempt.

i'll submit as is and we can discuss what the right approach is later (i'm not
sure what your reaction was to my suggestion to duplicate shortAttempt and
ShortTimeouts).