MAAS

Code review comment for lp:~julian-edwards/maas/only-ready-when-power-is-off

only-ready-when-power-is-off
Merge into trunk

Revision history for this message

Julian Edwards (julian-edwards) wrote on 2014-09-16:

On Monday 15 Sep 2014 07:07:25 you wrote:
> I think it's OK for release() to power off the node always, because our
> knowledge of its current power state may be out of date. I doubt we
> control all state transitions thoroughly at the moment, so there may be
> obscure paths like: node is switched off; power poller registers that it's
> off; node is marked as broken; power poller stops looking at the node
> because it's broken; admin powers node on and works their magic; admin
> marks node as fixed; user allocates node; MAAS issues power-on command;
> node misses the command because it's already on.
>
> So as far as I'm concerned, the XXX comment can become just a regular
> comment note. In fact I probably wouldn't even bother with a special code
> path for the case where the node is already off: the power method can
> figure out that it's not needed, or if it prefers, it can issue a probably
> redundant power-off command for luck.

I'll try it and see how it works out, but I am concerned it will lead to
concurrent power operations - we must avoid the situation where we need to
serialize them at the power driver level, that's just madness. It's good
practice to catch problems as early as you can!

> And looking at the code under that XXX comment, does it make sense to keep
> the owner set when releasing a node? The user is done with the node at
> that point. For debugging it may well be interesting to know who was using
> it, but that's historical information — maybe it belongs in the node event
> log. So I think I'd just clear the node's owner during release() either
> way.

It's not released until it's off IMO, which is why I kept the owner on. The
state RELEASING implies it's not released yet as well.

> One thing is completely unclear to me: what does the RESERVED state
> represent? It may have come up in London but if so I don't remember.
> There's a comment in the enum that talks about the node being ready for
> “named” deployment. I have no idea what that means. I'm not even sure
> whether it's talking about a DNS daemon or the normal adjective. I don't
> see any place in the code where a node might enter this state. Yet your
> test uses this state for a powered-off node and not for a powered-on
> node... Why? What's the distinction?

It can be deleted as we discussed on the call.

Thanks for the review!

On Monday 15 Sep 2014 07:07:25 you wrote:
> I think it's OK for release() to power off the node always, because our
> knowledge of its current power state may be out of date.  I doubt we
> control all state transitions thoroughly at the moment, so there may be
> obscure paths like: node is switched off; power poller registers that it's
> off; node is marked as broken; power poller stops looking at the node
> because it's broken; admin powers node on and works their magic; admin
> marks node as fixed; user allocates node; MAAS issues power-on command;
> node misses the command because it's already on.
> 
> So as far as I'm concerned, the XXX comment can become just a regular
> comment note.  In fact I probably wouldn't even bother with a special code
> path for the case where the node is already off: the power method can
> figure out that it's not needed, or if it prefers, it can issue a probably
> redundant power-off command for luck.

I'll try it and see how it works out, but I am concerned it will lead to 
concurrent power operations - we must avoid the situation where we need to 
serialize them at the power driver level, that's just madness.  It's good 
practice to catch problems as early as you can!

> And looking at the code under that XXX comment, does it make sense to keep
> the owner set when releasing a node?  The user is done with the node at
> that point.  For debugging it may well be interesting to know who was using
> it, but that's historical information — maybe it belongs in the node event
> log.  So I think I'd just clear the node's owner during release() either
> way.

It's not released until it's off IMO, which is why I kept the owner on.  The 
state RELEASING implies it's not released yet as well.

> One thing is completely unclear to me: what does the RESERVED state
> represent?  It may have come up in London but if so I don't remember. 
> There's a comment in the enum that talks about the node being ready for
> “named” deployment.  I have no idea what that means.  I'm not even sure
> whether it's talking about a DNS daemon or the normal adjective.  I don't
> see any place in the code where a node might enter this state.  Yet your
> test uses this state for a powered-off node and not for a powered-on
> node...  Why?  What's the distinction?

It can be deleted as we discussed on the call.

Thanks for the review!

« Back to merge proposal