Migrate juju destroy-environment to use API

Migrate juju destroy-environment to use API

This change introduces a new client API
method, DestroyJuju. This method does the
following, server-side, in order:
 - marks the environment as Dying, preventing
   the addition of machines and services to
   state
 - ensures there are no non-manager manually-
   provisioned machines in state
 - marks all services as Dying, thus prevening
   units from being added
 - stops all non-manager machine instances
 - marks the environment as Dead

Machine agents watch, via the machiner API, for
changes to the environment's lifecycle, and
terminate themselves when the environment
becomes Dead. This is how we terminate manual
bootstrap nodes.

A followup will update agent termination logic
to uninstall mongo on state servers, and also
remove the data-dir/log-dir.

Fixes #1218688
Fixes #1246343

https://code.launchpad.net/~axwalk/juju-core/lp1246983-cli-api-removeagents/+merge/194801

(do not edit description out of merge proposal)

[axw, 2013-11-12 07:29:34]

Please take a look.

[jameinel, 2013-11-12 08:39:25]

some thougts. we did most of the discussion on IRC, I think.

https://codereview.appspot.com/22870045/diff/1/cmd/juju/destroyenvironment.go
File cmd/juju/destroyenvironment.go (right):

https://codereview.appspot.com/22870045/diff/1/cmd/juju/destroyenvironment.go...
cmd/juju/destroyenvironment.go:83: if err =
conn.State.Client().DestroyJuju(removeAgentsTimeout); err != nil {
I'm intrigued that you pass in the timeout.I was wondering if we need it
configable (I know my manual machines take at least 2min to shutdown cleanly).
At least we can expose it on the CLI if we find it is necessary.

https://codereview.appspot.com/22870045/diff/1/state/api/machiner/machiner.go
File state/api/machiner/machiner.go (right):

https://codereview.appspot.com/22870045/diff/1/state/api/machiner/machiner.go...
state/api/machiner/machiner.go:58: func (st *State) Environment(machineTag
string) (*Environment, error) {
I don't quite understand why you take a machineTag for an environment.
Is this just a copy and paste that didn't finish renaming things?

But the MachineEnvironmentResult is still here, too.

If it *does* need a Machine, then I'm not sure why this is on State rather than
Machine. (though to be fair, this is Machine.State and not state.State which is
a personal source of much confusion).

https://codereview.appspot.com/22870045/diff/1/state/apiserver/client/destroy...
File state/apiserver/client/destroyjuju.go (right):

https://codereview.appspot.com/22870045/diff/1/state/apiserver/client/destroy...
state/apiserver/client/destroyjuju.go:49: func destroyUnits(st *state.State,
attemptStrategy utils.AttemptStrategy) error {
It is surprising to me that destroyUnits would be triggered on the Client side,
rather than on the API server in response to the env.Destroy request.

https://codereview.appspot.com/22870045/diff/1/state/apiserver/machine/machin...
File state/apiserver/machine/machiner.go (right):

https://codereview.appspot.com/22870045/diff/1/state/apiserver/machine/machin...
state/apiserver/machine/machiner.go:61: // the corresponding machine's
environment.
I don't think we want to design multi-tenancy into the API today.

I'm willing to be corrected by someone else, but we should keep in mind the
future, but only build what we need right now. (Because we're likely to get it
wrong until we're actually building the next thing anyway.)

[axw, 2013-11-12 10:03:08]

So, based on conversations on IRC, I'm going to rework this. From fwereade:

"(1) abort if manual machines exist (2) destroy services, set destroying flag
(3) directly StopInstances for non-managers in the API server (4)
environ.Destroy in the CLI"

Sounds fair, I'll do that.

A follow-up will deal with manual machines, and will do the two-phase start/wait
business with a watcher and timeout in between.

https://codereview.appspot.com/22870045/diff/1/cmd/juju/destroyenvironment.go
File cmd/juju/destroyenvironment.go (right):

https://codereview.appspot.com/22870045/diff/1/cmd/juju/destroyenvironment.go...
cmd/juju/destroyenvironment.go:83: if err =
conn.State.Client().DestroyJuju(removeAgentsTimeout); err != nil {
On 2013/11/12 08:39:26, jameinel wrote:
> I'm intrigued that you pass in the timeout.I was wondering if we need it
> configable (I know my manual machines take at least 2min to shutdown cleanly).
> At least we can expose it on the CLI if we find it is necessary.

rogpeppe has suggested we break the API into two (in the usual async pattern):
one that starts, one that finalises. The client can use the all-watcher to
observe machine/unit destruction, and provide feedback along the way. Despite
the added complexity, I'm in favour.

https://codereview.appspot.com/22870045/diff/1/state/api/machiner/machiner.go
File state/api/machiner/machiner.go (right):

https://codereview.appspot.com/22870045/diff/1/state/api/machiner/machiner.go...
state/api/machiner/machiner.go:58: func (st *State) Environment(machineTag
string) (*Environment, error) {
On 2013/11/12 08:39:26, jameinel wrote:
> I don't quite understand why you take a machineTag for an environment.
> Is this just a copy and paste that didn't finish renaming things?
> 
> But the MachineEnvironmentResult is still here, too.
> 
> If it *does* need a Machine, then I'm not sure why this is on State rather
than
> Machine. (though to be fair, this is Machine.State and not state.State which
is
> a personal source of much confusion).

It needs a Machine for when we have multi-tenancy. The machine-agent doesn't
know its environment name at the point of call, hence this. I'm open to
suggestions, I know it's a bit weird.

https://codereview.appspot.com/22870045/diff/1/state/apiserver/client/destroy...
File state/apiserver/client/destroyjuju.go (right):

https://codereview.appspot.com/22870045/diff/1/state/apiserver/client/destroy...
state/apiserver/client/destroyjuju.go:49: func destroyUnits(st *state.State,
attemptStrategy utils.AttemptStrategy) error {
On 2013/11/12 08:39:26, jameinel wrote:
> It is surprising to me that destroyUnits would be triggered on the Client
side,
> rather than on the API server in response to the env.Destroy request.

This is happening on the API server side. This is just the "Client API", as used
by the CLI and GUI. Am I misunderstanding your comment?

https://codereview.appspot.com/22870045/diff/1/state/apiserver/machine/machin...
File state/apiserver/machine/machiner.go (right):

https://codereview.appspot.com/22870045/diff/1/state/apiserver/machine/machin...
state/apiserver/machine/machiner.go:61: // the corresponding machine's
environment.
On 2013/11/12 08:39:26, jameinel wrote:
> I don't think we want to design multi-tenancy into the API today.
> 
> I'm willing to be corrected by someone else, but we should keep in mind the
> future, but only build what we need right now. (Because we're likely to get it
> wrong until we're actually building the next thing anyway.)

Understood, I just didn't want to code us into a corner. I can take out the
parameters for now, and then we can add them in when they're needed.

[axw, 2013-11-13 05:14:44]

Please take a look.

[william.reade, 2013-11-18 14:43:36]

Some good bits, some I'ma bit alarmed about. Can we do a single CL for the state
changes, please, and leave the API bits and additional workers and so on out for
the moment?

There are a couple of open questions in the state code, it's true, but resolving
and reproposing those will be the quickest way to get some of this landed.

https://codereview.appspot.com/22870045/diff/1/state/api/machiner/machiner.go
File state/api/machiner/machiner.go (right):

https://codereview.appspot.com/22870045/diff/1/state/api/machiner/machiner.go...
state/api/machiner/machiner.go:58: func (st *State) Environment(machineTag
string) (*Environment, error) {
On 2013/11/12 10:03:08, axw wrote:
> On 2013/11/12 08:39:26, jameinel wrote:
> > I don't quite understand why you take a machineTag for an environment.
> > Is this just a copy and paste that didn't finish renaming things?
> > 
> > But the MachineEnvironmentResult is still here, too.
> > 
> > If it *does* need a Machine, then I'm not sure why this is on State rather
> than
> > Machine. (though to be fair, this is Machine.State and not state.State which
> is
> > a personal source of much confusion).
> 
> It needs a Machine for when we have multi-tenancy. The machine-agent doesn't
> know its environment name at the point of call, hence this. I'm open to
> suggestions, I know it's a bit weird.

I don't think it is necessary, actually, because we will surely know what
environment we're connected to  -- and I don't think it'd  work *anyway*,
because the machine-tag doesn't identify an environment anyway.

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

https://codereview.appspot.com/22870045/diff/20001/state/api/machiner/environ...
state/api/machiner/environ.go:39: func (e *Environment) Watch()
(watcher.NotifyWatcher, error) {
Environment?

https://codereview.appspot.com/22870045/diff/20001/state/api/machiner/machine...
File state/api/machiner/machiner.go (right):

https://codereview.appspot.com/22870045/diff/20001/state/api/machiner/machine...
state/api/machiner/machiner.go:65: tag:  result.EnvironmentTag,
Whoa. I hope this doesn't include a name -- the moment we start messing around
with environment tags we need to un-screw-up this situation (environment names
are not globally unique -- any time we use it we must be aware it's only an
alias).

Regardless I don't think there's a justification for talking about more than one
environment at this level anyway.

https://codereview.appspot.com/22870045/diff/20001/state/apiserver/client/des...
File state/apiserver/client/destroyjuju.go (right):

https://codereview.appspot.com/22870045/diff/20001/state/apiserver/client/des...
state/apiserver/client/destroyjuju.go:97: return env.StopInstances(instances)
There's a wrinkle here in that some machines may be missing for random stupid
provider reasons, and those won't end up getting stopped. I'm becoming convinced
that it's really stupid to have StopInstances take Instances instead of ids --
it means that we need to go through all the hassle of finding out everything
about one (ie hitting the provider api, potentially triggering rate limiting,
etc) before we can just simply stop them.

https://codereview.appspot.com/22870045/diff/20001/state/apiserver/client/des...
state/apiserver/client/destroyjuju.go:124: // Due to an import loop in tests, we
cannot import manual.
In tests? I'm surprised by that.

https://codereview.appspot.com/22870045/diff/20001/state/apiserver/machine/ma...
File state/apiserver/machine/machiner.go (right):

https://codereview.appspot.com/22870045/diff/20001/state/apiserver/machine/ma...
state/apiserver/machine/machiner.go:60: var result
params.MachineEnvironmentResult
Why do we persist in naming things after the entities that call them? This is
surely an EnvironmentResult.

And FWIW, the whole domain-object-centricity is not something we want to
encourage. This is a new API for new functionality, let's not even bother
calling the API at this stage: just call Life() directly, and only when we need
it, ie in response to a watcher event.

https://codereview.appspot.com/22870045/diff/20001/state/environ.go
File state/environ.go (left):

https://codereview.appspot.com/22870045/diff/20001/state/environ.go#oldcode56
state/environ.go:56: return names.EnvironTag(e.doc.Name)
Right, yeah, this is completely broken. Not your fault, it was a bad call I made
in atlanta, but it needs to be resolved.

I suggest that anything currently using an environment tag be changed to ignore
the name, and anything producing an environment tag use the UUID instead of the
name.

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

https://codereview.appspot.com/22870045/diff/20001/state/environ.go#newcode83
state/environ.go:83: return e.advanceLifecycle(Dead)
What does it mean for an environment to jump straight to Dead from Alive? I'm
not sure this is a sane operation.

In fact, I think an environment is like a Service or a Relation, not like a Unit
or Machine, and so Dead makes no sense for it. When does it ever make sense for
an Environment object to exist but access to it be invalid? That's what Dead
means basically.

https://codereview.appspot.com/22870045/diff/20001/state/environ.go#newcode130
state/environ.go:130: }
How does all this handle existing environments without Life fields? Is it that
Alive == 0 and so nobody notices? It's a bit of a potential timebomb when it
comes to selecting them, but I guess it's enough of an edge case...

...ah, hmm. You assert on setting to Dead, but not to Dying? Is that why?

https://codereview.appspot.com/22870045/diff/20001/state/state.go
File state/state.go (left):

https://codereview.appspot.com/22870045/diff/20001/state/state.go#oldcode753
state/state.go:753: // already exists.
This comment is now a lie.

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

https://codereview.appspot.com/22870045/diff/20001/state/state_test.go#newcod...
state/state_test.go:223: c.Assert(err, gc.ErrorMatches, ".*environment is being
destroyed")
Never use leading .*s in error tests, please -- they just hide ugly messages
that subsequently slap our users in the face.

https://codereview.appspot.com/22870045/diff/20001/state/state_test.go#newcod...
state/state_test.go:554: // Check that machines cannot be added if the
environment is Dying.
s/machines/services/

https://codereview.appspot.com/22870045/diff/20001/worker/machiner/machiner.go
File worker/machiner/machiner.go (right):

https://codereview.appspot.com/22870045/diff/20001/worker/machiner/machiner.g...
worker/machiner/machiner.go:126: }
I question the utility of this type. We're terminating all provider instances,
so machine teardown is moot; and allowing the possibility for manual machines to
do this doesn't seem like a good idea, because it'll leave the units lying
around.

[axw, 2013-11-19 03:07:17]

I'll create a new CL for the state bits, as requested.

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

https://codereview.appspot.com/22870045/diff/20001/state/api/machiner/environ...
state/api/machiner/environ.go:39: func (e *Environment) Watch()
(watcher.NotifyWatcher, error) {
On 2013/11/18 14:43:37, william.reade wrote:
> Environment?

Are you talking about the comment? Fixed.

https://codereview.appspot.com/22870045/diff/20001/state/api/machiner/machine...
File state/api/machiner/machiner.go (right):

https://codereview.appspot.com/22870045/diff/20001/state/api/machiner/machine...
state/api/machiner/machiner.go:65: tag:  result.EnvironmentTag,
On 2013/11/18 14:43:37, william.reade wrote:
> Whoa. I hope this doesn't include a name -- the moment we start messing around
> with environment tags we need to un-screw-up this situation (environment names
> are not globally unique -- any time we use it we must be aware it's only an
> alias).
> 
> Regardless I don't think there's a justification for talking about more than
one
> environment at this level anyway.

I'm not sure what you mean by "talking about more than one environment". That
tag is just a handle so we can get a watcher, and is always expected to be the
same.

Do you have a preferred API here?

https://codereview.appspot.com/22870045/diff/20001/state/apiserver/client/des...
File state/apiserver/client/destroyjuju.go (right):

https://codereview.appspot.com/22870045/diff/20001/state/apiserver/client/des...
state/apiserver/client/destroyjuju.go:97: return env.StopInstances(instances)
On 2013/11/18 14:43:37, william.reade wrote:
> There's a wrinkle here in that some machines may be missing for random stupid
> provider reasons, and those won't end up getting stopped. I'm becoming
convinced
> that it's really stupid to have StopInstances take Instances instead of ids --
> it means that we need to go through all the hassle of finding out everything
> about one (ie hitting the provider api, potentially triggering rate limiting,
> etc) before we can just simply stop them.

Agreed.

https://codereview.appspot.com/22870045/diff/20001/state/apiserver/client/des...
state/apiserver/client/destroyjuju.go:124: // Due to an import loop in tests, we
cannot import manual.
On 2013/11/18 14:43:37, william.reade wrote:
> In tests? I'm surprised by that.

The environs/manual tests import something that imports state/apiserver. Thus,
importing environs/manual here prevents running the environs/manual tests.

https://codereview.appspot.com/22870045/diff/20001/state/apiserver/machine/ma...
File state/apiserver/machine/machiner.go (right):

https://codereview.appspot.com/22870045/diff/20001/state/apiserver/machine/ma...
state/apiserver/machine/machiner.go:60: var result
params.MachineEnvironmentResult
On 2013/11/18 14:43:37, william.reade wrote:
> Why do we persist in naming things after the entities that call them? This is
> surely an EnvironmentResult.

The idea here was "give me the environment which this machine belongs to", not
an arbitrary naming-after-the-caller. In light of previous discussions about not
caring about multi tenancy today, this can be renamed.

> And FWIW, the whole domain-object-centricity is not something we want to
> encourage. This is a new API for new functionality, let's not even bother
> calling the API at this stage: just call Life() directly, and only when we
need
> it, ie in response to a watcher event.

Life requires an entity tag. Watch requires an entity tag. How can we "call
Life() directly, and only when we need it, ie in response to a watcher event"
without the tag?

https://codereview.appspot.com/22870045/diff/20001/state/environ.go
File state/environ.go (left):

https://codereview.appspot.com/22870045/diff/20001/state/environ.go#oldcode56
state/environ.go:56: return names.EnvironTag(e.doc.Name)
On 2013/11/18 14:43:37, william.reade wrote:
> Right, yeah, this is completely broken. Not your fault, it was a bad call I
made
> in atlanta, but it needs to be resolved.
> 
> I suggest that anything currently using an environment tag be changed to
ignore
> the name, and anything producing an environment tag use the UUID instead of
the
> name.

Okay. I'll do that in the new CL.

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

https://codereview.appspot.com/22870045/diff/20001/state/environ.go#newcode83
state/environ.go:83: return e.advanceLifecycle(Dead)
On 2013/11/18 14:43:37, william.reade wrote:
> What does it mean for an environment to jump straight to Dead from Alive? I'm
> not sure this is a sane operation.

No, probably not.

> In fact, I think an environment is like a Service or a Relation, not like a
Unit
> or Machine, and so Dead makes no sense for it. When does it ever make sense
for
> an Environment object to exist but access to it be invalid? That's what Dead
> means basically.

I will drop EnsureDead, and add Remove instead. Does that make more sense?

https://codereview.appspot.com/22870045/diff/20001/state/environ.go#newcode130
state/environ.go:130: }
On 2013/11/18 14:43:37, william.reade wrote:
> How does all this handle existing environments without Life fields? Is it that
> Alive == 0 and so nobody notices? It's a bit of a potential timebomb when it
> comes to selecting them, but I guess it's enough of an edge case...

Right, I verified that lack of Life is equivalent to Life==0, which is Alive.
This doesn't handle existing environments with Dying/Dead machines, but I
consider that an edge case that we could live with.

> ...ah, hmm. You assert on setting to Dead, but not to Dying? Is that why?

The assertion is the other way around: setting to Dead always succeeds, but
Dying only if it's not Dead. Anyway, I don't see what that has to do with
missing Life?

https://codereview.appspot.com/22870045/diff/20001/state/state.go
File state/state.go (left):

https://codereview.appspot.com/22870045/diff/20001/state/state.go#oldcode753
state/state.go:753: // already exists.
On 2013/11/18 14:43:37, william.reade wrote:
> This comment is now a lie.

Thanks, updated.

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

https://codereview.appspot.com/22870045/diff/20001/state/state_test.go#newcod...
state/state_test.go:223: c.Assert(err, gc.ErrorMatches, ".*environment is being
destroyed")
On 2013/11/18 14:43:37, william.reade wrote:
> Never use leading .*s in error tests, please -- they just hide ugly messages
> that subsequently slap our users in the face.

Fixed.

https://codereview.appspot.com/22870045/diff/20001/state/state_test.go#newcod...
state/state_test.go:554: // Check that machines cannot be added if the
environment is Dying.
On 2013/11/18 14:43:37, william.reade wrote:
> s/machines/services/

Done.

https://codereview.appspot.com/22870045/diff/20001/worker/machiner/machiner.go
File worker/machiner/machiner.go (right):

https://codereview.appspot.com/22870045/diff/20001/worker/machiner/machiner.g...
worker/machiner/machiner.go:126: }
On 2013/11/18 14:43:37, william.reade wrote:
> I question the utility of this type. We're terminating all provider instances,
> so machine teardown is moot; and allowing the possibility for manual machines
to
> do this doesn't seem like a good idea, because it'll leave the units lying
> around.

The environment won't be destroyed until there are no units remaining, so that
cannot happen.