state: aggressive LeaveScope

state: aggressive LeaveScope

The last RelationUnit to leave a Dying scope will now remove the associated
relation.

https://code.launchpad.net/~fwereade/juju-core/aggressive-leave-scope/+merge/129584

Requires: https://code.launchpad.net/~fwereade/juju-core/service-lifecycles/+merge/129570

(do not edit description out of merge proposal)

[fwereade, 2012-10-14 08:59:46]

Please take a look.

[niemeyer, 2012-10-15 20:38:46]

A few comments.. we'd probably benefit from some live chat on this:

https://codereview.appspot.com/6683048/diff/1/state/life.go
File state/life.go (right):

https://codereview.appspot.com/6683048/diff/1/state/life.go#newcode58
state/life.go:58: // lifecycle has failed to advance to (or beyond) Dead, due to
assertion
What's beyond Dead?

https://codereview.appspot.com/6683048/diff/1/state/life.go#newcode73
state/life.go:73: decorate := func(err error) error {
This is getting pretty confusing. I cannot tell what error is returned in which
circumstances and how to call this function anymore without careful
consideration.

I suggest changing it like this:

type ensureDeadError struct {
        what string
        id interface{}
        err error
}

func (e *ensureDeadError) Error() string {
        return fmt.Sprintf("cannot finish termination of %s %#v: v", e.what,
e.id, e.err)
}

func (e *ensureDeadError) onAbort(msg string) error {
        if e.err == txn.ErrAborted {
                return &ensureDeadError{e.what, e.id, errors.New(msg)}
        }
        return e
}

func ensureDead(st *State, coll *mgo.Collection, id interface{}, what string,
assertOps []txn.Op) (err error) {


Then, within the function return plain &ensureDeadError{what, id, err} in all
error cases. No decoration or prefixing.

We don't need assertMsg. Call sites can use the ensureDeadError value, and can
do stuff like:

    if e, ok := err.(*ensureDeadError); ok {
            return e.onAbort("my custom message")
    }

or they can compare e.err directly and do whatever they please with the
abortion.

https://codereview.appspot.com/6683048/diff/1/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6683048/diff/1/state/relation.go#newcode325
state/relation.go:325: if _, ok := err.(*cannotKillError); ok {
This is looking a bit suspect. First, it's assuming it knows what lives in
rel.EnsureDead assertions, which isn't so nice. But perhaps we should even talk
about the overall approach first: as it is, the killing of the relation is
advisory, because it's leaving the relation no matter what, and then considering
what to do with memory state that can go away in a blink. Should it perhaps be
in the same transaction, so we get the guarantee that last-leaving == relation
dying?

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

https://codereview.appspot.com/6683048/diff/1/state/state.go#newcode494
state/state.go:494: // If aborted, either we're deleted... or a new relation,
with
s/we're deleted... or/the relation was previously removed, or/

That said, this isn't entirely true. There's an assertion above regarding
relationcount that must be accounted for, and we should error in a different way
in such cases, because there will probably be weird consequences if we assume
the relation is gone when in fact it's not.

[niemeyer, 2012-10-15 20:38:46]

A few comments.. we'd probably benefit from some live chat on this:

https://codereview.appspot.com/6683048/diff/1/state/life.go
File state/life.go (right):

https://codereview.appspot.com/6683048/diff/1/state/life.go#newcode58
state/life.go:58: // lifecycle has failed to advance to (or beyond) Dead, due to
assertion
What's beyond Dead?

https://codereview.appspot.com/6683048/diff/1/state/life.go#newcode73
state/life.go:73: decorate := func(err error) error {
This is getting pretty confusing. I cannot tell what error is returned in which
circumstances and how to call this function anymore without careful
consideration.

I suggest changing it like this:

type ensureDeadError struct {
        what string
        id interface{}
        err error
}

func (e *ensureDeadError) Error() string {
        return fmt.Sprintf("cannot finish termination of %s %#v: v", e.what,
e.id, e.err)
}

func (e *ensureDeadError) onAbort(msg string) error {
        if e.err == txn.ErrAborted {
                return &ensureDeadError{e.what, e.id, errors.New(msg)}
        }
        return e
}

func ensureDead(st *State, coll *mgo.Collection, id interface{}, what string,
assertOps []txn.Op) (err error) {


Then, within the function return plain &ensureDeadError{what, id, err} in all
error cases. No decoration or prefixing.

We don't need assertMsg. Call sites can use the ensureDeadError value, and can
do stuff like:

    if e, ok := err.(*ensureDeadError); ok {
            return e.onAbort("my custom message")
    }

or they can compare e.err directly and do whatever they please with the
abortion.

https://codereview.appspot.com/6683048/diff/1/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6683048/diff/1/state/relation.go#newcode325
state/relation.go:325: if _, ok := err.(*cannotKillError); ok {
This is looking a bit suspect. First, it's assuming it knows what lives in
rel.EnsureDead assertions, which isn't so nice. But perhaps we should even talk
about the overall approach first: as it is, the killing of the relation is
advisory, because it's leaving the relation no matter what, and then considering
what to do with memory state that can go away in a blink. Should it perhaps be
in the same transaction, so we get the guarantee that last-leaving == relation
dying?

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

https://codereview.appspot.com/6683048/diff/1/state/state.go#newcode494
state/state.go:494: // If aborted, either we're deleted... or a new relation,
with
s/we're deleted... or/the relation was previously removed, or/

That said, this isn't entirely true. There's an assertion above regarding
relationcount that must be accounted for, and we should error in a different way
in such cases, because there will probably be weird consequences if we assume
the relation is gone when in fact it's not.

[fwereade, 2012-10-15 21:20:02]

https://codereview.appspot.com/6683048/diff/1/state/life.go
File state/life.go (right):

https://codereview.appspot.com/6683048/diff/1/state/life.go#newcode73
state/life.go:73: decorate := func(err error) error {
On 2012/10/15 20:38:46, niemeyer wrote:
> This is getting pretty confusing. I cannot tell what error is returned in
which
> circumstances and how to call this function anymore without careful
> consideration.
> 
> I suggest changing it like this:
> 
> type ensureDeadError struct {
>         what string
>         id interface{}
>         err error
> }
> 
> func (e *ensureDeadError) Error() string {
>         return fmt.Sprintf("cannot finish termination of %s %#v: v", e.what,
> e.id, e.err)
> }
> 
> func (e *ensureDeadError) onAbort(msg string) error {
>         if e.err == txn.ErrAborted {
>                 return &ensureDeadError{e.what, e.id, errors.New(msg)}
>         }
>         return e
> }
> 
> func ensureDead(st *State, coll *mgo.Collection, id interface{}, what string,
> assertOps []txn.Op) (err error) {
> 
> 
> Then, within the function return plain &ensureDeadError{what, id, err} in all
> error cases. No decoration or prefixing.
> 
> We don't need assertMsg. Call sites can use the ensureDeadError value, and can
> do stuff like:
> 
>     if e, ok := err.(*ensureDeadError); ok {
>             return e.onAbort("my custom message")
>     }
> 
> or they can compare e.err directly and do whatever they please with the
> abortion.

SGTM

https://codereview.appspot.com/6683048/diff/1/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6683048/diff/1/state/relation.go#newcode325
state/relation.go:325: if _, ok := err.(*cannotKillError); ok {
On 2012/10/15 20:38:46, niemeyer wrote:
> This is looking a bit suspect. First, it's assuming it knows what lives in
> rel.EnsureDead assertions, which isn't so nice. But perhaps we should even
talk
> about the overall approach first: as it is, the killing of the relation is
> advisory, because it's leaving the relation no matter what, and then
considering
> what to do with memory state that can go away in a blink. Should it perhaps be
> in the same transaction, so we get the guarantee that last-leaving == relation
> dying?

I think that would be an excellent idea, but I shied away from it on the basis
that it makes Relation.EnsureDead somewhat obsolete -- this will then, I think,
become the only meaningful client of that method, and I'd rather not duplicate
the logic. I'll implement it as seems sensible; see what you think.

[fwereade, 2012-10-16 23:15:45]

Please take a look.

https://codereview.appspot.com/6683048/diff/1/state/life.go
File state/life.go (right):

https://codereview.appspot.com/6683048/diff/1/state/life.go#newcode73
state/life.go:73: decorate := func(err error) error {
On 2012/10/15 21:20:02, fwereade wrote:
> On 2012/10/15 20:38:46, niemeyer wrote:
> > This is getting pretty confusing. I cannot tell what error is returned in
> which
> > circumstances and how to call this function anymore without careful
> > consideration.
> > 
> > I suggest changing it like this:
> > 
> > type ensureDeadError struct {
> >         what string
> >         id interface{}
> >         err error
> > }
> > 
> > func (e *ensureDeadError) Error() string {
> >         return fmt.Sprintf("cannot finish termination of %s %#v: v", e.what,
> > e.id, e.err)
> > }
> > 
> > func (e *ensureDeadError) onAbort(msg string) error {
> >         if e.err == txn.ErrAborted {
> >                 return &ensureDeadError{e.what, e.id, errors.New(msg)}
> >         }
> >         return e
> > }
> > 
> > func ensureDead(st *State, coll *mgo.Collection, id interface{}, what
string,
> > assertOps []txn.Op) (err error) {
> > 
> > 
> > Then, within the function return plain &ensureDeadError{what, id, err} in
all
> > error cases. No decoration or prefixing.
> > 
> > We don't need assertMsg. Call sites can use the ensureDeadError value, and
can
> > do stuff like:
> > 
> >     if e, ok := err.(*ensureDeadError); ok {
> >             return e.onAbort("my custom message")
> >     }
> > 
> > or they can compare e.err directly and do whatever they please with the
> > abortion.
> 
> SGTM

You know what, it's even easier if I just revert this file entirely.

[niemeyer, 2012-10-17 01:36:15]

LGTM

https://codereview.appspot.com/6683048/diff/9001/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6683048/diff/9001/state/relation.go#newcode158
state/relation.go:158: func (r *Relation) removeOps(asserts D) []txn.Op {
Nice idea for this case.

https://codereview.appspot.com/6683048/diff/9001/state/relation.go#newcode327
state/relation.go:327: // of the relation; if the relation is dying when its
last member unit
Nice docs.

https://codereview.appspot.com/6683048/diff/9001/state/relation.go#newcode351
state/relation.go:351: // than we can resolve.
Logic looks great, but the comment feels a bit hard to understand. Here is a
suggestion for a more prosaic version:

// The logic below is involved because we remove a dying relation
// with the last unit that leaves a scope in it. It handles three
// possible cases:
//
// 1. Relation is alive: just leave the scope.
//
// 2. Relation is dying, and other units remain: just leave the scope.
//
// 3. Relation is dying, and this is the last unit: leave the scope
//    and remove the relation.
//
// In each of those cases, proper assertions are done to guarantee
// that the condition observed is still valid when the transaction is
// applied. If an abort happens, it observes the new condition and
// retries. In theory, a worst case will try at most all of the
// conditions once, because units cannot join a scope once its relation
// is dying.
//
// Keep in mind that in the first iteration of the loop it's possible
// to have a Dying relation with a smaller-than-real unit count, because
// EnsureDying changes the Life attribute in memory (units could join
// before the database is actually changed).

https://codereview.appspot.com/6683048/diff/9001/state/relation.go#newcode353
state/relation.go:353: for i := 0; i <= limit; i++ {

What do you think of:

for attempt := 0; attempt < 3; attempt++ {

https://codereview.appspot.com/6683048/diff/9001/state/relation.go#newcode360
state/relation.go:360: return fmt.Errorf("unit %q cannot leave relation %q
scope: inconsistent state", ru.unit, ru.relation)
This can be moved to after the for loop in place of the unreachable panic, and
the loop condition changed to <.

https://codereview.appspot.com/6683048/diff/9001/state/relation.go#newcode395
state/relation.go:395: }
This is the same as:

    if err = ru.st.runner.Run(ops, "", nil); err != txn.ErrAborted {
        return err
    }
    if err := ru.relation.Refresh(); IsNotFound(err) {
        return nil
    } else if err != nil {
        return err
    }
}

https://codereview.appspot.com/6683048/diff/9001/state/relation_test.go
File state/relation_test.go (right):

https://codereview.appspot.com/6683048/diff/9001/state/relation_test.go#newco...
state/relation_test.go:369: c.Assert(state.IsNotFound(err), Equals, true)
Woot.

[fwereade, 2012-10-18 14:41:21]

*** Submitted:

state: aggressive LeaveScope

The last RelationUnit to leave a Dying scope will now remove the associated
relation.

R=niemeyer
CC=
https://codereview.appspot.com/6683048

https://codereview.appspot.com/6683048/diff/9001/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6683048/diff/9001/state/relation.go#newcode351
state/relation.go:351: // than we can resolve.
On 2012/10/17 01:36:15, niemeyer wrote:
> Logic looks great, but the comment feels a bit hard to understand. Here is a
> suggestion for a more prosaic version:
> 
> // The logic below is involved because we remove a dying relation
> // with the last unit that leaves a scope in it. It handles three
> // possible cases:
> //
> // 1. Relation is alive: just leave the scope.
> //
> // 2. Relation is dying, and other units remain: just leave the scope.
> //
> // 3. Relation is dying, and this is the last unit: leave the scope
> //    and remove the relation.
> //
> // In each of those cases, proper assertions are done to guarantee
> // that the condition observed is still valid when the transaction is
> // applied. If an abort happens, it observes the new condition and
> // retries. In theory, a worst case will try at most all of the
> // conditions once, because units cannot join a scope once its relation
> // is dying.
> //
> // Keep in mind that in the first iteration of the loop it's possible
> // to have a Dying relation with a smaller-than-real unit count, because
> // EnsureDying changes the Life attribute in memory (units could join
> // before the database is actually changed).

Excellent-- took it verbatim. Thanks :).

https://codereview.appspot.com/6683048/diff/9001/state/relation.go#newcode353
state/relation.go:353: for i := 0; i <= limit; i++ {
On 2012/10/17 01:36:15, niemeyer wrote:
> 
> What do you think of:
> 
> for attempt := 0; attempt < 3; attempt++ {

Done.

https://codereview.appspot.com/6683048/diff/9001/state/relation.go#newcode360
state/relation.go:360: return fmt.Errorf("unit %q cannot leave relation %q
scope: inconsistent state", ru.unit, ru.relation)
On 2012/10/17 01:36:15, niemeyer wrote:
> This can be moved to after the for loop in place of the unreachable panic, and
> the loop condition changed to <.

Done.

https://codereview.appspot.com/6683048/diff/9001/state/relation.go#newcode395
state/relation.go:395: }
On 2012/10/17 01:36:15, niemeyer wrote:
> This is the same as:
> 
>     if err = ru.st.runner.Run(ops, "", nil); err != txn.ErrAborted {
>         return err
>     }
>     if err := ru.relation.Refresh(); IsNotFound(err) {
>         return nil
>     } else if err != nil {
>         return err
>     }
> }

Done, but wrapped the mgo error when not nil.