From ba0690c9d7a3bc283735d266e52f505418d63106 Mon Sep 17 00:00:00 2001 From: Brian Warner <warner@lothar.com> Date: Tue, 29 Dec 2009 15:37:46 -0800 Subject: [PATCH] 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. --- src/allmydata/interfaces.py | 4 ++++ src/allmydata/mutable/checker.py | 2 +- src/allmydata/mutable/repairer.py | 18 +++++++++++++--- src/allmydata/test/test_mutable.py | 33 ++++++++++++++++++++++++++++++ 4 files changed, 53 insertions(+), 4 deletions(-) diff --git a/src/allmydata/interfaces.py b/src/allmydata/interfaces.py index e8ff0b44..7928b433 100644 --- 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 ca2d121d..92ed05d8 100644 --- 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 b6693be8..5cef5e7d 100644 --- 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 0431d4fa..a79197af 100644 --- 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)) -- 2.45.2