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)
  }