Code review comment for lp://staging/~fwereade/juju-core/state-dying-service-fix

Reviewers: mp+158970_code.launchpad.net,

Message:
Please take a look.

Description:
state: fix service unit removal

Removing the last unit from a dying service that was in a relation would
fail, because we were constructing assertions that failed to take
relation
count into consideration.

https://code.launchpad.net/~fwereade/juju-core/state-dying-service-fix/+merge/158970

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M state/service.go
   M state/unit_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:
<email address hidden>
+New revision: <email address hidden>

Index: state/service.go
=== modified file 'state/service.go'
--- state/service.go 2013-04-14 22:12:49 +0000
+++ state/service.go 2013-04-15 15:59:52 +0000
@@ -643,7 +643,13 @@
   if s.doc.Life == Alive {
    svcOp.Assert = D{{"life", Alive}, {"unitcount", D{{"$gt", 0}}}}
   } else {
- svcOp.Assert = D{{"life", Dying}, {"unitcount", D{{"$gt", 1}}}}
+ svcOp.Assert = D{
+ {"life", Dying},
+ {"$or", []D{
+ {{"unitcount", D{{"$gt", 1}}}},
+ {{"relationcount", D{{"$gt", 0}}}},
+ }},
+ }
   }
   return append(ops, svcOp), nil
  }

Index: state/unit_test.go
=== modified file 'state/unit_test.go'
--- state/unit_test.go 2013-04-11 15:46:31 +0000
+++ state/unit_test.go 2013-04-15 15:59:52 +0000
@@ -714,6 +714,49 @@
   c.Assert(err, IsNil)
  }

+func (s *UnitSuite) TestRemovePathological(c *C) {
+ // Add a relation between wordpress and mysql...
+ wordpress := s.service
+ wordpress0 := s.unit
+ mysql, err := s.State.AddService("mysql", s.AddTestingCharm(c, "mysql"))
+ c.Assert(err, IsNil)
+ eps, err := s.State.InferEndpoints([]string{"wordpress", "mysql"})
+ c.Assert(err, IsNil)
+ rel, err := s.State.AddRelation(eps...)
+ c.Assert(err, IsNil)
+
+ // Enter scope with a unit of mysql, to keep the relation (and thus
+ // wordpress itself) from being removed.
+ mysql0, err := mysql.AddUnit()
+ c.Assert(err, IsNil)
+ mysql0ru, err := rel.Unit(mysql0)
+ c.Assert(err, IsNil)
+ err = mysql0ru.EnterScope(nil)
+ c.Assert(err, IsNil)
+
+ // Destroy wordpress, and remove its last unit.
+ err = wordpress.Destroy()
+ c.Assert(err, IsNil)
+ err = wordpress0.EnsureDead()
+ c.Assert(err, IsNil)
+ err = wordpress0.Remove()
+ c.Assert(err, IsNil)
+
+ // Check this didn't kill the service or relation yet...
+ err = wordpress.Refresh()
+ c.Assert(err, IsNil)
+ err = rel.Refresh()
+ c.Assert(err, IsNil)
+
+ // ...but finally leaving relation scope on the other side does.
+ err = mysql0ru.LeaveScope()
+ c.Assert(err, IsNil)
+ err = wordpress.Refresh()
+ c.Assert(state.IsNotFound(err), Equals, true)
+ err = rel.Refresh()
+ c.Assert(state.IsNotFound(err), Equals, true)
+}
+
  func (s *UnitSuite) TestWatchSubordinates(c *C) {
   w := s.unit.WatchSubordinateUnits()
   defer stop(c, w)