Merge lp:~mterry/duplicity/ropath.index into lp:duplicity/0.6

[Michael Terry]

This branch does two main things:

1) Skips base dir entries when compiling the list of deleted delta iters. (this gracefully recovers from the sort of situations that lead to bug 929067). I'm reasonably confident this is an uninvasive change, but please confirm.

2) Overwrites the sigtar file on backup-restart. This is because AFAICT, duplicity will rewrite the entire sigtar each restart. But we were opening the sigtar file as "ab", so we'd just dump the contents on top of the previous contents. Which was causing some confusion in bug 929067. If I'm wrong that we don't always rewrite the entire sigtar each time, this needs some rethink. Please also confirm that. :)

In addition, I add two tests for the above two changes and make some improvements elsewhere in the restarttest.py file while I was at it.

=== modified file 'bin/duplicity'
--- bin/duplicity	2012-09-13 14:08:52 +0000
+++ bin/duplicity	2012-09-28 15:53:25 +0000
@@ -472,7 +472,8 @@
     fh = dup_temp.get_fileobj_duppath(globals.archive_dir,
                                       part_sig_filename,
                                       perm_sig_filename,
-                                      remote_sig_filename)
+                                      remote_sig_filename,
+                                      overwrite=True)
     return fh
 
 

=== modified file 'duplicity/diffdir.py'
--- duplicity/diffdir.py	2011-10-05 14:27:04 +0000
+++ duplicity/diffdir.py	2012-09-28 15:53:25 +0000
@@ -189,8 +189,10 @@
         log.Debug(_("Comparing %s and %s") % (new_path and new_path.index,
                                               sig_path and sig_path.index))
         if not new_path or not new_path.type:
-            # file doesn't exist
-            if sig_path and sig_path.exists():
+            # File doesn't exist (but ignore attempts to delete base dir;
+            # old versions of duplicity could have written out the sigtar in
+            # such a way as to fool us; LP: #929067)
+            if sig_path and sig_path.exists() and sig_path.index != ():
                 # but signature says it did
                 log.Info(_("D %s") %
                          (sig_path.get_relative_path(),),

=== modified file 'duplicity/dup_temp.py'
--- duplicity/dup_temp.py	2011-10-08 15:55:26 +0000
+++ duplicity/dup_temp.py	2012-09-28 15:53:25 +0000
@@ -60,7 +60,7 @@
         return fh
 
 
-def get_fileobj_duppath(dirpath, partname, permname, remname):
+def get_fileobj_duppath(dirpath, partname, permname, remname, overwrite=False):
     """
     Return a file object open for writing, will write to filename
 
@@ -76,7 +76,10 @@
                            partname = partname, permname = permname, remname = remname)
     else:
         dp = path.DupPath(dirpath.name, index = (partname,))
-        fh = FileobjHooked(dp.filtered_open("ab"), tdp = None, dirpath = dirpath,
+        mode = "ab"
+        if overwrite:
+            mode = "wb"
+        fh = FileobjHooked(dp.filtered_open(mode), tdp = None, dirpath = dirpath,
                            partname = partname, permname = permname, remname = remname)
 
     def rename_and_forget():

=== modified file 'testing/tests/restarttest.py'
--- testing/tests/restarttest.py	2011-12-18 12:49:41 +0000
+++ testing/tests/restarttest.py	2012-09-28 15:53:25 +0000
@@ -21,6 +21,8 @@
 
 import helper
 import sys, os, unittest
+import glob
+import subprocess
 
 import duplicity.backend
 from duplicity import path
@@ -41,7 +43,9 @@
 
 class CmdError(Exception):
     """Indicates an error running an external command"""
-    pass
+    def __init__(self, code):
+        Exception.__init__(self, code)
+        self.exit_status = code
 
 class RestartTest(unittest.TestCase):
     """
@@ -49,6 +53,14 @@
     """
     def setUp(self):
         assert not os.system("tar xzf testfiles.tar.gz > /dev/null 2>&1")
+        assert not os.system("rm -rf testfiles/output "
+                             "testfiles/restore_out testfiles/cache")
+        assert not os.system("mkdir testfiles/output testfiles/cache")
+        backend = duplicity.backend.get_backend(backend_url)
+        bl = backend.list()
+        if bl:
+            backend.delete(backend.list())
+        backend.close()
 
     def tearDown(self):
         assert not os.system("rm -rf testfiles tempdir temp2.tar")
@@ -72,7 +84,7 @@
 #        print "CMD: %s" % cmdline
         return_val = os.system(cmdline)
         if return_val:
-            raise CmdError(return_val)
+            raise CmdError(os.WEXITSTATUS(return_val))
 
     def backup(self, type, input_dir, options = [], current_time = None):
         """Run duplicity backup to default directory"""
@@ -103,25 +115,11 @@
             options.extend(['--restore-time', str(time)])
         self.run_duplicity(args, options, current_time)
 
-    def deltmp(self):
-        """
-        Delete temporary directories
-        """
-        assert not os.system("rm -rf testfiles/output "
-                             "testfiles/restore_out testfiles/cache")
-        assert not os.system("mkdir testfiles/output testfiles/cache")
-        backend = duplicity.backend.get_backend(backend_url)
-        bl = backend.list()
-        if bl:
-            backend.delete(backend.list())
-        backend.close()
-
     def runtest(self, dirlist, backup_options = [], restore_options = []):
         """
         Run backup/restore test on directories in dirlist
         """
         assert len(dirlist) >= 1
-        self.deltmp()
 
         # Back up directories to local backend
         current_time = 100000
@@ -142,6 +140,12 @@
                 self.verify(dirname,
                             time = current_time, options = restore_options)
 
+    def make_largefiles(self):
+        # create 3 2M files
+        assert not os.system("mkdir testfiles/largefiles")
+        for n in (1,2,3):
+            assert not os.system("dd if=/dev/urandom of=testfiles/largefiles/file%d bs=1024 count=2048 > /dev/null 2>&1" % n)
+
     def check_same(self, filename1, filename2):
         """
         Verify two filenames are the same
@@ -153,14 +157,14 @@
         """
         Test basic Checkpoint/Restart
         """
-        excludes = ["--exclude **/output",
-                    "--exclude **/cache",]
-        self.deltmp()
+        excludes = ["--exclude '**/output'",
+                    "--exclude '**/cache'",]
         # we know we're going to fail this one, its forced
         try:
             self.backup("full", "testfiles", options = ["--vol 1", "--fail 1"] + excludes)
-        except CmdError:
-            pass
+            self.fail()
+        except CmdError, e:
+            self.assertEqual(30, e.exit_status)
         # this one should pass OK
         self.backup("full", "testfiles", options = excludes)
         self.verify("testfiles", options = excludes)
@@ -169,39 +173,42 @@
         """
         Test multiple Checkpoint/Restart
         """
-        excludes = ["--exclude **/output",
-                    "--exclude **/cache",]
-        self.deltmp()
+        excludes = ["--exclude '**/output'",
+                    "--exclude '**/cache'",]
+        self.make_largefiles()
         # we know we're going to fail these, they are forced
         try:
-            self.backup("full", "testfiles", options = ["--vol 1", "--fail 1"] + excludes)
-        except CmdError:
-            pass
-        try:
-            self.backup("full", "testfiles", options = ["--vol 1", "--fail 2"] + excludes)
-        except CmdError:
-            pass
-        try:
-            self.backup("full", "testfiles", options = ["--vol 1", "--fail 3"] + excludes)
-        except CmdError:
-            pass
+            self.backup("full", "testfiles/largefiles", options = ["--vol 1", "--fail 1"] + excludes)
+            self.fail()
+        except CmdError, e:
+            self.assertEqual(30, e.exit_status)
+        try:
+            self.backup("full", "testfiles/largefiles", options = ["--vol 1", "--fail 2"] + excludes)
+            self.fail()
+        except CmdError, e:
+            self.assertEqual(30, e.exit_status)
+        try:
+            self.backup("full", "testfiles/largefiles", options = ["--vol 1", "--fail 3"] + excludes)
+            self.fail()
+        except CmdError, e:
+            self.assertEqual(30, e.exit_status)
         # this one should pass OK
-        self.backup("full", "testfiles", options = excludes)
-        self.verify("testfiles", options = excludes)
+        self.backup("full", "testfiles/largefiles", options = excludes)
+        self.verify("testfiles/largefiles", options = excludes)
 
     def test_first_volume_failure(self):
         """
         Test restart when no volumes are available on the remote.
         Caused when duplicity fails before the first transfer.
         """
-        excludes = ["--exclude **/output",
-                    "--exclude **/cache",]
-        self.deltmp()
+        excludes = ["--exclude '**/output'",
+                    "--exclude '**/cache'",]
         # we know we're going to fail these, they are forced
         try:
             self.backup("full", "testfiles", options = ["--vol 1", "--fail 1"] + excludes)
-        except CmdError:
-            pass
+            self.fail()
+        except CmdError, e:
+            self.assertEqual(30, e.exit_status)
         assert not os.system("rm testfiles/output/duplicity-full*difftar*")
         # this one should pass OK
         self.backup("full", "testfiles", options = excludes)
@@ -213,12 +220,12 @@
         than the local manifest has on record.  Caused when duplicity
         fails the last queued transfer(s).
         """
-        self.deltmp()
         # we know we're going to fail these, they are forced
         try:
             self.backup("full", "/bin", options = ["--vol 1", "--fail 3"])
-        except CmdError:
-            pass
+            self.fail()
+        except CmdError, e:
+            self.assertEqual(30, e.exit_status)
         assert not os.system("rm testfiles/output/duplicity-full*vol[23].difftar*")
         # this one should pass OK
         self.backup("full", "/bin", options = ["--vol 1"])
@@ -230,16 +237,13 @@
         Caused when the user deletes a file after a failure.  This test puts
         the file in the middle of the backup, with files following.
         """
-        self.deltmp()
-        # create 3 2M files
-        assert not os.system("mkdir testfiles/largefiles")
-        for n in (1,2,3):
-            assert not os.system("dd if=/dev/urandom of=testfiles/largefiles/file%d bs=1024 count=2048 > /dev/null 2>&1" % n)
+        self.make_largefiles()
         # we know we're going to fail, it's forced
         try:
             self.backup("full", "testfiles/largefiles", options = ["--vol 1", "--fail 3"])
-        except CmdError:
-            pass
+            self.fail()
+        except CmdError, e:
+            self.assertEqual(30, e.exit_status)
         assert not os.system("rm testfiles/largefiles/file2")
         # this one should pass OK
         self.backup("full", "testfiles/largefiles", options = ["--vol 1"])
@@ -253,16 +257,13 @@
         Caused when the user deletes a file after a failure.  This test puts
         the file at the end of the backup, with no files following.
         """
-        self.deltmp()
-        # create 3 2M files
-        assert not os.system("mkdir testfiles/largefiles")
-        for n in (1,2,3):
-            assert not os.system("dd if=/dev/urandom of=testfiles/largefiles/file%d bs=1024 count=2048 > /dev/null 2>&1" % n)
+        self.make_largefiles()
         # we know we're going to fail, it's forced
         try:
             self.backup("full", "testfiles/largefiles", options = ["--vol 1", "--fail 6"])
-        except CmdError:
-            pass
+            self.fail()
+        except CmdError, e:
+            self.assertEqual(30, e.exit_status)
         assert not os.system("rm testfiles/largefiles/file3")
         # this one should pass OK
         self.backup("full", "testfiles/largefiles", options = ["--vol 1"])
@@ -276,19 +277,73 @@
         """
         # Make first normal full backup
         self.backup("full", "testfiles/dir1")
-        # create 3 2M files
-        assert not os.system("mkdir testfiles/largefiles")
-        for n in (1,2,3):
-            assert not os.system("dd if=/dev/urandom of=testfiles/largefiles/file%d bs=1024 count=2048 > /dev/null 2>&1" % n)
+        self.make_largefiles()
         # Force a failure partway through
         try:
             self.backup("inc", "testfiles/largefiles", options = ["--vols 1", "--fail 2"])
-            assert False # shouldn't get this far
+            self.fail()
         except CmdError, e:
-            pass
+            self.assertEqual(30, e.exit_status)
         # Now finish that incremental
         self.backup("inc", "testfiles/largefiles")
         self.verify("testfiles/largefiles")
 
+    def test_no_write_double_snapshot(self):
+        """
+        Test that restarting a full backup does not write duplicate entries
+        into the sigtar, causing problems reading it back in older
+        versions.
+        https://launchpad.net/bugs/929067
+        """
+        self.make_largefiles()
+        # Start backup
+        try:
+            self.backup("full", "testfiles/largefiles", options = ["--fail 2", "--vols 1", "--no-encryption"])
+            self.fail()
+        except CmdError, e:
+            self.assertEqual(30, e.exit_status)
+        # Finish it
+        self.backup("full", "testfiles/largefiles", options = ["--no-encryption"])
+        # Now check sigtar
+        sigtars = glob.glob("testfiles/output/duplicity-full*.sigtar.gz")
+        self.assertEqual(1, len(sigtars))
+        sigtar = sigtars[0]
+        output = subprocess.check_output(["tar", "t", "--file=%s" % sigtar])
+        self.assertEqual(1, output.split("\n").count("snapshot/"))
+
+    def xtest_ignore_double_snapshot(self):
+        """
+        Test that we gracefully ignore double snapshot entries in a signature
+        file.  This winds its way through duplicity as a deleted base dir,
+        which doesn't make sense and should be ignored.  An older version of
+        duplicity accidentally created such files as a result of a restart.
+        https://launchpad.net/bugs/929067
+        """
+        # Intial normal backup
+        self.backup("full", "testfiles/blocktartest", options = ["--no-encryption"])
+        # Create an exact clone of the snapshot folder in the sigtar already.
+        # Permissions and mtime must match.
+        os.mkdir("testfiles/snapshot", 0755)
+        os.utime("testfiles/snapshot", (1030384548, 1030384548))
+        # Adjust the sigtar.gz file to have a bogus second snapshot/ entry
+        # at the beginning.
+        sigtars = glob.glob("testfiles/output/duplicity-full*.sigtar.gz")
+        self.assertEqual(1, len(sigtars))
+        sigtar = sigtars[0]
+        self.assertEqual(0, os.system("tar c --file=testfiles/snapshot.sigtar -C testfiles snapshot"))
+        self.assertEqual(0, os.system("gunzip -c %s > testfiles/full.sigtar" % sigtar))
+        self.assertEqual(0, os.system("tar A --file=testfiles/snapshot.sigtar testfiles/full.sigtar"))
+        self.assertEqual(0, os.system("gzip testfiles/snapshot.sigtar"))
+        os.remove(sigtar)
+        os.rename("testfiles/snapshot.sigtar.gz", sigtar)
+        # Clear cache so our adjusted sigtar will be sync'd back into the cache
+        self.assertEqual(0, os.system("rm -r testfiles/cache"))
+        # Try a follow on incremental (which in buggy versions, would create
+        # a deleted entry for the base dir)
+        self.backup("inc", "testfiles/blocktartest")
+        self.assertEqual(1, len(glob.glob("testfiles/output/duplicity-new*.sigtar.gz")))
+        # Confirm we can restore it (which in buggy versions, would fail)
+        self.restore()
+
 if __name__ == "__main__":
     unittest.main()