Merge lp:~axwalk/juju-core/state-environment-life into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

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://codereview.appspot.com/28880043/

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://codereview.appspot.com/28880043/

[Andrew Wilkins (axwalk)]

Reviewers: mp+195710_code.launchpad.net,

Message:
Please take a look.

Description:
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.

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

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/28880043/

Affected files (+172, -34 lines):
   A [revision details]
   M names/environ.go
   M state/environ.go
   M state/environ_test.go
   M state/initialize_test.go
   M state/interface.go
   M state/state.go
   M state/state_test.go
   M state/watcher.go

[Andrew Wilkins (axwalk)]

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

https://codereview.appspot.com/28880043/

[Frank Mueller (themue)]

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)?

https://codereview.appspot.com/28880043/

[Andrew Wilkins (axwalk)]

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.

https://codereview.appspot.com/28880043/

[William Reade (fwereade)]

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#newcode75
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 tran...

[Andrew Wilkins (axwalk)]

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
de...

[William Reade (fwereade)]

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#newcode567
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 carefu...

[Andrew Wilkins (axwalk)]

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#newcode567
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 machin...

[Andrew Wilkins (axwalk)]

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#newcode567
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.

https://codereview.appspot.com/28880043/

[William Reade (fwereade)]

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#oldcode69
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...

https://codereview.appspot.com/28880043/

[Andrew Wilkins (axwalk)]

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#oldcode69
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*
c...

[William Reade (fwereade)]

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 jus...

[Andrew Wilkins (axwalk)]

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.

https://codereview.appspot.com/28880043/

[Andrew Wilkins (axwalk)]

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#newcode54
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...

[William Reade (fwereade)]

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.

https://codereview.appspot.com/28880043/

[Andrew Wilkins (axwalk)]

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.

https://codereview.appspot.com/28880043/

[Andrew Wilkins (axwalk)]

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.

https://codereview.appspot.com/28880043/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/28880043/

=== modified file 'names/environ.go'
--- names/environ.go	2013-09-13 14:48:13 +0000
+++ names/environ.go	2013-12-11 09:15:40 +0000
@@ -7,13 +7,15 @@
 	"strings"
 )
 
-// EnvironTag returns the tag of an environment with the given name.
-func EnvironTag(name string) string {
-	return makeTag(EnvironTagKind, name)
+// EnvironTag returns the tag of an environment with the given environment UUID.
+func EnvironTag(uuid string) string {
+	return makeTag(EnvironTagKind, uuid)
 }
 
-// IsEnvironment returns whether id is a valid environment id.
-// TODO(rog) stricter constraints
-func IsEnvironment(name string) bool {
-	return !strings.Contains(name, "/")
+// IsEnvironment returns whether id is a valid environment UUID.
+func IsEnvironment(id string) bool {
+	// TODO(axw) 2013-12-04 #1257587
+	// We should not accept environment tags that
+	// do not look like UUIDs.
+	return !strings.Contains(id, "/")
 }

=== modified file 'state/addmachine.go'
--- state/addmachine.go	2013-12-03 18:09:32 +0000
+++ state/addmachine.go	2013-12-11 09:15:40 +0000
@@ -10,6 +10,7 @@
 	"labix.org/v2/mgo/txn"
 
 	"launchpad.net/juju-core/constraints"
+	"launchpad.net/juju-core/errors"
 	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/state/api/params"
 	"launchpad.net/juju-core/utils"
@@ -176,7 +177,20 @@
 }
 
 func (st *State) addMachine(mdoc *machineDoc, ops []txn.Op) (*Machine, error) {
+	env, err := st.Environment()
+	if err != nil {
+		return nil, err
+	} else if env.Life() != Alive {
+		return nil, fmt.Errorf("environment is no longer alive")
+	}
+	ops = append([]txn.Op{env.assertAliveOp()}, ops...)
 	if err := st.runTransaction(ops); err != nil {
+		enverr := env.Refresh()
+		if (enverr == nil && env.Life() != Alive) || errors.IsNotFoundError(enverr) {
+			return nil, fmt.Errorf("environment is no longer alive")
+		} else if enverr != nil {
+			err = enverr
+		}
 		return nil, err
 	}
 	m := newMachine(st, mdoc)

=== modified file 'state/cleanup.go'
--- state/cleanup.go	2013-12-10 09:28:57 +0000
+++ state/cleanup.go	2013-12-11 09:15:40 +0000
@@ -55,6 +55,8 @@
 			err = st.cleanupSettings(doc.Prefix)
 		case "units":
 			err = st.cleanupUnits(doc.Prefix)
+		case "services":
+			err = st.cleanupServices()
 		case "machine":
 			err = st.cleanupMachine(doc.Prefix)
 		default:
@@ -94,6 +96,26 @@
 	return nil
 }
 
+// cleanupServices sets all services to Dying, if they are not already Dying
+// or Dead. It's expected to be used when an environment is destroyed.
+func (st *State) cleanupServices() error {
+	// This won't miss services, because a Dying environment cannot have
+	// services added to it. But we do have to remove the services themselves
+	// via individual transactions, because they could be in any state at all.
+	service := &Service{st: st}
+	sel := D{{"life", Alive}}
+	iter := st.services.Find(sel).Iter()
+	for iter.Next(&service.doc) {
+		if err := service.Destroy(); err != nil {
+			return err
+		}
+	}
+	if err := iter.Err(); err != nil {
+		return fmt.Errorf("cannot read service document: %v", err)
+	}
+	return nil
+}
+
 // cleanupUnits sets all units with the given prefix to Dying, if they are not
 // already Dying or Dead. It's expected to be used when a service is destroyed.
 func (st *State) cleanupUnits(prefix string) error {

=== modified file 'state/cleanup_test.go'
--- state/cleanup_test.go	2013-12-10 09:28:57 +0000
+++ state/cleanup_test.go	2013-12-11 09:15:40 +0000
@@ -55,6 +55,48 @@
 	c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
 }
 
+func (s *CleanupSuite) TestCleanupEnvironmentServices(c *gc.C) {
+	s.assertDoesNotNeedCleanup(c)
+
+	// Create a service with some units.
+	mysql, err := s.State.AddService("mysql", s.AddTestingCharm(c, "mysql"))
+	c.Assert(err, gc.IsNil)
+	units := make([]*state.Unit, 3)
+	for i := range units {
+		unit, err := mysql.AddUnit()
+		c.Assert(err, gc.IsNil)
+		units[i] = unit
+	}
+	s.assertDoesNotNeedCleanup(c)
+
+	// Destroy the environment and check the service and units are
+	// unaffected, but a cleanup for the service has been scheduled.
+	env, err := s.State.Environment()
+	c.Assert(err, gc.IsNil)
+	err = env.Destroy()
+	c.Assert(err, gc.IsNil)
+	s.assertNeedsCleanup(c)
+	s.assertCleanupRuns(c)
+	err = mysql.Refresh()
+	c.Assert(err, gc.IsNil)
+	c.Assert(mysql.Life(), gc.Equals, state.Dying)
+	for _, unit := range units {
+		err = unit.Refresh()
+		c.Assert(err, gc.IsNil)
+		c.Assert(unit.Life(), gc.Equals, state.Alive)
+	}
+
+	// The first cleanup Destroys the service, which
+	// schedules another cleanup to destroy the units.
+	s.assertNeedsCleanup(c)
+	s.assertCleanupRuns(c)
+	for _, unit := range units {
+		err = unit.Refresh()
+		c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
+	}
+	s.assertDoesNotNeedCleanup(c)
+}
+
 func (s *CleanupSuite) TestCleanupRelationSettings(c *gc.C) {
 	s.assertDoesNotNeedCleanup(c)
 

=== modified file 'state/environ.go'
--- state/environ.go	2013-11-15 13:02:24 +0000
+++ state/environ.go	2013-12-11 09:15:40 +0000
@@ -26,21 +26,15 @@
 type environmentDoc struct {
 	UUID string `bson:"_id"`
 	Name string
+	Life Life
 }
 
 // Environment returns the environment entity.
 func (st *State) Environment() (*Environment, error) {
-	doc := environmentDoc{}
-	err := st.environments.Find(nil).One(&doc)
-	if err == mgo.ErrNotFound {
-		return nil, errors.NotFoundf("environment")
-	} else if err != nil {
+	env := &Environment{st: st}
+	if err := env.refresh(st.environments.Find(nil)); err != nil {
 		return nil, err
 	}
-	env := &Environment{
-		st:  st,
-		doc: doc,
-	}
 	env.annotator = annotator{
 		globalKey: env.globalKey(),
 		tag:       env.Tag(),
@@ -52,24 +46,77 @@
 // Tag returns a name identifying the environment.
 // The returned name will be different from other Tag values returned
 // by any other entities from the same state.
-func (e Environment) Tag() string {
-	return names.EnvironTag(e.doc.Name)
+func (e *Environment) Tag() string {
+	return names.EnvironTag(e.doc.UUID)
 }
 
 // UUID returns the universally unique identifier of the environment.
-func (e Environment) UUID() string {
+func (e *Environment) UUID() string {
 	return e.doc.UUID
 }
 
+// Life returns whether the environment is Alive, Dying or Dead.
+func (e *Environment) Life() Life {
+	return e.doc.Life
+}
+
 // globalKey returns the global database key for the environment.
 func (e *Environment) globalKey() string {
 	return environGlobalKey
 }
 
+func (e *Environment) Refresh() error {
+	return e.refresh(e.st.environments.FindId(e.UUID()))
+}
+
+func (e *Environment) refresh(query *mgo.Query) error {
+	err := query.One(&e.doc)
+	if err == mgo.ErrNotFound {
+		return errors.NotFoundf("environment")
+	}
+	return err
+}
+
+// Destroy sets the environment's lifecycle to Dying, preventing
+// addition of services or machines to state.
+func (e *Environment) Destroy() error {
+	if e.Life() != Alive {
+		return nil
+	}
+	// TODO(axw) 2013-12-11 #1218688
+	// Resolve the race between checking for manual machines and
+	// destroying the environment. We can set Environment to Dying
+	// to resolve the race, but that puts the environment into an
+	// unusable state.
+	//
+	// We add a cleanup for services, but not for machines;
+	// machines are destroyed via the provider interface. The
+	// exception to this rule is manual machines; the API prevents
+	// destroy-environment from succeeding if any non-manager
+	// manual machines exist.
+	ops := []txn.Op{{
+		C:      e.st.environments.Name,
+		Id:     e.doc.UUID,
+		Update: D{{"$set", D{{"life", Dying}}}},
+		Assert: isAliveDoc,
+	}, e.st.newCleanupOp("services", "")}
+	err := e.st.runTransaction(ops)
+	switch err {
+	case nil, txn.ErrAborted:
+		// If the transaction aborted, the environment is either
+		// Dying or Dead; neither case is an error. If it's Dead,
+		// reporting it as Dying is not incorrect; the user thought
+		// it was Alive, so we've progressed towards Dead. If the
+		// user then calls Refresh they'll get the true value.
+		e.doc.Life = Dying
+	}
+	return err
+}
+
 // createEnvironmentOp returns the operation needed to create
 // an environment document with the given name and UUID.
 func createEnvironmentOp(st *State, name, uuid string) txn.Op {
-	doc := &environmentDoc{uuid, name}
+	doc := &environmentDoc{uuid, name, Alive}
 	return txn.Op{
 		C:      st.environments.Name,
 		Id:     uuid,
@@ -77,3 +124,12 @@
 		Insert: doc,
 	}
 }
+
+// assertAliveOp returns a txn.Op that asserts the environment is alive.
+func (e *Environment) assertAliveOp() txn.Op {
+	return txn.Op{
+		C:      e.st.environments.Name,
+		Id:     e.UUID(),
+		Assert: isAliveDoc,
+	}
+}

=== modified file 'state/environ_test.go'
--- state/environ_test.go	2013-08-19 11:20:02 +0000
+++ state/environ_test.go	2013-12-11 09:15:40 +0000
@@ -24,9 +24,7 @@
 }
 
 func (s *EnvironSuite) TestTag(c *gc.C) {
-	cfg, err := s.State.EnvironConfig()
-	c.Assert(err, gc.IsNil)
-	expected := "environment-" + cfg.Name()
+	expected := "environment-" + s.env.UUID()
 	c.Assert(s.env.Tag(), gc.Equals, expected)
 }
 

=== modified file 'state/initialize_test.go'
--- state/initialize_test.go	2013-12-06 04:47:45 +0000
+++ state/initialize_test.go	2013-12-11 09:15:40 +0000
@@ -51,6 +51,8 @@
 	_, err := s.State.EnvironConfig()
 	c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
 	_, err = s.State.FindEntity("environment-foo")
+	// TODO(axw) 2013-12-04 #1257587
+	// remove backwards compatibility for environment-tag; see state.go
 	c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
 	_, err = s.State.EnvironConstraints()
 	c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
@@ -66,10 +68,13 @@
 	cfg, err = s.State.EnvironConfig()
 	c.Assert(err, gc.IsNil)
 	c.Assert(cfg.AllAttrs(), gc.DeepEquals, initial)
-	env0, err := s.State.FindEntity("environment-" + cfg.Name())
-	c.Assert(err, gc.IsNil)
-	env := env0.(state.Annotator)
-	annotations, err := env.Annotations()
+
+	env, err := s.State.Environment()
+	c.Assert(err, gc.IsNil)
+	entity, err := s.State.FindEntity("environment-" + env.UUID())
+	c.Assert(err, gc.IsNil)
+	annotator := entity.(state.Annotator)
+	annotations, err := annotator.Annotations()
 	c.Assert(err, gc.IsNil)
 	c.Assert(annotations, gc.HasLen, 0)
 	cons, err := s.State.EnvironConstraints()

=== modified file 'state/interface.go'
--- state/interface.go	2013-10-02 13:14:49 +0000
+++ state/interface.go	2013-12-11 09:15:40 +0000
@@ -128,6 +128,7 @@
 	_ NotifyWatcherFactory = (*Machine)(nil)
 	_ NotifyWatcherFactory = (*Unit)(nil)
 	_ NotifyWatcherFactory = (*Service)(nil)
+	_ NotifyWatcherFactory = (*Environment)(nil)
 )
 
 // AgentEntity represents an entity that can

=== modified file 'state/state.go'
--- state/state.go	2013-12-06 04:47:45 +0000
+++ state/state.go	2013-12-11 09:15:40 +0000
@@ -390,16 +390,29 @@
 	case names.ServiceTagKind:
 		return st.Service(id)
 	case names.EnvironTagKind:
-		conf, err := st.EnvironConfig()
+		env, err := st.Environment()
 		if err != nil {
 			return nil, err
 		}
 		// Return an invalid entity error if the requested environment is not
 		// the current one.
-		if id != conf.Name() {
-			return nil, errors.NotFoundf("environment %q", id)
+		if id != env.UUID() {
+			if utils.IsValidUUIDString(id) {
+				return nil, errors.NotFoundf("environment %q", id)
+			}
+			// TODO(axw) 2013-12-04 #1257587
+			// We should not accept environment tags that do not match the
+			// environment's UUID. We accept anything for now, to cater
+			// both for past usage, and for potentially supporting aliases.
+			logger.Warningf("environment-tag does not match current environment UUID: %q != %q", id, env.UUID())
+			conf, err := st.EnvironConfig()
+			if err != nil {
+				logger.Warningf("EnvironConfig failed: %v", err)
+			} else if id != conf.Name() {
+				logger.Warningf("environment-tag does not match current environment name: %q != %q", id, conf.Name())
+			}
 		}
-		return st.Environment()
+		return env, nil
 	case names.RelationTagKind:
 		return st.KeyRelation(id)
 	}
@@ -424,6 +437,8 @@
 		coll = st.users.Name
 	case names.RelationTagKind:
 		coll = st.relations.Name
+	case names.EnvironTagKind:
+		coll = st.environments.Name
 	default:
 		return "", "", fmt.Errorf("%q is not a valid collection tag", tag)
 	}
@@ -511,6 +526,12 @@
 	} else if exists {
 		return nil, fmt.Errorf("service already exists")
 	}
+	env, err := st.Environment()
+	if err != nil {
+		return nil, err
+	} else if env.Life() != Alive {
+		return nil, fmt.Errorf("environment is no longer alive")
+	}
 	// Create the service addition operations.
 	peers := ch.Meta().Peers
 	svcDoc := &serviceDoc{
@@ -523,6 +544,7 @@
 	}
 	svc := newService(st, svcDoc)
 	ops := []txn.Op{
+		env.assertAliveOp(),
 		createConstraintsOp(st, svc.globalKey(), constraints.Value{}),
 		createSettingsOp(st, svc.settingsKey(), nil),
 		{
@@ -544,9 +566,15 @@
 	ops = append(ops, peerOps...)
 
 	// Run the transaction; happily, there's never any reason to retry,
-	// because all the possible failed assertions imply that the service
-	// already exists.
+	// because all the possible failed assertions imply that either the
+	// service already exists, or the environment is being destroyed.
 	if err := st.runTransaction(ops); err == txn.ErrAborted {
+		err := env.Refresh()
+		if (err == nil && env.Life() != Alive) || errors.IsNotFoundError(err) {
+			return nil, fmt.Errorf("environment is no longer alive")
+		} else if err != nil {
+			return nil, err
+		}
 		return nil, fmt.Errorf("service already exists")
 	} else if err != nil {
 		return nil, err

=== modified file 'state/state_test.go'
--- state/state_test.go	2013-12-06 04:47:45 +0000
+++ state/state_test.go	2013-12-11 09:15:40 +0000
@@ -207,6 +207,33 @@
 	check(m[1], "1", "blahblah", allJobs)
 }
 
+func (s *StateSuite) TestAddMachinesEnvironmentDying(c *gc.C) {
+	_, err := s.State.AddMachine("quantal", state.JobHostUnits)
+	c.Assert(err, gc.IsNil)
+	env, err := s.State.Environment()
+	c.Assert(err, gc.IsNil)
+	err = env.Destroy()
+	c.Assert(err, gc.IsNil)
+	// Check that machines cannot be added if the environment is initially Dying.
+	_, err = s.State.AddMachine("quantal", state.JobHostUnits)
+	c.Assert(err, gc.ErrorMatches, "cannot add a new machine: environment is no longer alive")
+}
+
+func (s *StateSuite) TestAddMachinesEnvironmentDyingAfterInitial(c *gc.C) {
+	_, err := s.State.AddMachine("quantal", state.JobHostUnits)
+	c.Assert(err, gc.IsNil)
+	env, err := s.State.Environment()
+	c.Assert(err, gc.IsNil)
+	// Check that machines cannot be added if the environment is initially
+	// Alive but set to Dying immediately before the transaction is run.
+	defer state.SetBeforeHooks(c, s.State, func() {
+		c.Assert(env.Life(), gc.Equals, state.Alive)
+		c.Assert(env.Destroy(), gc.IsNil)
+	}).Check()
+	_, err = s.State.AddMachine("quantal", state.JobHostUnits)
+	c.Assert(err, gc.ErrorMatches, "cannot add a new machine: environment is no longer alive")
+}
+
 func (s *StateSuite) TestAddMachineExtraConstraints(c *gc.C) {
 	err := s.State.SetEnvironConstraints(constraints.MustParse("mem=4G"))
 	c.Assert(err, gc.IsNil)
@@ -585,6 +612,35 @@
 	c.Assert(ch.URL(), gc.DeepEquals, charm.URL())
 }
 
+func (s *StateSuite) TestAddServiceEnvironmentDying(c *gc.C) {
+	charm := s.AddTestingCharm(c, "dummy")
+	_, err := s.State.AddService("s0", charm)
+	c.Assert(err, gc.IsNil)
+	// Check that services cannot be added if the environment is initially Dying.
+	env, err := s.State.Environment()
+	c.Assert(err, gc.IsNil)
+	err = env.Destroy()
+	c.Assert(err, gc.IsNil)
+	_, err = s.State.AddService("s1", charm)
+	c.Assert(err, gc.ErrorMatches, `cannot add service "s1": environment is no longer alive`)
+}
+
+func (s *StateSuite) TestAddServiceEnvironmentDyingAfterInitial(c *gc.C) {
+	charm := s.AddTestingCharm(c, "dummy")
+	_, err := s.State.AddService("s0", charm)
+	c.Assert(err, gc.IsNil)
+	env, err := s.State.Environment()
+	c.Assert(err, gc.IsNil)
+	// Check that services cannot be added if the environment is initially
+	// Alive but set to Dying immediately before the transaction is run.
+	defer state.SetBeforeHooks(c, s.State, func() {
+		c.Assert(env.Life(), gc.Equals, state.Alive)
+		c.Assert(env.Destroy(), gc.IsNil)
+	}).Check()
+	_, err = s.State.AddService("s1", charm)
+	c.Assert(err, gc.ErrorMatches, `cannot add service "s1": environment is no longer alive`)
+}
+
 func (s *StateSuite) TestServiceNotFound(c *gc.C) {
 	_, err := s.State.Service("bummer")
 	c.Assert(err, gc.ErrorMatches, `service "bummer" not found`)
@@ -1611,10 +1667,12 @@
 	c.Assert(err, gc.IsNil)
 }
 
-var findEntityTests = []struct {
+type findEntityTest struct {
 	tag string
 	err string
-}{{
+}
+
+var findEntityTests = []findEntityTest{{
 	tag: "",
 	err: `"" is not a valid tag`,
 }, {
@@ -1651,8 +1709,8 @@
 	tag: "service-foo/bar",
 	err: `"service-foo/bar" is not a valid service tag`,
 }, {
-	tag: "environment-foo",
-	err: `environment "foo" not found`,
+	tag: "environment-9f484882-2f18-4fd2-967d-db9663db7bea",
+	err: `environment "9f484882-2f18-4fd2-967d-db9663db7bea" not found`,
 }, {
 	tag: "machine-1234",
 	err: `machine 1234 not found`,
@@ -1673,7 +1731,13 @@
 }, {
 	tag: "user-arble",
 }, {
+	// TODO(axw) 2013-12-04 #1257587
+	// remove backwards compatibility for environment-tag; see state.go
+	tag: "environment-notauuid",
+	//err: `"environment-notauuid" is not a valid environment tag`,
+}, {
 	tag: "environment-testenv",
+	//err: `"environment-testenv" is not a valid environment tag`,
 }}
 
 var entityTypes = map[string]interface{}{
@@ -1702,6 +1766,14 @@
 	c.Assert(err, gc.IsNil)
 	c.Assert(rel.String(), gc.Equals, "wordpress:db ser-vice2:server")
 
+	// environment tag is dynamically generated
+	env, err := s.State.Environment()
+	c.Assert(err, gc.IsNil)
+	findEntityTests = append([]findEntityTest{}, findEntityTests...)
+	findEntityTests = append(findEntityTests, findEntityTest{
+		tag: "environment-" + env.UUID(),
+	})
+
 	for i, test := range findEntityTests {
 		c.Logf("test %d: %q", i, test.tag)
 		e, err := s.State.FindEntity(test.tag)
@@ -1712,7 +1784,14 @@
 			kind, err := names.TagKind(test.tag)
 			c.Assert(err, gc.IsNil)
 			c.Assert(e, gc.FitsTypeOf, entityTypes[kind])
-			c.Assert(e.Tag(), gc.Equals, test.tag)
+			if kind == "environment" {
+				// TODO(axw) 2013-12-04 #1257587
+				// We *should* only be able to get the entity with its tag, but
+				// for backwards-compatibility we accept any non-UUID tag.
+				c.Assert(e.Tag(), gc.Equals, env.Tag())
+			} else {
+				c.Assert(e.Tag(), gc.Equals, test.tag)
+			}
 		}
 	}
 }
@@ -1725,7 +1804,6 @@
 		"foo-",
 		"---",
 		"foo-bar",
-		"environment-foo",
 		"unit-foo",
 	}
 	for _, name := range bad {
@@ -1767,6 +1845,14 @@
 	c.Assert(coll, gc.Equals, "users")
 	c.Assert(id, gc.Equals, user.Name())
 	c.Assert(err, gc.IsNil)
+
+	// Parse an environment entity name.
+	env, err := s.State.Environment()
+	c.Assert(err, gc.IsNil)
+	coll, id, err = state.ParseTag(s.State, env.Tag())
+	c.Assert(coll, gc.Equals, "environments")
+	c.Assert(id, gc.Equals, env.UUID())
+	c.Assert(err, gc.IsNil)
 }
 
 func (s *StateSuite) TestWatchCleanups(c *gc.C) {

=== modified file 'state/watcher.go'
--- state/watcher.go	2013-11-25 05:03:44 +0000
+++ state/watcher.go	2013-12-11 09:15:40 +0000
@@ -1070,6 +1070,11 @@
 	return newEntityWatcher(u.st, u.st.units, u.doc.Name)
 }
 
+// Watch returns a watcher for observing changes to an environment.
+func (e *Environment) Watch() NotifyWatcher {
+	return newEntityWatcher(e.st, e.st.environments, e.doc.UUID)
+}
+
 // WatchForEnvironConfigChanges return a NotifyWatcher waiting for the Environ
 // Config to change. This differs from WatchEnvironConfig in that the watcher
 // is a NotifyWatcher that does not give content during Changes()