state: fix service unit removal

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)

[fwereade, 2013-04-15 16:07:36]

Please take a look.

[dimitern, 2013-04-15 16:11:33]

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?

[dimitern, 2013-04-15 16:44:53]

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

[fwereade, 2013-04-15 18:01:23]

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.

[fwereade, 2013-04-15 18:01:33]

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)".

[thumper, 2013-04-15 23:03:24]

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.

[fwereade, 2013-04-16 00:56:04]

*** 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/.