Merge lp:~natefinch/juju-core/051-agent-addresses into lp:~go-bot/juju-core/trunk

[Nate Finch]

API should report all non-machine-local addresses

To support the manual provider, we need to report all addresses that are not machine-local for the API servers. Previously, we were only reporting cloud-local addresses, which often will not work if you're using the manual provider.

https://codereview.appspot.com/91270044/

[Nate Finch (natefinch)]

Reviewers: mp+218878_code.launchpad.net,

Message:
Please take a look.

Description:
API should report all non-machine-local addresses

To support the manual provider, we need to report all addresses that are
not machine-local for the API servers. Previously, we were only
reporting cloud-local addresses, which often will not work if you're
using the manual provider.

https://code.launchpad.net/~natefinch/juju-core/051-agent-addresses/+merge/218878

(do not edit description out of merge proposal)

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

Affected files (+8, -10 lines):
   A [revision details]
   M agent/agent.go
   M agent/agent_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-20140507125024-3qwgzgd4kzdr6prt
+New revision: <email address hidden>

Index: agent/agent.go
=== modified file 'agent/agent.go'
--- agent/agent.go 2014-04-23 08:50:28 +0000
+++ agent/agent.go 2014-05-08 20:12:21 +0000
@@ -440,9 +440,10 @@
   }
   var addrs []string
   for _, serverHostPorts := range servers {
- addr := instance.SelectInternalHostPort(serverHostPorts, false)
- if addr != "" {
- addrs = append(addrs, addr)
+ for _, hp := range serverHostPorts {
+ if hp.NetworkScope != instance.NetworkMachineLocal {
+ addrs = append(addrs, hp.NetAddr())
+ }
    }
   }
   c.apiDetails.addresses = addrs

Index: agent/agent_test.go
=== modified file 'agent/agent_test.go'
--- agent/agent_test.go 2014-04-23 08:50:28 +0000
+++ agent/agent_test.go 2014-05-08 20:12:21 +0000
@@ -508,12 +508,7 @@
   c.Assert(err, gc.IsNil)
   c.Assert(addrs, gc.DeepEquals, attributeParams.APIAddresses)

- // The first cloud-local address for each server is used,
- // else if there are none then the first public- or unknown-
- // scope address.
- //
- // If a server has only machine-local addresses, or none
- // at all, then it will be excluded.
+ // All addresses which are not machine-local will be used.
   server1 := instance.NewAddresses("0.1.2.3", "0.1.2.4", "zeroonetwothree")
   server1[0].NetworkScope = instance.NetworkCloudLocal
   server1[1].NetworkScope = instance.NetworkCloudLocal
@@ -530,5 +525,5 @@
   })
   addrs, err = conf.APIAddresses()
   c.Assert(err, gc.IsNil)
- c.Assert(addrs, gc.DeepEquals, []string{"0.1.2.3:123", "0.1.2.5:125"})
+ c.Assert(addrs, jc.SameContents,
[]string{"0.1.2.3:123", "0.1.2.5:125", "0.1.2.4:123", "zeroonetwothree:123", "zeroonetwofive:125"})
  }

[William Reade (fwereade)]

LGTM.

Only quibble is that it might be worth extracting that new logic to a
selection func that can live next to the original one.

https://codereview.appspot.com/91270044/diff/1/agent/agent.go
File agent/agent.go (right):

https://codereview.appspot.com/91270044/diff/1/agent/agent.go#newcode444
agent/agent.go:444: if hp.NetworkScope != instance.NetworkMachineLocal {
is there any possibility this will fail on the local provider?

https://codereview.appspot.com/91270044/diff/1/agent/agent_test.go
File agent/agent_test.go (left):

https://codereview.appspot.com/91270044/diff/1/agent/agent_test.go#oldcode516
agent/agent_test.go:516: // at all, then it will be excluded.
ok, I guess if machine-local was definitely already excluded we're fine
:)

https://codereview.appspot.com/91270044/

[John A Meinel (jameinel)]

On 2014/05/08 21:07:17, fwereade wrote:
> LGTM.

> Only quibble is that it might be worth extracting that new logic to a
selection
> func that can live next to the original one.

> https://codereview.appspot.com/91270044/diff/1/agent/agent.go
> File agent/agent.go (right):

https://codereview.appspot.com/91270044/diff/1/agent/agent.go#newcode444
> agent/agent.go:444: if hp.NetworkScope != instance.NetworkMachineLocal
{
> is there any possibility this will fail on the local provider?

> https://codereview.appspot.com/91270044/diff/1/agent/agent_test.go
> File agent/agent_test.go (left):

https://codereview.appspot.com/91270044/diff/1/agent/agent_test.go#oldcode516
> agent/agent_test.go:516: // at all, then it will be excluded.
> ok, I guess if machine-local was definitely already excluded we're
fine :)

I'd be happier if we were doing HostPorts storage on this, so that we
could see the list of ones to try first, and then what to try second
(cloud-local first, etc).
Are there plans to push all the way to that point? This could be an ok
bridge-the-gap, I just worry that it will be good-enough-for-now and we
don't actually fix the underlying issues.

https://codereview.appspot.com/91270044/

[Nate Finch (natefinch)]

On 2014/05/09 09:15:52, jameinel wrote:
> I'd be happier if we were doing HostPorts storage on this, so that we
could see
> the list of ones to try first, and then what to try second
(cloud-local first,
> etc).
> Are there plans to push all the way to that point? This could be an ok
> bridge-the-gap, I just worry that it will be good-enough-for-now and
we don't
> actually fix the underlying issues.

I hadn't had plans to do that, though I do agree it's a better approach.
  I can do that if we think it's worth a little extra work.

https://codereview.appspot.com/91270044/

[William Reade (fwereade)]

OK, let's go with hostport storage for consistency's sake as well.

https://codereview.appspot.com/91270044/

Unmerged revisions

2705. By Nate Finch on 2014-05-08

report all addresses that aren't machine-local for API addresses

=== modified file 'agent/agent.go'
--- agent/agent.go	2014-04-23 08:50:28 +0000
+++ agent/agent.go	2014-05-08 20:22:57 +0000
@@ -440,9 +440,10 @@
 	}
 	var addrs []string
 	for _, serverHostPorts := range servers {
-		addr := instance.SelectInternalHostPort(serverHostPorts, false)
-		if addr != "" {
-			addrs = append(addrs, addr)
+		for _, hp := range serverHostPorts {
+			if hp.NetworkScope != instance.NetworkMachineLocal {
+				addrs = append(addrs, hp.NetAddr())
+			}
 		}
 	}
 	c.apiDetails.addresses = addrs

=== modified file 'agent/agent_test.go'
--- agent/agent_test.go	2014-04-23 08:50:28 +0000
+++ agent/agent_test.go	2014-05-08 20:22:57 +0000
@@ -508,12 +508,7 @@
 	c.Assert(err, gc.IsNil)
 	c.Assert(addrs, gc.DeepEquals, attributeParams.APIAddresses)
 
-	// The first cloud-local address for each server is used,
-	// else if there are none then the first public- or unknown-
-	// scope address.
-	//
-	// If a server has only machine-local addresses, or none
-	// at all, then it will be excluded.
+	// All addresses which are not machine-local will be used.
 	server1 := instance.NewAddresses("0.1.2.3", "0.1.2.4", "zeroonetwothree")
 	server1[0].NetworkScope = instance.NetworkCloudLocal
 	server1[1].NetworkScope = instance.NetworkCloudLocal
@@ -530,5 +525,5 @@
 	})
 	addrs, err = conf.APIAddresses()
 	c.Assert(err, gc.IsNil)
-	c.Assert(addrs, gc.DeepEquals, []string{"0.1.2.3:123", "0.1.2.5:125"})
+	c.Assert(addrs, jc.SameContents, []string{"0.1.2.3:123", "0.1.2.5:125", "0.1.2.4:123", "zeroonetwothree:123", "zeroonetwofive:125"})
 }