state: stricter txn assertions on service destroy

state: stricter txn assertions on service destroy

As originally formulated, a changed unit count would not be detected if the
relation count was unchanged; this engendered lp:1152717. We now check
rather more strictly.

Several service destruction tests were rearranged and renamed for clarity.

https://code.launchpad.net/~fwereade/juju-core/stricter-service-destroy-ops/+merge/153113

(do not edit description out of merge proposal)

[fwereade, 2013-03-13 11:55:29]

Please take a look.

[dimitern, 2013-03-13 11:59:40]

LGTM, but I'm not entirely sure of the implications of simplifying the asserts
there. Can you give examples what could go wrong?

[dimitern, 2013-03-13 12:00:38]

Also, should this be linked to lp:1152717 and resolve it?

[fwereade, 2013-03-14 08:44:38]

On 2013/03/13 11:59:40, dimitern wrote:
> LGTM, but I'm not entirely sure of the implications of simplifying the asserts
> there. Can you give examples what could go wrong?

On 2013/03/13 11:59:40, dimitern wrote:
> LGTM, but I'm not entirely sure of the implications of simplifying the asserts
> there. Can you give examples what could go wrong?

Original implementation allowed a stale unit count of 1 (really 0) to pass
because it was $or with the check for matching relation count (which was valid).

Check for $gt removeCount won't work because a relation could be added without
being detected (leaving an Alive relation with a Dying service), so we
definitely need to stick with equality with doc.RelationCount (removed relations
should always be caught by the separate per-relation checks collected earlier in
the method).

Hmm, maybe we can do this by making the RelationCount check unconditional, and
conditionally checking for UnitCount >0 or ==0 depending on the last observed
state. That eliminates the major drawback of a txn-revno check, which is that
*any* change to the document will cause it to abort, rather than just the
changes we care about.

Reproposing in a mo; someone please think moderately hard about my logic :).

[fwereade, 2013-03-14 08:44:51]

On 2013/03/13 12:00:38, dimitern wrote:
> Also, should this be linked to lp:1152717 and resolve it?

I think it is?

[fwereade, 2013-03-14 08:50:56]

Please take a look.

[dimitern, 2013-03-14 09:03:09]

LGTM

[rog, 2013-03-15 10:22:56]

LGTM except i'd really like to see some better testing around this.

https://codereview.appspot.com/7591044/diff/7001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/7591044/diff/7001/state/service.go#newcode128
state/service.go:128: func (s *Service) destroyOps() ([]txn.Op, error) {
the logic here is really quite involved. i'd like to believe that it works in
all circumstances, but i'm not entirely convinced that the current hree tests
actually exercise all the possibilities.

if nothing else, an internal test that tested the output of destroyOps itself
might be useful.

[fwereade, 2013-03-15 10:47:56]

On 2013/03/15 10:22:56, rog wrote:
> LGTM except i'd really like to see some better testing around this.
> 
> https://codereview.appspot.com/7591044/diff/7001/state/service.go
> File state/service.go (right):
> 
> https://codereview.appspot.com/7591044/diff/7001/state/service.go#newcode128
> state/service.go:128: func (s *Service) destroyOps() ([]txn.Op, error) {
> the logic here is really quite involved. i'd like to believe that it works in
> all circumstances, but i'm not entirely convinced that the current hree tests
> actually exercise all the possibilities.
> 
> if nothing else, an internal test that tested the output of destroyOps itself
> might be useful.

Yeah, I've felt uncomfortable about this for a while. There's at least one more
obvious external test I should write, but there are more than three in
existence; the naming is, however, somewhat inconsistent, and it's not always
clear what's being tested.

I'm a touch ambivalent about really detailed testing of the ops themselves: I'll
see what I can conclude about coverage once I've made the various destruction
tests more consistent.

[fwereade, 2013-03-15 14:52:29]

*** Submitted:

state: stricter txn assertions on service destroy

As originally formulated, a changed unit count would not be detected if the
relation count was unchanged; this engendered lp:1152717. We now check
rather more strictly.

Several service destruction tests were rearranged and renamed for clarity.

R=dimitern, rog
CC=
https://codereview.appspot.com/7591044