launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/gina-unpack-cleanup into lp:launchpad.

Commit message:
Make gina remove partially-unpacked source packages even if it fails to process them.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1523591 in Launchpad itself: "gina fails to clean up after unpack failures"
  https://bugs.launchpad.net/launchpad/+bug/1523591

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/gina-unpack-cleanup/+merge/279827

Make gina remove partially-unpacked source packages even if it fails to process them.

Originally they were kept for forensics.  I don't think this is terribly useful, as developers don't have access to iron and failures of this kind will typically be reproducible anyway; it doesn't seem worth the risk of running the system out of disk space.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/gina-unpack-cleanup into lp:launchpad.

=== modified file 'lib/lp/soyuz/scripts/gina/packages.py'
--- lib/lp/soyuz/scripts/gina/packages.py	2015-09-28 17:38:45 +0000
+++ lib/lp/soyuz/scripts/gina/packages.py	2015-12-07 22:07:43 +0000
@@ -244,9 +244,7 @@
             self.do_package(distro_name, archive_root)
         finally:
             os.chdir(cwd)
-        # We only rmtree if everything worked as expected; otherwise,
-        # leave it around for forensics.
-        shutil.rmtree(tempdir)
+            shutil.rmtree(tempdir)
 
         self.date_uploaded = UTC_NOW
         return True

=== modified file 'lib/lp/soyuz/scripts/tests/test_gina.py'
--- lib/lp/soyuz/scripts/tests/test_gina.py	2015-05-20 11:31:11 +0000
+++ lib/lp/soyuz/scripts/tests/test_gina.py	2015-12-07 22:07:43 +0000
@@ -4,9 +4,11 @@
 from doctest import DocTestSuite
 import hashlib
 import os
+import tempfile
 from textwrap import dedent
 from unittest import TestLoader
 
+from fixtures import EnvironmentVariableFixture
 import transaction
 
 from lp.archiveuploader.tagfiles import parse_tagfile
@@ -226,6 +228,65 @@
         # But all is well in a Debian context.
         sp_data.do_package("debian", archive_root)
 
+    def test_process_package_cleans_up_after_unpack_failure(self):
+        archive_root = self.useTempDir()
+        pool_dir = os.path.join(archive_root, "pool/main/f/foo")
+        os.makedirs(pool_dir)
+
+        with open(os.path.join(
+            pool_dir, "foo_1.0.orig.tar.gz"), "wb+") as buffer:
+            orig_tar = LaunchpadWriteTarFile(buffer)
+            orig_tar.add_directory("foo-1.0")
+            orig_tar.close()
+            buffer.seek(0)
+            orig_tar_contents = buffer.read()
+        with open(os.path.join(
+            pool_dir, "foo_1.0-1.debian.tar.gz"), "wb+") as buffer:
+            debian_tar = LaunchpadWriteTarFile(buffer)
+            debian_tar.add_file("debian/source/format", "3.0 (quilt)\n")
+            debian_tar.add_file("debian/patches/series", "--- corrupt patch\n")
+            debian_tar.add_file("debian/rules", "")
+            debian_tar.close()
+            buffer.seek(0)
+            debian_tar_contents = buffer.read()
+        dsc_path = os.path.join(pool_dir, "foo_1.0-1.dsc")
+        with open(dsc_path, "w") as dsc:
+            dsc.write(dedent("""\
+                Format: 3.0 (quilt)
+                Source: foo
+                Binary: foo
+                Architecture: all
+                Version: 1.0-1
+                Maintainer: Foo Bar <foo.bar@xxxxxxxxxxxxx>
+                Files:
+                 %s %s foo_1.0.orig.tar.gz
+                 %s %s foo_1.0-1.debian.tar.gz
+                """ % (
+                    hashlib.md5(orig_tar_contents).hexdigest(),
+                    len(orig_tar_contents),
+                    hashlib.md5(debian_tar_contents).hexdigest(),
+                    len(debian_tar_contents))))
+
+        dsc_contents = parse_tagfile(dsc_path)
+        dsc_contents["Directory"] = pool_dir
+        dsc_contents["Package"] = "foo"
+        dsc_contents["Component"] = "main"
+        dsc_contents["Section"] = "misc"
+
+        sp_data = SourcePackageData(**dsc_contents)
+        unpack_tmpdir = self.makeTemporaryDirectory()
+        with EnvironmentVariableFixture("TMPDIR", unpack_tmpdir):
+            # Force tempfile to recheck TMPDIR.
+            tempfile.tempdir = None
+            try:
+                self.assertRaises(
+                    ExecutionError,
+                    sp_data.process_package, "ubuntu", archive_root)
+            finally:
+                # Force tempfile to recheck TMPDIR for future tests.
+                tempfile.tempdir = None
+        self.assertEqual([], os.listdir(unpack_tmpdir))
+
 
 class TestSourcePackageHandler(TestCaseWithFactory):