launchpad-reviewers team mailing list archive

["j.c.sackett" <jonathan.sackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/redirects-681034 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #681034 UnknownURLScheme raised running distributionmirror-prober script
  https://bugs.launchpad.net/bugs/681034


Summary
=======

Redirects in the distribution mirror prober can take go to urls that aren't supported. While the entry of url that is unsupported as the initial prober target should be treated as an OOPS, there's nothing launchpad can do about a redirection set by another machine. At best we can log the error and move on, using the existing methods for dealing with url issues and expected errors.

Pre Implementation
==================

Spoke with Curtis Hovey regarding the source of the problem. Spoke with Guilherme Salgado regarding handling of errors and suppressing the OOPS in the prober.

Proposed Fix
============

Rather than simply raising the error, we need to log the error and then escape from the redirect method safely.

Implementation
==============

MirrorCallbacks already have a set of expected errors; this branch creates a new exception descended from UnknownURLScheme called UnknownURLSchemeAfterRedirect and adds it to the list of expected failures. UnknownURLScheme isn't added because it should still trigger a full error if it happens on the initial call of probe--we just don't deal with it after redirection.

In the redirect method, if an UnknownURLScheme is raised, UnknownURLSchemeAfterRedirect is returned in the failure method in its place, and a message is logged.

Tests
=====

bin/test -m lp.registry.tests.test_distributionmirror_prober

Lint
====

make lint output:


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/scripts/distributionmirror_prober.py
  lib/lp/registry/tests/test_distributionmirror_prober.py

./lib/lp/registry/tests/test_distributionmirror_prober.py
     143: E301 expected 1 blank line, found 0
     149: E301 expected 1 blank line, found 0
     180: E301 expected 1 blank line, found 0
     201: E301 expected 1 blank line, found 0
     211: E301 expected 1 blank line, found 0
     219: E301 expected 1 blank line, found 0
     236: E301 expected 1 blank line, found 2
     309: E301 expected 1 blank line, found 0
     400: E301 expected 1 blank line, found 0
     411: E301 expected 1 blank line, found 0
     529: E301 expected 1 blank line, found 0
     537: E301 expected 1 blank line, found 0
     609: E301 expected 1 blank line, found 0
     731: E301 expected 1 blank line, found 0
     736: E301 expected 1 blank line, found 0

All E301s are nested functions.
-- 
https://code.launchpad.net/~jcsackett/launchpad/redirects-681034/+merge/43392
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/redirects-681034 into lp:launchpad.

=== modified file 'lib/lp/registry/scripts/distributionmirror_prober.py'
--- lib/lp/registry/scripts/distributionmirror_prober.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/scripts/distributionmirror_prober.py	2010-12-10 21:18:14 +0000
@@ -327,8 +327,13 @@
             # XXX Guilherme Salgado 2007-04-23 bug=109223:
             # We can't assume url to be absolute here.
             self.setURL(url)
-        except (InfiniteLoopDetected, UnknownURLScheme), e:
+        except (UnknownURLScheme,), e:
+            # Since we've got the UnknownURLScheme after a redirect, we need
+            # to raise it in a form that can be ignored in the layer above.
+            self.failed(UnknownURLSchemeAfterRedirect(url))
+        except (InfiniteLoopDetected,), e:
             self.failed(e)
+
         else:
             self.connect()
 
@@ -399,6 +404,13 @@
                 "URLs: %s" % self.url)
 
 
+class UnknownURLSchemeAfterRedirect(UnknownURLScheme):
+
+    def __str__(self):
+        return ("The mirror prober was redirected to: %s. It doesn't know how"
+                "to check this kind of URL." % self.url)
+
+
 class ArchiveMirrorProberCallbacks(LoggingMixin):
 
     expected_failures = (BadResponseCode, ProberTimeout, ConnectionSkipped)
@@ -540,7 +552,12 @@
 
 class MirrorCDImageProberCallbacks(LoggingMixin):
 
-    expected_failures = (BadResponseCode, ProberTimeout, ConnectionSkipped)
+    expected_failures = (
+        BadResponseCode,
+        ProberTimeout,
+        ConnectionSkipped,
+        UnknownURLSchemeAfterRedirect,
+        )
 
     def __init__(self, mirror, distroseries, flavour, log_file):
         self.mirror = mirror
@@ -735,4 +752,3 @@
         assert port.isdigit()
         port = int(port)
     return scheme, host, port, path
-

=== modified file 'lib/lp/registry/tests/test_distributionmirror_prober.py'
--- lib/lp/registry/tests/test_distributionmirror_prober.py	2010-11-22 13:55:32 +0000
+++ lib/lp/registry/tests/test_distributionmirror_prober.py	2010-12-10 21:18:14 +0000
@@ -59,7 +59,7 @@
     RequestManager,
     restore_http_proxy,
     should_skip_host,
-    UnknownURLScheme,
+    UnknownURLSchemeAfterRedirect,
     )
 from lp.registry.tests.distributionmirror_http_server import (
     DistributionMirrorTestHTTPServer,
@@ -79,8 +79,7 @@
     def tacfile(self):
         return os.path.abspath(os.path.join(
             os.path.dirname(canonical.__file__), os.pardir, os.pardir,
-            'daemons/distributionmirror_http_server.tac'
-            ))
+            'daemons/distributionmirror_http_server.tac'))
 
     @property
     def pidfile(self):
@@ -195,7 +194,7 @@
         prober = RedirectAwareProberFactory(
             'http://localhost:%s/redirect-unknown-url-scheme' % self.port)
         deferred = prober.probe()
-        return self.assertFailure(deferred, UnknownURLScheme)
+        return self.assertFailure(deferred, UnknownURLSchemeAfterRedirect)
 
     def test_200(self):
         d = self._createProberAndProbe(self.urls['200'])
@@ -651,10 +650,16 @@
         # some times.
         self.failUnlessEqual(
             set(self.callbacks.expected_failures),
-            set([BadResponseCode, ProberTimeout, ConnectionSkipped]))
+            set([
+                BadResponseCode,
+                ProberTimeout,
+                ConnectionSkipped,
+                UnknownURLSchemeAfterRedirect,
+                ]))
         exceptions = [BadResponseCode(str(httplib.NOT_FOUND)),
                       ProberTimeout('http://localhost/', 5),
-                      ConnectionSkipped()]
+                      ConnectionSkipped(),
+                      UnknownURLSchemeAfterRedirect('https://localhost')]
         for exception in exceptions:
             failure = self.callbacks.ensureOrDeleteMirrorCDImageSeries(
                 [(defer.FAILURE, Failure(exception))])