Launchpad itself

Merge lp://staging/~thumper/launchpad/more-careful-network-service-usage into lp://staging/launchpad/db-devel

  1. more-careful-network-service-usage
  2. Merge into db-devel
Proposed by Tim Penhey
Status: Merged
Merged at revision: not available
Proposed branch: lp://staging/~thumper/launchpad/more-careful-network-service-usage
Merge into: lp://staging/launchpad/db-devel
Diff against target: 465 lines (+224/-17)
10 files modified
lib/canonical/launchpad/webapp/errorlog.py (+9/-1)
lib/canonical/launchpad/webapp/interfaces.py (+5/-0)
lib/lp/code/model/branchmergeproposaljob.py (+9/-0)
lib/lp/code/model/tests/test_branchmergeproposaljobs.py (+98/-1)
lib/lp/code/model/tests/test_diff.py (+2/-4)
lib/lp/codehosting/scanner/tests/test_bzrsync.py (+4/-2)
lib/lp/services/job/runner.py (+22/-7)
lib/lp/services/job/tests/test_runner.py (+63/-0)
lib/lp/testing/__init__.py (+11/-1)
lib/lp/testing/tests/test_fixture.py (+1/-1)
To merge this branch: bzr merge lp://staging/~thumper/launchpad/more-careful-network-service-usage
Reviewer Review Type Date Requested Status
Michael Hudson-Doyle Approve
Björn Tillenius (community) Approve
Review via email: mp+24469@code.staging.launchpad.net

Commit message

Add missing operation descriptions for the jobs, and catch errors that might arise during emailing users about errors.

Description of the change

This branch adds a default getOperationDescription to the BaseRunnableJob, and also adds extra exception handling around the handing of other exceptions.

If the run job raises an error, we attempt to notify the recipients. However, as we have found with staging, that can fail. The extra try/except block catches this failures to notify about failures, but it just logs the error and makes an oops.

Extra tests were added for the other job types to make sure they have a sensible getOperationDescription.

To post a comment you must log in.
Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

As mentioned on IRC, please us logger.exception() instead of using logger.error(e).

Otherwise fine.

review: Approve
Revision history for this message
Björn Tillenius (bjornt) wrote :

On Fri, Apr 30, 2010 at 05:26:29AM -0000, Tim Penhey wrote:
> === modified file 'lib/lp/services/job/runner.py'
> --- lib/lp/services/job/runner.py 2010-04-06 03:37:16 +0000
> +++ lib/lp/services/job/runner.py 2010-04-30 05:22:33 +0000

> @@ -181,13 +184,24 @@
> with self.error_utility.oopsMessage(
> dict(job.getOopsVars())):
> try:
> - self.logger.debug('Running %r', job)
> - self.runJob(job)
> - except job.user_error_types, e:
> - job.notifyUserError(e)
> - except Exception:
> + try:
> + self.logger.debug('Running %r', job)
> + self.runJob(job)
> + except job.user_error_types, e:
> + job.notifyUserError(e)
> + except Exception:
> + info = sys.exc_info()
> + return self._doOops(job, info)
> + except Exception, e:
> + # This only happens if sending attempting to notify users
> + # about errors fails for some reason (like a misconfigured
> + # email server).
> + self.logger.exception(e)
> info = sys.exc_info()
> - return self._doOops(job, info)
> + self.error_utility.raising(info)
> + oops = self.error_utility.getLastOopsReport()
> + # Returning the oops says something went wrong.
> + return oops

As discussed on IRC, this new exception handling is untested. To avoid
things breaking, and to avoid people getting tempted into refactoring
the outer exception handling into using self._doOops(), we need two
tests; one where job.notifyUserError() errors out, and one where
job.notifyOops() errors out.

Since it's EOD for you now, I'd be willing to let this branch land
without tests (since it won't break anything not already broken), so
that you can QA it more easily on Monday. Given that you promise to
write the tests on Monday, of course.

    vote approve

--
Björn Tillenius | https://launchpad.net/~bjornt

review: Approve
Revision history for this message
Tim Penhey (thumper) wrote :

I've updated the test cases for the job runner to confirm that the
errors are handled as expected and that oopses are generated
when we expect them to be.

As a part of this change I have updated the base launchpad TestCase to record
oopses that are generated during the test execution.

These are available as self.oopses and is a list.

This is done by adding an object event to the global error reporting utility
class so that when new oopses are generated and saved it notifies listeners.
The test case starts listening in setUp and stops listening in tearDown thanks
to Fixture code that was in the branch scanner. This code has been moved into
the testing module as no code was currently using it (due to a past change in
the scanner). The tests for the fixtures were also moved into the testing
module.

This also fixes bug 567689, and I did a drive by fix of a spurious test failure,
bug 567257.

updated.diff
=== modified file 'lib/canonical/launchpad/webapp/errorlog.py'
--- lib/canonical/launchpad/webapp/errorlog.py 2009-11-26 21:25:17 +0000
+++ lib/canonical/launchpad/webapp/errorlog.py 2010-05-02 22:58:46 +0000
@@ -20,7 +20,9 @@
20import urllib20import urllib
2121
22import pytz22import pytz
23from zope.component.interfaces import ObjectEvent
23from zope.error.interfaces import IErrorReportingUtility24from zope.error.interfaces import IErrorReportingUtility
25from zope.event import notify
24from zope.exceptions.exceptionformatter import format_exception26from zope.exceptions.exceptionformatter import format_exception
25from zope.interface import implements27from zope.interface import implements
26from zope.publisher.interfaces.xmlrpc import IXMLRPCRequest28from zope.publisher.interfaces.xmlrpc import IXMLRPCRequest
@@ -35,7 +37,7 @@
35 get_request_statements, get_request_duration,37 get_request_statements, get_request_duration,
36 soft_timeout_expired)38 soft_timeout_expired)
37from canonical.launchpad.webapp.interfaces import (39from canonical.launchpad.webapp.interfaces import (
38 IErrorReport, IErrorReportRequest)40 IErrorReport, IErrorReportEvent, IErrorReportRequest)
39from canonical.launchpad.webapp.opstats import OpStats41from canonical.launchpad.webapp.opstats import OpStats
4042
41UTC = pytz.utc43UTC = pytz.utc
@@ -130,6 +132,11 @@
130 *(int(elem) for elem in re.findall('[0-9]+', datestring)[:7]))132 *(int(elem) for elem in re.findall('[0-9]+', datestring)[:7]))
131133
132134
135class ErrorReportEvent(ObjectEvent):
136 """A new error report has been created."""
137 implements(IErrorReportEvent)
138
139
133class ErrorReport:140class ErrorReport:
134 implements(IErrorReport)141 implements(IErrorReport)
135142
@@ -426,6 +433,7 @@
426 if self.copy_to_zlog:433 if self.copy_to_zlog:
427 self._do_copy_to_zlog(434 self._do_copy_to_zlog(
428 entry.time, entry.type, entry.url, info, entry.id)435 entry.time, entry.type, entry.url, info, entry.id)
436 notify(ErrorReportEvent(entry))
429437
430 def _makeErrorReport(self, info, request=None, now=None,438 def _makeErrorReport(self, info, request=None, now=None,
431 informational=False):439 informational=False):
432440
=== modified file 'lib/canonical/launchpad/webapp/interfaces.py'
--- lib/canonical/launchpad/webapp/interfaces.py 2010-04-01 17:20:31 +0000
+++ lib/canonical/launchpad/webapp/interfaces.py 2010-05-02 22:56:22 +0000
@@ -8,6 +8,7 @@
8import logging8import logging
99
10import zope.app.publication.interfaces10import zope.app.publication.interfaces
11from zope.component.interfaces import IObjectEvent
11from zope.interface import Interface, Attribute, implements12from zope.interface import Interface, Attribute, implements
12from zope.app.security.interfaces import (13from zope.app.security.interfaces import (
13 IAuthentication, IPrincipal, IPrincipalSource)14 IAuthentication, IPrincipal, IPrincipalSource)
@@ -674,6 +675,10 @@
674 """675 """
675676
676677
678class IErrorReportEvent(IObjectEvent):
679 """A new error report has been created."""
680
681
677class IErrorReport(Interface):682class IErrorReport(Interface):
678 id = TextLine(description=u"The name of this error report.")683 id = TextLine(description=u"The name of this error report.")
679 type = TextLine(description=u"The type of the exception that occurred.")684 type = TextLine(description=u"The type of the exception that occurred.")
680685
=== modified file 'lib/lp/code/model/tests/test_diff.py'
--- lib/lp/code/model/tests/test_diff.py 2010-04-26 00:24:26 +0000
+++ lib/lp/code/model/tests/test_diff.py 2010-05-03 00:02:07 +0000
@@ -260,10 +260,10 @@
260 def test_fromFile_withError(self):260 def test_fromFile_withError(self):
261 # If the diff is formatted such that generating the diffstat fails, we261 # If the diff is formatted such that generating the diffstat fails, we
262 # want to record an oops but continue.262 # want to record an oops but continue.
263 last_oops_id = errorlog.globalErrorUtility.lastid
264 diff_bytes = "not a real diff"263 diff_bytes = "not a real diff"
265 diff = Diff.fromFile(StringIO(diff_bytes), len(diff_bytes))264 diff = Diff.fromFile(StringIO(diff_bytes), len(diff_bytes))
266 self.assertNotEqual(last_oops_id, errorlog.globalErrorUtility.lastid)265 oops = self.oopses[0]
266 self.assertEqual('MalformedPatchHeader', oops.type)
267 self.assertIs(None, diff.diffstat)267 self.assertIs(None, diff.diffstat)
268 self.assertIs(None, diff.added_lines_count)268 self.assertIs(None, diff.added_lines_count)
269 self.assertIs(None, diff.removed_lines_count)269 self.assertIs(None, diff.removed_lines_count)
270270
=== modified file 'lib/lp/services/job/tests/test_runner.py'
--- lib/lp/services/job/tests/test_runner.py 2010-04-06 00:01:24 +0000
+++ lib/lp/services/job/tests/test_runner.py 2010-05-03 01:56:20 +0000
@@ -76,6 +76,37 @@
76 raise RaisingJobException(self.message)76 raise RaisingJobException(self.message)
7777
7878
79class RaisingJobUserError(NullJob):
80 """A job that raises when it runs."""
81
82 user_error_types = (RaisingJobException, )
83
84 def run(self):
85 raise RaisingJobException(self.message)
86
87
88class RaisingJobRaisingNotifyOops(NullJob):
89 """A job that raises when it runs, and when getting oops recipients."""
90
91 def run(self):
92 raise RaisingJobException(self.message)
93
94 def notifyOops(self, oops):
95 raise RaisingJobException('oops notifying oops')
96
97
98class RaisingJobRaisingNotifyUserError(NullJob):
99 """A job that raises when it runs, and when notifying user errors."""
100
101 user_error_types = (RaisingJobException, )
102
103 def run(self):
104 raise RaisingJobException(self.message)
105
106 def notifyUserError(self, error):
107 raise RaisingJobException('oops notifying users')
108
109
79class TestJobRunner(TestCaseWithFactory):110class TestJobRunner(TestCaseWithFactory):
80 """Ensure JobRunner behaves as expected."""111 """Ensure JobRunner behaves as expected."""
81112
@@ -254,6 +285,38 @@
254 transaction.abort()285 transaction.abort()
255 self.assertEqual(JobStatus.FAILED, job.job.status)286 self.assertEqual(JobStatus.FAILED, job.job.status)
256287
288 def test_runJobHandleErrors_oops_generated(self):
289 """The handle errors method records an oops for raised errors."""
290 job = RaisingJob('boom')
291 runner = JobRunner([job])
292 runner.runJobHandleError(job)
293 self.assertEqual(1, len(self.oopses))
294
295 def test_runJobHandleErrors_user_error_no_oops(self):
296 """If the job raises a user error, there is no oops."""
297 job = RaisingJobUserError('boom')
298 runner = JobRunner([job])
299 runner.runJobHandleError(job)
300 self.assertEqual(0, len(self.oopses))
301
302 def test_runJobHandleErrors_oops_generated_notify_fails(self):
303 """A second oops is logged if the notification of the oops fails."""
304 job = RaisingJobRaisingNotifyOops('boom')
305 runner = JobRunner([job])
306 runner.runJobHandleError(job)
307 self.assertEqual(2, len(self.oopses))
308
309 def test_runJobHandleErrors_oops_generated_user_notify_fails(self):
310 """A second oops is logged if the notification of the oops fails.
311
312 In this test case the error is a user expected error, so the
313 notifyUserError is called, and in this case the notify raises too.
314 """
315 job = RaisingJobRaisingNotifyUserError('boom')
316 runner = JobRunner([job])
317 runner.runJobHandleError(job)
318 self.assertEqual(1, len(self.oopses))
319
257320
258class StuckJob(BaseRunnableJob):321class StuckJob(BaseRunnableJob):
259 """Simulation of a job that stalls."""322 """Simulation of a job that stalls."""
260323
=== modified file 'lib/lp/testing/__init__.py'
--- lib/lp/testing/__init__.py 2010-04-27 04:46:58 +0000
+++ lib/lp/testing/__init__.py 2010-05-03 00:01:25 +0000
@@ -72,7 +72,7 @@
7272
73from windmill.authoring import WindmillTestClient73from windmill.authoring import WindmillTestClient
7474
75from zope.component import getUtility75from zope.component import adapter, getUtility
76import zope.event76import zope.event
77from zope.interface.verify import verifyClass, verifyObject77from zope.interface.verify import verifyClass, verifyObject
78from zope.security.proxy import (78from zope.security.proxy import (
@@ -81,6 +81,7 @@
8181
82from canonical.launchpad.webapp import errorlog82from canonical.launchpad.webapp import errorlog
83from canonical.config import config83from canonical.config import config
84from canonical.launchpad.webapp.errorlog import ErrorReportEvent
84from canonical.launchpad.webapp.interaction import ANONYMOUS85from canonical.launchpad.webapp.interaction import ANONYMOUS
85from canonical.launchpad.webapp.interfaces import ILaunchBag86from canonical.launchpad.webapp.interfaces import ILaunchBag
86from canonical.launchpad.windmill.testing import constants87from canonical.launchpad.windmill.testing import constants
@@ -95,6 +96,7 @@
95from lp.testing._tales import test_tales96from lp.testing._tales import test_tales
96from lp.testing._webservice import (97from lp.testing._webservice import (
97 launchpadlib_credentials_for, launchpadlib_for, oauth_access_token_for)98 launchpadlib_credentials_for, launchpadlib_for, oauth_access_token_for)
99from lp.testing.fixture import ZopeEventHandlerFixture
98100
99# zope.exception demands more of frame objects than twisted.python.failure101# zope.exception demands more of frame objects than twisted.python.failure
100# provides in its fake frames. This is enough to make it work with them102# provides in its fake frames. This is enough to make it work with them
@@ -382,6 +384,14 @@
382 testtools.TestCase.setUp(self)384 testtools.TestCase.setUp(self)
383 from lp.testing.factory import ObjectFactory385 from lp.testing.factory import ObjectFactory
384 self.factory = ObjectFactory()386 self.factory = ObjectFactory()
387 # Record the oopses generated during the test run.
388 self.oopses = []
389 self.installFixture(ZopeEventHandlerFixture(self._recordOops))
390
391 @adapter(ErrorReportEvent)
392 def _recordOops(self, event):
393 """Add the oops to the testcase's list."""
394 self.oopses.append(event.object)
385395
386 def assertStatementCount(self, expected_count, function, *args, **kwargs):396 def assertStatementCount(self, expected_count, function, *args, **kwargs):
387 """Assert that the expected number of SQL statements occurred.397 """Assert that the expected number of SQL statements occurred.
388398
=== renamed file 'lib/lp/codehosting/scanner/fixture.py' => 'lib/lp/testing/fixture.py'
=== renamed file 'lib/lp/codehosting/scanner/tests/test_fixture.py' => 'lib/lp/testing/tests/test_fixture.py'
--- lib/lp/codehosting/scanner/tests/test_fixture.py 2009-06-30 16:56:07 +0000
+++ lib/lp/testing/tests/test_fixture.py 2010-05-02 23:39:28 +0000
@@ -9,7 +9,7 @@
99
10from zope.interface import implements10from zope.interface import implements
1111
12from lp.codehosting.scanner.fixture import (12from lp.testing.fixture import (
13 Fixtures, FixtureWithCleanup, IFixture, run_with_fixture, with_fixture)13 Fixtures, FixtureWithCleanup, IFixture, run_with_fixture, with_fixture)
14from lp.testing import TestCase14from lp.testing import TestCase
1515
Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

All the new stuff looks great to me. There *may* be oops-related APIs that aren't needed any more, but that can wait.

review: Approve
Revision history for this message
Björn Tillenius (bjornt) wrote :

Those changes look good. firing off an event is a good idea, and the tests look good.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to status/vote changes: