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

Proposed by Aaron Bentley
Status: Merged
Approved by: Aaron Bentley
Approved revision: no longer in the source branch.
Merged at revision: 13420
Proposed branch: lp://qastaging/~abentley/launchpad/memlimit-sendbranchmail
Merge into: lp://qastaging/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://qastaging/~abentley/launchpad/memlimit-sendbranchmail
Reviewer Review Type Date Requested Status
Aaron Bentley (community) Approve
Review via email: mp+67604@code.qastaging.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.
Revision history for this message
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
Revision history for this message
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-----

Revision history for this message
Aaron Bentley (abentley) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.