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

On 07/23/2014 02:33 PM, Julian Edwards wrote:
> 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!

Hum, I still think the stacktrace belongs to the log file… but one thing
is sure (and means that, like you said, we need to take care of this
case): we want the node marked as broken when the template execution
blows up.

>> Fixed (I didn't realize we had so many matchers to deal with mocks). Here
>> and elsewhere.
>
> They're great, add more :)

The errors I'm getting aren't so great sometimes:
[...]
      raise mismatch_error
MismatchError: !=:
reference = [call(context-key-22VPn2=u'context-val-JmgwGQ',
power_change=u'on')]
actual = [call(context-key-22VPn2=u'context-val-JmgwGQ',
power_change=u'on')]
: after <function get_mock_calls> on <Mock name='mock().execute'
id='140547462746960'>: calls do not match