Merge lp:~axwalk/juju-core/machiner-host-addresses into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

worker/machiner: set machine addresses in SetUp

The machiner worker is updated to, at setup time,
set the machine's addresses to all of the non-loopback
interface addresses as detected from within the
machine. This enables us to record addresses for
containers created by the local provider.

https://codereview.appspot.com/54710043/

worker/machiner: set machine addresses in SetUp

The machiner worker is updated to, at setup time,
set the machine's addresses to all of the non-loopback
interface addresses as detected from within the
machine. This enables us to record addresses for
containers created by the local provider.

https://codereview.appspot.com/54710043/

[Andrew Wilkins (axwalk)]

Reviewers: mp+202383_code.launchpad.net,

Message:
Please take a look.

Description:
worker/machiner: set machine addresses in SetUp

The machiner worker is updated to, at setup time,
set the machine's addresses to all of the non-loopback
interface addresses as detected from within the
machine. This enables us to record addresses for
containers created by the local provider.

https://code.launchpad.net/~axwalk/juju-core/machiner-host-addresses/+merge/202383

(do not edit description out of merge proposal)

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

Affected files (+74, -0 lines):
   A [revision details]
   A worker/machiner/export_test.go
   M worker/machiner/machiner.go
   M worker/machiner/machiner_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-20140120214742-p8sw7oef0btqzfb0
+New revision: <email address hidden>

Index: worker/machiner/export_test.go
=== added file 'worker/machiner/export_test.go'
--- worker/machiner/export_test.go 1970-01-01 00:00:00 +0000
+++ worker/machiner/export_test.go 2014-01-20 22:01:41 +0000
@@ -0,0 +1,6 @@
+// Copyright 2014 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package machiner
+
+var InterfaceAddrs = &interfaceAddrs

Index: worker/machiner/machiner.go
=== modified file 'worker/machiner/machiner.go'
--- worker/machiner/machiner.go 2014-01-03 15:34:52 +0000
+++ worker/machiner/machiner.go 2014-01-20 22:04:23 +0000
@@ -4,10 +4,12 @@

  import (
   "fmt"
+ "net"

   "launchpad.net/loggo"

   "launchpad.net/juju-core/agent"
+ "launchpad.net/juju-core/instance"
   "launchpad.net/juju-core/state/api/machiner"
   "launchpad.net/juju-core/state/api/params"
   "launchpad.net/juju-core/state/api/watcher"
@@ -41,6 +43,11 @@
   }
   mr.machine = m

+ // Set the addresses in state to the host's addresses.
+ if err := setMachineAddresses(m); err != nil {
+ return nil, err
+ }
+
   // Mark the machine as started and log it.
   if err := m.SetStatus(params.StatusStarted, "", nil); err != nil {
    return nil, fmt.Errorf("%s failed to set status started: %v", mr.tag,
err)
@@ -50,6 +57,40 @@
   return m.Watch()
  }

+var interfaceAddrs = net.InterfaceAddrs
+
+// setMachineAddresses sets the addresses for this machine to all of the
+// host's non-loopback interface IP addresses. We only need to do this for
+// containers; instances' addresses are updated via the addressupdater
worker
+// by querying the provider.
+func setMachineAddresses(m *machiner.Machine) error {
+ addrs, err := interfaceAddrs()
+ if err != nil {
+ return err
+ }
+ var hostAddresses []instance.Address
+ for _, addr := range addrs {
+ var ip net.IP
+ switch addr := addr.(type) {
+ case *net.IPAddr:
+ ip = addr.IP
+ case *net.IPNet:
+ ip = addr.IP
+ default:
+ continue
+ }
+ if ip.IsLoopback() {
+ continue
+ }
+ hostAddresses = append(hostAddresses, instance.NewAddress(ip.String()))
+ }
+ if len(hostAddresses) == 0 {
+ return nil
+ }
+ logger.Infof("setting addresses for %v to %q"...

[Ian Booth (wallyworld)]

LGTM, a couple of comments

https://codereview.appspot.com/54710043/diff/1/worker/machiner/machiner.go
File worker/machiner/machiner.go (right):

https://codereview.appspot.com/54710043/diff/1/worker/machiner/machiner.go#newcode64
worker/machiner/machiner.go:64: // containers; instances' addresses are
updated via the addressupdater worker
comment needs to be tweaked

https://codereview.appspot.com/54710043/diff/1/worker/machiner/machiner.go#newcode77
worker/machiner/machiner.go:77: case *net.IPNet:
can these 2 case statements be combined?

https://codereview.appspot.com/54710043/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/54710043/diff/1/worker/machiner/machiner.go
File worker/machiner/machiner.go (right):

https://codereview.appspot.com/54710043/diff/1/worker/machiner/machiner.go#newcode64
worker/machiner/machiner.go:64: // containers; instances' addresses are
updated via the addressupdater worker
On 2014/01/20 22:24:11, wallyworld wrote:
> comment needs to be tweaked

Done.

https://codereview.appspot.com/54710043/diff/1/worker/machiner/machiner.go#newcode77
worker/machiner/machiner.go:77: case *net.IPNet:
On 2014/01/20 22:24:11, wallyworld wrote:
> can these 2 case statements be combined?

Not for a type switch.

https://codereview.appspot.com/54710043/

[John A Meinel (jameinel)]

https://codereview.appspot.com/54710043/diff/1/worker/machiner/machiner.go
File worker/machiner/machiner.go (right):

https://codereview.appspot.com/54710043/diff/1/worker/machiner/machiner.go#newcode77
worker/machiner/machiner.go:77: case *net.IPNet:
On 2014/01/20 22:59:02, axw wrote:
> On 2014/01/20 22:24:11, wallyworld wrote:
> > can these 2 case statements be combined?

> Not for a type switch.

Couldn't you do:

case *net.IPAddr:
   fallthrough
case *net.IPNet:
   ip = addr.IP

I'd also say, reusing the variable in the switch seems *really*
confusing to me.

switch addr := addr.(type)

I'd rather see something like 'realAddr' or something that makes it
clear what variable you're talking about instead of overloading it.
Maybe that's just me.

https://codereview.appspot.com/54710043/

[Andrew Wilkins (axwalk)]

On 2014/01/21 07:09:37, jameinel wrote:

https://codereview.appspot.com/54710043/diff/1/worker/machiner/machiner.go
> File worker/machiner/machiner.go (right):

https://codereview.appspot.com/54710043/diff/1/worker/machiner/machiner.go#newcode77
> worker/machiner/machiner.go:77: case *net.IPNet:
> On 2014/01/20 22:59:02, axw wrote:
> > On 2014/01/20 22:24:11, wallyworld wrote:
> > > can these 2 case statements be combined?
> >
> > Not for a type switch.

> Couldn't you do:

> case *net.IPAddr:
> fallthrough
> case *net.IPNet:
> ip = addr.IP

Nope. In either case the block is operating on the newly declared "addr"
which has a type specific to that case. While the blocks *look* the
same, they're really:
     ip = addr.IP, where addr is of type *net.IPAddr
   and
     ip = addr.IP, where addr is of type *net.IPNet

> I'd also say, reusing the variable in the switch seems *really*
confusing to me.

> switch addr := addr.(type)

> I'd rather see something like 'realAddr' or something that makes it
clear what
> variable you're talking about instead of overloading it. Maybe that's
just me.

My opinion is that there's rarely a useful alternative name to give
something that's being type-asserted. To me, "realAddr" doesn't seem
right; how is it any more real than the net.Addr interface value? If
there's no additional information, I don't see the value of introducing
a new name. In this case I could have called it ipAddr, I think, on the
grounds that I've asserted that it's an Addr that has an IP (but not
necessarily just a *net.IPAddr).

It's not just you, people on golang-nuts have debated this before. I'll
try to be more mindful of this in the future. I pretty much always just
do "x := x.(type)" without thinking about it.

https://codereview.appspot.com/54710043/

=== added file 'worker/machiner/export_test.go'
--- worker/machiner/export_test.go	1970-01-01 00:00:00 +0000
+++ worker/machiner/export_test.go	2014-01-20 22:57:49 +0000
@@ -0,0 +1,6 @@
+// Copyright 2014 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package machiner
+
+var InterfaceAddrs = &interfaceAddrs

=== modified file 'worker/machiner/machiner.go'
--- worker/machiner/machiner.go	2014-01-03 15:34:52 +0000
+++ worker/machiner/machiner.go	2014-01-20 22:57:49 +0000
@@ -4,10 +4,12 @@
 
 import (
 	"fmt"
+	"net"
 
 	"launchpad.net/loggo"
 
 	"launchpad.net/juju-core/agent"
+	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/state/api/machiner"
 	"launchpad.net/juju-core/state/api/params"
 	"launchpad.net/juju-core/state/api/watcher"
@@ -41,6 +43,11 @@
 	}
 	mr.machine = m
 
+	// Set the addresses in state to the host's addresses.
+	if err := setMachineAddresses(m); err != nil {
+		return nil, err
+	}
+
 	// Mark the machine as started and log it.
 	if err := m.SetStatus(params.StatusStarted, "", nil); err != nil {
 		return nil, fmt.Errorf("%s failed to set status started: %v", mr.tag, err)
@@ -50,6 +57,38 @@
 	return m.Watch()
 }
 
+var interfaceAddrs = net.InterfaceAddrs
+
+// setMachineAddresses sets the addresses for this machine to all of the
+// host's non-loopback interface IP addresses.
+func setMachineAddresses(m *machiner.Machine) error {
+	addrs, err := interfaceAddrs()
+	if err != nil {
+		return err
+	}
+	var hostAddresses []instance.Address
+	for _, addr := range addrs {
+		var ip net.IP
+		switch addr := addr.(type) {
+		case *net.IPAddr:
+			ip = addr.IP
+		case *net.IPNet:
+			ip = addr.IP
+		default:
+			continue
+		}
+		if ip.IsLoopback() {
+			continue
+		}
+		hostAddresses = append(hostAddresses, instance.NewAddress(ip.String()))
+	}
+	if len(hostAddresses) == 0 {
+		return nil
+	}
+	logger.Infof("setting addresses for %v to %q", m.Tag(), hostAddresses)
+	return m.SetMachineAddresses(hostAddresses)
+}
+
 func (mr *Machiner) Handle() error {
 	if err := mr.machine.Refresh(); params.IsCodeNotFoundOrCodeUnauthorized(err) {
 		return worker.ErrTerminateAgent

=== modified file 'worker/machiner/machiner_test.go'
--- worker/machiner/machiner_test.go	2013-09-27 10:59:13 +0000
+++ worker/machiner/machiner_test.go	2014-01-20 22:57:49 +0000
@@ -4,12 +4,14 @@
 package machiner_test
 
 import (
+	"net"
 	stdtesting "testing"
 	"time"
 
 	gc "launchpad.net/gocheck"
 
 	"launchpad.net/juju-core/agent"
+	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/juju/testing"
 	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/state/api"
@@ -133,3 +135,26 @@
 	c.Assert(s.machine.Refresh(), gc.IsNil)
 	c.Assert(s.machine.Life(), gc.Equals, state.Dead)
 }
+
+func (s *MachinerSuite) TestMachineAddresses(c *gc.C) {
+	s.PatchValue(machiner.InterfaceAddrs, func() ([]net.Addr, error) {
+		addrs := []net.Addr{
+			&net.IPAddr{IP: net.IPv4(10, 0, 0, 1)},
+			&net.IPAddr{IP: net.IPv4(127, 0, 0, 1)}, // loopback, ignored
+			&net.IPAddr{IP: net.IPv6loopback},       // loopback, ignored
+			&net.UnixAddr{},                         // not IP, ignored
+			&net.IPNet{IP: net.ParseIP("2001:db8::1")},
+		}
+		return addrs, nil
+	})
+	mr := s.makeMachiner()
+	defer worker.Stop(mr)
+	c.Assert(s.machine.Destroy(), gc.IsNil)
+	s.State.StartSync()
+	c.Assert(mr.Wait(), gc.Equals, worker.ErrTerminateAgent)
+	c.Assert(s.machine.Refresh(), gc.IsNil)
+	c.Assert(s.machine.MachineAddresses(), gc.DeepEquals, []instance.Address{
+		instance.NewAddress("10.0.0.1"),
+		instance.NewAddress("2001:db8::1"),
+	})
+}