Merge lp:~abentley/launchpad/memlimit-sendbranchmail into lp:launchpad

Proposed by Aaron Bentley on 2011-07-11
Status: Merged
Approved by: Aaron Bentley on 2011-07-11
Approved revision: no longer in the source branch.
Merged at revision: 13420
Proposed branch: lp:~abentley/launchpad/memlimit-sendbranchmail
Merge into: lp:launchpad
Diff against target: 388 lines (+117/-50)
8 files modified
cronscripts/sendbranchmail.py (+12/-15)
lib/lp/code/model/branchjob.py (+60/-11)
lib/lp/code/scripts/tests/test_sendbranchmail.py (+10/-10)
lib/lp/codehosting/vfs/transport.py (+10/-1)
lib/lp/registry/model/packagingjob.py (+2/-11)
lib/lp/services/utils.py (+20/-2)
lib/lp/translations/model/translationpackagingjob.py (+1/-0)
lib/lp/translations/tests/test_translationtemplatesbuildjob.py (+2/-0)
To merge this branch: bzr merge lp:~abentley/launchpad/memlimit-sendbranchmail
Reviewer Review Type Date Requested Status
Aaron Bentley (community) Approve on 2011-07-11
Review via email: mp+67604@code.launchpad.net

This proposal supersedes a proposal from 2011-06-23.

Commit message

Memory-limit branch mail jobs.

Description of the change

The original landing was rolled back because it failed in production. The production failures appear to be because there was a wrapper script on production that was setting the hard limit lower than 2.0 GB. This version uses the same memory limit as the wrapper script's hard limit.

= Summary =
Fix bug #585126: sendbranchmail with lp:~vcs-imports/linux/trunk is eating memory

== Proposed fix ==
Introduce a memory limit to branch mail jobs.

== Pre-implementation notes ==
None

== Implementation details ==
The TwistedJobRunner now supports memory limits for jobs, which means that each job runs in a separate process, and raises a MemoryError if it exceeds allowable memory use. This branch uses that mechanism for the sendbranchmail jobs.

In order to use the memory limit, we must switch to the TwistedJobRunner. In order to use the TwistedJobRunner, we need a suitable job source, so BranchMailJobSource was implemented. In order to implement BranchMailJobSource, we changed BranchJobDerived.get so that it will return instances of any given type. In order to make BranchJobDerived.get to return instances of any given type, we used RegisteredSubclass as its metaclass and provided _subclass and _register_subclass.

There were several drivebys as well:
AsyncVirtualServer can now behave as a ContextManager, meaning that the following works:
with get_ro_server():
    server_ops

Old bzr workarounds that are no longer needed were deleted.

Lots of lint was fixed

== Tests ==

bin/test -vt sendbranchmail -t branchjob

== Demo and Q/A ==
Not sure whether this is QAable.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/translations/tests/test_translationtemplatesbuildjob.py
  cronscripts/sendbranchmail.py
  lib/lp/registry/model/packagingjob.py
  lib/lp/codehosting/vfs/transport.py
  lib/lp/code/scripts/tests/test_sendbranchmail.py
  lib/lp/code/model/branchjob.py
  lib/lp/services/utils.py

To post a comment you must log in.
Gavin Panella (allenap) wrote : Posted in a previous version of this proposal

Looks good.

[1]

+ @staticmethod
+ def _register_subclass(cls):
+ """Register this class with its enumeration."""
+ # This would be a classmethod, except that subclasses (e.g.
+ # TranslationPackagingJob) need to be able to override it and call
+ # into it, and there's no syntax to call a base class's version of a
+ # classmethod with the subclass as the first parameter.

I think super(BranchJobDerived, cls)._register_subclass() might work.

[2]

+class BranchMailJobSource(BaseRunnableJobSource):
+ """Source of jobs that send mail about branches."""
+
+ memory_limit = 2 * (1024 ** 3)

2GB is quite a high limit. Is that correct?

review: Approve
Aaron Bentley (abentley) wrote : Posted in a previous version of this proposal

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

On 11-06-23 12:36 PM, Gavin Panella wrote:
> [1]
>
> + @staticmethod
> + def _register_subclass(cls):
> + """Register this class with its enumeration."""
> + # This would be a classmethod, except that subclasses (e.g.
> + # TranslationPackagingJob) need to be able to override it and call
> + # into it, and there's no syntax to call a base class's version of a
> + # classmethod with the subclass as the first parameter.
>
> I think super(BranchJobDerived, cls)._register_subclass() might work.

This doesn't work because _register_subclass is called before
BranchJobDerived is part of the global namespace (because it's called by
the metaclass).

class Bar(type):

    def __init__(cls, name, bases, dict_):
        cls.upcall()

class Foo(object):

    __metaclass__ = Bar

    @classmethod
    def upcall(cls):
        super(Foo, cls)

$ python superme.py
Traceback (most recent call last):
  File "superme.py", line 7, in <module>
    class Foo(object):
  File "superme.py", line 4, in __init__
    cls.upcall()
  File "superme.py", line 13, in upcall
    super(Foo, cls)
NameError: global name 'Foo' is not defined

> [2]
>
> +class BranchMailJobSource(BaseRunnableJobSource):
> + """Source of jobs that send mail about branches."""
> +
> + memory_limit = 2 * (1024 ** 3)
>
> 2GB is quite a high limit. Is that correct?

I'm using the limit applied to cronscripts/scan_branches.py (in 12991)
as an example. sendbranchmail runs on the same machine and uses similar
technology, including bzrlib.

The bug reports that 5GB is problematic, so this cuts it by more than half.

If you're wondering whether bzrlib really needs that much, well, it has
been quite memory-hungry historically, but I understand bzr 2.4 will be
a vast improvement.

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

iEYEARECAAYFAk4DeNAACgkQ0F+nu1YWqI2WkQCfZri8hj8jxxSp76TSrDZ1cvAL
7vEAniM+dsU6qFgVa72NuZvtr/YxM9PU
=Mzw0
-----END PGP SIGNATURE-----

Aaron Bentley (abentley) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'cronscripts/sendbranchmail.py'
2--- cronscripts/sendbranchmail.py 2011-06-28 23:43:37 +0000
3+++ cronscripts/sendbranchmail.py 2011-07-11 18:22:18 +0000
4@@ -1,6 +1,6 @@
5 #!/usr/bin/python -S
6 #
7-# Copyright 2009 Canonical Ltd. This software is licensed under the
8+# Copyright 2008-2011 Canonical Ltd. This software is licensed under the
9 # GNU Affero General Public License version 3 (see the file LICENSE).
10
11 # pylint: disable-msg=W0403
12@@ -12,14 +12,18 @@
13
14 __metaclass__ = type
15
16+
17+import logging
18+
19 import _pythonpath
20-from zope.component import getUtility
21
22 from canonical.config import config
23-from lp.codehosting.vfs import get_ro_server
24-from lp.services.job.runner import JobRunner
25-from lp.code.interfaces.branchjob import (
26- IRevisionMailJobSource, IRevisionsAddedJobSource)
27+from lp.services.job.runner import (
28+ TwistedJobRunner,
29+ )
30+from lp.code.model.branchjob import (
31+ BranchMailJobSource,
32+ )
33 from lp.services.scripts.base import LaunchpadCronScript
34 from canonical.launchpad.webapp.errorlog import globalErrorUtility
35
36@@ -29,15 +33,8 @@
37
38 def main(self):
39 globalErrorUtility.configure('sendbranchmail')
40- jobs = list(getUtility(IRevisionMailJobSource).iterReady())
41- jobs.extend(getUtility(IRevisionsAddedJobSource).iterReady())
42- runner = JobRunner(jobs, self.logger)
43- server = get_ro_server()
44- server.start_server()
45- try:
46- runner.runAll()
47- finally:
48- server.stop_server()
49+ runner = TwistedJobRunner.runFromSource(
50+ BranchMailJobSource, 'send-branch-mail', logging.getLogger())
51 self.logger.info(
52 'Ran %d RevisionMailJobs.' % len(runner.completed_jobs))
53
54
55=== modified file 'lib/lp/code/model/branchjob.py'
56--- lib/lp/code/model/branchjob.py 2011-06-28 23:43:37 +0000
57+++ lib/lp/code/model/branchjob.py 2011-07-11 18:22:18 +0000
58@@ -13,6 +13,7 @@
59 ]
60
61 import contextlib
62+from itertools import chain
63 import operator
64 import os
65 import shutil
66@@ -106,7 +107,11 @@
67 from lp.scripts.helpers import TransactionFreeOperation
68 from lp.services.job.interfaces.job import JobStatus
69 from lp.services.job.model.job import Job
70-from lp.services.job.runner import BaseRunnableJob
71+from lp.services.job.runner import (
72+ BaseRunnableJob,
73+ BaseRunnableJobSource,
74+ )
75+from lp.services.utils import RegisteredSubclass
76 from lp.translations.interfaces.translationimportqueue import (
77 ITranslationImportQueue,
78 )
79@@ -217,6 +222,20 @@
80
81 class BranchJobDerived(BaseRunnableJob):
82
83+ __metaclass__ = RegisteredSubclass
84+ _subclass = {}
85+
86+ @staticmethod
87+ def _register_subclass(cls):
88+ """Register this class with its enumeration."""
89+ # Why not a classmethod? See RegisteredSubclass.__init__.
90+ job_type = getattr(cls, 'class_job_type', None)
91+ if job_type is not None:
92+ value = cls._subclass.setdefault(job_type, cls)
93+ assert value is cls, (
94+ '%s already registered to %s.' % (
95+ job_type.name, value.__name__))
96+
97 delegates(IBranchJob)
98
99 def __init__(self, branch_job):
100@@ -251,19 +270,29 @@
101 And(BranchJob.job_type == cls.class_job_type,
102 BranchJob.job == Job.id,
103 Job.id.is_in(Job.ready_jobs)))
104- return (cls(job) for job in jobs)
105-
106- @classmethod
107- def get(cls, key):
108- """Return the instance of this class whose key is supplied.
109+ return (cls._getInstance(job) for job in jobs)
110+
111+ @classmethod
112+ def _getInstance(cls, job):
113+ return cls._subclass[job.job_type](job)
114+
115+ @classmethod
116+ def get(cls, key, desired_classes=None):
117+ """Return the instance of this class (or a subclass) whose key
118+ is supplied. Calling this method on a subclass returns only values
119+ for that subclass.
120
121 :raises: SQLObjectNotFound
122 """
123- instance = IStore(BranchJob).get(BranchJob, key)
124- if instance is None or instance.job_type != cls.class_job_type:
125- raise SQLObjectNotFound(
126- 'No occurrence of %s has key %s' % (cls.__name__, key))
127- return cls(instance)
128+ if desired_classes is None:
129+ desired_classes = cls
130+ branchjob = IStore(BranchJob).get(BranchJob, key)
131+ if branchjob is not None:
132+ job = cls._getInstance(branchjob)
133+ if isinstance(job, cls):
134+ return job
135+ raise SQLObjectNotFound(
136+ 'No occurrence of %s has key %s' % (cls.__name__, key))
137
138 def getOopsVars(self):
139 """See `IRunnableJob`."""
140@@ -764,6 +793,26 @@
141 return outf.getvalue()
142
143
144+class BranchMailJobSource(BaseRunnableJobSource):
145+ """Source of jobs that send mail about branches."""
146+
147+ # This is the same limit used in the wrapper script.
148+ memory_limit = 1843200
149+
150+ @staticmethod
151+ def contextManager():
152+ return get_ro_server()
153+
154+ @staticmethod
155+ def iterReady():
156+ return chain(
157+ RevisionMailJob.iterReady(), RevisionsAddedJob.iterReady())
158+
159+ @staticmethod
160+ def get(key):
161+ return BranchJobDerived.get(key, (RevisionMailJob, RevisionsAddedJob))
162+
163+
164 class RosettaUploadJob(BranchJobDerived):
165 """A Job that uploads translation files to Rosetta."""
166
167
168=== modified file 'lib/lp/code/scripts/tests/test_sendbranchmail.py'
169--- lib/lp/code/scripts/tests/test_sendbranchmail.py 2011-06-28 23:43:37 +0000
170+++ lib/lp/code/scripts/tests/test_sendbranchmail.py 2011-07-11 18:22:18 +0000
171@@ -20,7 +20,6 @@
172 RevisionMailJob,
173 RevisionsAddedJob,
174 )
175-from lp.services.osutils import override_environ
176 from lp.testing import TestCaseWithFactory
177
178
179@@ -41,8 +40,7 @@
180 tree.add('foo')
181 # XXX: AaronBentley 2010-08-06 bug=614404: a bzr username is
182 # required to generate the revision-id.
183- with override_environ(BZR_EMAIL='me@example.com'):
184- tree.commit('Added foo.', rev_id='rev1')
185+ tree.commit('Added foo.', rev_id='rev1', committer='me@example.com')
186 return branch, tree
187
188 def test_sendbranchmail(self):
189@@ -55,7 +53,9 @@
190 retcode, stdout, stderr = run_script(
191 'cronscripts/sendbranchmail.py', [])
192 self.assertEqual(
193- 'INFO Creating lockfile: /var/lock/launchpad-sendbranchmail.lock\n'
194+ 'INFO Creating lockfile: '
195+ '/var/lock/launchpad-sendbranchmail.lock\n'
196+ 'INFO Running through Twisted.\n'
197 'INFO Ran 1 RevisionMailJobs.\n', stderr)
198 self.assertEqual('', stdout)
199 self.assertEqual(0, retcode)
200@@ -70,7 +70,8 @@
201 retcode, stdout, stderr = run_script(
202 'cronscripts/sendbranchmail.py', [])
203 self.assertIn(
204- 'INFO Creating lockfile: /var/lock/launchpad-sendbranchmail.lock\n',
205+ 'INFO Creating lockfile: '
206+ '/var/lock/launchpad-sendbranchmail.lock\n',
207 stderr)
208 self.assertIn('INFO Job resulted in OOPS:', stderr)
209 self.assertIn('INFO Ran 0 RevisionMailJobs.\n', stderr)
210@@ -82,17 +83,16 @@
211 self.useBzrBranches()
212 branch, tree = self.createBranch()
213 tree.bzrdir.root_transport.put_bytes('foo', 'baz')
214- # XXX: AaronBentley 2010-08-06 bug=614404: a bzr username is
215- # required to generate the revision-id.
216- with override_environ(BZR_EMAIL='me@example.com'):
217- tree.commit('Added foo.', rev_id='rev2')
218+ tree.commit('Added foo.', rev_id='rev2', committer='me@example.com')
219 RevisionsAddedJob.create(
220 branch, 'rev1', 'rev2', 'from@example.org')
221 transaction.commit()
222 retcode, stdout, stderr = run_script(
223 'cronscripts/sendbranchmail.py', [])
224 self.assertEqual(
225- 'INFO Creating lockfile: /var/lock/launchpad-sendbranchmail.lock\n'
226+ 'INFO Creating lockfile:'
227+ ' /var/lock/launchpad-sendbranchmail.lock\n'
228+ 'INFO Running through Twisted.\n'
229 'INFO Ran 1 RevisionMailJobs.\n',
230 stderr)
231 self.assertEqual('', stdout)
232
233=== modified file 'lib/lp/codehosting/vfs/transport.py'
234--- lib/lp/codehosting/vfs/transport.py 2011-06-28 23:43:37 +0000
235+++ lib/lp/codehosting/vfs/transport.py 2011-07-11 18:22:18 +0000
236@@ -102,7 +102,7 @@
237 """Return the absolute, escaped path to `relpath` without the schema.
238 """
239 return urlutils.joinpath(
240- self.base[len(self.server.get_url())-1:], relpath)
241+ self.base[len(self.server.get_url()) - 1:], relpath)
242
243 def _getUnderylingTransportAndPath(self, relpath):
244 """Return the underlying transport and path for `relpath`."""
245@@ -178,6 +178,7 @@
246
247 def iter_files_recursive(self):
248 deferred = self._getUnderylingTransportAndPath('.')
249+
250 @no_traceback_failures
251 def iter_files((transport, path)):
252 return transport.clone(path).iter_files_recursive()
253@@ -186,6 +187,7 @@
254
255 def listable(self):
256 deferred = self._getUnderylingTransportAndPath('.')
257+
258 @no_traceback_failures
259 def listable((transport, path)):
260 return transport.listable()
261@@ -427,3 +429,10 @@
262 return
263 self._is_started = False
264 unregister_transport(self.get_url(), self._transportFactory)
265+
266+ def __enter__(self):
267+ self.start_server()
268+ return self
269+
270+ def __exit__(self, exc_type, exc_value, traceback):
271+ self.stop_server()
272
273=== modified file 'lib/lp/registry/model/packagingjob.py'
274--- lib/lp/registry/model/packagingjob.py 2011-06-28 23:43:37 +0000
275+++ lib/lp/registry/model/packagingjob.py 2011-07-11 18:22:18 +0000
276@@ -35,6 +35,7 @@
277 JobStatus,
278 )
279 from lp.services.job.model.job import Job
280+from lp.services.utils import RegisteredSubclass
281
282
283 class PackagingJobType(DBEnumeratedType):
284@@ -96,13 +97,6 @@
285 self.productseries = productseries
286
287
288-class RegisteredSubclass(type):
289- """Metaclass for when subclasses should be registered."""
290-
291- def __init__(cls, name, bases, dict_):
292- cls._register_subclass(cls)
293-
294-
295 class PackagingJobDerived:
296 """Base class for specialized Packaging Job types."""
297
298@@ -120,10 +114,7 @@
299 @staticmethod
300 def _register_subclass(cls):
301 """Register this class with its enumeration."""
302- # This would be a classmethod, except that subclasses (e.g.
303- # TranslationPackagingJob) need to be able to override it and call
304- # into it, and there's no syntax to call a base class's version of a
305- # classmethod with the subclass as the first parameter.
306+ # Why not a classmethod? See RegisteredSubclass.__init__.
307 job_type = getattr(cls, 'class_job_type', None)
308 if job_type is not None:
309 value = cls._subclass.setdefault(job_type, cls)
310
311=== modified file 'lib/lp/services/utils.py'
312--- lib/lp/services/utils.py 2011-07-07 10:14:55 +0000
313+++ lib/lp/services/utils.py 2011-07-11 18:22:18 +0000
314@@ -18,6 +18,7 @@
315 'file_exists',
316 'iter_list_chunks',
317 'iter_split',
318+ 'RegisteredSubclass',
319 'run_capturing_output',
320 'synchronize',
321 'text_delta',
322@@ -132,7 +133,7 @@
323 I'm amazed this isn't in itertools (mwhudson).
324 """
325 for i in range(0, len(a_list), size):
326- yield a_list[i:i+size]
327+ yield a_list[i:i + size]
328
329
330 def synchronize(source, target, add, remove):
331@@ -242,7 +243,7 @@
332 then reassemble.
333 """
334 # Make sure there is at least one newline so the split works.
335- first, rest = (s+'\n').split('\n', 1)
336+ first, rest = (s + '\n').split('\n', 1)
337 return (first + '\n' + dedent(rest)).strip()
338
339
340@@ -290,6 +291,23 @@
341 sys._getframe(1).f_locals["__traceback_info__"] = info
342
343
344+class RegisteredSubclass(type):
345+ """Metaclass for when subclasses should be registered."""
346+
347+ def __init__(cls, name, bases, dict_):
348+ # _register_subclass must be a static method to permit upcalls.
349+ #
350+ # We cannot use super(Class, cls) to do the upcalls, because Class
351+ # isn't fully defined yet. (Remember, we're calling this from a
352+ # metaclass.)
353+ #
354+ # Without using super, a classmethod that overrides another
355+ # classmethod has no reasonable way to call the overridden version AND
356+ # provide its class as first parameter (i.e. "cls"). Therefore, we
357+ # must use a static method.
358+ cls._register_subclass(cls)
359+
360+
361 def utc_now():
362 """Return a timezone-aware timestamp for the current time."""
363 return datetime.now(tz=pytz.UTC)
364
365=== modified file 'lib/lp/translations/model/translationpackagingjob.py'
366--- lib/lp/translations/model/translationpackagingjob.py 2011-06-28 23:43:37 +0000
367+++ lib/lp/translations/model/translationpackagingjob.py 2011-07-11 18:22:18 +0000
368@@ -52,6 +52,7 @@
369
370 @staticmethod
371 def _register_subclass(cls):
372+ # Why not a classmethod? See RegisteredSubclass.__init__.
373 PackagingJobDerived._register_subclass(cls)
374 job_type = getattr(cls, 'class_job_type', None)
375 if job_type is not None:
376
377=== modified file 'lib/lp/translations/tests/test_translationtemplatesbuildjob.py'
378--- lib/lp/translations/tests/test_translationtemplatesbuildjob.py 2011-06-28 23:43:37 +0000
379+++ lib/lp/translations/tests/test_translationtemplatesbuildjob.py 2011-07-11 18:22:18 +0000
380@@ -156,6 +156,8 @@
381 behavior by setting an attribute of the class (not an object!) at
382 the beginning of every test.
383 """
384+ class_job_type = None
385+
386 # Fake _hasPotteryCompatibleSetup, and if so, make it give what
387 # answer?
388 fake_pottery_compatibility = None