launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-819674 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #819674 in Launchpad itself: "Handle ctrl-C in publish-ftpmaster"
  https://bugs.launchpad.net/launchpad/+bug/819674

For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-819674/+merge/70135

= Summary =

If you hit ctrl-C while running publish-ftpmaster, the sensible thing happens: the only parts of the filesystem that are affected are the archive pool (which may get some extra files but won't reference them yet), and the working dists directory (which won't become the current dists directory until success).

This wasn't fully tested, so here goes.

I tried to do this the proper TDD way, and then found that the only real code change I needed was to make this behaviour testable.  Then I discovered that the "except:" catches not just Exceptions but KeyboardInterrupts as well (the latter is no longer derived from the former in recent python versions).  But I believe the tests are still useful.

I used FakeMethods in two of their four capacities here:

1. To force a method into raising a specific failure.

2. To keep track of calls to a method.

(The other two, which I did not use here, are to turn a modifying method into a no-op and to force a querying method into returning a specific value).

The actual work done by recoverWorkingDists (including its integration with the newly extracted recoverArchiveWorkingDirectory) is already covered by existing tests.  The new tests cover new aspects of how publish and recoverWorkingDists fit together.


== Tests ==

{{{
./bin/test -vvc lp.archivepublisher.tests.test_publish_ftpmaster
}}}


== Demo and Q/A ==

Ongoing: make sure the new python-based publish-ftpmaster script works well enough to replace the old shell script in production.


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/archivepublisher/scripts/publish_ftpmaster.py
  lib/lp/archivepublisher/tests/test_publish_ftpmaster.py
-- 
https://code.launchpad.net/~jtv/launchpad/bug-819674/+merge/70135
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-819674 into lp:launchpad.

=== modified file 'lib/lp/archivepublisher/scripts/publish_ftpmaster.py'
--- lib/lp/archivepublisher/scripts/publish_ftpmaster.py	2011-07-27 10:54:53 +0000
+++ lib/lp/archivepublisher/scripts/publish_ftpmaster.py	2011-08-02 11:02:34 +0000
@@ -321,6 +321,19 @@
                 failure=LaunchpadScriptFailure(
                     "Failed to rsync new dists for %s." % purpose.title))
 
+    def recoverArchiveWorkingDir(self, archive_config):
+        """Recover working dists dir for `archive_config`.
+
+        If there is a dists directory for `archive_config` in the working
+        location, kick it back to the backup location.
+        """
+        working_location = get_working_dists(archive_config)
+        if file_exists(working_location):
+            self.logger.info(
+                "Recovering working directory %s from failed run.",
+                working_location)
+            os.rename(working_location, get_backup_dists(archive_config))
+
     def recoverWorkingDists(self):
         """Look for and recover any dists left in transient working state.
 
@@ -330,12 +343,7 @@
         permanent location.
         """
         for archive_config in self.configs.itervalues():
-            working_location = get_working_dists(archive_config)
-            if file_exists(working_location):
-                self.logger.info(
-                    "Recovering working directory %s from failed run.",
-                    working_location)
-                os.rename(working_location, get_backup_dists(archive_config))
+            self.recoverArchiveWorkingDir(archive_config)
 
     def setUpDirs(self):
         """Create archive roots and such if they did not yet exist."""

=== modified file 'lib/lp/archivepublisher/tests/test_publish_ftpmaster.py'
--- lib/lp/archivepublisher/tests/test_publish_ftpmaster.py	2011-07-22 15:35:08 +0000
+++ lib/lp/archivepublisher/tests/test_publish_ftpmaster.py	2011-08-02 11:02:34 +0000
@@ -736,6 +736,75 @@
                 "Did not find expected marker for %s."
                 % archive.purpose.title)
 
+    def test_publish_reraises_exception(self):
+        # If an Exception comes up while publishing, it bubbles up out
+        # of the publish method even though the method must intercept
+        # it for its own purposes.
+        class MoonPhaseError(Exception):
+            """Simulated failure."""
+
+        failure = MoonPhaseError(self.factory.getUniqueString())
+        script = self.makeScript()
+        script.publishAllUploads = FakeMethod(failure=failure)
+        script.setUp()
+        try:
+            script.publish()
+        except MoonPhaseError, e:
+            self.assertEqual(failure, e)
+        else:
+            self.assertTrue(False, "Exception wasn't re-raised.")
+
+    def test_publish_obeys_keyboard_interrupt(self):
+        # Similar to an Exception, a keyboard interrupt does not get
+        # swallowed.
+        failure = KeyboardInterrupt(self.factory.getUniqueString())
+        script = self.makeScript()
+        script.publishAllUploads = FakeMethod(failure=failure)
+        script.setUp()
+        try:
+            script.publish()
+        except KeyboardInterrupt, e:
+            self.assertEqual(failure, e)
+        else:
+            self.assertTrue(False, "KeyboardInterrupt wasn't re-raised.")
+
+    def test_publish_recovers_working_dists_on_exception(self):
+        # If an Exception comes up while publishing, the publish method
+        # recovers its working directory.
+        class MoonPhaseError(Exception):
+            """Simulated failure."""
+
+        failure = MoonPhaseError(self.factory.getUniqueString())
+
+        script = self.makeScript()
+        script.publishAllUploads = FakeMethod(failure=failure)
+        script.recoverArchiveWorkingDir = FakeMethod()
+        script.setUp()
+
+        try:
+            script.publish()
+        except MoonPhaseError:
+            pass
+
+        self.assertEqual(1, script.recoverArchiveWorkingDir.call_count)
+
+    def test_publish_recovers_working_dists_on_ctrl_C(self):
+        # If the user hits ctrl-C while publishing, the publish method
+        # recovers its working directory.
+        failure = KeyboardInterrupt("Ctrl-C!")
+
+        script = self.makeScript()
+        script.publishAllUploads = FakeMethod(failure=failure)
+        script.recoverArchiveWorkingDir = FakeMethod()
+        script.setUp()
+
+        try:
+            script.publish()
+        except KeyboardInterrupt:
+            pass
+
+        self.assertEqual(1, script.recoverArchiveWorkingDir.call_count)
+
 
 class TestCreateDistroSeriesIndexes(TestCaseWithFactory, HelpersMixin):
     """Test initial creation of archive indexes for a `DistroSeries`."""