Merge lp:~jcsackett/launchpad/redirects-681034 into lp:launchpad

[j.c.sackett]

[r=henninge][ui=none][bug=681034] Changes the UnknownURLScheme handling in the RedirectAwareProberFactory to log and ignore incidents in redirect(), since we have no control over redirects on another machine.

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.

[Henning Eggers (henninge)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hello Jonathan,
thank you for this improvement. The code looks good, I just have a little
suggestion and a question.

 review approve code

Cheers,
Henning

Am 10.12.2010 22:18, schrieb j.c.sackett:
> === 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

[...]

> @@ -540,7 +552,12 @@
>
> class MirrorCDImageProberCallbacks(LoggingMixin):
>
> - expected_failures = (BadResponseCode, ProberTimeout, ConnectionSkipped)
> + expected_failures = (
> + BadResponseCode,
> + ProberTimeout,
> + ConnectionSkipped,
> + UnknownURLSchemeAfterRedirect,
> + )

Thanks for doing the reformatting. AFAICT the order of the content of
expected_failures is irrelevant, so you could as well order them
alphabetically, like imports.

>
> 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

[...]

> @@ -651,10 +650,16 @@
> # some times.
> self.failUnlessEqual(
> set(self.callbacks.expected_failures),
> - set([BadResponseCode, ProberTimeout, ConnectionSkipped]))
> + set([
> + BadResponseCode,
> + ProberTimeout,
> + ConnectionSkipped,
> + UnknownURLSchemeAfterRedirect,
> + ]))

I realize that this is twsited-related code, so there may be a reason why
LaunchpadTestCase(WithFactory) is not use. With it, you could use cool stuff
like "assertContentEqual" and could do without the "set" trick. So you know a
reason that this test uses unittest.TestCase?

> 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))])

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk0GON0ACgkQBT3oW1L17ijBWACgiEnQu82vvgPQQRqY56erjlpg
QY4An3xYVbKokM0kHn54TeYcRy49pZdh
=egAt
-----END PGP SIGNATURE-----

review: Approve (code)

[j.c.sackett (jcsackett)]

On Dec 13, 2010, at 10:19 AM, Henning Eggers wrote:
>> === 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
>
> [...]
>
>> @@ -540,7 +552,12 @@
>>
>> class MirrorCDImageProberCallbacks(LoggingMixin):
>>
>> - expected_failures = (BadResponseCode, ProberTimeout, ConnectionSkipped)
>> + expected_failures = (
>> + BadResponseCode,
>> + ProberTimeout,
>> + ConnectionSkipped,
>> + UnknownURLSchemeAfterRedirect,
>> + )
>
> Thanks for doing the reformatting. AFAICT the order of the content of
> expected_failures is irrelevant, so you could as well order them
> alphabetically, like imports.

Sounds good, I'll reorder them.

>>
>> 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
>
> [...]
>
>> @@ -651,10 +650,16 @@
>> # some times.
>> self.failUnlessEqual(
>> set(self.callbacks.expected_failures),
>> - set([BadResponseCode, ProberTimeout, ConnectionSkipped]))
>> + set([
>> + BadResponseCode,
>> + ProberTimeout,
>> + ConnectionSkipped,
>> + UnknownURLSchemeAfterRedirect,
>> + ]))
>
> I realize that this is twsited-related code, so there may be a reason why
> LaunchpadTestCase(WithFactory) is not use. With it, you could use cool stuff
> like "assertContentEqual" and could do without the "set" trick. So you know a
> reason that this test uses unittest.TestCase?

I don't know, actually; but given how fragile the whole prober testing infrastructure seems, I was leery to change things. I'll try that update though--if everything works, good times.

=== 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-13 15:47:54 +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,
+        ConnectionSkipped,
+        ProberTimeout,
+        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-13 15:47:54 +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))])