duplicity-team team mailing list archive
-
duplicity-team team
-
Mailing list archive
-
Message #01304
[Merge] lp:~mterry/duplicity/ropath.index into lp:duplicity
Michael Terry has proposed merging lp:~mterry/duplicity/ropath.index into lp:duplicity.
Requested reviews:
duplicity-team (duplicity-team)
Related bugs:
Bug #929067 in Duplicity: "Crash during restore: "assert ropath.index == (), ropath.index""
https://bugs.launchpad.net/duplicity/+bug/929067
For more details, see:
https://code.launchpad.net/~mterry/duplicity/ropath.index/+merge/127007
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.
--
https://code.launchpad.net/~mterry/duplicity/ropath.index/+merge/127007
Your team duplicity-team is requested to review the proposed merge of lp:~mterry/duplicity/ropath.index into lp:duplicity.
=== 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()
Follow ups
