Add simplestreams tools retry logic

Add simplestreams tools retry logic

If tools are uploaded, the then tools fetching should
retry if the tools are not found in case there are
eventual consistencu issues. Otherwise, there does not
need to be any retries as it just slows things down.

https://code.launchpad.net/~wallyworld/juju-core/simplestreams-tools-retries/+merge/186255

Requires: https://code.launchpad.net/~wallyworld/juju-core/reduce-timeouts/+merge/185988

(do not edit description out of merge proposal)

[wallyworld, 2013-09-18 07:42:22]

Please take a look.

[wallyworld, 2013-09-19 00:36:19]

Please take a look.

[thumper, 2013-09-19 01:52:59]

https://codereview.appspot.com/13577045/diff/4001/cmd/juju/bootstrap_test.go
File cmd/juju/bootstrap_test.go (right):

https://codereview.appspot.com/13577045/diff/4001/cmd/juju/bootstrap_test.go#...
cmd/juju/bootstrap_test.go:13: "fmt"
this should be moved up to the standard library block

https://codereview.appspot.com/13577045/diff/4001/cmd/juju/bootstrap_test.go#...
cmd/juju/bootstrap_test.go:54: allowRetryValues []bool
How about "expectedAllowRetry", I took this part of the test struct as an input
param, not a thing to be tested against.

https://codereview.appspot.com/13577045/diff/4001/cmd/juju/bootstrap_test.go#...
cmd/juju/bootstrap_test.go:72: fmt.Printf("test %d: %s\n", i, test.info)
icky icky, don't do this.

use c.Logf, otherwise you are making noisy tests

https://codereview.appspot.com/13577045/diff/4001/environs/tools/tools.go
File environs/tools/tools.go (right):

https://codereview.appspot.com/13577045/diff/4001/environs/tools/tools.go#new...
environs/tools/tools.go:116: filter coretools.Filter, allowRetry bool) (list
coretools.List, err error) {
I have a strong dislike for bool parameters.  They don't tell the reader at the
call-site anything. I'd prefer well named constants.

[wallyworld, 2013-09-19 02:25:49]

Please take a look.

https://codereview.appspot.com/13577045/diff/4001/cmd/juju/bootstrap_test.go
File cmd/juju/bootstrap_test.go (right):

https://codereview.appspot.com/13577045/diff/4001/cmd/juju/bootstrap_test.go#...
cmd/juju/bootstrap_test.go:54: allowRetryValues []bool
On 2013/09/19 01:52:59, thumper wrote:
> How about "expectedAllowRetry", I took this part of the test struct as an
input
> param, not a thing to be tested against.

Done.

https://codereview.appspot.com/13577045/diff/4001/cmd/juju/bootstrap_test.go#...
cmd/juju/bootstrap_test.go:72: fmt.Printf("test %d: %s\n", i, test.info)
On 2013/09/19 01:52:59, thumper wrote:
> icky icky, don't do this.
> 
> use c.Logf, otherwise you are making noisy tests

Done.

https://codereview.appspot.com/13577045/diff/4001/environs/tools/tools.go
File environs/tools/tools.go (right):

https://codereview.appspot.com/13577045/diff/4001/environs/tools/tools.go#new...
environs/tools/tools.go:116: filter coretools.Filter, allowRetry bool) (list
coretools.List, err error) {
On 2013/09/19 01:52:59, thumper wrote:
> I have a strong dislike for bool parameters.  They don't tell the reader at
the
> call-site anything. I'd prefer well named constants.

I used a type alias

[fwereade, 2013-09-19 12:59:38]

I'm a little queasy about the additional bool, but the bulk of this looks solid.
Couple of points that might bear discussion:

https://codereview.appspot.com/13577045/diff/11001/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/13577045/diff/11001/cmd/juju/bootstrap.go#newc...
cmd/juju/bootstrap.go:131: 
should this be the named constant?

https://codereview.appspot.com/13577045/diff/11001/cmd/juju/bootstrap_test.go
File cmd/juju/bootstrap_test.go (right):

https://codereview.appspot.com/13577045/diff/11001/cmd/juju/bootstrap_test.go...
cmd/juju/bootstrap_test.go:66: 
These can be smooshed up for a bit less indentation and slightly better
readability (IMO)

... []test{{

},{

}}

https://codereview.appspot.com/13577045/diff/11001/environs/bootstrap/bootstr...
File environs/bootstrap/bootstrap.go (right):

https://codereview.appspot.com/13577045/diff/11001/environs/bootstrap/bootstr...
environs/bootstrap/bootstrap.go:62: 
ditto named constant

https://codereview.appspot.com/13577045/diff/11001/environs/manual/provisione...
File environs/manual/provisioner_test.go (right):

https://codereview.appspot.com/13577045/diff/11001/environs/manual/provisione...
environs/manual/provisioner_test.go:53: 
this param list is actually getting a touch alarming

https://codereview.appspot.com/13577045/diff/11001/environs/tools/tools.go
File environs/tools/tools.go (right):

https://codereview.appspot.com/13577045/diff/11001/environs/tools/tools.go#ne...
environs/tools/tools.go:110: 
Ah, I guess we don't have names? I think we should really, naming the type alone
is a pretty flimsy figleaf.

But I'm starting to feel pretty strongly that we're in params-struct territory
here anyway, in which case I wouldn't mind a plain bool.

https://codereview.appspot.com/13577045/diff/11001/environs/tools/tools.go#ne...
environs/tools/tools.go:166: 
Is this only for testing this package? If so, please export_test it. If not, is
it possible to arrange whatever other package that needs it so that we can patch
out FBT at the point of use? As it is I feel it's exposing things it really
shouldn't.

(I'm not sure why patching out a func in a module makes me feel dirtier than
patching out a var like DataDir, though. I suspect they're actually equally
dirty, and I've just grown inured to the latter.)

https://codereview.appspot.com/13577045/diff/11001/environs/tools/tools.go#ne...
environs/tools/tools.go:226: 
s/availale/available/

https://codereview.appspot.com/13577045/diff/11001/environs/tools/urls_test.go
File environs/tools/urls_test.go (left):

https://codereview.appspot.com/13577045/diff/11001/environs/tools/urls_test.g...
environs/tools/urls_test.go:62: 
Is this missing an AssertExpectedSources?

https://codereview.appspot.com/13577045/diff/11001/environs/tools/urls_test.go
File environs/tools/urls_test.go (right):

https://codereview.appspot.com/13577045/diff/11001/environs/tools/urls_test.g...
environs/tools/urls_test.go:71: 
doesn't the AssertExpectedSources guarantee that we'll get at least one pass
through the loop? AFAICT that's all that haveExpectedSources is verifying.

[fwereade, 2013-09-19 22:47:14]

After discussion online, LGTM conditional on introduction of params structs
where appropriate, and on a slightly more dourly explicit patching mechanism --
eg unexported `var bootstrapFindTools = envtools.FindTools` and an exported
patching func with a clearly test-only name.

[wallyworld, 2013-09-20 00:33:57]

https://codereview.appspot.com/13577045/diff/11001/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/13577045/diff/11001/cmd/juju/bootstrap.go#newc...
cmd/juju/bootstrap.go:131: 
On 2013/09/19 12:59:38, fwereade wrote:
> should this be the named constant?

Done.

https://codereview.appspot.com/13577045/diff/11001/cmd/juju/bootstrap_test.go
File cmd/juju/bootstrap_test.go (right):

https://codereview.appspot.com/13577045/diff/11001/cmd/juju/bootstrap_test.go...
cmd/juju/bootstrap_test.go:66: 
On 2013/09/19 12:59:38, fwereade wrote:
> These can be smooshed up for a bit less indentation and slightly better
> readability (IMO)
> 
> ... []test{{
> 
> },{
> 
> }}

Done.

https://codereview.appspot.com/13577045/diff/11001/environs/bootstrap/bootstr...
File environs/bootstrap/bootstrap.go (right):

https://codereview.appspot.com/13577045/diff/11001/environs/bootstrap/bootstr...
environs/bootstrap/bootstrap.go:62: 
On 2013/09/19 12:59:38, fwereade wrote:
> ditto named constant

Done.

https://codereview.appspot.com/13577045/diff/11001/environs/manual/provisione...
File environs/manual/provisioner_test.go (right):

https://codereview.appspot.com/13577045/diff/11001/environs/manual/provisione...
environs/manual/provisioner_test.go:53: 
On 2013/09/19 12:59:38, fwereade wrote:
> this param list is actually getting a touch alarming

params struct used

https://codereview.appspot.com/13577045/diff/11001/environs/tools/tools.go
File environs/tools/tools.go (right):

https://codereview.appspot.com/13577045/diff/11001/environs/tools/tools.go#ne...
environs/tools/tools.go:166: 
On 2013/09/19 12:59:38, fwereade wrote:
> Is this only for testing this package? If so, please export_test it. If not,
is
> it possible to arrange whatever other package that needs it so that we can
patch
> out FBT at the point of use? As it is I feel it's exposing things it really
> shouldn't.
> 
> (I'm not sure why patching out a func in a module makes me feel dirtier than
> patching out a var like DataDir, though. I suspect they're actually equally
> dirty, and I've just grown inured to the latter.)

Done.

https://codereview.appspot.com/13577045/diff/11001/environs/tools/tools.go#ne...
environs/tools/tools.go:226: 
On 2013/09/19 12:59:38, fwereade wrote:
> s/availale/available/

Done.

https://codereview.appspot.com/13577045/diff/11001/environs/tools/urls_test.go
File environs/tools/urls_test.go (left):

https://codereview.appspot.com/13577045/diff/11001/environs/tools/urls_test.g...
environs/tools/urls_test.go:62: 
On 2013/09/19 12:59:38, fwereade wrote:
> Is this missing an AssertExpectedSources?

Sigh. Yes.

https://codereview.appspot.com/13577045/diff/11001/environs/tools/urls_test.go
File environs/tools/urls_test.go (right):

https://codereview.appspot.com/13577045/diff/11001/environs/tools/urls_test.g...
environs/tools/urls_test.go:71: 
On 2013/09/19 12:59:38, fwereade wrote:
> doesn't the AssertExpectedSources guarantee that we'll get at least one pass
> through the loop? AFAICT that's all that haveExpectedSources is verifying.

Bah. haveExpectedSources = true was in the wrong spot. Fixed.