Machine lifecycle is out of scope; SOA concepts demand more discussion
in a separate forum; I think I've addressed the rest.


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

https://codereview.appspot.com/24790044/diff/1/cmd/juju/destroymachine.go#newcode34
cmd/juju/destroymachine.go:34: f.BoolVar(&c.Force, "force", false,
"completely remove machine and all dependencies")
On 2013/11/12 00:43:42, wallyworld wrote:
> Perhaps expand a bit:

> "completely remove machine and all dependencies, even if Juju thinks
the machine
> has running units or containers"

> I think we need to be verbose as to what the implications are. I would
even go
> so far as to say we need a [y/N] prompt like for destroy-environment
as a typo
> in the machine id could be catastrophic.

Expanded the doc string -- I'm kinda -1 on an additional gate beyond
specifying "--force" though.

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

https://codereview.appspot.com/24790044/diff/1/cmd/juju/destroymachine_test.go#newcode48
cmd/juju/destroymachine_test.go:48: // is warned about.
On 2013/11/12 00:43:42, wallyworld wrote:
> The above comment needs rewording now the test has been split up

Whoops, thanks.

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

https://codereview.appspot.com/24790044/diff/1/state/api/client.go#newcode154
state/api/client.go:154: func (c *Client) ForceDestroyMachines(machines
...string) error {
On 2013/11/12 02:58:37, jameinel wrote:
> On 2013/11/12 00:43:42, wallyworld wrote:
> > It makes me sad that we need to add a whole new endpoint because Go
doesn't
> > support default parameter values :-(

> It isn't really an endpoint, but it is a new function.

> I only make the distinction because I was about to say the next
change:
> -func (c *Client) DestroyServiceUnits(unitNames []string) error {
> +func (c *Client) DestroyServiceUnits(unitNames ...string) error {


> was an API break, but this is all just within the client process.

> so we *could* spell it:

> DestroyMachines(force bool, machines ...string)

> I'm guessing it was a bit ackward and William wanted to avoid bool in
APIs
> especially for a Force sort of function.

Yeah, it's just a new function, and the bool option felt horrible. Re
DestroyServiceUnits I'm willing to impose that minimal pain on anyone
who's using api.Client in an external library, because I think the
global pain of inconsistency is worse.

https://codereview.appspot.com/24790044/diff/1/state/apiserver/client/client_test.go
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/24790044/diff/1/state/apiserver/client/client_test.go#newcode549
state/apiserver/client/client_test.go:549: assertLife(c, m0,
state.Alive)
On 2013/11/12 02:58:37, jameinel wrote:
> On 2013/11/12 00:43:42, wallyworld wrote:
> > Why doesn't force destroy cause the life to be set to Dying? I find
it
> > particularly unintuitive that a status would show the machine as
alive when in
> > fact it is on death row and waiting for the clean up job to remove
it.

> In earlier discussions, this is a limitation of the Machine lifecycle
model. A
> Machine can't be set to dying until all Units are no longer on the
machine.

It sucks, I agree. I *think* it's *slightly* worse to add a new flag and
pollute status than to put up with that for the period of time in which
we can't set Dying at the right time. (In particular,
double-force-destroying will not be harmful.)

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

https://codereview.appspot.com/24790044/diff/1/state/cleanup_test.go#newcode93
state/cleanup_test.go:93: func (s *CleanupSuite)
TestForceDestroyMachineErrors(c *gc.C) {
On 2013/11/12 00:43:42, wallyworld wrote:
> The code in this test can definitely be extracted out into a helper

> func (s *CleanupSuite) assertForceDestroyMachineError(c *gc.C, job
> state.MachineJob) {
>      m, err := s.State.AddMachine("quantal", job)
>      c.Assert(err, gc.IsNil)
>      err = environManager.ForceDestroy()
>      c.Assert(err, gc.ErrorMatches, "machine [0-9] is required by the
> environment")
>      s.assertDoesNotNeedCleanup(c)
>      assertLife(c, m, state.Alive)
> }

Done.

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

https://codereview.appspot.com/24790044/diff/1/state/machine.go#newcode325
state/machine.go:325: }
On 2013/11/12 00:43:42, wallyworld wrote:
> Yeah, so IMO we should set the machine to Dying here to avoid
ambiguity. I was
> very surprised reading the tests that had the machine as Alive after
being
> forcibly destroyed.

Agreed. We'll fix the machine lifecycle model one day -- maybe even soon
-- but it's not quite bubbled up to the top of the list yet.

https://codereview.appspot.com/24790044/