Merge lp:~mwhudson/launchpad/lp-serve-report-branch-access-bug-532210 into lp:launchpad

[Michael Hudson-Doyle]

Report the branch being accessed in the lp-serve process' process name

Hi there,

This branch reports which branch an lp-serve accesses in its process title.

I didn't have a pre-imp call for this, and if I did I probably wouldn't be so worried that the code in lp.codehosting.vfs.hooks isn't in a sensibly named file.

Cheers,
mwh

[Tim Penhey (thumper)]

Looks good. I hope it works :-)

review: Approve

[Jonathan Lange (jml)]

Cool! At first I thought this was about logging branch access to a file, but it's still great news.

This might be a silly question, but have you run this locally?

[Michael Hudson-Doyle (mwhudson)]

On 23/03/10 23:29, Jonathan Lange wrote:
> Cool! At first I thought this was about logging branch access to a file, but it's still great news.

Yeah, we should probably do that, but multiprocess synchronization argh.
  What I'd like is to have bzr lp-serve processes log to ~/.bzr.log.$pid
rather than all smearing into ~/.bzr.log but that was annoying last time
I looked into it.

> This might be a silly question, but have you run this locally?

Yes I have. It seems to work :)

Cheers,
mwh

=== modified file 'bzrplugins/lpserve.py'
--- bzrplugins/lpserve.py	2010-03-16 19:01:26 +0000
+++ bzrplugins/lpserve.py	2010-03-24 00:47:00 +0000
@@ -88,13 +88,14 @@
     def run(self, user_id, port=None, upload_directory=None,
             mirror_directory=None, branchfs_endpoint_url=None, inet=False):
         from lp.codehosting.bzrutils import install_oops_handler
-        from lp.codehosting.vfs import get_lp_server
+        from lp.codehosting.vfs import get_lp_server, hooks
         install_oops_handler(user_id)
         four_gig = int(4e9)
         resource.setrlimit(resource.RLIMIT_AS, (four_gig, four_gig))
+        seen_new_branch = hooks.SetProcTitleHook()
         lp_server = get_lp_server(
             int(user_id), branchfs_endpoint_url,
-            upload_directory, mirror_directory)
+            upload_directory, mirror_directory, seen_new_branch.seen)
         lp_server.start_server()
 
         old_lockdir_timeout = lockdir._DEFAULT_TIMEOUT_SECONDS
 
=== modified file 'lib/lp/codehosting/vfs/branchfs.py'
--- lib/lp/codehosting/vfs/branchfs.py	2010-02-14 23:02:37 +0000
+++ lib/lp/codehosting/vfs/branchfs.py	2010-03-24 00:47:00 +0000
@@ -368,16 +368,20 @@
         path on that transport.
     """
 
-    def __init__(self, scheme, authserver, user_id):
+    def __init__(self, scheme, authserver, user_id,
+                 seen_new_branch_hook=None):
         """Construct a LaunchpadServer.
 
         :param scheme: The URL scheme to use.
         :param authserver: An XML-RPC client that implements callRemote.
         :param user_id: The database ID for the user who is accessing
             branches.
+        :param seen_new_branch_hook: A callable that will be called once for
+            each branch accessed via this server.
         """
         AsyncVirtualServer.__init__(self, scheme)
-        self._authserver = BranchFileSystemClient(authserver, user_id)
+        self._authserver = BranchFileSystemClient(
+            authserver, user_id, seen_new_branch_hook=seen_new_branch_hook)
         self._is_start_server = False
 
     def translateVirtualPath(self, virtual_url_fragment):
@@ -556,7 +560,7 @@
     asyncTransportFactory = AsyncLaunchpadTransport
 
     def __init__(self, authserver, user_id, hosted_transport,
-                 mirror_transport):
+                 mirror_transport, seen_new_branch_hook=None):
         """Construct a `LaunchpadServer`.
 
         See `_BaseLaunchpadServer` for more information.
@@ -575,9 +579,12 @@
         :param mirror_transport: A Bazaar `Transport` that points to the
             "mirrored" area of Launchpad. See module docstring for more
             information.
+        :param seen_new_branch_hook: A callable that will be called once for
+            each branch accessed via this server.
         """
         scheme = 'lp-%d:///' % id(self)
-        super(LaunchpadServer, self).__init__(scheme, authserver, user_id)
+        super(LaunchpadServer, self).__init__(
+            scheme, authserver, user_id, seen_new_branch_hook)
         mirror_transport = get_readonly_transport(mirror_transport)
         self._transport_dispatch = TransportDispatch(
             hosted_transport, mirror_transport)
@@ -641,7 +648,7 @@
 
 
 def get_lp_server(user_id, branchfs_endpoint_url=None, hosted_directory=None,
-                  mirror_directory=None):
+                  mirror_directory=None, seen_new_branch_hook=None):
     """Create a Launchpad server.
 
     :param user_id: A unique database ID of the user whose branches are
@@ -649,6 +656,7 @@
     :param branchfs_endpoint_url: URL for the branch file system end-point.
     :param hosted_directory: Where the branches are uploaded to.
     :param mirror_directory: Where all Launchpad branches are mirrored.
+    :param seen_new_branch_hook:
     :return: A `LaunchpadServer`.
     """
     # Get the defaults from the config.
@@ -667,7 +675,7 @@
     mirror_transport = get_chrooted_transport(mirror_url)
     lp_server = LaunchpadServer(
         BlockingProxy(branchfs_client), user_id,
-        hosted_transport, mirror_transport)
+        hosted_transport, mirror_transport, seen_new_branch_hook)
     return lp_server
 
 
 
=== modified file 'lib/lp/codehosting/vfs/branchfsclient.py'
--- lib/lp/codehosting/vfs/branchfsclient.py	2009-06-25 04:06:00 +0000
+++ lib/lp/codehosting/vfs/branchfsclient.py	2010-03-24 00:47:00 +0000
@@ -46,18 +46,24 @@
     """
 
     def __init__(self, branchfs_endpoint, user_id, expiry_time=None,
-                 _now=time.time):
+                 seen_new_branch_hook=None, _now=time.time):
         """Construct a caching branchfs_endpoint.
 
         :param branchfs_endpoint: An XML-RPC proxy that implements callRemote.
         :param user_id: The database ID of the user who will be making these
             requests. An integer.
+        :param expiry_time: If supplied, only cache the results of
+            translatePath for this many seconds.  If not supplied, cache the
+            results of translatePath for as long as this instance exists.
+        :param seen_new_branch_hook: A callable that will be called with the
+            unique_name of each new branch that is accessed.
         """
         self._branchfs_endpoint = branchfs_endpoint
         self._cache = {}
         self._user_id = user_id
         self.expiry_time = expiry_time
         self._now = _now
+        self.seen_new_branch_hook = seen_new_branch_hook
 
     def _getMatchedPart(self, path, transport_tuple):
         """Return the part of 'path' that the endpoint actually matched."""
@@ -77,6 +83,8 @@
         (transport_type, data, trailing_path) = transport_tuple
         matched_part = self._getMatchedPart(path, transport_tuple)
         if transport_type == BRANCH_TRANSPORT:
+            if self.seen_new_branch_hook:
+                self.seen_new_branch_hook(matched_part.strip('/'))
             self._cache[matched_part] = (transport_type, data, self._now())
         return transport_tuple
 
 
=== added file 'lib/lp/codehosting/vfs/hooks.py'
--- lib/lp/codehosting/vfs/hooks.py	1970-01-01 00:00:00 +0000
+++ lib/lp/codehosting/vfs/hooks.py	2010-03-24 00:47:00 +0000
@@ -0,0 +1,25 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Implementations for the `seen_new_branch_hook` of `BranchFileSystemClient`.
+"""
+
+__metaclass__ = type
+__all__ = ['SetProcTitleHook']
+
+import setproctitle
+
+
+class SetProcTitleHook:
+    """Use seen() as the hook to report branch access in ps(1) output."""
+
+    def __init__(self, setproctitle_mod=None):
+        if setproctitle_mod is None:
+            setproctitle_mod = setproctitle
+        self.setproctitle_mod = setproctitle_mod
+        self.basename = setproctitle_mod.getproctitle()
+
+    def seen(self, branch_url):
+        branch_url = branch_url.strip('/')
+        self.setproctitle_mod.setproctitle(
+            self.basename + ' BRANCH:' + branch_url)
 
=== modified file 'lib/lp/codehosting/vfs/tests/test_branchfsclient.py'
--- lib/lp/codehosting/vfs/tests/test_branchfsclient.py	2009-06-25 04:06:00 +0000
+++ lib/lp/codehosting/vfs/tests/test_branchfsclient.py	2010-03-24 00:47:00 +0000
@@ -36,13 +36,14 @@
         """
         self.fake_time.advance(amount)
 
-    def makeClient(self, expiry_time=None):
+    def makeClient(self, expiry_time=None, seen_new_branch_hook=None):
         """Make a `BranchFileSystemClient`.
 
         The created client interacts with the InMemoryFrontend.
         """
         return BranchFileSystemClient(
             self._xmlrpc_client, self.user.id, expiry_time=expiry_time,
+            seen_new_branch_hook=seen_new_branch_hook,
             _now=self.fake_time.now)
 
     def test_translatePath(self):
@@ -213,6 +214,41 @@
         return deferred.addCallbacks(
             translated_successfully, failed_translation)
 
+    def test_seen_new_branch_hook_called_for_new_branch(self):
+        # A callable passed as the seen_new_branch_hook when constructing a
+        # BranchFileSystemClient will be called with a previously unseen
+        # branch's unique_name when a path for a that branch is translated for
+        # the first time.
+        seen_branches = []
+        client = self.makeClient(seen_new_branch_hook=seen_branches.append)
+        branch = self.factory.makeAnyBranch()
+        client.translatePath('/' + branch.unique_name + '/trailing')
+        self.assertEqual([branch.unique_name], seen_branches)
+
+    def test_seen_new_branch_hook_called_for_each_branch(self):
+        # The seen_new_branch_hook is called for a each branch that is
+        # accessed.
+        seen_branches = []
+        client = self.makeClient(seen_new_branch_hook=seen_branches.append)
+        branch1 = self.factory.makeAnyBranch()
+        branch2 = self.factory.makeAnyBranch()
+        client.translatePath('/' + branch1.unique_name + '/trailing')
+        client.translatePath('/' + branch2.unique_name + '/trailing')
+        self.assertEqual(
+            [branch1.unique_name, branch2.unique_name], seen_branches)
+
+    def test_seen_new_branch_hook_called_once_for_a_new_branch(self):
+        # The seen_new_branch_hook is only called once for a given branch.
+        seen_branches = []
+        client = self.makeClient(seen_new_branch_hook=seen_branches.append)
+        branch1 = self.factory.makeAnyBranch()
+        branch2 = self.factory.makeAnyBranch()
+        client.translatePath('/' + branch1.unique_name + '/trailing')
+        client.translatePath('/' + branch2.unique_name + '/trailing')
+        client.translatePath('/' + branch1.unique_name + '/different')
+        self.assertEqual(
+            [branch1.unique_name, branch2.unique_name], seen_branches)
+
 
 class TestTrapFault(TestCase):
     """Tests for `trap_fault`."""
 
=== added file 'lib/lp/codehosting/vfs/tests/test_hooks.py'
--- lib/lp/codehosting/vfs/tests/test_hooks.py	1970-01-01 00:00:00 +0000
+++ lib/lp/codehosting/vfs/tests/test_hooks.py	2010-03-24 00:47:00 +0000
@@ -0,0 +1,63 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for the hooks in lp.codehosting.vfs.hooks."""
+
+__metaclass__ = type
+
+import unittest
+
+from lp.codehosting.vfs.hooks import SetProcTitleHook
+from lp.testing import TestCase
+
+
+class FakeSetProcTitleModule:
+    """A fake for the setproctitle module.
+
+    The `setproctitle` module (obviously) has global effects, so can't really
+    be used in unit tests.  Instances of this class can be used as a safe
+    replacement.
+    """
+
+    def __init__(self, initial_title):
+        self.title = initial_title
+
+    def getproctitle(self):
+        return self.title
+
+    def setproctitle(self, new_title):
+        self.title = new_title
+
+
+class TestSetProcTitleHook(TestCase):
+    """Tests for `SetProcTitleHook`."""
+
+    def test_hook_once(self):
+        # Calling the hook once records the passed branch identifier in the
+        # process title.
+        initial_title = self.factory.getUniqueString()
+        setproctitle_mod = FakeSetProcTitleModule(initial_title)
+        branch_url = self.factory.getUniqueString()
+        hook = SetProcTitleHook(setproctitle_mod)
+        hook.seen(branch_url)
+        self.assertEqual(
+            initial_title + " BRANCH:" + branch_url,
+            setproctitle_mod.getproctitle())
+
+    def test_hook_twice(self):
+        # Calling the hook twice replaces the first branch identifier in the
+        # process title.
+        initial_title = self.factory.getUniqueString()
+        setproctitle_mod = FakeSetProcTitleModule(initial_title)
+        branch_url1 = self.factory.getUniqueString()
+        branch_url2 = self.factory.getUniqueString()
+        hook = SetProcTitleHook(setproctitle_mod)
+        hook.seen(branch_url1)
+        hook.seen(branch_url2)
+        self.assertEqual(
+            initial_title + " BRANCH:" + branch_url2,
+            setproctitle_mod.getproctitle())
+
+
+def test_suite():
+    return unittest.TestLoader().loadTestsFromName(__name__)
 
=== modified file 'setup.py'
--- setup.py	2010-03-19 14:25:54 +0000
+++ setup.py	2010-03-24 00:47:00 +0000
@@ -54,12 +54,13 @@
         'pytz',
         # This appears to be a broken indirect dependency from zope.security:
         'RestrictedPython',
+        'setproctitle',
         'setuptools',
         'sourcecodegen',
         'storm',
         'testtools',
         'transaction',
-        'Twisted', 
+        'Twisted',
         'wadllib',
         'z3c.pt',
         'z3c.ptcompat',
 
=== modified file 'versions.cfg'
--- versions.cfg	2010-03-23 18:27:27 +0000
+++ versions.cfg	2010-03-24 00:47:00 +0000
@@ -52,6 +52,7 @@
 pytz = 2009l
 RestrictedPython = 3.5.1
 roman = 1.4.0
+setproctitle = 1.0
 setuptools = 0.6c9
 simplejson = 2.0.9
 simplesettings = 0.4