state: service API changes

state: service API changes

Removed:

  * Service.EnsureDying
  * Service.EnsureDead
  * State.RemoveService

Added:

  * Service.Destroy

Service.Destroy will remove the service and all its relations, if possible;
any that cannot be will be set to Dying. Service removal is now handled via
Service.RemoveUnit (which should become Unit.Remove, but this CL is big
enough already) and RelationUnit.LeaveScope, each of which will remove any
Dying service to which they hold the last reference.

In addition to the motivating lp:1089285 (service API), this inevitably
addresses lp:1089284 (service removal on LeaveScope) and lp:1100070 (unit
removal failure).

Reviewers unfamiliar with the details of the existing operations are likely
to be best served by reading doc/draft/death-and-destruction.txt, and to
compare the implementation against the model described therein, rather than
to worry *too* much about the previous implementation.

https://code.launchpad.net/~fwereade/juju-core/state-service-api/+merge/143394

(do not edit description out of merge proposal)

[fwereade, 2013-01-15 23:32:38]

Please take a look.

[dave_cheney.net, 2013-01-16 03:20:25]

Some initial comments, mainly about scoping issues with the err value.

https://codereview.appspot.com/7095059/diff/6001/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/7095059/diff/6001/state/relation.go#newcode100
state/relation.go:100: defer func() {
given the number of review comments on this function, maybe it could be handled
differently.

https://codereview.appspot.com/7095059/diff/6001/state/relation.go#newcode112
state/relation.go:112: ops, _, err := rel.destroyOps("")
this err is in a new scope to the referenced by defer, I suggest calling it e,
err1 or bob.

https://codereview.appspot.com/7095059/diff/6001/state/relation.go#newcode119
state/relation.go:119: if err := rel.st.runner.Run(ops, "", nil); err !=
txn.ErrAborted {
same

https://codereview.appspot.com/7095059/diff/6001/state/relation.go#newcode122
state/relation.go:122: if err := rel.Refresh(); IsNotFound(err) {
same

https://codereview.appspot.com/7095059/diff/6001/state/relation.go#newcode144
state/relation.go:144: ops, err := r.removeOps(mode, destroyingService)
this could be just return r.removeOps(...)

convention dictates that you cannot assume anything about the other return
values until you have consulted the error.

https://codereview.appspot.com/7095059/diff/6001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/7095059/diff/6001/state/service.go#newcode67
state/service.go:67: ops, err := svc.destroyOps()
err is shadowed.

https://codereview.appspot.com/7095059/diff/6001/state/service.go#newcode75
state/service.go:75: if err := svc.st.runner.Run(ops, "", nil); err !=
txn.ErrAborted {
twice

https://codereview.appspot.com/7095059/diff/6001/state/service.go#newcode280
state/service.go:280: if err == mgo.ErrNotFound {
can be merged with the previous line.

https://codereview.appspot.com/7095059/diff/6001/state/service.go#newcode382
state/service.go:382: if err := svc.st.runner.Run(ops, "", nil); err !=
txn.ErrAborted {
err scoping.

[fwereade, 2013-01-16 08:02:05]

I'm worried there's something I just Do Not Get about the error scoping
complaints...

https://codereview.appspot.com/7095059/diff/6001/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/7095059/diff/6001/state/relation.go#newcode112
state/relation.go:112: ops, _, err := rel.destroyOps("")
On 2013/01/16 03:20:25, dfc wrote:
> this err is in a new scope to the referenced by defer, I suggest calling it e,
> err1 or bob.

Why does this matter? The err checked in the defer is the named return value,
which will take the value of the error we actually return, regardless of what
happens with other vars that may or may not happen to be called err. I see this
as normal, common, and unsurprising; would you expand on your objections please?

https://codereview.appspot.com/7095059/diff/6001/state/relation.go#newcode119
state/relation.go:119: if err := rel.st.runner.Run(ops, "", nil); err !=
txn.ErrAborted {
On 2013/01/16 03:20:25, dfc wrote:
> same

Ditto.

https://codereview.appspot.com/7095059/diff/6001/state/relation.go#newcode122
state/relation.go:122: if err := rel.Refresh(); IsNotFound(err) {
On 2013/01/16 03:20:25, dfc wrote:
> same

Ditto.

https://codereview.appspot.com/7095059/diff/6001/state/relation.go#newcode144
state/relation.go:144: ops, err := r.removeOps(mode, destroyingService)
On 2013/01/16 03:20:25, dfc wrote:
> this could be just return r.removeOps(...)

How so?

> convention dictates that you cannot assume anything about the other return
> values until you have consulted the error.

Er. Where do I fail to do this?

https://codereview.appspot.com/7095059/diff/6001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/7095059/diff/6001/state/service.go#newcode67
state/service.go:67: ops, err := svc.destroyOps()
On 2013/01/16 03:20:25, dfc wrote:
> err is shadowed.
So what?

https://codereview.appspot.com/7095059/diff/6001/state/service.go#newcode75
state/service.go:75: if err := svc.st.runner.Run(ops, "", nil); err !=
txn.ErrAborted {
On 2013/01/16 03:20:25, dfc wrote:
> twice

And the consequences of doing this are..?

https://codereview.appspot.com/7095059/diff/6001/state/service.go#newcode280
state/service.go:280: if err == mgo.ErrNotFound {
On 2013/01/16 03:20:25, dfc wrote:
> can be merged with the previous line.

Good point, thanks.

https://codereview.appspot.com/7095059/diff/6001/state/service.go#newcode382
state/service.go:382: if err := svc.st.runner.Run(ops, "", nil); err !=
txn.ErrAborted {
On 2013/01/16 03:20:25, dfc wrote:
> err scoping.

Don't see the problem.

[fwereade, 2013-01-16 09:03:14]

Please take a look.

[rog, 2013-01-16 16:17:58]

LGTM, although i'm not sure i've grasped the full details of some of the
changes.

a few minor thoughts below.

https://codereview.appspot.com/7095059/diff/6001/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/7095059/diff/6001/state/relation.go#newcode112
state/relation.go:112: ops, _, err := rel.destroyOps("")
On 2013/01/16 08:02:06, fwereade wrote:
> On 2013/01/16 03:20:25, dfc wrote:
> > this err is in a new scope to the referenced by defer, I suggest calling it
e,
> > err1 or bob.
> 
> Why does this matter? The err checked in the defer is the named return value,
> which will take the value of the error we actually return, regardless of what
> happens with other vars that may or may not happen to be called err. I see
this
> as normal, common, and unsurprising; would you expand on your objections
please?

i'm with william. i don't think there's a problem with the error handling here.

http://play.golang.org/p/x_db5UU8Gh

https://codereview.appspot.com/7095059/diff/12001/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/7095059/diff/12001/state/relation.go#newcode160
state/relation.go:160: type removeMode string
any particular reason this isn't int, with an iota const below? it seems only to
be used internally.

https://codereview.appspot.com/7095059/diff/12001/state/relation.go#newcode203
state/relation.go:203: // service identified by the supplied endpoint, as
determined by mode and
"to maintain the service" ?

https://codereview.appspot.com/7095059/diff/12001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/7095059/diff/12001/state/service.go#newcode111
state/service.go:111: removeCount += 1
removeCount++

https://codereview.appspot.com/7095059/diff/12001/state/service_test.go
File state/service_test.go (right):

https://codereview.appspot.com/7095059/diff/12001/state/service_test.go#newco...
state/service_test.go:209: // TODO(fwereade): maybe service destruction should
always unexpose?
+1

[niemeyer, 2013-01-16 23:15:47]

This is solely reviewing the death-and-destruction doc.

Quite amazing coverage, really. Thanks!

https://codereview.appspot.com/7095059/diff/12001/doc/draft/death-and-destruc...
File doc/draft/death-and-destruction.txt (right):

https://codereview.appspot.com/7095059/diff/12001/doc/draft/death-and-destruc...
doc/draft/death-and-destruction.txt:13: * Entities might be removed immediately
they are destroyed, but this is not
I think there's an adverb missing after immediately, such as "after", or "when",
or "at the time".

https://codereview.appspot.com/7095059/diff/12001/doc/draft/death-and-destruc...
doc/draft/death-and-destruction.txt:15: * If an entity is not removed
immediately it is destroyed, its eventual
Same.

https://codereview.appspot.com/7095059/diff/12001/doc/draft/death-and-destruc...
doc/draft/death-and-destruction.txt:61: * If the machine has JobManageEnviron,
or has any assigned units, abort with
We might say s/has/is the last machine with/ here, in anticipation of intended
semantics. Won't make a difference in the short term, though.

https://codereview.appspot.com/7095059/diff/12001/doc/draft/death-and-destruc...
doc/draft/death-and-destruction.txt:71: * The machine's agent terminates itself
and refuses to run again.
That's actually a follow up on the actions of Dying, rather than the machine
agent observing the Dead change.

https://codereview.appspot.com/7095059/diff/12001/doc/draft/death-and-destruc...
doc/draft/death-and-destruction.txt:100: * The unit's agent terminates itself
and refuses to run again.
Same as before, this is a follow up action on Dying above.

https://codereview.appspot.com/7095059/diff/12001/doc/draft/death-and-destruc...
doc/draft/death-and-destruction.txt:111: one relation, decrement its relation
count; otherwise remove the service.
"decrement its relation count"?

https://codereview.appspot.com/7095059/diff/12001/doc/draft/death-and-destruc...
doc/draft/death-and-destruction.txt:142: * If the relation has container scope,
and no other Alive relation exists
Shouldn't this be no other Alive *container* relation?

https://codereview.appspot.com/7095059/diff/12001/doc/draft/death-and-destruc...
doc/draft/death-and-destruction.txt:149: guarantees of the relation's -- and
hence the affected services' -- Aliveness:
The end of this sentence is a bit confusing. There's a colon right after a
strong emphasis on service liveness, but the list below is covering the
beginning of the sentence which is actually about the operations to deal with a
Dying relation.

https://codereview.appspot.com/7095059/diff/12001/doc/draft/death-and-destruc...
doc/draft/death-and-destruction.txt:152: "the counterpart service" refers to the
other service in the relation.
Perhaps:

* Decrement the relation count of the service (*) ...

...

(*) Here, "the service" ...

https://codereview.appspot.com/7095059/diff/12001/doc/draft/death-and-destruc...
doc/draft/death-and-destruction.txt:155: service clearly therefore has a unit
count greater than zero).
Ideally we should assert that.

https://codereview.appspot.com/7095059/diff/12001/doc/draft/death-and-destruc...
doc/draft/death-and-destruction.txt:160: counterpart service.
Impressive coverage of all the steps. Thank you!

https://codereview.appspot.com/7095059/diff/12001/doc/draft/death-and-destruc...
doc/draft/death-and-destruction.txt:170: * If it's already Dying, skip it.
s/it's/the relation is/; (slightly ambiguous in the context)

https://codereview.appspot.com/7095059/diff/12001/doc/draft/death-and-destruc...
doc/draft/death-and-destruction.txt:171: * If it's Alive, destroy the relation
without modifying the service's
Same

https://codereview.appspot.com/7095059/diff/12001/doc/draft/death-and-destruc...
doc/draft/death-and-destruction.txt:176: the service to Dying and decrement its
relation count by the value of
I got a bit lost in the last two points. There's some magic happening around
that counter that I'm unable to grasp at that time of the day, and would
appreciate some live conversation on it.

https://codereview.appspot.com/7095059/diff/12001/doc/draft/death-and-destruc...
doc/draft/death-and-destruction.txt:183: Dying.
..., and moves on with the destruction procedure. ?

[dave_cheney.net, 2013-01-16 23:30:00]

https://codereview.appspot.com/7095059/diff/6001/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/7095059/diff/6001/state/relation.go#newcode112
state/relation.go:112: ops, _, err := rel.destroyOps("")
Wow. My god that is fucking confusing.

The value of err in the inner scope is copied into the memory location
referenced by the err in the outer scope before the defer runs.

My apologies for not understanding this. http://play.golang.org/p/VvuW_OuCKe.

You'd best ignore the rest of my comments.

[rog, 2013-01-17 07:56:06]

On 16 January 2013 23:30,  <dave@cheney.net> wrote:
>
> https://codereview.appspot.com/7095059/diff/6001/state/relation.go
> File state/relation.go (right):
>
> https://codereview.appspot.com/7095059/diff/6001/state/relation.go#newcode112
> state/relation.go:112: ops, _, err := rel.destroyOps("")
> Wow. My god that is fucking confusing.
>
> The value of err in the inner scope is copied into the memory location
> referenced by the err in the outer scope before the defer runs.

it's not too confusing really, honest! a return parameter always
refers to the value returned.

[dave_cheney.net, 2013-01-17 08:03:48]

That both blew and imploded my brain at the same time.  But now I understand
On 17 Jan 2013 18:56, "roger peppe" <rogpeppe@gmail.com> wrote:

> On 16 January 2013 23:30,  <dave@cheney.net> wrote:
> >
> > https://codereview.appspot.com/7095059/diff/6001/state/relation.go
> > File state/relation.go (right):
> >
> >
> https://codereview.appspot.com/7095059/diff/6001/state/relation.go#newcode112
> > state/relation.go:112: ops, _, err := rel.destroyOps("")
> > Wow. My god that is fucking confusing.
> >
> > The value of err in the inner scope is copied into the memory location
> > referenced by the err in the outer scope before the defer runs.
>
> it's not too confusing really, honest! a return parameter always
> refers to the value returned.
>

[fwereade, 2013-01-17 09:12:20]

Please take a look.

https://codereview.appspot.com/7095059/diff/6001/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/7095059/diff/6001/state/relation.go#newcode112
state/relation.go:112: ops, _, err := rel.destroyOps("")
On 2013/01/16 23:30:01, dfc wrote:
> Wow. My god that is fucking confusing.
> 
> The value of err in the inner scope is copied into the memory location
> referenced by the err in the outer scope before the defer runs.
> 
> My apologies for not understanding this. http://play.golang.org/p/VvuW_OuCKe.
> 
> You'd best ignore the rest of my comments.

No worries, thanks :).

https://codereview.appspot.com/7095059/diff/12001/doc/draft/death-and-destruc...
File doc/draft/death-and-destruction.txt (right):

https://codereview.appspot.com/7095059/diff/12001/doc/draft/death-and-destruc...
doc/draft/death-and-destruction.txt:13: * Entities might be removed immediately
they are destroyed, but this is not
On 2013/01/16 23:15:48, niemeyer wrote:
> I think there's an adverb missing after immediately, such as "after", or
"when",
> or "at the time".

"Immediately" is a perfectly good conjunction on its own; I believe that it is
correct, if slightly formal/old-fashioned, in this context. IMO inserting extra
words will weaken the impact and muddy the meaning of the sentence.

https://codereview.appspot.com/7095059/diff/12001/doc/draft/death-and-destruc...
doc/draft/death-and-destruction.txt:15: * If an entity is not removed
immediately it is destroyed, its eventual
On 2013/01/16 23:15:48, niemeyer wrote:
> Same.

Ditto.

https://codereview.appspot.com/7095059/diff/12001/doc/draft/death-and-destruc...
doc/draft/death-and-destruction.txt:61: * If the machine has JobManageEnviron,
or has any assigned units, abort with
On 2013/01/16 23:15:48, niemeyer wrote:
> We might say s/has/is the last machine with/ here, in anticipation of intended
> semantics. Won't make a difference in the short term, though.

SGTM.

https://codereview.appspot.com/7095059/diff/12001/doc/draft/death-and-destruc...
doc/draft/death-and-destruction.txt:71: * The machine's agent terminates itself
and refuses to run again.
On 2013/01/16 23:15:48, niemeyer wrote:
> That's actually a follow up on the actions of Dying, rather than the machine
> agent observing the Dead change.

Disagree. The agent won't terminate until *after* the machine is Dead; and nor
will it refuse to run until after that point. Observation has nothing to do with
it, and the fact that Dying must have been set at some point in the past is a
red herring... I think ;).

https://codereview.appspot.com/7095059/diff/12001/doc/draft/death-and-destruc...
doc/draft/death-and-destruction.txt:100: * The unit's agent terminates itself
and refuses to run again.
On 2013/01/16 23:15:48, niemeyer wrote:
> Same as before, this is a follow up action on Dying above.

Ditto.

https://codereview.appspot.com/7095059/diff/12001/doc/draft/death-and-destruc...
doc/draft/death-and-destruction.txt:111: one relation, decrement its relation
count; otherwise remove the service.
On 2013/01/16 23:15:48, niemeyer wrote:
> "decrement its relation count"?
Sorry, crack, thank you.

https://codereview.appspot.com/7095059/diff/12001/doc/draft/death-and-destruc...
doc/draft/death-and-destruction.txt:142: * If the relation has container scope,
and no other Alive relation exists
On 2013/01/16 23:15:48, niemeyer wrote:
> Shouldn't this be no other Alive *container* relation?

Good catch. Thanks.

https://codereview.appspot.com/7095059/diff/12001/doc/draft/death-and-destruc...
doc/draft/death-and-destruction.txt:149: guarantees of the relation's -- and
hence the affected services' -- Aliveness:
On 2013/01/16 23:15:48, niemeyer wrote:
> The end of this sentence is a bit confusing. There's a colon right after a
> strong emphasis on service liveness, but the list below is covering the
> beginning of the sentence which is actually about the operations to deal with
a
> Dying relation.

Thanks, I'll reword it.

https://codereview.appspot.com/7095059/diff/12001/doc/draft/death-and-destruc...
doc/draft/death-and-destruction.txt:152: "the counterpart service" refers to the
other service in the relation.
On 2013/01/16 23:15:48, niemeyer wrote:
> Perhaps:
> 
> * Decrement the relation count of the service (*) ...
> 
> ...
> 
> (*) Here, "the service" ...

Yeah, probably better. Thanks.

https://codereview.appspot.com/7095059/diff/12001/doc/draft/death-and-destruc...
doc/draft/death-and-destruction.txt:155: service clearly therefore has a unit
count greater than zero).
On 2013/01/16 23:15:48, niemeyer wrote:
> Ideally we should assert that.

OK, sounds good -- even if we can't cleanly handle the failure, we can at least
prevent a bad txn from running.

https://codereview.appspot.com/7095059/diff/12001/doc/draft/death-and-destruc...
doc/draft/death-and-destruction.txt:170: * If it's already Dying, skip it.
On 2013/01/16 23:15:48, niemeyer wrote:
> s/it's/the relation is/; (slightly ambiguous in the context)

Cheers.

https://codereview.appspot.com/7095059/diff/12001/doc/draft/death-and-destruc...
doc/draft/death-and-destruction.txt:171: * If it's Alive, destroy the relation
without modifying the service's
On 2013/01/16 23:15:48, niemeyer wrote:
> Same
Ditto.

https://codereview.appspot.com/7095059/diff/12001/doc/draft/death-and-destruc...
doc/draft/death-and-destruction.txt:176: the service to Dying and decrement its
relation count by the value of
On 2013/01/16 23:15:48, niemeyer wrote:
> I got a bit lost in the last two points. There's some magic happening around
> that counter that I'm unable to grasp at that time of the day, and would
> appreciate some live conversation on it.

I'll try to reword this -- it's actually pretty simple but apparently not well
expressed.

https://codereview.appspot.com/7095059/diff/12001/doc/draft/death-and-destruc...
doc/draft/death-and-destruction.txt:183: Dying.
On 2013/01/16 23:15:48, niemeyer wrote:
> ..., and moves on with the destruction procedure. ?

Maybe just "observes the change and destroys its unit."?

https://codereview.appspot.com/7095059/diff/12001/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/7095059/diff/12001/state/relation.go#newcode160
state/relation.go:160: type removeMode string
On 2013/01/16 16:17:58, rog wrote:
> any particular reason this isn't int, with an iota const below? it seems only
to
> be used internally.

Pre-emptive assumption that one day someone will want to log something around
here, and that strings are kinder than ints. Is there any particular reason it
should be int?

https://codereview.appspot.com/7095059/diff/12001/state/relation.go#newcode203
state/relation.go:203: // service identified by the supplied endpoint, as
determined by mode and
On 2013/01/16 16:17:58, rog wrote:
> "to maintain the service" ?

I think it's important to make clear the distinction between the service we're
affecting (ided by the endpoint) and the service that's attached to the mode (ie
serviceName). So I don't think it's a good idea to shorten the comment.

If you're saying "maintain" is a bad name, I'm not sure: this method returns an
[]txn.Op that might or might not affect ep's service in a few different possible
ways. Can you think of a verb that encapsulates that better?

https://codereview.appspot.com/7095059/diff/12001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/7095059/diff/12001/state/service.go#newcode111
state/service.go:111: removeCount += 1
On 2013/01/16 16:17:58, rog wrote:
> removeCount++

SGTM

https://codereview.appspot.com/7095059/diff/12001/state/service_test.go
File state/service_test.go (right):

https://codereview.appspot.com/7095059/diff/12001/state/service_test.go#newco...
state/service_test.go:209: // TODO(fwereade): maybe service destruction should
always unexpose?
On 2013/01/16 16:17:58, rog wrote:
> +1

Holding out for more sanity-checks/support; if we agree on this, I can trivially
work the change into this CL.

[rog, 2013-01-17 10:28:07]

LGTM assuming the endpointRemoveOps comment is fixed.

https://codereview.appspot.com/7095059/diff/12001/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/7095059/diff/12001/state/relation.go#newcode160
state/relation.go:160: type removeMode string
On 2013/01/17 09:12:21, fwereade wrote:
> On 2013/01/16 16:17:58, rog wrote:
> > any particular reason this isn't int, with an iota const below? it seems
only
> to
> > be used internally.
> 
> Pre-emptive assumption that one day someone will want to log something around
> here, and that strings are kinder than ints. Is there any particular reason it
> should be int?

i have a gut feeling ints are conceptually cleaner, but that's probably just my
C background speaking. it's ok really.
*hits self with hammer*.

https://codereview.appspot.com/7095059/diff/12001/state/relation.go#newcode203
state/relation.go:203: // service identified by the supplied endpoint, as
determined by mode and
On 2013/01/17 09:12:21, fwereade wrote:
> On 2013/01/16 16:17:58, rog wrote:
> > "to maintain the service" ?
> 
> I think it's important to make clear the distinction between the service we're
> affecting (ided by the endpoint) and the service that's attached to the mode
(ie
> serviceName). So I don't think it's a good idea to shorten the comment.
> 
> If you're saying "maintain" is a bad name, I'm not sure: this method returns
an
> []txn.Op that might or might not affect ep's service in a few different
possible
> ways. Can you think of a verb that encapsulates that better?

as discussed online, "operations that are needed to maintain the service"
doesn't say anything about what the operations are actually doing.

[niemeyer, 2013-01-17 13:06:05]

Finding the interaction on these op-returning functions a bit tricky to follow.
Some suggestions:

https://codereview.appspot.com/7095059/diff/6001/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/7095059/diff/6001/state/relation.go#newcode103
state/relation.go:103: r.doc.Life = Dying
Looks rather like a plain and dangerous lie. If Life is Dead, it will transition
*back* onto Dying, apparently.

https://codereview.appspot.com/7095059/diff/6001/state/relation.go#newcode135
state/relation.go:135: func (r *Relation) destroyOps(destroyingService string)
([]txn.Op, bool, error) {
The middle bool is unnamed, undocumented, and apparently unused.

https://codereview.appspot.com/7095059/diff/6001/state/relation.go#newcode202
state/relation.go:202: // endpointRemoveOps returns the operations that are
needed to maintain the
I don't fully understand what's documented here. endpointRemoveOps "maintains
the service"? "as determined by mode and serviceName"? What do they determine?
That's not really saying much about what this is doing or why.

https://codereview.appspot.com/7095059/diff/6001/state/relation.go#newcode210
state/relation.go:210: if name == serviceName {
This is where I realized I was confused. There's an assumption across the whole
change that serviceName is a parameter for mode, but that's not obvious nor
documented.

Going back and looking through the use of mode, I think it'd be more clear to
use this in all of these cases instead:

    func f(leavingScope bool, destroyService string)

and get rid of mode. In the specific case here, though, this check seems
misplaced. If we don't want to change the endpoint when we're removing the
endpoint, we shouldn't call a method whose goal is to change the endpoint.
That's the only reason why this is being called with a serviceName and mode.

https://codereview.appspot.com/7095059/diff/6001/state/relation.go#newcode215
state/relation.go:215: if name != serviceName {
When will we have a serviceName on modeLeaveScope? Seems like another hint of
the previous points.

[fwereade, 2013-01-17 14:18:20]

removeMode and endpointRemoveOps are clearly the contentious part. I think the
code became clearer in a more recent changeset, and I think some of your
concerns may be addressed with the doc proposed below. Thoughts?

https://codereview.appspot.com/7095059/diff/6001/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/7095059/diff/6001/state/relation.go#newcode103
state/relation.go:103: r.doc.Life = Dying
On 2013/01/17 13:06:05, niemeyer wrote:
> Looks rather like a plain and dangerous lie. If Life is Dead, it will
transition
> *back* onto Dying, apparently.

.doc.Life will never be Dead. The worst that can happen, AFAICT, is that a
relation that's really removed will be set to Dying locally -- so the local view
will be a little bit out of date, as all clients can and must always expect. Am
I off base here?

https://codereview.appspot.com/7095059/diff/6001/state/relation.go#newcode135
state/relation.go:135: func (r *Relation) destroyOps(destroyingService string)
([]txn.Op, bool, error) {
On 2013/01/17 13:06:05, niemeyer wrote:
> The middle bool is unnamed, undocumented, and apparently unused.

// destroyOps returns the operations necessary to destroy the relation, and
// whether those operations will lead to the relation's removal.

It's used by Service.Destroy (or destroyOps or something). Would it be clearer
with "the relation's immediate removal"?

https://codereview.appspot.com/7095059/diff/6001/state/relation.go#newcode202
state/relation.go:202: // endpointRemoveOps returns the operations that are
needed to maintain the
On 2013/01/17 13:06:05, niemeyer wrote:
> I don't fully understand what's documented here. endpointRemoveOps "maintains
> the service"? "as determined by mode and serviceName"? What do they determine?
> That's not really saying much about what this is doing or why.

How about:

// endpointRemoveOps returns the operations that are needed
// to maintain global consistency with respect to the
// service identified by the supplied endpoint during the
// relation's removal. The appropriate operations vary
// according to the reason for the removal, which is
// identified by mode; serviceName is used in
// modeDestroyService (to identify the service being
// destroyed) and modeLeaveScope (to identify the service
// of the unit that is leaving scope).

[fwereade, 2013-01-17 17:16:03]

Please take a look.

[niemeyer, 2013-01-17 17:29:05]

On service.go:

https://codereview.appspot.com/7095059/diff/8002/state/service.go
File state/service.go (right):

https://codereview.appspot.com/7095059/diff/8002/state/service.go#newcode56
state/service.go:56: // service has any units in scope, they will all be removed
immediately.
s/will all be/are all/?

https://codereview.appspot.com/7095059/diff/8002/state/service.go#newcode99
state/service.go:99: if len(rels) != s.doc.RelationCount {
// This is just an early bail out. The relations obtained may still
// be wrong, and that'll be prevented by the txn-revno assert below.

https://codereview.appspot.com/7095059/diff/8002/state/service.go#newcode114
state/service.go:114: asserts := D{{"life", Alive}, {"txn-revno",
s.doc.TxnRevno}}
// Relations that aren't Alive won't have been removed because
// they're already on their way to death by other means.
// If that's the case, do not remove the service either,
// and let it be dealt with together with the dying relations.

https://codereview.appspot.com/7095059/diff/8002/state/service.go#newcode131
state/service.go:131: // removeOps returns the oprations required to remove the
service. Supplied
s/op/ope/, although I'm almost tempted to start using "opration".. sounds great.
:-)

https://codereview.appspot.com/7095059/diff/8002/state/service.go#newcode132
state/service.go:132: // asserts will be applied to the service document.
s/applied to.*/included in the operation on the service document./

https://codereview.appspot.com/7095059/diff/8002/state/service.go#newcode280
state/service.go:280: return "", notFound("service %q", s)
This will have to be changed before merging, as Roger renamed it to notFoundf.

https://codereview.appspot.com/7095059/diff/8002/state/service.go#newcode422
state/service.go:422: return append(ops, s.removeOps(nil)...)
Shouldn't this assert the txn-revno? It might change before we get there and get
new units or relations.

[niemeyer, 2013-01-17 18:14:42]

And everything else has been reviewed. Just one minor on tests.

I'll just wait for the changes to service.go before a final +1.

https://codereview.appspot.com/7095059/diff/20001/state/service_test.go
File state/service_test.go (right):

https://codereview.appspot.com/7095059/diff/20001/state/service_test.go#newco...
state/service_test.go:425: // Recreate service and relation.
The fact we're *recreating* makes it sound like the stuff above is actually an
independent test.

[fwereade, 2013-01-17 18:47:16]

Please take a look.

https://codereview.appspot.com/7095059/diff/8002/state/service.go
File state/service.go (right):

https://codereview.appspot.com/7095059/diff/8002/state/service.go#newcode56
state/service.go:56: // service has any units in scope, they will all be removed
immediately.
On 2013/01/17 17:29:06, niemeyer wrote:
> s/will all be/are all/?

Done.

https://codereview.appspot.com/7095059/diff/8002/state/service.go#newcode131
state/service.go:131: // removeOps returns the oprations required to remove the
service. Supplied
On 2013/01/17 17:29:06, niemeyer wrote:
> s/op/ope/, although I'm almost tempted to start using "opration".. sounds
great.
> :-)

Done.

https://codereview.appspot.com/7095059/diff/8002/state/service.go#newcode132
state/service.go:132: // asserts will be applied to the service document.
On 2013/01/17 17:29:06, niemeyer wrote:
> s/applied to.*/included in the operation on the service document./

Done.

https://codereview.appspot.com/7095059/diff/8002/state/service.go#newcode280
state/service.go:280: return "", notFound("service %q", s)
On 2013/01/17 17:29:06, niemeyer wrote:
> This will have to be changed before merging, as Roger renamed it to notFoundf.

Done.

https://codereview.appspot.com/7095059/diff/8002/state/service.go#newcode422
state/service.go:422: return append(ops, s.removeOps(nil)...)
On 2013/01/17 17:29:06, niemeyer wrote:
> Shouldn't this assert the txn-revno? It might change before we get there and
get
> new units or relations.

life/unitcount/relationcount; but, yes it should, thanks. Done.

https://codereview.appspot.com/7095059/diff/20001/state/service_test.go
File state/service_test.go (right):

https://codereview.appspot.com/7095059/diff/20001/state/service_test.go#newco...
state/service_test.go:425: // Recreate service and relation.
On 2013/01/17 18:14:42, niemeyer wrote:
> The fact we're *recreating* makes it sound like the stuff above is actually an
> independent test.

Done.

[fwereade, 2013-01-17 19:42:14]

Please take a look.

[niemeyer, 2013-01-17 19:46:21]

LGTM, thanks for the great work.

[fwereade, 2013-01-17 19:57:59]

*** Submitted:

state: service API changes

Removed:

  * Service.EnsureDying
  * Service.EnsureDead
  * State.RemoveService

Added:

  * Service.Destroy

Service.Destroy will remove the service and all its relations, if possible;
any that cannot be will be set to Dying. Service removal is now handled via
Service.RemoveUnit (which should become Unit.Remove, but this CL is big
enough already) and RelationUnit.LeaveScope, each of which will remove any
Dying service to which they hold the last reference.

In addition to the motivating lp:1089285 (service API), this inevitably
addresses lp:1089284 (service removal on LeaveScope) and lp:1100070 (unit
removal failure).

Reviewers unfamiliar with the details of the existing operations are likely
to be best served by reading doc/draft/death-and-destruction.txt, and to
compare the implementation against the model described therein, rather than
to worry *too* much about the previous implementation.

R=dfc, rog, niemeyer
CC=
https://codereview.appspot.com/7095059