[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Fwd: [rdiff-backup-users] Resuming initial backups]

From: Andrew Ferguson
Subject: Re: [Fwd: [rdiff-backup-users] Resuming initial backups]
Date: Sun, 5 Oct 2008 13:31:51 -0400

On Oct 3, 2008, at 12:36 PM, Josh Nisly wrote:
After over a month, I finally have a patch to implement this. It detects a failed initial backup by the following checks:
* No current_mirror file
* 0 or 1 error_log files
* 0 or 1 mirror_metadata files
* No files in the increments directory

If all of the above are true, then it removes all contents of the rdiff-backup directory, allowing backup resumption. I believe that this guarantees that there will be no information lost, since there are no historical increments being lost.

Andrew, what do you think of the patch? (Especially the delete_dir_no_files function.)

I think that criteria makes sense for establishing that the initial backup failed. I have other comments about the patch itself below.

--- rdiff_backup/Security.py    20 Aug 2008 17:43:25 -0000      1.36
+++ rdiff_backup/Security.py    3 Oct 2008 16:14:15 -0000
@@ -45,7 +45,8 @@
                                 'os.chown':0, 'os.remove':0, 'os.removedirs':0,
                                 'os.rename':0, 'os.renames':0, 'os.rmdir':0, 
                                 'os.utime':0, 'os.lchown':0, 'os.link':1, 
-                                'os.mkdir':0, 'os.makedirs':0}
+                                'os.mkdir':0, 'os.makedirs':0,
+                                'rpath.delete_dir_no_files':0}
def initialize(action, cmdpairs):
        """Initialize allowable request list and chroot"""
@@ -180,6 +181,7 @@
        if sec_level == "all":
                l.extend(["os.mkdir", "os.chown", "os.lchown", "os.rename",
                                  "os.unlink", "os.remove", "os.chmod", 
+                                 "rpath.delete_dir_no_files",
--- rdiff_backup/rpath.py       22 Jul 2008 17:46:59 -0000      1.126
+++ rdiff_backup/rpath.py       3 Oct 2008 16:14:07 -0000
@@ -358,6 +358,14 @@
        else: basestr = ".".join(dotsplit[:-2])
        return (compressed, timestring, ext, basestr)

+def delete_dir_no_files(rp):
+       """Deletes the directory at self.path if empty. Raises if the
+       directory contains files."""
+       log.Log("Determining if directory contains files: %s" % rp.path, 7)
+       if rp.contains_files():
+               raise RPathException("Directory contains files.")
+       rp.delete()

Perhaps verify that rp is a directory? (either with assert or a simple return if not true)

class RORPath:
        """Read Only RPath - carry information about a path
@@ -1047,6 +1055,18 @@
                else: self.conn.os.unlink(self.path)

+       def contains_files(self):
+ log.Log("Determining if directory contains files: %s" % self.path, 7)
+               dir_entries = self.listdir()
+               for entry in dir_entries:
+                       child_rp = self.append_path(entry)
+                       if not child_rp.isdir():
+                               return False
+                       else:
+                               if not child_rp.contains_files():
+                                       return False
+               return True

I think there are some issues here:

1) "child_rp = self.append_path(entry)" should be "child_rp = self.append(entry)"

2) I think there should be a check that self is a directory before the call to self.listdir() ... otherwise, if called on a file, OSError is thrown. We want to guard against potential future mistakes.

3) What should contains_files() return? Based on the name, my instinct says that it should return True if the directory contains files. It looks to me like it returns False if it contains files and True if empty....

4) There's a logic problem here: let's say the directory self contains a file called 'bar' and a directory called 'foo', which contains files.The first time through "for entry in dir_entries" loop, we check 'if not child_rp.isdir()' on 'bar' and return False, without testing 'foo' at all ... is this something you wanted to do? Maybe I'm confused on what you intend.

Note: that problem is even worse than I made it out because os.listdir() returns files in arbitrary order. See iterate_in_dir() and listdir() functions in selection.py to see how rdiff-backup handles recursing through directories.

5) Lastly, why is the function recursing? If it contains a directory (even an empty one), then it technically contains a file -- no recursion necessary.

        def quote(self):
                """Return quoted self.path for use with os.system()"""
                return '"%s"' % self.regex_chars_to_quote.sub(
--- rdiff_backup/Main.py        20 Aug 2008 17:43:25 -0000      1.119
+++ rdiff_backup/Main.py        3 Oct 2008 14:34:11 -0000
@@ -378,6 +378,46 @@
                Log.FatalError("Source %s is not a directory" % rpin.path)
        Globals.rbdir = rpout.append_path("rdiff-backup-data")

+def fix_failed_initial_backup(rpout):
+       if Globals.rbdir.lstat():
+               rbdir_files = Globals.rbdir.listdir()
+               mirror_markers = filter(lambda x: 
+                                                               rbdir_files)
+               error_logs = filter(lambda x: x.startswith("error_log"),
+                                                       rbdir_files)
+ metadata_mirrors = filter(lambda x: x.startswith("mirror_metadata"),
+                                                               rbdir_files)
+               # If we have no current_mirror marker, and the increments 
+ # is empty, we most likely have a failed backup. At any rate, if the + # increments directory is empty, we can't lose any historical diffs,
+               # because there aren't any.
+               if not mirror_markers and len(error_logs) <= 1 and \
+                               len(metadata_mirrors) <= 1:
+                       Log("Found interrupted initial backup. Removing...",  2)
+                       # Try to delete the increments dir first
+                       if 'increments' in rbdir_files:
+                               rbdir_files.remove('increments') # Only try to 
delete once
+                               rp = Globals.rbdir.append('increments')
+                               try:
+                                       rp.conn.rpath.delete_dir_no_files(rp)
+                               except rpath.RPathException:
+                                       Log("Increments dir contains files.", 4)
+                                       return
+                               except Security.Violation:
+                                       Log("Server doesn't support resuming.", 
+                                       return
+                       for file_name in rbdir_files:
+                               rp = Globals.rbdir.append_path(file_name)
+                               if rp.isdir():
+                                       try:
+                                       except rpath.RPathException:
+                                               Log("Directory contains 
files.", 4)
+                                               return
+                               else:
+                                       rp.delete()

In the rdiff-backup-data directory, I believe the only directories which could potentially exist besides increments/ are the temporary directories created by the FS abilities tests -- those would be safe to delete without stopping with the "Directory contains files" message. Oh, there's also the long_filename_data/ directory ... not sure how you want to deal with that. I suppose we should keep all of the entries in there.

def backup_set_rbdir(rpin, rpout):
        """Initialize data dir and logging"""
        global incdir
@@ -389,6 +429,8 @@
rdiff-backup like this could mess up what is currently in it.  If you
want to update or overwrite it, run rdiff-backup with the --force
option.""" % rpout.path)
+       else: # Check for interrupted initial backup
+               fix_failed_initial_backup(rpout)

        if not Globals.rbdir.lstat(): Globals.rbdir.mkdir()
        SetConnections.UpdateGlobal('rbdir', Globals.rbdir)

I think it might be cleaner to separate this code into two steps:

elif check_failed_initial_backup(rpout):

so that we can work on the test and the fix separately.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]