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

[Andrew Wilkins]

Introduce worker/apiaddressupdater

This proposal introduces a new worker that
watches changes to APIHostPorts, and then
updates agent config. The machine and unit
agents have both been updated to start this
worker.

https://codereview.appspot.com/83500043/

Introduce worker/apiaddressupdater

This proposal introduces a new worker that
watches changes to APIHostPorts, and then
updates agent config. The machine and unit
agents have both been updated to start this
worker.

https://codereview.appspot.com/83500043/

[Andrew Wilkins (axwalk)]

Reviewers: mp+213778_code.launchpad.net,

Message:
Please take a look.

Description:
Introduce worker/apiaddressupdater

This proposal introduces a new worker that
watches changes to APIHostPorts, and then
updates agent config. The machine and unit
agents have both been updated to start this
worker.

https://code.launchpad.net/~axwalk/juju-core/agent-watch-apihostports/+merge/213778

(do not edit description out of merge proposal)

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

Affected files (+242, -0 lines):
   A [revision details]
   M cmd/jujud/agent.go
   M cmd/jujud/machine.go
   M cmd/jujud/machine_test.go
   M cmd/jujud/unit.go
   M cmd/jujud/unit_test.go
   A worker/apiaddressupdater/apiaddressupdater.go
   A worker/apiaddressupdater/apiaddressupdater_test.go

[Roger Peppe (rogpeppe)]

LGTM with a few queries below, thanks!

https://codereview.appspot.com/83500043/diff/1/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/83500043/diff/1/cmd/jujud/machine_test.go#newcode851
cmd/jujud/machine_test.go:851: s.State.StartSync()
shouldn't this be BackingState? (i think the watcher is actually in the
API, right?)

https://codereview.appspot.com/83500043/diff/1/cmd/jujud/machine_test.go#newcode852
cmd/jujud/machine_test.go:852: timeout :=
time.After(coretesting.LongWait)
perhaps this might be better phrased as an attempt loop?

https://codereview.appspot.com/83500043/diff/1/cmd/jujud/unit_test.go
File cmd/jujud/unit_test.go (right):

https://codereview.appspot.com/83500043/diff/1/cmd/jujud/unit_test.go#newcode270
cmd/jujud/unit_test.go:270: s.State.StartSync()
shouldn't this be BackingState?

https://codereview.appspot.com/83500043/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/83500043/diff/1/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/83500043/diff/1/cmd/jujud/machine_test.go#newcode851
cmd/jujud/machine_test.go:851: s.State.StartSync()
On 2014/04/02 08:51:08, rog wrote:
> shouldn't this be BackingState? (i think the watcher is actually in
the API,
> right?)

Done.

https://codereview.appspot.com/83500043/diff/1/cmd/jujud/machine_test.go#newcode852
cmd/jujud/machine_test.go:852: timeout :=
time.After(coretesting.LongWait)
On 2014/04/02 08:51:08, rog wrote:
> perhaps this might be better phrased as an attempt loop?

Done, much nicer.

https://codereview.appspot.com/83500043/diff/1/cmd/jujud/unit_test.go
File cmd/jujud/unit_test.go (right):

https://codereview.appspot.com/83500043/diff/1/cmd/jujud/unit_test.go#newcode270
cmd/jujud/unit_test.go:270: s.State.StartSync()
On 2014/04/02 08:51:08, rog wrote:
> shouldn't this be BackingState?

Done.

https://codereview.appspot.com/83500043/

=== modified file 'cmd/jujud/agent.go'
--- cmd/jujud/agent.go	2014-03-28 13:24:36 +0000
+++ cmd/jujud/agent.go	2014-04-02 09:44:43 +0000
@@ -13,6 +13,7 @@
 
 	"launchpad.net/juju-core/agent"
 	"launchpad.net/juju-core/cmd"
+	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/log"
 	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/state/api"
@@ -82,6 +83,13 @@
 	return ch._config.Clone()
 }
 
+// SetAPIHostPorts satisfies worker/apiaddressupdater/APIAddressSetter.
+func (a *AgentConf) SetAPIHostPorts(servers [][]instance.HostPort) error {
+	return a.ChangeConfig(func(c agent.ConfigSetter) {
+		c.SetAPIHostPorts(servers)
+	})
+}
+
 func importance(err error) int {
 	switch {
 	case err == nil:

=== modified file 'cmd/jujud/machine.go'
--- cmd/jujud/machine.go	2014-03-28 10:11:59 +0000
+++ cmd/jujud/machine.go	2014-04-02 09:44:43 +0000
@@ -33,6 +33,7 @@
 	"launchpad.net/juju-core/utils"
 	"launchpad.net/juju-core/version"
 	"launchpad.net/juju-core/worker"
+	"launchpad.net/juju-core/worker/apiaddressupdater"
 	"launchpad.net/juju-core/worker/authenticationworker"
 	"launchpad.net/juju-core/worker/charmrevisionworker"
 	"launchpad.net/juju-core/worker/cleaner"
@@ -212,6 +213,9 @@
 	a.startWorkerAfterUpgrade(runner, "machiner", func() (worker.Worker, error) {
 		return machiner.NewMachiner(st.Machiner(), agentConfig), nil
 	})
+	a.startWorkerAfterUpgrade(runner, "apiaddressupdater", func() (worker.Worker, error) {
+		return apiaddressupdater.NewAPIAddressUpdater(st.Machiner(), a), nil
+	})
 	a.startWorkerAfterUpgrade(runner, "logger", func() (worker.Worker, error) {
 		return workerlogger.NewLogger(st.Logger(), agentConfig), nil
 	})

=== modified file 'cmd/jujud/machine_test.go'
--- cmd/jujud/machine_test.go	2014-04-01 00:12:08 +0000
+++ cmd/jujud/machine_test.go	2014-04-02 09:44:43 +0000
@@ -833,6 +833,32 @@
 	})
 }
 
+func (s *MachineSuite) TestMachineAgentRunsAPIAddressUpdaterWorker(c *gc.C) {
+	// Start the machine agent.
+	m, _, _ := s.primeAgent(c, version.Current, state.JobHostUnits)
+	a := s.newAgent(c, m)
+	go func() { c.Check(a.Run(nil), gc.IsNil) }()
+	defer func() { c.Check(a.Stop(), gc.IsNil) }()
+
+	// Update the API addresses.
+	updatedServers := [][]instance.HostPort{instance.AddressesWithPort(
+		instance.NewAddresses("localhost"), 1234,
+	)}
+	err := s.BackingState.SetAPIHostPorts(updatedServers)
+	c.Assert(err, gc.IsNil)
+
+	// Wait for config to be updated.
+	s.BackingState.StartSync()
+	for attempt := coretesting.LongAttempt.Start(); attempt.Next(); {
+		addrs, err := a.CurrentConfig().APIAddresses()
+		c.Assert(err, gc.IsNil)
+		if reflect.DeepEqual(addrs, []string{"localhost:1234"}) {
+			return
+		}
+	}
+	c.Fatalf("timeout while waiting for agent config to change")
+}
+
 // MachineWithCharmsSuite provides infrastructure for tests which need to
 // work with charms.
 type MachineWithCharmsSuite struct {

=== modified file 'cmd/jujud/unit.go'
--- cmd/jujud/unit.go	2014-03-27 18:25:41 +0000
+++ cmd/jujud/unit.go	2014-04-02 09:44:43 +0000
@@ -15,6 +15,7 @@
 	"launchpad.net/juju-core/names"
 	"launchpad.net/juju-core/version"
 	"launchpad.net/juju-core/worker"
+	"launchpad.net/juju-core/worker/apiaddressupdater"
 	workerlogger "launchpad.net/juju-core/worker/logger"
 	"launchpad.net/juju-core/worker/rsyslog"
 	"launchpad.net/juju-core/worker/uniter"
@@ -96,6 +97,9 @@
 	runner.StartWorker("uniter", func() (worker.Worker, error) {
 		return uniter.NewUniter(st.Uniter(), entity.Tag(), dataDir), nil
 	})
+	runner.StartWorker("apiaddressupdater", func() (worker.Worker, error) {
+		return apiaddressupdater.NewAPIAddressUpdater(st.Uniter(), a), nil
+	})
 	runner.StartWorker("rsyslog", func() (worker.Worker, error) {
 		return newRsyslogConfigWorker(st.Rsyslog(), agentConfig, rsyslog.RsyslogModeForwarding)
 	})

=== modified file 'cmd/jujud/unit_test.go'
--- cmd/jujud/unit_test.go	2014-03-28 08:28:04 +0000
+++ cmd/jujud/unit_test.go	2014-04-02 09:44:43 +0000
@@ -4,6 +4,7 @@
 package main
 
 import (
+	"reflect"
 	"time"
 
 	gc "launchpad.net/gocheck"
@@ -11,6 +12,7 @@
 	"launchpad.net/juju-core/agent"
 	"launchpad.net/juju-core/cmd"
 	envtesting "launchpad.net/juju-core/environs/testing"
+	"launchpad.net/juju-core/instance"
 	jujutesting "launchpad.net/juju-core/juju/testing"
 	"launchpad.net/juju-core/names"
 	"launchpad.net/juju-core/state"
@@ -250,3 +252,28 @@
 		c.Assert(mode, gc.Equals, rsyslog.RsyslogModeForwarding)
 	}
 }
+
+func (s *UnitSuite) TestUnitAgentRunsAPIAddressUpdaterWorker(c *gc.C) {
+	_, unit, _, _ := s.primeAgent(c)
+	a := s.newAgent(c, unit)
+	go func() { c.Check(a.Run(nil), gc.IsNil) }()
+	defer func() { c.Check(a.Stop(), gc.IsNil) }()
+
+	// Update the API addresses.
+	updatedServers := [][]instance.HostPort{instance.AddressesWithPort(
+		instance.NewAddresses("localhost"), 1234,
+	)}
+	err := s.BackingState.SetAPIHostPorts(updatedServers)
+	c.Assert(err, gc.IsNil)
+
+	// Wait for config to be updated.
+	s.BackingState.StartSync()
+	for attempt := coretesting.LongAttempt.Start(); attempt.Next(); {
+		addrs, err := a.CurrentConfig().APIAddresses()
+		c.Assert(err, gc.IsNil)
+		if reflect.DeepEqual(addrs, []string{"localhost:1234"}) {
+			return
+		}
+	}
+	c.Fatalf("timeout while waiting for agent config to change")
+}

=== added directory 'worker/apiaddressupdater'
=== added file 'worker/apiaddressupdater/apiaddressupdater.go'
--- worker/apiaddressupdater/apiaddressupdater.go	1970-01-01 00:00:00 +0000
+++ worker/apiaddressupdater/apiaddressupdater.go	2014-04-02 09:44:43 +0000
@@ -0,0 +1,64 @@
+// Copyright 2014 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package apiaddressupdater
+
+import (
+	"fmt"
+
+	"github.com/juju/loggo"
+
+	"launchpad.net/juju-core/instance"
+	"launchpad.net/juju-core/state/api/watcher"
+	"launchpad.net/juju-core/worker"
+)
+
+var logger = loggo.GetLogger("juju.worker.apiaddressupdater")
+
+// APIAddressUpdater is responsible for cleaning up the state.
+type APIAddressUpdater struct {
+	addresser APIAddresser
+	setter    APIAddressSetter
+}
+
+// APIAddresser is an interface that is provided to NewAPIAddressUpdater
+// which can be used to watch for API address changes.
+type APIAddresser interface {
+	APIHostPorts() ([][]instance.HostPort, error)
+	WatchAPIHostPorts() (watcher.NotifyWatcher, error)
+}
+
+// APIAddressSetter is an interface that is provided to NewAPIAddressUpdater
+// whose SetAPIHostPorts method will be invoked whenever address changes occur.
+type APIAddressSetter interface {
+	SetAPIHostPorts(servers [][]instance.HostPort) error
+}
+
+// NewAPIAddressUpdater returns a worker.Worker that runs state.Cleanup()
+// if the CleanupWatcher signals documents marked for deletion.
+func NewAPIAddressUpdater(addresser APIAddresser, setter APIAddressSetter) worker.Worker {
+	return worker.NewNotifyWorker(&APIAddressUpdater{
+		addresser: addresser,
+		setter:    setter,
+	})
+}
+
+func (c *APIAddressUpdater) SetUp() (watcher.NotifyWatcher, error) {
+	return c.addresser.WatchAPIHostPorts()
+}
+
+func (c *APIAddressUpdater) Handle() error {
+	addresses, err := c.addresser.APIHostPorts()
+	if err != nil {
+		return fmt.Errorf("error getting addresses: %v", err)
+	}
+	if err := c.setter.SetAPIHostPorts(addresses); err != nil {
+		return fmt.Errorf("error setting addresses: %v", err)
+	}
+	logger.Infof("API addresses updated to %q", addresses)
+	return nil
+}
+
+func (c *APIAddressUpdater) TearDown() error {
+	return nil
+}

=== added file 'worker/apiaddressupdater/apiaddressupdater_test.go'
--- worker/apiaddressupdater/apiaddressupdater_test.go	1970-01-01 00:00:00 +0000
+++ worker/apiaddressupdater/apiaddressupdater_test.go	2014-04-02 09:44:43 +0000
@@ -0,0 +1,97 @@
+// Copyright 2014 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package apiaddressupdater_test
+
+import (
+	stdtesting "testing"
+	"time"
+
+	gc "launchpad.net/gocheck"
+
+	"launchpad.net/juju-core/instance"
+	jujutesting "launchpad.net/juju-core/juju/testing"
+	"launchpad.net/juju-core/state"
+	coretesting "launchpad.net/juju-core/testing"
+	"launchpad.net/juju-core/worker/apiaddressupdater"
+)
+
+func TestPackage(t *stdtesting.T) {
+	coretesting.MgoTestPackage(t)
+}
+
+type APIAddressUpdaterSuite struct {
+	jujutesting.JujuConnSuite
+}
+
+var _ = gc.Suite(&APIAddressUpdaterSuite{})
+
+type apiAddressSetter struct {
+	servers chan [][]instance.HostPort
+	err     error
+}
+
+func (s *apiAddressSetter) SetAPIHostPorts(servers [][]instance.HostPort) error {
+	s.servers <- servers
+	return s.err
+}
+
+func (s *APIAddressUpdaterSuite) TestStartStop(c *gc.C) {
+	st, _ := s.OpenAPIAsNewMachine(c, state.JobHostUnits)
+	worker := apiaddressupdater.NewAPIAddressUpdater(st.Machiner(), &apiAddressSetter{})
+	worker.Kill()
+	c.Assert(worker.Wait(), gc.IsNil)
+}
+
+func (s *APIAddressUpdaterSuite) TestAddressInitialUpdate(c *gc.C) {
+	updatedServers := [][]instance.HostPort{instance.AddressesWithPort(
+		instance.NewAddresses("localhost", "127.0.0.1"),
+		1234,
+	)}
+	err := s.State.SetAPIHostPorts(updatedServers)
+	c.Assert(err, gc.IsNil)
+
+	setter := &apiAddressSetter{servers: make(chan [][]instance.HostPort, 1)}
+	st, _ := s.OpenAPIAsNewMachine(c, state.JobHostUnits)
+	worker := apiaddressupdater.NewAPIAddressUpdater(st.Machiner(), setter)
+	defer func() { c.Assert(worker.Wait(), gc.IsNil) }()
+	defer worker.Kill()
+
+	// SetAPIHostPorts should be called with the initial value.
+	select {
+	case <-time.After(coretesting.LongWait):
+		c.Fatalf("timed out waiting for SetAPIHostPorts to be called")
+	case servers := <-setter.servers:
+		c.Assert(servers, gc.DeepEquals, updatedServers)
+	}
+}
+
+func (s *APIAddressUpdaterSuite) TestAddressChange(c *gc.C) {
+	setter := &apiAddressSetter{servers: make(chan [][]instance.HostPort, 1)}
+	st, _ := s.OpenAPIAsNewMachine(c, state.JobHostUnits)
+	worker := apiaddressupdater.NewAPIAddressUpdater(st.Machiner(), setter)
+	defer func() { c.Assert(worker.Wait(), gc.IsNil) }()
+	defer worker.Kill()
+
+	updatedServers := [][]instance.HostPort{instance.AddressesWithPort(
+		instance.NewAddresses("localhost", "127.0.0.1"),
+		1234,
+	)}
+	err := s.State.SetAPIHostPorts(updatedServers)
+	c.Assert(err, gc.IsNil)
+
+	// SetAPIHostPorts should be called with the initial value (empty),
+	// and then the updated value.
+	n := 0
+	select {
+	case <-time.After(coretesting.LongWait):
+		c.Fatalf("timed out waiting for SetAPIHostPorts to be called")
+	case servers := <-setter.servers:
+		if n == 0 {
+			c.Assert(servers, gc.HasLen, 0)
+		} else {
+			c.Assert(servers, gc.DeepEquals, updatedServers)
+		}
+		n++
+	}
+}