juju-core

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

  1. agent-setapihostports
  2. Merge into trunk
Proposed by Andrew Wilkins
Status: Merged
Approved by: Andrew Wilkins
Approved revision: no longer in the source branch.
Merged at revision: 2518
Proposed branch: lp:~axwalk/juju-core/agent-setapihostports
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 91 lines (+52/-0)
2 files modified
agent/agent.go (+18/-0)
agent/agent_test.go (+34/-0)
To merge this branch: bzr merge lp:~axwalk/juju-core/agent-setapihostports
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+213409@code.launchpad.net

Commit message

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/

Description of the change

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/

To post a comment you must log in.
Revision history for this message
Dave Cheney (dave-cheney) wrote :

+ 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 ?

Revision history for this message
Andrew Wilkins (axwalk) wrote :
Download full text (3.9 KiB)

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...

Read more...

Revision history for this message
Dave Cheney (dave-cheney) 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 ?

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

Revision history for this message
Andrew Wilkins (axwalk) 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/

Revision history for this message
Dave Cheney (dave-cheney) wrote :

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/

Revision history for this message
Dimiter Naydenov (dimitern) wrote :

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/

Revision history for this message
John A Meinel (jameinel) wrote :

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/

Revision history for this message
Andrew Wilkins (axwalk) wrote :

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/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'agent/agent.go'
2--- agent/agent.go 2014-03-28 08:55:45 +0000
3+++ agent/agent.go 2014-03-31 06:35:35 +0000
4@@ -16,6 +16,7 @@
5 "github.com/errgo/errgo"
6 "github.com/juju/loggo"
7
8+ "launchpad.net/juju-core/instance"
9 "launchpad.net/juju-core/state"
10 "launchpad.net/juju-core/state/api"
11 "launchpad.net/juju-core/state/api/params"
12@@ -133,6 +134,9 @@
13 // the agent has successfully upgraded to.
14 SetUpgradedToVersion(newVersion version.Number)
15
16+ // SetAPIHostPorts sets the API host/port addresses to connect to.
17+ SetAPIHostPorts(servers [][]instance.HostPort)
18+
19 // Migrate takes an existing agent config and applies the given
20 // parameters to change it.
21 //
22@@ -423,6 +427,20 @@
23 c.upgradedToVersion = newVersion
24 }
25
26+func (c *configInternal) SetAPIHostPorts(servers [][]instance.HostPort) {
27+ if c.apiDetails == nil {
28+ return
29+ }
30+ var addrs []string
31+ for _, serverHostPorts := range servers {
32+ addr := instance.SelectInternalHostPort(serverHostPorts, false)
33+ if addr != "" {
34+ addrs = append(addrs, addr)
35+ }
36+ }
37+ c.apiDetails.addresses = addrs
38+}
39+
40 func (c *configInternal) SetValue(key, value string) {
41 if value == "" {
42 delete(c.values, key)
43
44=== modified file 'agent/agent_test.go'
45--- agent/agent_test.go 2014-03-28 08:28:04 +0000
46+++ agent/agent_test.go 2014-03-31 06:35:35 +0000
47@@ -10,6 +10,7 @@
48 gc "launchpad.net/gocheck"
49
50 "launchpad.net/juju-core/agent"
51+ "launchpad.net/juju-core/instance"
52 "launchpad.net/juju-core/state"
53 "launchpad.net/juju-core/state/api"
54 "launchpad.net/juju-core/state/api/params"
55@@ -430,3 +431,36 @@
56 conf.SetUpgradedToVersion(expectVers)
57 c.Assert(conf.UpgradedToVersion(), gc.Equals, expectVers)
58 }
59+
60+func (*suite) TestSetAPIHostPorts(c *gc.C) {
61+ conf, err := agent.NewAgentConfig(attributeParams)
62+ c.Assert(err, gc.IsNil)
63+
64+ addrs, err := conf.APIAddresses()
65+ c.Assert(err, gc.IsNil)
66+ c.Assert(addrs, gc.DeepEquals, attributeParams.APIAddresses)
67+
68+ // The first cloud-local address for each server is used,
69+ // else if there are none then the first public- or unknown-
70+ // scope address.
71+ //
72+ // If a server has only machine-local addresses, or none
73+ // at all, then it will be excluded.
74+ server1 := instance.NewAddresses("0.1.2.3", "0.1.2.4", "zeroonetwothree")
75+ server1[0].NetworkScope = instance.NetworkCloudLocal
76+ server1[1].NetworkScope = instance.NetworkCloudLocal
77+ server1[2].NetworkScope = instance.NetworkPublic
78+ server2 := instance.NewAddresses("127.0.0.1")
79+ server2[0].NetworkScope = instance.NetworkMachineLocal
80+ server3 := instance.NewAddresses("0.1.2.5", "zeroonetwofive")
81+ server3[0].NetworkScope = instance.NetworkUnknown
82+ server3[1].NetworkScope = instance.NetworkUnknown
83+ conf.SetAPIHostPorts([][]instance.HostPort{
84+ instance.AddressesWithPort(server1, 123),
85+ instance.AddressesWithPort(server2, 124),
86+ instance.AddressesWithPort(server3, 125),
87+ })
88+ addrs, err = conf.APIAddresses()
89+ c.Assert(err, gc.IsNil)
90+ c.Assert(addrs, gc.DeepEquals, []string{"0.1.2.3:123", "0.1.2.5:125"})
91+}

Subscribers

People subscribed via source and target branches

to status/vote changes: