juju: cache API endpoints and credentials

juju: cache API endpoints and credentials

Fixes bug #1268471:
1. After the first successful connection to the API,
the endpoints and credentials are cached in the
environment's .jenv file.
2. Cached API connection settings are tried first
when connecting to the API server (if available),
otherwise we failback to using the provider storage
to fetch the provider-state, wait for the instance
DNS address and then use that to connect (slower).
3. After each successful API connection the cache
is updated (in the future we might also fetch all
API server addresses from the server using an API
call, so we can always have up-to-date addresses).

Tested live on EC2 - works like charm and speeds
up all CLI commands quite a bit.

https://code.launchpad.net/~dimitern/juju-core/230-cache-bootstrap-address-1268471/+merge/201593

(do not edit description out of merge proposal)

[dimitern, 2014-01-14 14:00:29]

Please take a look.

[rog, 2014-01-14 15:49:13]

Looks good in general, with some suggestions below.

Two other points:

- We need some tests.

- We probably want to update the cached addresses from the addresses currently
found in the state. We could potentially do this asynchronously so that we don't
pay another round trip cost or we could make it an explicit method. Either way,
it doesn't need to be done now, but it will be important in the future when
there's a gradually changing set of server addresses.

https://codereview.appspot.com/52050043/diff/1/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/52050043/diff/1/juju/api.go#newcode40
juju/api.go:40: func prepareAPIInfo(environ environs.Environ) (*api.Info, error)
{
This isn't a great name (especially with Prepare as a verb already in use with
another meaning).

Given that it gets the environment info from the Environ, how about
environAPIInfo ?

I'd also move it near the end of the file, because it's a leaf function and this
file is generally laid out high to low level.

https://codereview.appspot.com/52050043/diff/1/juju/api.go#newcode197
juju/api.go:197: info.SetAPIEndpoint(configstore.APIEndpoint{
info can be nil here (if we were legacy-bootstrapping without a .jenv file).
If we want to continue supporting that behaviour, either we should create the
info file here, or not cache the info. I can think of arguments both ways, but I
think I'd prefer the former.

https://codereview.appspot.com/52050043/diff/1/juju/api.go#newcode308
juju/api.go:308: apiConn, err := NewAPIConn(environ, api.DefaultDialOpts())
If we call apiOpen directly here, we'll only fetch StateInfo once.

https://codereview.appspot.com/52050043/diff/1/names/user.go
File names/user.go (right):

https://codereview.appspot.com/52050043/diff/1/names/user.go#newcode17
names/user.go:17: func UserTag(userName string) string {
I know it's trivial, but you might want to add a test.

[jameinel, 2014-01-15 07:29:20]

I think the change is great for the config code path, but when opening via the
API this is just writing back the values you used to open the API (which doesn't
really help anything).

I'd love to chat with you about how we can add an asynchronous API call to fetch
updated values on every connection, and then play with that to make "juju
api-endpoints" wait for that to finish and use the updated values.

I don't think we want to synchronously block for updated API endpoints on every
connect (because they are unlikely to change frequently, and many CLI commands
are only a single round trip after Login anyway).

https://codereview.appspot.com/52050043/diff/1/juju/api.go
File juju/api.go (left):

https://codereview.appspot.com/52050043/diff/1/juju/api.go#oldcode207
juju/api.go:207: Addrs:    endpoint.Addresses,
If I'm reading this correctly, you're populating api.Info with the local
information, which you are then writing back to disk once we've connected.

That seems a bit circular.

I'm looking to create a function (for juju api-endpoints) that would give the
up-to-date values. I'd rather fire that off as a separate goroutine (which can
be cancelled if we finish our other work before we get a response).

I really like your change for the apiConfigConnect version of the code, but the
way this is structured we're just rewriting the file with the current content.

Though maybe we can start with what you've done here, and then I'll figure out
what an asynchronous update to the apiInfo looks like.

I'd almost rather just send "nil" back for apiInfo and handle that in the other
code, for when we don't have newer data to write. Rather than just feeding back
what we already have. (No reason to rewrite a file that hasn't changed, etc.)

https://codereview.appspot.com/52050043/diff/1/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/52050043/diff/1/juju/api.go#newcode160
juju/api.go:160: logger.Infof("no cached API connection settings found")
These are probably Debugf level messages.

[dimitern, 2014-01-15 11:21:59]

Please take a look.

https://codereview.appspot.com/52050043/diff/1/juju/api.go
File juju/api.go (left):

https://codereview.appspot.com/52050043/diff/1/juju/api.go#oldcode207
juju/api.go:207: Addrs:    endpoint.Addresses,
On 2014/01/15 07:29:20, jameinel wrote:
> If I'm reading this correctly, you're populating api.Info with the local
> information, which you are then writing back to disk once we've connected.
> 
> That seems a bit circular.
> 
Now the cache gets updated only when we connect without cached info (from the
environment).

> I'm looking to create a function (for juju api-endpoints) that would give the
> up-to-date values. I'd rather fire that off as a separate goroutine (which can
> be cancelled if we finish our other work before we get a response).
I think updating the addresses with an API call is an improvement over this CL,
but can be done in a follow-up (we have caching with this, and addresses are not
changing yet).

> 
> I really like your change for the apiConfigConnect version of the code, but
the
> way this is structured we're just rewriting the file with the current content.
> 
That's the point of caching - we connected successfully by fetching the info
from the provider, so we don't want to do it again next time.

> Though maybe we can start with what you've done here, and then I'll figure out
> what an asynchronous update to the apiInfo looks like.
> 
> I'd almost rather just send "nil" back for apiInfo and handle that in the
other
> code, for when we don't have newer data to write. Rather than just feeding
back
> what we already have. (No reason to rewrite a file that hasn't changed, etc.)
Cache updating is now conditional, not done always.

https://codereview.appspot.com/52050043/diff/1/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/52050043/diff/1/juju/api.go#newcode40
juju/api.go:40: func prepareAPIInfo(environ environs.Environ) (*api.Info, error)
{
On 2014/01/14 15:49:13, rog wrote:
> This isn't a great name (especially with Prepare as a verb already in use with
> another meaning).
> 
> Given that it gets the environment info from the Environ, how about
> environAPIInfo ?
> 
> I'd also move it near the end of the file, because it's a leaf function and
this
> file is generally laid out high to low level.

Renamed to environAPIInfo and moved at the end.

https://codereview.appspot.com/52050043/diff/1/juju/api.go#newcode160
juju/api.go:160: logger.Infof("no cached API connection settings found")
On 2014/01/15 07:29:20, jameinel wrote:
> These are probably Debugf level messages.

Done.

https://codereview.appspot.com/52050043/diff/1/juju/api.go#newcode197
juju/api.go:197: info.SetAPIEndpoint(configstore.APIEndpoint{
On 2014/01/14 15:49:13, rog wrote:
> info can be nil here (if we were legacy-bootstrapping without a .jenv file).
> If we want to continue supporting that behaviour, either we should create the
> info file here, or not cache the info. I can think of arguments both ways, but
I
> think I'd prefer the former.
> 

Added a check for info != nil. I don't think is worth creating the file if it
did not exist after bootstrap.

https://codereview.appspot.com/52050043/diff/1/juju/api.go#newcode308
juju/api.go:308: apiConn, err := NewAPIConn(environ, api.DefaultDialOpts())
On 2014/01/14 15:49:13, rog wrote:
> If we call apiOpen directly here, we'll only fetch StateInfo once.

Done.

https://codereview.appspot.com/52050043/diff/1/names/user.go
File names/user.go (right):

https://codereview.appspot.com/52050043/diff/1/names/user.go#newcode17
names/user.go:17: func UserTag(userName string) string {
On 2014/01/14 15:49:13, rog wrote:
> I know it's trivial, but you might want to add a test.

Done.

[rog, 2014-01-15 13:31:09]

Looks great modulo tests and some suggestions below.

https://codereview.appspot.com/52050043/diff/20001/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/52050043/diff/20001/juju/api.go#newcode40
juju/api.go:40: fromEnviron bool
We could lose this field and rename connectedInfo to cacheInfo;
then the code could cache the info only if cacheInfo is non-nil.

https://codereview.appspot.com/52050043/diff/20001/juju/api.go#newcode185
juju/api.go:185: }
Please do the dynamic type conversion once only.

I would usually do something like:

     val0, err := try.Result()
     if err != nil {
         ...
     }
     val := val0.(apiState)
     if info != nil && val.cacheInfo != nil {
         ...
     }
     return val.st

https://codereview.appspot.com/52050043/diff/20001/juju/api.go#newcode191
juju/api.go:191: // Cache the successful connection info for future use, but
only if we
How about putting the body of this into its own function?

say:

func cacheInfo(info configstore.EnvironInfo, apiInfo *api.Info) error

[dimitern, 2014-01-15 14:24:02]

Please take a look.

https://codereview.appspot.com/52050043/diff/20001/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/52050043/diff/20001/juju/api.go#newcode40
juju/api.go:40: fromEnviron bool
On 2014/01/15 13:31:09, rog wrote:
> We could lose this field and rename connectedInfo to cacheInfo;
> then the code could cache the info only if cacheInfo is non-nil.

Done.

https://codereview.appspot.com/52050043/diff/20001/juju/api.go#newcode185
juju/api.go:185: }
On 2014/01/15 13:31:09, rog wrote:
> Please do the dynamic type conversion once only.
> 
> I would usually do something like:
> 
>      val0, err := try.Result()
>      if err != nil {
>          ...
>      }
>      val := val0.(apiState)
>      if info != nil && val.cacheInfo != nil {
>          ...
>      }
>      return val.st

Done.

https://codereview.appspot.com/52050043/diff/20001/juju/api.go#newcode191
juju/api.go:191: // Cache the successful connection info for future use, but
only if we
On 2014/01/15 13:31:09, rog wrote:
> How about putting the body of this into its own function?
> 
> say:
> 
> func cacheInfo(info configstore.EnvironInfo, apiInfo *api.Info) error

Done.

[rog, 2014-01-15 14:42:04]

LGTM modulo the below, thanks.

https://codereview.appspot.com/52050043/diff/40001/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/52050043/diff/40001/juju/api.go#newcode37
juju/api.go:37: // cachedInfo is set only when the connection was made using the
// If cachedInfo is non-nil, it indicates that the
// info has been newly retrieved, and should
// be cached in the config store.

?

https://codereview.appspot.com/52050043/diff/40001/juju/api.go#newcode303
juju/api.go:303: logger.Warningf("not caching API connection settings: invalid
API user tag: %v", err)
rather than logging here, i'd return the error and log the warning in the
caller.

that makes it worthwhile to return the error (and the warning message will be
consistent then too).

https://codereview.appspot.com/52050043/diff/40001/juju/api.go#newcode315
juju/api.go:315: logger.Debugf("updated API connection settings cache")
I'd move this into the calling code too.

https://codereview.appspot.com/52050043/diff/40001/juju/apiconn_test.go
File juju/apiconn_test.go (right):

https://codereview.appspot.com/52050043/diff/40001/juju/apiconn_test.go#newco...
juju/apiconn_test.go:143: st, err := juju.NewAPIFromName("noconfig",
&readOnlyStore{store})
Panicking seems a bit over the top here.
Also, I'm pretty sure you haven't made sure that this
test fails if it does actually write the configstore
info, because configstore.Storage doesn't have a Write
method, so I believe that the readOnlyStore.Write is
ignored.

How about something more like this? (please verify that you
can get the test to fail if the info *is* written)


type storageWithWriteNotify struct {
	written bool
	store configstore.Storage
}

func (*storageWithWriteNotify) CreateInfo(envName string)
(configstore.EnvironInfo, error) {
	panic("CreateInfo not implemented")
}

func (s *storageWithWriteNotify) ReadInfo(envName string)
(configstore.EnvironInfo, error) {
	info, err := s.store.ReadInfo(envName)
	if err != nil {
		return nil, err
	}
 	return &infoWithWriteNotify{
		written: &s.written,
		EnvironInfo: info,
	}, nil
}

type infoWithWriteNotify struct {
	configstore.EnvironInfo
	written *bool
}

func (info *infoWithWriteNotify) Write() error {
	*info.written = true
	return info.EnvironInfo.Write()
}

https://codereview.appspot.com/52050043/diff/40001/names/user_test.go
File names/user_test.go (right):

https://codereview.appspot.com/52050043/diff/40001/names/user_test.go#newcode17
names/user_test.go:17: func (s *userSuite) TestUnitTag(c *gc.C) {
s/UnitTag/UserTag/

https://codereview.appspot.com/52050043/diff/40001/names/user_test.go#newcode21
names/user_test.go:21: func (s *userSuite) TestIsUnit(c *gc.C) {
s/IsUnit/IsUser/

[dimitern, 2014-01-15 15:17:05]

Please take a look.

https://codereview.appspot.com/52050043/diff/40001/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/52050043/diff/40001/juju/api.go#newcode37
juju/api.go:37: // cachedInfo is set only when the connection was made using the
On 2014/01/15 14:42:04, rog wrote:
> // If cachedInfo is non-nil, it indicates that the
> // info has been newly retrieved, and should
> // be cached in the config store.
> 
> ?

Done.

https://codereview.appspot.com/52050043/diff/40001/juju/api.go#newcode303
juju/api.go:303: logger.Warningf("not caching API connection settings: invalid
API user tag: %v", err)
On 2014/01/15 14:42:04, rog wrote:
> rather than logging here, i'd return the error and log the warning in the
> caller.
> 
> that makes it worthwhile to return the error (and the warning message will be
> consistent then too).

Done.

https://codereview.appspot.com/52050043/diff/40001/juju/api.go#newcode315
juju/api.go:315: logger.Debugf("updated API connection settings cache")
On 2014/01/15 14:42:04, rog wrote:
> I'd move this into the calling code too.

Done.

https://codereview.appspot.com/52050043/diff/40001/juju/apiconn_test.go
File juju/apiconn_test.go (right):

https://codereview.appspot.com/52050043/diff/40001/juju/apiconn_test.go#newco...
juju/apiconn_test.go:143: st, err := juju.NewAPIFromName("noconfig",
&readOnlyStore{store})
On 2014/01/15 14:42:04, rog wrote:
> Panicking seems a bit over the top here.
> Also, I'm pretty sure you haven't made sure that this
> test fails if it does actually write the configstore
> info, because configstore.Storage doesn't have a Write
> method, so I believe that the readOnlyStore.Write is
> ignored.
> 
> How about something more like this? (please verify that you
> can get the test to fail if the info *is* written)
> 
> 
> type storageWithWriteNotify struct {
> 	written bool
> 	store configstore.Storage
> }
> 
> func (*storageWithWriteNotify) CreateInfo(envName string)
> (configstore.EnvironInfo, error) {
> 	panic("CreateInfo not implemented")
> }
> 
> func (s *storageWithWriteNotify) ReadInfo(envName string)
> (configstore.EnvironInfo, error) {
> 	info, err := s.store.ReadInfo(envName)
> 	if err != nil {
> 		return nil, err
> 	}
>  	return &infoWithWriteNotify{
> 		written: &s.written,
> 		EnvironInfo: info,
> 	}, nil
> }
> 
> type infoWithWriteNotify struct {
> 	configstore.EnvironInfo
> 	written *bool
> }
> 
> func (info *infoWithWriteNotify) Write() error {
> 	*info.written = true
> 	return info.EnvironInfo.Write()
> }

Updated as suggested. Also forced the test to fail by temporarily modifying the
code to cache always and running the test manually.

https://codereview.appspot.com/52050043/diff/40001/names/user_test.go
File names/user_test.go (right):

https://codereview.appspot.com/52050043/diff/40001/names/user_test.go#newcode17
names/user_test.go:17: func (s *userSuite) TestUnitTag(c *gc.C) {
On 2014/01/15 14:42:04, rog wrote:
> s/UnitTag/UserTag/

Done.

https://codereview.appspot.com/52050043/diff/40001/names/user_test.go#newcode21
names/user_test.go:21: func (s *userSuite) TestIsUnit(c *gc.C) {
On 2014/01/15 14:42:04, rog wrote:
> s/IsUnit/IsUser/

Done.

[rog, 2014-01-15 15:48:31]

Still LGTM, thanks.