Launchpad itself

Merge lp:~cjwatson/launchpad/opt-in-zopeless-immediate-mail into lp:launchpad

  1. opt-in-zopeless-immediate-mail
  2. Merge into devel
Proposed by Colin Watson
Status: Rejected
Rejected by: Colin Watson
Proposed branch: lp:~cjwatson/launchpad/opt-in-zopeless-immediate-mail
Merge into: lp:launchpad
Prerequisite: lp:~cjwatson/launchpad/more-pop-notifications
Diff against target: 421 lines (+67/-49)
14 files modified
daemons/buildd-manager.tac (+2/-8)
lib/lp/bugs/scripts/bugnotification.py (+11/-2)
lib/lp/bugs/scripts/tests/test_bugnotification.py (+1/-1)
lib/lp/bugs/tests/bugs-emailinterface.txt (+6/-11)
lib/lp/code/mail/codehandler.py (+1/-1)
lib/lp/services/job/celeryjob.py (+1/-3)
lib/lp/services/job/runner.py (+8/-7)
lib/lp/services/mail/basemailer.py (+7/-2)
lib/lp/services/mail/sendmail.py (+13/-1)
lib/lp/services/mail/tests/incomingmail.txt (+6/-2)
lib/lp/services/mail/tests/test_incoming.py (+2/-0)
lib/lp/services/scripts/base.py (+1/-6)
lib/lp/testing/layers.py (+1/-5)
scripts/mlist-import.py (+7/-0)
To merge this branch: bzr merge lp:~cjwatson/launchpad/opt-in-zopeless-immediate-mail
Reviewer Review Type Date Requested Status
Launchpad code reviewers Pending
Review via email: mp+270383@code.launchpad.net

Commit message

Disable Zopeless immediate mail delivery, except for BaseMailer, job OOPS/error notifications, and a few other places that still need it.

Description of the change

Disable Zopeless immediate mail delivery, except for BaseMailer, job OOPS/error notifications, and a few other places that still need it.

This is a step towards being able to disable it across the board so that we can avoid ever having situations where operation/mail/operation/mail sequences send duplicate mail if a non-first operation fails and has to be retried. There are still several things that need to be fixed, notably archiveuploader; but this branch at least means that it's disabled for tests by default, which will simplify the process of fixing archiveuploader.

To post a comment you must log in.
Revision history for this message
Jes Slow (cluelesscoder) wrote :

This seems pretty stale - is it even relevant at this point?

I accidentally clicked a red "-" button - not sure what it did, sorry if it screwed something up.

Revision history for this message
Jes Slow (cluelesscoder) wrote :

OK, looking around at the UI elsewhere I guess I must've removed a related bug - is there a log anywhere where I can find that bug and re-add it?

Revision history for this message
Colin Watson (cjwatson) wrote :

It's absolutely still relevant; it's just a fairly invasive change so it's probably scared off reviewers. :-) I've restored the bug link.

Revision history for this message
Jes Slow (cluelesscoder) wrote :

Bit off-topic to ask this here, but I suppose there's no log where I could look up my change where I removed the bug report? Also wondering if I could pull up a list of my past comments...? Hunted around for a bug report (33 at https://bugs.launchpad.net/launchpad?field.searchtext=activity+log) and noticed that I can pull up an "Full Activity Log" at a bug report like https://bugs.launchpad.net/launchpad/+bug/294644

Basically would like a full activity log for myself.

Also found https://bugs.launchpad.net/launchpad/+bug/787668 which I suppose could cover this? I can leave a comment there.

Revision history for this message
Colin Watson (cjwatson) wrote :

I got it in email about this merge proposal, and it's in the activity log for the bug, but but merge proposals don't themselves have an activity log. You can search for bugs on which you've commented, but there likewise isn't a full activity log for everything a person has done (and that part is pretty much exactly bug 787668).

This is indeed off-topic here though, as you say, and it'd be best not to continue this conversation in this merge proposal because it's likely to get in the way of people reviewing it.

Revision history for this message
Colin Watson (cjwatson) wrote :

Superseded by https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/373727.

Unmerged revisions

17726. By Colin Watson

Drop immediate mail delivery from LaunchpadScript and LaunchpadZopelessLayer.

17725. By Colin Watson

Drop explicit immediate mail delivery from buildd-manager; build mail all goes via BaseMailer now, which handles that.

17724. By Colin Watson

Drop immediate mail delivery from jobs, except for OOPS and user error notifications.

17723. By Colin Watson

Fix CodeHandler to abort before sending error notifications.

17722. By Colin Watson

Force immediate mail delivery for MailingListImport.

17721. By Colin Watson

Force immediate mail delivery for bug notifications and BaseMailer, which rely on this for SMTP exception handling.

17720. By Colin Watson

Adjust some tests to stop assuming immediate mail delivery.

17719. By Colin Watson

Introduce an immediate_mail_delivery context manager, allowing more fine-grained control.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'daemons/buildd-manager.tac'
2--- daemons/buildd-manager.tac 2011-12-29 05:29:36 +0000
3+++ daemons/buildd-manager.tac 2015-09-08 11:52:24 +0000
4@@ -1,4 +1,4 @@
5-# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
6+# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
7 # GNU Affero General Public License version 3 (see the file LICENSE).
8
9 # Twisted Application Configuration file.
10@@ -6,20 +6,15 @@
11
12 from twisted.application import service
13 from twisted.scripts.twistd import ServerOptions
14-from twisted.web import server
15
16+from lp.buildmaster.manager import BuilddManager
17 from lp.services.config import dbconfig
18 from lp.services.daemons import readyservice
19 from lp.services.scripts import execute_zcml_for_scripts
20-from lp.buildmaster.manager import BuilddManager
21-from lp.services.mail.sendmail import set_immediate_mail_delivery
22 from lp.services.twistedsupport.loggingsupport import RotatableFileLogObserver
23
24 execute_zcml_for_scripts()
25 dbconfig.override(dbuser='buildd_manager', isolation_level='read_committed')
26-# XXX wgrant 2011-09-24 bug=29744: initZopeless used to do this.
27-# Should be removed from callsites verified to not need it.
28-set_immediate_mail_delivery(True)
29
30 options = ServerOptions()
31 options.parseOptions()
32@@ -34,4 +29,3 @@
33 # Service for scanning buildd slaves.
34 service = BuilddManager()
35 service.setServiceParent(application)
36-
37
38=== modified file 'lib/lp/bugs/scripts/bugnotification.py'
39--- lib/lp/bugs/scripts/bugnotification.py 2015-08-06 12:09:28 +0000
40+++ lib/lp/bugs/scripts/bugnotification.py 2015-09-08 11:52:24 +0000
41@@ -35,7 +35,10 @@
42 from lp.services.database.constants import UTC_NOW
43 from lp.services.mail.helpers import get_email_template
44 from lp.services.mail.mailwrapper import MailWrapper
45-from lp.services.mail.sendmail import sendmail
46+from lp.services.mail.sendmail import (
47+ immediate_mail_delivery,
48+ sendmail,
49+ )
50 from lp.services.scripts.base import LaunchpadCronScript
51 from lp.services.scripts.logger import log
52 from lp.services.webapp import canonical_url
53@@ -339,7 +342,7 @@
54
55
56 class SendBugNotifications(LaunchpadCronScript):
57- def main(self):
58+ def send_notifications(self):
59 notifications_sent = False
60 bug_notification_set = getUtility(IBugNotificationSet)
61 deferred_notifications = \
62@@ -392,3 +395,9 @@
63
64 if not notifications_sent:
65 self.logger.debug("No notifications are pending to be sent.")
66+
67+ def main(self):
68+ # XXX cjwatson 2015-09-05 bug=29744: SMTP exception handling here
69+ # currently relies on this.
70+ with immediate_mail_delivery(True):
71+ self.send_notifications()
72
73=== modified file 'lib/lp/bugs/scripts/tests/test_bugnotification.py'
74--- lib/lp/bugs/scripts/tests/test_bugnotification.py 2015-08-06 12:09:28 +0000
75+++ lib/lp/bugs/scripts/tests/test_bugnotification.py 2015-09-08 11:52:24 +0000
76@@ -1368,7 +1368,7 @@
77 "send-bug-notifications", config.malone.bugnotification_dbuser,
78 ["-q"])
79 script.txn = self.layer.txn
80- script.main()
81+ script.send_notifications()
82
83 self.assertEqual(1, len(self.oopses))
84 self.assertIn(
85
86=== modified file 'lib/lp/bugs/tests/bugs-emailinterface.txt'
87--- lib/lp/bugs/tests/bugs-emailinterface.txt 2015-07-21 09:04:01 +0000
88+++ lib/lp/bugs/tests/bugs-emailinterface.txt 2015-09-08 11:52:24 +0000
89@@ -81,8 +81,8 @@
90
91 >>> def process_email(raw_mail):
92 ... msg = construct_email(raw_mail)
93- ... handler.process(msg, msg['To'],
94- ... )
95+ ... handler.process(msg, msg['To'])
96+ ... transaction.commit()
97
98 >>> process_email(submit_mail)
99
100@@ -454,9 +454,7 @@
101 ... signature = None
102 >>> msg = email.message_from_string(
103 ... comment_mail, _class=MockUnsignedMessage)
104- >>> handler.process(
105- ... msg, msg['To'],
106- ... )
107+ >>> handler.process(msg, msg['To'])
108 True
109 >>> transaction.commit()
110
111@@ -514,9 +512,7 @@
112 ... return construct_email(edit_mail)
113
114 >>> def submit_command_email(msg):
115- ... handler.process(
116- ... msg, msg['To'],
117- ... )
118+ ... handler.process(msg, msg['To'])
119 ... transaction.commit()
120
121 >>> def submit_commands(bug, *commands):
122@@ -1805,10 +1801,9 @@
123 >>> msg = signed_message_from_string(submit_mail)
124 >>> import email.utils
125 >>> msg['Message-Id'] = email.utils.make_msgid()
126- >>> handler.process(
127- ... msg, msg['To'],
128- ... )
129+ >>> handler.process(msg, msg['To'])
130 True
131+ >>> transaction.commit()
132 >>> print_latest_email()
133 Subject: Submit Request Failure
134 To: test@canonical.com
135
136=== modified file 'lib/lp/code/mail/codehandler.py'
137--- lib/lp/code/mail/codehandler.py 2015-07-08 16:05:11 +0000
138+++ lib/lp/code/mail/codehandler.py 2015-09-08 11:52:24 +0000
139@@ -316,11 +316,11 @@
140 message, context.vote, context.vote_tags, mail)
141
142 except IncomingEmailError as error:
143+ transaction.abort()
144 send_process_error_notification(
145 str(user.preferredemail.email),
146 'Submit Request Failure',
147 error.message, mail, error.failing_command)
148- transaction.abort()
149 return True
150
151 @staticmethod
152
153=== modified file 'lib/lp/services/job/celeryjob.py'
154--- lib/lp/services/job/celeryjob.py 2015-08-03 07:06:45 +0000
155+++ lib/lp/services/job/celeryjob.py 2015-09-08 11:52:24 +0000
156@@ -1,4 +1,4 @@
157-# Copyright 2012 Canonical Ltd. This software is licensed under the
158+# Copyright 2012-2015 Canonical Ltd. This software is licensed under the
159 # GNU Affero General Public License version 3 (see the file LICENSE).
160
161 """Celery-specific Job code.
162@@ -34,7 +34,6 @@
163 install_feature_controller,
164 make_script_feature_controller,
165 )
166-from lp.services.mail.sendmail import set_immediate_mail_delivery
167 from lp.services.job.model.job import (
168 Job,
169 UniversalJobSource,
170@@ -139,7 +138,6 @@
171 return
172 transaction.abort()
173 scripts.execute_zcml_for_scripts(use_web_security=False)
174- set_immediate_mail_delivery(True)
175 needs_zcml = False
176
177
178
179=== modified file 'lib/lp/services/job/runner.py'
180--- lib/lp/services/job/runner.py 2015-08-04 00:19:36 +0000
181+++ lib/lp/services/job/runner.py 2015-09-08 11:52:24 +0000
182@@ -1,4 +1,4 @@
183-# Copyright 2009-2012 Canonical Ltd. This software is licensed under the
184+# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
185 # GNU Affero General Public License version 3 (see the file LICENSE).
186
187 """Facilities for running Jobs."""
188@@ -72,8 +72,8 @@
189 IRunnableJob,
190 )
191 from lp.services.mail.sendmail import (
192+ immediate_mail_delivery,
193 MailController,
194- set_immediate_mail_delivery,
195 )
196 from lp.services.twistedsupport import run_reactor
197 from lp.services.webapp import errorlog
198@@ -177,7 +177,9 @@
199 """Report this oops."""
200 ctrl = self.getOopsMailController(oops['id'])
201 if ctrl is not None:
202- ctrl.send()
203+ # XXX cjwatson 2015-09-06 bug=29744: Can this be removed?
204+ with immediate_mail_delivery(True):
205+ ctrl.send()
206
207 def getOopsVars(self):
208 """See `IRunnableJob`."""
209@@ -187,7 +189,9 @@
210 """See `IRunnableJob`."""
211 ctrl = self.getUserErrorMailController(e)
212 if ctrl is not None:
213- ctrl.send()
214+ # XXX cjwatson 2015-09-06 bug=29744: Can this be removed?
215+ with immediate_mail_delivery(True):
216+ ctrl.send()
217
218 def makeOopsReport(self, oops_config, info):
219 """Generate an OOPS report using the given OOPS configuration."""
220@@ -406,9 +410,6 @@
221 scripts.execute_zcml_for_scripts(use_web_security=False)
222 signal(SIGHUP, handler)
223 dbconfig.override(dbuser=cls.dbuser, isolation_level='read_committed')
224- # XXX wgrant 2011-09-24 bug=29744: initZopeless used to do this.
225- # Should be removed from callsites verified to not need it.
226- set_immediate_mail_delivery(True)
227
228 @staticmethod
229 def __exit__(exc_type, exc_val, exc_tb):
230
231=== modified file 'lib/lp/services/mail/basemailer.py'
232--- lib/lp/services/mail/basemailer.py 2015-08-25 14:05:24 +0000
233+++ lib/lp/services/mail/basemailer.py 2015-09-08 11:52:24 +0000
234@@ -20,6 +20,7 @@
235 from lp.services.mail.sendmail import (
236 append_footer,
237 format_address,
238+ immediate_mail_delivery,
239 MailController,
240 )
241 from lp.services.utils import text_delta
242@@ -204,8 +205,12 @@
243
244 def sendAll(self):
245 """Send notifications to all recipients."""
246- for email, recipient in sorted(self._recipients.getRecipientPersons()):
247- self.sendOne(email, recipient)
248+ # XXX cjwatson 2015-09-05 bug=29744: We currently depend on this to
249+ # handle SMTPExceptions in the correct place.
250+ with immediate_mail_delivery(True):
251+ for email, recipient in sorted(
252+ self._recipients.getRecipientPersons()):
253+ self.sendOne(email, recipient)
254
255
256 class RecipientReason:
257
258=== modified file 'lib/lp/services/mail/sendmail.py'
259--- lib/lp/services/mail/sendmail.py 2015-07-21 09:04:01 +0000
260+++ lib/lp/services/mail/sendmail.py 2015-09-08 11:52:24 +0000
261@@ -1,4 +1,4 @@
262-# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
263+# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
264 # GNU Affero General Public License version 3 (see the file LICENSE).
265
266 """The One True Way to send mail from the Launchpad application.
267@@ -19,6 +19,7 @@
268 'format_address',
269 'format_address_for_person',
270 'get_msgid',
271+ 'immediate_mail_delivery',
272 'MailController',
273 'sendmail',
274 'set_immediate_mail_delivery',
275@@ -29,6 +30,7 @@
276
277
278 from binascii import b2a_qp
279+from contextlib import contextmanager
280 from email import charset
281 from email.encoders import encode_base64
282 from email.header import Header
283@@ -178,6 +180,16 @@
284 _immediate_mail_delivery = enabled
285
286
287+@contextmanager
288+def immediate_mail_delivery(enabled):
289+ """Context manager to temporarily change immediate mail delivery."""
290+ global _immediate_mail_delivery
291+ previous = _immediate_mail_delivery
292+ set_immediate_mail_delivery(enabled)
293+ yield
294+ set_immediate_mail_delivery(previous)
295+
296+
297 def simple_sendmail(from_addr, to_addrs, subject, body, headers=None,
298 bulk=True):
299 """Send an email from from_addr to to_addrs with the subject and body
300
301=== modified file 'lib/lp/services/mail/tests/incomingmail.txt'
302--- lib/lp/services/mail/tests/incomingmail.txt 2015-09-08 11:52:24 +0000
303+++ lib/lp/services/mail/tests/incomingmail.txt 2015-09-08 11:52:24 +0000
304@@ -45,16 +45,20 @@
305
306 Now we send a few test mails to foo.com, bar.com, and baz.com:
307
308+ >>> import transaction
309 >>> from lp.services.mail.tests.helpers import read_test_message
310 >>> from lp.services.mail.sendmail import sendmail as original_sendmail
311 >>> from lp.testing.dbuser import switch_dbuser
312
313 For these examples, we don't want the Precedence header added. Domains
314 are treated without regard to case: for incoming mail, foo.com and
315-FOO.COM are treated equivalently.
316+FOO.COM are treated equivalently. We also commit to ensure that mail is
317+sent immediately.
318
319 >>> def sendmail(msg, to_addrs=None):
320- ... return original_sendmail(msg, to_addrs=to_addrs, bulk=False)
321+ ... msgid = original_sendmail(msg, to_addrs=to_addrs, bulk=False)
322+ ... transaction.commit()
323+ ... return msgid
324
325 >>> switch_dbuser('launchpad')
326 >>> msgids = {'foo.com': [], 'bar.com': [], 'baz.com': []}
327
328=== modified file 'lib/lp/services/mail/tests/test_incoming.py'
329--- lib/lp/services/mail/tests/test_incoming.py 2015-07-08 16:05:11 +0000
330+++ lib/lp/services/mail/tests/test_incoming.py 2015-09-08 11:52:24 +0000
331@@ -75,6 +75,7 @@
332 email_address, 'to@example.com', 'subject', invalid_body,
333 bulk=False)
334 ctrl.send()
335+ transaction.commit()
336 handleMail()
337 self.assertEqual([], self.oopses)
338 [notification] = pop_notifications()
339@@ -101,6 +102,7 @@
340 email_address, 'to@example.com', 'subject', fat_body,
341 bulk=False)
342 ctrl.send()
343+ transaction.commit()
344 handleMail()
345 self.assertEqual([], self.oopses)
346 [notification] = pop_notifications()
347
348=== modified file 'lib/lp/services/scripts/base.py'
349--- lib/lp/services/scripts/base.py 2014-08-29 01:34:04 +0000
350+++ lib/lp/services/scripts/base.py 2015-09-08 11:52:24 +0000
351@@ -1,4 +1,4 @@
352-# Copyright 2009-2012 Canonical Ltd. This software is licensed under the
353+# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
354 # GNU Affero General Public License version 3 (see the file LICENSE).
355
356 __metaclass__ = type
357@@ -44,7 +44,6 @@
358 install_feature_controller,
359 make_script_feature_controller,
360 )
361-from lp.services.mail.sendmail import set_immediate_mail_delivery
362 from lp.services.scripts.interfaces.scriptactivity import IScriptActivitySet
363 from lp.services.scripts.logger import OopsHandler
364 from lp.services.webapp.errorlog import globalErrorUtility
365@@ -307,10 +306,6 @@
366 self._init_zca(use_web_security=use_web_security)
367 self._init_db(isolation=isolation)
368
369- # XXX wgrant 2011-09-24 bug=29744: initZopeless used to do this.
370- # Should be called directly by scripts that actually need it.
371- set_immediate_mail_delivery(True)
372-
373 date_started = datetime.datetime.now(UTC)
374 profiler = None
375 if self.options.profile:
376
377=== modified file 'lib/lp/testing/layers.py'
378--- lib/lp/testing/layers.py 2013-08-14 11:15:51 +0000
379+++ lib/lp/testing/layers.py 2015-09-08 11:52:24 +0000
380@@ -1,4 +1,4 @@
381-# Copyright 2009-2012 Canonical Ltd. This software is licensed under the
382+# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
383 # GNU Affero General Public License version 3 (see the file LICENSE).
384
385 """Layers used by Launchpad tests.
386@@ -1498,10 +1498,6 @@
387 @profiled
388 def testSetUp(cls):
389 dbconfig.override(isolation_level='read_committed')
390- # XXX wgrant 2011-09-24 bug=29744: initZopeless used to do this.
391- # Tests that still need it should eventually set this directly,
392- # so the whole layer is not polluted.
393- set_immediate_mail_delivery(True)
394
395 # Connect Storm
396 reconnect_stores()
397
398=== modified file 'scripts/mlist-import.py'
399--- scripts/mlist-import.py 2013-01-07 02:40:55 +0000
400+++ scripts/mlist-import.py 2015-09-08 11:52:24 +0000
401@@ -24,6 +24,7 @@
402
403 from lp.registry.scripts.mlistimport import Importer
404 from lp.services.config import config
405+from lp.services.mail.sendmail import set_immediate_mail_delivery
406 from lp.services.scripts.base import LaunchpadScript
407
408
409@@ -56,6 +57,12 @@
410
411 def main(self):
412 """See `LaunchpadScript`."""
413+ # XXX cjwatson 2015-09-06 bug=29744: We need immediate mail delivery
414+ # for the current implementation of the --notifications switch, and
415+ # because the tests expect notifications to end up in
416+ # LayerProcessController.smtp_controller.
417+ set_immediate_mail_delivery(True)
418+
419 team_name = None
420 if len(self.args) == 0:
421 self.parser.error('Missing team name')