state: add Life to state.Environment

state: add Life to state.Environment

Environment has been given new methods
to mark an environment as dying, and
to remove the environment from state.
An environment must now exist in state
and be Alive for services or machines
to be added to state.

A secondary change here is to use the
environment's UUID in the tag, rather
than the environment's name, which is
non-unique. We continue to accept the
name as input, until we know all users
are passing in UUIDs.

https://code.launchpad.net/~axwalk/juju-core/state-environment-life/+merge/195710

(do not edit description out of merge proposal)

[axw, 2013-11-19 06:47:52]

Please take a look.

[axw, 2013-11-19 07:00:22]

On 2013/11/19 06:47:52, axw wrote:
> Please take a look.

I should have mentioned in the description: this is an offshoot of
https://codereview.appspot.com/22870045/

The purpose of this CL is twofold:
 - to prevent machines and services from being added while an environment is
being destroyed
 - and to provide a clean-termination mechanism for state-server machine agents

[mue, 2013-11-20 15:24:18]

LGTM, only a real minor comment.

https://codereview.appspot.com/28880043/diff/1/names/environ.go
File names/environ.go (right):

https://codereview.appspot.com/28880043/diff/1/names/environ.go#newcode15
names/environ.go:15: // IsEnvironment returns whether id is a valid environment
id.
id (in the comment) or name (in the code)?

[axw, 2013-11-21 03:15:08]

Please take a look.

https://codereview.appspot.com/28880043/diff/1/names/environ.go
File names/environ.go (right):

https://codereview.appspot.com/28880043/diff/1/names/environ.go#newcode15
names/environ.go:15: // IsEnvironment returns whether id is a valid environment
id.
On 2013/11/20 15:24:19, mue wrote:
> id (in the comment) or name (in the code)?

id, but actually, we can do that TODO now since the id is now a UUID with a well
defined format. Updated to do that, and updated tests.

[fwereade, 2013-11-21 13:04:18]

Needs a bit more thought, I'm afraid.

https://codereview.appspot.com/28880043/diff/20001/names/environ.go
File names/environ.go (right):

https://codereview.appspot.com/28880043/diff/20001/names/environ.go#newcode16
names/environ.go:16: func IsEnvironment(id string) bool {
Have we checked all possible clients of this? I'm pretty sure we don't accept
environment tags ever, but we do produce them, and the GUI may need to be
prepared for this change -- please check with frankban.

https://codereview.appspot.com/28880043/diff/20001/state/environ.go
File state/environ.go (right):

https://codereview.appspot.com/28880043/diff/20001/state/environ.go#newcode85
state/environ.go:85: Assert: notDeadDoc,
If it's already dying we should abort, too, so this is just the assertAliveOp

https://codereview.appspot.com/28880043/diff/20001/state/environ.go#newcode96
state/environ.go:96: return err
We can't track the complete set of services transactionally, so I reckon the
next best thing is to add a cleanup job that sets all services to dying one the
environment has been set to dying.

I kinda hate this, but we're stuck with it without a schemachange for a
total-service-refcount somewhere -- so please tag the code with SCHEMACHANGE so
we have a chance of fixing it.

https://codereview.appspot.com/28880043/diff/20001/state/environ.go#newcode108
state/environ.go:108: return onAbort(e.st.runTransaction([]txn.Op{op}), nil)
I'm not sure how meaningful this code is at this point -- *everything* has an
implicit environment reference at the moment, so I'm not when this can be sanely
called. Ofc, something like this is a necessary component of a multi-tenant
server, in the future -- but it will certainly require that removing the
environment removes everything that references it. Thoughts? Can we maybe even
just stick with alive/dying for now?

https://codereview.appspot.com/28880043/diff/20001/state/initialize_test.go
File state/initialize_test.go (right):

https://codereview.appspot.com/28880043/diff/20001/state/initialize_test.go#n...
state/initialize_test.go:75: annotations, err := annotator.Annotations()
Find out from the GUI guys exactly what they use environment annotations for, if
anything, and whether the tag change will affect them.

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

https://codereview.appspot.com/28880043/diff/20001/state/state.go#newcode359
state/state.go:359: env, err := st.Environment()
If we're going to pay the DB hit for the environment here, we should probably
check for life and abort here as well.

To avoid duplicating code, you may prefer to structure this as a loop for i < 2.

https://codereview.appspot.com/28880043/diff/20001/state/state.go#newcode616
state/state.go:616: if id != env.UUID() {
It may be that this is the only bit we need to fix -- and we should just accept
that there's only one environment ever in play, so we should just assume that
environment-<anything> refers to the local environment.

That'll work for now, at least.

https://codereview.appspot.com/28880043/diff/20001/state/state.go#newcode735
state/state.go:735: return nil, err
Check for dying before running transaction.

Maybe structure it as a loop, as suggested for machines?

https://codereview.appspot.com/28880043/diff/20001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/28880043/diff/20001/state/state_test.go#newcod...
state/state_test.go:567: }
We'll need explicit testing for the possibility of new services racing with
dying environments. If you do a cleanup job for services, the races won't be a
problem in themselves.

[axw, 2013-11-28 09:42:48]

Please take a look.

https://codereview.appspot.com/28880043/diff/20001/names/environ.go
File names/environ.go (right):

https://codereview.appspot.com/28880043/diff/20001/names/environ.go#newcode16
names/environ.go:16: func IsEnvironment(id string) bool {
On 2013/11/21 13:04:18, fwereade wrote:
> Have we checked all possible clients of this? I'm pretty sure we don't accept
> environment tags ever, but we do produce them, and the GUI may need to be
> prepared for this change -- please check with frankban.

Turns out that Landscape is using it, but only in IS. Have discussed with Bjorn
Tillenius, and he will update Landscape to use UUID with fallback to name for
the next version of Landscape, and we'll make the change in the following
version of Juju. After that is in IS, Landscape can drop the backwards
compatibility.

https://codereview.appspot.com/28880043/diff/20001/state/environ.go
File state/environ.go (right):

https://codereview.appspot.com/28880043/diff/20001/state/environ.go#newcode85
state/environ.go:85: Assert: notDeadDoc,
On 2013/11/21 13:04:18, fwereade wrote:
> If it's already dying we should abort, too, so this is just the assertAliveOp

Why should we abort if Dying? While destroying other entities aborts if they're
dying, they ultimately do not return an error to the caller.

Is there a reason to prefer aborting and then checking if Life==Dying and
returning nil? Is the no-op modification not really a no-op?

I don't have a problem doing it, but I'd rather understand why first.

https://codereview.appspot.com/28880043/diff/20001/state/environ.go#newcode96
state/environ.go:96: return err
On 2013/11/21 13:04:18, fwereade wrote:
> We can't track the complete set of services transactionally, so I reckon the
> next best thing is to add a cleanup job that sets all services to dying one
the
> environment has been set to dying.

Okay, I'll need to add a new cleanup kind then. What about machines? I can't use
the "machine" cleanup as that implies --force, which I don't think we want here
do we?

> I kinda hate this, but we're stuck with it without a schemachange for a
> total-service-refcount somewhere -- so please tag the code with SCHEMACHANGE
so
> we have a chance of fixing it.

Done.

https://codereview.appspot.com/28880043/diff/20001/state/environ.go#newcode108
state/environ.go:108: return onAbort(e.st.runTransaction([]txn.Op{op}), nil)
On 2013/11/21 13:04:18, fwereade wrote:
> I'm not sure how meaningful this code is at this point -- *everything* has an
> implicit environment reference at the moment, so I'm not when this can be
sanely
> called. Ofc, something like this is a necessary component of a multi-tenant
> server, in the future -- but it will certainly require that removing the
> environment removes everything that references it. Thoughts? Can we maybe even
> just stick with alive/dying for now?

Okay, fair enough. The intention was that Remove would trigger state
termination, so there'd be nothing left anyway.

Without it we're back to having no way of signalling manager nodes to terminate.
I will have to go back to my earlier approach of "ssh <host> pkill -SIGABRT
jujud" in the manual provider, and all others will be destroyed via the provider
API. On reflection, it's probably more sensible anyway; it keeps manual and
cloud better aligned.

https://codereview.appspot.com/28880043/diff/20001/state/initialize_test.go
File state/initialize_test.go (right):

https://codereview.appspot.com/28880043/diff/20001/state/initialize_test.go#n...
state/initialize_test.go:75: annotations, err := annotator.Annotations()
On 2013/11/21 13:04:18, fwereade wrote:
> Find out from the GUI guys exactly what they use environment annotations for,
if
> anything, and whether the tag change will affect them.

Done.

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

https://codereview.appspot.com/28880043/diff/20001/state/state.go#newcode359
state/state.go:359: env, err := st.Environment()
On 2013/11/21 13:04:18, fwereade wrote:
> If we're going to pay the DB hit for the environment here, we should probably
> check for life and abort here as well.

Done.

> To avoid duplicating code, you may prefer to structure this as a loop for i <
2.

Not practical here, since the transaction is actually run outside this function.

https://codereview.appspot.com/28880043/diff/20001/state/state.go#newcode616
state/state.go:616: if id != env.UUID() {
On 2013/11/21 13:04:18, fwereade wrote:
> It may be that this is the only bit we need to fix -- and we should just
accept
> that there's only one environment ever in play, so we should just assume that
> environment-<anything> refers to the local environment.
> 
> That'll work for now, at least.

It's not: we also check the tag value looks like a UUID. We'll be fine once
Launchpad is updated.

https://codereview.appspot.com/28880043/diff/20001/state/state.go#newcode735
state/state.go:735: return nil, err
On 2013/11/21 13:04:18, fwereade wrote:
> Check for dying before running transaction.

Done.

> Maybe structure it as a loop, as suggested for machines?

IMHO, much simpler without the loop. I don't think it's worth it for the sake of
not duplicating two lines. I'll bow if you feel strongly about it.

https://codereview.appspot.com/28880043/diff/20001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/28880043/diff/20001/state/state_test.go#newcod...
state/state_test.go:567: }
On 2013/11/21 13:04:18, fwereade wrote:
> We'll need explicit testing for the possibility of new services racing with
> dying environments. If you do a cleanup job for services, the races won't be a
> problem in themselves.

I've added the cleanup, and added a test in cleanup_test.go that ensures that a
cleanup is scheduled to destroy preexisting services.

[fwereade, 2013-12-02 08:58:18]

Nearly there, I think.

I do feel that accepting both env-tag kinds is the best way for us to support
the transition, but maybe I'm confused about something.

https://codereview.appspot.com/28880043/diff/20001/state/environ.go
File state/environ.go (right):

https://codereview.appspot.com/28880043/diff/20001/state/environ.go#newcode85
state/environ.go:85: Assert: notDeadDoc,
On 2013/11/28 09:42:48, axw wrote:
> On 2013/11/21 13:04:18, fwereade wrote:
> > If it's already dying we should abort, too, so this is just the
assertAliveOp
> 
> Why should we abort if Dying? While destroying other entities aborts if
they're
> dying, they ultimately do not return an error to the caller.
> 
> Is there a reason to prefer aborting and then checking if Life==Dying and
> returning nil? Is the no-op modification not really a no-op?
> 
> I don't have a problem doing it, but I'd rather understand why first.

It's just defensive -- if the transaction ever grows such that it's not a no-op,
it'll already be correct.

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

https://codereview.appspot.com/28880043/diff/20001/state/state.go#newcode616
state/state.go:616: if id != env.UUID() {
On 2013/11/28 09:42:48, axw wrote:
> On 2013/11/21 13:04:18, fwereade wrote:
> > It may be that this is the only bit we need to fix -- and we should just
> accept
> > that there's only one environment ever in play, so we should just assume
that
> > environment-<anything> refers to the local environment.
> > 
> > That'll work for now, at least.
> 
> It's not: we also check the tag value looks like a UUID. We'll be fine once
> Launchpad is updated.

For now, though, LP is not updated. Is it not better to accept both forms (ie in
IsEnvironment too), treat any value as "this environment", and target a bug for
some future point at which we can drop the old form?

https://codereview.appspot.com/28880043/diff/20001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/28880043/diff/20001/state/state_test.go#newcod...
state/state_test.go:567: }
On 2013/11/28 09:42:48, axw wrote:
> On 2013/11/21 13:04:18, fwereade wrote:
> > We'll need explicit testing for the possibility of new services racing with
> > dying environments. If you do a cleanup job for services, the races won't be
a
> > problem in themselves.
> 
> I've added the cleanup, and added a test in cleanup_test.go that ensures that
a
> cleanup is scheduled to destroy preexisting services.

The cleanup is good -- it shows that any services that did exist before the env
was destroyed will be cleaned up -- but I think we still need to prevent new
services from being created, lest they be added after the cleanup has run.

https://codereview.appspot.com/28880043/diff/40001/state/environ.go
File state/environ.go (right):

https://codereview.appspot.com/28880043/diff/40001/state/environ.go#newcode84
state/environ.go:84: // not currently have.
I don't think this is quite right -- surely it's always ok to destroy an
environment and take down all the services, and all those other machines? The
real problem is manually-provisioned machines, of whatever kind, which need more
careful cleanup (ie units and containers need to be removed properly).

https://codereview.appspot.com/28880043/diff/40001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/28880043/diff/40001/state/state_test.go#newcod...
state/state_test.go:220: }
This needs a test with transaction-hooks to prove that machine addition is still
correctly blocked if a destroy-env transaction lands after the machine txn was
generated but before it ran.

https://codereview.appspot.com/28880043/diff/40001/state/state_test.go#newcod...
state/state_test.go:555: _, err = s.State.AddService("s1", charm)
ditto

[axw, 2013-12-04 05:28:49]

Please take a look.

https://codereview.appspot.com/28880043/diff/20001/state/environ.go
File state/environ.go (right):

https://codereview.appspot.com/28880043/diff/20001/state/environ.go#newcode85
state/environ.go:85: Assert: notDeadDoc,
On 2013/12/02 08:58:18, fwereade wrote:
> On 2013/11/28 09:42:48, axw wrote:
> > On 2013/11/21 13:04:18, fwereade wrote:
> > > If it's already dying we should abort, too, so this is just the
> assertAliveOp
> > 
> > Why should we abort if Dying? While destroying other entities aborts if
> they're
> > dying, they ultimately do not return an error to the caller.
> > 
> > Is there a reason to prefer aborting and then checking if Life==Dying and
> > returning nil? Is the no-op modification not really a no-op?
> > 
> > I don't have a problem doing it, but I'd rather understand why first.
> 
> It's just defensive -- if the transaction ever grows such that it's not a
no-op,
> it'll already be correct.

Fair enough, thanks for explaining. Done.

https://codereview.appspot.com/28880043/diff/20001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/28880043/diff/20001/state/state_test.go#newcod...
state/state_test.go:567: }
On 2013/12/02 08:58:18, fwereade wrote:
> On 2013/11/28 09:42:48, axw wrote:
> > On 2013/11/21 13:04:18, fwereade wrote:
> > > We'll need explicit testing for the possibility of new services racing
with
> > > dying environments. If you do a cleanup job for services, the races won't
be
> a
> > > problem in themselves.
> > 
> > I've added the cleanup, and added a test in cleanup_test.go that ensures
that
> a
> > cleanup is scheduled to destroy preexisting services.
> 
> The cleanup is good -- it shows that any services that did exist before the
env
> was destroyed will be cleaned up -- but I think we still need to prevent new
> services from being created, lest they be added after the cleanup has run.

This is handled with the "env.assertAliveOp" assertion in add-service (and
add-machine) transactions. Am I missing something?

https://codereview.appspot.com/28880043/diff/40001/state/environ.go
File state/environ.go (right):

https://codereview.appspot.com/28880043/diff/40001/state/environ.go#newcode84
state/environ.go:84: // not currently have.
On 2013/12/02 08:58:18, fwereade wrote:
> I don't think this is quite right -- surely it's always ok to destroy an
> environment and take down all the services, and all those other machines? The
> real problem is manually-provisioned machines, of whatever kind, which need
more
> careful cleanup (ie units and containers need to be removed properly).

After discussion on IRC, I'm going to remove this comment. I think the way we're
doing it is right. Destroy environment prevents new machines and services;
schedule cleanups for services to destroy the existing ones; machines are
destroyed via provider interface after destroy-environment API returns
successfully.

Manual is the exception, but for that we can just prevent destroy-environment
from completing while there are manual non-manager machines in state, as
discussed previously. We could, potentially, destroy them later using the same
mechanism as I intend to use for manager machines (ssh pkill).

I think there will come a time when we want to add cleanups the machines -
namely for JAAS. But I think that can wait.

https://codereview.appspot.com/28880043/diff/40001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/28880043/diff/40001/state/state_test.go#newcod...
state/state_test.go:220: }
On 2013/12/02 08:58:18, fwereade wrote:
> This needs a test with transaction-hooks to prove that machine addition is
still
> correctly blocked if a destroy-env transaction lands after the machine txn was
> generated but before it ran.

Done.

https://codereview.appspot.com/28880043/diff/40001/state/state_test.go#newcod...
state/state_test.go:555: _, err = s.State.AddService("s1", charm)
On 2013/12/02 08:58:18, fwereade wrote:
> ditto

Done.

[axw, 2013-12-04 06:45:10]

https://codereview.appspot.com/28880043/diff/20001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/28880043/diff/20001/state/state_test.go#newcod...
state/state_test.go:567: }
On 2013/12/04 05:28:49, axw wrote:
> On 2013/12/02 08:58:18, fwereade wrote:
> > On 2013/11/28 09:42:48, axw wrote:
> > > On 2013/11/21 13:04:18, fwereade wrote:
> > > > We'll need explicit testing for the possibility of new services racing
> with
> > > > dying environments. If you do a cleanup job for services, the races
won't
> be
> > a
> > > > problem in themselves.
> > > 
> > > I've added the cleanup, and added a test in cleanup_test.go that ensures
> that
> > a
> > > cleanup is scheduled to destroy preexisting services.
> > 
> > The cleanup is good -- it shows that any services that did exist before the
> env
> > was destroyed will be cleaned up -- but I think we still need to prevent new
> > services from being created, lest they be added after the cleanup has run.
> 
> This is handled with the "env.assertAliveOp" assertion in add-service (and
> add-machine) transactions. Am I missing something?

Ignore this comment, this was pre-discussion and I forgot to delete it.

[fwereade, 2013-12-04 14:03:23]

Last few trivials. Thanks for bearing with me on this.

https://codereview.appspot.com/28880043/diff/60001/state/cleanup.go
File state/cleanup.go (right):

https://codereview.appspot.com/28880043/diff/60001/state/cleanup.go#newcode108
state/cleanup.go:108: for iter.Next(&service.doc) {
Hmm, I do worry a bit about annotations. I guess it's probably not that
important here.

https://codereview.appspot.com/28880043/diff/60001/state/environ.go
File state/environ.go (right):

https://codereview.appspot.com/28880043/diff/60001/state/environ.go#newcode69
state/environ.go:69: err := e.st.environments.Find(nil).One(&e.doc)
FindId(envGlobalKey)? I'm sure there's a constant alone those lines somewhere.

https://codereview.appspot.com/28880043/diff/60001/state/environ.go#newcode79
state/environ.go:79: if e.Life() == Dying {
!= Alive?

https://codereview.appspot.com/28880043/diff/60001/state/environ.go#newcode85
state/environ.go:85: // of which prevent a successful destroy-environment.
This isn't enforced in state, just at the API level; maybe make that clearer?

Hopefully soon we'll have this stuff behind the API and be able to maintain it
properly...

https://codereview.appspot.com/28880043/diff/60001/state/environ.go#newcode105
state/environ.go:105: panic(err)
Please, no panics in the API server. And it's not an error to destroy something
that's already dead -- given that this is currently the only fail case, I think
we're ok just returning nil.

If you're worried about the inconsistency compared to what I said about the
transaction list I wouldn't object to an explicit check -- in the case where we
can't figure out what went wrong we usually go with ErrExcessiveContention (even
though in practice it *usually* means ErrSomeoneMessedUpTheCode).

https://codereview.appspot.com/28880043/diff/60001/state/initialize_test.go
File state/initialize_test.go (left):

https://codereview.appspot.com/28880043/diff/60001/state/initialize_test.go#o...
state/initialize_test.go:69: env0, err := s.State.FindEntity("environment-" +
cfg.Name())
Shouldn't this still work, until we know no clients are still using it?

https://codereview.appspot.com/28880043/diff/60001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/28880043/diff/60001/state/state.go#newcode785
state/state.go:785: if err := env.Refresh(); err != nil {
Generally we go with "X is no longer alive", IIRC -- because then you get a
consistent message for a NotFound. Or at least it's *usually* consistent, and in
this case I think it always is, because to have made a state connection implies
the existence of this environment at some point in the past.

Probably worth using that message for all the "is being destroyed"s in this
CL...

[axw, 2013-12-06 08:37:39]

Please take a look.

https://codereview.appspot.com/28880043/diff/60001/state/cleanup.go
File state/cleanup.go (right):

https://codereview.appspot.com/28880043/diff/60001/state/cleanup.go#newcode108
state/cleanup.go:108: for iter.Next(&service.doc) {
On 2013/12/04 14:03:23, fwereade wrote:
> Hmm, I do worry a bit about annotations. I guess it's probably not that
> important here.

Sorry, what about annotations?

https://codereview.appspot.com/28880043/diff/60001/state/environ.go
File state/environ.go (right):

https://codereview.appspot.com/28880043/diff/60001/state/environ.go#newcode69
state/environ.go:69: err := e.st.environments.Find(nil).One(&e.doc)
On 2013/12/04 14:03:23, fwereade wrote:
> FindId(envGlobalKey)? I'm sure there's a constant alone those lines somewhere.

Updated to use FindId(UUID). State.Environment() still needs to use Find(nil),
since it doesn't know the UUID.

https://codereview.appspot.com/28880043/diff/60001/state/environ.go#newcode79
state/environ.go:79: if e.Life() == Dying {
On 2013/12/04 14:03:23, fwereade wrote:
> != Alive?

Done.

https://codereview.appspot.com/28880043/diff/60001/state/environ.go#newcode85
state/environ.go:85: // of which prevent a successful destroy-environment.
On 2013/12/04 14:03:23, fwereade wrote:
> This isn't enforced in state, just at the API level; maybe make that clearer?
> 
> Hopefully soon we'll have this stuff behind the API and be able to maintain it
> properly...

Done.

https://codereview.appspot.com/28880043/diff/60001/state/environ.go#newcode105
state/environ.go:105: panic(err)
On 2013/12/04 14:03:23, fwereade wrote:
> Please, no panics in the API server. And it's not an error to destroy
something
> that's already dead -- given that this is currently the only fail case, I
think
> we're ok just returning nil.
> 
> If you're worried about the inconsistency compared to what I said about the
> transaction list I wouldn't object to an explicit check -- in the case where
we
> can't figure out what went wrong we usually go with ErrExcessiveContention
(even
> though in practice it *usually* means ErrSomeoneMessedUpTheCode).

Fixed. Sorry, I was under the mistaken impression that the other Destroy methods
returned an error if Destroy was called on something that's dead. I appear to be
mistaken.

https://codereview.appspot.com/28880043/diff/60001/state/initialize_test.go
File state/initialize_test.go (left):

https://codereview.appspot.com/28880043/diff/60001/state/initialize_test.go#o...
state/initialize_test.go:69: env0, err := s.State.FindEntity("environment-" +
cfg.Name())
On 2013/12/04 14:03:23, fwereade wrote:
> Shouldn't this still work, until we know no clients are still using it?

Yes, it should and does. Backwards compatibility is tested explicitly in
TestFindEntity.

https://codereview.appspot.com/28880043/diff/60001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/28880043/diff/60001/state/state.go#newcode785
state/state.go:785: if err := env.Refresh(); err != nil {
On 2013/12/04 14:03:23, fwereade wrote:
> Generally we go with "X is no longer alive", IIRC -- because then you get a
> consistent message for a NotFound. Or at least it's *usually* consistent, and
in
> this case I think it always is, because to have made a state connection
implies
> the existence of this environment at some point in the past.
> 
> Probably worth using that message for all the "is being destroyed"s in this
> CL...

Done.

[fwereade, 2013-12-09 10:14:40]

Final round of trivials. LGTM with the below if they're clear to you.

https://codereview.appspot.com/28880043/diff/60001/state/cleanup.go
File state/cleanup.go (right):

https://codereview.appspot.com/28880043/diff/60001/state/cleanup.go#newcode108
state/cleanup.go:108: for iter.Next(&service.doc) {
On 2013/12/06 08:37:40, axw wrote:
> On 2013/12/04 14:03:23, fwereade wrote:
> > Hmm, I do worry a bit about annotations. I guess it's probably not that
> > important here.
> 
> Sorry, what about annotations?

The service type -- and a few others -- have annotator fields that we're really
lax about cloning properly because they were added really late and aren't
generally used (except by the API) IIRC. It doesn't matter... until it does...
but I think this is a case where it doesn't because they won't leak out of this
method at all.

It just tripped one of my "watch out for this" flags, but not usefully or
actionably -- I probably just shouldn't have mentioned it.

https://codereview.appspot.com/28880043/diff/60001/state/environ.go
File state/environ.go (right):

https://codereview.appspot.com/28880043/diff/60001/state/environ.go#newcode105
state/environ.go:105: panic(err)
On 2013/12/06 08:37:40, axw wrote:
> On 2013/12/04 14:03:23, fwereade wrote:
> > Please, no panics in the API server. And it's not an error to destroy
> something
> > that's already dead -- given that this is currently the only fail case, I
> think
> > we're ok just returning nil.
> > 
> > If you're worried about the inconsistency compared to what I said about the
> > transaction list I wouldn't object to an explicit check -- in the case where
> we
> > can't figure out what went wrong we usually go with ErrExcessiveContention
> (even
> > though in practice it *usually* means ErrSomeoneMessedUpTheCode).
> 
> Fixed. Sorry, I was under the mistaken impression that the other Destroy
methods
> returned an error if Destroy was called on something that's dead. I appear to
be
> mistaken.

No worries :). The rationale is that racing destroys are not a problem -- both
clients want it dead, and both clients' desires will be satisfied no matter what
order things happen in, so we should be good.

https://codereview.appspot.com/28880043/diff/80001/state/environ.go
File state/environ.go (right):

https://codereview.appspot.com/28880043/diff/80001/state/environ.go#newcode90
state/environ.go:90: // manual machines exist.
This is a race and deserves a bug. Doesn't need to be fixed today though.

https://codereview.appspot.com/28880043/diff/80001/state/environ.go#newcode104
state/environ.go:104: err = e.Refresh()
hmm, but if it's actually *removed* -- in the multi-tenant future, in which it
can be removed -- this'll be an error where it doesn't actually need to be. I'd
be fine with setting `e.doc.Life = Dying` (and returning nil) in the case of
either nil or ErrAborted.

(FWIW, the reason that's not crazy is that setting it to Dying doesn't violate
any expectations at the client end: all the client sees is that the lifecycle
advanced, as expected, and if it happens to then refresh and find it gone that's
still consistent with the model. We couldn't set Dead speculatively, because it
might actually just be Dying, and thereby run the risk of a later refresh moving
the life state back from Dead to Dying: *that* would be a violation of client
expectations.)

https://codereview.appspot.com/28880043/diff/80001/state/environ.go#newcode121
state/environ.go:121: // assertAliveOp returns a read-only txn.Op that asserts
I think "read-only" is slightly ambiguous and doesn't really add value. YMMV,
though, keep it if you feel strongly.

https://codereview.appspot.com/28880043/diff/80001/state/initialize_test.go
File state/initialize_test.go (right):

https://codereview.appspot.com/28880043/diff/80001/state/initialize_test.go#n...
state/initialize_test.go:54: // TODO(axw) remove backwards compatibility for
environment-tag; see lp:1257587
There's a standard format for these:

TODO(person) date bug#
explanation

https://codereview.appspot.com/28880043/diff/80001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/28880043/diff/80001/state/state.go#newcode492
state/state.go:492: return nil, fmt.Errorf("environment is no longer alive")
Again, future-proofing: if we get a NotFound, convert it to "no longer alive".

https://codereview.appspot.com/28880043/diff/80001/state/state.go#newcode631
state/state.go:631: if id != conf.Name() {
Hmm. Should we log a warning instead here, and just return the env anyway? As we
move towards names being aliases, we open up the possibility of multiple clients
having different names for the same environment. That may be a paranoion too far
though... I can't see a path that enables multiple names before we switch to
using UUIDs across the board.

https://codereview.appspot.com/28880043/diff/80001/state/state.go#newcode795
state/state.go:795: return nil, fmt.Errorf("environment is no longer alive")
ditto NotFound handling

[axw, 2013-12-09 13:27:05]

https://codereview.appspot.com/28880043/diff/80001/state/environ.go
File state/environ.go (right):

https://codereview.appspot.com/28880043/diff/80001/state/environ.go#newcode90
state/environ.go:90: // manual machines exist.
On 2013/12/09 10:14:41, fwereade wrote:
> This is a race and deserves a bug. Doesn't need to be fixed today though.

Can you please clarify? What is the race exactly? Sorry if I'm being dense.

https://codereview.appspot.com/28880043/diff/80001/state/environ.go#newcode104
state/environ.go:104: err = e.Refresh()
On 2013/12/09 10:14:41, fwereade wrote:
> hmm, but if it's actually *removed* -- in the multi-tenant future, in which it
> can be removed -- this'll be an error where it doesn't actually need to be.
I'd
> be fine with setting `e.doc.Life = Dying` (and returning nil) in the case of
> either nil or ErrAborted.
> 
> (FWIW, the reason that's not crazy is that setting it to Dying doesn't violate
> any expectations at the client end: all the client sees is that the lifecycle
> advanced, as expected, and if it happens to then refresh and find it gone
that's
> still consistent with the model. We couldn't set Dead speculatively, because
it
> might actually just be Dying, and thereby run the risk of a later refresh
moving
> the life state back from Dead to Dying: *that* would be a violation of client
> expectations.)

Sounds crazy at first blush, but I understand and it sounds reasonable. Will do.

[axw, 2013-12-11 07:35:19]

Please take a look.

https://codereview.appspot.com/28880043/diff/80001/state/environ.go
File state/environ.go (right):

https://codereview.appspot.com/28880043/diff/80001/state/environ.go#newcode104
state/environ.go:104: err = e.Refresh()
On 2013/12/09 13:27:05, axw wrote:
> On 2013/12/09 10:14:41, fwereade wrote:
> > hmm, but if it's actually *removed* -- in the multi-tenant future, in which
it
> > can be removed -- this'll be an error where it doesn't actually need to be.
> I'd
> > be fine with setting `e.doc.Life = Dying` (and returning nil) in the case of
> > either nil or ErrAborted.
> > 
> > (FWIW, the reason that's not crazy is that setting it to Dying doesn't
violate
> > any expectations at the client end: all the client sees is that the
lifecycle
> > advanced, as expected, and if it happens to then refresh and find it gone
> that's
> > still consistent with the model. We couldn't set Dead speculatively, because
> it
> > might actually just be Dying, and thereby run the risk of a later refresh
> moving
> > the life state back from Dead to Dying: *that* would be a violation of
client
> > expectations.)
> 
> Sounds crazy at first blush, but I understand and it sounds reasonable. Will
do.

Done.

https://codereview.appspot.com/28880043/diff/80001/state/environ.go#newcode121
state/environ.go:121: // assertAliveOp returns a read-only txn.Op that asserts
On 2013/12/09 10:14:41, fwereade wrote:
> I think "read-only" is slightly ambiguous and doesn't really add value. YMMV,
> though, keep it if you feel strongly.

Done.

https://codereview.appspot.com/28880043/diff/80001/state/initialize_test.go
File state/initialize_test.go (right):

https://codereview.appspot.com/28880043/diff/80001/state/initialize_test.go#n...
state/initialize_test.go:54: // TODO(axw) remove backwards compatibility for
environment-tag; see lp:1257587
On 2013/12/09 10:14:41, fwereade wrote:
> There's a standard format for these:
> 
> TODO(person) date bug#
> explanation

Done.

https://codereview.appspot.com/28880043/diff/80001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/28880043/diff/80001/state/state.go#newcode492
state/state.go:492: return nil, fmt.Errorf("environment is no longer alive")
On 2013/12/09 10:14:41, fwereade wrote:
> Again, future-proofing: if we get a NotFound, convert it to "no longer alive".

Sorry, I don't see what that would future proof against. What's wrong with just
returning not found when the environment is not found?

https://codereview.appspot.com/28880043/diff/80001/state/state.go#newcode631
state/state.go:631: if id != conf.Name() {
On 2013/12/09 10:14:41, fwereade wrote:
> Hmm. Should we log a warning instead here, and just return the env anyway? As
we
> move towards names being aliases, we open up the possibility of multiple
clients
> having different names for the same environment. That may be a paranoion too
far
> though... I can't see a path that enables multiple names before we switch to
> using UUIDs across the board.

Do we need to support that before we move to UUIDs? I previously had the code
supporting anything, but changed it to check name again. That's what it was
doing before. Can we leave future functionality to the future, or does this need
to be done now?

[fwereade, 2013-12-11 07:46:22]

https://codereview.appspot.com/28880043/diff/80001/state/environ.go
File state/environ.go (right):

https://codereview.appspot.com/28880043/diff/80001/state/environ.go#newcode90
state/environ.go:90: // manual machines exist.
On 2013/12/09 13:27:05, axw wrote:
> On 2013/12/09 10:14:41, fwereade wrote:
> > This is a race and deserves a bug. Doesn't need to be fixed today though.
> 
> Can you please clarify? What is the race exactly? Sorry if I'm being dense.

Client 1 checks no manual machines
Client 2 adds a manual machine
Client 1 destroys environment

Bad Thing.

If you're still around when I come online tomorrow morning, can we chat a little
bit about making machines' providers explicit? At the moment the best we have is
the "manual:" prefix on instance ids, but I don't think that's quite adequate --
it's merely *unlikely* that such an id willcollide with a legitimate one from
another provider.

[axw, 2013-12-11 08:17:21]

On 2013/12/11 07:46:22, fwereade wrote:
> https://codereview.appspot.com/28880043/diff/80001/state/environ.go
> File state/environ.go (right):
> 
> https://codereview.appspot.com/28880043/diff/80001/state/environ.go#newcode90
> state/environ.go:90: // manual machines exist.
> On 2013/12/09 13:27:05, axw wrote:
> > On 2013/12/09 10:14:41, fwereade wrote:
> > > This is a race and deserves a bug. Doesn't need to be fixed today though.
> > 
> > Can you please clarify? What is the race exactly? Sorry if I'm being dense.
> 
> Client 1 checks no manual machines
> Client 2 adds a manual machine
> Client 1 destroys environment
> 
> Bad Thing.

As discussed on IRC: the way I *had* intended to handle that was to first set
Environment to Dying, and then check for manual machines. But, as you point out,
this leaves the environment in an unusable state if it does fail. I'll leave the
race in there (with a TODO), and we can come back to it with a way to
transactionally ensure no manual machines have been added since last check.

> If you're still around when I come online tomorrow morning, can we chat a
little
> bit about making machines' providers explicit? At the moment the best we have
is
> the "manual:" prefix on instance ids, but I don't think that's quite adequate
--
> it's merely *unlikely* that such an id willcollide with a legitimate one from
> another provider.

[axw, 2013-12-11 08:54:30]

Please take a look.

https://codereview.appspot.com/28880043/diff/80001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/28880043/diff/80001/state/state.go#newcode492
state/state.go:492: return nil, fmt.Errorf("environment is no longer alive")
On 2013/12/11 07:35:20, axw wrote:
> On 2013/12/09 10:14:41, fwereade wrote:
> > Again, future-proofing: if we get a NotFound, convert it to "no longer
alive".
> 
> Sorry, I don't see what that would future proof against. What's wrong with
just
> returning not found when the environment is not found?

Done.

https://codereview.appspot.com/28880043/diff/80001/state/state.go#newcode631
state/state.go:631: if id != conf.Name() {
On 2013/12/11 07:35:20, axw wrote:
> On 2013/12/09 10:14:41, fwereade wrote:
> > Hmm. Should we log a warning instead here, and just return the env anyway?
As
> we
> > move towards names being aliases, we open up the possibility of multiple
> clients
> > having different names for the same environment. That may be a paranoion too
> far
> > though... I can't see a path that enables multiple names before we switch to
> > using UUIDs across the board.
> 
> Do we need to support that before we move to UUIDs? I previously had the code
> supporting anything, but changed it to check name again. That's what it was
> doing before. Can we leave future functionality to the future, or does this
need
> to be done now?

I'm not convinced either way, but not doing it could put us in a difficult
position later. Done.

https://codereview.appspot.com/28880043/diff/80001/state/state.go#newcode795
state/state.go:795: return nil, fmt.Errorf("environment is no longer alive")
On 2013/12/09 10:14:41, fwereade wrote:
> ditto NotFound handling

Done.

[axw, 2013-12-11 09:18:47]

Please take a look.