Merge lp:~jameinel/juju-core/1.18-provisioner-no-tools-is-fatal-1311676 into lp:juju-core/1.18

Proposed by John A Meinel
Status: Merged
Approved by: John A Meinel
Approved revision: no longer in the source branch.
Merged at revision: 2274
Proposed branch: lp:~jameinel/juju-core/1.18-provisioner-no-tools-is-fatal-1311676
Merge into: lp:juju-core/1.18
Diff against target: 74 lines (+43/-2)
3 files modified
testing/constants.go (+1/-1)
worker/provisioner/provisioner_task.go (+7/-1)
worker/provisioner/provisioner_test.go (+35/-0)
To merge this branch: bzr merge lp:~jameinel/juju-core/1.18-provisioner-no-tools-is-fatal-1311676
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+217011@code.launchpad.net

Commit message

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://codereview.appspot.com/93720044/

Description of the change

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://codereview.appspot.com/93720044/

To post a comment you must log in.
Revision history for this message
John A Meinel (jameinel) wrote :
Download full text (4.2 KiB)

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 mach...

Read more...

Revision history for this message
Nate Finch (natefinch) wrote :
Revision history for this message
William Reade (fwereade) wrote :

LGTM if either use the existing method or have some good reason why it's
unsuitable.

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

https://codereview.appspot.com/93720044/diff/1/worker/provisioner/provisioner_task.go#newcode423
worker/provisioner/provisioner_task.go:423: if err1 :=
machine.SetStatus(params.StatusError, err.Error(), nil); err1 != nil {
isn't there a setErrorStatus method already?

https://codereview.appspot.com/93720044/

Revision history for this message
John A Meinel (jameinel) wrote :

>> isn't there a setErrorStatus method already?
Not on 1.18, but there is one in Trunk which I want to use.

I'll double check if I can use it, but the existing code wasn't, so I thought it didn't exist.

Revision history for this message
Go Bot (go-bot) wrote :
Download full text (163.0 KiB)

The attempt to merge lp:~jameinel/juju-core/1.18-provisioner-no-tools-is-fatal-1311676 into lp:juju-core/1.18 failed. Below is the output from the failed tests.

ok launchpad.net/juju-core 0.013s
ok launchpad.net/juju-core/agent 1.054s
ok launchpad.net/juju-core/agent/mongo 0.547s
ok launchpad.net/juju-core/agent/tools 0.190s
ok launchpad.net/juju-core/bzr 5.118s
ok launchpad.net/juju-core/cert 2.349s
ok launchpad.net/juju-core/charm 0.365s
? 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.038s
ok launchpad.net/juju-core/cloudinit/sshinit 0.926s
ok launchpad.net/juju-core/cmd 0.197s
ok launchpad.net/juju-core/cmd/charm-admin 0.763s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]

----------------------------------------------------------------------
FAIL: bootstrap_test.go:525: BootstrapSuite.TestAutoUploadAfterFailedSync

[LOG] 82.34938 DEBUG juju.environs.tools no architecture specified when finding tools, looking for any
[LOG] 82.34943 DEBUG juju.environs.tools no series specified when finding tools, looking for any
[LOG] 82.34955 DEBUG juju.environs.simplestreams fetchData failed for "file:///tmp/jc18test.I4E/gocheck-2775422040480279449/13/tools/streams/v1/index.sjson": stat /tmp/jc18test.I4E/gocheck-2775422040480279449/13/tools/streams/v1/index.sjson: no such file or directory
[LOG] 82.34963 DEBUG juju.environs.simplestreams cannot load index "file:///tmp/jc18test.I4E/gocheck-2775422040480279449/13/tools/streams/v1/index.sjson": invalid URL "file:///tmp/jc18test.I4E/gocheck-2775422040480279449/13/tools/streams/v1/index.sjson" not found
[LOG] 82.34967 DEBUG juju.environs.simplestreams fetchData failed for "file:///tmp/jc18test.I4E/gocheck-2775422040480279449/13/tools/streams/v1/index.json": stat /tmp/jc18test.I4E/gocheck-2775422040480279449/13/tools/streams/v1/index.json: no such file or directory
[LOG] 82.34976 DEBUG juju.environs.simplestreams cannot load index "file:///tmp/jc18test.I4E/gocheck-2775422040480279449/13/tools/streams/v1/index.json": invalid URL "file:///tmp/jc18test.I4E/gocheck-2775422040480279449/13/tools/streams/v1/index.json" not found
[LOG] 82.35256 INFO juju.environs.tools Writing tools/streams/v1/index.json
[LOG] 82.35304 INFO juju.environs.tools Writing tools/streams/v1/com.ubuntu.juju:released:tools.json
[LOG] 82.35427 DEBUG juju.environs.tools no architecture specified when finding tools, looking for any
[LOG] 82.35428 DEBUG juju.environs.tools no series specified when finding tools, looking for any
[LOG] 82.35435 DEBUG juju.environs.simplestreams fetchData failed for "file:///tmp/jc18test.I4E/gocheck-2775422040480279449/14/tools/streams/v1/index.sjson": stat /tmp/jc18test.I4E/gocheck-2775422040480279449/14/tools/streams/v1/index.sjson: no such file or directory
[LOG] 82.35439 DEBUG juju.environs.simplestreams cannot load index "file:///tmp/jc18test.I4E/gocheck-2775422040480279449/14/tools/streams/v1/index.sjson": invalid URL "file:///tmp/jc18test.I4E/gocheck-2775422040480279449/14/tools/streams/v1/index.sjson" not found
[L...

Revision history for this message
Go Bot (go-bot) wrote :
Download full text (10.4 KiB)

The attempt to merge lp:~jameinel/juju-core/1.18-provisioner-no-tools-is-fatal-1311676 into lp:juju-core/1.18 failed. Below is the output from the failed tests.

ok launchpad.net/juju-core 0.013s
ok launchpad.net/juju-core/agent 1.088s
ok launchpad.net/juju-core/agent/mongo 0.519s
ok launchpad.net/juju-core/agent/tools 0.210s
ok launchpad.net/juju-core/bzr 4.951s
ok launchpad.net/juju-core/cert 2.667s
ok launchpad.net/juju-core/charm 0.410s
? 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.029s
ok launchpad.net/juju-core/cloudinit/sshinit 0.815s
ok launchpad.net/juju-core/cmd 0.185s
ok launchpad.net/juju-core/cmd/charm-admin 0.702s
? 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 210.956s
ok launchpad.net/juju-core/cmd/jujud 64.010s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 8.183s
? launchpad.net/juju-core/cmd/plugins/juju-restore [no test files]
ok launchpad.net/juju-core/cmd/plugins/local 0.161s
? launchpad.net/juju-core/cmd/plugins/local/juju-local [no test files]
ok launchpad.net/juju-core/constraints 0.021s
ok launchpad.net/juju-core/container 0.042s
ok launchpad.net/juju-core/container/factory 0.039s
ok launchpad.net/juju-core/container/kvm 0.210s
ok launchpad.net/juju-core/container/kvm/mock 0.049s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 4.308s
? 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.293s
ok launchpad.net/juju-core/environs 2.335s
ok launchpad.net/juju-core/environs/bootstrap 10.345s
ok launchpad.net/juju-core/environs/cloudinit 0.442s
ok launchpad.net/juju-core/environs/config 3.050s
ok launchpad.net/juju-core/environs/configstore 0.028s
ok launchpad.net/juju-core/environs/filestorage 0.026s
ok launchpad.net/juju-core/environs/httpstorage 0.842s
ok launchpad.net/juju-core/environs/imagemetadata 0.500s
? 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.170s
ok launchpad.net/juju-core/environs/manual 11.797s

----------------------------------------------------------------------
FAIL: simplestreams_test.go:417: simplestreamsSuite.TestSupportedSeries

simplestreams_test.go:422:
    c.Assert(series, gc.DeepEquals, coretesting.SupportedSeries)
... obtained []string = []string{"precise", "quantal", "raring", "saucy", "trusty", "utopic"}
... expected []string = []string{"precise", "quantal", "raring", "saucy", "trusty"}

OOPS: 45 passed, 1 FAILED
--- FAIL: Test (0.07 seconds)
FAIL
FAIL launchpad.net/juju-core/environs/simplestreams 0.305s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 0.906s
ok laun...

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'testing/constants.go'
2--- testing/constants.go 2014-02-11 03:40:13 +0000
3+++ testing/constants.go 2014-04-24 13:57:10 +0000
4@@ -27,4 +27,4 @@
5 }
6
7 // SupportedSeries lists the series known to Juju.
8-var SupportedSeries = []string{"precise", "quantal", "raring", "saucy", "trusty"}
9+var SupportedSeries = []string{"precise", "quantal", "raring", "saucy", "trusty", "utopic"}
10
11=== modified file 'worker/provisioner/provisioner_task.go'
12--- worker/provisioner/provisioner_task.go 2014-04-02 08:44:20 +0000
13+++ worker/provisioner/provisioner_task.go 2014-04-24 13:57:10 +0000
14@@ -419,7 +419,13 @@
15 }
16 possibleTools, err := task.possibleTools(series, cons)
17 if err != nil {
18- return err
19+ logger.Errorf("cannot find tools for machine %q: %v", machine, err)
20+ if err1 := machine.SetStatus(params.StatusError, err.Error(), nil); err1 != nil {
21+ // Something is wrong with this machine, better report it back.
22+ logger.Errorf("cannot set error status for machine %q: %v", machine, err1)
23+ return err1
24+ }
25+ return nil
26 }
27 machineConfig, err := task.machineConfig(machine)
28 if err != nil {
29
30=== modified file 'worker/provisioner/provisioner_test.go'
31--- worker/provisioner/provisioner_test.go 2014-04-03 19:11:11 +0000
32+++ worker/provisioner/provisioner_test.go 2014-04-24 13:57:10 +0000
33@@ -390,6 +390,41 @@
34 s.checkStartInstanceCustom(c, m, "pork", cons)
35 }
36
37+func (s *ProvisionerSuite) TestProvisionerSetsErrorStatusWhenNoToolsAreAvailable(c *gc.C) {
38+ p := s.newEnvironProvisioner(c)
39+ defer stop(c, p)
40+
41+ // Check that an instance is not provisioned when the machine is created...
42+ m, err := s.BackingState.AddOneMachine(state.MachineTemplate{
43+ // We need a valid series that has no tools uploaded
44+ Series: "raring",
45+ Jobs: []state.MachineJob{state.JobHostUnits},
46+ Constraints: s.defaultConstraints,
47+ })
48+ c.Assert(err, gc.IsNil)
49+ s.checkNoOperations(c)
50+
51+ t0 := time.Now()
52+ for time.Since(t0) < coretesting.LongWait {
53+ // And check the machine status is set to error.
54+ status, info, _, err := m.Status()
55+ c.Assert(err, gc.IsNil)
56+ if status == params.StatusPending {
57+ time.Sleep(coretesting.ShortWait)
58+ continue
59+ }
60+ c.Assert(status, gc.Equals, params.StatusError)
61+ c.Assert(info, gc.Equals, "no matching tools available")
62+ break
63+ }
64+
65+ // Restart the PA to make sure the machine is skipped again.
66+ stop(c, p)
67+ p = s.newEnvironProvisioner(c)
68+ defer stop(c, p)
69+ s.checkNoOperations(c)
70+}
71+
72 func (s *ProvisionerSuite) TestProvisionerSetsErrorStatusWhenStartInstanceFailed(c *gc.C) {
73 brokenMsg := breakDummyProvider(c, s.State, "StartInstance")
74 p := s.newEnvironProvisioner(c)

Subscribers

People subscribed via source and target branches

to all changes: