Code review comment for lp:~fwereade/juju-core/api-force-destroy-machines
Revision history for this message
| William Reade (fwereade) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
f33e652
(Get the code!)
Machine lifecycle is out of scope; SOA concepts demand more discussion
in a separate forum; I think I've addressed the rest.
https:/
File cmd/juju/
https:/
cmd/juju/
"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:/
File cmd/juju/
https:/
cmd/juju/
On 2013/11/12 00:43:42, wallyworld wrote:
> The above comment needs rewording now the test has been split up
Whoops, thanks.
https:/
File state/api/client.go (right):
https:/
state/api/
...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) DestroyServiceU
> +func (c *Client) DestroyServiceU
> was an API break, but this is all just within the client process.
> so we *could* spell it:
> DestroyMachines
> 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:/
File state/apiserver
https:/
state/apiserver
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-
https:/
File state/cleanup_
https:/
state/cleanup_
TestForceDestro
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) assertForceDest
> state.MachineJob) {
> m, err := s.State.
> c.Assert(err, gc.IsNil)
> err = environManager.
> c.Assert(err, gc.ErrorMatches, "machine [0-9] is required by the
> environment")
> s.assertDoesNot
> assertLife(c, m, state.Alive)
> }
Done.
https:/
File state/machine.go (right):
https:/
state/machine.
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:/