Merge lp:~mwhudson/launchpad/incremental-code-imports-bug-512683 into lp:launchpad/db-devel

[Michael Hudson-Doyle]

Support incremental imports for git -- repositories will be imported 1000 revisions at a time.

[Michael Hudson-Doyle (mwhudson)]

Hi there,

This branch adds support to incrementally pull 5000 revisions at a time for git imports.

Not much more to say really, I hope the implementation mostly makes sense (at least if you know this area of the code).

Cheers,
mwh

[Michael Hudson-Doyle (mwhudson)]

Oh, the new icon looks like this: http://people.canonical.com/~mwh/partial-success.png

Do we need a UI review? I guess we probably should.

[Tim Penhey (thumper)]

lib/lp/codehosting/codeimport/worker.py

> + if bazaar_tree.branch.last_revision() \
> + == foreign_branch.last_revision():

Our coding standard does suggest to use braces rather than line continuation
characters.

Or even:

    last_revision = bazaar_tree.branch.last_revision()
    if (last_revision == foreign_branch.last_revision()):

Everything else looks good!

  merge approved

review: Approve

[Michael Hudson-Doyle (mwhudson)]

Hi Tim,

This branch failed tests because some other tests (the distrobrancher ones, I think) failed when run with the bzr-git plugin loaded. So I added a layer that prevents any tests from being run in the same process after the code import worker tests. Can you review this approach please? The interdiff is here: http://pastebin.ubuntu.com/381384/

With this change, the branch passed all tests in ec2.

Cheers,
mwh

[Tim Penhey (thumper)]

Add a comment about why this works, and land it. We can discuss the unplugability of the foreign branch bits at a later date.

review: Approve

=== modified file 'lib/canonical/config/schema-lazr.conf'
--- lib/canonical/config/schema-lazr.conf	2010-02-20 13:16:38 +0000
+++ lib/canonical/config/schema-lazr.conf	2010-02-22 05:40:42 +0000
@@ -439,6 +439,10 @@
 # in a row.
 consecutive_failure_limit: 5
 
+# Import only this many revisions at once.
+# Only applies to git imports for now.
+revisions_import_limit: 1000
+
 
 [codeimportdispatcher]
 # The directory where the code import worker should be directed to
 
=== added file 'lib/canonical/launchpad/images/yes-gray.png'
 Binary files lib/canonical/launchpad/images/yes-gray.png	1970-01-01 00:00:00 +0000 and lib/canonical/launchpad/images/yes-gray.png	2010-02-22 05:40:42 +0000 differ
=== modified file 'lib/canonical/launchpad/testing/codeimporthelpers.py'
--- lib/canonical/launchpad/testing/codeimporthelpers.py	2009-06-25 05:30:52 +0000
+++ lib/canonical/launchpad/testing/codeimporthelpers.py	2010-02-22 05:40:42 +0000
@@ -119,17 +119,13 @@
     return code_import
 
 
-def make_all_result_types(code_import=None, factory=None, machine=None):
+def make_all_result_types(code_import, factory, machine, start, count):
     """Make a code import result of each possible type for the code import."""
-    if factory is None:
-        factory = LaunchpadObjectFactory()
-    if code_import is None:
-        code_import = factory.makeCodeImport()
     start_dates = time_counter(
         datetime(2007,12,1,12, tzinfo=UTC), timedelta(days=1))
     end_dates = time_counter(
         datetime(2007,12,1,13, tzinfo=UTC), timedelta(days=1, hours=1))
-    for result_status in CodeImportResultStatus.items:
+    for result_status in sorted(CodeImportResultStatus.items)[start:start+count]:
         factory.makeCodeImportResult(
             code_import, result_status, start_dates.next(), end_dates.next(),
             machine=machine)
 
=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py	2010-02-01 03:49:23 +0000
+++ lib/lp/code/browser/branch.py	2010-02-22 05:40:42 +0000
@@ -514,6 +514,8 @@
         """The icon to represent the `CodeImportResultStatus` `status`."""
         if status in CodeImportResultStatus.successes:
             return "/@@/yes"
+        elif status == CodeImportResultStatus.SUCCESS_PARTIAL:
+            return "/@@/yes-gray"
         else:
             return "/@@/no"
 
 
=== modified file 'lib/lp/code/enums.py'
--- lib/lp/code/enums.py	2010-02-01 03:49:23 +0000
+++ lib/lp/code/enums.py	2010-02-22 05:40:42 +0000
@@ -806,6 +806,13 @@
         import.
         """)
 
+    SUCCESS_PARTIAL = DBItem(120, """
+        Partial Success
+
+        Import job successfully imported some but not all of the foreign
+        revisions.
+        """)
+
     FAILURE = DBItem(200, """
         Failure
 
 
=== modified file 'lib/lp/code/model/codeimportjob.py'
--- lib/lp/code/model/codeimportjob.py	2010-01-27 02:48:13 +0000
+++ lib/lp/code/model/codeimportjob.py	2010-02-22 05:40:42 +0000
@@ -286,7 +286,10 @@
                 dict(review_status=CodeImportReviewStatus.FAILING), None)
         # Only start a new one if the import is still in the REVIEWED state.
         if code_import.review_status == CodeImportReviewStatus.REVIEWED:
-            self.newJob(code_import)
+            extra = {}
+            if status == CodeImportResultStatus.SUCCESS_PARTIAL:
+                extra['date_due'] = UTC_NOW
+            self.newJob(code_import, **extra)
         # If the status was successful, update date_last_successful.
         if status in [CodeImportResultStatus.SUCCESS,
                       CodeImportResultStatus.SUCCESS_NOCHANGE]:
 
=== modified file 'lib/lp/code/model/tests/test_codeimportjob.py'
--- lib/lp/code/model/tests/test_codeimportjob.py	2010-02-01 03:55:59 +0000
+++ lib/lp/code/model/tests/test_codeimportjob.py	2010-02-22 05:40:42 +0000
@@ -776,6 +776,21 @@
             new_job.date_due - running_job.date_due,
             code_import.effective_update_interval)
 
+    def test_partialSuccessCreatesNewJobDueNow(self):
+        # If called with a status of SUCCESS_PARTIAL, finishJob() creates a
+        # new CodeImportJob for the given CodeImport that is due to run right
+        # now.
+        running_job = self.makeRunningJob()
+        code_import = running_job.code_import
+        self.switchDbUser()
+        getUtility(ICodeImportJobWorkflow).finishJob(
+            running_job, CodeImportResultStatus.SUCCESS_PARTIAL, None)
+        new_job = code_import.import_job
+        self.assert_(new_job is not None)
+        self.assertEqual(new_job.state, CodeImportJobState.PENDING)
+        self.assertEqual(new_job.machine, None)
+        self.assertSqlAttributeEqualsDate(new_job, 'date_due', UTC_NOW)
+
     def test_doesntCreateNewJobIfCodeImportNotReviewed(self):
         # finishJob() creates a new CodeImportJob for the given CodeImport,
         # unless the CodeImport has been suspended or marked invalid.
 
=== modified file 'lib/lp/code/stories/codeimport/xx-codeimport-results.txt'
--- lib/lp/code/stories/codeimport/xx-codeimport-results.txt	2010-01-27 03:49:06 +0000
+++ lib/lp/code/stories/codeimport/xx-codeimport-results.txt	2010-02-22 05:40:42 +0000
@@ -6,29 +6,28 @@
     >>> login('[e-mail]')
     >>> from canonical.launchpad.testing.codeimporthelpers import (
     ...     make_all_result_types)
-    >>> code_import = factory.makeCodeImport()
+    >>> code_import_1 = factory.makeCodeImport()
+    >>> code_import_2 = factory.makeCodeImport()
 
 The make_all_result_types helper method adds a code import result of
-each possible status value.
+each possible status value.  There are more status values than are
+shown on the branch page, so we create two imports in order to test
+how each result type is rendered.
 
     >>> odin = factory.makeCodeImportMachine(hostname='odin')
-    >>> make_all_result_types(code_import, factory, machine=odin)
-    >>> branch_url = canonical_url(code_import.branch)
+    >>> make_all_result_types(code_import_1, factory, machine=odin, start=0, count=7)
+    >>> branch_url_1 = canonical_url(code_import_1.branch)
+    >>> make_all_result_types(code_import_2, factory, machine=odin, start=7, count=7)
+    >>> branch_url_2 = canonical_url(code_import_2.branch)
     >>> logout()
 
 For each import result, the start date and finish date is shown along
 with the duration.  A link to the log file is shown if there was a log
 file stored with the result.
 
-    >>> browser.open(branch_url)
+    >>> browser.open(branch_url_1)
     >>> import_results = find_tag_by_id(browser.contents, 'import-results')
     >>> print extract_text(import_results).replace('—', '--')
-    Import started on 2007-12-10 on odin and finished on 2007-12-10
-      taking ten hours -- see the log
-    Import started on 2007-12-09 on odin and finished on 2007-12-09
-      taking nine hours -- see the log
-    Import started on 2007-12-08 on odin and finished on 2007-12-08
-      taking eight hours -- see the log
     Import started on 2007-12-07 on odin and finished on 2007-12-07
       taking seven hours -- see the log
     Import started on 2007-12-06 on odin and finished on 2007-12-06
@@ -46,19 +45,25 @@
 
 Each of the lines is prefixed with a tick if the result status was
 success, or a cross if the status was a failure.  The title of the image
-is the text of the failure type.
+is the text of the failure or success type.
 
     >>> # The ordering here is dependant on the order the status values
     >>> # are declared in the enumeration.
     >>> for img in import_results.fetch('img'):
     ...     print img
-    <img src="/@@/no" title="Job killed" />
-    <img src="/@@/no" title="Job reclaimed" />
-    <img src="/@@/no" title="Bazaar Update Failed" />
-    <img src="/@@/no" title="Source Update Failed" />
     <img src="/@@/no" title="Bazaar Import Failed" />
     <img src="/@@/no" title="Source Checkout Failed" />
     <img src="/@@/no" title="Internal Failure" />
     <img src="/@@/no" title="Failure" />
+    <img src="/@@/yes-gray" title="Partial Success" />
     <img src="/@@/yes" title="Success with no changes" />
     <img src="/@@/yes" title="Success" />
+
+    >>> browser.open(branch_url_2)
+    >>> import_results = find_tag_by_id(browser.contents, 'import-results')
+    >>> for img in import_results.fetch('img'):
+    ...     print img
+    <img src="/@@/no" title="Job killed" />
+    <img src="/@@/no" title="Job reclaimed" />
+    <img src="/@@/no" title="Bazaar Update Failed" />
+    <img src="/@@/no" title="Source Update Failed" />
 
=== modified file 'lib/lp/codehosting/codeimport/tests/test_worker.py'
--- lib/lp/codehosting/codeimport/tests/test_worker.py	2010-02-03 19:29:27 +0000
+++ lib/lp/codehosting/codeimport/tests/test_worker.py	2010-02-22 05:40:42 +0000
@@ -23,7 +23,6 @@
 
 from CVS import Repository, tree as CVSTree
 
-from canonical.cachedproperty import cachedproperty
 from canonical.config import config
 from canonical.launchpad.scripts.logger import QuietFakeLogger
 from canonical.testing import BaseLayer
@@ -37,22 +36,46 @@
     CVSServer, GitServer, MercurialServer, SubversionServer)
 from lp.codehosting.tests.helpers import (
     create_branch_with_one_revision)
-from lp.testing.factory import LaunchpadObjectFactory
+from lp.testing import TestCase
 
 import pysvn
 
 
+class ForeignBranchPluginLayer(BaseLayer):
+    """Ensure only specific tests are run with foreign branch plugins loaded.
+    """
+
+    @classmethod
+    def setUp(cls):
+        pass
+
+    @classmethod
+    def tearDown(cls):
+        # Raise NotImplementedError to signal that this layer cannot be torn
+        # down.  This means that the test runner will run subsequent tests in
+        # a different process.
+        raise NotImplementedError
+
+    @classmethod
+    def testSetUp(cls):
+        pass
+
+    @classmethod
+    def testTearDown(cls):
+        pass
+
+
 default_format = BzrDirFormat.get_default_format()
 
 
-class WorkerTest(TestCaseWithTransport):
+class WorkerTest(TestCaseWithTransport, TestCase):
     """Base test case for things that test the code import worker.
 
     Provides Bazaar testing features, access to Launchpad objects and
     factories for some code import objects.
     """
 
-    layer = BaseLayer
+    layer = ForeignBranchPluginLayer
 
     def setUp(self):
         TestCaseWithTransport.setUp(self)
@@ -70,10 +93,6 @@
         self.assertEqual(
             sorted(list_files(directory1)), sorted(list_files(directory2)))
 
-    @cachedproperty
-    def factory(self):
-        return LaunchpadObjectFactory()
-
     def makeTemporaryDirectory(self):
         directory = tempfile.mkdtemp()
         self.addCleanup(shutil.rmtree, directory)
@@ -906,6 +925,21 @@
         return self.factory.makeCodeImportSourceDetails(
             rcstype='git', url=repository_path)
 
+    def test_partial(self):
+        # Only config.codeimport.revisions_import_limit will be imported in a
+        # given run.  When bzr-svn and bzr-hg support revision import limits,
+        # this test case can be moved up to PullingImportWorkerTests.
+        worker = self.makeImportWorker(self.makeSourceDetails(
+            'trunk', [('README', 'Original contents')]))
+        self.makeForeignCommit(worker.source_details)
+        self.assertTrue(self.foreign_commit_count > 1)
+        self.pushConfig(
+            'codeimport', revisions_import_limit=self.foreign_commit_count-1)
+        self.assertEqual(
+            CodeImportWorkerExitCode.SUCCESS_PARTIAL, worker.run())
+        self.assertEqual(
+            CodeImportWorkerExitCode.SUCCESS, worker.run())
+
 
 class TestMercurialImport(WorkerTest, TestActualImportMixin,
                           PullingImportWorkerTests):
 
=== modified file 'lib/lp/codehosting/codeimport/tests/test_workermonitor.py'
--- lib/lp/codehosting/codeimport/tests/test_workermonitor.py	2010-02-03 19:29:27 +0000
+++ lib/lp/codehosting/codeimport/tests/test_workermonitor.py	2010-02-22 05:40:42 +0000
@@ -334,6 +334,21 @@
         # callFinishJob did not swallow the error, this will fail the test.
         return ret
 
+    def test_callFinishJobCallsFinishJobPartial(self):
+        # If the argument to callFinishJob indicates that the subprocess
+        # exited with a code of CodeImportWorkerExitCode.SUCCESS_PARTIAL, it
+        # calls finishJob with a status of SUCCESS_PARTIAL.
+        calls = self.patchOutFinishJob()
+        ret = self.worker_monitor.callFinishJob(
+            makeFailure(
+                error.ProcessTerminated,
+                exitCode=CodeImportWorkerExitCode.SUCCESS_PARTIAL))
+        self.assertEqual(calls, [CodeImportResultStatus.SUCCESS_PARTIAL])
+        self.assertOopsesLogged([])
+        # We return the deferred that callFinishJob returns -- if
+        # callFinishJob did not swallow the error, this will fail the test.
+        return ret
+
     def test_callFinishJobLogsTracebackOnFailure(self):
         # When callFinishJob is called with a failure, it dumps the traceback
         # of the failure into the log file.
 
=== modified file 'lib/lp/codehosting/codeimport/worker.py'
--- lib/lp/codehosting/codeimport/worker.py	2010-02-01 04:26:12 +0000
+++ lib/lp/codehosting/codeimport/worker.py	2010-02-22 05:40:42 +0000
@@ -21,7 +21,7 @@
 import os
 import shutil
 
-from bzrlib.branch import Branch
+from bzrlib.branch import Branch, InterBranch
 from bzrlib.bzrdir import BzrDir, BzrDirFormat
 from bzrlib.transport import get_transport
 from bzrlib.errors import NoSuchFile, NotBranchError
@@ -50,6 +50,7 @@
     SUCCESS = 0
     FAILURE = 1
     SUCCESS_NOCHANGE = 2
+    SUCCESS_PARTIAL = 3
 
 
 class BazaarBranchStore:
@@ -407,11 +408,7 @@
         saved_pwd = os.getcwd()
         os.chdir(working_directory)
         try:
-            non_trivial = self._doImport()
-            if non_trivial:
-                return CodeImportWorkerExitCode.SUCCESS
-            else:
-                return CodeImportWorkerExitCode.SUCCESS_NOCHANGE
+            return self._doImport()
         finally:
             shutil.rmtree(working_directory)
             os.chdir(saved_pwd)
@@ -497,7 +494,10 @@
         self.importToBazaar(foreign_tree, bazaar_tree)
         non_trivial = self.pushBazaarWorkingTree(bazaar_tree)
         self.foreign_tree_store.archive(foreign_tree)
-        return non_trivial
+        if non_trivial:
+            return CodeImportWorkerExitCode.SUCCESS
+        else:
+            return CodeImportWorkerExitCode.SUCCESS_NOCHANGE
 
 
 class PullingImportWorker(ImportWorker):
@@ -511,6 +511,15 @@
         """The format classes that should be tried for this import."""
         raise NotImplementedError
 
+    def getExtraPullArgs(self):
+        """Return extra arguments to `InterBranch.pull`.
+
+        This method only really exists because only bzr-git supports the
+        'limit' argument to this method.  When bzr-svn and bzr-hg plugin do
+        too, this method can go away.
+        """
+        return {}
+
     def _doImport(self):
         bazaar_tree = self.getBazaarWorkingTree()
         self.bazaar_branch_store.push(
@@ -529,10 +538,20 @@
             else:
                 raise NotBranchError(self.source_details.url)
             foreign_branch = format.open(transport).open_branch()
-            bazaar_tree.branch.pull(foreign_branch, overwrite=True)
+            inter_branch = InterBranch.get(foreign_branch, bazaar_tree.branch)
+            pull_result = inter_branch.pull(
+                overwrite=True, **self.getExtraPullArgs())
+            self.pushBazaarWorkingTree(bazaar_tree)
+            last_imported_revison = bazaar_tree.branch.last_revision()
+            if last_imported_revison == foreign_branch.last_revision():
+                if pull_result.old_revid != pull_result.new_revid:
+                    return CodeImportWorkerExitCode.SUCCESS
+                else:
+                    return CodeImportWorkerExitCode.SUCCESS_NOCHANGE
+            else:
+                return CodeImportWorkerExitCode.SUCCESS_PARTIAL
         finally:
             bzrlib.ui.ui_factory = saved_factory
-        return self.pushBazaarWorkingTree(bazaar_tree)
 
 
 class GitImportWorker(PullingImportWorker):
@@ -549,6 +568,10 @@
             LocalGitBzrDirFormat, RemoteGitBzrDirFormat)
         return [LocalGitBzrDirFormat, RemoteGitBzrDirFormat]
 
+    def getExtraPullArgs(self):
+        """See `PullingImportWorker.getExtraPullArgs`."""
+        return {'limit': config.codeimport.revisions_import_limit}
+
     def getBazaarWorkingTree(self):
         """See `ImportWorker.getBazaarWorkingTree`.
 
 
=== modified file 'lib/lp/codehosting/codeimport/workermonitor.py'
--- lib/lp/codehosting/codeimport/workermonitor.py	2010-02-01 03:49:23 +0000
+++ lib/lp/codehosting/codeimport/workermonitor.py	2010-02-22 05:40:42 +0000
@@ -302,6 +302,9 @@
                 if reason.value.exitCode == \
                        CodeImportWorkerExitCode.SUCCESS_NOCHANGE:
                     return CodeImportResultStatus.SUCCESS_NOCHANGE
+                elif reason.value.exitCode == \
+                       CodeImportWorkerExitCode.SUCCESS_PARTIAL:
+                    return CodeImportResultStatus.SUCCESS_PARTIAL
             return CodeImportResultStatus.FAILURE
         else:
             return CodeImportResultStatus.SUCCESS
 
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2010-02-20 13:16:38 +0000
+++ lib/lp/testing/factory.py	2010-02-22 05:40:42 +0000
@@ -255,6 +255,31 @@
             host = "%s.domain.com" % self.getUniqueString('domain')
         return '%s://%s/%s' % (scheme, host, self.getUniqueString('path'))
 
+    def makeCodeImportSourceDetails(self, branch_id=None, rcstype=None,
+                                    url=None, cvs_root=None, cvs_module=None):
+        if branch_id is None:
+            branch_id = self.getUniqueInteger()
+        if rcstype is None:
+            rcstype = 'svn'
+        if rcstype in ['svn', 'bzr-svn', 'hg']:
+            assert cvs_root is cvs_module is None
+            if url is None:
+                url = self.getUniqueURL()
+        elif rcstype == 'cvs':
+            assert url is None
+            if cvs_root is None:
+                cvs_root = self.getUniqueString()
+            if cvs_module is None:
+                cvs_module = self.getUniqueString()
+        elif rcstype == 'git':
+            assert cvs_root is cvs_module is None
+            if url is None:
+                url = self.getUniqueURL(scheme='git')
+        else:
+            raise AssertionError("Unknown rcstype %r." % rcstype)
+        return CodeImportSourceDetails(
+            branch_id, rcstype, url, cvs_root, cvs_module)
+
 
 class LaunchpadObjectFactory(ObjectFactory):
     """Factory methods for creating Launchpad objects.
@@ -1409,31 +1434,6 @@
             code_import, machine, requesting_user, log_excerpt, log_alias,
             result_status, date_started, date_finished)
 
-    def makeCodeImportSourceDetails(self, branch_id=None, rcstype=None,
-                                    url=None, cvs_root=None, cvs_module=None):
-        if branch_id is None:
-            branch_id = self.getUniqueInteger()
-        if rcstype is None:
-            rcstype = 'svn'
-        if rcstype in ['svn', 'bzr-svn', 'hg']:
-            assert cvs_root is cvs_module is None
-            if url is None:
-                url = self.getUniqueURL()
-        elif rcstype == 'cvs':
-            assert url is None
-            if cvs_root is None:
-                cvs_root = self.getUniqueString()
-            if cvs_module is None:
-                cvs_module = self.getUniqueString()
-        elif rcstype == 'git':
-            assert cvs_root is cvs_module is None
-            if url is None:
-                url = self.getUniqueURL(scheme='git')
-        else:
-            raise AssertionError("Unknown rcstype %r." % rcstype)
-        return CodeImportSourceDetails(
-            branch_id, rcstype, url, cvs_root, cvs_module)
-
     def makeCodeReviewComment(self, sender=None, subject=None, body=None,
                               vote=None, vote_tag=None, parent=None,
                               merge_proposal=None):
 
=== modified file 'utilities/sourcedeps.conf'
--- utilities/sourcedeps.conf	2010-02-18 13:53:20 +0000
+++ utilities/sourcedeps.conf	2010-02-22 05:40:42 +0000
@@ -1,10 +1,10 @@
 bzr-builder lp:~launchpad-pqm/bzr-builder/trunk;revno=63
-bzr-git lp:~launchpad-pqm/bzr-git/devel;revno=249
+bzr-git lp:~launchpad-pqm/bzr-git/devel;revno=250
 bzr-hg lp:~launchpad-pqm/bzr-hg/devel;revno=281
 bzr-loom lp:~launchpad-pqm/bzr-loom/trunk;revno=47
 bzr-svn lp:~launchpad-pqm/bzr-svn/devel;revno=2707
 cscvs lp:~launchpad-pqm/launchpad-cscvs/devel;revno=430
-dulwich lp:~launchpad-pqm/dulwich/devel;revno=415
+dulwich lp:~launchpad-pqm/dulwich/devel;revno=416
 launchpad-loggerhead lp:~launchpad-pqm/launchpad-loggerhead/devel;revno=54
 loggerhead lp:~launchpad-pqm/loggerhead/devel;revno=174
 lpreview lp:~launchpad-pqm/bzr-lpreview/devel;revno=23