Launchpad itself

lib/lp/app/browser/tales.py (+2/-0)
lib/lp/buildmaster/manager.py (+45/-14)
lib/lp/buildmaster/model/buildfarmjob.py (+4/-1)
lib/lp/buildmaster/tests/test_manager.py (+112/-5)

To merge this branch:

bzr merge lp://staging/~julian-edwards/launchpad/cancel-build-bug-173018-part2

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Brad Crittenden (community)	code	2011-10-28	Approve on 2011-10-28
Review via email: mp+80662@code.staging.launchpad.net

Commit message

[r=bac][bug=173018] Alter the buildd-manager to notice that a build state is CANCELLING, and rip it forcefully off the builder by resetting the virtual machine.

Description of the change

Second branch in a pipeline of changes to add mid-build cancellation on the build farm. This one fixes the buildd-manager to notice that build states are CANCELLING just before it tries to check an active build and rips it forcefully off the builder by resetting the virtual machine. (hence it only works on virtual builders)

Most of the branch is tests, the actual fix is easy.

Revision history for this message

Brad Crittenden (bac) wrote on 2011-10-28:

Hi Julian,

Nice branch and tests.

I notice in your tests there is an inconsistency whether you 'return d' or not. It is unclear to me that it is necessary. Could you take a pass and see if it is required?

Also, in these tests the lambda can be eliminated:

d.addCallback(lambda result: self.assertFalse(result))

can be

d.addCallback(self.assertFalse)

Is there a reason to prefer the wordier pattern?

In general the tests are non-trivial but well structured and easy to follow!

review: Approve (code)

Revision history for this message

Brad Crittenden (bac) wrote on 2011-10-28:

I forgot to ask that you clean up some lint.

Revision history for this message

Julian Edwards (julian-edwards) wrote on 2011-10-31:

On Friday 28 October 2011 17:54:24 you wrote:
> Review: Approve code
>
> Hi Julian,
>
> Nice branch and tests.

Thank you, and thanks for the review.

> I notice in your tests there is an inconsistency whether you 'return d' or
> not. It is unclear to me that it is necessary. Could you take a pass and
> see if it is required?

Argh, well spotted. I do indeed need to return d. It's not strictly
necessary but it helps the test runner spot unfired Deferreds.

> Also, in these tests the lambda can be eliminated:
>
> d.addCallback(lambda result: self.assertFalse(result))
>
> can be
>
> d.addCallback(self.assertFalse)
>
> Is there a reason to prefer the wordier pattern?

It's a left-over from some simplifications - I guess I can simplify it
further!

> In general the tests are non-trivial but well structured and easy to follow!

Thank you - I tried. Twisted makes it somewhat harder to follow than normal.
And the buildd-manager is not exactly the easiest part of LP ...

Cheers!

(PS yes I'll fix lint)