Merge lp:~wgrant/launchpad/no-key-oopses into lp:launchpad

Proposed by William Grant on 2011-09-29
Status: Merged
Approved by: William Grant on 2011-09-29
Approved revision: no longer in the source branch.
Merged at revision: 14071
Proposed branch: lp:~wgrant/launchpad/no-key-oopses
Merge into: lp:launchpad
Diff against target: 240 lines (+55/-50)
5 files modified
lib/lp/archiveuploader/nascentupload.py (+2/-15)
lib/lp/archiveuploader/tests/nascentupload.txt (+2/-2)
lib/lp/archiveuploader/tests/test_uploadprocessor.py (+31/-23)
lib/lp/archiveuploader/uploadprocessor.py (+17/-7)
lib/lp/soyuz/doc/soyuz-upload.txt (+3/-3)
To merge this branch: bzr merge lp:~wgrant/launchpad/no-key-oopses
Reviewer Review Type Date Requested Status
Jeroen T. Vermeulen (community) 2011-09-29 Approve on 2011-09-29
Review via email: mp+77447@code.launchpad.net

Commit message

[r=jtv][bug=862032] Don't OOPS on FatalUploadErrors. Even though we can't tell anyone about them, they are still user error.

Description of the change

This branch fixes some user-error Soyuz OOPSes: FatalUploadErrors from process-upload.

FatalUploadErrors are caused by ChangesFile in case of signature issues (no signature, bad signature, unrecognized key, etc.) or corrupt changes files. Since we use the changes file to determine the notification recipient, such a failure means that we're unable to email anyone about it.

Long ago it was decided to turn such unreportable user errors into OOPSes. But that runs counter to ZeroOopsPolicy, so we should not do it. This branch logs them at DEBUG instead, with the rest of the normal processing stuff.

To post a comment you must log in.
Jeroen T. Vermeulen (jtv) wrote :
Download full text (6.6 KiB)

This is a change that I've been craving. So much kudos for implementing it.

I heartily approve of most of this branch. Of course as a reviewer, I have a reputation to uphold and thus:

=== modified file 'lib/lp/archiveuploader/nascentupload.py'
--- lib/lp/archiveuploader/nascentupload.py 2011-08-23 14:35:43 +0000
+++ lib/lp/archiveuploader/nascentupload.py 2011-09-29 05:22:06 +0000

@@ -114,23 +110,14 @@
     def from_changesfile_path(cls, changesfile_path, policy, logger):
         """Create a NascentUpload from the given changesfile path.

- May raise FatalUploadError due to unrecoverable problems building
+ May raise UploadError due to unrecoverable problems building
         the ChangesFile object.

If we're crossing the boundary of "fatal" here, consider replacing or removing the word "unrecoverable" as well. It's not so much the error that is unrecoverable, as an extent of code. But what extent exactly? Like "Fatal," "unrecoverable" sort of suggests that that extent is the entire script run.

=== modified file 'lib/lp/archiveuploader/tests/test_uploadprocessor.py'
--- lib/lp/archiveuploader/tests/test_uploadprocessor.py 2011-08-26 06:14:54 +0000
+++ lib/lp/archiveuploader/tests/test_uploadprocessor.py 2011-09-29 05:22:06 +0000

@@ -1361,28 +1363,27 @@
         self.options.builds = False
         processor = self.getUploadProcessor(self.layer.txn)

- upload_dir = self.queueUpload("foocomm_1.0-1_proposed")
- bogus_changesfile_data = '''
- Ubuntu is a community developed, Linux-based operating system that is
- perfect for laptops, desktops and servers. It contains all the
- applications you need - a web browser, presentation, document and
- spreadsheet software, instant messaging and much more.
- '''
- file_handle = open(
- '%s/%s' % (upload_dir, 'bogus.changes'), 'w')
- file_handle.write(bogus_changesfile_data)
- file_handle.close()
+ self.queueUpload("foocomm_1.0-1_proposed")
+
+ # Any code that causes an OOPS is a bug that must be fixed, so
+ # let's monkeypatch something in that we know will OOPS.

I find this explanation unnecessarily confusing. To a hurried reader (and we all are from time to time) it seems to say "let's create bugs that must be fixed," which of course is fine from a job-security standpoint but...

What I think you mean is: "This test verifies that oopses get handled properly, which is very important because oopses must be caught and fixed. The following simulates an error that will be treated as an oops. We verify that the oops gets handled properly later."

If so, the first of those sentences probably belongs at the top of the test (if roughly the same thing isn't being said there already), and the last belongs at the point of verification.

For some reason the term "let's" seems to be a very reliable indicator of this kind of muddled expression in tests, so treat it as inherently suspicious.

+ class SomeException(Exception):
+ pass
+
+ def from_changesfile_path(cls, changesfile_path, policy, logger):
+ raise SomeException("I am an explanation.")
+ ...

Read more...

review: Approve
William Grant (wgrant) wrote :

> Well done, well explained. Just one question here: wouldn't logger.info make
> more sense than logger.debug? After all this situation is still a bit of a
> problem.
>
> In particular, I'm assuming that sometimes we'll want to search the logs for
> this message to find out what happened to an upload. If so, I imagine we
> would want this logged by default.

Good point. I've bumped it to INFO and adding the changes file path to the message, as everything else that mentions the path is at DEBUG.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/archiveuploader/nascentupload.py'
2--- lib/lp/archiveuploader/nascentupload.py 2011-08-23 14:35:43 +0000
3+++ lib/lp/archiveuploader/nascentupload.py 2011-09-29 12:10:29 +0000
4@@ -49,10 +49,6 @@
5 PARTNER_COMPONENT_NAME = 'partner'
6
7
8-class FatalUploadError(Exception):
9- """A fatal error occurred processing the upload; processing aborted."""
10-
11-
12 class EarlyReturnUploadError(Exception):
13 """An error occurred that prevented further error collection."""
14
15@@ -114,23 +110,14 @@
16 def from_changesfile_path(cls, changesfile_path, policy, logger):
17 """Create a NascentUpload from the given changesfile path.
18
19- May raise FatalUploadError due to unrecoverable problems building
20+ May raise UploadError due to unrecoverable problems building
21 the ChangesFile object.
22
23 :param changesfile_path: path to the changesfile to be uploaded.
24 :param policy: the upload policy to be used.
25 :param logger: the logger to be used.
26 """
27- try:
28- changesfile = ChangesFile(changesfile_path, policy, logger)
29- except UploadError, e:
30- # We can't run reject() because unfortunately we don't have
31- # the address of the uploader to notify -- we broke in that
32- # exact step.
33- # XXX cprov 2007-03-26: we should really be emailing this
34- # rejection to the archive admins. For now, this will end
35- # up in the script log.
36- raise FatalUploadError(str(e))
37+ changesfile = ChangesFile(changesfile_path, policy, logger)
38 return cls(changesfile, policy, logger)
39
40 def process(self, build=None):
41
42=== modified file 'lib/lp/archiveuploader/tests/nascentupload.txt'
43--- lib/lp/archiveuploader/tests/nascentupload.txt 2011-06-28 15:04:29 +0000
44+++ lib/lp/archiveuploader/tests/nascentupload.txt 2011-09-29 12:10:29 +0000
45@@ -45,14 +45,14 @@
46 Constructing a NascentUpload instance verifies that the changes file
47 specified exists and tries to build a ChangesFile (see
48 doc/nascentuploadfile.txt) object based on that. If anything goes
49-wrong during this process FatalUploadError is raised:
50+wrong during this process UploadError is raised:
51
52 >>> from lp.services.log.logger import DevNullLogger, FakeLogger
53 >>> NascentUpload.from_changesfile_path(
54 ... datadir("DOES-NOT-EXIST"), buildd_policy, FakeLogger())
55 Traceback (most recent call last):
56 ...
57- FatalUploadError:...
58+ UploadError:...
59
60 Otherwise a ChangesFile object is ready to use.
61
62
63=== modified file 'lib/lp/archiveuploader/tests/test_uploadprocessor.py'
64--- lib/lp/archiveuploader/tests/test_uploadprocessor.py 2011-08-26 06:14:54 +0000
65+++ lib/lp/archiveuploader/tests/test_uploadprocessor.py 2011-09-29 12:10:29 +0000
66@@ -15,6 +15,7 @@
67 from StringIO import StringIO
68 import tempfile
69
70+from fixtures import MonkeyPatch
71 from storm.locals import Store
72 import transaction
73 from zope.component import (
74@@ -42,6 +43,7 @@
75 parse_build_upload_leaf_name,
76 UploadHandler,
77 UploadProcessor,
78+ UploadStatusEnum,
79 )
80 from lp.buildmaster.enums import (
81 BuildFarmJobType,
82@@ -92,6 +94,7 @@
83 TestCase,
84 TestCaseWithFactory,
85 )
86+from lp.testing.fakemethod import FakeMethod
87 from lp.testing.mail_helpers import pop_notifications
88
89
90@@ -1352,37 +1355,27 @@
91 self.checkComponentOverride("bar_1.0-1", "universe")
92
93 def testOopsCreation(self):
94- """Test the the creation of an OOPS upon upload processing failure.
95-
96- In order to trigger the exception needed a bogus changes file will be
97- used.
98- That exception will then initiate the creation of an OOPS report.
99- """
100+ """Test that an unhandled exception generates an OOPS."""
101 self.options.builds = False
102 processor = self.getUploadProcessor(self.layer.txn)
103
104- upload_dir = self.queueUpload("foocomm_1.0-1_proposed")
105- bogus_changesfile_data = '''
106- Ubuntu is a community developed, Linux-based operating system that is
107- perfect for laptops, desktops and servers. It contains all the
108- applications you need - a web browser, presentation, document and
109- spreadsheet software, instant messaging and much more.
110- '''
111- file_handle = open(
112- '%s/%s' % (upload_dir, 'bogus.changes'), 'w')
113- file_handle.write(bogus_changesfile_data)
114- file_handle.close()
115+ self.queueUpload("foocomm_1.0-1_proposed")
116+
117+ # Inject an unhandled exception into the upload processor.
118+ class SomeException(Exception):
119+ pass
120+ self.useFixture(
121+ MonkeyPatch(
122+ 'lp.archiveuploader.nascentupload.NascentUpload.'
123+ 'from_changesfile_path',
124+ FakeMethod(failure=SomeException("I am an explanation."))))
125
126 processor.processUploadQueue()
127
128 error_utility = ErrorReportingUtility()
129 error_report = error_utility.getLastOopsReport()
130- self.assertEqual('FatalUploadError', error_report.type)
131- # The upload policy requires a signature but none is present, so
132- # we get gpg verification errors.
133- expected_explanation = (
134- "Verification failed 3 times: ['No data', 'No data', 'No data']")
135- self.assertIn(expected_explanation, error_report.tb_text)
136+ self.assertEqual('SomeException', error_report.type)
137+ self.assertIn("I am an explanation", error_report.tb_text)
138
139 def testLZMADebUpload(self):
140 """Make sure that data files compressed with lzma in Debs work.
141@@ -1919,6 +1912,21 @@
142 self.PGPSignatureNotPreserved(archive=self.breezy.main_archive)
143 self.switchToUploader()
144
145+ def test_unsigned_upload_is_silently_rejected(self):
146+ # Unsigned uploads are rejected without OOPS or email.
147+ uploadprocessor = self.setupBreezyAndGetUploadProcessor()
148+ upload_dir = self.queueUpload("netapplet_1.0-1")
149+
150+ last_oops = ErrorReportingUtility().getLastOopsReport()
151+
152+ [result] = self.processUpload(uploadprocessor, upload_dir)
153+
154+ self.assertEqual(UploadStatusEnum.REJECTED, result)
155+ self.assertLogContains(
156+ "INFO Failed to parse changes file")
157+ self.assertEqual(len(stub.test_emails), 0)
158+ self.assertNoNewOops(last_oops)
159+
160
161 class TestBuildUploadProcessor(TestUploadProcessorBase):
162 """Test that processing build uploads works."""
163
164=== modified file 'lib/lp/archiveuploader/uploadprocessor.py'
165--- lib/lp/archiveuploader/uploadprocessor.py 2011-05-04 06:37:50 +0000
166+++ lib/lp/archiveuploader/uploadprocessor.py 2011-09-29 12:10:29 +0000
167@@ -63,8 +63,8 @@
168 from lp.app.errors import NotFoundError
169 from lp.archiveuploader.nascentupload import (
170 EarlyReturnUploadError,
171- FatalUploadError,
172 NascentUpload,
173+ UploadError,
174 )
175 from lp.archiveuploader.uploadpolicy import (
176 BuildDaemonUploadPolicy,
177@@ -370,8 +370,22 @@
178 # The path we want for NascentUpload is the path to the folder
179 # containing the changes file (and the other files referenced by it).
180 changesfile_path = os.path.join(self.upload_path, changes_file)
181- upload = NascentUpload.from_changesfile_path(
182- changesfile_path, policy, self.processor.log)
183+ try:
184+ upload = NascentUpload.from_changesfile_path(
185+ changesfile_path, policy, self.processor.log)
186+ except UploadError as e:
187+ # We failed to parse the changes file, so we have no key or
188+ # Changed-By to notify of the rejection. Just log it and
189+ # move on.
190+ # XXX wgrant 2011-09-29 bug=499438: With some refactoring we
191+ # could do better here: if we have a signature then we have
192+ # somebody to email, even if the rest of the file is
193+ # corrupt.
194+ logger.info(
195+ "Failed to parse changes file '%s': %s" % (
196+ os.path.join(self.upload_path, changes_file),
197+ str(e)))
198+ return UploadStatusEnum.REJECTED
199
200 # Reject source upload to buildd upload paths.
201 first_path = relative_path.split(os.path.sep)[0]
202@@ -401,10 +415,6 @@
203 "%s " % e)
204 logger.debug(
205 "UploadPolicyError escaped upload.process", exc_info=True)
206- except FatalUploadError, e:
207- upload.reject("UploadError escaped upload.process: %s" % e)
208- logger.debug(
209- "UploadError escaped upload.process", exc_info=True)
210 except (KeyboardInterrupt, SystemExit):
211 raise
212 except EarlyReturnUploadError:
213
214=== modified file 'lib/lp/soyuz/doc/soyuz-upload.txt'
215--- lib/lp/soyuz/doc/soyuz-upload.txt 2011-09-18 10:22:51 +0000
216+++ lib/lp/soyuz/doc/soyuz-upload.txt 2011-09-29 12:10:29 +0000
217@@ -246,12 +246,12 @@
218 >>> process.returncode
219 0
220
221-Check the four uploads are all where we expect - number 1 in failed,
222+Check the four uploads are all where we expect - number 1 in rejected,
223 the other three still in incoming.
224
225 >>> for i in range(4):
226 ... find_upload_dir_result(i + 1)
227- 'failed'
228+ 'rejected'
229 'incoming'
230 'incoming'
231 'incoming'
232@@ -339,7 +339,7 @@
233
234 >>> for i in range(0, 4):
235 ... find_upload_dir_result(i + 1)
236- 'failed'
237+ 'rejected'
238 'failed'
239
240 Also check the upload folders contain all the files we uploaded.