Merge lp:~jelmer/launchpad/upgrade-stderr into lp:launchpad

[Jelmer Vernooij]

[r=jcsackett,mbp][bug=808035] Fix processing of code imports that are currently stored in an old bzr format.

Fix processing of code imports that are in an old bzr format:

 1) Implement show_user_warning in the custom LoggingUIFactory used in the code import
    workers. This fixes the immediate issue that causes the traceback,
    and will prevent issues in the future if parts of bazaar print user warnings.

 2) Reintroduce the automatic upgrading of code imports, and fix the related test.
    Previously the test would compare the formats of the bzr dirs, both of which
    would be "Bazaar-NG meta directory, format 1". The test now compares the
    repository formats, which are different.

[Martin Pool (mbp)]

merge approve
vote approve

I wonder if the automatic upgrades will be safe. Perhaps you should check
with Tim or guard it with a flag, if they're available there.
On Jul 21, 2011 3:30 AM, "Jelmer Vernooij" <email address hidden> wrote:
> Jelmer Vernooij has proposed merging lp:~jelmer/launchpad/upgrade-stderr
into lp:launchpad.
>
> Requested reviews:
> Launchpad code reviewers (launchpad-reviewers)
> Related bugs:
> Bug #808035 in Launchpad itself: "AttributeError: 'NoneType' object has no
attribute 'write' during git import"
> https://bugs.launchpad.net/launchpad/+bug/808035
>
> For more details, see:
> https://code.launchpad.net/~jelmer/launchpad/upgrade-stderr/+merge/68579
>
> Fix processing of code imports that are in an old bzr format:
>
> 1) Implement show_user_warning in the custom LoggingUIFactory used in the
code import
> workers. This fixes the immediate issue that causes the traceback,
> and will prevent issues in the future if parts of bazaar print user
warnings.
>
> 2) Reintroduce the automatic upgrading of code imports, and fix the
related test.
> Previously the test would compare the formats of the bzr dirs, both of
which
> would be "Bazaar-NG meta directory, format 1". The test now compares the
> repository formats, which are different.
> --
> https://code.launchpad.net/~jelmer/launchpad/upgrade-stderr/+merge/68579
> You are subscribed to branch lp:launchpad.

[j.c.sackett (jcsackett)]

I'm going to second pinging Tim to make sure this is safe; I see that the associated bug in his XXX is fix released (though its flags seem off), but I'd like to make sure we can safely throw the automatic upgrade switch before approving.

review: Needs Information

[Jelmer Vernooij (jelmer)]

Hi Tim,

Since you last touched the auto upgrade code in Launchpad before me, can you perhaps review this branch? We've had some fall out from this earlier, so would like to be extra sure it's doing the right thing.

[Jelmer Vernooij (jelmer)]

> I'm going to second pinging Tim to make sure this is safe; I see that the
> associated bug in his XXX is fix released (though its flags seem off), but I'd
> like to make sure we can safely throw the automatic upgrade switch before
> approving.
Thanks for the review.

The XXX bug was fixed earlier but then rolled back because of an incident ( https://wiki.canonical.com/IncidentReports/2011-06-17-LP-codeimport-upgrade-breakage)

[Tim Penhey (thumper)]

The primary reason that we disabled the automatic upgrade was that there are hundreds, maybe thousands of code imports in the old format. Also upgrading to packs was a slow process. We only had so code import workers, and we didn't want to clog them all up doing upgrades.

Perhaps the best solution is to work out how to limit the number of workers on any given slave are doing upgrades.

[Martin Pool (mbp)]

Perhaps we should land just the stderr fix, and then do the upgrades
asynchronously from imports?

[Jelmer Vernooij (jelmer)]

Hi Tim!

Thanks for looking at this.

> The primary reason that we disabled the automatic upgrade was that there are
> hundreds, maybe thousands of code imports in the old format. Also upgrading
> to packs was a slow process. We only had so code import workers, and we
> didn't want to clog them all up doing upgrades.
>
> Perhaps the best solution is to work out how to limit the number of workers on
> any given slave are doing upgrades.
I don't think this problem is as bad as it used to be. The upgrade code in Bazaar has improved really significantly since the auto-upgrades were disabled in Launchpad, and is now quicker than the foreign plugins.

All of the branches that were enabled and in the older format have been marked as failing because of the regression (introduced around July 1) that's fixed by this MP. According to the FailingCodeImportsMetrics graph, that appears to be between 65 branches and 100 branches.

[Jelmer Vernooij (jelmer)]

Another thing that should make the upgrade a lot quicker is that the upgrades now happen on the local copy of the workers, rather than on the vcs-imports mirror copy.

[Martin Pool (mbp)]

this still looks reasonable to me, though it perhaps still needs a review from ~launchpad

review: Approve

[j.c.sackett (jcsackett)]

Jelmer has pointed out that all the old repositories needing upgrading are disabled right now; we can land this, and enable those repositories a few at a time to avoid any issues.

Given that, I think this code looks good to land.

review: Approve

=== modified file 'lib/lp/codehosting/codeimport/tests/test_uifactory.py'
--- lib/lp/codehosting/codeimport/tests/test_uifactory.py	2010-08-20 20:31:18 +0000
+++ lib/lp/codehosting/codeimport/tests/test_uifactory.py	2011-07-26 08:48:53 +0000
@@ -98,6 +98,16 @@
              'hi 3/10'],
             self.messages)
 
+    def test_user_warning(self):
+        factory = self.makeLoggingUIFactory()
+        factory.show_user_warning('cross_format_fetch',
+            from_format="athing", to_format="anotherthing")
+        message = factory._user_warning_templates['cross_format_fetch'] % {
+            "from_format": "athing",
+            "to_format": "anotherthing",
+            }
+        self.assertEqual([message], self.messages)
+
 
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/codehosting/codeimport/tests/test_worker.py'
--- lib/lp/codehosting/codeimport/tests/test_worker.py	2011-06-21 05:38:15 +0000
+++ lib/lp/codehosting/codeimport/tests/test_worker.py	2011-07-26 08:48:53 +0000
@@ -24,12 +24,10 @@
     )
 from bzrlib.errors import (
     NoSuchFile,
-    NotBranchError,
     )
 from bzrlib.tests import TestCaseWithTransport
 from bzrlib import trace
 from bzrlib.transport import get_transport
-from bzrlib.upgrade import upgrade
 from bzrlib.urlutils import (
     join as urljoin,
     local_path_from_url,
@@ -210,16 +208,17 @@
         store = self.makeBranchStore()
         target_url = store._getMirrorURL(self.arbitrary_branch_id)
         knit_format = format_registry.get('knit')()
-        create_branch_with_one_revision(target_url, format=knit_format)
-        default_format = BzrDirFormat.get_default_format()
+        tree = create_branch_with_one_revision(target_url, format=knit_format)
+        self.assertNotEquals(tree.bzrdir._format.repository_format.network_name(),
+            default_format.repository_format.network_name())
 
         # The fetched branch is in the default format.
         new_branch = store.pull(
             self.arbitrary_branch_id, self.temp_dir, default_format)
         # Make sure backup.bzr is removed, as it interferes with CSCVS.
         self.assertEquals(os.listdir(self.temp_dir), [".bzr"])
-        self.assertEqual(
-            default_format, new_branch.bzrdir._format)
+        self.assertEquals(new_branch.repository._format.network_name(),
+            default_format.repository_format.network_name())
 
     def test_pushUpgradesFormat(self):
         # A branch should always be in the most up-to-date format before a
@@ -228,7 +227,6 @@
         target_url = store._getMirrorURL(self.arbitrary_branch_id)
         knit_format = format_registry.get('knit')()
         create_branch_with_one_revision(target_url, format=knit_format)
-        default_format = BzrDirFormat.get_default_format()
 
         # The fetched branch is in the default format.
         new_branch = store.pull(

=== modified file 'lib/lp/codehosting/codeimport/uifactory.py'
--- lib/lp/codehosting/codeimport/uifactory.py	2010-08-20 20:31:18 +0000
+++ lib/lp/codehosting/codeimport/uifactory.py	2011-07-26 08:48:53 +0000
@@ -36,9 +36,13 @@
         """
         TextUIFactory.__init__(self)
         self.interval = interval
+        self.writer = writer
         self._progress_view = LoggingTextProgressView(
             time_source, writer, interval)
 
+    def show_user_warning(self, warning_id, **message_args):
+        self.writer(self.format_user_warning(warning_id, message_args))
+
 
 class LoggingTextProgressView(TextProgressView):
     """Support class for `LoggingUIFactory`. """

=== modified file 'lib/lp/codehosting/codeimport/worker.py'
--- lib/lp/codehosting/codeimport/worker.py	2011-06-21 05:38:15 +0000
+++ lib/lp/codehosting/codeimport/worker.py	2011-07-26 08:48:53 +0000
@@ -100,15 +100,6 @@
             if needs_tree:
                 local_branch.bzrdir.create_workingtree()
             return local_branch
-        # XXX Tim Penhey 2009-09-18 bug 432217 Automatic upgrade of import
-        # branches disabled.  Need an orderly upgrade process.
-        if False and remote_bzr_dir.needs_format_conversion(
-            format=required_format):
-            try:
-                remote_bzr_dir.root_transport.delete_tree('backup.bzr')
-            except NoSuchFile:
-                pass
-            upgrade(remote_url, required_format)
         # 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
@@ -121,6 +112,12 @@
         target_control.create_prefix()
         remote_bzr_dir.transport.copy_tree_to_transport(target_control)
         local_bzr_dir = BzrDir.open_from_transport(target)
+        if local_bzr_dir.needs_format_conversion(format=required_format):
+            try:
+                local_bzr_dir.root_transport.delete_tree('backup.bzr')
+            except NoSuchFile:
+                pass
+            upgrade(target_path, required_format, clean_up=True)
         if needs_tree:
             local_bzr_dir.create_workingtree()
         return local_bzr_dir.open_branch()