launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> === modified file 'lib/lp/archiveuploader/tests/test_uploadprocessor.py'
> --- lib/lp/archiveuploader/tests/test_uploadprocessor.py	2015-01-23 17:57:59 +0000
> +++ lib/lp/archiveuploader/tests/test_uploadprocessor.py	2015-02-19 01:43:44 +0000
> @@ -142,6 +142,7 @@
>  
>          self.queue_folder = tempfile.mkdtemp()
>          self.incoming_folder = os.path.join(self.queue_folder, "incoming")
> +        self.failed_folder = os.path.join(self.queue_folder, "failed")
>          os.makedirs(self.incoming_folder)
>  
>          self.test_files_dir = os.path.join(config.root,
> @@ -2258,9 +2259,7 @@
>          self.assertIsNot(None, build.duration)
>          self.assertIs(None, build.upload_log)
>  
> -    def testBuildWithInvalidStatus(self):
> -        # Builds with an invalid (non-UPLOADING) status should trigger
> -        # a warning but be left alone.
> +    def processUploadWithBuildStatus(self, status):
>          upload_dir = self.queueUpload("bar_1.0-1")
>          self.processUpload(self.uploadprocessor, upload_dir)
>          source_pub = self.publishPackage('bar', '1.0-1')
> @@ -2272,9 +2271,10 @@
>              status=PackageUploadStatus.ACCEPTED,
>              version=u"1.0-1", name=u"bar")
>          queue_item.setDone()
> +        stub.test_emails.pop()
>  
>          build.buildqueue_record.markAsBuilding(self.factory.makeBuilder())
> -        build.updateStatus(BuildStatus.BUILDING)
> +        build.updateStatus(status)
>          self.switchToUploader()
>  
>          shutil.rmtree(upload_dir)
> @@ -2287,17 +2287,40 @@
>              "bar_1.0-1_binary", queue_entry=leaf_name)
>          self.options.context = 'buildd'
>          self.options.builds = True
> -        len(stub.test_emails)
> +        self.assertEqual([], stub.test_emails)
>          BuildUploadHandler(self.uploadprocessor, self.incoming_folder,
>              leaf_name).process()
>          self.layer.txn.commit()
> +
> +        return build, leaf_name
> +
> +    def testBuildStillBuilding(self):
> +        # Builds that are still BUILDING should be left alone.  The
> +        # upload directory may already be in place, but buildd-manager
> +        # will set the status to UPLOADING when it's handed off.
> +        build, leaf_name = self.processUploadWithBuildStatus(
> +            BuildStatus.BUILDING)
>          # The build status is not changed
>          self.assertTrue(
>              os.path.exists(os.path.join(self.incoming_folder, leaf_name)))
>          self.assertEquals(BuildStatus.BUILDING, build.status)
> +        self.assertLogContains("Build status is BUILDING. Ignoring.")
> +
> +    def testBuildWithInvalidStatus(self):
> +        # Builds with an invalid (not UPLOADING or BUILDING) status
> +        # should trigger a failure. We've probably raced with
> +        # buildd-manager due to a new and assuredly extra-special bug.
> +        build, leaf_name = self.processUploadWithBuildStatus(
> +            BuildStatus.NEEDSBUILD)
> +        # The build status is not changed, but the upload has moved.
> +        self.assertFalse(
> +            os.path.exists(os.path.join(self.incoming_folder, leaf_name)))
> +        self.assertTrue(
> +            os.path.exists(os.path.join(self.failed_folder, leaf_name)))
> +        self.assertEquals(BuildStatus.NEEDSBUILD, build.status)
>          self.assertLogContains(
> -            "Expected build status to be 'UPLOADING', was BUILDING. "
> -            "Ignoring.")
> +            "Expected build status to be UPLOADING or BUILDING, was "
> +            "NEEDSBUILD.")
>  
>      def testOrderFilenames(self):
>          """orderFilenames sorts _source.changes ahead of other files."""
> 
> === modified file 'lib/lp/archiveuploader/uploadprocessor.py'
> --- lib/lp/archiveuploader/uploadprocessor.py	2015-01-23 17:57:59 +0000
> +++ lib/lp/archiveuploader/uploadprocessor.py	2015-02-19 01:43:44 +0000
> @@ -618,10 +618,21 @@
>          Build uploads always contain a single package per leaf.
>          """
>          logger = BufferLogger()
> -        if self.build.status != BuildStatus.UPLOADING:
> +        if self.build.status == BuildStatus.BUILDING:
> +            # Handoff from buildd-manager isn't complete until the
> +            # status is UPLOADING.
> +            self.processor.log.info("Build status is BUILDING. Ignoring.")
> +            return
> +        elif self.build.status != BuildStatus.UPLOADING:
> +            # buildd-manager should only have given us a directory
> +            # during BUILDING or UPLOADING. Anything else indicates a
> +            # bug, so get the upload out of the queue before the status
> +            # changes to something that might accidentally let it be
> +            # accepted.
>              self.processor.log.warn(
> -                "Expected build status to be 'UPLOADING', was %s. Ignoring." %
> +                "Expected build status to be UPLOADING or BUILDING, was %s.",
>                  self.build.status.name)
> +            self.moveUpload(UploadStatusEnum.FAILED, logger)
>              return
>          try:
>              # The recipe may have been deleted so we need to flag that here
> @@ -659,7 +670,7 @@
>              if recipe_deleted:
>                  # For a deleted recipe, no need to notify that uploading has
>                  # failed - we just log a warning.
> -                self.processor.log.warn(
> +                self.processor.log.info(
>                      "Recipe for build %s was deleted. Ignoring." %
>                      self.upload)
>              else:
> 
> === modified file 'lib/lp/buildmaster/model/buildfarmjobbehaviour.py'
> --- lib/lp/buildmaster/model/buildfarmjobbehaviour.py	2015-02-15 23:33:23 +0000
> +++ lib/lp/buildmaster/model/buildfarmjobbehaviour.py	2015-02-19 01:43:44 +0000
> @@ -220,6 +220,7 @@
>              'Processing finished job %s (%s) from builder %s: %s'
>              % (self.build.build_cookie, self.build.title,
>                 self.build.buildqueue_record.builder.name, status))
> +        build_status = None
>          if status == 'OK':
>              yield self.storeLogFromSlave()
>              # handleSuccess will sometimes perform write operations
> @@ -227,18 +228,26 @@
>              # here and the commit can cause duplicated results. For
>              # example, a BinaryPackageBuild will end up in the upload
>              # queue twice if notify() crashes.
> -            yield self.handleSuccess(slave_status, logger)
> +            build_status = yield self.handleSuccess(slave_status, logger)
>          elif status in fail_status_map:
>              # XXX wgrant: The builder should be set long before here, but
>              # currently isn't.
>              yield self.storeLogFromSlave()
> -            self.build.updateStatus(
> -                fail_status_map[status],
> -                builder=self.build.buildqueue_record.builder,
> -                slave_status=slave_status)
> +            build_status = fail_status_map[status]
>          else:
>              raise BuildDaemonError(
>                  "Build returned unexpected status: %r" % status)
> +
> +        # Set the status and dequeue the build atomically. Setting the
> +        # status to UPLOADING constitutes handoff to process-upload, so
> +        # doing that before we've removed the BuildQueue causes races.
> +
> +        # XXX wgrant: The builder should be set long before here, but
> +        # currently isn't.
> +        self.build.updateStatus(
> +            build_status,
> +            builder=self.build.buildqueue_record.builder,
> +            slave_status=slave_status)
>          if notify:
>              self.build.notify()
>          self.build.buildqueue_record.destroySelf()
> @@ -260,8 +269,7 @@
>          if build.job_type == BuildFarmJobType.PACKAGEBUILD:
>              build = build.buildqueue_record.specific_build
>              if not build.current_source_publication:
> -                build.updateStatus(BuildStatus.SUPERSEDED)
> -                return
> +                defer.returnValue(BuildStatus.SUPERSEDED)
>  
>          self.verifySuccessfulBuild()
>  
> @@ -295,11 +303,6 @@
>              filenames_to_download.append((filemap[filename], out_file_name))
>          yield self._slave.getFiles(filenames_to_download)
>  
> -        # XXX wgrant: The builder should be set long before here, but
> -        # currently isn't.
> -        build.updateStatus(
> -            BuildStatus.UPLOADING, builder=build.buildqueue_record.builder,
> -            slave_status=slave_status)
>          transaction.commit()
>  
>          # Move the directory used to grab the binaries into incoming
> @@ -312,3 +315,5 @@
>          if not os.path.exists(target_dir):
>              os.mkdir(target_dir)
>          os.rename(grab_dir, os.path.join(target_dir, upload_leaf))
> +
> +        defer.returnValue(BuildStatus.UPLOADING)
> 
> === modified file 'lib/lp/buildmaster/tests/test_manager.py'
> --- lib/lp/buildmaster/tests/test_manager.py	2015-02-19 01:43:44 +0000
> +++ lib/lp/buildmaster/tests/test_manager.py	2015-02-19 01:43:44 +0000
> @@ -582,9 +582,7 @@
>          # Mock out the build behaviour's handleSuccess so it doesn't
>          # try to upload things to the librarian or queue.
>          def handleSuccess(self, slave_status, logger):
> -            build.updateStatus(
> -                BuildStatus.UPLOADING, builder, slave_status=slave_status)
> -            transaction.commit()
> +            return BuildStatus.UPLOADING
>          self.patch(
>              BinaryPackageBuildBehaviour, 'handleSuccess', handleSuccess)
>  
> 
> === modified file 'lib/lp/translations/model/translationtemplatesbuildbehaviour.py'
> --- lib/lp/translations/model/translationtemplatesbuildbehaviour.py	2014-06-26 12:33:51 +0000
> +++ lib/lp/translations/model/translationtemplatesbuildbehaviour.py	2015-02-19 01:43:44 +0000
> @@ -101,10 +101,6 @@
>          If this fails for whatever unforeseen reason, a future run will
>          retry it.
>          """
> -        self.build.updateStatus(
> -            BuildStatus.UPLOADING,
> -            builder=self.build.buildqueue_record.builder)
> -        transaction.commit()

I wonder if this ought to be restored so that it's a little clearer what's going on if this state persists for more than a short period of time for some reason.  A TTBJ isn't likely to upload anything that gets into a race with archiveuploader, so being in UPLOADING shouldn't be a problem.

>          logger.debug("Processing successful templates build.")
>          filemap = slave_status.get('filemap')
>          filename = yield self._readTarball(
> @@ -116,7 +112,6 @@
>          # dangerous.
>          if filename is None:
>              logger.error("Build produced no tarball.")
> -            self.build.updateStatus(BuildStatus.FULLYBUILT)
>          else:
>              tarball_file = open(filename)
>              try:
> @@ -129,9 +124,9 @@
>                      self._uploadTarball(
>                          self.build.buildqueue_record.specific_build.branch,
>                          tarball, logger)
> +                    transaction.commit()
>                      logger.debug("Upload complete.")
>              finally:
> -                self.build.updateStatus(BuildStatus.FULLYBUILT)
>                  tarball_file.close()
>                  os.remove(filename)
> -        transaction.commit()
> +        defer.returnValue(BuildStatus.FULLYBUILT)
> 


-- 
https://code.launchpad.net/~wgrant/launchpad/buildd-manager-handoff/+merge/250246
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.