Merge lp:~axwalk/juju-core/agent-setapihostports into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

agent: add ConfigSetterOnly.SetAPIHostPorts

We still only store addresses in the config,
so we lose some metadata. We make use of the
scope metadata before storing, though, to
prune the addresses to the internal ones.

https://codereview.appspot.com/82520043/

agent: add ConfigSetterOnly.SetAPIHostPorts

We still only store addresses in the config,
so we lose some metadata. We make use of the
scope metadata before storing, though, to
prune the addresses to the internal ones.

https://codereview.appspot.com/82520043/

[Dave Cheney (dave-cheney)]

+ conf.SetAPIHostPorts([][]instance.HostPort{
+ instance.AddressesWithPort(server1, 123),
+ instance.AddressesWithPort(server2, 124),
+ instance.AddressesWithPort(server3, 125),
+ })
+ 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"})

Does conf.APIAddresses return a set or an ordered list ?

[Andrew Wilkins (axwalk)]

Reviewers: mp+213409_code.launchpad.net,

Message:
Please take a look.

Description:
agent: add ConfigSetterOnly.SetAPIHostPorts

We still only store addresses in the config,
so we lose some metadata. We make use of the
scope metadata before storing, though, to
prune the addresses to the internal ones.

https://code.launchpad.net/~axwalk/juju-core/agent-setapihostports/+merge/213409

(do not edit description out of merge proposal)

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

Affected files (+54, -0 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-20140330221944-tt65zj7hl6967aad
+New revision: <email address hidden>

Index: agent/agent.go
=== modified file 'agent/agent.go'
--- agent/agent.go 2014-03-28 08:55:45 +0000
+++ agent/agent.go 2014-03-31 06:30:43 +0000
@@ -16,6 +16,7 @@
   "github.com/errgo/errgo"
   "github.com/juju/loggo"

+ "launchpad.net/juju-core/instance"
   "launchpad.net/juju-core/state"
   "launchpad.net/juju-core/state/api"
   "launchpad.net/juju-core/state/api/params"
@@ -133,6 +134,9 @@
   // the agent has successfully upgraded to.
   SetUpgradedToVersion(newVersion version.Number)

+ // SetAPIHostPorts sets the API host/port addresses to connect to.
+ SetAPIHostPorts(servers [][]instance.HostPort)
+
   // Migrate takes an existing agent config and applies the given
   // parameters to change it.
   //
@@ -423,6 +427,20 @@
   c.upgradedToVersion = newVersion
  }

+func (c *configInternal) SetAPIHostPorts(servers [][]instance.HostPort) {
+ if c.apiDetails == nil {
+ return
+ }
+ var addrs []string
+ for _, serverHostPorts := range servers {
+ addr := instance.SelectInternalHostPort(serverHostPorts, false)
+ if addr != "" {
+ addrs = append(addrs, addr)
+ }
+ }
+ c.apiDetails.addresses = addrs
+}
+
  func (c *configInternal) SetValue(key, value string) {
   if value == "" {
    delete(c.values, key)

Index: agent/agent_test.go
=== modified file 'agent/agent_test.go'
--- agent/agent_test.go 2014-03-28 08:28:04 +0000
+++ agent/agent_test.go 2014-03-31 06:30:43 +0000
@@ -10,6 +10,7 @@
   gc "launchpad.net/gocheck"

   "launchpad.net/juju-core/agent"
+ "launchpad.net/juju-core/instance"
   "launchpad.net/juju-core/state"
   "launchpad.net/juju-core/state/api"
   "launchpad.net/juju-core/state/api/params"
@@ -430,3 +431,36 @@
   conf.SetUpgradedToVersion(expectVers)
   c.Assert(conf.UpgradedToVersion(), gc.Equals, expectVers)
  }
+
+func (*suite) TestSetAPIHostPorts(c *gc.C) {
+ conf, err := agent.NewAgentConfig(attributeParams)
+ c.Assert(err, gc.IsNil)
+
+ addrs, err := conf.APIAddresses()
+ 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.
+ server1 := instan...

[Dave Cheney (dave-cheney)]

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

https://codereview.appspot.com/82520043/diff/1/agent/agent_test.go#newcode465
agent/agent_test.go:465: c.Assert(addrs, gc.DeepEquals,
[]string{"0.1.2.3:123", "0.1.2.5:125"})
Does conf.APIAddresses return a set or an ordered list ?

https://codereview.appspot.com/82520043/

[Andrew Wilkins (axwalk)]

On 2014/03/31 06:44:02, dfc wrote:
> https://codereview.appspot.com/82520043/diff/1/agent/agent_test.go
> File agent/agent_test.go (right):

https://codereview.appspot.com/82520043/diff/1/agent/agent_test.go#newcode465
> agent/agent_test.go:465: c.Assert(addrs, gc.DeepEquals,
[]string{"0.1.2.3:123",
> "0.1.2.5:125"})
> Does conf.APIAddresses return a set or an ordered list ?

APIAddresses returns the same order as specified in the list in the
config file.
I'm told order is important for addresses, which is why I've done it
with DeepEquals.

https://codereview.appspot.com/82520043/

[Dave Cheney (dave-cheney)]

Fair enough.

On Mon, Mar 31, 2014 at 6:24 PM, <email address hidden> wrote:
> On 2014/03/31 06:44:02, dfc wrote:
>>
>> https://codereview.appspot.com/82520043/diff/1/agent/agent_test.go
>> File agent/agent_test.go (right):
>
>
>
> https://codereview.appspot.com/82520043/diff/1/agent/agent_test.go#newcode465
>>
>> agent/agent_test.go:465: c.Assert(addrs, gc.DeepEquals,
>
> []string{"0.1.2.3:123",
>>
>> "0.1.2.5:125"})
>> Does conf.APIAddresses return a set or an ordered list ?
>
>
> APIAddresses returns the same order as specified in the list in the
> config file.
> I'm told order is important for addresses, which is why I've done it
> with DeepEquals.
>
> https://codereview.appspot.com/82520043/

[Dimiter Naydenov (dimitern)]

LGTM

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

https://codereview.appspot.com/82520043/diff/1/agent/agent.go#newcode432
agent/agent.go:432: return
Why return here rather than creating apiDetails?

https://codereview.appspot.com/82520043/

[John A Meinel (jameinel)]

This feels like an incorrect layer to be doing the validation of
addresses.

I won't block it, but I'd like you to consider if the config level
should just take a list, write it down, and read it back again, and
instead the callers of this would be the ones doing the filtering.

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

https://codereview.appspot.com/82520043/diff/1/agent/agent.go#newcode435
agent/agent.go:435: for _, serverHostPorts := range servers {
It feels like the wrong layer for *configInternal* to be the thing that
decides that we save CloudLocal only addresses.

Shouldn't this be done in the layer *above* config, and config is just
about "write this stuff to disk and read it back again" ?

https://codereview.appspot.com/82520043/

[Andrew Wilkins (axwalk)]

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

https://codereview.appspot.com/82520043/diff/1/agent/agent.go#newcode432
agent/agent.go:432: return
On 2014/03/31 11:57:11, dimitern wrote:
> Why return here rather than creating apiDetails?

Because apiDetails should be != nil if we're the API (i.e. always). We
should probably just error out if Addresses is empty in NewAgentConfig.

https://codereview.appspot.com/82520043/diff/1/agent/agent.go#newcode435
agent/agent.go:435: for _, serverHostPorts := range servers {
On 2014/03/31 12:26:15, jameinel wrote:
> It feels like the wrong layer for *configInternal* to be the thing
that decides
> that we save CloudLocal only addresses.

> Shouldn't this be done in the layer *above* config, and config is just
about
> "write this stuff to disk and read it back again" ?

You're right, I think, but I think it'd be unhelpful to change that now.
Eventually we should store [][]instance.HostPort in agent.conf, and then
use that in api.Open.

https://codereview.appspot.com/82520043/

=== modified file 'agent/agent.go'
--- agent/agent.go	2014-03-28 08:55:45 +0000
+++ agent/agent.go	2014-03-31 06:35:35 +0000
@@ -16,6 +16,7 @@
 	"github.com/errgo/errgo"
 	"github.com/juju/loggo"
 
+	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/state/api"
 	"launchpad.net/juju-core/state/api/params"
@@ -133,6 +134,9 @@
 	// the agent has successfully upgraded to.
 	SetUpgradedToVersion(newVersion version.Number)
 
+	// SetAPIHostPorts sets the API host/port addresses to connect to.
+	SetAPIHostPorts(servers [][]instance.HostPort)
+
 	// Migrate takes an existing agent config and applies the given
 	// parameters to change it.
 	//
@@ -423,6 +427,20 @@
 	c.upgradedToVersion = newVersion
 }
 
+func (c *configInternal) SetAPIHostPorts(servers [][]instance.HostPort) {
+	if c.apiDetails == nil {
+		return
+	}
+	var addrs []string
+	for _, serverHostPorts := range servers {
+		addr := instance.SelectInternalHostPort(serverHostPorts, false)
+		if addr != "" {
+			addrs = append(addrs, addr)
+		}
+	}
+	c.apiDetails.addresses = addrs
+}
+
 func (c *configInternal) SetValue(key, value string) {
 	if value == "" {
 		delete(c.values, key)
 
=== modified file 'agent/agent_test.go'
--- agent/agent_test.go	2014-03-28 08:28:04 +0000
+++ agent/agent_test.go	2014-03-31 06:35:35 +0000
@@ -10,6 +10,7 @@
 	gc "launchpad.net/gocheck"
 
 	"launchpad.net/juju-core/agent"
+	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/state/api"
 	"launchpad.net/juju-core/state/api/params"
@@ -430,3 +431,36 @@
 	conf.SetUpgradedToVersion(expectVers)
 	c.Assert(conf.UpgradedToVersion(), gc.Equals, expectVers)
 }
+
+func (*suite) TestSetAPIHostPorts(c *gc.C) {
+	conf, err := agent.NewAgentConfig(attributeParams)
+	c.Assert(err, gc.IsNil)
+
+	addrs, err := conf.APIAddresses()
+	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.
+	server1 := instance.NewAddresses("0.1.2.3", "0.1.2.4", "zeroonetwothree")
+	server1[0].NetworkScope = instance.NetworkCloudLocal
+	server1[1].NetworkScope = instance.NetworkCloudLocal
+	server1[2].NetworkScope = instance.NetworkPublic
+	server2 := instance.NewAddresses("127.0.0.1")
+	server2[0].NetworkScope = instance.NetworkMachineLocal
+	server3 := instance.NewAddresses("0.1.2.5", "zeroonetwofive")
+	server3[0].NetworkScope = instance.NetworkUnknown
+	server3[1].NetworkScope = instance.NetworkUnknown
+	conf.SetAPIHostPorts([][]instance.HostPort{
+		instance.AddressesWithPort(server1, 123),
+		instance.AddressesWithPort(server2, 124),
+		instance.AddressesWithPort(server3, 125),
+	})
+	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"})
+}