launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02806
[Merge] lp:~wgrant/launchpad/squash-cw-protocolerrors into lp:launchpad
William Grant has proposed merging lp:~wgrant/launchpad/squash-cw-protocolerrors into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#719738 Broken remote bug trackers should not cause OOPSes
https://bugs.launchpad.net/bugs/719738
For more details, see:
https://code.launchpad.net/~wgrant/launchpad/squash-cw-protocolerrors/+merge/51680
checkwatches regularly sees down or otherwise broken remote bug trackers. These often manifest as xmlrpc.ProtocolErrors when attempting to detect which ExternalBugTracker to use, which end up logged as OOPSes (bug #719738). This makes up the largest share of checkwatches' OOPSes.
This branch catches them lower, notifies each relevant watch of failure, and logs the error at INFO without OOPSing.
One minor note: bug #390543 is also half-fixed; occurrences are now logged at INFO, as well as having an OOPS logged. It made sense to reuse that code path for handling ProtocolErrors too, but the OOPSes cannot be removed until a later branch significantly refactors tests.
--
https://code.launchpad.net/~wgrant/launchpad/squash-cw-protocolerrors/+merge/51680
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/squash-cw-protocolerrors into lp:launchpad.
=== modified file 'lib/lp/bugs/doc/checkwatches-cli-switches.txt'
--- lib/lp/bugs/doc/checkwatches-cli-switches.txt 2010-12-24 16:39:02 +0000
+++ lib/lp/bugs/doc/checkwatches-cli-switches.txt 2011-03-01 02:37:58 +0000
@@ -107,6 +107,7 @@
DEBUG Enabled by DEFAULT section
INFO Resetting 5 bug watches for bug tracker 'savannah'
INFO Updating 5 watches on bug tracker 'savannah'
+ INFO 'Unsupported Bugtracker' error updating http://savannah.gnu.org/: SAVANE
WARNING ExternalBugtracker for BugTrackerType 'SAVANE' is not known.
(OOPS-...)
INFO 0 watches left to check on bug tracker 'savannah'
=== modified file 'lib/lp/bugs/doc/checkwatches.txt'
--- lib/lp/bugs/doc/checkwatches.txt 2011-02-14 23:34:02 +0000
+++ lib/lp/bugs/doc/checkwatches.txt 2011-03-01 02:37:58 +0000
@@ -52,6 +52,7 @@
DEBUG No watches to update on http://bugzilla.gnome.org/
DEBUG No watches to update on http://bugzilla.gnome.org/bugs
DEBUG No watches to update on https://bugzilla.mozilla.org/
+ INFO 'Unsupported Bugtracker' error updating http://savannah.gnu.org/: SAVANE
WARNING ExternalBugtracker for BugTrackerType 'SAVANE' is not known.
(OOPS-...)
DEBUG No watches to update on http://sourceforge.net/
@@ -273,7 +274,7 @@
... urllib2.urlopen = urlopen
DEBUG No global batch size specified.
INFO Updating 5 watches for 5 bugs on http://bugs.example.com
- INFO Connection timed out when updating ... (OOPS-...)
+ INFO Connection timed out when updating ...
Updating all the watches on a bug tracker
@@ -318,6 +319,7 @@
>>> update_all('savannah', batch_size)
INFO Resetting 1 bug watches for bug tracker 'savannah'
INFO Updating 1 watches on bug tracker 'savannah'
+ INFO 'Unsupported Bugtracker' error updating http://savannah.gnu.org/: SAVANE
WARNING ExternalBugtracker for BugTrackerType 'SAVANE' is not known.
(OOPS-...)
INFO 0 watches left to check on bug tracker 'savannah'
=== modified file 'lib/lp/bugs/scripts/checkwatches/core.py'
--- lib/lp/bugs/scripts/checkwatches/core.py 2010-12-02 16:13:51 +0000
+++ lib/lp/bugs/scripts/checkwatches/core.py 2011-03-01 02:37:58 +0000
@@ -27,6 +27,7 @@
import sys
import threading
import time
+from xmlrpclib import ProtocolError
import pytz
from twisted.internet import reactor
@@ -404,7 +405,7 @@
try:
trackers_and_watches = self._getExternalBugTrackersAndWatches(
bug_tracker, bug_watches_to_update)
- except UnknownBugTrackerTypeError, error:
+ except (UnknownBugTrackerTypeError, ProtocolError), error:
# We update all the bug watches to reflect the fact that
# this error occurred. We also update their last checked
# date to ensure that they don't get checked for another
@@ -414,10 +415,17 @@
with self.transaction:
getUtility(IBugWatchSet).bulkSetError(
bug_watches_to_update, error_type)
- message = (
- "ExternalBugtracker for BugTrackerType '%s' is not "
- "known." % (error.bugtrackertypename))
- self.warning(message)
+ self.logger.info(
+ "'%s' error updating %s: %s" % (
+ error_type.title, bug_tracker.baseurl, error))
+ # This is retained to make tests happy. It will be
+ # removed later when the checkwatches OOPS
+ # infrastructure can be.
+ if isinstance(error, UnknownBugTrackerTypeError):
+ message = (
+ "ExternalBugtracker for BugTrackerType '%s' is not "
+ "known." % (error.bugtrackertypename))
+ self.warning(message)
else:
for remotesystem, bug_watch_batch in trackers_and_watches:
self.updateBugWatches(
=== modified file 'lib/lp/bugs/scripts/checkwatches/tests/test_core.py'
--- lib/lp/bugs/scripts/checkwatches/tests/test_core.py 2010-12-20 03:21:03 +0000
+++ lib/lp/bugs/scripts/checkwatches/tests/test_core.py 2011-03-01 02:37:58 +0000
@@ -7,6 +7,7 @@
from datetime import datetime
import threading
import unittest
+from xmlrpclib import ProtocolError
import transaction
from zope.component import getUtility
@@ -26,6 +27,7 @@
BugTrackerType,
IBugTrackerSet,
)
+from lp.bugs.interfaces.bugwatch import BugWatchActivityStatus
from lp.bugs.scripts import checkwatches
from lp.bugs.scripts.checkwatches.base import (
CheckWatchesErrorUtility,
@@ -164,6 +166,15 @@
self.failIf(remote_system.sync_comments)
+class BrokenCheckwatchesMaster(CheckwatchesMaster):
+
+ error_code = None
+
+ def _getExternalBugTrackersAndWatches(self, bug_tracker, bug_watches):
+ raise ProtocolError(
+ "http://example.com/";, self.error_code, "Borked", "")
+
+
class TestCheckwatchesMaster(TestCaseWithFactory):
layer = LaunchpadZopelessLayer
@@ -176,12 +187,7 @@
# Regression test for bug 497141. KeyErrors raised in
# RemoteBugUpdater.updateRemoteBug() shouldn't cause
# checkwatches to abort.
-
- # Create a couple of bug watches for testing purposes.
- bug_tracker = self.factory.makeBugTracker()
- bug_watches = [
- self.factory.makeBugWatch(bugtracker=bug_tracker)
- for i in range(2)]
+ (bug_tracker, bug_watches) = self.factory.makeBugTrackerWithWatches()
# Use a test XML-RPC transport to ensure no connections happen.
test_transport = TestBugzillaAPIXMLRPCTransport(bug_tracker.baseurl)
@@ -228,6 +234,46 @@
checkwatches.core.suggest_batch_size(remote_system, 99999)
self.failUnlessEqual(247, remote_system.batch_size)
+ def test_xmlrpc_connection_errors_set_activity_properly(self):
+ # HTTP status codes of 502, 503 and 504 indicate connection
+ # errors. An XML-RPC request that fails with one of those is
+ # logged as a connection failure, not an OOPS.
+ master = BrokenCheckwatchesMaster(
+ transaction.manager, logger=BufferLogger())
+ master.error_code = 503
+ (bug_tracker, bug_watches) = self.factory.makeBugTrackerWithWatches(
+ base_url='http://example.com/')
+ transaction.commit()
+ master._updateBugTracker(bug_tracker)
+ for bug_watch in bug_watches:
+ self.assertEquals(
+ BugWatchActivityStatus.CONNECTION_ERROR,
+ bug_watch.last_error_type)
+ self.assertEqual(
+ "INFO 'Connection Error' error updating http://example.com/: "
+ "<ProtocolError for http://example.com/: 503 Borked>\n",
+ master.logger.getLogBuffer())
+
+ def test_xmlrpc_other_errors_set_activity_properly(self):
+ # HTTP status codes that indicate anything other than a
+ # connection error still aren't OOPSes. They are logged as an
+ # unknown error instead.
+ master = BrokenCheckwatchesMaster(
+ transaction.manager, logger=BufferLogger())
+ master.error_code = 403
+ (bug_tracker, bug_watches) = self.factory.makeBugTrackerWithWatches(
+ base_url='http://example.com/')
+ transaction.commit()
+ master._updateBugTracker(bug_tracker)
+ for bug_watch in bug_watches:
+ self.assertEquals(
+ BugWatchActivityStatus.UNKNOWN,
+ bug_watch.last_error_type)
+ self.assertEqual(
+ "INFO 'Unknown' error updating http://example.com/: "
+ "<ProtocolError for http://example.com/: 403 Borked>\n",
+ master.logger.getLogBuffer())
+
class TestUpdateBugsWithLinkedQuestions(unittest.TestCase):
"""Tests for updating bugs with linked questions."""
=== modified file 'lib/lp/bugs/scripts/checkwatches/utilities.py'
--- lib/lp/bugs/scripts/checkwatches/utilities.py 2011-01-19 00:10:48 +0000
+++ lib/lp/bugs/scripts/checkwatches/utilities.py 2011-03-01 02:37:58 +0000
@@ -10,6 +10,7 @@
]
import socket
+from xmlrpclib import ProtocolError
from lp.bugs.externalbugtracker import (
BugNotFound,
@@ -42,6 +43,13 @@
def get_bugwatcherrortype_for_error(error):
"""Return the correct `BugWatchActivityStatus` for a given error."""
+ # ProtocolError is rather generic, but always indicates a server
+ # failure. 502 Bad Gateway, 503 Service Unavailable and 504 Gateway
+ # Timeout are connection errors, while the rest are unknown.
+ if isinstance(error, ProtocolError):
+ if error.errcode in (502, 503, 504):
+ return BugWatchActivityStatus.CONNECTION_ERROR
+
for exc_type, bugwatcherrortype in _exception_to_bugwatcherrortype:
if isinstance(error, exc_type):
return bugwatcherrortype
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py 2011-02-25 14:59:44 +0000
+++ lib/lp/testing/factory.py 2011-03-01 02:37:58 +0000
@@ -1673,6 +1673,14 @@
return getUtility(IBugTrackerSet).ensureBugTracker(
base_url, owner, bugtrackertype, title=title, name=name)
+ def makeBugTrackerWithWatches(self, base_url=None, count=2):
+ """Make a new bug tracker with some watches."""
+ bug_tracker = self.makeBugTracker(base_url=base_url)
+ bug_watches = [
+ self.makeBugWatch(bugtracker=bug_tracker)
+ for i in range(count)]
+ return (bug_tracker, bug_watches)
+
def makeBugTrackerComponentGroup(self, name=None, bug_tracker=None):
"""Make a new bug tracker component group."""
if name is None:
