launchpad-reviewers team mailing list archive

[Julian Edwards <julian.edwards@xxxxxxxxxxxxx>]

Julian Edwards has proposed merging lp:~julian-edwards/launchpad/cancel-build-bug-173018-part2 into lp:launchpad with lp:~julian-edwards/launchpad/cancel-build-bug-173018 as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #173018 in Launchpad itself: "In progress builds cannot be cancelled"
  https://bugs.launchpad.net/launchpad/+bug/173018

For more details, see:
https://code.launchpad.net/~julian-edwards/launchpad/cancel-build-bug-173018-part2/+merge/80662

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.
-- 
https://code.launchpad.net/~julian-edwards/launchpad/cancel-build-bug-173018-part2/+merge/80662
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/launchpad/cancel-build-bug-173018-part2 into lp:launchpad.

=== modified file 'lib/lp/buildmaster/manager.py'
--- lib/lp/buildmaster/manager.py	2011-10-28 12:19:35 +0000
+++ lib/lp/buildmaster/manager.py	2011-10-28 12:19:35 +0000
@@ -187,6 +187,32 @@
                 exc_info=True)
             transaction.abort()
 
+    def checkCancellation(self, builder):
+        """See if there is a pending cancellation request.
+
+        If the current build is in status CANCELLING then terminate it
+        immediately.
+
+        :return: A deferred whose value is True if we cancelled the build.
+        """
+        if not builder.virtualized:
+            return defer.succeed(False)
+        buildqueue = self.builder.getBuildQueue()
+        if not buildqueue:
+            return defer.succeed(False)
+        build = buildqueue.specific_job.build
+        if build.status != BuildStatus.CANCELLING:
+            return defer.succeed(False)
+
+        def resume_done(ignored):
+            return defer.succeed(True)
+
+        buildqueue.cancel()
+        transaction.commit()
+        d = builder.resumeSlaveHost()
+        d.addCallback(resume_done)
+        return d
+
     def scan(self):
         """Probe the builder and update/dispatch/collect as appropriate.
 
@@ -215,14 +241,6 @@
 
         self.builder = get_builder(self.builder_name)
 
-        if self.builder.builderok:
-            # TODO: check build status for CANCELLING and virtual bulder
-            #   a) set to CANCELLED,
-            #   b) reset builder
-            d = self.builder.updateStatus(self.logger)
-        else:
-            d = defer.succeed(None)
-
         def status_updated(ignored):
             # Commit the changes done while possibly rescuing jobs, to
             # avoid holding table locks.
@@ -282,8 +300,22 @@
                     return None
             return d.addCallback(job_started)
 
-        d.addCallback(status_updated)
-        d.addCallback(build_updated)
+        def cancellation_checked(cancelled):
+            if cancelled:
+                return defer.succeed(None)
+            d = self.builder.updateStatus(self.logger)
+            d.addCallback(status_updated)
+            d.addCallback(build_updated)
+            return d
+
+        if self.builder.builderok:
+            d = self.checkCancellation(self.builder)
+            d.addCallback(cancellation_checked)
+        else:
+            d = defer.succeed(None)
+            d.addCallback(status_updated)
+            d.addCallback(build_updated)
+
         return d
 
 

=== modified file 'lib/lp/buildmaster/model/buildfarmjob.py'
--- lib/lp/buildmaster/model/buildfarmjob.py	2011-10-28 12:19:35 +0000
+++ lib/lp/buildmaster/model/buildfarmjob.py	2011-10-28 12:19:35 +0000
@@ -113,6 +113,10 @@
         """See `IBuildFarmJobOld`."""
         pass
 
+    def jobCancel(self):
+        """See `IBuildFarmJobOld`."""
+        pass
+
     @staticmethod
     def addCandidateSelectionCriteria(processor, virtualized):
         """See `IBuildFarmJobOld`."""

=== modified file 'lib/lp/buildmaster/tests/test_manager.py'
--- lib/lp/buildmaster/tests/test_manager.py	2011-10-26 12:31:14 +0000
+++ lib/lp/buildmaster/tests/test_manager.py	2011-10-28 12:19:35 +0000
@@ -82,8 +82,6 @@
         'bob' builder.
         """
         super(TestSlaveScannerScan, self).setUp()
-        self.slave = self.useFixture(BuilddSlaveTestSetup())
-
         # Creating the required chroots needed for dispatching.
         test_publisher = make_publisher()
         ubuntu = getUtility(IDistributionSet).getByName('ubuntu')
@@ -212,6 +210,7 @@
 
     def testScanRescuesJobFromBrokenBuilder(self):
         # The job assigned to a broken builder is rescued.
+        self.useFixture(BuilddSlaveTestSetup())
 
         # Sampledata builder is enabled and is assigned to an active job.
         builder = getUtility(IBuilderSet)[BOB_THE_BUILDER_NAME]
@@ -420,6 +419,112 @@
 
         return d.addCallback(check)
 
+    def test_cancelling_a_build(self):
+        # When scanning an in-progress build, if its state is CANCELLING
+        # then the build should be stopped and moved to the CANCELLED state.
+
+        # Set up a building slave with a fake resume method so we can see
+        # if it got called later.
+        slave = BuildingSlave(build_id="8-1")
+        call_counter = FakeMethod()
+        def fake_resume():
+            call_counter()
+            return defer.succeed((None, None, 0))
+        slave.resume = fake_resume
+
+        # Set the sample data builder building with the slave from above.
+        builder = getUtility(IBuilderSet)[BOB_THE_BUILDER_NAME]
+        login('foo.bar@xxxxxxxxxxxxx')
+        builder.builderok = True
+        # For now, we can only cancel virtual builds.
+        builder.virtualized = True
+        builder.vm_host = "fake_vm_host"
+        builder.setSlaveForTesting(slave)
+        transaction.commit()
+        login(ANONYMOUS)
+        buildqueue = builder.currentjob
+        self.assertBuildingJob(buildqueue, builder)
+
+        # Now set the build to CANCELLING.
+        build = getUtility(IBinaryPackageBuildSet).getByQueueEntry(buildqueue)
+        build.status = BuildStatus.CANCELLING
+
+        # Run 'scan' and check its results.
+        self.layer.switchDbUser(config.builddmaster.dbuser)
+        scanner = self._getScanner()
+        d = scanner.scan()
+
+        # The build state should be cancelled and we should have also
+        # called the resume() method on the slave that resets the virtual
+        # machine.
+        def check_cancelled(ignore, builder, buildqueue):
+            self.assertEqual(1, call_counter.call_count)
+            self.assertEqual(BuildStatus.CANCELLED, build.status)
+
+        d.addCallback(check_cancelled, builder, buildqueue)
+        return d
+
+
+class TestCancellationChecking(TestCaseWithFactory):
+    """Unit tests for the checkCancellation method."""
+
+    layer = ZopelessDatabaseLayer
+    run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=20)
+
+    def setUp(self):
+        super(TestCancellationChecking, self).setUp()
+        builder_name = BOB_THE_BUILDER_NAME
+        self.builder = getUtility(IBuilderSet)[builder_name]
+        self.builder.virtualized = True
+        self.scanner = SlaveScanner(builder_name, BufferLogger())
+        self.scanner.builder = self.builder
+        self.scanner.logger.name = 'slave-scanner'
+
+    def test_ignores_nonvirtual(self):
+        # If the builder is nonvirtual make sure we return False.
+        self.builder.virtualized = False
+        d = self.scanner.checkCancellation(self.builder)
+        d.addCallback(lambda result: self.assertFalse(result))
+
+    def test_ignores_no_buildqueue(self):
+        # If the builder has no buildqueue associated,
+        # make sure we return False.
+        buildqueue = self.builder.currentjob
+        buildqueue.reset()
+        d = self.scanner.checkCancellation(self.builder)
+        d.addCallback(lambda result: self.assertFalse(result))
+
+    def test_ignores_build_not_cancelling(self):
+        # If the active build is not in a CANCELLING state, ignore it.
+        buildqueue = self.builder.currentjob
+        build = getUtility(IBinaryPackageBuildSet).getByQueueEntry(buildqueue)
+        build.status = BuildStatus.BUILDING
+        d = self.scanner.checkCancellation(self.builder)
+        d.addCallback(lambda result: self.assertFalse(result))
+
+    def test_cancelling_build_is_cancelled(self):
+        # If a build is CANCELLING, make sure True is returned and the
+        # slave was resumed.
+        call_counter = FakeMethod()
+        def fake_resume():
+            call_counter()
+            return defer.succeed((None, None, 0))
+        slave = OkSlave()
+        slave.resume = fake_resume
+        self.builder.vm_host = "fake_vm_host"
+        self.builder.setSlaveForTesting(slave)
+        buildqueue = self.builder.currentjob
+        build = getUtility(IBinaryPackageBuildSet).getByQueueEntry(buildqueue)
+        build.status = BuildStatus.CANCELLING
+
+        def check(result):
+            self.assertEqual(1, call_counter.call_count)
+            self.assertTrue(result)
+            self.assertEqual(BuildStatus.CANCELLED, build.status)
+
+        d = self.scanner.checkCancellation(self.builder)
+        d.addCallback(check)
+
 
 class TestBuilddManager(TestCase):