Code review comment for lp:~fwereade/juju-core/force-dead-machine-for-1.16

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.assertNeedsCleanup(c)

- // Clean up, and check that all the machines have been removed...
+ // Clean up, and check that the container has been removed...
   s.assertCleanupRuns(c)
   s.assertDoesNotNeedCleanup(c)
- err = machine.Refresh()
- c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
   err = container.Refresh()
   c.Assert(err, jc.Satisfies, errors.IsNotFoundError)

@@ -198,11 +196,15 @@
   assertRemoved(c, prr.ru0)
   assertRemoved(c, prr.ru1)

- // ...and none of the units have left relation scopes occupied.
+ // ...and none of the units have left relation scopes occupied...
   assertNotInScope(c, prr.pru0)
   assertNotInScope(c, prr.pru1)
   assertNotInScope(c, prr.rru0)
   assertNotInScope(c, prr.rru1)
+
+ // ...but that the machine remains, and is Dead, ready for removal by the
+ // provisioner.
+ assertLife(c, machine, state.Dead)
  }

  func (s *CleanupSuite) TestNothingToCleanup(c *gc.C) {

Index: cmd/juju/destroymachine_test.go
=== 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:52:04 +0000
@@ -103,10 +103,12 @@
   // Clean up, check state.
   err = s.State.Cleanup()
   c.Assert(err, gc.IsNil)
- err = m0.Refresh()
- c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
   err = u.Refresh()
   c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
+ err = m0.Refresh()
+ c.Assert(err, gc.IsNil)
+ c.Assert(m0.Life(), gc.Equals, state.Dead)
+
   err = m1.Refresh()
   c.Assert(err, gc.IsNil)
   c.Assert(m1.Life(), gc.Equals, state.Alive)

Index: state/apiserver/client/client_test.go
=== 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:52:04 +0000
@@ -415,9 +415,9 @@

   err = s.State.Cleanup()
   c.Assert(err, gc.IsNil)
- assertRemoved(c, m0)
+ assertLife(c, m0, state.Dead)
   assertLife(c, m1, state.Alive)
- assertRemoved(c, m2)
+ assertLife(c, m2, state.Dead)
   assertRemoved(c, u)
  }