Code review comment for lp:~rvb/maas/retry-power-changes

On Wednesday 23 Jul 2014 12:23:49 you wrote:
> Not dealing with errors was deliberate: I don't want the retry loop to catch
> failures at all. Failures should only happen when there is a problem
> executing the power template (syntax problem, missing argument, etc.) and I
> don't think we should catch any of those. If something inside
> perform_power_change() raises an exceptions, none of the twisted magic will
> swallow it right? It will simply be "raised" from 'yield deferToThread(…)
> correct?

I'm afraid I don't agree with this approach. :)

Yes, Twisted internally turns it into a Failure, and then yield will re-raise
the exception it contains.

We've had instances in the past where we've had support requests for "power
not working" which was caused by a dodgy template that someone had edited, and
they were not looking at the cluster log at all!

I really think that we should catch these and flag them up somewhere.

> That's not part of the interface (yet). I think the node event log will
> contain the explanation of why it failed but we can always refine this
> later based on actual user testing.

OK.

I'd like to see a failure_reason text on the node for this situation. The
user should be able to see this alongside the BROKEN status IMO.

> Fixed (I didn't realize we had so many matchers to deal with mocks). Here
> and elsewhere.

They're great, add more :)