Merge lp:~gz/juju-core/unit_publicaddress_from_machine into lp:~go-bot/juju-core/trunk

Proposed by Martin Packman
Status: Merged
Approved by: Martin Packman
Approved revision: no longer in the source branch.
Merged at revision: 1623
Proposed branch: lp:~gz/juju-core/unit_publicaddress_from_machine
Merge into: lp:~go-bot/juju-core/trunk
Prerequisite: lp:~gz/juju-core/machine_doc_addresses
Diff against target: 89 lines (+42/-2)
3 files modified
state/machine_test.go (+2/-1)
state/unit.go (+18/-1)
state/unit_test.go (+22/-0)
To merge this branch: bzr merge lp:~gz/juju-core/unit_publicaddress_from_machine
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+177983@code.launchpad.net

Commit message

state: Use machine addresses from unit

Make Unit.PublicAddress check for addresses an associated machine
by preference and select an appropriate one. This is the first
step towards removing PublicAddress and PrivateAddress from unit.

https://codereview.appspot.com/12218043/

R=dimitern, wallyworld

Description of the change

state: Use machine addresses from unit

Make Unit.PublicAddress check for addresses an associated machine
by preference and select an appropriate one. This is the first
step towards removing PublicAddress and PrivateAddress from unit.

https://codereview.appspot.com/12218043/

To post a comment you must log in.
Revision history for this message
Martin Packman (gz) wrote :

Reviewers: mp+177983_code.launchpad.net,

Message:
Please take a look.

Description:
Attempt at using machine addresses from unit

Non-working attempt at getting Unit.PublicAddress to take the
value from the addresses stored on a related Machine where
available.

The test added fails as the machineDoc created from the unit
does not contain the addresses just assigned. Have they ever
reached mongo? Does the machine need constructing differently,
or the test need some added magic?

https://code.launchpad.net/~gz/juju-core/unit_publicaddress_from_machine/+merge/177983

Requires:
https://code.launchpad.net/~gz/juju-core/machine_doc_addresses/+merge/177978

(do not edit description out of merge proposal)

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

Affected files:
   [revision details]
   state/unit.go
   state/unit_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: <email address hidden>
+New revision: <email address hidden>

Index: state/unit.go
=== modified file 'state/unit.go'
--- state/unit.go 2013-07-31 14:19:29 +0000
+++ state/unit.go 2013-08-01 01:42:09 +0000
@@ -442,7 +442,16 @@

  // PublicAddress returns the public address of the unit and whether it is
valid.
  func (u *Unit) PublicAddress() (string, bool) {
- return u.doc.PublicAddress, u.doc.PublicAddress != ""
+ publicaddress := u.doc.PublicAddress
+ if u.doc.MachineId != "" {
+ m, err := u.st.Machine(u.doc.MachineId)
+ if err != nil {
+ // XXX(gz) Or treat as not having an address?
+ panic(err)
+ }
+ publicaddress = instance.SelectPublicAddress(m.Addresses())
+ }
+ return publicaddress, publicaddress != ""
  }

  // PrivateAddress returns the private address of the unit and whether it
is valid.

Index: state/unit_test.go
=== modified file 'state/unit_test.go'
--- state/unit_test.go 2013-07-31 14:19:29 +0000
+++ state/unit_test.go 2013-08-01 01:42:09 +0000
@@ -169,6 +169,29 @@
   c.Assert(err, ErrorMatches, `cannot set public address of
unit "wordpress/0": unit not found`)
  }

+func (s *UnitSuite) TestGetPublicAddressFromMachine(c *C) {
+ machine, err := s.State.AddMachine("series", state.JobHostUnits)
+ c.Assert(err, IsNil)
+ err = s.unit.AssignToMachine(machine)
+ c.Assert(err, IsNil)
+
+ address, ok := s.unit.PublicAddress()
+ c.Check(address, Equals, "")
+ c.Assert(ok, Equals, false)
+
+ addresses := []instance.Address{
+ instance.NewAddress("127.0.0.1"),
+ instance.NewAddress("8.8.8.8"),
+ }
+ err = machine.SetAddresses(addresses)
+ c.Assert(err, IsNil)
+
+ // XXX(gz) Following fails, machine doc lacks addresses when read, why?
+ address, ok = s.unit.PublicAddress()
+ c.Check(address, Equals, "8.8.8.8")
+ c.Assert(ok, Equals, true)
+}
+
  func (s *UnitSuite) TestGetSetPrivateAddress(c *C) {
   _, ok := s.unit.PrivateAddress()
   c.Assert(ok, Equals, false)

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

LGTM modulo the below

https://codereview.appspot.com/12218043/diff/11001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/12218043/diff/11001/state/unit.go#newcode445
state/unit.go:445: publicaddress := u.doc.PublicAddress
publicAddress please;

Also do we need doc.PublicAddress anymore, now that we're using the
machine one?

https://codereview.appspot.com/12218043/diff/11001/state/unit.go#newcode449
state/unit.go:449: // XXX(gz) Or treat as not having an address?
I think just logging it and returning false should be enough here, no
need to panic.

https://codereview.appspot.com/12218043/diff/11001/state/unit_test.go
File state/unit_test.go (right):

https://codereview.appspot.com/12218043/diff/11001/state/unit_test.go#newcode191
state/unit_test.go:191: c.Assert(ok, Equals, true)
How about another test when the unit is not assigned to a machine?

https://codereview.appspot.com/12218043/

Revision history for this message
Martin Packman (gz) wrote :

https://codereview.appspot.com/12218043/diff/11001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/12218043/diff/11001/state/unit.go#newcode445
state/unit.go:445: publicaddress := u.doc.PublicAddress
On 2013/08/01 14:30:00, dimitern wrote:
> publicAddress please;

Okay.

> Also do we need doc.PublicAddress anymore, now that we're using the
machine one?

Yes, though potentially with some adjustments. We may need, for
backwards compat reasons, to care about charms manually setting a public
address on a unit, in which case we'd have to pay attention to that.
Most of that is for a follow up branch though.

https://codereview.appspot.com/12218043/diff/11001/state/unit.go#newcode449
state/unit.go:449: // XXX(gz) Or treat as not having an address?
On 2013/08/01 14:30:00, dimitern wrote:
> I think just logging it and returning false should be enough here, no
need to
> panic.

Okay.

https://codereview.appspot.com/12218043/diff/11001/state/unit_test.go
File state/unit_test.go (right):

https://codereview.appspot.com/12218043/diff/11001/state/unit_test.go#newcode191
state/unit_test.go:191: c.Assert(ok, Equals, true)
On 2013/08/01 14:30:00, dimitern wrote:
> How about another test when the unit is not assigned to a machine?

The existing test above should cover that case fine I think. As
mentioned, there may be other funny scenarios we need to care about
later though.

https://codereview.appspot.com/12218043/

Revision history for this message
Martin Packman (gz) wrote :
Revision history for this message
Ian Booth (wallyworld) wrote :

LGTM. Perhaps with another test like Dimiter suggested.
Plus the err in the log message.

https://codereview.appspot.com/12218043/diff/18001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/12218043/diff/18001/state/unit.go#newcode454
state/unit.go:454: unitLogger.Errorf("unit %v misses machine id %v", u,
id)
please include err in the logged message

https://codereview.appspot.com/12218043/diff/18001/state/unit.go#newcode455
state/unit.go:455: return "", false
did we really want to return "" here? why not u.doc.PublicAddress?

https://codereview.appspot.com/12218043/

Revision history for this message
Tim Penhey (thumper) wrote :

https://codereview.appspot.com/12218043/diff/18001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/12218043/diff/18001/state/unit.go#newcode457
state/unit.go:457: publicAddress =
instance.SelectPublicAddress(m.Addresses())
Is there likely to be a case where u.doc.PublicAddress is valid, but
m.Addresses() is empty? Because if there is, we'd be returning "" here
instead of u.doc.PublicAddress

https://codereview.appspot.com/12218043/

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

https://codereview.appspot.com/12218043/diff/18001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/12218043/diff/18001/state/unit.go#newcode29
state/unit.go:29: var unitLogger = loggo.GetLogger("juju.state.unit")
I think this should be "juju.state" and please move it in state/state.go
perhaps?

https://codereview.appspot.com/12218043/

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

https://codereview.appspot.com/12218043/diff/18001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/12218043/diff/18001/state/unit.go#newcode29
state/unit.go:29: var unitLogger = loggo.GetLogger("juju.state.unit")
I think this should be "juju.state" and please move it in state/state.go
perhaps?

https://codereview.appspot.com/12218043/

Revision history for this message
Go Bot (go-bot) wrote :
Revision history for this message
Go Bot (go-bot) wrote :
Download full text (146.2 KiB)

The attempt to merge lp:~gz/juju-core/unit_publicaddress_from_machine into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 2.467s
ok launchpad.net/juju-core/agent/tools 25.304s
ok launchpad.net/juju-core/bzr 6.765s
ok launchpad.net/juju-core/cert 1.887s
ok launchpad.net/juju-core/charm 0.972s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.021s
ok launchpad.net/juju-core/cmd 0.223s
? launchpad.net/juju-core/cmd/builddb [no test files]
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
listing available tools
found 4 tools
found 4 recent tools (version 1.0.0)
listing target bucket
found 3 tools in target; 4 tools to be copied
copying 1.0.0-defaultseries-amd64 from http://127.0.0.1:56898/tools/juju-1.0.0-defaultseries-amd64.tgz
copying tools/juju-1.0.0-defaultseries-amd64.tgz
downloaded tools/juju-1.0.0-defaultseries-amd64.tgz (0kB), uploading
download 0kB, uploading
copying 1.0.0-precise-amd64 from http://127.0.0.1:56898/tools/juju-1.0.0-precise-amd64.tgz
copying tools/juju-1.0.0-precise-amd64.tgz
downloaded tools/juju-1.0.0-precise-amd64.tgz (0kB), uploading
download 0kB, uploading
copying 1.0.0-quantal-amd64 from http://127.0.0.1:56898/tools/juju-1.0.0-quantal-amd64.tgz
copying tools/juju-1.0.0-quantal-amd64.tgz
downloaded tools/juju-1.0.0-quantal-amd64.tgz (0kB), uploading
download 0kB, uploading
copying 1.0.0-quantal-i386 from http://127.0.0.1:56898/tools/juju-1.0.0-quantal-i386.tgz
copying tools/juju-1.0.0-quantal-i386.tgz
downloaded tools/juju-1.0.0-quantal-i386.tgz (0kB), uploading
download 0kB, uploading
copied 4 tools

----------------------------------------------------------------------
FAIL: debughooks_test.go:69: DebugHooksSuite.TestDebugHooksCommand

[LOG] 22.39726 INFO juju environs/testing: uploading FAKE tools 1.13.1-precise-amd64
[LOG] 22.39735 INFO juju environs: reading tools with major version 1
[LOG] 22.39736 DEBUG juju.agent.tools reading v1.* tools
[LOG] 22.39738 INFO juju environs: falling back to public bucket
[LOG] 22.39739 DEBUG juju.agent.tools reading v1.* tools
[LOG] 22.39742 DEBUG juju.agent.tools found 1.13.1-precise-amd64
[LOG] 22.39744 INFO juju environs: filtering tools by series: precise
[LOG] 22.39746 INFO juju environs: filtering tools by version: 1.13.1
[LOG] 22.39748 INFO juju environs/dummy: would pick tools from 1.13.1-precise-amd64
[LOG] 22.43057 INFO juju state: opening state; mongo addresses: ["localhost:55024"]; entity ""
[LOG] 22.43381 INFO juju state: connection established
[LOG] 22.46165 INFO juju state: initializing environment
[LOG] 22.48270 INFO juju state/api: listening on "127.0.0.1:55142"
[LOG] 22.49972 INFO juju state: opening state; mongo addresses: ["localhost:55024"]; entity ""
[LOG] 22.50562 INFO juju state: connection established
[LOG] 22.50624 INFO juju juju: authorization error while connecting to state server; retrying
[LOG] 22.50628 INFO juju state: opening state; mongo addresses: ["localhost:55024"]; entity ""
[LOG] 22.50984 INFO juju state: connection established
[LOG] 22.54675 INFO juju state/api: di...

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'state/machine_test.go'
2--- state/machine_test.go 2013-08-01 15:09:01 +0000
3+++ state/machine_test.go 2013-08-08 16:45:26 +0000
4@@ -946,6 +946,7 @@
5 }
6 err = machine.SetAddresses(addresses)
7 c.Assert(err, IsNil)
8- machine.Refresh()
9+ err = machine.Refresh()
10+ c.Assert(err, IsNil)
11 c.Assert(machine.Addresses(), DeepEquals, addresses)
12 }
13
14=== modified file 'state/unit.go'
15--- state/unit.go 2013-08-08 08:12:29 +0000
16+++ state/unit.go 2013-08-08 16:45:26 +0000
17@@ -13,6 +13,8 @@
18 "labix.org/v2/mgo/bson"
19 "labix.org/v2/mgo/txn"
20
21+ "launchpad.net/loggo"
22+
23 "launchpad.net/juju-core/agent/tools"
24 "launchpad.net/juju-core/charm"
25 "launchpad.net/juju-core/constraints"
26@@ -24,6 +26,8 @@
27 "launchpad.net/juju-core/utils"
28 )
29
30+var unitLogger = loggo.GetLogger("juju.state.unit")
31+
32 // AssignmentPolicy controls what machine a unit will be assigned to.
33 type AssignmentPolicy string
34
35@@ -450,7 +454,20 @@
36
37 // PublicAddress returns the public address of the unit and whether it is valid.
38 func (u *Unit) PublicAddress() (string, bool) {
39- return u.doc.PublicAddress, u.doc.PublicAddress != ""
40+ publicAddress := u.doc.PublicAddress
41+ id := u.doc.MachineId
42+ if id != "" {
43+ m, err := u.st.Machine(id)
44+ if err != nil {
45+ unitLogger.Errorf("unit %v misses machine id %v", u, id)
46+ return "", false
47+ }
48+ addresses := m.Addresses()
49+ if len(addresses) > 0 {
50+ publicAddress = instance.SelectPublicAddress(addresses)
51+ }
52+ }
53+ return publicAddress, publicAddress != ""
54 }
55
56 // PrivateAddress returns the private address of the unit and whether it is valid.
57
58=== modified file 'state/unit_test.go'
59--- state/unit_test.go 2013-08-06 04:03:58 +0000
60+++ state/unit_test.go 2013-08-08 16:45:26 +0000
61@@ -169,6 +169,28 @@
62 c.Assert(err, ErrorMatches, `cannot set public address of unit "wordpress/0": unit not found`)
63 }
64
65+func (s *UnitSuite) TestGetPublicAddressFromMachine(c *C) {
66+ machine, err := s.State.AddMachine("series", state.JobHostUnits)
67+ c.Assert(err, IsNil)
68+ err = s.unit.AssignToMachine(machine)
69+ c.Assert(err, IsNil)
70+
71+ address, ok := s.unit.PublicAddress()
72+ c.Check(address, Equals, "")
73+ c.Assert(ok, Equals, false)
74+
75+ addresses := []instance.Address{
76+ instance.NewAddress("127.0.0.1"),
77+ instance.NewAddress("8.8.8.8"),
78+ }
79+ err = machine.SetAddresses(addresses)
80+ c.Assert(err, IsNil)
81+
82+ address, ok = s.unit.PublicAddress()
83+ c.Check(address, Equals, "8.8.8.8")
84+ c.Assert(ok, Equals, true)
85+}
86+
87 func (s *UnitSuite) TestGetSetPrivateAddress(c *C) {
88 _, ok := s.unit.PrivateAddress()
89 c.Assert(ok, Equals, false)

Subscribers

People subscribed via source and target branches

to status/vote changes: