Merge into devel : robust-process-accepted : Code : Launchpad itself

Status:

Merged

Approved by:

Jelmer Vernooij on 2010-04-13

Approved revision:

no longer in the source branch.

Merged at revision:

not available

Proposed branch:

lp:~jelmer/launchpad/robust-process-accepted

Merge into:

lp:launchpad

Diff against target:

156 lines (+68/-18)

2 files modified

lib/lp/soyuz/scripts/processaccepted.py (+13/-5)
lib/lp/soyuz/tests/test_processaccepted.py (+55/-13)

To merge this branch:

bzr merge lp:~jelmer/launchpad/robust-process-accepted

Medium

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Henning Eggers (community)	code	2010-04-13	Approve on 2010-04-13
Review via email: mp+23312@code.launchpad.net

Commit message

Log OOPSes when encountering errors in process-accepted and skip just the problematic item rather than aborting.

Description of the change

This branch should make process-accepted a bit more robust. It changes it to file an OOPS when it encounters an exception while processing an item in the queue, rather than re-raising that exception, skipping the processing of the rest of the queue and rolling back the transactions.

This should make it impossible for a single problematic package to block process-accepted, as happened about a week ago.

Revision history for this message

Henning Eggers (henninge) wrote on 2010-04-13:

#

Download full text (7.4 KiB)

Am 13.04.2010 13:57, schrieb Jelmer Vernooij:
> This branch should make process-accepted a bit more robust. It changes it to file an OOPS when it encounters an exception while processing an item in the queue, rather than re-raising that exception, skipping the processing of the rest of the queue and rolling back the transactions.
>
> This should make it impossible for a single problematic package to block process-accepted, as happened about a week ago.

Very good, thank you for fixing this!

> === modified file 'lib/lp/soyuz/scripts/processaccepted.py'
> --- lib/lp/soyuz/scripts/processaccepted.py 2010-02-23 10:30:56 +0000
> +++ lib/lp/soyuz/scripts/processaccepted.py 2010-04-13 11:56:59 +0000
> @@ -11,12 +11,16 @@
> 'ProcessAccepted',
> ]
>
> +import sys
> +
> from zope.component import getUtility
>
> from lp.archiveuploader.tagfiles import parse_tagfile_lines
> from lp.bugs.interfaces.bug import IBugSet
> from lp.bugs.interfaces.bugtask import BugTaskStatus
> from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
> +from canonical.launchpad.webapp.errorlog import (
> + ErrorReportingUtility, ScriptRequest)
> from canonical.launchpad.webapp.interfaces import NotFoundError
> from lp.soyuz.interfaces.archive import ArchivePurpose, IArchiveSet
> from lp.registry.interfaces.distribution import IDistributionSet
> @@ -244,11 +248,15 @@
> for queue_item in queue_items:
> try:
> queue_item.realiseUpload(self.logger)

Um, I thought we use en_US as the standard language ... ;-)
Not for you to fix here, though. (realizeUpload)

> - except:
> - self.logger.error(
> - "Failure processing queue_item %d" % (
> - queue_item.id), exc_info=True)
> - raise
> + except Exception:
> + message = "Failure processing queue_item %d" % (
> + queue_item.id)
> + properties = [('error-explanation', message)]
> + request = ScriptRequest(properties)
> + error_utility = ErrorReportingUtility()
> + error_utility.raising(sys.exc_info(), request)
> + self.logger.error('%s (%s)' % (message,
> + request.oopsid))

Please put the newline right after the opening "(", as I think that conforms more to our coding style (similar to a function call in looks).

self.logger.error('%s (%s)' % (
message, request.oopsid))

I have never had to file an OOPS before so I trust that this is the right way to do it.

> else:
> processed_queue_ids.append(queue_item.id)
>
>
> === modified file 'lib/lp/soyuz/tests/test_processaccepted.py'
> --- lib/lp/soyuz/tests/test_processaccepted.py 2010-02-23 10:30:56 +0000
> +++ lib/lp/soyuz/tests/test_processaccepted.py 2010-04-13 11:56:59 +0...

Am 13.04.2010 13:57, schrieb Jelmer Vernooij:
> This branch should make process-accepted a bit more robust. It changes it to file an OOPS when it encounters an exception while processing an item in the queue, rather than re-raising that exception, skipping the processing of the rest of the queue and rolling back the transactions.
> 
> This should make it impossible for a single problematic package to block process-accepted, as happened about a week ago.

Very good, thank you for fixing this!

> === modified file 'lib/lp/soyuz/scripts/processaccepted.py'
> --- lib/lp/soyuz/scripts/processaccepted.py	2010-02-23 10:30:56 +0000
> +++ lib/lp/soyuz/scripts/processaccepted.py	2010-04-13 11:56:59 +0000
> @@ -11,12 +11,16 @@
>      'ProcessAccepted',
>      ]
>  
> +import sys
> +
>  from zope.component import getUtility
>  
>  from lp.archiveuploader.tagfiles import parse_tagfile_lines
>  from lp.bugs.interfaces.bug import IBugSet
>  from lp.bugs.interfaces.bugtask import BugTaskStatus
>  from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
> +from canonical.launchpad.webapp.errorlog import (
> +    ErrorReportingUtility, ScriptRequest)
>  from canonical.launchpad.webapp.interfaces import NotFoundError
>  from lp.soyuz.interfaces.archive import ArchivePurpose, IArchiveSet
>  from lp.registry.interfaces.distribution import IDistributionSet
> @@ -244,11 +248,15 @@
>                      for queue_item in queue_items:
>                          try:
>                              queue_item.realiseUpload(self.logger)

Um, I thought we use en_US as the standard language ... ;-)
Not for you to fix here, though. (realizeUpload)

> -                        except:
> -                            self.logger.error(
> -                                "Failure processing queue_item %d" % (
> -                                    queue_item.id), exc_info=True)
> -                            raise
> +                        except Exception:
> +                            message = "Failure processing queue_item %d" % (
> +                                queue_item.id)
> +                            properties = [('error-explanation', message)]
> +                            request = ScriptRequest(properties)
> +                            error_utility = ErrorReportingUtility()
> +                            error_utility.raising(sys.exc_info(), request)
> +                            self.logger.error('%s (%s)' % (message,
> +                                request.oopsid))

Please put the newline right after the opening "(", as I think that conforms more to our coding style (similar to a function call in looks).

self.logger.error('%s (%s)' % (
                                message, request.oopsid))

I have never had to file an OOPS before so I trust that this is the right way to do it.

>                          else:
>                              processed_queue_ids.append(queue_item.id)
>  
> 
> === modified file 'lib/lp/soyuz/tests/test_processaccepted.py'
> --- lib/lp/soyuz/tests/test_processaccepted.py	2010-02-23 10:30:56 +0000
> +++ lib/lp/soyuz/tests/test_processaccepted.py	2010-04-13 11:56:59 +0000
> @@ -3,13 +3,17 @@
>  
>  """Test process-accepted.py"""
>  
> +from cStringIO import StringIO
> +
>  from zope.component import getUtility
>  
>  from canonical.config import config
>  from canonical.launchpad.scripts import QuietFakeLogger
> +from canonical.launchpad.webapp.errorlog import ErrorReportingUtility
>  from canonical.testing import LaunchpadZopelessLayer
>  
>  from lp.registry.interfaces.distribution import IDistributionSet
> +from lp.registry.interfaces.series import SeriesStatus
>  from lp.soyuz.interfaces.archive import ArchivePurpose
>  from lp.soyuz.interfaces.publishing import PackagePublishingStatus
>  from lp.soyuz.scripts.processaccepted import ProcessAccepted
> @@ -39,13 +43,45 @@
>          script.txn = self.layer.txn
>          return script
>  
> -    def createWaitingAcceptancePackage(self, archive=None):
> +    def createWaitingAcceptancePackage(self, archive=None, distroseries=None,
> +            sourcename=None):
>          """Create some pending publications."""
>          if archive is None:
>              archive = getUtility(
> -                IDistributionSet).getByName('ubuntu').main_archive
> +                IDistributionSet).getByName('ubuntutest').main_archive

OK, we talked about this on IRC. I suggest that you put the "getUtility().getByName" call into the setUp method and use something like "self.distribution" here. If possible, use a factory-made distribution, if not, add a comment in setUp as to why you are using "ubuntutest".

> +        if sourcename is None:
> +            sourcename = self.test_package_name
>          return self.stp.getPubSource(
> -            archive=archive, sourcename=self.test_package_name, spr_only=True)
> +            archive=archive, sourcename=sourcename, distroseries=distroseries,
> +            spr_only=True)
> +
> +    def testRobustness(self):

Actually, TestsStyleGuide proposes to use underscores, like in function names. Also, I'd be more specific about what is going on. How about "test_oops_reporting"?

> +        """Test that a broken package doesn't block the publication of other 
> +        packages."""
> +        # Attempt to upload one source to a frozen series
> +        distroseries = self.factory.makeDistroSeries(
> +            distribution=getUtility(IDistributionSet).getByName('ubuntutest'))

What makes the series frozen, I wonder?

> +        broken_source = self.createWaitingAcceptancePackage(
> +            distroseries=distroseries, sourcename="notaccepted")
> +        distroseries.status = SeriesStatus.SUPPORTED

Oh, it was in a different state before (by default)? I am not very familiar with SeriesStatus, I admit ... May you could make this clearer with a comment or two?

> +        # Also upload some other things
> +        other_source = self.createWaitingAcceptancePackage()
> +        script = self.getScript([])
> +        script.main()
> +
> +        # The other source should be published now
> +        published_main = self.stp.ubuntutest.main_archive.getPublishedSources(
> +            name=self.test_package_name)
> +        self.assertEqual(published_main.count(), 1)
> +
> +        # And an oops should be filed for the first
> +        error_utility = ErrorReportingUtility()
> +        error_report = error_utility.getLastOopsReport()
> +        fp = StringIO()
> +        error_report.write(fp)
> +        error_text = fp.getvalue()
> +        self.failUnless("error-explanation=Failure processing queue_item" 
> +            in error_text)

I started out using "failUnless", too, but was pointed by a reviewer to the fact that "assert" is used far more in our code. You even see it in this file.

Also, it would be nice to be able to use "assertEqual" here to compare the error text completely. Can you not extract the id of the queue item to put into the string? Also, the line wrapping needs to be fixed.

expected_error = (
            "error-explanation=Failure processing queue_item % d" % (
                queue_item_id))
        self.assertEqual(expected_error, error_text)

>  
>      def testAcceptCopyArchives(self):

Ah, I see you have had bad examples ... ;-) I think this is really "test_accept_copy_archives" ?

>          """Test that publications in a copy archive are accepted properly."""

Nothing really bad that needs serious fixing but please do consider applying my suggestions before landing this.

Cheers,
Henning

review: Approve (code)

Launchpad itself

Merge lp:~jelmer/launchpad/robust-process-accepted into lp:launchpad

Commit message

Description of the change

Preview Diff

Subscribers

 === modified file 'lib/lp/soyuz/scripts/processaccepted.py'
 --- lib/lp/soyuz/scripts/processaccepted.py	2010-02-23 10:30:56 +0000
 +++ lib/lp/soyuz/scripts/processaccepted.py	2010-04-13 13:58:19 +0000
@@ -11,12 +11,16 @@
      'ProcessAccepted',
+     ]
++import sys
++
  from zope.component import getUtility
  from lp.archiveuploader.tagfiles import parse_tagfile_lines
  from lp.bugs.interfaces.bug import IBugSet
  from lp.bugs.interfaces.bugtask import BugTaskStatus
  from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
++from canonical.launchpad.webapp.errorlog import (
++    ErrorReportingUtility, ScriptRequest)
  from canonical.launchpad.webapp.interfaces import NotFoundError
  from lp.soyuz.interfaces.archive import ArchivePurpose, IArchiveSet
  from lp.registry.interfaces.distribution import IDistributionSet
@@ -244,11 +248,15 @@
                      for queue_item in queue_items:
                          try:
                              queue_item.realiseUpload(self.logger)
--                        except:
--                            self.logger.error(
--                                "Failure processing queue_item %d" % (
--                                    queue_item.id), exc_info=True)
--                            raise
++                        except Exception:
++                            message = "Failure processing queue_item %d" % (
++                                queue_item.id)
++                            properties = [('error-explanation', message)]
++                            request = ScriptRequest(properties)
++                            error_utility = ErrorReportingUtility()
++                            error_utility.raising(sys.exc_info(), request)
++                            self.logger.error('%s (%s)' % (message,
++                                request.oopsid))
                          else:
                              processed_queue_ids.append(queue_item.id)
 === modified file 'lib/lp/soyuz/tests/test_processaccepted.py'
 --- lib/lp/soyuz/tests/test_processaccepted.py	2010-02-23 10:30:56 +0000
 +++ lib/lp/soyuz/tests/test_processaccepted.py	2010-04-13 13:58:19 +0000
@@ -3,13 +3,14 @@
  """Test process-accepted.py"""
--from zope.component import getUtility
++from cStringIO import StringIO
  from canonical.config import config
  from canonical.launchpad.scripts import QuietFakeLogger
++from canonical.launchpad.webapp.errorlog import ErrorReportingUtility
  from canonical.testing import LaunchpadZopelessLayer
--from lp.registry.interfaces.distribution import IDistributionSet
++from lp.registry.interfaces.series import SeriesStatus
  from lp.soyuz.interfaces.archive import ArchivePurpose
  from lp.soyuz.interfaces.publishing import PackagePublishingStatus
  from lp.soyuz.scripts.processaccepted import ProcessAccepted
@@ -28,34 +29,75 @@
          self.stp = SoyuzTestPublisher()
          self.stp.prepareBreezyAutotest()
          self.test_package_name = "accept-test"
++        self.distro = self.factory.makeDistribution()
      def getScript(self, test_args=None):
          """Return a ProcessAccepted instance."""
          if test_args is None:
              test_args = []
--        test_args.append(self.stp.ubuntutest.name)
++        test_args.append(self.distro.name)
          script = ProcessAccepted("process accepted", test_args=test_args)
          script.logger = QuietFakeLogger()
          script.txn = self.layer.txn
          return script
--    def createWaitingAcceptancePackage(self, archive=None):
++    def createWaitingAcceptancePackage(self, distroseries, archive=None,
++            sourcename=None):
          """Create some pending publications."""
          if archive is None:
--            archive = getUtility(
--                IDistributionSet).getByName('ubuntu').main_archive
++            archive = self.distro.main_archive
++        if sourcename is None:
++            sourcename = self.test_package_name
          return self.stp.getPubSource(
--            archive=archive, sourcename=self.test_package_name, spr_only=True)
--
--    def testAcceptCopyArchives(self):
++            archive=archive, sourcename=sourcename, distroseries=distroseries,
++            spr_only=True)
++
++    def test_robustness(self):
++        """Test that a broken package doesn't block the publication of other
++        packages."""
++        # Attempt to upload one source to a supported series
++        # The record is created first and then the status of the series
++        # is changed from DEVELOPMENT to SUPPORTED, otherwise it's impossible
++        # to create the record
++        distroseries = self.factory.makeDistroSeries(distribution=self.distro)
++        broken_source = self.createWaitingAcceptancePackage(
++            distroseries=distroseries, sourcename="notaccepted")
++        distroseries.status = SeriesStatus.SUPPORTED
++        # Also upload some other things
++        other_distroseries = self.factory.makeDistroSeries(
++            distribution=self.distro)
++        other_source = self.createWaitingAcceptancePackage(
++            distroseries=other_distroseries)
++        script = self.getScript([])
++        self.layer.txn.commit()
++        self.layer.switchDbUser(self.dbuser)
++        script.main()
++
++        # The other source should be published now
++        published_main = self.distro.main_archive.getPublishedSources(
++            name=self.test_package_name)
++        self.assertEqual(published_main.count(), 1)
++
++        # And an oops should be filed for the first
++        error_utility = ErrorReportingUtility()
++        error_report = error_utility.getLastOopsReport()
++        fp = StringIO()
++        error_report.write(fp)
++        error_text = fp.getvalue()
++        expected_error = "error-explanation=Failure processing queue_item"
++        self.assertTrue(expected_error in error_text)
++
++    def test_accept_copy_archives(self):
          """Test that publications in a copy archive are accepted properly."""
          # Upload some pending packages in a copy archive.
++        distroseries = self.factory.makeDistroSeries(distribution=self.distro)
          copy_archive = self.factory.makeArchive(
--            distribution=self.stp.ubuntutest, purpose=ArchivePurpose.COPY)
++            distribution=self.distro, purpose=ArchivePurpose.COPY)
          copy_source = self.createWaitingAcceptancePackage(
--            archive=copy_archive)
++            archive=copy_archive, distroseries=distroseries)
          # Also upload some stuff in the main archive.
--        main_source = self.createWaitingAcceptancePackage()
++        main_source = self.createWaitingAcceptancePackage(
++            distroseries=distroseries)
          # Before accepting, the package should not be published at all.
          published_copy = copy_archive.getPublishedSources(
@@ -72,7 +114,7 @@
          script.main()
          # Packages in main archive should not be accepted and published.
--        published_main = self.stp.ubuntutest.main_archive.getPublishedSources(
++        published_main = self.distro.main_archive.getPublishedSources(
              name=self.test_package_name)
          self.assertEqual(published_main.count(), 0)