juju-core

Merge lp:~fwereade/juju-core/force-dead-machine-for-1.16 into lp:juju-core/1.16

  1. force-dead-machine-for-1.16
  2. Merge into 1.16
Proposed by William Reade
Status: Merged
Approved by: William Reade
Approved revision: no longer in the source branch.
Merged at revision: 1999
Proposed branch: lp:~fwereade/juju-core/force-dead-machine-for-1.16
Merge into: lp:juju-core/1.16
Diff against target: 133 lines (+33/-19)
4 files modified
cmd/juju/destroymachine_test.go (+4/-2)
state/apiserver/client/client_test.go (+2/-2)
state/cleanup.go (+15/-5)
state/cleanup_test.go (+12/-10)
To merge this branch: bzr merge lp:~fwereade/juju-core/force-dead-machine-for-1.16
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+198361@code.launchpad.net

Commit message

state: don't remove force-destroyed machines

fixes lp:1259473

https://codereview.appspot.com/37610044/

Description of the change

state: don't remove force-destroyed machines

fixes lp:1259473

https://codereview.appspot.com/37610044/

To post a comment you must log in.
Revision history for this message
William Reade (fwereade) wrote :
Download full text (5.1 KiB)

Reviewers: mp+198361_code.launchpad.net,

Message:
Please take a look.

Description:
state: don't remove force-destroyed machines

fixes lp:1259473

https://code.launchpad.net/~fwereade/juju-core/force-dead-machine-for-1.16/+merge/198361

(do not edit description out of merge proposal)

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

Affected files (+35, -19 lines):
   A [revision details]
   M cmd/juju/destroymachine_test.go
   M state/apiserver/client/client_test.go
   M state/cleanup.go
   M state/cleanup_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-20131209105211-4qnv0z07ivx7t9ig
+New revision: <email address hidden>

Index: state/cleanup.go
=== modified file 'state/cleanup.go'
--- state/cleanup.go 2013-11-13 08:59:24 +0000
+++ state/cleanup.go 2013-12-10 09:52:04 +0000
@@ -151,14 +151,15 @@
   // again -- which it *probably* will anyway -- the issue can be resolved
by
   // force-destroying the machine again; that's better than adding layer
   // upon layer of complication here.
- if err := machine.EnsureDead(); err != nil {
- return err
- }
- return machine.Remove()
+ return machine.EnsureDead()
+
+ // Note that we do *not* remove the machine entirely: we leave it for the
+ // provisioner to clean up, so that we don't end up with an unreferenced
+ // instance that would otherwise be ignored when in provisioner-safe-mode.
  }

  // cleanupContainers recursively calls cleanupMachine on the supplied
-// machine's containers.
+// machine's containers, and removes them from state entirely.
  func (st *State) cleanupContainers(machine *Machine) error {
   containerIds, err := machine.Containers()
   if errors.IsNotFoundError(err) {
@@ -170,6 +171,15 @@
    if err := st.cleanupMachine(containerId); err != nil {
     return err
    }
+ container, err := st.Machine(containerId)
+ if errors.IsNotFoundError(err) {
+ return nil
+ } else if err != nil {
+ return err
+ }
+ if err := container.Remove(); err != nil {
+ return err
+ }
   }
   return nil
  }

Index: state/cleanup_test.go
=== modified file 'state/cleanup_test.go'
--- state/cleanup_test.go 2013-11-13 09:07:28 +0000
+++ state/cleanup_test.go 2013-12-10 09:52:04 +0000
@@ -128,17 +128,17 @@
   c.Assert(err, gc.IsNil)
   s.assertNeedsCleanup(c)

- // Clean up, and check that the machine has been removed...
+ // Clean up, and check that the unit has been removed...
   s.assertCleanupRuns(c)
   s.assertDoesNotNeedCleanup(c)
- err = machine.Refresh()
- c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
-
- // ...and so has the unit...
   assertRemoved(c, pr.u0)

- // ...and the unit has departed relation scope.
+ // ...and the unit has departed relation scope...
   assertNotInScope(c, pr.ru0)
+
+ // ...but that the machine remains, and is Dead, ready for removal by the
+ // provisioner.
+ assertLife(c, machine, state.Dead)
  }

  func (s *CleanupSuite) TestCleanupForceDestroyedMachineWithContainer(c
*gc.C) {
@@ -184,11 +184,9 @@
   c.Assert(err, gc.IsNil)
   s.assertNeedsCl...

Read more...

Revision history for this message
Dimiter Naydenov (dimitern) wrote :

LGTM

https://codereview.appspot.com/37610044/

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

The attempt to merge lp:~fwereade/juju-core/force-dead-machine-for-1.16 into lp:juju-core/1.16 failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 0.350s
ok launchpad.net/juju-core/agent/tools 0.220s
ok launchpad.net/juju-core/bzr 6.857s
ok launchpad.net/juju-core/cert 3.164s
ok launchpad.net/juju-core/charm 0.548s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.026s
ok launchpad.net/juju-core/cmd 0.227s
? 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 178.581s
ok launchpad.net/juju-core/cmd/jujud 48.368s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 2.479s
? launchpad.net/juju-core/cmd/plugins/juju-update-bootstrap [no test files]
ok launchpad.net/juju-core/constraints 0.028s
ok launchpad.net/juju-core/container/lxc 0.293s
? launchpad.net/juju-core/container/lxc/mock [no test files]
ok launchpad.net/juju-core/downloader 5.314s
ok launchpad.net/juju-core/environs 3.235s
ok launchpad.net/juju-core/environs/bootstrap 5.093s
ok launchpad.net/juju-core/environs/cloudinit 0.529s
ok launchpad.net/juju-core/environs/config 0.836s
ok launchpad.net/juju-core/environs/configstore 0.039s
ok launchpad.net/juju-core/environs/filestorage 0.031s
ok launchpad.net/juju-core/environs/httpstorage 0.906s
ok launchpad.net/juju-core/environs/imagemetadata 0.548s
ok launchpad.net/juju-core/environs/instances 0.052s
ok launchpad.net/juju-core/environs/jujutest 0.259s
ok launchpad.net/juju-core/environs/manual 4.315s
ok launchpad.net/juju-core/environs/simplestreams 0.341s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 1.231s
ok launchpad.net/juju-core/environs/storage 1.283s
ok launchpad.net/juju-core/environs/sync 28.639s
ok launchpad.net/juju-core/environs/testing 0.219s
ok launchpad.net/juju-core/environs/tools 7.042s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.015s
ok launchpad.net/juju-core/instance 0.031s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 18.050s
ok launchpad.net/juju-core/juju/osenv 0.018s
? launchpad.net/juju-core/juju/testing [no test files]
ok launchpad.net/juju-core/log 0.015s
ok launchpad.net/juju-core/log/syslog 0.024s
ok launchpad.net/juju-core/names 0.026s
? launchpad.net/juju-core/provider [no test files]
? launchpad.net/juju-core/provider/all [no test files]
ok launchpad.net/juju-core/provider/azure 6.433s
ok launchpad.net/juju-core/provider/common 0.306s
ok launchpad.net/juju-core/provider/dummy 21.348s
ok launchpad.net/juju-core/provider/ec2 5.505s
ok launchpad.net/juju-core/provider/ec2/httpstorage 0.193s
ok launchpad.net/juju-core/provider/local 2.281s
ok launchpad.net/juju-core/provider/maas 11.435s
ok launchpad.net/juju-core/provider/null 1.203s
ok launchpad.net/juju-core/provider/openstack 14.001s
ok launchpad.net/juju-core/rpc 0.096s
ok launchpad.net/juj...

Read more...

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

Reapproving -- test timeout appears to be unrelated.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'cmd/juju/destroymachine_test.go'
2--- cmd/juju/destroymachine_test.go 2013-11-27 12:14:31 +0000
3+++ cmd/juju/destroymachine_test.go 2013-12-10 09:54:42 +0000
4@@ -103,10 +103,12 @@
5 // Clean up, check state.
6 err = s.State.Cleanup()
7 c.Assert(err, gc.IsNil)
8- err = m0.Refresh()
9- c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
10 err = u.Refresh()
11 c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
12+ err = m0.Refresh()
13+ c.Assert(err, gc.IsNil)
14+ c.Assert(m0.Life(), gc.Equals, state.Dead)
15+
16 err = m1.Refresh()
17 c.Assert(err, gc.IsNil)
18 c.Assert(m1.Life(), gc.Equals, state.Alive)
19
20=== modified file 'state/apiserver/client/client_test.go'
21--- state/apiserver/client/client_test.go 2013-11-27 12:14:31 +0000
22+++ state/apiserver/client/client_test.go 2013-12-10 09:54:42 +0000
23@@ -415,9 +415,9 @@
24
25 err = s.State.Cleanup()
26 c.Assert(err, gc.IsNil)
27- assertRemoved(c, m0)
28+ assertLife(c, m0, state.Dead)
29 assertLife(c, m1, state.Alive)
30- assertRemoved(c, m2)
31+ assertLife(c, m2, state.Dead)
32 assertRemoved(c, u)
33 }
34
35
36=== modified file 'state/cleanup.go'
37--- state/cleanup.go 2013-11-13 08:59:24 +0000
38+++ state/cleanup.go 2013-12-10 09:54:42 +0000
39@@ -151,14 +151,15 @@
40 // again -- which it *probably* will anyway -- the issue can be resolved by
41 // force-destroying the machine again; that's better than adding layer
42 // upon layer of complication here.
43- if err := machine.EnsureDead(); err != nil {
44- return err
45- }
46- return machine.Remove()
47+ return machine.EnsureDead()
48+
49+ // Note that we do *not* remove the machine entirely: we leave it for the
50+ // provisioner to clean up, so that we don't end up with an unreferenced
51+ // instance that would otherwise be ignored when in provisioner-safe-mode.
52 }
53
54 // cleanupContainers recursively calls cleanupMachine on the supplied
55-// machine's containers.
56+// machine's containers, and removes them from state entirely.
57 func (st *State) cleanupContainers(machine *Machine) error {
58 containerIds, err := machine.Containers()
59 if errors.IsNotFoundError(err) {
60@@ -170,6 +171,15 @@
61 if err := st.cleanupMachine(containerId); err != nil {
62 return err
63 }
64+ container, err := st.Machine(containerId)
65+ if errors.IsNotFoundError(err) {
66+ return nil
67+ } else if err != nil {
68+ return err
69+ }
70+ if err := container.Remove(); err != nil {
71+ return err
72+ }
73 }
74 return nil
75 }
76
77=== modified file 'state/cleanup_test.go'
78--- state/cleanup_test.go 2013-11-13 09:07:28 +0000
79+++ state/cleanup_test.go 2013-12-10 09:54:42 +0000
80@@ -128,17 +128,17 @@
81 c.Assert(err, gc.IsNil)
82 s.assertNeedsCleanup(c)
83
84- // Clean up, and check that the machine has been removed...
85+ // Clean up, and check that the unit has been removed...
86 s.assertCleanupRuns(c)
87 s.assertDoesNotNeedCleanup(c)
88- err = machine.Refresh()
89- c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
90-
91- // ...and so has the unit...
92 assertRemoved(c, pr.u0)
93
94- // ...and the unit has departed relation scope.
95+ // ...and the unit has departed relation scope...
96 assertNotInScope(c, pr.ru0)
97+
98+ // ...but that the machine remains, and is Dead, ready for removal by the
99+ // provisioner.
100+ assertLife(c, machine, state.Dead)
101 }
102
103 func (s *CleanupSuite) TestCleanupForceDestroyedMachineWithContainer(c *gc.C) {
104@@ -184,11 +184,9 @@
105 c.Assert(err, gc.IsNil)
106 s.assertNeedsCleanup(c)
107
108- // Clean up, and check that all the machines have been removed...
109+ // Clean up, and check that the container has been removed...
110 s.assertCleanupRuns(c)
111 s.assertDoesNotNeedCleanup(c)
112- err = machine.Refresh()
113- c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
114 err = container.Refresh()
115 c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
116
117@@ -198,11 +196,15 @@
118 assertRemoved(c, prr.ru0)
119 assertRemoved(c, prr.ru1)
120
121- // ...and none of the units have left relation scopes occupied.
122+ // ...and none of the units have left relation scopes occupied...
123 assertNotInScope(c, prr.pru0)
124 assertNotInScope(c, prr.pru1)
125 assertNotInScope(c, prr.rru0)
126 assertNotInScope(c, prr.rru1)
127+
128+ // ...but that the machine remains, and is Dead, ready for removal by the
129+ // provisioner.
130+ assertLife(c, machine, state.Dead)
131 }
132
133 func (s *CleanupSuite) TestNothingToCleanup(c *gc.C) {

Subscribers

People subscribed via source and target branches

to all changes: