juju: NewAPIClientFromName connection

juju: NewAPIClientFromName connection

We change it so it connects to cached API endpoint info
if available, falling back to information from the environment
configuration if that fails or takes a while.

https://code.launchpad.net/~rogpeppe/juju-core/396-newclientfromname/+merge/186303

(do not edit description out of merge proposal)

[rog, 2013-09-18 12:38:32]

Please take a look.

[gz, 2013-09-18 13:23:59]

LGTM, but this is pretty complicated and I had to think hard about several of
the interactions to understand them. A followup review from someone else would
be great.

I have a niggling fear about having logic this involved in an area that is a
hotspot for user issues. Even if the code is correct, errors over connecting are
the kind of things that come up regularly in user support in all kinds of
interesting variations, so being able to quickly diagnose what exactly in their
setup is borked from the errors reported is really important.

https://codereview.appspot.com/13251052/diff/5001/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/13251052/diff/5001/juju/api.go#newcode218
juju/api.go:218: case <-stop:
So, if newAPIFromName returns (because infoConnect completed) before delay is
up, this gets triggered and no second connect happens, correct?

https://codereview.appspot.com/13251052/diff/5001/juju/apiconn_test.go
File juju/apiconn_test.go (right):

https://codereview.appspot.com/13251052/diff/5001/juju/apiconn_test.go#newcod...
juju/apiconn_test.go:138: expectErr := fmt.Errorf("an error")
I guess the string used for the expected error doesn't really matter, this made
me look twice though.

[rog, 2013-09-18 13:56:28]

> being able to quickly diagnose what exactly in
> their setup is borked from the errors reported
> is really important.

That's a good point. It would be nice if the returned error message indicated
somehow where it was getting the endpoint information. One potential issue with
changing the error messages is that code calling  NewAPIClientFromName might be
looking for specific error codes in the error. Needs some thought.

https://codereview.appspot.com/13251052/diff/5001/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/13251052/diff/5001/juju/api.go#newcode218
juju/api.go:218: case <-stop:
On 2013/09/18 13:23:59, gz wrote:
> So, if newAPIFromName returns (because infoConnect completed) before delay is
> up, this gets triggered and no second connect happens, correct?

Yes. It occurs to me that I don't have a test for that actually.

[rog, 2013-09-18 14:06:32]

Please take a look.

[rog, 2013-09-18 14:14:15]

Please take a look.

[fwereade, 2013-09-18 15:33:18]

Nearly there, a few thoughts.

https://codereview.appspot.com/13251052/diff/4001/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/13251052/diff/4001/juju/api.go#newcode84
juju/api.go:84: envName = envs.Default
This takes its value from whichever of the many places is applicable today, I
presume? It feels a bit off, but because it's constrained, I guess.

https://codereview.appspot.com/13251052/diff/4001/juju/api.go#newcode105
juju/api.go:105: // and never hit the provider.
I feel that this is shading towards being too clever -- but if we clean up the
resources we use properly, I rather like it.

https://codereview.appspot.com/13251052/diff/4001/juju/api.go#newcode143
juju/api.go:143: // finally succeeds or fails. Unless we are making hundreds
But if it succeeds, it'll linger forever, right? That's not ideal. See below for
a possibility...

https://codereview.appspot.com/13251052/diff/4001/juju/api.go#newcode149
juju/api.go:149: // have one, because that information should be most current.
I'm not so happy that we ignore infoErr here. Would you log it please?

https://codereview.appspot.com/13251052/diff/4001/juju/api.go#newcode230
juju/api.go:230: resultc <- apiOpenResult{st, err}
Perhaps a:

select {
case <-stop:
    if st != nil {
        st.Close()
    }
resultc <- apiOpenResult{st, err}
}

...and another in apiInfoConnnect, might make this all a bit neater at cleanup
time?

https://codereview.appspot.com/13251052/diff/4001/juju/apiconn_test.go
File juju/apiconn_test.go (right):

https://codereview.appspot.com/13251052/diff/4001/juju/apiconn_test.go#newcod...
juju/apiconn_test.go:364: }
Thank you for the detailed tests.

[rog, 2013-09-18 17:39:26]

Please take a look.

https://codereview.appspot.com/13251052/diff/4001/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/13251052/diff/4001/juju/api.go#newcode84
juju/api.go:84: envName = envs.Default
On 2013/09/18 15:33:18, fwereade wrote:
> This takes its value from whichever of the many places is applicable today, I
> presume? It feels a bit off, but because it's constrained, I guess.

I'm not sure what you mean by this.

https://codereview.appspot.com/13251052/diff/4001/juju/api.go#newcode149
juju/api.go:149: // have one, because that information should be most current.
On 2013/09/18 15:33:18, fwereade wrote:
> I'm not so happy that we ignore infoErr here. Would you log it please?

Done.

https://codereview.appspot.com/13251052/diff/4001/juju/api.go#newcode230
juju/api.go:230: resultc <- apiOpenResult{st, err}
On 2013/09/18 15:33:18, fwereade wrote:
> Perhaps a:
> 
> select {
> case <-stop:
>     if st != nil {
>         st.Close()
>     }
> resultc <- apiOpenResult{st, err}
> }
> 
> ...and another in apiInfoConnnect, might make this all a bit neater at cleanup
> time?

Good catch. Done.

[gz, 2013-09-18 17:43:37]

Changes LGTM.

[rog, 2013-09-19 06:36:33]

Please take a look.

[fwereade, 2013-09-19 08:09:48]

LGTM, thanks, unless you think I'll be alarmed by the answers to the questions
below ;).

https://codereview.appspot.com/13251052/diff/24001/environs/configstore/disk_...
File environs/configstore/disk_test.go (left):

https://codereview.appspot.com/13251052/diff/24001/environs/configstore/disk_...
environs/configstore/disk_test.go:46: second line'
Just a thought... is there some nicer way we can do multiline text in YAML that
doesm't involve the \n-doubling? 'cos it's kinda ugly.

https://codereview.appspot.com/13251052/diff/24001/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/13251052/diff/24001/juju/api.go#newcode88
juju/api.go:88: envName = envs.Default
Sorry, WRT the previous comment, I was thinking of the chain of
environment.yaml, then wherever it is we store the "current environment", then
the value of JUJU_ENVIRON or whatever it is, and finally the -e param; and I was
asking if we're 100% sure that we're respecting that chain here; and expecting
that most of the chain would have been handled previously, such that "" could
only possibly indicate "get it from environments.yaml".

But, yeah, I think I got distracted while commenting and lost half of it. Sorry.

https://codereview.appspot.com/13251052/diff/24001/juju/api.go#newcode150
juju/api.go:150: return st, nil
This is no longer accurate, I think. AIUI a second connection may still briefly
persist beyond the end of this func, but it should now be cleaned up in short
order rather than hanging around.

https://codereview.appspot.com/13251052/diff/24001/juju/api.go#newcode155
juju/api.go:155: logger.Warningf("discarding API open error: %v", infoErr)
`if infoErr != nil {`?

Also this message feels a bit generic, but perhaps the logger gives enough
context.

https://codereview.appspot.com/13251052/diff/24001/juju/api.go#newcode209
juju/api.go:209: logger.Warningf("disarding stale API open error: %v", err)
nice

[rog, 2013-09-19 14:25:11]

https://codereview.appspot.com/13251052/diff/24001/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/13251052/diff/24001/juju/api.go#newcode88
juju/api.go:88: envName = envs.Default
On 2013/09/19 08:09:48, fwereade wrote:
> Sorry, WRT the previous comment, I was thinking of the chain of
> environment.yaml, then wherever it is we store the "current environment", then
> the value of JUJU_ENVIRON or whatever it is, and finally the -e param; and I
was
> asking if we're 100% sure that we're respecting that chain here; and expecting
> that most of the chain would have been handled previously, such that "" could
> only possibly indicate "get it from environments.yaml".

It's the same logic that we're currently using in environs, so I think it's
sound. All that cruft is set up before environs gets a look in, BTW, in
cmd/environmentcommand.go.

https://codereview.appspot.com/13251052/diff/24001/juju/api.go#newcode150
juju/api.go:150: return st, nil
On 2013/09/19 08:09:48, fwereade wrote:
> This is no longer accurate, I think. AIUI a second connection may still
briefly
> persist beyond the end of this func, but it should now be cleaned up in short
> order rather than hanging around.

Actually it was the "briefly persist" reason that I originally wrote this
comment (I'd forgotten about closing the opened state). It is still an issue, as
the old dial to the old address may take minutes to time out, so I'll leave the
comment if that's ok.

https://codereview.appspot.com/13251052/diff/24001/juju/api.go#newcode155
juju/api.go:155: logger.Warningf("discarding API open error: %v", infoErr)
On 2013/09/19 08:09:48, fwereade wrote:
> `if infoErr != nil {`?

We can only get here if both errors are non-nil.


> Also this message feels a bit generic, but perhaps the logger gives enough
> context.

I hope so, but suggestions for better log messages are always appreciated.

[rog, 2013-09-19 15:24:13]

Please take a look.