Merge lp:~mwhudson/launchpad/puller-import-hack into lp:launchpad

[Michael Hudson-Doyle]

Use vfs-level copies when moving import branches around -- faster than 2a fetch, and safe because we control how the branches are made.

Hi,

This branch adds a few hacks to the handling of code imports to avoid the problems of cpu intensive slow fetch.

In particular, when getting the branch from the central area to the slave and when pulling an imported branch to the codehost for the first time, just copy the branch at the vfs level.

Deploying this on production depends on https://code.edge.launchpad.net/~mwhudson/lp-production-configs/sftp-imports/+merge/23369 being deployed.

Cheers,
mwh

[Aaron Bentley (abentley)]

What is the impact of removing the testrunner bzr_imports_root_url? Does this cause it to default to a more suitable value?

Instead of remote_bzr_dir.transport.clone('..'), you should be able to use remote_bzr_dir.root_transport.

Some of the test changes look like drive-bys, e.g. not wanting it to leave directories behind. Is that right?

Is there a reason to join config.launchpad.bzr_imports_root_url and the branch id with path.join instead of urlutils.join?

Did you consider using more specific name than _is_import? e.g. _vfs_initial_clone ? Or what about providing an initial_clone method on the BranchPolicy itself?

What about using sets to represent files_before and files_after? That's closer to the abstract representation, because order isn't important, and entries can only appear once. I don't really understand the purpose of this loop, either.

It might be nice to turn _runWithTransformFallbackLocationHookInstalled into a ContextManager, but that's not necessary for this branch.

review: Needs Information

[Michael Hudson-Doyle (mwhudson)]

On 15/04/10 03:41, Aaron Bentley wrote:
> Review: Needs Information
> What is the impact of removing the testrunner bzr_imports_root_url? Does this cause it to default to a more suitable value?

Yes, the value in the development config is a file:/// url.

> Instead of remote_bzr_dir.transport.clone('..'), you should be able to use remote_bzr_dir.root_transport.

Ah, cool, thanks.

> Some of the test changes look like drive-bys, e.g. not wanting it to leave directories behind. Is that right?

I don't think I've really changed anything here, just folded the
addCleanup that followed every invocation of makeCleanDirectory into the
method itself.

> Is there a reason to join config.launchpad.bzr_imports_root_url and the branch id with path.join instead of urlutils.join?

No, none. Fixed.

> Did you consider using more specific name than _is_import? e.g. _vfs_initial_clone ? Or what about providing an initial_clone method on the BranchPolicy itself?

I thought about that name for about a microsecond, I have to admit.

Making it a method on the policy makes sense, partly because then it can
be tested separately...

> What about using sets to represent files_before and files_after? That's closer to the abstract representation, because order isn't important, and entries can only appear once. I don't really understand the purpose of this loop, either.

Yes, good point.

The point of the loop is to cope with the not-quite-entirely-theoretical
possibility that the branch being copied might be being written to as we
grab it. I think it can be written a bit more clearly now you mention
it, take a look.

> It might be nice to turn _runWithTransformFallbackLocationHookInstalled into a ContextManager, but that's not necessary for this branch.

Please no :-)

I think I have a strategy for getting rid of using the transform
fallback hook entirely now bzr has an ignore_fallbacks argument for
branch opening, but I'm more likely to do that in my no-hosted-area work
than here :)

Interdiff attached, although just looking at the preview diff again is
likely just as clear.

Cheers,
mwh

[Aaron Bentley (abentley)]

There was still another spot where you could use root_transport in lib/lp/codehosting/codeimport/worker.py, but otherwise, looks good.

review: Approve

=== modified file 'configs/testrunner/launchpad-lazr.conf'
--- configs/testrunner/launchpad-lazr.conf	2010-03-30 17:25:52 +0000
+++ configs/testrunner/launchpad-lazr.conf	2010-04-15 06:06:18 +0000
@@ -127,7 +127,6 @@
 
 [launchpad]
 max_attachment_size: 1024
-bzr_imports_root_url: http://localhost:10899
 geoip_database: /usr/share/GeoIP/GeoLiteCity.dat
 
 [launchpad_session]
 
=== modified file 'lib/lp/codehosting/codeimport/worker.py'
--- lib/lp/codehosting/codeimport/worker.py	2010-04-12 01:42:34 +0000
+++ lib/lp/codehosting/codeimport/worker.py	2010-04-15 06:06:18 +0000
@@ -89,14 +89,18 @@
             except NoSuchFile:
                 pass
             upgrade(remote_url, required_format)
-        local_branch = remote_bzr_dir.sprout(
-            target_path, create_tree_if_local=needs_tree).open_branch()
-        # Because of the way we do incremental imports, there may be revisions
-        # in the branch's repo that are not in the ancestry of the branch tip.
-        # We need to transfer them too.
-        local_branch.repository.fetch(
-            remote_bzr_dir.open_repository())
-        return local_branch
+        # The proper thing to do here would be to call
+        # "remote_bzr_dir.sprout()".  But 2a fetch slowly checks which
+        # revisions are in the ancestry of the tip of the remote branch, which
+        # we strictly don't care about, so we just copy the whole thing down
+        # at the vfs level.
+        target = get_transport(target_path)
+        target.ensure_base()
+        remote_bzr_dir.root_transport.copy_tree_to_transport(target)
+        local_bzr_dir = BzrDir.open_from_transport(target)
+        if needs_tree:
+            local_bzr_dir.create_workingtree()
+        return local_bzr_dir.open_branch()
 
     def push(self, db_branch_id, bzr_branch, required_format):
         """Push up `bzr_branch` as the Bazaar branch for `code_import`.
 
=== modified file 'lib/lp/codehosting/puller/tests/__init__.py'
--- lib/lp/codehosting/puller/tests/__init__.py	2009-11-21 00:28:10 +0000
+++ lib/lp/codehosting/puller/tests/__init__.py	2010-04-15 06:06:18 +0000
@@ -135,6 +135,7 @@
         if os.path.exists(path):
             shutil.rmtree(path)
         os.makedirs(path)
+        self.addCleanup(shutil.rmtree, path)
 
     def pushToBranch(self, branch, tree):
         """Push a Bazaar branch to a given Launchpad branch's hosted area.
 
=== modified file 'lib/lp/codehosting/puller/tests/test_acceptance.py'
--- lib/lp/codehosting/puller/tests/test_acceptance.py	2010-02-24 04:24:01 +0000
+++ lib/lp/codehosting/puller/tests/test_acceptance.py	2010-04-15 06:06:18 +0000
@@ -8,10 +8,8 @@
 
 
 import os
-import shutil
 from subprocess import PIPE, Popen
 import unittest
-from urlparse import urlparse
 
 import transaction
 
@@ -22,6 +20,7 @@
 from bzrlib.tests import HttpServer
 from bzrlib.transport import get_transport
 from bzrlib.upgrade import upgrade
+from bzrlib.urlutils import join as urljoin, local_path_from_url
 
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
@@ -49,11 +48,9 @@
         self._puller_script = os.path.join(
             config.root, 'cronscripts', 'supermirror-pull.py')
         self.makeCleanDirectory(config.codehosting.hosted_branches_root)
-        self.addCleanup(
-            shutil.rmtree, config.codehosting.hosted_branches_root)
         self.makeCleanDirectory(config.codehosting.mirrored_branches_root)
-        self.addCleanup(
-            shutil.rmtree, config.codehosting.mirrored_branches_root)
+        self.makeCleanDirectory(
+            local_path_from_url(config.launchpad.bzr_imports_root_url))
 
     def assertMirrored(self, db_branch, source_branch=None,
                        accessing_user=None):
@@ -136,10 +133,9 @@
         retcode, output, error = self.runSubprocess(command)
         return command, retcode, output, error
 
-    def serveOverHTTP(self, port=0):
+    def serveOverHTTP(self):
         """Serve the current directory over HTTP, returning the server URL."""
         http_server = HttpServer()
-        http_server.port = port
         http_server.start_server()
         # Join cleanup added before the tearDown so the tearDown is executed
         # first as this tells the thread to die.  We then join explicitly as
@@ -392,18 +388,6 @@
         self.assertRaises(
             errors.NotStacked, mirrored_branch.get_stacked_on_url)
 
-    def _getImportMirrorPort(self):
-        """Return the port used to serve imported branches, as specified in
-        config.launchpad.bzr_imports_root_url.
-        """
-        address = urlparse(config.launchpad.bzr_imports_root_url)[1]
-        host, port = address.split(':')
-        self.assertEqual(
-            'localhost', host,
-            'bzr_imports_root_url must be configured on localhost: %s'
-            % (config.launchpad.bzr_imports_root_url,))
-        return int(port)
-
     def test_mirror_imported_branch(self):
         # Run the puller on a populated imported branch pull queue.
         # Create the branch in the database.
@@ -412,18 +396,18 @@
         db_branch.requestMirror()
         transaction.commit()
 
-        # Create the Bazaar branch and serve it in the expected location.
-        branch_path = '%08x' % db_branch.id
-        os.mkdir(branch_path)
-        tree = self.make_branch_and_tree(branch_path)
+        # Create the Bazaar branch in the expected location.
+        branch_url = urljoin(
+            config.launchpad.bzr_imports_root_url, '%08x' % db_branch.id)
+        branch = BzrDir.create_branch_convenience(branch_url)
+        tree = branch.bzrdir.open_workingtree()
         tree.commit('rev1')
-        self.serveOverHTTP(self._getImportMirrorPort())
 
         # Run the puller.
         command, retcode, output, error = self.runPuller()
         self.assertRanSuccessfully(command, retcode, output, error)
 
-        self.assertMirrored(db_branch, source_branch=tree.branch)
+        self.assertMirrored(db_branch, source_branch=branch)
 
     def test_mirror_empty(self):
         # Run the puller on an empty pull queue.
 
=== modified file 'lib/lp/codehosting/puller/worker.py'
--- lib/lp/codehosting/puller/worker.py	2010-01-20 20:56:29 +0000
+++ lib/lp/codehosting/puller/worker.py	2010-04-15 06:06:18 +0000
@@ -11,9 +11,6 @@
 from bzrlib.branch import Branch
 from bzrlib.bzrdir import BzrDir
 from bzrlib import errors
-from bzrlib.plugins.loom.branch import LoomSupport
-from bzrlib.transport import get_transport
-from bzrlib import urlutils
 from bzrlib.ui import SilentUIFactory
 import bzrlib.ui
 
@@ -227,41 +224,14 @@
         'source_branch'. Any content already at 'destination_url' will be
         deleted.
 
-        If 'source_branch' is stacked, then the destination branch will be
-        stacked on the same URL, relative to 'destination_url'.
-
         :param source_branch: The Bazaar branch that will be mirrored.
         :param destination_url: The place to make the destination branch. This
             URL must point to a writable location.
         :return: The destination branch.
         """
-        dest_transport = get_transport(destination_url)
-        if dest_transport.has('.'):
-            dest_transport.delete_tree('.')
-        bzrdir = source_branch.bzrdir
-        # We check to see if the stacked on branch exists in the mirrored area
-        # so that we can nicely signal to the scheduler that the pulling of
-        # this branch should be deferred before we even create the branch in
-        # the mirrored area.
-        stacked_on_url = (
-            self.policy.getStackedOnURLForDestinationBranch(
-                source_branch, destination_url))
-        if stacked_on_url is not None:
-            stacked_on_url = urlutils.join(destination_url, stacked_on_url)
-            try:
-                Branch.open(stacked_on_url)
-            except errors.NotBranchError:
-                raise StackedOnBranchNotFound()
-        if isinstance(source_branch, LoomSupport):
-            # Looms suck.
-            revision_id = None
-        else:
-            revision_id = 'null:'
-        self._runWithTransformFallbackLocationHookInstalled(
-            bzrdir.clone_on_transport, dest_transport,
-            revision_id=revision_id)
-        branch = Branch.open(destination_url)
-        return branch
+        return self._runWithTransformFallbackLocationHookInstalled(
+            self.policy.createDestinationBranch, source_branch,
+            destination_url)
 
     def openDestinationBranch(self, source_branch, destination_url):
         """Open or create the destination branch at 'destination_url'.
@@ -269,9 +239,7 @@
         :param source_branch: The Bazaar branch that will be mirrored.
         :param destination_url: The place to make the destination branch. This
             URL must point to a writable location.
-        :return: (branch, up_to_date), where 'branch' is the destination
-            branch, and 'up_to_date' is a boolean saying whether the returned
-            branch is up-to-date with the source branch.
+        :return: The opened or created branch.
         """
         try:
             branch = Branch.open(destination_url)
 
=== modified file 'lib/lp/codehosting/vfs/branchfs.py'
--- lib/lp/codehosting/vfs/branchfs.py	2010-04-09 12:39:23 +0000
+++ lib/lp/codehosting/vfs/branchfs.py	2010-04-15 06:06:18 +0000
@@ -66,10 +66,12 @@
 
 import xmlrpclib
 
+from bzrlib.branch import Branch
 from bzrlib.bzrdir import BzrDir, BzrDirFormat
 from bzrlib.errors import (
     NoSuchFile, NotBranchError, NotStacked, PermissionDenied,
     TransportNotPossible, UnstackableBranchFormat)
+from bzrlib.plugins.loom.branch import LoomSupport
 from bzrlib.transport import get_transport
 from bzrlib.transport.memory import MemoryServer
 from bzrlib import urlutils
@@ -697,6 +699,46 @@
     stacked.
     """
 
+    def createDestinationBranch(self, source_branch, destination_url):
+        """Create a destination branch for 'source_branch'.
+
+        Creates a branch at 'destination_url' that is has the same format as
+        'source_branch'.  Any content already at 'destination_url' will be
+        deleted.  Generally the new branch will have no revisions, but they
+        will be copied for import branches, because this can be done safely
+        and efficiently with a vfs-level copy (see `ImportedBranchPolicy`,
+        below).
+
+        :param source_branch: The Bazaar branch that will be mirrored.
+        :param destination_url: The place to make the destination branch. This
+            URL must point to a writable location.
+        :return: The destination branch.
+        :raises StackedOnBranchNotFound: if the branch that the destination
+            branch will be stacked on does not yet exist in the mirrored area.
+        """
+        dest_transport = get_transport(destination_url)
+        if dest_transport.has('.'):
+            dest_transport.delete_tree('.')
+        stacked_on_url = (
+            self.getStackedOnURLForDestinationBranch(
+                source_branch, destination_url))
+        if stacked_on_url is not None:
+            stacked_on_url = urlutils.join(destination_url, stacked_on_url)
+            try:
+                Branch.open(stacked_on_url)
+            except NotBranchError:
+                from lp.codehosting.puller.worker import (
+                    StackedOnBranchNotFound)
+                raise StackedOnBranchNotFound()
+        if isinstance(source_branch, LoomSupport):
+            # Looms suck.
+            revision_id = None
+        else:
+            revision_id = 'null:'
+        source_branch.bzrdir.clone_on_transport(
+            dest_transport, revision_id=revision_id)
+        return Branch.open(destination_url)
+
     def getStackedOnURLForDestinationBranch(self, source_branch,
                                             destination_url):
         """Return the URL of the branch to stack the mirrored copy on.
@@ -766,15 +808,6 @@
      - assert we're pulling from a lp-hosted:/// URL.
     """
 
-    def shouldFollowReferences(self):
-        """See `BranchPolicy.shouldFollowReferences`.
-
-        We do not traverse references for HOSTED branches because that may
-        cause us to connect to remote locations, which we do not allow because
-        we want hosted branches to be mirrored quickly.
-        """
-        return False
-
     def _bzrdirExists(self, url):
         """Return whether a BzrDir exists at `url`."""
         try:
@@ -924,6 +957,27 @@
      - assert the URLs start with the prefix we expect for imported branches.
     """
 
+    def createDestinationBranch(self, source_branch, destination_url):
+        """See `BranchPolicy.createDestinationBranch`.
+
+        Because we control the process that creates import branches, a
+        vfs-level copy is safe and more efficient than a bzr fetch.
+        """
+        source_transport = source_branch.bzrdir.root_transport
+        dest_transport = get_transport(destination_url)
+        while True:
+            # We loop until the remote file list before and after the copy is
+            # the same to catch the case where the remote side is being
+            # mutated as we copy it.
+            if dest_transport.has('.'):
+                dest_transport.delete_tree('.')
+            files_before = set(source_transport.iter_files_recursive())
+            source_transport.copy_tree_to_transport(dest_transport)
+            files_after = set(source_transport.iter_files_recursive())
+            if files_before == files_after:
+                break
+        return Branch.open_from_transport(dest_transport)
+
     def shouldFollowReferences(self):
         """See `BranchPolicy.shouldFollowReferences`.