Merge lp:~thumper/juju-core/provisioner-reget-state-info into lp:~go-bot/juju-core/trunk

[Tim Penhey]

Provisioner gets addresses for each new machine.

The addresses are retrieved each time we attempt to start a machine. This
allows the provisioner to take advantage of any new api or state servers that
are started as part of a block of machines being provisioned at one time.

https://codereview.appspot.com/9824047/

Provisioner gets addresses for each new machine.

The addresses are retrieved each time we attempt to start a machine. This
allows the provisioner to take advantage of any new api or state servers that
are started as part of a block of machines being provisioned at one time.

There is some saving of addresses at this stage to make the existing tests
pass, even though this goes a little against what William has mentioned
before. If we have invalid config, the tests assume that we continue with old
info, whereas William has suggested that we pause provisioning while the
config is broken.

One behavioural tweak in this branch. If we do fail to setup authentication,
we no longer kill the task, but instead log and continue. We will attempt to
start it again next time through the loop.

https://codereview.appspot.com/9824047/

[Tim Penhey (thumper)]

Reviewers: mp+168562_code.launchpad.net,

Message:
Please take a look.

Description:
Provisioner gets addresses for each new machine.

The addresses are retrieved each time we attempt to start a machine.
This
allows the provisioner to take advantage of any new api or state servers
that
are started as part of a block of machines being provisioned at one
time.

There is some saving of addresses at this stage to make the existing
tests
pass, even though this goes a little against what William has mentioned
before. If we have invalid config, the tests assume that we continue
with old
info, whereas William has suggested that we pause provisioning while the
config is broken.

One behavioural tweak in this branch. If we do fail to setup
authentication,
we no longer kill the task, but instead log and continue. We will
attempt to
start it again next time through the loop.

https://code.launchpad.net/~thumper/juju-core/provisioner-reget-state-info/+merge/168562

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M worker/provisioner/export_test.go
   M worker/provisioner/provisioner.go
   M worker/provisioner/provisioner_task.go

[William Reade (fwereade)]

Strongly -1 on the clever bits here. Just barf on failure and I'll be
happy :).

https://codereview.appspot.com/9824047/diff/1/worker/provisioner/provisioner_task.go
File worker/provisioner/provisioner_task.go (right):

https://codereview.appspot.com/9824047/diff/1/worker/provisioner/provisioner_task.go#newcode304
worker/provisioner/provisioner_task.go:304: // Don't return a real
error, just try again next time.
This is problematic, because the watcher won't report that machine again
until it changes -- which it won't. For this to work, we need the
provisioner to be restarted regularly... but I'm hoping for a
provisioner that can run for weeks without breaking sweat.

https://codereview.appspot.com/9824047/diff/1/worker/provisioner/provisioner_task.go#newcode360
worker/provisioner/provisioner_task.go:360: // info if config becomes
invalid.
Honestly, I think this is sufficient justification to nuke the whole
provisioner; it'll be restarted in a few seconds, and for now I'd rather
not even try to provision a machine (which can be expected to cost our
users actual money) unless I'm as sure as I can be that I'll be able to
start a functioning machine agent...

https://codereview.appspot.com/9824047/

[William Reade (fwereade)]

Re what we do without a valid environment... hmm. Thinking aloud:

* we certainly start off with an invalid environ config, and only get a
valid one once whoever started the environment has connected for the
first time.

* today, on first startup, there will be one machine present; but it'll
be marked as provisioned already, so it won't cause env access directly;
and if we're tolerant of list-instances failures, we'll be able to
complete the first processMachines without difficulty. But I don't think
we can necessarily depend on that forever...

* in theory, it is impossible to set an invalid environ config in state,
except via the special mechanism at bootstrap time.

* in practice, an environ config can most certainly become invalid --
you can (1) replace the config with *any* valid config, which might ofc
be invalid in this context (changed environ type, for example, will
screw us most comprehensively) and (2) config update is racy anyway,
such that two users making valid changes concurrently can end up with an
environment config that reflects neither of their desires... and I bet
there's a way for that to cause invalidity. (oh, and, on inspection,
(3): it looks like values can't be dependably unset anyway. we probably
need that too. grar.)

* BUT the consequences of an invalid environ affect many more things
than just the provisioner.

* AND SO it would be folly to spend time and effort hardening the
provisioner against catastrophic environ failure when we could be fixing
the root cause.

* AND THUS you should (1) wait for a valid config at startup before
processing machines and (2) proceed otherwise as though the environment
configuration were always guaranteed consistent and valid from a juju
perspective and (3) use the time thereby saved to fix SetEnvironConfig.

* BUT ALSO remember that a valid *environ config* does not necessarily
contain valid *provider credentials*. So issues can always arise at
runtime anyway, and should I think be dealt with as follows:

   * StartInstance: record on machine via SetInstanceError.
   * AllInstances: log and ignore -- I don't think there are negative
consequences (out-of-date
     instance list is always possible anyway).
   * StopInstances: log and ignore, they'll be tried again next time we
processMachines. (We should
     add a periodic processMachines call so that we do at least handle
them in something resembling
     a timely fashion. Not too hard, I think?)

https://codereview.appspot.com/9824047/

[Tim Penhey (thumper)]

Please take a look.

https://codereview.appspot.com/9824047/diff/1/worker/provisioner/provisioner_task.go
File worker/provisioner/provisioner_task.go (right):

https://codereview.appspot.com/9824047/diff/1/worker/provisioner/provisioner_task.go#newcode304
worker/provisioner/provisioner_task.go:304: // Don't return a real
error, just try again next time.
On 2013/06/11 09:08:21, fwereade wrote:
> This is problematic, because the watcher won't report that machine
again until
> it changes -- which it won't. For this to work, we need the
provisioner to be
> restarted regularly... but I'm hoping for a provisioner that can run
for weeks
> without breaking sweat.

OK, reverted.

https://codereview.appspot.com/9824047/diff/1/worker/provisioner/provisioner_task.go#newcode360
worker/provisioner/provisioner_task.go:360: // info if config becomes
invalid.
On 2013/06/11 09:08:21, fwereade wrote:
> Honestly, I think this is sufficient justification to nuke the whole
> provisioner; it'll be restarted in a few seconds, and for now I'd
rather not
> even try to provision a machine (which can be expected to cost our
users actual
> money) unless I'm as sure as I can be that I'll be able to start a
functioning
> machine agent...

OK, nuking in progress.

https://codereview.appspot.com/9824047/

[William Reade (fwereade)]

LGTM

https://codereview.appspot.com/9824047/

[Ian Booth (wallyworld)]

LGTM

https://codereview.appspot.com/9824047/

=== modified file 'worker/provisioner/export_test.go'
--- worker/provisioner/export_test.go	2013-05-02 15:55:42 +0000
+++ worker/provisioner/export_test.go	2013-06-17 21:50:34 +0000
@@ -11,13 +11,13 @@
 // exported so we can manually close the Provisioners underlying
 // state connection.
 func (p *Provisioner) CloseState() error {
-	return p.st.Close()
+	return p.state.Close()
 }
 
 // exported so we can discover all machines visible to the
 // Provisioners state connection.
 func (p *Provisioner) AllMachines() ([]*state.Machine, error) {
-	return p.st.AllMachines()
+	return p.state.AllMachines()
 }
 
 func (o *configObserver) SetObserver(observer chan<- *config.Config) {

=== modified file 'worker/provisioner/provisioner.go'
--- worker/provisioner/provisioner.go	2013-06-11 02:15:31 +0000
+++ worker/provisioner/provisioner.go	2013-06-17 21:50:34 +0000
@@ -19,7 +19,7 @@
 
 // Provisioner represents a running provisioning worker.
 type Provisioner struct {
-	st        *state.State
+	state     *state.State
 	machineId string // Which machine runs the provisioner.
 	environ   environs.Environ
 	tomb      tomb.Tomb
@@ -46,7 +46,7 @@
 // and allocates them to the new machines.
 func NewProvisioner(st *state.State, machineId string) *Provisioner {
 	p := &Provisioner{
-		st:        st,
+		state:     st,
 		machineId: machineId,
 	}
 	go func() {
@@ -57,7 +57,7 @@
 }
 
 func (p *Provisioner) loop() error {
-	environWatcher := p.st.WatchEnvironConfig()
+	environWatcher := p.state.WatchEnvironConfig()
 	defer watcher.Stop(environWatcher, &p.tomb)
 
 	var err error
@@ -66,27 +66,18 @@
 		return err
 	}
 
-	// Get a new StateInfo from the environment: the one used to
-	// launch the agent may refer to localhost, which will be
-	// unhelpful when attempting to run an agent on a new machine.
-	stateInfo, apiInfo, err := p.environ.StateInfo()
-	if err != nil {
-		return err
-	}
-
 	// Start a new worker for the environment provider.
 
 	// Start responding to changes in machines, and to any further updates
 	// to the environment config.
-	machinesWatcher := p.st.WatchEnvironMachines()
+	machinesWatcher := p.state.WatchEnvironMachines()
 	environmentBroker := newEnvironBroker(p.environ)
 	environmentProvisioner := newProvisionerTask(
 		p.machineId,
-		p.st,
+		p.state,
 		machinesWatcher,
 		environmentBroker,
-		stateInfo,
-		apiInfo)
+	)
 	defer watcher.Stop(environmentProvisioner, &p.tomb)
 
 	for {
@@ -103,6 +94,11 @@
 			}
 			if err := p.setConfig(cfg); err != nil {
 				logger.Errorf("loaded invalid environment configuration: %v", err)
+				// We *shouldn't* ever get into a state here where we have an
+				// invalid config.  If we do, stop the task and let jujud
+				// restart the provisioner, which will then wait for valid
+				// config.
+				return err
 			}
 		}
 	}

=== modified file 'worker/provisioner/provisioner_task.go'
--- worker/provisioner/provisioner_task.go	2013-06-14 01:02:53 +0000
+++ worker/provisioner/provisioner_task.go	2013-06-17 21:50:34 +0000
@@ -30,25 +30,17 @@
 	Changes() <-chan []string
 }
 
-type MachineGetter interface {
-	Machine(id string) (*state.Machine, error)
-}
-
 func newProvisionerTask(
 	machineId string,
-	machineGetter MachineGetter,
+	st *state.State,
 	watcher Watcher,
 	broker Broker,
-	stateInfo *state.Info,
-	apiInfo *api.Info,
 ) ProvisionerTask {
 	task := &provisionerTask{
 		machineId:      machineId,
-		machineGetter:  machineGetter,
+		state:          st,
 		machineWatcher: watcher,
 		broker:         broker,
-		stateInfo:      stateInfo,
-		apiInfo:        apiInfo,
 		machines:       make(map[string]*state.Machine),
 	}
 	go func() {
@@ -60,12 +52,10 @@
 
 type provisionerTask struct {
 	machineId      string
-	machineGetter  MachineGetter
+	state          *state.State
 	machineWatcher Watcher
 	broker         Broker
 	tomb           tomb.Tomb
-	stateInfo      *state.Info
-	apiInfo        *api.Info
 
 	// instance id -> instance
 	instances map[state.InstanceId]instance.Instance
@@ -174,7 +164,7 @@
 	// change list.
 	// TODO(thumper): update for API server later to get all machines in one go.
 	for _, id := range ids {
-		machine, err := task.machineGetter.Machine(id)
+		machine, err := task.state.Machine(id)
 		switch {
 		case errors.IsNotFoundError(err):
 			logger.Debugf("machine %q not found in state", id)
@@ -306,7 +296,7 @@
 	// state.Info as the PA.
 	stateInfo, apiInfo, err := task.setupAuthentication(machine)
 	if err != nil {
-		logger.Errorf("failed to setup authentication: %v", err)
+		logger.Warningf("failed to setup authentication for machine %q: %v", machine.Id(), err)
 		return err
 	}
 	cons, err := machine.Constraints()
@@ -355,6 +345,22 @@
 }
 
 func (task *provisionerTask) setupAuthentication(machine *state.Machine) (*state.Info, *api.Info, error) {
+	// Grab a new list of state and api addresses each time along with the
+	// cert, as these can change during a processing loop.  If for example the
+	// provisioner was starting 100 machines in one go, some of those may be
+	// new api servers.  We should take advantage of those ASAP.
+	stateAddresses, err := task.state.Addresses()
+	if err != nil {
+		// This will only return an error if the config becomes invalid.  In
+		// those situations, the provisioner bombs out and is restarted.
+		return nil, nil, fmt.Errorf("cannot get addresses from state: %v", err)
+	}
+	apiAddresses, err := task.state.APIAddresses()
+	if err != nil {
+		// Same for the api addresses.  We technically shouldn't get here if
+		// we didn't fail before, but best to check.
+		return nil, nil, fmt.Errorf("cannot get api addresses from state: %v", err)
+	}
 	password, err := utils.RandomPassword()
 	if err != nil {
 		return nil, nil, fmt.Errorf("cannot make password for machine %v: %v", machine, err)
@@ -362,11 +368,16 @@
 	if err := machine.SetMongoPassword(password); err != nil {
 		return nil, nil, fmt.Errorf("cannot set password for machine %v: %v", machine, err)
 	}
-	stateInfo := *task.stateInfo
-	stateInfo.Tag = machine.Tag()
-	stateInfo.Password = password
-	apiInfo := *task.apiInfo
-	apiInfo.Tag = machine.Tag()
-	apiInfo.Password = password
-	return &stateInfo, &apiInfo, nil
+	cert := task.state.CACert()
+	return &state.Info{
+			Addrs:    stateAddresses,
+			CACert:   cert,
+			Tag:      machine.Tag(),
+			Password: password,
+		}, &api.Info{
+			Addrs:    apiAddresses,
+			CACert:   cert,
+			Tag:      machine.Tag(),
+			Password: password,
+		}, nil
 }

=== modified file 'worker/provisioner/provisioner_test.go'
--- worker/provisioner/provisioner_test.go	2013-06-14 01:23:48 +0000
+++ worker/provisioner/provisioner_test.go	2013-06-17 21:50:34 +0000
@@ -361,27 +361,6 @@
 	s.checkStartInstance(c, m)
 }
 
-func (s *ProvisionerSuite) TestProvisioningDoesOccurAfterInvalidEnvironmentPublished(c *C) {
-	p := provisioner.NewProvisioner(s.State, "0")
-	defer stop(c, p)
-
-	// place a new machine into the state
-	m, err := s.State.AddMachine(config.DefaultSeries, state.JobHostUnits)
-	c.Assert(err, IsNil)
-
-	s.checkStartInstance(c, m)
-
-	err = s.invalidateEnvironment(c)
-	c.Assert(err, IsNil)
-
-	// create a second machine
-	m, err = s.State.AddMachine(config.DefaultSeries, state.JobHostUnits)
-	c.Assert(err, IsNil)
-
-	// the PA should create it using the old environment
-	s.checkStartInstance(c, m)
-}
-
 func (s *ProvisionerSuite) TestProvisioningDoesNotProvisionTheSameMachineAfterRestart(c *C) {
 	p := provisioner.NewProvisioner(s.State, "0")
 	defer stop(c, p)
@@ -466,55 +445,3 @@
 	c.Assert(err, IsNil)
 	c.Assert(m0.Life(), Equals, state.Dying)
 }
-
-func (s *ProvisionerSuite) TestProvisioningRecoversAfterInvalidEnvironmentPublished(c *C) {
-	p := provisioner.NewProvisioner(s.State, "0")
-	defer stop(c, p)
-
-	// place a new machine into the state
-	m, err := s.State.AddMachine(config.DefaultSeries, state.JobHostUnits)
-	c.Assert(err, IsNil)
-	s.checkStartInstance(c, m)
-
-	err = s.invalidateEnvironment(c)
-	c.Assert(err, IsNil)
-	s.State.StartSync()
-
-	// create a second machine
-	m, err = s.State.AddMachine(config.DefaultSeries, state.JobHostUnits)
-	c.Assert(err, IsNil)
-
-	// the PA should create it using the old environment
-	s.checkStartInstance(c, m)
-
-	err = s.fixEnvironment()
-	c.Assert(err, IsNil)
-
-	// insert our observer
-	cfgObserver := make(chan *config.Config, 1)
-	p.SetObserver(cfgObserver)
-
-	cfg, err := s.State.EnvironConfig()
-	c.Assert(err, IsNil)
-	attrs := cfg.AllAttrs()
-	attrs["secret"] = "beef"
-	cfg, err = config.New(attrs)
-	c.Assert(err, IsNil)
-	err = s.State.SetEnvironConfig(cfg)
-
-	s.State.StartSync()
-
-	// wait for the PA to load the new configuration
-	select {
-	case <-cfgObserver:
-	case <-time.After(200 * time.Millisecond):
-		c.Fatalf("PA did not action config change")
-	}
-
-	// create a third machine
-	m, err = s.State.AddMachine(config.DefaultSeries, state.JobHostUnits)
-	c.Assert(err, IsNil)
-
-	// the PA should create it using the new environment
-	s.checkStartInstanceCustom(c, m, "beef", constraints.Value{})
-}