Code review comment for lp:~jameinel/juju-core/1.18-provisioner-no-tools-is-fatal-1311676

Reviewers: mp+217011_code.launchpad.net,

Message:
Please take a look.

Description:
worker/provisioner: no tools is an error

We were treating no tools found as an error of the Provisioner, rather
than an error with the machine we were trying to provision (bug
#1311676). This now sets the status of the machine to an error state,
which lets us continue with our lives.

In the original bug about not having tools, it was shown that in the
local provider, doing

   juju bootstrap -e local
   juju deploy -e local precise/ubuntu
   juju deploy -e local trusty/ubuntu ubuntu-t

Would end up with both machines stuck in Pending. Now it says:
$ juju status -e local
environment: local
machines:
   "0":
     agent-state: started
     agent-version: 1.18.2.1
     dns-name: localhost
     instance-id: localhost
     series: trusty
   "1":
     agent-state-info: '(error: no matching tools available)'
     instance-id: pending
     series: precise
   "2":
     agent-state: started
     agent-version: 1.18.2.1
     dns-name: 10.0.3.194
     instance-id: jameinel-local-machine-2
     series: trusty
     hardware: arch=amd64

And you can see that it succeeded in deploying trusty because it put
precise into an error state.

https://code.launchpad.net/~jameinel/juju-core/1.18-provisioner-no-tools-is-fatal-1311676/+merge/217011

(do not edit description out of merge proposal)

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

Affected files (+44, -1 lines):
   A [revision details]
   M worker/provisioner/provisioner_task.go
   M worker/provisioner/provisioner_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-20140412095413-8rps4bhva01cu0o9
+New revision: <email address hidden>

Index: worker/provisioner/provisioner_task.go
=== modified file 'worker/provisioner/provisioner_task.go'
--- worker/provisioner/provisioner_task.go 2014-04-02 08:44:20 +0000
+++ worker/provisioner/provisioner_task.go 2014-04-24 09:10:44 +0000
@@ -419,7 +419,13 @@
   }
   possibleTools, err := task.possibleTools(series, cons)
   if err != nil {
- return err
+ logger.Errorf("cannot find tools for machine %q: %v", machine, err)
+ if err1 := machine.SetStatus(params.StatusError, err.Error(), nil);
err1 != nil {
+ // Something is wrong with this machine, better report it back.
+ logger.Errorf("cannot set error status for machine %q: %v", machine,
err1)
+ return err1
+ }
+ return nil
   }
   machineConfig, err := task.machineConfig(machine)
   if err != nil {

Index: worker/provisioner/provisioner_test.go
=== modified file 'worker/provisioner/provisioner_test.go'
--- worker/provisioner/provisioner_test.go 2014-04-03 19:11:11 +0000
+++ worker/provisioner/provisioner_test.go 2014-04-24 09:10:44 +0000
@@ -390,6 +390,41 @@
   s.checkStartInstanceCustom(c, m, "pork", cons)
  }

+func (s *ProvisionerSuite)
TestProvisionerSetsErrorStatusWhenNoToolsAreAvailable(c *gc.C) {
+ p := s.newEnvironProvisioner(c)
+ defer stop(c, p)
+
+ // Check that an instance is not provisioned when the machine is
created...
+ m, err := s.BackingState.AddOneMachine(state.MachineTemplate{
+ // We need a valid series that has no tools uploaded
+ Series: "raring",
+ Jobs: []state.MachineJob{state.JobHostUnits},
+ Constraints: s.defaultConstraints,
+ })
+ c.Assert(err, gc.IsNil)
+ s.checkNoOperations(c)
+
+ t0 := time.Now()
+ for time.Since(t0) < coretesting.LongWait {
+ // And check the machine status is set to error.
+ status, info, _, err := m.Status()
+ c.Assert(err, gc.IsNil)
+ if status == params.StatusPending {
+ time.Sleep(coretesting.ShortWait)
+ continue
+ }
+ c.Assert(status, gc.Equals, params.StatusError)
+ c.Assert(info, gc.Equals, "no matching tools available")
+ break
+ }
+
+ // Restart the PA to make sure the machine is skipped again.
+ stop(c, p)
+ p = s.newEnvironProvisioner(c)
+ defer stop(c, p)
+ s.checkNoOperations(c)
+}
+
  func (s *ProvisionerSuite)
TestProvisionerSetsErrorStatusWhenStartInstanceFailed(c *gc.C) {
   brokenMsg := breakDummyProvider(c, s.State, "StartInstance")
   p := s.newEnvironProvisioner(c)