Merge lp://staging/~fwereade/juju-core/state-dying-service-fix into lp://staging/~juju/juju-core/trunk

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)

LGTM

https://codereview.appspot.com/8748048/diff/1/state/service.go
File state/service.go (right):

https://codereview.appspot.com/8748048/diff/1/state/service.go#newcode647
state/service.go:647: {"life", Dying},
so this is: Dying, OR (unitcount>1 AND relationcount > 0), right?

https://codereview.appspot.com/8748048/

I think you need to add removeOps to this case as well:
...
rvba> fwereade: doesn't look like your fix worked:
http://paste.ubuntu.com/5710819/
dimitern> rvba: that wasn't the case before, right? the both the unit
and the service were in the status output
rvba> dimitern: indeed (that was before the fix:
http://paste.ubuntu.com/5710599/)
dimitern> fwereade , rvba: I think the fix should include removeOps in
the amended case, like we're doing a bit further up in the "if
s.doc.Life == Dying && s.doc.RelationCount == 0 && s.doc.UnitCount == 1"
case

https://codereview.appspot.com/8748048/

On 2013/04/15 16:44:53, dimitern wrote:
> I think you need to add removeOps to this case as well:
> ...
> rvba> fwereade: doesn't look like your fix worked:
> http://paste.ubuntu.com/5710819/
> dimitern> rvba: that wasn't the case before, right? the both the unit
and the
> service were in the status output
> rvba> dimitern: indeed (that was before the fix:
> http://paste.ubuntu.com/5710599/)
> dimitern> fwereade , rvba: I think the fix should include removeOps in
the
> amended case, like we're doing a bit further up in the "if s.doc.Life
== Dying
> && s.doc.RelationCount == 0 && s.doc.UnitCount == 1" case

Working as intended: mediawiki is in an error state, and is still
holding a ref to the relation, which is still holding a ref to the
service. `juju resolved mediawiki/0` will unblock it, and cause mysql to
be removed when mediawiki/0 departs the relation.

https://codereview.appspot.com/8748048/

https://codereview.appspot.com/8748048/diff/1/state/service.go
File state/service.go (right):

https://codereview.appspot.com/8748048/diff/1/state/service.go#newcode647
state/service.go:647: {"life", Dying},
On 2013/04/15 16:11:33, dimitern wrote:
> so this is: Dying, OR (unitcount>1 AND relationcount > 0), right?

This is Dying AND (unitcount>1 OR relationcount >0). To put it another
way: "there must be at least one other entity holding a reference to a
dying service at the point we remove a unit; if not, we should abort and
recalculate (and expect to hit the hasLastRef branch above)".

https://codereview.appspot.com/8748048/

LGTM - Generally hoping that one day we have tests that are more
understandable with less implied knowledge.

https://codereview.appspot.com/8748048/diff/1/state/unit_test.go
File state/unit_test.go (right):

https://codereview.appspot.com/8748048/diff/1/state/unit_test.go#newcode735
state/unit_test.go:735: c.Assert(err, IsNil)
It isn't obvious to me what these commands are here for at all. Perhaps
in the future we could have some higher level testing functions that
make the tests more readable and understandable dealing with higher
level concepts.

https://codereview.appspot.com/8748048/

*** Submitted:

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.

R=dimitern, thumper
CC=
https://codereview.appspot.com/8748048

https://codereview.appspot.com/8748048/diff/1/state/unit_test.go
File state/unit_test.go (right):

https://codereview.appspot.com/8748048/diff/1/state/unit_test.go#newcode735
state/unit_test.go:735: c.Assert(err, IsNil)
On 2013/04/15 23:03:24, thumper wrote:
> It isn't obvious to me what these commands are here for at all.
Perhaps in the
> future we could have some higher level testing functions that make the
tests
> more readable and understandable dealing with higher level concepts.

I *think* that everything I could say here on top of the existing
comments is covered in doc/.

https://codereview.appspot.com/8748048/