Merge lp://staging/~hduran-8/juju-core/apiworker_force_local_only into lp://staging/~go-bot/juju-core/trunk

Reviewers: mp+221788_code.launchpad.net,

Message:
Please take a look.

Description:
Use localhost only if pressent for opening state

Recently a change was made so localhost is added in the list
of addresses to open state if we are servingstatus.
In that change the addresses where sorted to favor localhost
ones, with this new change, if localhost is present, its the
only one used and the other ones are discarded.

https://code.launchpad.net/~hduran-8/juju-core/apiworker_force_local_only/+merge/221788

(do not edit description out of merge proposal)

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

Affected files (+11, -66 lines):
   A [revision details]
   M state/api/apiclient.go
   M state/api/apiclient_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20140602172913-xpffu1jxxw059h3p
+New revision: <email address hidden>

Index: state/api/apiclient.go
=== modified file 'state/api/apiclient.go'
--- state/api/apiclient.go 2014-05-30 16:12:13 +0000
+++ state/api/apiclient.go 2014-06-02 20:54:43 +0000
@@ -8,7 +8,6 @@
   "crypto/x509"
   "fmt"
   "io"
- "sort"
   "strings"
   "time"

@@ -110,18 +109,6 @@
   }
  }

-type LocalFirst []string
-
-func (l LocalFirst) Len() int {
- return len(l)
-}
-func (l LocalFirst) Swap(i, j int) {
- l[i], l[j] = l[j], l[i]
-}
-func (l LocalFirst) Less(i, j int) bool {
- return strings.HasPrefix(l[i], "localhost")
&& !strings.HasPrefix(l[j], "localhost")
-}
-
  func Open(info *Info, opts DialOpts) (*State, error) {
   if len(info.Addrs) == 0 {
    return nil, fmt.Errorf("no API addresses to connect to")
@@ -137,8 +124,15 @@
   try := parallel.NewTry(0, nil)
   defer try.Kill()
   var addrs []string
- addrs = append(addrs, info.Addrs...)
- sort.Sort(LocalFirst(addrs))
+ for _, addr := range info.Addrs {
+ if strings.HasPrefix(addr, "localhost:") {
+ addrs = append(addrs, addr)
+ break
+ }
+ }
+ if len(addrs) == 0 {
+ addrs = info.Addrs
+ }
   for _, addr := range addrs {
    err := dialWebsocket(addr, opts, pool, try)
    if err == parallel.ErrStopped {

Index: state/api/apiclient_test.go
=== modified file 'state/api/apiclient_test.go'
--- state/api/apiclient_test.go 2014-05-30 16:12:13 +0000
+++ state/api/apiclient_test.go 2014-06-02 20:39:13 +0000
@@ -6,7 +6,6 @@
  import (
   "io"
   "net"
- "sort"

   gc "launchpad.net/gocheck"

@@ -21,56 +20,6 @@

  var _ = gc.Suite(&apiclientSuite{})

-func (s *apiclientSuite) TestSortLocalhost(c *gc.C) {
- addrs := []string{
- "notlocalhost1",
- "notlocalhost2",
- "notlocalhost3",
- "localhost1",
- "localhost2",
- "localhost3",
- }
- expectedAddrs := []string{
- "localhost1",
- "localhost2",
- "localhost3",
- "notlocalhost1",
- "notlocalhost2",
- "notlocalhost3",
- }
- var sortedAddrs []string
- sortedAddrs = append(sortedAddrs, addrs...)
- sort.Sort(api.LocalFirst(sortedAddrs))
- c.Assert(addrs, gc.Not(gc.DeepEquals), sortedAddrs)
- c.Assert(sortedAddrs, gc.HasLen, 6)
- c.Assert(sortedAddrs, gc.DeepEquals, expectedAddrs)
-
-}
-
-func (s *apicl...

given that localhost should not be in the list more than once, LGTM

bonus points if you fix the test not to use a hard coded port number in
a test you didn't touch :-)

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

https://codereview.appspot.com/103820044/diff/1/state/api/apiclient_test.go#newcode30
state/api/apiclient_test.go:30: listener, err := net.Listen("tcp",
"localhost:26104")
hard coded port number in the test? really?

https://codereview.appspot.com/103820044/

Please take a look.

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

https://codereview.appspot.com/103820044/diff/1/state/api/apiclient_test.go#newcode30
state/api/apiclient_test.go:30: listener, err := net.Listen("tcp",
"localhost:26104")
On 2014/06/02 21:08:23, thumper wrote:
> hard coded port number in the test? really?

Done.

https://codereview.appspot.com/103820044/

LGTM, just wondering about cleaning up that test. You are good to land
if you cannot use the result of the listen address.

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

https://codereview.appspot.com/103820044/diff/20001/state/api/apiclient_test.go#newcode48
state/api/apiclient_test.go:48: _, port, err :=
net.SplitHostPort(listenerAddress)
Can't you just use this as the expectedHostPort?

https://codereview.appspot.com/103820044/

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

https://codereview.appspot.com/103820044/diff/20001/state/api/apiclient_test.go#newcode48
state/api/apiclient_test.go:48: _, port, err :=
net.SplitHostPort(listenerAddress)
On 2014/06/03 01:38:50, thumper wrote:
> Can't you just use this as the expectedHostPort?

No, even though I start the listener using localhost, the Addr method
returns the ip, so I get the port and use it.

https://codereview.appspot.com/103820044/

https://codereview.appspot.com/103820044/

LGTM

A comment about "listenAddress contains the actual IP address, but
APIHostPorts is going to report localhost, so just find the port"

https://codereview.appspot.com/103820044/