state: machine death restrictions

state: machine death restrictions

Machine.EnsureDying and .EnsureDead will now fail when the machine is
running JobManageEnviron, or when it has units assigned.

https://code.launchpad.net/~fwereade/juju-core/machine-death-restrictions/+merge/141088

(do not edit description out of merge proposal)

[fwereade, 2012-12-21 15:16:21]

Please take a look.

[dimitern, 2013-01-03 22:19:03]

LGTM, but I have a couple of questions.

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

https://codereview.appspot.com/7005047/diff/1/state/machine.go#newcode162
state/machine.go:162: for i := 0; i < 2; i++ {
why retry up to 2 times?

https://codereview.appspot.com/7005047/diff/1/state/machine.go#newcode196
state/machine.go:196: for i := 0; i < 2; i++ {
why the loop? (and why to 2)

[fwereade, 2013-01-04 08:30:36]

On 2013/01/03 22:19:03, dimitern wrote:
> LGTM, but I have a couple of questions.
> 
> https://codereview.appspot.com/7005047/diff/1/state/machine.go
> File state/machine.go (right):
> 
> https://codereview.appspot.com/7005047/diff/1/state/machine.go#newcode162
> state/machine.go:162: for i := 0; i < 2; i++ {
> why retry up to 2 times?
> 
> https://codereview.appspot.com/7005047/diff/1/state/machine.go#newcode196
> state/machine.go:196: for i := 0; i < 2; i++ {
> why the loop? (and why to 2)

In both cases, explained just above:

// deathFailureReason returns an error indicating why the machine may have
// failed to advance its lifecycle to Dying or Dead. If deathFailureReason
// returns no error, a failed lifecycle operation should be retried once,
// on the basis that the blocker may have been removed in between the txn
// and the diagnosis; otherwise this should be treated as a serious error.

Maybe I should be explicitly pointing to that documentation? When I wrote it it
seemed as though that was overkill, but maybe not. Hmm.

[dimitern, 2013-01-04 09:14:14]

On 2013/01/04 08:30:36, fwereade wrote:
> On 2013/01/03 22:19:03, dimitern wrote:
> > LGTM, but I have a couple of questions.
> > 
> > https://codereview.appspot.com/7005047/diff/1/state/machine.go
> > File state/machine.go (right):
> > 
> > https://codereview.appspot.com/7005047/diff/1/state/machine.go#newcode162
> > state/machine.go:162: for i := 0; i < 2; i++ {
> > why retry up to 2 times?
> > 
> > https://codereview.appspot.com/7005047/diff/1/state/machine.go#newcode196
> > state/machine.go:196: for i := 0; i < 2; i++ {
> > why the loop? (and why to 2)
> 
> In both cases, explained just above:
> 
> // deathFailureReason returns an error indicating why the machine may have
> // failed to advance its lifecycle to Dying or Dead. If deathFailureReason
> // returns no error, a failed lifecycle operation should be retried once,
> // on the basis that the blocker may have been removed in between the txn
> // and the diagnosis; otherwise this should be treated as a serious error.
> 
> Maybe I should be explicitly pointing to that documentation? When I wrote it
it
> seemed as though that was overkill, but maybe not. Hmm.

Well, now it's obvious the 2 is because the states are two (e.g. Dying and
Dead). But probably a short comment before the loop is in order to mention that
(or probably I'm just too shortsighted :) )

[fwereade, 2013-01-04 10:38:34]

On 2013/01/04 09:14:14, dimitern wrote:
> On 2013/01/04 08:30:36, fwereade wrote:
> > On 2013/01/03 22:19:03, dimitern wrote:
> > > LGTM, but I have a couple of questions.
> > > 
> > > https://codereview.appspot.com/7005047/diff/1/state/machine.go
> > > File state/machine.go (right):
> > > 
> > > https://codereview.appspot.com/7005047/diff/1/state/machine.go#newcode162
> > > state/machine.go:162: for i := 0; i < 2; i++ {
> > > why retry up to 2 times?
> > > 
> > > https://codereview.appspot.com/7005047/diff/1/state/machine.go#newcode196
> > > state/machine.go:196: for i := 0; i < 2; i++ {
> > > why the loop? (and why to 2)
> > 
> > In both cases, explained just above:
> > 
> > // deathFailureReason returns an error indicating why the machine may have
> > // failed to advance its lifecycle to Dying or Dead. If deathFailureReason
> > // returns no error, a failed lifecycle operation should be retried once,
> > // on the basis that the blocker may have been removed in between the txn
> > // and the diagnosis; otherwise this should be treated as a serious error.
> > 
> > Maybe I should be explicitly pointing to that documentation? When I wrote it
> it
> > seemed as though that was overkill, but maybe not. Hmm.
> 
> Well, now it's obvious the 2 is because the states are two (e.g. Dying and
> Dead). But probably a short comment before the loop is in order to mention
that
> (or probably I'm just too shortsighted :) )

That's not the reason -- looks like I definitely need to comment it better :).

On reflection, I think I should maybe bump it up a little -- it's *unlikely*
that the 2nd attempt could fail for a sane reason, and it grows ever more
unlikely for every subsequent retry, but there's actually no point at which we
can be certain of corrupt state. Bah. (It's the same situation as
Relation.Destroy -- it's possible for the number of assigned units to flip
between 0 and 1 indefinitely, perfectly timed to block the transaction (when
it's 1) but offer no reason for the failure (when it's 0).)

[rog, 2013-01-07 09:48:49]

needs some thought, i think. some suggestions below.

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

https://codereview.appspot.com/7005047/diff/1/state/machine.go#newcode140
state/machine.go:140: if j == JobManageEnviron {
i'm not sure about this. we may well allow several environ-managing machines in
the future, and it's a only a problem when the last one is removed. i'm not sure
we can express that in a transaction though - maybe we'd need a environ-managers
ref count somewhere.

https://codereview.appspot.com/7005047/diff/1/state/machine.go#newcode145
state/machine.go:145: return fmt.Errorf("unit %q is assigned",
m.doc.Principals[0])
s/assigned/assigned to it/
?

https://codereview.appspot.com/7005047/diff/1/state/machine.go#newcode162
state/machine.go:162: for i := 0; i < 2; i++ {
On 2013/01/03 22:19:03, dimitern wrote:
> why retry up to 2 times?

i think we shouldn't retry. a simple "machine is in use" error would be
sufficient, i think, if we can't diagnose the problem more accurately.

then we lose the need for the "please contact" error, which seems a bit unusual.

[fwereade, 2013-01-07 11:59:52]

Please take a look.

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

https://codereview.appspot.com/7005047/diff/1/state/machine.go#newcode140
state/machine.go:140: if j == JobManageEnviron {
On 2013/01/07 09:48:49, rog wrote:
> i'm not sure about this. we may well allow several environ-managing machines
in
> the future, and it's a only a problem when the last one is removed. i'm not
sure
> we can express that in a transaction though - maybe we'd need a
environ-managers
> ref count somewhere.

Agreed on IRC that this is ok for now, because otherwise we'd need icky special
cases for machine 0, so long as I comment that it may need changing.

https://codereview.appspot.com/7005047/diff/1/state/machine.go#newcode145
state/machine.go:145: return fmt.Errorf("unit %q is assigned",
m.doc.Principals[0])
On 2013/01/07 09:48:49, rog wrote:
> s/assigned/assigned to it/
> ?

Done.

https://codereview.appspot.com/7005047/diff/1/state/machine.go#newcode162
state/machine.go:162: for i := 0; i < 2; i++ {
On 2013/01/07 09:48:49, rog wrote:
> On 2013/01/03 22:19:03, dimitern wrote:
> > why retry up to 2 times?
> 
> i think we shouldn't retry. a simple "machine is in use" error would be
> sufficient, i think, if we can't diagnose the problem more accurately.
> 
> then we lose the need for the "please contact" error, which seems a bit
unusual.

Discussed at length in IRC; grudgingly agreed that a retry isn't *too* awful,
and kicked the error message question upstairs to lp:1096840.

[rog, 2013-01-07 12:21:29]

one more comment

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

https://codereview.appspot.com/7005047/diff/1/state/machine.go#newcode157
state/machine.go:157: defer func() {
i'm not sure i see that this defer is worth it.
there's only one nil return. why not set m.doc.Life there?

same applies to EnsureDead below.

[rog, 2013-01-07 12:23:27]

LGTM modulo above comment.

[rog, 2013-01-07 12:25:03]

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

https://codereview.appspot.com/7005047/diff/1/state/machine.go#newcode157
state/machine.go:157: defer func() {
On 2013/01/07 12:21:29, rog wrote:
> i'm not sure i see that this defer is worth it.
> there's only one nil return. why not set m.doc.Life there?
> 
> same applies to EnsureDead below.

oops, scratch that, i just noticed the other possibly-nil return.

[fwereade, 2013-01-07 12:33:45]

*** Submitted:

state: machine death restrictions

Machine.EnsureDying and .EnsureDead will now fail when the machine is
running JobManageEnviron, or when it has units assigned.

R=dimitern, rog
CC=
https://codereview.appspot.com/7005047