state: integrate service relations with life

state: integrate service relations with life

* setting a service to dying also sets all its relations to dying (units are
  not set to dying; we don't want any 100000-op transactions).
* relations can no longer be added when involved services are not alive.
* adding a relation adds to the relations refcount of each involved service.
* removing a relation decrefs its services.
* a service cannot be set to dead while it still has unremoved relations.
* units cannot be added to a dying relation
* the last unit to leave a dying relation removes it directly

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

(do not edit description out of merge proposal)

[fwereade, 2012-10-13 18:49:45]

Please take a look.

[niemeyer, 2012-10-15 19:46:42]

Very good stuff. Pretty much all non-trivial and sensible.

Some comments:

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

https://codereview.appspot.com/6678046/diff/1/state/relation.go#newcode136
state/relation.go:136: // EnsureDead tries to set the relation lifecycle to Dead
if it is Alive or
I suggest avoiding the "tries to" wording, as in theory all the methods that may
return an error "try to do" whatever they're meant to do.

// EnsureDead sets the relation lifecycle to Dead if it is Alive or Dying,
// and does nothing if already Dead.
// It's an error to call it while there are still units within one
// or more scopes in the relation.

https://codereview.appspot.com/6678046/diff/1/state/relation.go#newcode239
state/relation.go:239: var ErrScopeDying = errors.New("scope is closed to new
member units")
Do we need such an error variable? We have quite a few of those cases already,
an in general we've been handling them by returning errNotAlive. If you haven't
seen it, please grep for errNotAlive to get a feeling for it.

https://codereview.appspot.com/6678046/diff/1/state/relation.go#newcode243
state/relation.go:243: // be entered because the relation is dying, or worse, it
returns ErrScopeDying.
I think this would explain more tersely and avoid the "or worse" case:

// It is an error to enter a scope of a relation that is not alive, and
// no relation becomes Dead before all units have left.

https://codereview.appspot.com/6678046/diff/1/state/relation.go#newcode274
state/relation.go:274: // the relation is dying (or worse). If the former, don't
treat this
s/dying (or worse)/not alive/

https://codereview.appspot.com/6678046/diff/1/state/relation.go#newcode276
state/relation.go:276: // to read the settings will fail predictably, and the
root cause can
I'm not entirely sure if this is true. There's an assertion on DocMissing for
settings above, which will fail when we leave the scope, and if we try to enter
the scope again it will already exist even though our actual relationScopeDoc
isn't present and we can't be said to be part of the relation. It feels like
that assert on settings could go away, but even then we cannot use the presence
of *settings* to determine what the case is. The unit may already be in the
scope, or the relation can be dead. With ErrAborted we can only tell one of the
assertions failed.

https://codereview.appspot.com/6678046/diff/1/state/relation.go#newcode278
state/relation.go:278: settings, err := readSettings(ru.st, key)
if IsNotFound(err) {
    return errNotAlive
} else if err != nil {
    return err
}

?

https://codereview.appspot.com/6678046/diff/1/state/relation.go#newcode298
state/relation.go:298: // not, or never was, a member of.
Nice clarification.

https://codereview.appspot.com/6678046/diff/1/state/relation.go#newcode316
state/relation.go:316: if err == txn.ErrAborted {
One of the asserts above is a consistency check. If we're checking that, we
should also check here to make sure we know which assertion has failed, because
otherwise we can say "All good!" when in fact it's completely broken and the
unit hasn't even left.

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

https://codereview.appspot.com/6678046/diff/1/state/service.go#newcode58
state/service.go:58: for {
for s.doc.Life == Alive {

The first TxnRevno may well be the one we already have in memory. I think it
would be okay to refresh the service in this case, and use s itself below
instead of current. A lot of logic goes away with this.

https://codereview.appspot.com/6678046/diff/1/state/service.go#newcode72
state/service.go:72: }
All of the above can go away.

https://codereview.appspot.com/6678046/diff/1/state/service.go#newcode79
state/service.go:79: log.Debugf("state: service %q relations changed; retrying",
s)
if err = s.Refresh(); err != nil {
    return err
}

https://codereview.appspot.com/6678046/diff/1/state/service.go#newcode86
state/service.go:86: {"relationcount", current.RelationCount},
The relation count doesn't really matter here. n+1-1 == n. It's txn-revno that
gives us assurance of correctness.

https://codereview.appspot.com/6678046/diff/1/state/service.go#newcode96
state/service.go:96: Assert: isAlive,
I suspect notDead would actually work well here.

https://codereview.appspot.com/6678046/diff/1/state/service.go#newcode102
state/service.go:102: log.Debugf("state: service %q changed, retrying", s)
if err = s.Refresh(); err != nil {
    return err
}

https://codereview.appspot.com/6678046/diff/1/state/service.go#newcode109
state/service.go:109: return nil
        s.doc.Life = Dying
    }
    return nil
}

With the suggested changes the loop condition can deal with it.

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

https://codereview.appspot.com/6678046/diff/1/state/state.go#newcode420
state/state.go:420: if svc.Life() != Alive {
if IsNotFound(err) || svc.Life() != Alive {
    ...
} else if err != nil {
    return nil, err
}

... I think.

https://codereview.appspot.com/6678046/diff/1/state/state.go#newcode458
state/state.go:458: // RemoveRelation removes the supplied relation and all its
unit settings.
Hmm.. wasn't that handled in the other CL? Either way, we discussed that today
on IRC.

[fwereade, 2012-10-16 06:31:39]

Gaah: I have a couple of preliminary thoughts, but I've just realised I missed
the prereq :/. I'll address those comments in
https://codereview.appspot.com/6687043/ and try to sort this all out.

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

https://codereview.appspot.com/6678046/diff/1/state/relation.go#newcode136
state/relation.go:136: // EnsureDead tries to set the relation lifecycle to Dead
if it is Alive or
On 2012/10/15 19:46:42, niemeyer wrote:
> I suggest avoiding the "tries to" wording, as in theory all the methods that
may
> return an error "try to do" whatever they're meant to do.
> 
> // EnsureDead sets the relation lifecycle to Dead if it is Alive or Dying,
> // and does nothing if already Dead.
> // It's an error to call it while there are still units within one
> // or more scopes in the relation.

Nice, thanks

https://codereview.appspot.com/6678046/diff/1/state/relation.go#newcode239
state/relation.go:239: var ErrScopeDying = errors.New("scope is closed to new
member units")
On 2012/10/15 19:46:42, niemeyer wrote:
> Do we need such an error variable? We have quite a few of those cases already,
> an in general we've been handling them by returning errNotAlive. If you
haven't
> seen it, please grep for errNotAlive to get a feeling for it.

I think we do -- the uniter needs to distinguish between unexpectedly dead
relations (ErrScopeDying -- no big deal, just don't enter the scope), and other
errors (bail out); but not between "you successfully entered for the first time"
and "you were already there" (no error).

https://codereview.appspot.com/6678046/diff/1/state/relation.go#newcode243
state/relation.go:243: // be entered because the relation is dying, or worse, it
returns ErrScopeDying.
On 2012/10/15 19:46:42, niemeyer wrote:
> I think this would explain more tersely and avoid the "or worse" case:
> 
> // It is an error to enter a scope of a relation that is not alive, and
> // no relation becomes Dead before all units have left.

Nice.

[fwereade, 2012-10-16 13:59:57]

comments to go with the latest push (was -wip)

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

https://codereview.appspot.com/6678046/diff/1/state/relation.go#newcode136
state/relation.go:136: // EnsureDead tries to set the relation lifecycle to Dead
if it is Alive or
On 2012/10/16 06:31:39, fwereade wrote:
> On 2012/10/15 19:46:42, niemeyer wrote:
> > I suggest avoiding the "tries to" wording, as in theory all the methods that
> may
> > return an error "try to do" whatever they're meant to do.
> > 
> > // EnsureDead sets the relation lifecycle to Dead if it is Alive or Dying,
> > // and does nothing if already Dead.
> > // It's an error to call it while there are still units within one
> > // or more scopes in the relation.
> 
> Nice, thanks

Done in https://codereview.appspot.com/6687043/

https://codereview.appspot.com/6678046/diff/1/state/relation.go#newcode239
state/relation.go:239: var ErrScopeDying = errors.New("scope is closed to new
member units")
On 2012/10/16 06:31:39, fwereade wrote:
> On 2012/10/15 19:46:42, niemeyer wrote:
> > Do we need such an error variable? We have quite a few of those cases
already,
> > an in general we've been handling them by returning errNotAlive. If you
> haven't
> > seen it, please grep for errNotAlive to get a feeling for it.
> 
> I think we do -- the uniter needs to distinguish between unexpectedly dead
> relations (ErrScopeDying -- no big deal, just don't enter the scope), and
other
> errors (bail out); but not between "you successfully entered for the first
time"
> and "you were already there" (no error).

Renamed in https://codereview.appspot.com/6687043/

https://codereview.appspot.com/6678046/diff/1/state/relation.go#newcode243
state/relation.go:243: // be entered because the relation is dying, or worse, it
returns ErrScopeDying.
On 2012/10/15 19:46:42, niemeyer wrote:
> I think this would explain more tersely and avoid the "or worse" case:
> 
> // It is an error to enter a scope of a relation that is not alive, and
> // no relation becomes Dead before all units have left.

Done in https://codereview.appspot.com/6687043/

https://codereview.appspot.com/6678046/diff/1/state/relation.go#newcode316
state/relation.go:316: if err == txn.ErrAborted {
On 2012/10/15 19:46:42, niemeyer wrote:
> One of the asserts above is a consistency check. If we're checking that, we
> should also check here to make sure we know which assertion has failed,
because
> otherwise we can say "All good!" when in fact it's completely broken and the
> unit hasn't even left.

Done in https://codereview.appspot.com/6687043/

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

https://codereview.appspot.com/6678046/diff/1/state/service.go#newcode58
state/service.go:58: for {
On 2012/10/15 19:46:42, niemeyer wrote:
> for s.doc.Life == Alive {
> 
> The first TxnRevno may well be the one we already have in memory. I think it
> would be okay to refresh the service in this case, and use s itself below
> instead of current. A lot of logic goes away with this.

Nice. Done.

https://codereview.appspot.com/6678046/diff/1/state/service.go#newcode72
state/service.go:72: }
On 2012/10/15 19:46:42, niemeyer wrote:
> All of the above can go away.

Done.

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

https://codereview.appspot.com/6678046/diff/1/state/state.go#newcode420
state/state.go:420: if svc.Life() != Alive {
On 2012/10/15 19:46:42, niemeyer wrote:
> if IsNotFound(err) || svc.Life() != Alive {
>     ...
> } else if err != nil {
>     return nil, err
> }
> 
> ... I think.

I think so too. Neat. Thanks :).

[fwereade, 2012-10-16 15:51:18]

Please take a look.

[niemeyer, 2012-10-17 20:52:11]

It's looking good, and is pretty close, but we need another round:

https://codereview.appspot.com/6678046/diff/11001/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6678046/diff/11001/state/relation.go#newcode238
state/relation.go:238: // ErrScopeClosed indicates that a relation scope cannot
accept new members
If we do need it, this looks like:

var ErrRelationDying = errors.New("relation is dying")

https://codereview.appspot.com/6678046/diff/11001/state/relation.go#newcode252
state/relation.go:252: return err
That's an error from mgo. We should probably say something about what is
actually failing.

https://codereview.appspot.com/6678046/diff/11001/state/relation.go#newcode273
state/relation.go:273: ops = append(ops, txn.Op{
I think we want to create settings before we create the scope document, as we
used to.

https://codereview.appspot.com/6678046/diff/11001/state/relation.go#newcode280
state/relation.go:280: return err
May also be worth saying something about the context.

https://codereview.appspot.com/6678046/diff/11001/state/relation.go#newcode284
state/relation.go:284: return err
Also an error from mgo.

https://codereview.appspot.com/6678046/diff/11001/state/relation.go#newcode285
state/relation.go:285: } else if count == 0 {
Why are we assuming that *no relation scope* is a dead relation, and everything
else is inconsistency? Shouldn't we check that the relation is dying instead?

https://codereview.appspot.com/6678046/diff/11001/state/relation.go#newcode305
state/relation.go:305: return err
Also deserves adorning, i think.

https://codereview.appspot.com/6678046/diff/11001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6678046/diff/11001/state/state.go#newcode473
state/state.go:473: Assert: txn.DocMissing,
The assert here doesn't seem necessary.

https://codereview.appspot.com/6678046/diff/11001/state/state.go#newcode490
state/state.go:490: if err == txn.ErrAborted {
It doesn't look like we have to check it here. The only ways it can abort are:

1) Relation not found
2) Relation not dead
3) Relation id doesn't match relation

1 is what we want. 2 and 3 can only happen if the relation is removed and
re-inserted with a new id, which is a fine thing to happen.

https://codereview.appspot.com/6678046/diff/11001/state/state.go#newcode615
state/state.go:615: // Cleanup removes several documents that were previously
marked for removal,
Oh, nice. Wasn't expecting to see that yet, but very welcome.

https://codereview.appspot.com/6678046/diff/11001/state/state.go#newcode616
state/state.go:616: // if any such exist. A result of ErrNotClean indicates that
not all documents
This doesn't feel like an error, and also doesn't feel so useful in practice, in
the sense that we have to retry in a while anyway. I suggest naming it as
CleanupRound and just state that it doesn't collect all at once.

https://codereview.appspot.com/6678046/diff/11001/state/state.go#newcode636
state/state.go:636: panic(fmt.Errorf("unknown cleanup kind %q", doc.Kind))
It should log and ignore anything unknown, otherwise it's impossible to add new
cleanups without blowing things up.

https://codereview.appspot.com/6678046/diff/11001/state/state.go#newcode642
state/state.go:642: ops := []txn.Op{}
At least so far, I *think* it would be safe to use RemoveAll here, because by
the time we insert the cleanup documents, everybody should already have
abandoned any kind of settings handling on those documents. I also suggest
simply removing all of it at once. Even if it takes a while and loads the
database a bit, it will be done server side and without interaction, so
shouldn't be so problematic. The simplicity of the implementation pays off
trying at least.

https://codereview.appspot.com/6678046/diff/11001/state/state.go#newcode671
state/state.go:671: // If anything remains to be deleted, notify the client.
This can go too. I think this method will end up pleasantly small.

[fwereade, 2012-10-18 08:33:33]

Please take a look.

https://codereview.appspot.com/6678046/diff/11001/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6678046/diff/11001/state/relation.go#newcode238
state/relation.go:238: // ErrScopeClosed indicates that a relation scope cannot
accept new members
On 2012/10/17 20:52:11, niemeyer wrote:
> If we do need it, this looks like:
> 
> var ErrRelationDying = errors.New("relation is dying")

ErrRelationNotAlive, I think.

https://codereview.appspot.com/6678046/diff/11001/state/relation.go#newcode252
state/relation.go:252: return err
On 2012/10/17 20:52:11, niemeyer wrote:
> That's an error from mgo. We should probably say something about what is
> actually failing.

Done.

https://codereview.appspot.com/6678046/diff/11001/state/relation.go#newcode273
state/relation.go:273: ops = append(ops, txn.Op{
On 2012/10/17 20:52:11, niemeyer wrote:
> I think we want to create settings before we create the scope document, as we
> used to.

Gaah, and after we discussed it, too. Sorry :/.

https://codereview.appspot.com/6678046/diff/11001/state/relation.go#newcode280
state/relation.go:280: return err
On 2012/10/17 20:52:11, niemeyer wrote:
> May also be worth saying something about the context.

Done.

https://codereview.appspot.com/6678046/diff/11001/state/relation.go#newcode284
state/relation.go:284: return err
On 2012/10/17 20:52:11, niemeyer wrote:
> Also an error from mgo.

Done.

https://codereview.appspot.com/6678046/diff/11001/state/relation.go#newcode285
state/relation.go:285: } else if count == 0 {
On 2012/10/17 20:52:11, niemeyer wrote:
> Why are we assuming that *no relation scope* is a dead relation, and
everything
> else is inconsistency? Shouldn't we check that the relation is dying instead?
I won't bore you with half-assed handwaving. Done.

https://codereview.appspot.com/6678046/diff/11001/state/relation.go#newcode305
state/relation.go:305: return err
On 2012/10/17 20:52:11, niemeyer wrote:
> Also deserves adorning, i think.

Done.

https://codereview.appspot.com/6678046/diff/11001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6678046/diff/11001/state/state.go#newcode473
state/state.go:473: Assert: txn.DocMissing,
On 2012/10/17 20:52:11, niemeyer wrote:
> The assert here doesn't seem necessary.

Good point. Done.

https://codereview.appspot.com/6678046/diff/11001/state/state.go#newcode490
state/state.go:490: if err == txn.ErrAborted {
On 2012/10/17 20:52:11, niemeyer wrote:
> It doesn't look like we have to check it here. The only ways it can abort are:
> 
> 1) Relation not found
> 2) Relation not dead
> 3) Relation id doesn't match relation
> 
> 1 is what we want. 2 and 3 can only happen if the relation is removed and
> re-inserted with a new id, which is a fine thing to happen.

Hmm, good point again. Thanks :).

https://codereview.appspot.com/6678046/diff/11001/state/state.go#newcode636
state/state.go:636: panic(fmt.Errorf("unknown cleanup kind %q", doc.Kind))
On 2012/10/17 20:52:11, niemeyer wrote:
> It should log and ignore anything unknown, otherwise it's impossible to add
new
> cleanups without blowing things up.

Done.

https://codereview.appspot.com/6678046/diff/11001/state/state.go#newcode642
state/state.go:642: ops := []txn.Op{}
On 2012/10/17 20:52:11, niemeyer wrote:
> At least so far, I *think* it would be safe to use RemoveAll here, because by
> the time we insert the cleanup documents, everybody should already have
> abandoned any kind of settings handling on those documents. I also suggest
> simply removing all of it at once. Even if it takes a while and loads the
> database a bit, it will be done server side and without interaction, so
> shouldn't be so problematic. The simplicity of the implementation pays off
> trying at least.

SGTM.

It also emerged, as I was making these changes, that it's actually easier to
just blast through all the cleanups at once; see what you think.

https://codereview.appspot.com/6678046/diff/11001/state/state.go#newcode671
state/state.go:671: // If anything remains to be deleted, notify the client.
On 2012/10/17 20:52:11, niemeyer wrote:
> This can go too. I think this method will end up pleasantly small.

Done.

[niemeyer, 2012-10-18 13:47:00]

LGTM, thanks!

[fwereade, 2012-10-18 14:12:35]

*** Submitted:

state: integrate service relations with life

* setting a service to dying also sets all its relations to dying (units are
  not set to dying; we don't want any 100000-op transactions).
* relations can no longer be added when involved services are not alive.
* adding a relation adds to the relations refcount of each involved service.
* removing a relation decrefs its services.
* a service cannot be set to dead while it still has unremoved relations.
* units cannot be added to a dying relation
* the last unit to leave a dying relation removes it directly

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