Merge lp:~wallyworld/juju-core/provisioner-retry into lp:~go-bot/juju-core/trunk

[Ian Booth]

Provisioner retries failed machines

Introduce a new watcher - WatchMachineErrorRetry. This
watcher fires when the provisioner task should attempt
to re-provision any machines with transient errors. It
is done as a separate watcher to avoid changing the
semantics of the exisitng machine watcher close to
cutting a release, and to more easily allow the
flexibility of doing things like queuing retries
separately with different priorities etc. It's still
early days and the implemenation is still evolving
because the watcher currently just triggers every minute.
It's an internal implementation detail so can be changed
if required. Future work needs to make the watcher smarter
and able to interpret errors returned by the providers
in order to know when to retry.

https://codereview.appspot.com/80340043/

Provisioner retries failed machines

Introduce a new watcher - WatchMachineErrorRetry. This
watcher fires when the provisioner task should attempt
to re-provision any machines with transient errors. It
is done as a separate watcher to avoid changing the
semantics of the exisitng machine watcher close to
cutting a release, and to more easily allow the
flexibility of doing things like queuing retries
separately with different priorities etc. It's still
early days and the implemenation is still evolving
because the watcher currently just triggers every minute.
It's an internal implementation detail so can be changed
if required. Future work needs to make the watcher smarter
and able to interpret errors returned by the providers
in order to know when to retry.

https://codereview.appspot.com/80340043/

[Ian Booth (wallyworld)]

Reviewers: mp+212768_code.launchpad.net,

Message:
Please take a look.

Description:
Provisioner retries failed machines

Introduce a new watcher - WatchMachineErrorRetry. This
watcher fires when the provisioner task should attempt
to re-provision any machines with transient errors. It
is done as a separate watcher to avoid changing the
semantics of the exisitng machine watcher close to
cutting a release, and to more easily allow the
flexibility of doing things like queuing retries
separately with different priorities etc. It's still
early days and the implemenation is still evolving
because the watcher currently just triggers every minute.
It's an internal implementation detail so can be changed
if required. Future work needs to make the watcher smarter
and able to interpret errors returned by the providers
in order to know when to retry.

https://code.launchpad.net/~wallyworld/juju-core/provisioner-retry/+merge/212768

Requires:
https://code.launchpad.net/~wallyworld/juju-core/machineswithtransienterrors-api/+merge/212749

(do not edit description out of merge proposal)

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

Affected files (+286, -26 lines):
   A [revision details]
   M state/api/provisioner/provisioner.go
   A state/apiserver/provisioner/machineerror.go
   M state/apiserver/provisioner/provisioner.go
   M state/apiserver/provisioner/provisioner_test.go
   M worker/provisioner/provisioner.go
   M worker/provisioner/provisioner_task.go
   M worker/provisioner/provisioner_test.go

[Ian Booth (wallyworld)]

Please take a look.

https://codereview.appspot.com/80340043/

[John A Meinel (jameinel)]

I like where this is going, I wish our code base would let it be
smaller.

I'm a little concerned that the new test is going to be timing
dependent, but otherwise LGTM.

https://codereview.appspot.com/80340043/diff/20001/state/apiserver/provisioner/machineerror.go
File state/apiserver/provisioner/machineerror.go (right):

https://codereview.appspot.com/80340043/diff/20001/state/apiserver/provisioner/machineerror.go#newcode20
state/apiserver/provisioner/machineerror.go:20:
I really wish we could inherit from a common NotifyWatcher
implementation and then just implement the "loop()" in our custom code.

I think we could actually do that with a NotifyWatchLooper or some such
that did all of this work and just took an

type interface Looper {
   Loop(out chan struct{}) error
}

I've done some bits like that in the Workers, where I implemented
NotifyWorker that takes a Handler that triggers when the changes are
actually interesting.

Anyway, not something you have to do here, because our pattern in code
has definitely been to just copy & paste the boilerplate.

https://codereview.appspot.com/80340043/diff/20001/state/apiserver/provisioner/machineerror.go#newcode66
state/apiserver/provisioner/machineerror.go:66: // triggering every 2
minutes.
Your comment here doesn't match the actual ErrorRetryWaitDelay. Maybe
just reference the variable instead of an explicit time ?

https://codereview.appspot.com/80340043/diff/20001/state/apiserver/provisioner/machineerror.go#newcode73
state/apiserver/provisioner/machineerror.go:73: case
<-time.After(ErrorRetryWaitDelay):
But all of this boiler plate exists *only* for this line. (when this
channel triggers, trigger an output).

https://codereview.appspot.com/80340043/diff/20001/worker/provisioner/provisioner_test.go
File worker/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/80340043/diff/20001/worker/provisioner/provisioner_test.go#newcode773
worker/provisioner/provisioner_test.go:773:
s.PatchValue(&apiserverprovisioner.ErrorRetryWaitDelay,
50*time.Millisecond)
Can we use testing.ShortWait here instead of another hard-coded time?

https://codereview.appspot.com/80340043/diff/20001/worker/provisioner/provisioner_test.go#newcode804
worker/provisioner/provisioner_test.go:804: // Machine 4 is never
provisioned.
I don't understand why Machine 4 is listed as never provisioned, when
the StartInstance calls claim that it will be provisioned after 2
retries.
Is this sensitive to the timeout times? (we assume we poll at exactly
50ms, and we get 2 polls in the 100ms wait above?)

Anything that requires the timing to get right tends to fail on the bot.

It may be that checkStartInstance properly retries until success, or
something else.

Is there even a reason why we need to have ErrorRetryWaitDelay not
something like even 5ms?

https://codereview.appspot.com/80340043/

[Ian Booth (wallyworld)]

Please take a look.

https://codereview.appspot.com/80340043/diff/20001/state/apiserver/provisioner/machineerror.go
File state/apiserver/provisioner/machineerror.go (right):

https://codereview.appspot.com/80340043/diff/20001/state/apiserver/provisioner/machineerror.go#newcode20
state/apiserver/provisioner/machineerror.go:20:
On 2014/03/26 07:22:42, jameinel wrote:
> I really wish we could inherit from a common NotifyWatcher
implementation and
> then just implement the "loop()" in our custom code.

> I think we could actually do that with a NotifyWatchLooper or some
such that did
> all of this work and just took an

> type interface Looper {
> Loop(out chan struct{}) error
> }

> I've done some bits like that in the Workers, where I implemented
NotifyWorker
> that takes a Handler that triggers when the changes are actually
interesting.

> Anyway, not something you have to do here, because our pattern in code
has
> definitely been to just copy & paste the boilerplate.

Yeah, I agree. Tim and I have looked into this before and because Go
doesn't have things like virtual methods or generics it is really hard
to avoid all the boiler plate. I was in a rush for this branch to try
and make the 1.18 deadline so I didn't look into it oo much for this
branch.

https://codereview.appspot.com/80340043/diff/20001/state/apiserver/provisioner/machineerror.go#newcode66
state/apiserver/provisioner/machineerror.go:66: // triggering every 2
minutes.
On 2014/03/26 07:22:42, jameinel wrote:
> Your comment here doesn't match the actual ErrorRetryWaitDelay. Maybe
just
> reference the variable instead of an explicit time ?

Doh, will fix.

https://codereview.appspot.com/80340043/diff/20001/state/apiserver/provisioner/machineerror.go#newcode73
state/apiserver/provisioner/machineerror.go:73: case
<-time.After(ErrorRetryWaitDelay):
On 2014/03/26 07:22:42, jameinel wrote:
> But all of this boiler plate exists *only* for this line. (when this
channel
> triggers, trigger an output).

For now yes. But this is just the start. There's a lot more business
logic to be added. The time based trigger is just to get something
working.

https://codereview.appspot.com/80340043/diff/20001/worker/provisioner/provisioner_test.go
File worker/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/80340043/diff/20001/worker/provisioner/provisioner_test.go#newcode773
worker/provisioner/provisioner_test.go:773:
s.PatchValue(&apiserverprovisioner.ErrorRetryWaitDelay,
50*time.Millisecond)
On 2014/03/26 07:22:42, jameinel wrote:
> Can we use testing.ShortWait here instead of another hard-coded time?

Sure. Originally it wasn't ShortWait but it is now so I'll change it.

https://codereview.appspot.com/80340043/diff/20001/worker/provisioner/provisioner_test.go#newcode804
worker/provisioner/provisioner_test.go:804: // Machine 4 is never
provisioned.
On 2014/03/26 07:22:42, jameinel wrote:
> I don't understand why Machine 4 is listed as never provisioned, when
the
> StartInstance calls claim that it will be provisioned after 2 retries.
> Is this sensitive to the timeout times? (we assume we poll at exactly
50ms, and
> we get 2 polls in the 100ms wait above?)

Only machine 3 ...

[Ian Booth (wallyworld)]

Please take a look.

https://codereview.appspot.com/80340043/

[William Reade (fwereade)]

I would be so much happier if this was implemented as a StringsWatcher,
that sent down the ids-to-be-retried list, and we were able to call
exactly the same processMachines method -- and thus be able to convert
everything in one go to use the bulk apis that already exist.

https://codereview.appspot.com/80340043/diff/60001/state/machine_test.go
File state/machine_test.go (right):

https://codereview.appspot.com/80340043/diff/60001/state/machine_test.go#newcode1058
state/machine_test.go:1058: c.Assert(err, gc.ErrorMatches, `cannot set
status "pending"`)
We should also be able to set "pending" when it's currently in "error"
state, right?

https://codereview.appspot.com/80340043/

[Go Bot (go-bot)]

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

ok launchpad.net/juju-core 0.012s
ok launchpad.net/juju-core/agent 1.054s
ok launchpad.net/juju-core/agent/mongo 0.574s
ok launchpad.net/juju-core/agent/tools 0.178s
ok launchpad.net/juju-core/bzr 4.875s
ok launchpad.net/juju-core/cert 2.044s
ok launchpad.net/juju-core/charm 0.383s
? launchpad.net/juju-core/charm/hooks [no test files]
? launchpad.net/juju-core/charm/testing [no test files]
ok launchpad.net/juju-core/cloudinit 0.030s
ok launchpad.net/juju-core/cloudinit/sshinit 0.798s
ok launchpad.net/juju-core/cmd 0.179s
ok launchpad.net/juju-core/cmd/charm-admin 0.276s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 209.526s
ok launchpad.net/juju-core/cmd/jujud 64.785s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 11.406s
? launchpad.net/juju-core/cmd/plugins/juju-restore [no test files]
ok launchpad.net/juju-core/cmd/plugins/local 0.151s
? launchpad.net/juju-core/cmd/plugins/local/juju-local [no test files]
ok launchpad.net/juju-core/constraints 0.024s
ok launchpad.net/juju-core/container 0.043s
ok launchpad.net/juju-core/container/factory 0.045s
ok launchpad.net/juju-core/container/kvm 0.175s
ok launchpad.net/juju-core/container/kvm/mock 0.052s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 4.331s
? launchpad.net/juju-core/container/lxc/mock [no test files]
? launchpad.net/juju-core/container/lxc/testing [no test files]
? launchpad.net/juju-core/container/testing [no test files]
ok launchpad.net/juju-core/downloader 5.250s
ok launchpad.net/juju-core/environs 2.449s
ok launchpad.net/juju-core/environs/bootstrap 11.134s
ok launchpad.net/juju-core/environs/cloudinit 0.443s
ok launchpad.net/juju-core/environs/config 1.533s
ok launchpad.net/juju-core/environs/configstore 0.034s
ok launchpad.net/juju-core/environs/filestorage 0.030s
ok launchpad.net/juju-core/environs/httpstorage 0.607s
ok launchpad.net/juju-core/environs/imagemetadata 0.422s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.045s
ok launchpad.net/juju-core/environs/jujutest 0.201s
ok launchpad.net/juju-core/environs/manual 13.449s
ok launchpad.net/juju-core/environs/simplestreams 0.282s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 0.888s
ok launchpad.net/juju-core/environs/storage 0.960s
ok launchpad.net/juju-core/environs/sync 44.468s
ok launchpad.net/juju-core/environs/testing 0.157s
ok launchpad.net/juju-core/environs/tools 5.056s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.011s
ok launchpad.net/juju-core/instance 0.025s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 18.263s
ok launchpad.net/juju-core/juju/arch 0.014s...

=== modified file 'state/api/provisioner/provisioner.go'
--- state/api/provisioner/provisioner.go	2014-03-25 03:10:55 +0000
+++ state/api/provisioner/provisioner.go	2014-03-26 11:18:52 +0000
@@ -70,6 +70,19 @@
 	return w, nil
 }
 
+func (st *State) WatchMachineErrorRetry() (watcher.NotifyWatcher, error) {
+	var result params.NotifyWatchResult
+	err := st.call("WatchMachineErrorRetry", nil, &result)
+	if err != nil {
+		return nil, err
+	}
+	if err := result.Error; err != nil {
+		return nil, result.Error
+	}
+	w := watcher.NewNotifyWatcher(st.caller, result)
+	return w, nil
+}
+
 // StateAddresses returns the list of addresses used to connect to the state.
 func (st *State) StateAddresses() ([]string, error) {
 	var result params.StringsResult

=== modified file 'state/api/provisioner/provisioner_test.go'
--- state/api/provisioner/provisioner_test.go	2014-03-26 05:16:31 +0000
+++ state/api/provisioner/provisioner_test.go	2014-03-26 11:18:52 +0000
@@ -121,17 +121,7 @@
 	c.Assert(err, gc.IsNil)
 	err = machine.SetStatus(params.StatusError, "blah", params.StatusData{"transient": true})
 	c.Assert(err, gc.IsNil)
-	password, err := utils.RandomPassword()
-	c.Assert(err, gc.IsNil)
-	err = machine.SetPassword(password)
-	c.Assert(err, gc.IsNil)
-	err = machine.SetProvisioned("i-manager", "fake_nonce", nil)
-	c.Assert(err, gc.IsNil)
-	st := s.OpenAPIAsMachine(c, machine.Tag(), password, "fake_nonce")
-	c.Assert(s.st, gc.NotNil)
-	p := st.Provisioner()
-
-	machines, info, err := p.MachinesWithTransientErrors()
+	machines, info, err := s.provisioner.MachinesWithTransientErrors()
 	c.Assert(err, gc.IsNil)
 	c.Assert(machines, gc.HasLen, 1)
 	c.Assert(machines[0].Id(), gc.Equals, "1")

=== added file 'state/apiserver/provisioner/machineerror.go'
--- state/apiserver/provisioner/machineerror.go	1970-01-01 00:00:00 +0000
+++ state/apiserver/provisioner/machineerror.go	2014-03-26 11:18:52 +0000
@@ -0,0 +1,80 @@
+// Copyright 2014 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package provisioner
+
+import (
+	"time"
+
+	"launchpad.net/tomb"
+
+	"launchpad.net/juju-core/state"
+)
+
+// machineErrorRetry is a notify watcher that fires when it is
+// appropriate to retry provisioning machines with transient errors.
+type machineErrorRetry struct {
+	tomb tomb.Tomb
+	out  chan struct{}
+}
+
+func newWatchMachineErrorRetry() state.NotifyWatcher {
+	w := &machineErrorRetry{
+		out: make(chan struct{}),
+	}
+	go func() {
+		defer w.tomb.Done()
+		defer close(w.out)
+		w.tomb.Kill(w.loop())
+	}()
+	return w
+}
+
+// Stop stops the watcher, and returns any error encountered while running
+// or shutting down.
+func (w *machineErrorRetry) Stop() error {
+	w.Kill()
+	return w.Wait()
+}
+
+// Kill kills the watcher without waiting for it to shut down.
+func (w *machineErrorRetry) Kill() {
+	w.tomb.Kill(nil)
+}
+
+// Wait waits for the watcher to die and returns any
+// error encountered when it was running.
+func (w *machineErrorRetry) Wait() error {
+	return w.tomb.Wait()
+}
+
+// Err returns any error encountered while running or shutting down, or
+// tomb.ErrStillAlive if the watcher is still running.
+func (w *machineErrorRetry) Err() error {
+	return w.tomb.Err()
+}
+
+// Changes returns the event channel for the machineErrorRetry watcher.
+func (w *machineErrorRetry) Changes() <-chan struct{} {
+	return w.out
+}
+
+// ErrorRetryWaitDelay is the poll time currently used to trigger the watcher.
+var ErrorRetryWaitDelay = 1 * time.Minute
+
+// The initial implementation of this watcher simply acts as a poller,
+// triggering every ErrorRetryWaitDelay minutes.
+func (w *machineErrorRetry) loop() error {
+	out := w.out
+	for {
+		select {
+		case <-w.tomb.Dying():
+			return tomb.ErrDying
+		case <-time.After(ErrorRetryWaitDelay):
+			out = w.out
+		case out <- struct{}{}:
+			out = nil
+		}
+	}
+	return nil
+}

=== modified file 'state/apiserver/provisioner/provisioner.go'
--- state/apiserver/provisioner/provisioner.go	2014-03-26 05:16:31 +0000
+++ state/apiserver/provisioner/provisioner.go	2014-03-26 11:18:52 +0000
@@ -27,10 +27,11 @@
 	*common.EnvironMachinesWatcher
 	*common.InstanceIdGetter
 
-	st          *state.State
-	resources   *common.Resources
-	authorizer  common.Authorizer
-	getAuthFunc common.GetAuthFunc
+	st                  *state.State
+	resources           *common.Resources
+	authorizer          common.Authorizer
+	getAuthFunc         common.GetAuthFunc
+	getCanWatchMachines common.GetAuthFunc
 }
 
 // NewProvisionerAPI creates a new server-side ProvisionerAPI facade.
@@ -87,6 +88,7 @@
 		resources:              resources,
 		authorizer:             authorizer,
 		getAuthFunc:            getAuthFunc,
+		getCanWatchMachines:    getCanReadSecrets,
 	}, nil
 }
 
@@ -238,6 +240,11 @@
 		if !canAccessFunc(machine.Tag()) {
 			continue
 		}
+		if _, provisionedErr := machine.InstanceId(); provisionedErr == nil {
+			// Machine may have been provisioned but machiner hasn't set the
+			// status to Started yet.
+			continue
+		}
 		result := params.StatusResult{}
 		if result.Status, result.Info, result.Data, err = machine.Status(); err != nil {
 			continue
@@ -318,3 +325,24 @@
 	}
 	return result, nil
 }
+
+// WatchMachineErrorRetry returns a NotifyWatcher that notifies when
+// the provisioner should retry provisioning machines with transient errors.
+func (p *ProvisionerAPI) WatchMachineErrorRetry() (params.NotifyWatchResult, error) {
+	result := params.NotifyWatchResult{}
+	canWatch, err := p.getCanWatchMachines()
+	if err != nil {
+		return params.NotifyWatchResult{}, err
+	}
+	if !canWatch("") {
+		return result, common.ErrPerm
+	}
+	watch := newWatchMachineErrorRetry()
+	// Consume any initial event and forward it to the result.
+	if _, ok := <-watch.Changes(); ok {
+		result.NotifyWatcherId = p.resources.Register(watch)
+	} else {
+		return result, watcher.MustErr(watch)
+	}
+	return result, nil
+}

=== modified file 'state/apiserver/provisioner/provisioner_test.go'
--- state/apiserver/provisioner/provisioner_test.go	2014-03-26 05:16:31 +0000
+++ state/apiserver/provisioner/provisioner_test.go	2014-03-26 11:18:52 +0000
@@ -57,7 +57,7 @@
 	if withStateServer {
 		s.machines = append(s.machines, testing.AddStateServerMachine(c, s.State))
 	}
-	for i := 0; i < 4; i++ {
+	for i := 0; i < 5; i++ {
 		machine, err := s.State.AddMachine("quantal", state.JobHostUnits)
 		c.Check(err, gc.IsNil)
 		s.machines = append(s.machines, machine)
@@ -120,6 +120,7 @@
 			{Tag: s.machines[1].Tag(), Password: "xxx1-1234567890123457890"},
 			{Tag: s.machines[2].Tag(), Password: "xxx2-1234567890123457890"},
 			{Tag: s.machines[3].Tag(), Password: "xxx3-1234567890123457890"},
+			{Tag: s.machines[4].Tag(), Password: "xxx4-1234567890123457890"},
 			{Tag: "machine-42", Password: "foo"},
 			{Tag: "unit-foo-0", Password: "zzz"},
 			{Tag: "service-bar", Password: "abc"},
@@ -133,6 +134,7 @@
 			{nil},
 			{nil},
 			{nil},
+			{nil},
 			{apiservertesting.NotFoundError("machine 42")},
 			{apiservertesting.ErrUnauthorized},
 			{apiservertesting.ErrUnauthorized},
@@ -357,6 +359,13 @@
 	c.Assert(err, gc.IsNil)
 	err = s.machines[3].SetStatus(params.StatusError, "error", nil)
 	c.Assert(err, gc.IsNil)
+	// Machine 4 is provisioned but error not reset yet.
+	err = s.machines[4].SetStatus(params.StatusError, "transient error",
+		params.StatusData{"transient": true, "foo": "bar"})
+	c.Assert(err, gc.IsNil)
+	hwChars := instance.MustParseHardware("arch=i386", "mem=4G")
+	err = s.machines[4].SetProvisioned("i-am", "fake_nonce", &hwChars)
+	c.Assert(err, gc.IsNil)
 
 	result, err := s.provisioner.MachinesWithTransientErrors()
 	c.Assert(err, gc.IsNil)
@@ -705,7 +714,7 @@
 	c.Assert(err, gc.IsNil)
 	c.Assert(result, gc.DeepEquals, params.StringsWatchResult{
 		StringsWatcherId: "1",
-		Changes:          []string{"0", "1", "2", "3"},
+		Changes:          []string{"0", "1", "2", "3", "4"},
 	})
 
 	// Verify the resources were registered and stop them when done.
@@ -916,3 +925,35 @@
 		Result: s.State.CACert(),
 	})
 }
+
+func (s *withoutStateServerSuite) TestWatchMachineErrorRetry(c *gc.C) {
+	s.PatchValue(&provisioner.ErrorRetryWaitDelay, 2*coretesting.ShortWait)
+	c.Assert(s.resources.Count(), gc.Equals, 0)
+
+	_, err := s.provisioner.WatchMachineErrorRetry()
+	c.Assert(err, gc.IsNil)
+
+	// Verify the resources were registered and stop them when done.
+	c.Assert(s.resources.Count(), gc.Equals, 1)
+	resource := s.resources.Get("1")
+	defer statetesting.AssertStop(c, resource)
+
+	// Check that the Watch has consumed the initial event ("returned"
+	// in the Watch call)
+	wc := statetesting.NewNotifyWatcherC(c, s.State, resource.(state.NotifyWatcher))
+	wc.AssertNoChange()
+
+	// We should now get a time triggered change.
+	wc.AssertOneChange()
+
+	// Make sure WatchMachineErrorRetry fails with a machine agent login.
+	anAuthorizer := s.authorizer
+	anAuthorizer.MachineAgent = true
+	anAuthorizer.EnvironManager = false
+	aProvisioner, err := provisioner.NewProvisionerAPI(s.State, s.resources, anAuthorizer)
+	c.Assert(err, gc.IsNil)
+
+	result, err := aProvisioner.WatchMachineErrorRetry()
+	c.Assert(err, gc.ErrorMatches, "permission denied")
+	c.Assert(result, gc.DeepEquals, params.NotifyWatchResult{})
+}

=== modified file 'state/machine.go'
--- state/machine.go	2014-03-25 09:15:00 +0000
+++ state/machine.go	2014-03-26 11:18:52 +0000
@@ -960,7 +960,11 @@
 		StatusInfo: info,
 		StatusData: data,
 	}
-	if err := doc.validateSet(); err != nil {
+	// If a machine is not yet provisioned, we allow its status
+	// to be set back to pending (when a retry is to occur).
+	_, err := m.InstanceId()
+	allowPending := IsNotProvisionedError(err)
+	if err := doc.validateSet(allowPending); err != nil {
 		return err
 	}
 	ops := []txn.Op{{

=== modified file 'state/machine_test.go'
--- state/machine_test.go	2014-03-26 06:28:38 +0000
+++ state/machine_test.go	2014-03-26 11:18:52 +0000
@@ -1052,8 +1052,6 @@
 func (s *MachineSuite) TestGetSetStatusWhileAlive(c *gc.C) {
 	err := s.machine.SetStatus(params.StatusError, "", nil)
 	c.Assert(err, gc.ErrorMatches, `cannot set status "error" without info`)
-	err = s.machine.SetStatus(params.StatusPending, "", nil)
-	c.Assert(err, gc.ErrorMatches, `cannot set status "pending"`)
 	err = s.machine.SetStatus(params.StatusDown, "", nil)
 	c.Assert(err, gc.ErrorMatches, `cannot set status "down"`)
 	err = s.machine.SetStatus(params.Status("vliegkat"), "orville", nil)
@@ -1086,6 +1084,16 @@
 	})
 }
 
+func (s *MachineSuite) TestSetStatusPending(c *gc.C) {
+	err := s.machine.SetStatus(params.StatusPending, "", nil)
+	c.Assert(err, gc.IsNil)
+	// Cannot set status to pending once a machine is provisioned.
+	err = s.machine.SetProvisioned("umbrella/0", "fake_nonce", nil)
+	c.Assert(err, gc.IsNil)
+	err = s.machine.SetStatus(params.StatusPending, "", nil)
+	c.Assert(err, gc.ErrorMatches, `cannot set status "pending"`)
+}
+
 func (s *MachineSuite) TestGetSetStatusWhileNotAlive(c *gc.C) {
 	// When Dying set/get should work.
 	err := s.machine.Destroy()

=== modified file 'state/status.go'
--- state/status.go	2014-03-19 23:07:33 +0000
+++ state/status.go	2014-03-26 11:18:52 +0000
@@ -26,12 +26,16 @@
 
 // validateSet returns an error if the statusDoc does not represent a sane
 // SetStatus operation.
-func (doc statusDoc) validateSet() error {
+func (doc statusDoc) validateSet(allowPending bool) error {
 	if !doc.Status.Valid() {
 		return fmt.Errorf("cannot set invalid status %q", doc.Status)
 	}
 	switch doc.Status {
-	case params.StatusPending, params.StatusDown:
+	case params.StatusPending:
+		if !allowPending {
+			return fmt.Errorf("cannot set status %q", doc.Status)
+		}
+	case params.StatusDown:
 		return fmt.Errorf("cannot set status %q", doc.Status)
 	case params.StatusError:
 		if doc.StatusInfo == "" {

=== modified file 'state/unit.go'
--- state/unit.go	2014-03-19 23:07:33 +0000
+++ state/unit.go	2014-03-26 11:18:52 +0000
@@ -562,7 +562,7 @@
 		StatusInfo: info,
 		StatusData: data,
 	}
-	if err := doc.validateSet(); err != nil {
+	if err := doc.validateSet(false); err != nil {
 		return err
 	}
 	ops := []txn.Op{{

=== modified file 'worker/provisioner/provisioner.go'
--- worker/provisioner/provisioner.go	2014-03-05 19:41:34 +0000
+++ worker/provisioner/provisioner.go	2014-03-26 11:18:52 +0000
@@ -14,6 +14,7 @@
 	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/instance"
 	apiprovisioner "launchpad.net/juju-core/state/api/provisioner"
+	apiwatcher "launchpad.net/juju-core/state/api/watcher"
 	"launchpad.net/juju-core/state/watcher"
 	"launchpad.net/juju-core/worker"
 )
@@ -28,7 +29,7 @@
 type Provisioner interface {
 	worker.Worker
 	Stop() error
-	getWatcher() (Watcher, error)
+	getMachineWatcher() (apiwatcher.StringsWatcher, error)
 }
 
 // environProvisioner represents a running provisioning worker for machine nodes
@@ -103,7 +104,11 @@
 	}
 	// Start responding to changes in machines, and to any further updates
 	// to the environment config.
-	machineWatcher, err := p.getWatcher()
+	machineWatcher, err := p.getMachineWatcher()
+	if err != nil {
+		return nil, err
+	}
+	retryWatcher, err := p.st.WatchMachineErrorRetry()
 	if err != nil {
 		return nil, err
 	}
@@ -112,6 +117,7 @@
 		safeMode,
 		p.st,
 		machineWatcher,
+		retryWatcher,
 		p.broker,
 		auth)
 	return task, nil
@@ -183,7 +189,7 @@
 	}
 }
 
-func (p *environProvisioner) getWatcher() (Watcher, error) {
+func (p *environProvisioner) getMachineWatcher() (apiwatcher.StringsWatcher, error) {
 	return p.st.WatchEnvironMachines()
 }
 
@@ -250,7 +256,7 @@
 	return p.machine, nil
 }
 
-func (p *containerProvisioner) getWatcher() (Watcher, error) {
+func (p *containerProvisioner) getMachineWatcher() (apiwatcher.StringsWatcher, error) {
 	machine, err := p.getMachine()
 	if err != nil {
 		return nil, err

=== modified file 'worker/provisioner/provisioner_task.go'
--- worker/provisioner/provisioner_task.go	2014-03-26 00:24:30 +0000
+++ worker/provisioner/provisioner_task.go	2014-03-26 11:18:52 +0000
@@ -16,6 +16,7 @@
 	"launchpad.net/juju-core/names"
 	"launchpad.net/juju-core/state/api/params"
 	apiprovisioner "launchpad.net/juju-core/state/api/provisioner"
+	apiwatcher "launchpad.net/juju-core/state/api/watcher"
 	"launchpad.net/juju-core/state/watcher"
 	coretools "launchpad.net/juju-core/tools"
 	"launchpad.net/juju-core/utils"
@@ -35,28 +36,25 @@
 	SetSafeMode(safeMode bool)
 }
 
-type Watcher interface {
-	watcher.Errer
-	watcher.Stopper
-	Changes() <-chan []string
-}
-
 type MachineGetter interface {
 	Machine(tag string) (*apiprovisioner.Machine, error)
+	MachinesWithTransientErrors() ([]*apiprovisioner.Machine, []params.StatusResult, error)
 }
 
 func NewProvisionerTask(
 	machineTag string,
 	safeMode bool,
 	machineGetter MachineGetter,
-	watcher Watcher,
+	machineWatcher apiwatcher.StringsWatcher,
+	retryWatcher apiwatcher.NotifyWatcher,
 	broker environs.InstanceBroker,
 	auth environs.AuthenticationProvider,
 ) ProvisionerTask {
 	task := &provisionerTask{
 		machineTag:     machineTag,
 		machineGetter:  machineGetter,
-		machineWatcher: watcher,
+		machineWatcher: machineWatcher,
+		retryWatcher:   retryWatcher,
 		broker:         broker,
 		auth:           auth,
 		safeMode:       safeMode,
@@ -73,7 +71,8 @@
 type provisionerTask struct {
 	machineTag     string
 	machineGetter  MachineGetter
-	machineWatcher Watcher
+	machineWatcher apiwatcher.StringsWatcher
+	retryWatcher   apiwatcher.NotifyWatcher
 	broker         environs.InstanceBroker
 	tomb           tomb.Tomb
 	auth           environs.AuthenticationProvider
@@ -132,8 +131,6 @@
 			if !ok {
 				return watcher.MustErr(task.machineWatcher)
 			}
-			// TODO(dfc; lp:1042717) fire process machines periodically to shut down unknown
-			// instances.
 			if err := task.processMachines(ids); err != nil {
 				return fmt.Errorf("failed to process updated machines: %v", err)
 			}
@@ -152,6 +149,10 @@
 					return fmt.Errorf("failed to process machines after safe mode disabled: %v", err)
 				}
 			}
+		case <-task.retryWatcher.Changes():
+			if err := task.processMachinesWithTransientErrors(); err != nil {
+				return fmt.Errorf("failed to process machines with transient errors: %v", err)
+			}
 		}
 	}
 }
@@ -164,6 +165,29 @@
 	}
 }
 
+func (task *provisionerTask) processMachinesWithTransientErrors() error {
+	machines, statusResults, err := task.machineGetter.MachinesWithTransientErrors()
+	if err != nil {
+		return nil
+	}
+	logger.Tracef("processMachinesWithTransientErrors(%v)", statusResults)
+	var pending []*apiprovisioner.Machine
+	for i, status := range statusResults {
+		if status.Error != nil {
+			logger.Errorf("cannot retry provisioning of machine %q: %v", status.Id, status.Error)
+			continue
+		}
+		machine := machines[i]
+		if err := machine.SetStatus(params.StatusPending, "", nil); err != nil {
+			logger.Errorf("cannot reset status of machine %q: %v", status.Id, err)
+			continue
+		}
+		task.machines[machine.Tag()] = machine
+		pending = append(pending, machine)
+	}
+	return task.startMachines(pending)
+}
+
 func (task *provisionerTask) processMachines(ids []string) error {
 	logger.Tracef("processMachines(%v)", ids)
 	// Populate the tasks maps of current instances and machines.

=== modified file 'worker/provisioner/provisioner_test.go'
--- worker/provisioner/provisioner_test.go	2014-03-17 22:42:51 +0000
+++ worker/provisioner/provisioner_test.go	2014-03-26 11:18:52 +0000
@@ -14,6 +14,8 @@
 	"launchpad.net/juju-core/constraints"
 	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/environs/config"
+	"launchpad.net/juju-core/environs/simplestreams"
+	"launchpad.net/juju-core/environs/tools"
 	"launchpad.net/juju-core/errors"
 	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/juju/testing"
@@ -23,6 +25,7 @@
 	"launchpad.net/juju-core/state/api"
 	"launchpad.net/juju-core/state/api/params"
 	apiprovisioner "launchpad.net/juju-core/state/api/provisioner"
+	apiserverprovisioner "launchpad.net/juju-core/state/apiserver/provisioner"
 	coretesting "launchpad.net/juju-core/testing"
 	"launchpad.net/juju-core/utils"
 	"launchpad.net/juju-core/utils/set"
@@ -731,17 +734,21 @@
 	s.waitRemoved(c, m3)
 }
 
-func (s *ProvisionerSuite) newProvisionerTask(c *gc.C, safeMode bool) provisioner.ProvisionerTask {
-	env := s.APIConn.Environ
-	watcher, err := s.provisioner.WatchEnvironMachines()
+func (s *ProvisionerSuite) newProvisionerTask(c *gc.C, safeMode bool,
+	broker environs.InstanceBroker) provisioner.ProvisionerTask {
+
+	machineWatcher, err := s.provisioner.WatchEnvironMachines()
+	c.Assert(err, gc.IsNil)
+	retryWatcher, err := s.provisioner.WatchMachineErrorRetry()
 	c.Assert(err, gc.IsNil)
 	auth, err := environs.NewAPIAuthenticator(s.provisioner)
 	c.Assert(err, gc.IsNil)
-	return provisioner.NewProvisionerTask("machine-0", safeMode, s.provisioner, watcher, env, auth)
+	return provisioner.NewProvisionerTask(
+		"machine-0", safeMode, s.provisioner, machineWatcher, retryWatcher, broker, auth)
 }
 
 func (s *ProvisionerSuite) TestTurningOffSafeModeReapsUnknownInstances(c *gc.C) {
-	task := s.newProvisionerTask(c, true)
+	task := s.newProvisionerTask(c, true, s.APIConn.Environ)
 	defer stop(c, task)
 
 	// Initially create a machine, and an unknown instance, with safe mode on.
@@ -761,3 +768,66 @@
 	task.SetSafeMode(false)
 	s.checkStopInstances(c, i1)
 }
+
+func (s *ProvisionerSuite) TestProvisionerRetriesTransientErrors(c *gc.C) {
+	s.PatchValue(&apiserverprovisioner.ErrorRetryWaitDelay, 5*time.Millisecond)
+	var e environs.Environ = &mockBroker{Environ: s.APIConn.Environ, retryCount: make(map[string]int)}
+	task := s.newProvisionerTask(c, false, e)
+	defer stop(c, task)
+
+	// Provision some machines, some will be started first time,
+	// another will require retries.
+	m1, err := s.addMachine()
+	c.Assert(err, gc.IsNil)
+	m2, err := s.addMachine()
+	c.Assert(err, gc.IsNil)
+	m3, err := s.addMachine()
+	c.Assert(err, gc.IsNil)
+	m4, err := s.addMachine()
+	c.Assert(err, gc.IsNil)
+	s.checkStartInstance(c, m1)
+	s.checkStartInstance(c, m2)
+	thatsAllFolks := make(chan struct{})
+	go func() {
+		for {
+			select {
+			case <-thatsAllFolks:
+				return
+			case <-time.After(coretesting.ShortWait):
+				err := m3.SetStatus(params.StatusError, "info", params.StatusData{"transient": true})
+				c.Assert(err, gc.IsNil)
+			}
+		}
+	}()
+	s.checkStartInstance(c, m3)
+	close(thatsAllFolks)
+	// Machine 4 is never provisioned.
+	status, _, _, err := m4.Status()
+	c.Assert(err, gc.IsNil)
+	c.Assert(status, gc.Equals, params.StatusError)
+	_, err = m4.InstanceId()
+	c.Assert(err, jc.Satisfies, state.IsNotProvisionedError)
+}
+
+type mockBroker struct {
+	environs.Environ
+	retryCount map[string]int
+}
+
+func (b *mockBroker) StartInstance(args environs.StartInstanceParams) (instance.Instance, *instance.HardwareCharacteristics, error) {
+	// All machines except machines 3, 4 are provisioned successfully the first time.
+	// Machines 3 is provisioned after some attempts have been made.
+	// Machine 4 is never provisioned.
+	id := args.MachineConfig.MachineId
+	retries := b.retryCount[id]
+	if (id != "3" && id != "4") || retries > 2 {
+		return b.Environ.StartInstance(args)
+	} else {
+		b.retryCount[id] = retries + 1
+	}
+	return nil, nil, fmt.Errorf("error: some error")
+}
+
+func (b *mockBroker) GetToolsSources() ([]simplestreams.DataSource, error) {
+	return b.Environ.(tools.SupportsCustomSources).GetToolsSources()
+}