mutable repair: return successful=False when numshares<k (thus repair fails),

instead of weird errors. Closes #874 and #786.

Previously, if the file had 0 shares, this would raise TypeError as it tried
to call download_version(None). If the file had some shares but fewer than
'k', it would incorrectly raise MustForceRepairError.

Added get_successful() to the IRepairResults API, to give repair() a place to
report non-code-bug problems like this.

diff --git a/src/allmydata/interfaces.py b/src/allmydata/interfaces.py
index e8ff0b44106f896aef5dc336f5ff6c6e0b7a5e84..7928b43344ccad5f5f397eee92ea837a63e4f0ed 100644 (file)
--- a/src/allmydata/interfaces.py
+++ b/src/allmydata/interfaces.py
@@ -2002,6 +2002,10 @@ class IRepairable(Interface):
 
 class IRepairResults(Interface):
     """I contain the results of a repair operation."""
+    def get_successful(self):
+        """Returns a boolean: True if the repair made the file healthy, False
+        if not. Repair failure generally indicates a file that has been
+        damaged beyond repair."""
 
 
 class IClient(Interface):

diff --git a/src/allmydata/mutable/checker.py b/src/allmydata/mutable/checker.py
index ca2d121d8f330997fccc1cd826ff90aae8147c0c..92ed05d8c440023d3dcf3eeab52e9834718ac647 100644 (file)
--- a/src/allmydata/mutable/checker.py
+++ b/src/allmydata/mutable/checker.py
@@ -306,7 +306,7 @@ class MutableCheckAndRepairer(MutableChecker):
         self.cr_results.repair_attempted = True
         d = self._node.repair(self.results)
         def _repair_finished(repair_results):
-            self.cr_results.repair_successful = True
+            self.cr_results.repair_successful = repair_results.get_successful()
             r = CheckResults(from_string(self._node.get_uri()), self._storage_index)
             self.cr_results.post_repair_results = r
             self._fill_checker_results(repair_results.servermap, r)

diff --git a/src/allmydata/mutable/repairer.py b/src/allmydata/mutable/repairer.py
index b6693be8f4c9336bed3178b0313bc2a90d038bf8..5cef5e7dc961cbb102af993408f1860d2950e4dc 100644 (file)
--- a/src/allmydata/mutable/repairer.py
+++ b/src/allmydata/mutable/repairer.py
@@ -1,5 +1,6 @@
 
 from zope.interface import implements
+from twisted.internet import defer
 from allmydata.interfaces import IRepairResults, ICheckResults
 
 class RepairResults:
@@ -7,7 +8,10 @@ class RepairResults:
 
     def __init__(self, smap):
         self.servermap = smap
-
+    def set_successful(self, successful):
+        self.successful = successful
+    def get_successful(self):
+        return self.successful
     def to_string(self):
         return ""
 
@@ -52,6 +56,13 @@ class Repairer:
 
         smap = self.check_results.get_servermap()
 
+        best_version = smap.best_recoverable_version()
+        if not best_version:
+            # the file is damaged beyond repair
+            rr = RepairResults(smap)
+            rr.set_successful(False)
+            return defer.succeed(rr)
+
         if smap.unrecoverable_newer_versions():
             if not force:
                 raise MustForceRepairError("There were unrecoverable newer "
@@ -92,11 +103,12 @@ class Repairer:
         if not self.node.get_writekey():
             raise RepairRequiresWritecapError("Sorry, repair currently requires a writecap, to set the write-enabler properly.")
 
-        best_version = smap.best_recoverable_version()
         d = self.node.download_version(smap, best_version, fetch_privkey=True)
         d.addCallback(self.node.upload, smap)
         d.addCallback(self.get_results, smap)
         return d
 
     def get_results(self, res, smap):
-        return RepairResults(smap)
+        rr = RepairResults(smap)
+        rr.set_successful(True)
+        return rr

diff --git a/src/allmydata/test/test_mutable.py b/src/allmydata/test/test_mutable.py
index 0431d4fa43d6f5de9f6a555f58b6d646068d3a81..a79197afdf10b289110cc06281a40a62d9ed5bb3 100644 (file)
--- a/src/allmydata/test/test_mutable.py
+++ b/src/allmydata/test/test_mutable.py
@@ -1296,6 +1296,7 @@ class Repair(unittest.TestCase, PublishMixin, ShouldFailMixin):
         d.addCallback(lambda check_results: self._fn.repair(check_results))
         def _check_results(rres):
             self.failUnless(IRepairResults.providedBy(rres))
+            self.failUnless(rres.get_successful())
             # TODO: examine results
 
             self.copy_shares()
@@ -1338,6 +1339,36 @@ class Repair(unittest.TestCase, PublishMixin, ShouldFailMixin):
         current_shares = self.old_shares[-1]
         self.failUnlessEqual(old_shares, current_shares)
 
+    def test_unrepairable_0shares(self):
+        d = self.publish_one()
+        def _delete_all_shares(ign):
+            shares = self._storage._peers
+            for peerid in shares:
+                shares[peerid] = {}
+        d.addCallback(_delete_all_shares)
+        d.addCallback(lambda ign: self._fn.check(Monitor()))
+        d.addCallback(lambda check_results: self._fn.repair(check_results))
+        def _check(crr):
+            self.failUnlessEqual(crr.get_successful(), False)
+        d.addCallback(_check)
+        return d
+
+    def test_unrepairable_1share(self):
+        d = self.publish_one()
+        def _delete_all_shares(ign):
+            shares = self._storage._peers
+            for peerid in shares:
+                for shnum in list(shares[peerid]):
+                    if shnum > 0:
+                        del shares[peerid][shnum]
+        d.addCallback(_delete_all_shares)
+        d.addCallback(lambda ign: self._fn.check(Monitor()))
+        d.addCallback(lambda check_results: self._fn.repair(check_results))
+        def _check(crr):
+            self.failUnlessEqual(crr.get_successful(), False)
+        d.addCallback(_check)
+        return d
+
     def test_merge(self):
         self.old_shares = []
         d = self.publish_multiple()
@@ -1361,6 +1392,7 @@ class Repair(unittest.TestCase, PublishMixin, ShouldFailMixin):
                       self._fn.repair(check_results, force=True))
         # this should give us 10 shares of the highest roothash
         def _check_repair_results(rres):
+            self.failUnless(rres.get_successful())
             pass # TODO
         d.addCallback(_check_repair_results)
         d.addCallback(lambda res: self._fn.get_servermap(MODE_CHECK))
@@ -1395,6 +1427,7 @@ class Repair(unittest.TestCase, PublishMixin, ShouldFailMixin):
         d.addCallback(lambda check_results: self._fn.repair(check_results))
         # this should give us 10 shares of v3
         def _check_repair_results(rres):
+            self.failUnless(rres.get_successful())
             pass # TODO
         d.addCallback(_check_repair_results)
         d.addCallback(lambda res: self._fn.get_servermap(MODE_CHECK))