Launchpad itself

Merge lp:~sinzui/launchpad/prf-connection-0 into lp:launchpad

  1. prf-connection-0
  2. Merge into devel
Proposed by Curtis Hovey
Status: Merged
Merged at revision: 12371
Proposed branch: lp:~sinzui/launchpad/prf-connection-0
Merge into: lp:launchpad
Diff against target: 257 lines (+40/-47)
2 files modified
lib/lp/registry/scripts/productreleasefinder/walker.py (+10/-4)
lib/lp/registry/tests/test_prf_walker.py (+30/-43)
To merge this branch: bzr merge lp:~sinzui/launchpad/prf-connection-0
Reviewer Review Type Date Requested Status
Brad Crittenden (community) code Approve
Benji York (community) code* Approve
Review via email: mp+49456@code.launchpad.net

Description of the change

Recover from an open() failure when the product-release-finder calls walk().

    Launchpad bug: https://bugs.launchpad.net/bugs/717363
    Pre-implementation: no one
    Test command: ./bin/test -vv -t test_prf_walker

The product release finder failed on the first use the an FTPWalker. This is
probably an operational issue where the site or the port is blocked.
Regardless of the connectivity issue, the PRF should not fail.

From the prf log:
2011-02-11 07:22:36 INFO Connecting to ftp.gnu.org
2011-02-11 07:22:36 ERROR Unhandled exception

From the tb in the librarian
  File "/srv/launchpad.net/production/launchpad-rev-12335/lib/lp/registry/scripts/productreleasefinder/walker.py", line 126, in walk
    self.open()
  File "/srv/launchpad.net/production/launchpad-rev-12335/lib/lp/registry/scripts/productreleasefinder/walker.py", line 206, in open
    self.ftp.connect(self.host)
  File "/usr/lib/python2.6/ftplib.py", line 131, in connect
    self.sock = socket.create_connection((self.host, self.port), self.timeout)
  File "/usr/lib/python2.6/socket.py", line 514, in create_connection
    raise error, msg
error: [Errno 111] Connection refused

--------------------------------------------------------------------

RULES

    * Catch the exception raise by self.open(), log the issue and return
      early.
    * ADDENDUM: The local imports are not needed in the tests now that
      cyclic import issues were fixed.

QA

    If ftp.gnu.org is blocking Lp:
        * Run the product-release finder on staging and verify that the PRF
          completes. (The production instance is dying in the first 5 minutes).
    otherwise:
        * Hope for the best.

LINT

    lib/lp/registry/scripts/productreleasefinder/walker.py
    lib/lp/registry/tests/test_prf_walker.py

^ lint hates the long lines in the test data. I can clean this up, but I think
it will make the test more difficult to read, so I chose it ignore it.

IMPLEMENTATION

I removed the local import from the test. I added a new test to instrument
an exception from open(). The prf often checks for (IOError, socket.error)
though I believe that in the known case, only socket.error will be raised.
I also fixed some deprecated uses of raise reported by lint.
    lib/lp/registry/scripts/productreleasefinder/walker.py
    lib/lp/registry/tests/test_prf_walker.py

To post a comment you must log in.
Revision history for this message
Benji York (benji) wrote :

Approved* (Brad will have to review my review since I'm doing mentored
reviews.)

The test (like the rest of the branch) is nice and clean. I almost
suggested using list(walker) instead of the (apparently) do-nothing
loop, but I'm not sure if using list() is just clearer to me and most
people would understand the intent of the current code better.

I especially like the fact that the WalkerBase import can be moved to
the top of the test module. Much nicer now.

review: Approve (code*)
Revision history for this message
Brad Crittenden (bac) wrote :

I agree with Benji's suggestion to use list() as the dummy loop looks a bit silly. Using list() with a comment is even better:

# Listify the walker generator, which will log the expected error.
list(walker)

And thanks a zillion for the clean up!

review: Approve (code)
Revision history for this message
Robert Collins (lifeless) wrote :

You should either log an error (if we want OOPS for these - we
probably don't as they are user data : but how will users debug a
fail? - I think we need a new bug for that), or log the exception in
the message so that someone reading the log can determine the error it
experienced.

Revision history for this message
Curtis Hovey (sinzui) wrote :

We do not want to log an error for user data: The issue of getting feedback is covered in bug 70524. It is out of scope for this issue.

Revision history for this message
Robert Collins (lifeless) wrote :

On Sat, Feb 12, 2011 at 10:29 AM, Curtis Hovey
<email address hidden> wrote:
> We do not want to log an error for user data: The issue of getting feedback is covered in bug 70524. It is out of scope for this issue.

Agreed; am glad there is a bug already. But we should log /what the
error was/ not just that there was an error. e.g

except (IOError, OSError), e:
   log("failure %s" % e)

(done minimally here)

Revision history for this message
Curtis Hovey (sinzui) wrote :

Agreed and done.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/registry/scripts/productreleasefinder/walker.py'
--- lib/lp/registry/scripts/productreleasefinder/walker.py 2010-08-20 20:31:18 +0000
+++ lib/lp/registry/scripts/productreleasefinder/walker.py 2011-02-12 00:03:40 +0000
@@ -1,4 +1,4 @@
1# Copyright 2009 Canonical Ltd. This software is licensed under the1# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).2# GNU Affero General Public License version 3 (see the file LICENSE).
33
4"""HTTP and FTP walker.4"""HTTP and FTP walker.
@@ -92,7 +92,7 @@
92 (scheme, netloc, path, query, fragment) \92 (scheme, netloc, path, query, fragment) \
93 = urlsplit(base, self.URL_SCHEMES[0], self.FRAGMENTS)93 = urlsplit(base, self.URL_SCHEMES[0], self.FRAGMENTS)
94 if scheme not in self.URL_SCHEMES:94 if scheme not in self.URL_SCHEMES:
95 raise WalkerError, "Can't handle %s scheme" % scheme95 raise WalkerError("Can't handle %s scheme" % scheme)
96 self.scheme = scheme96 self.scheme = scheme
97 self.full_netloc = netloc97 self.full_netloc = netloc
9898
@@ -123,7 +123,12 @@
123 Yields (dirpath, dirnames, filenames) for each path under the base;123 Yields (dirpath, dirnames, filenames) for each path under the base;
124 dirnames can be modified as with os.walk.124 dirnames can be modified as with os.walk.
125 """125 """
126 self.open()126 try:
127 self.open()
128 except (IOError, socket.error), e:
129 self.log.info("Could not connect to %s" % self.base)
130 self.log.info("Failure: %s" % e)
131 return
127132
128 subdirs = [self.path]133 subdirs = [self.path]
129 while len(subdirs):134 while len(subdirs):
@@ -423,7 +428,8 @@
423 elif scheme in ["file"]:428 elif scheme in ["file"]:
424 return os.walk(path)429 return os.walk(path)
425 else:430 else:
426 raise WalkerError, "Unknown scheme: %s" % scheme431 raise WalkerError("Unknown scheme: %s" % scheme)
432
427433
428def combine_url(base, subdir, filename):434def combine_url(base, subdir, filename):
429 """Combine a URL from the three parts returned by walk()."""435 """Combine a URL from the three parts returned by walk()."""
430436
=== modified file 'lib/lp/registry/tests/test_prf_walker.py'
--- lib/lp/registry/tests/test_prf_walker.py 2010-08-20 20:31:18 +0000
+++ lib/lp/registry/tests/test_prf_walker.py 2011-02-12 00:03:40 +0000
@@ -1,4 +1,4 @@
1# Copyright 2009 Canonical Ltd. This software is licensed under the1# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).2# GNU Affero General Public License version 3 (see the file LICENSE).
33
4"""Tests for lp.registry.scripts.productreleasefinder.walker."""4"""Tests for lp.registry.scripts.productreleasefinder.walker."""
@@ -8,8 +8,10 @@
8import unittest8import unittest
9import urlparse9import urlparse
1010
11
11from canonical.lazr.utils import safe_hasattr12from canonical.lazr.utils import safe_hasattr
12from canonical.testing import reset_logging13from canonical.testing import reset_logging
14from lp.registry.scripts.productreleasefinder.walker import WalkerBase
13from lp.testing import TestCase15from lp.testing import TestCase
1416
1517
@@ -17,16 +19,12 @@
1719
18 def testCreatesDefaultLogger(self):20 def testCreatesDefaultLogger(self):
19 """WalkerBase creates a default logger."""21 """WalkerBase creates a default logger."""
20 from lp.registry.scripts.productreleasefinder.walker import (
21 WalkerBase)
22 from logging import Logger22 from logging import Logger
23 w = WalkerBase("/")23 w = WalkerBase("/")
24 self.failUnless(isinstance(w.log, Logger))24 self.failUnless(isinstance(w.log, Logger))
2525
26 def testCreatesChildLogger(self):26 def testCreatesChildLogger(self):
27 """WalkerBase creates a child logger if given a parent."""27 """WalkerBase creates a child logger if given a parent."""
28 from lp.registry.scripts.productreleasefinder.walker import (
29 WalkerBase)
30 from logging import getLogger28 from logging import getLogger
31 parent = getLogger("foo")29 parent = getLogger("foo")
32 w = WalkerBase("/", log_parent=parent)30 w = WalkerBase("/", log_parent=parent)
@@ -37,29 +35,21 @@
3735
38 def testSetsBase(self):36 def testSetsBase(self):
39 """WalkerBase sets the base property."""37 """WalkerBase sets the base property."""
40 from lp.registry.scripts.productreleasefinder.walker import (
41 WalkerBase)
42 w = WalkerBase("ftp://localhost/")38 w = WalkerBase("ftp://localhost/")
43 self.assertEquals(w.base, "ftp://localhost/")39 self.assertEquals(w.base, "ftp://localhost/")
4440
45 def testSetsScheme(self):41 def testSetsScheme(self):
46 """WalkerBase sets the scheme property."""42 """WalkerBase sets the scheme property."""
47 from lp.registry.scripts.productreleasefinder.walker import (
48 WalkerBase)
49 w = WalkerBase("ftp://localhost/")43 w = WalkerBase("ftp://localhost/")
50 self.assertEquals(w.scheme, "ftp")44 self.assertEquals(w.scheme, "ftp")
5145
52 def testSetsHost(self):46 def testSetsHost(self):
53 """WalkerBase sets the host property."""47 """WalkerBase sets the host property."""
54 from lp.registry.scripts.productreleasefinder.walker import (
55 WalkerBase)
56 w = WalkerBase("ftp://localhost/")48 w = WalkerBase("ftp://localhost/")
57 self.assertEquals(w.host, "localhost")49 self.assertEquals(w.host, "localhost")
5850
59 def testNoScheme(self):51 def testNoScheme(self):
60 """WalkerBase works when given a URL with no scheme."""52 """WalkerBase works when given a URL with no scheme."""
61 from lp.registry.scripts.productreleasefinder.walker import (
62 WalkerBase)
63 w = WalkerBase("/")53 w = WalkerBase("/")
64 self.assertEquals(w.host, "")54 self.assertEquals(w.host, "")
6555
@@ -71,45 +61,33 @@
7161
72 def testUnescapesHost(self):62 def testUnescapesHost(self):
73 """WalkerBase unescapes the host portion."""63 """WalkerBase unescapes the host portion."""
74 from lp.registry.scripts.productreleasefinder.walker import (
75 WalkerBase)
76 w = WalkerBase("ftp://local%40host/")64 w = WalkerBase("ftp://local%40host/")
77 self.assertEquals(w.host, "local@host")65 self.assertEquals(w.host, "local@host")
7866
79 def testNoUsername(self):67 def testNoUsername(self):
80 """WalkerBase stores None when there is no username."""68 """WalkerBase stores None when there is no username."""
81 from lp.registry.scripts.productreleasefinder.walker import (
82 WalkerBase)
83 w = WalkerBase("ftp://localhost/")69 w = WalkerBase("ftp://localhost/")
84 self.assertEquals(w.user, None)70 self.assertEquals(w.user, None)
8571
86 def testUsername(self):72 def testUsername(self):
87 """WalkerBase splits out the username from the host portion."""73 """WalkerBase splits out the username from the host portion."""
88 from lp.registry.scripts.productreleasefinder.walker import (
89 WalkerBase)
90 w = WalkerBase("ftp://scott@localhost/")74 w = WalkerBase("ftp://scott@localhost/")
91 self.assertEquals(w.user, "scott")75 self.assertEquals(w.user, "scott")
92 self.assertEquals(w.host, "localhost")76 self.assertEquals(w.host, "localhost")
9377
94 def testUnescapesUsername(self):78 def testUnescapesUsername(self):
95 """WalkerBase unescapes the username portion."""79 """WalkerBase unescapes the username portion."""
96 from lp.registry.scripts.productreleasefinder.walker import (
97 WalkerBase)
98 w = WalkerBase("ftp://scott%3awibble@localhost/")80 w = WalkerBase("ftp://scott%3awibble@localhost/")
99 self.assertEquals(w.user, "scott:wibble")81 self.assertEquals(w.user, "scott:wibble")
100 self.assertEquals(w.host, "localhost")82 self.assertEquals(w.host, "localhost")
10183
102 def testNoPassword(self):84 def testNoPassword(self):
103 """WalkerBase stores None when there is no password."""85 """WalkerBase stores None when there is no password."""
104 from lp.registry.scripts.productreleasefinder.walker import (
105 WalkerBase)
106 w = WalkerBase("ftp://scott@localhost/")86 w = WalkerBase("ftp://scott@localhost/")
107 self.assertEquals(w.passwd, None)87 self.assertEquals(w.passwd, None)
10888
109 def testPassword(self):89 def testPassword(self):
110 """WalkerBase splits out the password from the username."""90 """WalkerBase splits out the password from the username."""
111 from lp.registry.scripts.productreleasefinder.walker import (
112 WalkerBase)
113 w = WalkerBase("ftp://scott:wibble@localhost/")91 w = WalkerBase("ftp://scott:wibble@localhost/")
114 self.assertEquals(w.user, "scott")92 self.assertEquals(w.user, "scott")
115 self.assertEquals(w.passwd, "wibble")93 self.assertEquals(w.passwd, "wibble")
@@ -117,8 +95,6 @@
11795
118 def testUnescapesPassword(self):96 def testUnescapesPassword(self):
119 """WalkerBase unescapes the password portion."""97 """WalkerBase unescapes the password portion."""
120 from lp.registry.scripts.productreleasefinder.walker import (
121 WalkerBase)
122 w = WalkerBase("ftp://scott:wibble%20wobble@localhost/")98 w = WalkerBase("ftp://scott:wibble%20wobble@localhost/")
123 self.assertEquals(w.user, "scott")99 self.assertEquals(w.user, "scott")
124 self.assertEquals(w.passwd, "wibble wobble")100 self.assertEquals(w.passwd, "wibble wobble")
@@ -126,43 +102,31 @@
126102
127 def testPathOnly(self):103 def testPathOnly(self):
128 """WalkerBase stores the path if that's all there is."""104 """WalkerBase stores the path if that's all there is."""
129 from lp.registry.scripts.productreleasefinder.walker import (
130 WalkerBase)
131 w = WalkerBase("/path/to/something/")105 w = WalkerBase("/path/to/something/")
132 self.assertEquals(w.path, "/path/to/something/")106 self.assertEquals(w.path, "/path/to/something/")
133107
134 def testPathInUrl(self):108 def testPathInUrl(self):
135 """WalkerBase stores the path portion of a complete URL."""109 """WalkerBase stores the path portion of a complete URL."""
136 from lp.registry.scripts.productreleasefinder.walker import (
137 WalkerBase)
138 w = WalkerBase("ftp://localhost/path/to/something/")110 w = WalkerBase("ftp://localhost/path/to/something/")
139 self.assertEquals(w.path, "/path/to/something/")111 self.assertEquals(w.path, "/path/to/something/")
140112
141 def testAddsSlashToPath(self):113 def testAddsSlashToPath(self):
142 """WalkerBase adds a trailing slash to path if ommitted."""114 """WalkerBase adds a trailing slash to path if ommitted."""
143 from lp.registry.scripts.productreleasefinder.walker import (
144 WalkerBase)
145 w = WalkerBase("ftp://localhost/path/to/something")115 w = WalkerBase("ftp://localhost/path/to/something")
146 self.assertEquals(w.path, "/path/to/something/")116 self.assertEquals(w.path, "/path/to/something/")
147117
148 def testUnescapesPath(self):118 def testUnescapesPath(self):
149 """WalkerBase leaves the path escaped."""119 """WalkerBase leaves the path escaped."""
150 from lp.registry.scripts.productreleasefinder.walker import (
151 WalkerBase)
152 w = WalkerBase("ftp://localhost/some%20thing/")120 w = WalkerBase("ftp://localhost/some%20thing/")
153 self.assertEquals(w.path, "/some%20thing/")121 self.assertEquals(w.path, "/some%20thing/")
154122
155 def testStoresQuery(self):123 def testStoresQuery(self):
156 """WalkerBase stores the query portion of a supporting URL."""124 """WalkerBase stores the query portion of a supporting URL."""
157 from lp.registry.scripts.productreleasefinder.walker import (
158 WalkerBase)
159 w = WalkerBase("http://localhost/?foo")125 w = WalkerBase("http://localhost/?foo")
160 self.assertEquals(w.query, "foo")126 self.assertEquals(w.query, "foo")
161127
162 def testStoresFragment(self):128 def testStoresFragment(self):
163 """WalkerBase stores the fragment portion of a supporting URL."""129 """WalkerBase stores the fragment portion of a supporting URL."""
164 from lp.registry.scripts.productreleasefinder.walker import (
165 WalkerBase)
166 WalkerBase.FRAGMENTS = True130 WalkerBase.FRAGMENTS = True
167 try:131 try:
168 w = WalkerBase("http://localhost/#foo")132 w = WalkerBase("http://localhost/#foo")
@@ -180,8 +144,6 @@
180144
181 def test_walk_UnicodeEncodeError(self):145 def test_walk_UnicodeEncodeError(self):
182 """Verify that a UnicodeEncodeError is logged."""146 """Verify that a UnicodeEncodeError is logged."""
183 from lp.registry.scripts.productreleasefinder.walker import (
184 WalkerBase)
185147
186 class TestWalker(WalkerBase):148 class TestWalker(WalkerBase):
187149
@@ -202,12 +164,37 @@
202 logger.setLevel(logging.DEBUG)164 logger.setLevel(logging.DEBUG)
203 logger.addHandler(logging.StreamHandler(log_output))165 logger.addHandler(logging.StreamHandler(log_output))
204 walker = TestWalker('http://example.org/foo', logger)166 walker = TestWalker('http://example.org/foo', logger)
205 for dummy in walker:167 list(walker)
206 pass
207 self.assertEqual(168 self.assertEqual(
208 "Unicode error parsing http://example.org/foo page '/foo/'\n",169 "Unicode error parsing http://example.org/foo page '/foo/'\n",
209 log_output.getvalue())170 log_output.getvalue())
210171
172 def test_walk_open_fail(self):
173 # The walker handles an exception raised during open().
174
175 class TestWalker(WalkerBase):
176
177 def list(self, sub_dir):
178 pass
179
180 def open(self):
181 raise IOError("Test failure.")
182
183 def close(self):
184 pass
185
186 log_output = StringIO.StringIO()
187 logger = logging.getLogger()
188 self.addCleanup(logger.setLevel, logger.level)
189 logger.setLevel(logging.DEBUG)
190 logger.addHandler(logging.StreamHandler(log_output))
191 walker = TestWalker('ftp://example.org/foo', logger)
192 list(walker)
193 self.assertEqual(
194 "Could not connect to ftp://example.org/foo\n"
195 "Failure: Test failure.\n",
196 log_output.getvalue())
197
211198
212class FTPWalker_Base(TestCase):199class FTPWalker_Base(TestCase):
213200