Merge lp:~axwalk/juju-core/client-cache-apihostports into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

Cache API server addresses in .jenv

We now return API server addresses from
the Login API, and cache those addresses
in the client's configstore. If the client
connects using environments.yaml, then it
is expected to get updated addresses from
provider-state.

https://codereview.appspot.com/81780043/

Cache API server addresses in .jenv

We now return API server addresses from
the Login API, and cache those addresses
in the client's configstore. If the client
connects using environments.yaml, then it
is expected to get updated addresses from
provider-state.

https://codereview.appspot.com/81780043/

[Andrew Wilkins (axwalk)]

Reviewers: mp+213217_code.launchpad.net,

Message:
Please take a look.

Description:
Cache API server addresses in .jenv

We now return API server addresses from
the Login API, and cache those addresses
in the client's configstore. If the client
connects using environments.yaml, then it
is expected to get updated addresses from
provider-state.

https://code.launchpad.net/~axwalk/juju-core/client-cache-apihostports/+merge/213217

Requires:
https://code.launchpad.net/~axwalk/juju-core/apihostports-api/+merge/213200

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/81780043/

Affected files (+173, -11 lines):
   A [revision details]
   M juju/api.go
   M juju/apiconn_test.go
   M state/api/apiclient.go
   M state/api/params/params.go
   M state/api/state.go
   M state/apiserver/admin.go
   M state/apiserver/login_test.go

[Roger Peppe (rogpeppe)]

Great direction, thanks, with a few suggestions below.

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

https://codereview.appspot.com/81780043/diff/1/juju/api.go#newcode252
juju/api.go:252: logger.Warningf(err.Error())
This isn't right (the error might contain % characters).

logger.Warningf("Cannot cache cache API addresses: %v", err)

(and change the error returned from cacheChangedAPIAddresses
so that the message won't be repetitious)

https://codereview.appspot.com/81780043/diff/1/juju/api.go#newcode340
juju/api.go:340: endpoint.Addresses = addrs
I'd like to add a HostPorts entry to the APIEndpoint struct
(we could perhaps deprecate the Addresses field)
and store all the APIHostPorts info there.

So all the code here would deal only with instance.HostPort,
not address strings. This leaves us free to have clients
that are more intelligent about dialling in the future.

https://codereview.appspot.com/81780043/diff/1/juju/api.go#newcode356
juju/api.go:356: sort.Strings(a)
I don't think I'd sort the addresses - the order can be significant.

https://codereview.appspot.com/81780043/diff/1/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/81780043/diff/1/state/api/apiclient.go#newcode35
state/api/apiclient.go:35: addrs []string
I think it would probably be better to store the hostports exactly as
returned by APIHostPorts here, leaving it up to client code to decide
how best to use the information.

https://codereview.appspot.com/81780043/diff/1/state/api/apiclient.go#newcode206
state/api/apiclient.go:206: func (s *State) Addrs() []string {
I'd rename this HostPorts and return all
the APIHostPorts results, with the connecting
address added as an extra element if it's
not found in the result.

Also, I wonder if this logic wouldn't live more naturally
inside the Login method.

https://codereview.appspot.com/81780043/diff/1/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/81780043/diff/1/state/api/params/params.go#newcode645
state/api/params/params.go:645: APIHostPortsResult
I think I'd just include the field explicitly here
rather than embedding.

https://codereview.appspot.com/81780043/diff/1/state/api/state.go
File state/api/state.go (right):

https://codereview.appspot.com/81780043/diff/1/state/api/state.go#newcode35
state/api/state.go:35: // The client must attempt *all* addresses, or
else
This comment seems like it might be more helpful to put as part of the
Addrs (or HostPorts) doc comment.

https://codereview.appspot.com/81780043/diff/1/state/apiserver/admin.go
File state/apiserver/admin.go (right):

https://codereview.appspot.com/81780043/diff/1/state/apiserver/admin.go#newcode88
state/apiserver/admin.go:88: // state; failure here is non-fatal.
I think it's perfectly reasonable to make the error fatal here.
If we can't access the APIHostPorts document, something is seriously
wrong.

https://codereview.appspot.com/81780043/

[Andrew Wilkins (axwalk)]

Please take a look.

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

https://codereview.appspot.com/81780043/diff/1/juju/api.go#newcode252
juju/api.go:252: logger.Warningf(err.Error())
On 2014/03/28 10:02:12, rog wrote:
> This isn't right (the error might contain % characters).

> logger.Warningf("Cannot cache cache API addresses: %v", err)

> (and change the error returned from cacheChangedAPIAddresses
> so that the message won't be repetitious)

I copied that from newAPIFromStore and clearly I didn't look too close.
Fixed both.

https://codereview.appspot.com/81780043/diff/1/juju/api.go#newcode340
juju/api.go:340: endpoint.Addresses = addrs
On 2014/03/28 10:02:12, rog wrote:
> I'd like to add a HostPorts entry to the APIEndpoint struct
> (we could perhaps deprecate the Addresses field)
> and store all the APIHostPorts info there.

> So all the code here would deal only with instance.HostPort,
> not address strings. This leaves us free to have clients
> that are more intelligent about dialling in the future.

+1

https://codereview.appspot.com/81780043/diff/1/juju/api.go#newcode356
juju/api.go:356: sort.Strings(a)
On 2014/03/28 10:02:12, rog wrote:
> I don't think I'd sort the addresses - the order can be significant.

Done.

https://codereview.appspot.com/81780043/diff/1/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/81780043/diff/1/state/api/apiclient.go#newcode35
state/api/apiclient.go:35: addrs []string
On 2014/03/28 10:02:12, rog wrote:
> I think it would probably be better to store the hostports exactly as
returned
> by APIHostPorts here, leaving it up to client code to decide how best
to use the
> information.

Done.

https://codereview.appspot.com/81780043/diff/1/state/api/apiclient.go#newcode206
state/api/apiclient.go:206: func (s *State) Addrs() []string {
On 2014/03/28 10:02:12, rog wrote:
> I'd rename this HostPorts and return all
> the APIHostPorts results, with the connecting
> address added as an extra element if it's
> not found in the result.

Done. If the connecting address isn't found, it's added with
NetworkUnknown scope.

> Also, I wonder if this logic wouldn't live more naturally
> inside the Login method.

Yeah, it does, because there's a theoretical error when converting
s.addr->HostPort. Moved.

https://codereview.appspot.com/81780043/diff/1/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/81780043/diff/1/state/api/params/params.go#newcode645
state/api/params/params.go:645: APIHostPortsResult
On 2014/03/28 10:02:12, rog wrote:
> I think I'd just include the field explicitly here
> rather than embedding.

Done.

https://codereview.appspot.com/81780043/diff/1/state/api/state.go
File state/api/state.go (right):

https://codereview.appspot.com/81780043/diff/1/state/api/state.go#newcode35
state/api/state.go:35: // The client must attempt *all* addresses, or
else
On 2014/03/28 10:02:12, rog wrote:
> This comment seems like it might be more helpful to put as part of the
Addrs (or
> HostPorts) doc comment.

Done.

https://codereview.appspot.com/81780043/diff/1/state/apiserver/admin.go
File st...

[Roger Peppe (rogpeppe)]

LGTM with the juju tests fixed so they don't rely on calling methods on
the zero value, and a few other suggestions below.

https://codereview.appspot.com/81780043/diff/20001/juju/apiconn_test.go
File juju/apiconn_test.go (right):

https://codereview.appspot.com/81780043/diff/20001/juju/apiconn_test.go#newcode160
juju/apiconn_test.go:160: // be updated (to an empty slice).
As discussed online, I'm not sure that this is the correct approach - we
shouldn't rely on calling methods on types that we've created with New.

https://codereview.appspot.com/81780043/diff/20001/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/81780043/diff/20001/state/api/apiclient.go#newcode212
state/api/apiclient.go:212: func (s *State) HostPorts()
[][]instance.HostPort {
On further reflection, I think this method should be named APIHostPorts,
mirroring the same function in State, and making it hopefully clearer
what the returned addresses are about.

https://codereview.appspot.com/81780043/diff/20001/state/api/apiclient.go#newcode213
state/api/apiclient.go:213: return append([][]instance.HostPort{},
s.hostPorts...)
If you're serious about returning a copy, then you should probably copy
all the sub-slices too.

Otherwise, just return s.hostPorts and document that the returned slice
should not be modified.

https://codereview.appspot.com/81780043/diff/20001/state/api/state.go
File state/api/state.go (right):

https://codereview.appspot.com/81780043/diff/20001/state/api/state.go#newcode40
state/api/state.go:40: if err := st.addAddrHostPort(); err != nil {
rather than have addAddrHostPort be a method that mutates st.hostPorts,
perhaps it might be clearer to have something like:

hostPorts, err := addAddress(result.Servers, st.addr)
if err != nil {
     st.Close()
     return err
}
st.hostPorts = hostPort

// addAddress appends a new server derived from the given
// address to servers if the address is not already found
// there.
func addAddress(servers [][]instance.HostPort, addr string)
([][]instance.HostPort, error)

Then addAddress is easily testable if you want to.

BTW I have mixed feelings about returning an error if we can't split the
dial address. It means that if the dial address isn't parseable for some
reason (e.g. the port is non-numeric) we'll fail the connection, even
though we actually managed to connect.

https://codereview.appspot.com/81780043/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/81780043/diff/20001/juju/apiconn_test.go
File juju/apiconn_test.go (right):

https://codereview.appspot.com/81780043/diff/20001/juju/apiconn_test.go#newcode160
juju/apiconn_test.go:160: // be updated (to an empty slice).
On 2014/03/31 08:37:33, rog wrote:
> As discussed online, I'm not sure that this is the correct approach -
we
> shouldn't rely on calling methods on types that we've created with
New.

Done.

https://codereview.appspot.com/81780043/diff/20001/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/81780043/diff/20001/state/api/apiclient.go#newcode212
state/api/apiclient.go:212: func (s *State) HostPorts()
[][]instance.HostPort {
On 2014/03/31 08:37:33, rog wrote:
> On further reflection, I think this method should be named
APIHostPorts,
> mirroring the same function in State, and making it hopefully clearer
what the
> returned addresses are about.

Done.

https://codereview.appspot.com/81780043/diff/20001/state/api/apiclient.go#newcode213
state/api/apiclient.go:213: return append([][]instance.HostPort{},
s.hostPorts...)
On 2014/03/31 08:37:33, rog wrote:
> If you're serious about returning a copy, then you should probably
copy all the
> sub-slices too.

> Otherwise, just return s.hostPorts and document that the returned
slice should
> not be modified.

Hah, thanks for that. Fixed.

https://codereview.appspot.com/81780043/diff/20001/state/api/state.go
File state/api/state.go (right):

https://codereview.appspot.com/81780043/diff/20001/state/api/state.go#newcode40
state/api/state.go:40: if err := st.addAddrHostPort(); err != nil {
On 2014/03/31 08:37:33, rog wrote:
> rather than have addAddrHostPort be a method that mutates
st.hostPorts, perhaps
> it might be clearer to have something like:

> hostPorts, err := addAddress(result.Servers, st.addr)
> if err != nil {
> st.Close()
> return err
> }
> st.hostPorts = hostPort

> // addAddress appends a new server derived from the given
> // address to servers if the address is not already found
> // there.
> func addAddress(servers [][]instance.HostPort, addr string)
> ([][]instance.HostPort, error)

Done.

> Then addAddress is easily testable if you want to.

> BTW I have mixed feelings about returning an error if we can't split
the dial
> address. It means that if the dial address isn't parseable for some
reason (e.g.
> the port is non-numeric) we'll fail the connection, even though we
actually
> managed to connect.

Me too, but at the moment that remains theoretical. I'd rather have it
fail spectacularly if/when we decide to have a non-numeric
/etc/services-style port, and fix it then.

https://codereview.appspot.com/81780043/

[Go Bot (go-bot)]

The attempt to merge lp:~axwalk/juju-core/client-cache-apihostports into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core 0.014s
ok launchpad.net/juju-core/agent 1.133s
ok launchpad.net/juju-core/agent/mongo 0.544s
ok launchpad.net/juju-core/agent/tools 0.458s
ok launchpad.net/juju-core/bzr 5.334s
ok launchpad.net/juju-core/cert 3.032s
ok launchpad.net/juju-core/charm 0.347s
? launchpad.net/juju-core/charm/hooks [no test files]
? launchpad.net/juju-core/charm/testing [no test files]
ok launchpad.net/juju-core/cloudinit 0.033s
ok launchpad.net/juju-core/cloudinit/sshinit 0.848s
ok launchpad.net/juju-core/cmd 0.183s
ok launchpad.net/juju-core/cmd/charm-admin 0.742s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 201.222s
ok launchpad.net/juju-core/cmd/jujud 61.311s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 8.095s
? launchpad.net/juju-core/cmd/plugins/juju-restore [no test files]
ok launchpad.net/juju-core/cmd/plugins/local 0.219s
? launchpad.net/juju-core/cmd/plugins/local/juju-local [no test files]
ok launchpad.net/juju-core/constraints 0.022s
ok launchpad.net/juju-core/container 0.032s
ok launchpad.net/juju-core/container/factory 0.041s
ok launchpad.net/juju-core/container/kvm 0.184s
ok launchpad.net/juju-core/container/kvm/mock 0.035s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 4.328s
? launchpad.net/juju-core/container/lxc/mock [no test files]
? launchpad.net/juju-core/container/lxc/testing [no test files]
? launchpad.net/juju-core/container/testing [no test files]
ok launchpad.net/juju-core/downloader 5.222s
ok launchpad.net/juju-core/environs 2.492s
ok launchpad.net/juju-core/environs/bootstrap 10.562s
ok launchpad.net/juju-core/environs/cloudinit 0.494s
ok launchpad.net/juju-core/environs/config 2.452s
ok launchpad.net/juju-core/environs/configstore 0.029s
ok launchpad.net/juju-core/environs/filestorage 0.025s
ok launchpad.net/juju-core/environs/httpstorage 0.669s
ok launchpad.net/juju-core/environs/imagemetadata 0.412s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.036s
ok launchpad.net/juju-core/environs/jujutest 0.181s
ok launchpad.net/juju-core/environs/manual 11.573s
ok launchpad.net/juju-core/environs/simplestreams 0.245s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 0.806s
ok launchpad.net/juju-core/environs/storage 0.805s
ok launchpad.net/juju-core/environs/sync 43.459s
ok launchpad.net/juju-core/environs/testing 0.116s
ok launchpad.net/juju-core/environs/tools 4.456s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.011s
ok launchpad.net/juju-core/instance 0.018s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 18.117s
ok launchpad.net/juju-core/juju/arch 0.0...

[Roger Peppe (rogpeppe)]

still LGTM

https://codereview.appspot.com/81780043/diff/40001/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/81780043/diff/40001/state/api/apiclient.go#newcode215
state/api/apiclient.go:215: hostPorts[i] = append([]instance.HostPort{},
server...)
or hostPorts[i] = append(hostPorts[i], server...)
(slightly shorter)

https://codereview.appspot.com/81780043/

=== modified file 'juju/api.go'
--- juju/api.go	2014-03-19 03:18:41 +0000
+++ juju/api.go	2014-03-31 12:32:07 +0000
@@ -14,6 +14,7 @@
 	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/environs/configstore"
 	"launchpad.net/juju-core/errors"
+	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/juju/osenv"
 	"launchpad.net/juju-core/names"
 	"launchpad.net/juju-core/state/api"
@@ -26,24 +27,25 @@
 // The following are variables so that they can be
 // changed by tests.
 var (
-	apiOpen              = api.Open
-	apiClose             = (*api.State).Close
 	providerConnectDelay = 2 * time.Second
 )
 
-// apiState wraps an api.State, redefining its Close method
-// so we can abuse it for testing purposes.
-type apiState struct {
-	st *api.State
+// apiState provides a subset of api.State's public
+// interface, defined here so it can be mocked.
+type apiState interface {
+	Close() error
+	APIHostPorts() [][]instance.HostPort
+}
+
+type apiOpenFunc func(*api.Info, api.DialOpts) (apiState, error)
+
+type apiStateCachedInfo struct {
+	apiState
 	// If cachedInfo is non-nil, it indicates that the info has been
 	// newly retrieved, and should be cached in the config store.
 	cachedInfo *api.Info
 }
 
-func (st apiState) Close() error {
-	return apiClose(st.st)
-}
-
 // APIConn holds a connection to a juju environment and its
 // associated state through its API interface.
 type APIConn struct {
@@ -62,7 +64,7 @@
 		return nil, err
 	}
 
-	st, err := apiOpen(info, dialOpts)
+	st, err := api.Open(info, dialOpts)
 	// TODO(rog): handle errUnauthorized when the API handles passwords.
 	if err != nil {
 		return nil, err
@@ -76,7 +78,7 @@
 // Close terminates the connection to the environment and releases
 // any associated resources.
 func (c *APIConn) Close() error {
-	return apiClose(c.State)
+	return c.State.Close()
 }
 
 // NewAPIClientFromName returns an api.Client connected to the API Server for
@@ -112,12 +114,19 @@
 	if err != nil {
 		return nil, err
 	}
-	return newAPIFromStore(envName, store)
+	apiOpen := func(info *api.Info, opts api.DialOpts) (apiState, error) {
+		return api.Open(info, opts)
+	}
+	st, err := newAPIFromStore(envName, store, apiOpen)
+	if err != nil {
+		return nil, err
+	}
+	return st.(*api.State), nil
 }
 
 // newAPIFromStore implements the bulk of NewAPIClientFromName
 // but is separate for testing purposes.
-func newAPIFromStore(envName string, store configstore.Storage) (*api.State, error) {
+func newAPIFromStore(envName string, store configstore.Storage, apiOpen apiOpenFunc) (apiState, error) {
 	// Try to read the default environment configuration file.
 	// If it doesn't exist, we carry on in case
 	// there's some environment info for that environment.
@@ -170,7 +179,7 @@
 	if info != nil && len(info.APIEndpoint().Addresses) > 0 {
 		logger.Debugf("trying cached API connection settings")
 		try.Start(func(stop <-chan struct{}) (io.Closer, error) {
-			return apiInfoConnect(store, info, stop)
+			return apiInfoConnect(store, info, apiOpen, stop)
 		})
 		// Delay the config connection until we've spent
 		// some time trying to connect to the cached info.
@@ -179,7 +188,7 @@
 		logger.Debugf("no cached API connection settings found")
 	}
 	try.Start(func(stop <-chan struct{}) (io.Closer, error) {
-		return apiConfigConnect(info, envs, envName, stop, delay)
+		return apiConfigConnect(info, envs, envName, apiOpen, stop, delay)
 	})
 	try.Close()
 	val0, err := try.Result()
@@ -190,20 +199,23 @@
 		}
 		return nil, err
 	}
+
 	val := val0.(apiState)
-
-	if val.cachedInfo != nil && info != nil {
-		// Cache the connection settings only if we used the
-		// environment config, but any errors are just logged
-		// as warnings, because they're not fatal.
-		err = cacheAPIInfo(info, val.cachedInfo)
-		if err != nil {
-			logger.Warningf(err.Error())
-		} else {
-			logger.Debugf("updated API connection settings cache")
+	if cachedInfo, ok := val.(apiStateCachedInfo); ok {
+		val = cachedInfo.apiState
+		if cachedInfo.cachedInfo != nil && info != nil {
+			// Cache the connection settings only if we used the
+			// environment config, but any errors are just logged
+			// as warnings, because they're not fatal.
+			err = cacheAPIInfo(info, cachedInfo.cachedInfo)
+			if err != nil {
+				logger.Warningf("cannot cache API connection settings: %v", err.Error())
+			} else {
+				logger.Infof("updated API connection settings cache")
+			}
 		}
 	}
-	return val.st, nil
+	return val, nil
 }
 
 func errorImportance(err error) int {
@@ -230,10 +242,10 @@
 
 // apiInfoConnect looks for endpoint on the given environment and
 // tries to connect to it, sending the result on the returned channel.
-func apiInfoConnect(store configstore.Storage, info configstore.EnvironInfo, stop <-chan struct{}) (apiState, error) {
+func apiInfoConnect(store configstore.Storage, info configstore.EnvironInfo, apiOpen apiOpenFunc, stop <-chan struct{}) (apiState, error) {
 	endpoint := info.APIEndpoint()
 	if info == nil || len(endpoint.Addresses) == 0 {
-		return apiState{}, &infoConnectError{fmt.Errorf("no cached addresses")}
+		return nil, &infoConnectError{fmt.Errorf("no cached addresses")}
 	}
 	logger.Infof("connecting to API addresses: %v", endpoint.Addresses)
 	apiInfo := &api.Info{
@@ -244,9 +256,19 @@
 	}
 	st, err := apiOpen(apiInfo, api.DefaultDialOpts())
 	if err != nil {
-		return apiState{}, &infoConnectError{err}
-	}
-	return apiState{st, nil}, err
+		return nil, &infoConnectError{err}
+	}
+	var addrs []string
+	for _, serverHostPorts := range st.APIHostPorts() {
+		for _, hostPort := range serverHostPorts {
+			addrs = append(addrs, hostPort.NetAddr())
+		}
+	}
+	// Update API addresses if they've changed. Error is non-fatal.
+	if err := cacheChangedAPIAddresses(info, addrs); err != nil {
+		logger.Warningf("cannot cache API addresses: %v", err.Error())
+	}
+	return st, err
 }
 
 // apiConfigConnect looks for configuration info on the given environment,
@@ -254,7 +276,7 @@
 // its endpoint. It only starts the attempt after the given delay,
 // to allow the faster apiInfoConnect to hopefully succeed first.
 // It returns nil if there was no configuration information found.
-func apiConfigConnect(info configstore.EnvironInfo, envs *environs.Environs, envName string, stop <-chan struct{}, delay time.Duration) (apiState, error) {
+func apiConfigConnect(info configstore.EnvironInfo, envs *environs.Environs, envName string, apiOpen apiOpenFunc, stop <-chan struct{}, delay time.Duration) (apiState, error) {
 	var cfg *config.Config
 	var err error
 	if info != nil && len(info.BootstrapConfig()) > 0 {
@@ -262,30 +284,30 @@
 	} else if envs != nil {
 		cfg, err = envs.Config(envName)
 		if errors.IsNotFoundError(err) {
-			return apiState{}, err
+			return nil, err
 		}
 	} else {
-		return apiState{}, errors.NotFoundf("environment %q", envName)
+		return nil, errors.NotFoundf("environment %q", envName)
 	}
 	select {
 	case <-time.After(delay):
 	case <-stop:
-		return apiState{}, errAborted
+		return nil, errAborted
 	}
 	environ, err := environs.New(cfg)
 	if err != nil {
-		return apiState{}, err
+		return nil, err
 	}
 	apiInfo, err := environAPIInfo(environ)
 	if err != nil {
-		return apiState{}, err
+		return nil, err
 	}
 	st, err := apiOpen(apiInfo, api.DefaultDialOpts())
 	// TODO(rog): handle errUnauthorized when the API handles passwords.
 	if err != nil {
-		return apiState{}, err
+		return nil, err
 	}
-	return apiState{st, apiInfo}, nil
+	return apiStateCachedInfo{st, apiInfo}, nil
 }
 
 func environAPIInfo(environ environs.Environ) (*api.Info, error) {
@@ -312,14 +334,42 @@
 	})
 	_, username, err := names.ParseTag(apiInfo.Tag, names.UserTagKind)
 	if err != nil {
-		return fmt.Errorf("not caching API connection settings: invalid API user tag: %v", err)
+		return fmt.Errorf("invalid API user tag: %v", err)
 	}
 	info.SetAPICredentials(configstore.APICredentials{
 		User:     username,
 		Password: apiInfo.Password,
 	})
+	return info.Write()
+}
+
+// cacheChangedAPIAddresses updates the local environment settings (.jenv file)
+// with the provided API server addresses if they have changed.
+func cacheChangedAPIAddresses(info configstore.EnvironInfo, addrs []string) error {
+	endpoint := info.APIEndpoint()
+	if !addrsChanged(endpoint.Addresses, addrs) {
+		return nil
+	}
+	logger.Debugf("API addresses changed from %q to %q", endpoint.Addresses, addrs)
+	endpoint.Addresses = addrs
+	info.SetAPIEndpoint(endpoint)
 	if err := info.Write(); err != nil {
-		return fmt.Errorf("cannot cache API connection settings: %v", err)
+		return err
 	}
+	logger.Infof("updated API connection settings cache")
 	return nil
 }
+
+// addrsChanged returns true iff the two
+// slices are not equal. Order is important.
+func addrsChanged(a, b []string) bool {
+	if len(a) != len(b) {
+		return true
+	}
+	for i := range a {
+		if a[i] != b[i] {
+			return true
+		}
+	}
+	return false
+}

=== modified file 'juju/apiconn_test.go'
--- juju/apiconn_test.go	2014-03-19 22:16:15 +0000
+++ juju/apiconn_test.go	2014-03-31 12:32:07 +0000
@@ -8,7 +8,6 @@
 	"os"
 	"time"
 
-	"github.com/juju/testing"
 	jc "github.com/juju/testing/checkers"
 	gc "launchpad.net/gocheck"
 
@@ -18,6 +17,7 @@
 	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/environs/configstore"
 	envtesting "launchpad.net/juju-core/environs/testing"
+	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/juju"
 	"launchpad.net/juju-core/juju/osenv"
 	"launchpad.net/juju-core/provider/dummy"
@@ -131,8 +131,12 @@
 	store := newConfigStore("noconfig", storeConfig)
 
 	called := 0
-	expectState := new(api.State)
-	apiOpen := func(apiInfo *api.Info, opts api.DialOpts) (*api.State, error) {
+	expectState := &mockAPIState{
+		apiHostPorts: [][]instance.HostPort{
+			instance.AddressesWithPort([]instance.Address{instance.NewAddress("0.1.2.3")}, 1234),
+		},
+	}
+	apiOpen := func(apiInfo *api.Info, opts api.DialOpts) (juju.APIState, error) {
 		c.Check(apiInfo.Tag, gc.Equals, "user-foo")
 		c.Check(string(apiInfo.CACert), gc.Equals, "certificated")
 		c.Check(apiInfo.Password, gc.Equals, "foopass")
@@ -140,14 +144,25 @@
 		called++
 		return expectState, nil
 	}
+
 	// Give NewAPIFromStore a store interface that can report when the
-	// config was written to, to ensure the cache isn't updated.
-	s.PatchValue(juju.APIOpen, apiOpen)
+	// config was written to, to check if the cache is updated.
 	mockStore := &storageWithWriteNotify{store: store}
-	st, err := juju.NewAPIFromStore("noconfig", mockStore)
+	st, err := juju.NewAPIFromStore("noconfig", mockStore, apiOpen)
 	c.Assert(err, gc.IsNil)
 	c.Assert(st, gc.Equals, expectState)
 	c.Assert(called, gc.Equals, 1)
+	c.Assert(mockStore.written, jc.IsTrue)
+	info, err := store.ReadInfo("noconfig")
+	c.Assert(err, gc.IsNil)
+	c.Assert(info.APIEndpoint().Addresses, gc.DeepEquals, []string{"0.1.2.3:1234"})
+	mockStore.written = false
+
+	// If APIHostPorts haven't changed, then the store won't be updated.
+	st, err = juju.NewAPIFromStore("noconfig", mockStore, apiOpen)
+	c.Assert(err, gc.IsNil)
+	c.Assert(st, gc.Equals, expectState)
+	c.Assert(called, gc.Equals, 2)
 	c.Assert(mockStore.written, jc.IsFalse)
 }
 
@@ -177,8 +192,8 @@
 	c.Assert(info.APICredentials(), jc.DeepEquals, configstore.APICredentials{})
 
 	called := 0
-	expectState := new(api.State)
-	apiOpen := func(apiInfo *api.Info, opts api.DialOpts) (*api.State, error) {
+	expectState := &mockAPIState{}
+	apiOpen := func(apiInfo *api.Info, opts api.DialOpts) (juju.APIState, error) {
 		c.Check(apiInfo.Tag, gc.Equals, "user-admin")
 		c.Check(string(apiInfo.CACert), gc.Not(gc.Equals), "")
 		c.Check(apiInfo.Password, gc.Equals, "adminpass")
@@ -186,8 +201,7 @@
 		called++
 		return expectState, nil
 	}
-	s.PatchValue(juju.APIOpen, apiOpen)
-	st, err := juju.NewAPIFromStore("myenv", store)
+	st, err := juju.NewAPIFromStore("myenv", store, apiOpen)
 	c.Assert(err, gc.IsNil)
 	c.Assert(st, gc.Equals, expectState)
 	c.Assert(called, gc.Equals, 1)
@@ -209,16 +223,11 @@
 	defer coretesting.MakeEmptyFakeHome(c).Restore()
 	expectErr := fmt.Errorf("an error")
 	store := newConfigStoreWithError(expectErr)
-	s.PatchValue(juju.APIOpen, panicAPIOpen)
-	client, err := juju.NewAPIFromStore("noconfig", store)
+	client, err := juju.NewAPIFromStore("noconfig", store, panicAPIOpen)
 	c.Assert(err, gc.Equals, expectErr)
 	c.Assert(client, gc.IsNil)
 }
 
-func panicAPIOpen(apiInfo *api.Info, opts api.DialOpts) (*api.State, error) {
-	panic("api.Open called unexpectedly")
-}
-
 func (s *NewAPIClientSuite) TestWithInfoNoAddresses(c *gc.C) {
 	defer coretesting.MakeEmptyFakeHome(c).Restore()
 	store := newConfigStore("noconfig", &environInfo{
@@ -227,9 +236,7 @@
 			CACert:    "certificated",
 		},
 	})
-	s.PatchValue(juju.APIOpen, panicAPIOpen)
-
-	st, err := juju.NewAPIFromStore("noconfig", store)
+	st, err := juju.NewAPIFromStore("noconfig", store, panicAPIOpen)
 	c.Assert(err, gc.ErrorMatches, `environment "noconfig" not found`)
 	c.Assert(st, gc.IsNil)
 }
@@ -243,11 +250,10 @@
 	})
 
 	expectErr := fmt.Errorf("an error")
-	apiOpen := func(apiInfo *api.Info, opts api.DialOpts) (*api.State, error) {
+	apiOpen := func(apiInfo *api.Info, opts api.DialOpts) (juju.APIState, error) {
 		return nil, expectErr
 	}
-	s.PatchValue(juju.APIOpen, apiOpen)
-	st, err := juju.NewAPIFromStore("noconfig", store)
+	st, err := juju.NewAPIFromStore("noconfig", store, apiOpen)
 	c.Assert(err, gc.Equals, expectErr)
 	c.Assert(st, gc.IsNil)
 }
@@ -258,27 +264,30 @@
 	bootstrapEnv(c, coretesting.SampleEnvName, store)
 	setEndpointAddress(c, store, coretesting.SampleEnvName, "infoapi.invalid")
 
-	infoOpenedState := new(api.State)
+	infoOpenedState := &mockAPIState{}
 	infoEndpointOpened := make(chan struct{})
-	cfgOpenedState := new(api.State)
+	cfgOpenedState := &mockAPIState{}
 	// On a sample run with no delay, the logic took 45ms to run, so
 	// we make the delay slightly more than that, so that if the
 	// logic doesn't delay at all, the test will fail reasonably consistently.
 	s.PatchValue(juju.ProviderConnectDelay, 50*time.Millisecond)
-	apiOpen := func(info *api.Info, opts api.DialOpts) (*api.State, error) {
+	apiOpen := func(info *api.Info, opts api.DialOpts) (juju.APIState, error) {
 		if info.Addrs[0] == "infoapi.invalid" {
 			infoEndpointOpened <- struct{}{}
 			return infoOpenedState, nil
 		}
 		return cfgOpenedState, nil
 	}
-	s.PatchValue(juju.APIOpen, apiOpen)
 
-	stateClosed, restoreAPIClose := setAPIClosed()
-	defer restoreAPIClose.Restore()
+	stateClosed := make(chan juju.APIState)
+	infoOpenedState.close = func(st juju.APIState) error {
+		stateClosed <- st
+		return nil
+	}
+	cfgOpenedState.close = infoOpenedState.close
 
 	startTime := time.Now()
-	st, err := juju.NewAPIFromStore(coretesting.SampleEnvName, store)
+	st, err := juju.NewAPIFromStore(coretesting.SampleEnvName, store, apiOpen)
 	c.Assert(err, gc.IsNil)
 	// The connection logic should wait for some time before opening
 	// the API from the configuration.
@@ -320,13 +329,13 @@
 	bootstrapEnv(c, coretesting.SampleEnvName, store)
 	setEndpointAddress(c, store, coretesting.SampleEnvName, "infoapi.invalid")
 
-	infoOpenedState := new(api.State)
+	infoOpenedState := &mockAPIState{}
 	infoEndpointOpened := make(chan struct{})
-	cfgOpenedState := new(api.State)
+	cfgOpenedState := &mockAPIState{}
 	cfgEndpointOpened := make(chan struct{})
 
 	s.PatchValue(juju.ProviderConnectDelay, 0*time.Second)
-	apiOpen := func(info *api.Info, opts api.DialOpts) (*api.State, error) {
+	apiOpen := func(info *api.Info, opts api.DialOpts) (juju.APIState, error) {
 		if info.Addrs[0] == "infoapi.invalid" {
 			infoEndpointOpened <- struct{}{}
 			<-infoEndpointOpened
@@ -336,14 +345,17 @@
 		<-cfgEndpointOpened
 		return cfgOpenedState, nil
 	}
-	s.PatchValue(juju.APIOpen, apiOpen)
 
-	stateClosed, restoreAPIClose := setAPIClosed()
-	defer restoreAPIClose.Restore()
+	stateClosed := make(chan juju.APIState)
+	infoOpenedState.close = func(st juju.APIState) error {
+		stateClosed <- st
+		return nil
+	}
+	cfgOpenedState.close = infoOpenedState.close
 
 	done := make(chan struct{})
 	go func() {
-		st, err := juju.NewAPIFromStore(coretesting.SampleEnvName, store)
+		st, err := juju.NewAPIFromStore(coretesting.SampleEnvName, store, apiOpen)
 		c.Check(err, gc.IsNil)
 		c.Check(st, gc.Equals, infoOpenedState)
 		close(done)
@@ -387,14 +399,13 @@
 	setEndpointAddress(c, store, coretesting.SampleEnvName, "infoapi.invalid")
 
 	s.PatchValue(juju.ProviderConnectDelay, 0*time.Second)
-	apiOpen := func(info *api.Info, opts api.DialOpts) (*api.State, error) {
+	apiOpen := func(info *api.Info, opts api.DialOpts) (juju.APIState, error) {
 		if info.Addrs[0] == "infoapi.invalid" {
 			return nil, fmt.Errorf("info connect failed")
 		}
 		return nil, fmt.Errorf("config connect failed")
 	}
-	s.PatchValue(juju.APIOpen, apiOpen)
-	st, err := juju.NewAPIFromStore(coretesting.SampleEnvName, store)
+	st, err := juju.NewAPIFromStore(coretesting.SampleEnvName, store, apiOpen)
 	c.Check(err, gc.ErrorMatches, "config connect failed")
 	c.Check(st, gc.IsNil)
 }
@@ -427,7 +438,10 @@
 	err = os.Remove(osenv.JujuHomePath("environments.yaml"))
 	c.Assert(err, gc.IsNil)
 
-	st, err := juju.NewAPIFromStore(coretesting.SampleEnvName, store)
+	apiOpen := func(*api.Info, api.DialOpts) (juju.APIState, error) {
+		return &mockAPIState{}, nil
+	}
+	st, err := juju.NewAPIFromStore(coretesting.SampleEnvName, store, apiOpen)
 	c.Check(err, gc.IsNil)
 	st.Close()
 }
@@ -455,7 +469,10 @@
 	// Now we have info for envName2 which will actually
 	// cause a connection to the originally bootstrapped
 	// state.
-	st, err := juju.NewAPIFromStore(envName2, store)
+	apiOpen := func(*api.Info, api.DialOpts) (juju.APIState, error) {
+		return &mockAPIState{}, nil
+	}
+	st, err := juju.NewAPIFromStore(envName2, store, apiOpen)
 	c.Check(err, gc.IsNil)
 	st.Close()
 
@@ -478,19 +495,6 @@
 	c.Assert(envInfo.Name, gc.Equals, expectName)
 }
 
-func setAPIClosed() (<-chan *api.State, testing.Restorer) {
-	stateClosed := make(chan *api.State)
-	apiClose := func(st *api.State) error {
-		stateClosed <- st
-		return nil
-	}
-	return stateClosed, testing.PatchValue(juju.APIClose, apiClose)
-}
-
-func updateSecretsNoop(_ environs.Environ, _ *api.State) error {
-	return nil
-}
-
 // newConfigStoreWithError that will return the given
 // error from ReadInfo.
 func newConfigStoreWithError(err error) configstore.Storage {

=== modified file 'juju/export_test.go'
--- juju/export_test.go	2014-03-19 03:18:41 +0000
+++ juju/export_test.go	2014-03-31 12:32:07 +0000
@@ -1,8 +1,21 @@
 package juju
 
+import (
+	"launchpad.net/juju-core/environs/configstore"
+	"launchpad.net/juju-core/state/api"
+)
+
 var (
-	APIOpen              = &apiOpen
-	APIClose             = &apiClose
 	ProviderConnectDelay = &providerConnectDelay
-	NewAPIFromStore      = newAPIFromStore
 )
+
+type APIState apiState
+
+type APIOpenFunc func(*api.Info, api.DialOpts) (APIState, error)
+
+func NewAPIFromStore(envName string, store configstore.Storage, f APIOpenFunc) (APIState, error) {
+	apiOpen := func(info *api.Info, opts api.DialOpts) (apiState, error) {
+		return f(info, opts)
+	}
+	return newAPIFromStore(envName, store, apiOpen)
+}

=== added file 'juju/mock_test.go'
--- juju/mock_test.go	1970-01-01 00:00:00 +0000
+++ juju/mock_test.go	2014-03-31 12:32:07 +0000
@@ -0,0 +1,28 @@
+package juju_test
+
+import (
+	"launchpad.net/juju-core/instance"
+	"launchpad.net/juju-core/juju"
+	"launchpad.net/juju-core/state/api"
+)
+
+type mockAPIState struct {
+	close func(juju.APIState) error
+
+	apiHostPorts [][]instance.HostPort
+}
+
+func (s *mockAPIState) Close() error {
+	if s.close != nil {
+		return s.close(s)
+	}
+	return nil
+}
+
+func (s *mockAPIState) APIHostPorts() [][]instance.HostPort {
+	return s.apiHostPorts
+}
+
+func panicAPIOpen(apiInfo *api.Info, opts api.DialOpts) (juju.APIState, error) {
+	panic("api.Open called unexpectedly")
+}

=== modified file 'state/api/apiclient.go'
--- state/api/apiclient.go	2014-03-19 03:18:41 +0000
+++ state/api/apiclient.go	2014-03-31 12:32:07 +0000
@@ -11,6 +11,7 @@
 	"code.google.com/p/go.net/websocket"
 
 	"launchpad.net/juju-core/cert"
+	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/log"
 	"launchpad.net/juju-core/rpc"
 	"launchpad.net/juju-core/rpc/jsoncodec"
@@ -25,7 +26,13 @@
 type State struct {
 	client *rpc.Conn
 	conn   *websocket.Conn
-	addr   string
+
+	// addr is the address used to connect to the API server.
+	addr string
+
+	// hostPorts is the API server addresses returned from Login,
+	// which the client may cache and use for failover.
+	hostPorts [][]instance.HostPort
 
 	// authTag holds the authenticated entity's tag after login.
 	authTag string
@@ -189,7 +196,23 @@
 	return s.client
 }
 
-// Addr returns the address used to connect to the RPC server.
+// Addr returns the address used to connect to the API server.
 func (s *State) Addr() string {
 	return s.addr
 }
+
+// APIHostPorts returns addresses that may be used to connect
+// to the API server, including the address used to connect.
+//
+// The addresses are scoped (public, cloud-internal, etc.), so
+// the client may choose which addresses to attempt. For the
+// Juju CLI, all addresses must be attempted, as the CLI may
+// be invoked both within and outside the environment (think
+// private clouds).
+func (s *State) APIHostPorts() [][]instance.HostPort {
+	hostPorts := make([][]instance.HostPort, len(s.hostPorts))
+	for i, server := range s.hostPorts {
+		hostPorts[i] = append([]instance.HostPort{}, server...)
+	}
+	return hostPorts
+}

=== modified file 'state/api/params/params.go'
--- state/api/params/params.go	2014-03-28 13:31:26 +0000
+++ state/api/params/params.go	2014-03-31 12:32:07 +0000
@@ -640,6 +640,11 @@
 	Servers [][]instance.HostPort
 }
 
+// LoginResult holds the result of a Login call.
+type LoginResult struct {
+	Servers [][]instance.HostPort
+}
+
 // EnsureAvailability contains arguments for
 // the EnsureAvailability client API call.
 type EnsureAvailability struct {

=== modified file 'state/api/state.go'
--- state/api/state.go	2014-02-27 05:24:43 +0000
+++ state/api/state.go	2014-03-31 12:32:07 +0000
@@ -4,6 +4,10 @@
 package api
 
 import (
+	"net"
+	"strconv"
+
+	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/state/api/agent"
 	"launchpad.net/juju-core/state/api/charmrevisionupdater"
 	"launchpad.net/juju-core/state/api/deployer"
@@ -24,17 +28,54 @@
 // method is usually called automatically by Open. The machine nonce
 // should be empty unless logging in as a machine agent.
 func (st *State) Login(tag, password, nonce string) error {
+	var result params.LoginResult
 	err := st.Call("Admin", "", "Login", &params.Creds{
 		AuthTag:  tag,
 		Password: password,
 		Nonce:    nonce,
-	}, nil)
+	}, &result)
 	if err == nil {
 		st.authTag = tag
+		hostPorts, err := addAddress(result.Servers, st.addr)
+		if err != nil {
+			st.Close()
+			return err
+		}
+		st.hostPorts = hostPorts
 	}
 	return err
 }
 
+// addAddress appends a new server derived from the given
+// address to servers if the address is not already found
+// there.
+func addAddress(servers [][]instance.HostPort, addr string) ([][]instance.HostPort, error) {
+	for _, server := range servers {
+		for _, hostPort := range server {
+			if hostPort.NetAddr() == addr {
+				return servers, nil
+			}
+		}
+	}
+	host, portString, err := net.SplitHostPort(addr)
+	if err != nil {
+		return nil, err
+	}
+	port, err := strconv.Atoi(portString)
+	if err != nil {
+		return nil, err
+	}
+	hostPort := instance.HostPort{
+		Address: instance.Address{
+			Value:        host,
+			Type:         instance.DeriveAddressType(host),
+			NetworkScope: instance.NetworkUnknown,
+		},
+		Port: port,
+	}
+	return append(servers, []instance.HostPort{hostPort}), nil
+}
+
 // Client returns an object that can be used
 // to access client-specific functionality.
 func (st *State) Client() *Client {

=== modified file 'state/apiserver/admin.go'
--- state/apiserver/admin.go	2014-01-17 19:33:42 +0000
+++ state/apiserver/admin.go	2014-03-31 12:32:07 +0000
@@ -63,16 +63,16 @@
 // Login logs in with the provided credentials.
 // All subsequent requests on the connection will
 // act as the authenticated user.
-func (a *srvAdmin) Login(c params.Creds) error {
+func (a *srvAdmin) Login(c params.Creds) (params.LoginResult, error) {
 	a.mu.Lock()
 	defer a.mu.Unlock()
 	if a.loggedIn {
 		// This can only happen if Login is called concurrently.
-		return errAlreadyLoggedIn
+		return params.LoginResult{}, errAlreadyLoggedIn
 	}
 	entity, err := checkCreds(a.root.srv.state, c)
 	if err != nil {
-		return err
+		return params.LoginResult{}, err
 	}
 	if a.reqNotifier != nil {
 		a.reqNotifier.login(entity.Tag())
@@ -81,11 +81,18 @@
 	// to serve to them.
 	newRoot, err := a.apiRootForEntity(entity, c)
 	if err != nil {
-		return err
-	}
+		return params.LoginResult{}, err
+	}
+
+	// Fetch the API server addresses from state.
+	hostPorts, err := a.root.srv.state.APIHostPorts()
+	if err != nil {
+		return params.LoginResult{}, err
+	}
+	logger.Debugf("hostPorts: %v", hostPorts)
 
 	a.root.rpcConn.Serve(newRoot, serverError)
-	return nil
+	return params.LoginResult{hostPorts}, nil
 }
 
 func checkCreds(st *state.State, c params.Creds) (taggedAuthenticator, error) {

=== modified file 'state/apiserver/login_test.go'
--- state/apiserver/login_test.go	2014-03-13 13:42:50 +0000
+++ state/apiserver/login_test.go	2014-03-31 12:32:07 +0000
@@ -4,10 +4,14 @@
 package apiserver_test
 
 import (
+	"net"
+	"strconv"
+
 	"github.com/juju/loggo"
 	jc "github.com/juju/testing/checkers"
 	gc "launchpad.net/gocheck"
 
+	"launchpad.net/juju-core/instance"
 	jujutesting "launchpad.net/juju-core/juju/testing"
 	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/state/api"
@@ -160,8 +164,74 @@
 		// Now that we are logged in, we see the entity's tag
 		// [0-9.umns] is to handle timestamps that are ns, us, ms, or s
 		// long, though we expect it to be in the 'ms' range.
-		`-> \[[0-9A-F]+\] machine-0 [0-9.]+[umn]?s {"RequestId":1,"Response":{}} Admin\[""\].Login`,
+		`-> \[[0-9A-F]+\] machine-0 [0-9.]+[umn]?s {"RequestId":1,"Response":{"Servers":\[\]}} Admin\[""\].Login`,
 		`<- \[[0-9A-F]+\] machine-0 {"RequestId":2,"Type":"Machiner","Request":"Life","Params":{"Entities":\[{"Tag":"machine-0"}\]}}`,
 		`-> \[[0-9A-F]+\] machine-0 [0-9.umns]+ {"RequestId":2,"Response":{"Results":\[{"Life":"alive","Error":null}\]}} Machiner\[""\]\.Life`,
 	})
 }
+
+func (s *loginSuite) TestLoginAddrs(c *gc.C) {
+	machine, err := s.State.AddMachine("quantal", state.JobHostUnits)
+	c.Assert(err, gc.IsNil)
+	err = machine.SetProvisioned("foo", "fake_nonce", nil)
+	c.Assert(err, gc.IsNil)
+	password, err := utils.RandomPassword()
+	c.Assert(err, gc.IsNil)
+	err = machine.SetPassword(password)
+	c.Assert(err, gc.IsNil)
+	info, cleanup := s.setupServer(c)
+	defer cleanup()
+	info.Tag = machine.Tag()
+	info.Password = password
+	info.Nonce = "fake_nonce"
+
+	// Initially just the address we connect with is returned,
+	// despite there being no APIHostPorts in state.
+	connectedAddr, hostPorts := s.loginHostPorts(c, info)
+	connectedAddrHost, connectedAddrPortString, err := net.SplitHostPort(connectedAddr)
+	c.Assert(err, gc.IsNil)
+	connectedAddrPort, err := strconv.Atoi(connectedAddrPortString)
+	c.Assert(err, gc.IsNil)
+	connectedAddrHostPorts := [][]instance.HostPort{
+		[]instance.HostPort{{
+			instance.NewAddress(connectedAddrHost),
+			connectedAddrPort,
+		}},
+	}
+	c.Assert(hostPorts, gc.DeepEquals, connectedAddrHostPorts)
+
+	// After storing APIHostPorts in state, Login should store
+	// all of them and the address we connected with.
+	server1Addresses := []instance.Address{{
+		Value:        "server-1",
+		Type:         instance.HostName,
+		NetworkScope: instance.NetworkPublic,
+	}, {
+		Value:        "10.0.0.1",
+		Type:         instance.Ipv4Address,
+		NetworkName:  "internal",
+		NetworkScope: instance.NetworkCloudLocal,
+	}}
+	server2Addresses := []instance.Address{{
+		Value:        "::1",
+		Type:         instance.Ipv6Address,
+		NetworkName:  "loopback",
+		NetworkScope: instance.NetworkMachineLocal,
+	}}
+	stateAPIHostPorts := [][]instance.HostPort{
+		instance.AddressesWithPort(server1Addresses, 123),
+		instance.AddressesWithPort(server2Addresses, 456),
+	}
+	err = s.State.SetAPIHostPorts(stateAPIHostPorts)
+	c.Assert(err, gc.IsNil)
+	connectedAddr, hostPorts = s.loginHostPorts(c, info)
+	stateAPIHostPorts = append(stateAPIHostPorts, connectedAddrHostPorts...)
+	c.Assert(hostPorts, gc.DeepEquals, stateAPIHostPorts)
+}
+
+func (s *loginSuite) loginHostPorts(c *gc.C, info *api.Info) (connectedAddr string, hostPorts [][]instance.HostPort) {
+	st, err := api.Open(info, fastDialOpts)
+	c.Assert(err, gc.IsNil)
+	defer st.Close()
+	return st.Addr(), st.APIHostPorts()
+}