launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/code-import-push-bzr-ssh into lp:launchpad.

Commit message:
Push code imports over bzr+ssh rather than sftp.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1779572 in Launchpad itself: "svn import to bazaar stuck at "Fetching revisions:Inserting stream:Estimate""
  https://bugs.launchpad.net/launchpad/+bug/1779572

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/code-import-push-bzr-ssh/+merge/353243

Some imports seem to hang when pushing to sftp; I can reproduce this when experimenting locally, but bzr+ssh fixes things (as long as SSH connection sharing is disabled).

I've done this here rather than in the production configs in order that we can carry on pulling over sftp, which is apparently less CPU-intensive.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/code-import-push-bzr-ssh into lp:launchpad.

=== modified file 'lib/lp/codehosting/codeimport/tests/test_worker.py'
--- lib/lp/codehosting/codeimport/tests/test_worker.py	2018-05-06 08:52:34 +0000
+++ lib/lp/codehosting/codeimport/tests/test_worker.py	2018-08-16 12:53:42 +0000
@@ -169,6 +169,23 @@
             store.transport.base.rstrip('/'),
             config.codeimport.bazaar_branch_store.rstrip('/'))
 
+    def test__getMirrorURL(self):
+        # _getMirrorURL returns a URL for the branch with the given id.
+        store = BazaarBranchStore(get_transport_from_url(
+            'sftp://storage.example/branches'))
+        self.assertEqual(
+            'sftp://storage.example/branches/000186a0',
+            store._getMirrorURL(100000))
+
+    def test__getMirrorURL_push(self):
+        # _getMirrorURL prefers bzr+ssh over sftp when constructing push
+        # URLs.
+        store = BazaarBranchStore(get_transport_from_url(
+            'sftp://storage.example/branches'))
+        self.assertEqual(
+            'bzr+ssh://storage.example/branches/000186a0',
+            store._getMirrorURL(100000, push=True))
+
     def test_getNewBranch(self):
         # If there's no Bazaar branch of this id, then pull creates a new
         # Bazaar branch.

=== modified file 'lib/lp/codehosting/codeimport/worker.py'
--- lib/lp/codehosting/codeimport/worker.py	2018-03-15 20:44:04 +0000
+++ lib/lp/codehosting/codeimport/worker.py	2018-08-16 12:53:42 +0000
@@ -166,9 +166,18 @@
         """Construct a Bazaar branch store based at `transport`."""
         self.transport = transport
 
-    def _getMirrorURL(self, db_branch_id):
+    def _getMirrorURL(self, db_branch_id, push=False):
         """Return the URL that `db_branch` is stored at."""
-        return urljoin(self.transport.base, '%08x' % db_branch_id)
+        base_url = self.transport.base
+        if push:
+            # Pulling large branches over sftp is less CPU-intensive, but
+            # pushing over bzr+ssh seems to be more reliable.
+            split = urlsplit(base_url)
+            if split.scheme == 'sftp':
+                base_url = urlunsplit([
+                    'bzr+ssh', split.netloc, split.path, split.query,
+                    split.fragment])
+        return urljoin(base_url, '%08x' % db_branch_id)
 
     def pull(self, db_branch_id, target_path, required_format,
              needs_tree=False, stacked_on_url=None):
@@ -218,7 +227,7 @@
             (i.e. actually transferred revisions).
         """
         self.transport.create_prefix()
-        target_url = self._getMirrorURL(db_branch_id)
+        target_url = self._getMirrorURL(db_branch_id, push=True)
         try:
             remote_branch = Branch.open(target_url)
         except NotBranchError: