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
=== modified file 'cmd/juju/destroymachine_test.go'
--- cmd/juju/destroymachine_test.go 2013-11-27 12:14:31 +0000
+++ cmd/juju/destroymachine_test.go 2013-12-10 09:54:42 +0000
@@ -103,10 +103,12 @@
103 // Clean up, check state.103 // Clean up, check state.
104 err = s.State.Cleanup()104 err = s.State.Cleanup()
105 c.Assert(err, gc.IsNil)105 c.Assert(err, gc.IsNil)
106 err = m0.Refresh()
107 c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
108 err = u.Refresh()106 err = u.Refresh()
109 c.Assert(err, jc.Satisfies, errors.IsNotFoundError)107 c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
108 err = m0.Refresh()
109 c.Assert(err, gc.IsNil)
110 c.Assert(m0.Life(), gc.Equals, state.Dead)
111
110 err = m1.Refresh()112 err = m1.Refresh()
111 c.Assert(err, gc.IsNil)113 c.Assert(err, gc.IsNil)
112 c.Assert(m1.Life(), gc.Equals, state.Alive)114 c.Assert(m1.Life(), gc.Equals, state.Alive)
113115
=== modified file 'state/apiserver/client/client_test.go'
--- state/apiserver/client/client_test.go 2013-11-27 12:14:31 +0000
+++ state/apiserver/client/client_test.go 2013-12-10 09:54:42 +0000
@@ -415,9 +415,9 @@
415415
416 err = s.State.Cleanup()416 err = s.State.Cleanup()
417 c.Assert(err, gc.IsNil)417 c.Assert(err, gc.IsNil)
418 assertRemoved(c, m0)418 assertLife(c, m0, state.Dead)
419 assertLife(c, m1, state.Alive)419 assertLife(c, m1, state.Alive)
420 assertRemoved(c, m2)420 assertLife(c, m2, state.Dead)
421 assertRemoved(c, u)421 assertRemoved(c, u)
422}422}
423423
424424
=== modified file 'state/cleanup.go'
--- state/cleanup.go 2013-11-13 08:59:24 +0000
+++ state/cleanup.go 2013-12-10 09:54:42 +0000
@@ -151,14 +151,15 @@
151 // again -- which it *probably* will anyway -- the issue can be resolved by151 // again -- which it *probably* will anyway -- the issue can be resolved by
152 // force-destroying the machine again; that's better than adding layer152 // force-destroying the machine again; that's better than adding layer
153 // upon layer of complication here.153 // upon layer of complication here.
154 if err := machine.EnsureDead(); err != nil {154 return machine.EnsureDead()
155 return err155
156 }156 // Note that we do *not* remove the machine entirely: we leave it for the
157 return machine.Remove()157 // provisioner to clean up, so that we don't end up with an unreferenced
158 // instance that would otherwise be ignored when in provisioner-safe-mode.
158}159}
159160
160// cleanupContainers recursively calls cleanupMachine on the supplied161// cleanupContainers recursively calls cleanupMachine on the supplied
161// machine's containers.162// machine's containers, and removes them from state entirely.
162func (st *State) cleanupContainers(machine *Machine) error {163func (st *State) cleanupContainers(machine *Machine) error {
163 containerIds, err := machine.Containers()164 containerIds, err := machine.Containers()
164 if errors.IsNotFoundError(err) {165 if errors.IsNotFoundError(err) {
@@ -170,6 +171,15 @@
170 if err := st.cleanupMachine(containerId); err != nil {171 if err := st.cleanupMachine(containerId); err != nil {
171 return err172 return err
172 }173 }
174 container, err := st.Machine(containerId)
175 if errors.IsNotFoundError(err) {
176 return nil
177 } else if err != nil {
178 return err
179 }
180 if err := container.Remove(); err != nil {
181 return err
182 }
173 }183 }
174 return nil184 return nil
175}185}
176186
=== 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:54:42 +0000
@@ -128,17 +128,17 @@
128 c.Assert(err, gc.IsNil)128 c.Assert(err, gc.IsNil)
129 s.assertNeedsCleanup(c)129 s.assertNeedsCleanup(c)
130130
131 // Clean up, and check that the machine has been removed...131 // Clean up, and check that the unit has been removed...
132 s.assertCleanupRuns(c)132 s.assertCleanupRuns(c)
133 s.assertDoesNotNeedCleanup(c)133 s.assertDoesNotNeedCleanup(c)
134 err = machine.Refresh()
135 c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
136
137 // ...and so has the unit...
138 assertRemoved(c, pr.u0)134 assertRemoved(c, pr.u0)
139135
140 // ...and the unit has departed relation scope.136 // ...and the unit has departed relation scope...
141 assertNotInScope(c, pr.ru0)137 assertNotInScope(c, pr.ru0)
138
139 // ...but that the machine remains, and is Dead, ready for removal by the
140 // provisioner.
141 assertLife(c, machine, state.Dead)
142}142}
143143
144func (s *CleanupSuite) TestCleanupForceDestroyedMachineWithContainer(c *gc.C) {144func (s *CleanupSuite) TestCleanupForceDestroyedMachineWithContainer(c *gc.C) {
@@ -184,11 +184,9 @@
184 c.Assert(err, gc.IsNil)184 c.Assert(err, gc.IsNil)
185 s.assertNeedsCleanup(c)185 s.assertNeedsCleanup(c)
186186
187 // Clean up, and check that all the machines have been removed...187 // Clean up, and check that the container has been removed...
188 s.assertCleanupRuns(c)188 s.assertCleanupRuns(c)
189 s.assertDoesNotNeedCleanup(c)189 s.assertDoesNotNeedCleanup(c)
190 err = machine.Refresh()
191 c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
192 err = container.Refresh()190 err = container.Refresh()
193 c.Assert(err, jc.Satisfies, errors.IsNotFoundError)191 c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
194192
@@ -198,11 +196,15 @@
198 assertRemoved(c, prr.ru0)196 assertRemoved(c, prr.ru0)
199 assertRemoved(c, prr.ru1)197 assertRemoved(c, prr.ru1)
200198
201 // ...and none of the units have left relation scopes occupied.199 // ...and none of the units have left relation scopes occupied...
202 assertNotInScope(c, prr.pru0)200 assertNotInScope(c, prr.pru0)
203 assertNotInScope(c, prr.pru1)201 assertNotInScope(c, prr.pru1)
204 assertNotInScope(c, prr.rru0)202 assertNotInScope(c, prr.rru0)
205 assertNotInScope(c, prr.rru1)203 assertNotInScope(c, prr.rru1)
204
205 // ...but that the machine remains, and is Dead, ready for removal by the
206 // provisioner.
207 assertLife(c, machine, state.Dead)
206}208}
207209
208func (s *CleanupSuite) TestNothingToCleanup(c *gc.C) {210func (s *CleanupSuite) TestNothingToCleanup(c *gc.C) {

Subscribers

People subscribed via source and target branches

to all changes: