Merge lp:~salgado/launchpad/use-meliae into lp:launchpad

Proposed by Guilherme Salgado
Status: Merged
Merged at revision: not available
Proposed branch: lp:~salgado/launchpad/use-meliae
Merge into: lp:launchpad
Diff against target: 110 lines (+68/-0)
5 files modified
lib/canonical/launchpad/webapp/configure.zcml (+5/-0)
lib/canonical/launchpad/webapp/sigdumpmem.py (+18/-0)
lib/canonical/launchpad/webapp/tests/test_sigdumpmem.py (+43/-0)
setup.py (+1/-0)
versions.cfg (+1/-0)
To merge this branch: bzr merge lp:~salgado/launchpad/use-meliae
Reviewer Review Type Date Requested Status
Francis J. Lacoste (community) Approve
Review via email: mp+21674@code.launchpad.net

Description of the change

Use https://launchpad.net/meliae to get a memory dump (out of a running process) that can be analyzed later. To test you'll need to install python-meliae (from the launchpad PPA), but once this is ready to land I'll add python-meliae to launchpad-dependencies.

To post a comment you must log in.
Revision history for this message
Francis J. Lacoste (flacoste) wrote :

* Don't we need to add meliae to the buildout?
* Can we use a configurable file instead of an hardcoded one?

review: Needs Information
Revision history for this message
Guilherme Salgado (salgado) wrote :

On Thu, 2010-03-18 at 19:48 +0000, Francis J. Lacoste wrote:
> Review: Needs Information
> * Don't we need to add meliae to the buildout?

We could, but since we already have it packaged in Lucid (I just
backported the package to Karmic and Hardy) and the release tarball is
not eggified, I thought I'd go with the easiest option. After all, if
we need a newer version we can easily switch to using an egg.

> * Can we use a configurable file instead of an hardcoded one?

I considered that, but I couldn't come up with any use cases that would
require changing the file path. I also try to use config values only
for things that change between environments -- which doesn't seem to be
the case here.

--
Guilherme Salgado <email address hidden>

Revision history for this message
Francis J. Lacoste (flacoste) wrote :

On March 18, 2010, Guilherme Salgado wrote:
> On Thu, 2010-03-18 at 19:48 +0000, Francis J. Lacoste wrote:
> > Review: Needs Information
> > * Don't we need to add meliae to the buildout?
>
> We could, but since we already have it packaged in Lucid (I just
> backported the package to Karmic and Hardy) and the release tarball is
> not eggified, I thought I'd go with the easiest option. After all, if
> we need a newer version we can easily switch to using an egg

Well, it does slow down the deployment of this, since they SA will have to
review/rebuild the package for Hardy (they don't deploy from our PPA) and
install it on edge/staging before you can land this.

By not eggified, you mean it doesn't have a setup.py?

> .
>
> > * Can we use a configurable file instead of an hardcoded one?
>
> I considered that, but I couldn't come up with any use cases that would
> require changing the file path. I also try to use config values only
> for things that change between environments -- which doesn't seem to be
> the case here.

Well, a hardcoded path under /tmp is vulnerable to symlink attacks
(overwriting of arbitrary files owned by the user).

--
Francis J. Lacoste
<email address hidden>

Revision history for this message
Guilherme Salgado (salgado) wrote :

On Thu, 2010-03-18 at 20:42 +0000, Francis J. Lacoste wrote:
> On March 18, 2010, Guilherme Salgado wrote:
> > On Thu, 2010-03-18 at 19:48 +0000, Francis J. Lacoste wrote:
> > > Review: Needs Information
> > > * Don't we need to add meliae to the buildout?
> >
> > We could, but since we already have it packaged in Lucid (I just
> > backported the package to Karmic and Hardy) and the release tarball is
> > not eggified, I thought I'd go with the easiest option. After all, if
> > we need a newer version we can easily switch to using an egg
>
> Well, it does slow down the deployment of this, since they SA will have to
> review/rebuild the package for Hardy (they don't deploy from our PPA) and
> install it on edge/staging before you can land this.
>
> By not eggified, you mean it doesn't have a setup.py?

The problem was that we can't seem to use buildout to make an egg out of
it because it uses pyrex/cython. After discussing this with Gary we came
to the conclusion that the least painful option would be to provide the
eggs ourselves, for now.

>
> > .
> >
> > > * Can we use a configurable file instead of an hardcoded one?
> >
> > I considered that, but I couldn't come up with any use cases that would
> > require changing the file path. I also try to use config values only
> > for things that change between environments -- which doesn't seem to be
> > the case here.
>
> Well, a hardcoded path under /tmp is vulnerable to symlink attacks
> (overwriting of arbitrary files owned by the user).

To exploit that one would need access to the file system, right, in
which case they could just as easily read the hard-coded value from the
config (if we were using a config value). Anyway, I'm willing to move
it to a config variable if you feel strong about it.

--
Guilherme Salgado <email address hidden>

Revision history for this message
Francis J. Lacoste (flacoste) wrote :

> > >
> > > > * Can we use a configurable file instead of an hardcoded one?
> > >
> > > I considered that, but I couldn't come up with any use cases that would
> > > require changing the file path. I also try to use config values only
> > > for things that change between environments -- which doesn't seem to be
> > > the case here.
> >
> > Well, a hardcoded path under /tmp is vulnerable to symlink attacks
> > (overwriting of arbitrary files owned by the user).
>
> To exploit that one would need access to the file system, right, in
> which case they could just as easily read the hard-coded value from the
> config (if we were using a config value). Anyway, I'm willing to move
> it to a config variable if you feel strong about it.
>

OK, fine.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/canonical/launchpad/webapp/configure.zcml'
--- lib/canonical/launchpad/webapp/configure.zcml 2010-03-16 05:08:47 +0000
+++ lib/canonical/launchpad/webapp/configure.zcml 2010-03-22 15:42:30 +0000
@@ -740,6 +740,11 @@
740 for="zope.app.appsetup.IProcessStartingEvent"740 for="zope.app.appsetup.IProcessStartingEvent"
741 handler="canonical.launchpad.webapp.sigusr2.setup_sigusr2"741 handler="canonical.launchpad.webapp.sigusr2.setup_sigusr2"
742 />742 />
743 <subscriber
744 for="zope.app.appsetup.IProcessStartingEvent"
745 handler="canonical.launchpad.webapp.sigdumpmem.setup_sigdumpmem"
746 />
747
743748
744 <!-- Confirm the database is the correct revision level -->749 <!-- Confirm the database is the correct revision level -->
745 <subscriber750 <subscriber
746751
=== added file 'lib/canonical/launchpad/webapp/sigdumpmem.py'
--- lib/canonical/launchpad/webapp/sigdumpmem.py 1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/webapp/sigdumpmem.py 2010-03-22 15:42:30 +0000
@@ -0,0 +1,18 @@
1# Copyright 2010 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).
3
4import signal
5
6from meliae import scanner
7
8
9SIGDUMPMEM = signal.SIGRTMIN + 10
10DUMP_FILE = '/tmp/launchpad-memory.dump'
11
12
13def sigdumpmem_handler(signum, frame):
14 scanner.dump_all_objects(DUMP_FILE)
15
16
17def setup_sigdumpmem(event):
18 signal.signal(SIGDUMPMEM, sigdumpmem_handler)
019
=== added file 'lib/canonical/launchpad/webapp/tests/test_sigdumpmem.py'
--- lib/canonical/launchpad/webapp/tests/test_sigdumpmem.py 1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/webapp/tests/test_sigdumpmem.py 2010-03-22 15:42:30 +0000
@@ -0,0 +1,43 @@
1# Copyright 2010 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).
3
4"""Test the SIGDUMPMEM signal handler."""
5
6__metaclass__ = type
7
8import os
9import time
10
11from canonical.lazr.pidfile import get_pid
12
13from canonical.config import config
14from canonical.launchpad.webapp.sigdumpmem import (
15 DUMP_FILE, SIGDUMPMEM)
16from canonical.testing.layers import AppServerLayer
17from lp.testing import TestCase
18
19
20class SIGDUMPMEMTestCase(TestCase):
21 layer = AppServerLayer
22
23 def test_sigdumpmem(self):
24 # Remove the dump file, if one exists.
25 if os.path.exists(DUMP_FILE):
26 os.unlink(DUMP_FILE)
27 self.assertFalse(os.path.exists(DUMP_FILE))
28
29 # Get the pid for the process spawned by the AppServerLayer, which is
30 # the one we'll be sending the signal to.
31 orig_instance_name = config.instance_name
32 config.setInstance('testrunner-appserver')
33 pid = get_pid('launchpad')
34 config.setInstance(orig_instance_name)
35
36 # Send the signal and ensure the dump file is created.
37 os.kill(pid, SIGDUMPMEM)
38 timeout = 10
39 start_time = time.time()
40 while time.time() < start_time + timeout:
41 if os.path.exists(DUMP_FILE):
42 break
43 self.assertTrue(os.path.exists(DUMP_FILE))
044
=== modified file 'setup.py'
--- setup.py 2010-01-20 22:09:26 +0000
+++ setup.py 2010-03-22 15:42:30 +0000
@@ -43,6 +43,7 @@
43 'lazr.uri',43 'lazr.uri',
44 'lazr-js',44 'lazr-js',
45 'mechanize',45 'mechanize',
46 'meliae',
46 'mercurial',47 'mercurial',
47 'mocker',48 'mocker',
48 'oauth',49 'oauth',
4950
=== modified file 'versions.cfg'
--- versions.cfg 2010-03-17 14:46:16 +0000
+++ versions.cfg 2010-03-22 15:42:30 +0000
@@ -36,6 +36,7 @@
36lazr-js = 0.9.2DEVr17036lazr-js = 0.9.2DEVr170
37martian = 0.1137martian = 0.11
38mechanize = 0.1.1138mechanize = 0.1.11
39meliae = 0.2.0.final.0
39mercurial = 1.3.140mercurial = 1.3.1
40mocker = 0.10.141mocker = 0.10.1
41mozrunner = 1.3.442mozrunner = 1.3.4