From 0fcc054a613ac4036a38221cc58ddf843eeb810a Mon Sep 17 00:00:00 2001 From: Brian Warner Date: Fri, 25 May 2012 00:14:46 -0700 Subject: [PATCH] CheckResults: use fat init, add type-checking assertions Added assertions for sharemap, servermap, servers_responding, list_corrupt_shares, and list_incompatible_shares. --- src/allmydata/check_results.py | 57 ++++++++++---------- src/allmydata/immutable/checker.py | 56 ++++++++++---------- src/allmydata/immutable/filenode.py | 41 +++++++-------- src/allmydata/mutable/checker.py | 54 +++++++++---------- src/allmydata/test/common.py | 80 ++++++++++++++--------------- src/allmydata/test/test_checker.py | 72 +++++++++++++++----------- src/allmydata/test/test_mutable.py | 4 +- 7 files changed, 192 insertions(+), 172 deletions(-) diff --git a/src/allmydata/check_results.py b/src/allmydata/check_results.py index cd21d0cc..9fc7a863 100644 --- a/src/allmydata/check_results.py +++ b/src/allmydata/check_results.py @@ -7,36 +7,41 @@ from allmydata.util import base32 class CheckResults: implements(ICheckResults) - def __init__(self, uri, storage_index): + def __init__(self, uri, storage_index, + healthy, recoverable, needs_rebalancing, + count_shares_needed, count_shares_expected, + count_shares_good, count_good_share_hosts, + count_recoverable_versions, count_unrecoverable_versions, + servers_responding, sharemap, + count_wrong_shares, list_corrupt_shares, count_corrupt_shares, + list_incompatible_shares, count_incompatible_shares, + summary, report, share_problems, servermap): assert IURI.providedBy(uri), uri self.uri = uri self.storage_index = storage_index - self.problems = [] self.summary = "" self.report = [] - - def set_healthy(self, healthy): self.healthy = bool(healthy) if self.healthy: - assert (not hasattr(self, 'recoverable')) or self.recoverable, hasattr(self, 'recoverable') and self.recoverable - self.recoverable = True - self.summary = "healthy" + assert recoverable + if not summary: + summary = "healthy" else: - self.summary = "not healthy" - def set_recoverable(self, recoverable): + if not summary: + summary = "not healthy" self.recoverable = recoverable if not self.recoverable: - assert (not hasattr(self, 'healthy')) or not self.healthy - self.healthy = False - def set_needs_rebalancing(self, needs_rebalancing): + assert not self.healthy self.needs_rebalancing_p = bool(needs_rebalancing) - def set_data(self, - count_shares_needed, count_shares_expected, - count_shares_good, count_good_share_hosts, - count_recoverable_versions, count_unrecoverable_versions, - servers_responding, sharemap, - count_wrong_shares, list_corrupt_shares, count_corrupt_shares, - list_incompatible_shares, count_incompatible_shares): + for s in servers_responding: + assert isinstance(s, str), s + for shnum, serverids in sharemap.items(): + for serverid in serverids: + assert isinstance(serverid, str), serverid + for (serverid, SI, shnum) in list_corrupt_shares: + assert isinstance(serverid, str), serverid + for (serverid, SI, shnum) in list_incompatible_shares: + assert isinstance(serverid, str), serverid data = {"count-shares-needed": count_shares_needed, "count-shares-expected": count_shares_expected, "count-shares-good": count_shares_good, @@ -52,17 +57,15 @@ class CheckResults: "count-incompatible-shares": count_incompatible_shares, } self._data = data - def set_summary(self, summary): assert isinstance(summary, str) # should be a single string self.summary = summary - def set_report(self, report): assert not isinstance(report, str) # should be list of strings self.report = report - - def set_servermap(self, smap): - # mutable only - self.servermap = smap - + if servermap: + from allmydata.mutable.servermap import ServerMap + assert isinstance(servermap, ServerMap), servermap + self.servermap = servermap # mutable only + self._share_problems = share_problems def get_storage_index(self): return self.storage_index @@ -116,6 +119,8 @@ class CheckResults: return self.summary def get_report(self): return self.report + def get_share_problems(self): + return self._share_problems def get_servermap(self): return self.servermap diff --git a/src/allmydata/immutable/checker.py b/src/allmydata/immutable/checker.py index fe03f5eb..8521616b 100644 --- a/src/allmydata/immutable/checker.py +++ b/src/allmydata/immutable/checker.py @@ -738,7 +738,6 @@ class Checker(log.PrefixingLogMixin): def _format_results(self, results): SI = self._verifycap.get_storage_index() - cr = CheckResults(self._verifycap, SI) verifiedshares = dictutil.DictOfSets() # {sharenum: set(serverid)} servers = {} # {serverid: set(sharenums)} @@ -762,43 +761,46 @@ class Checker(log.PrefixingLogMixin): assert len(verifiedshares) <= self._verifycap.total_shares, (verifiedshares.keys(), self._verifycap.total_shares) if len(verifiedshares) == self._verifycap.total_shares: - cr.set_healthy(True) - cr.set_summary("Healthy") + healthy = True + summary = "Healthy" else: - cr.set_healthy(False) - cr.set_summary("Not Healthy: %d shares (enc %d-of-%d)" % - (len(verifiedshares), - self._verifycap.needed_shares, - self._verifycap.total_shares)) + healthy = False + summary = ("Not Healthy: %d shares (enc %d-of-%d)" % + (len(verifiedshares), + self._verifycap.needed_shares, + self._verifycap.total_shares)) if len(verifiedshares) >= self._verifycap.needed_shares: - cr.set_recoverable(True) recoverable = 1 unrecoverable = 0 else: - cr.set_recoverable(False) recoverable = 0 unrecoverable = 1 # The file needs rebalancing if the set of servers that have at least # one share is less than the number of uniquely-numbered shares # available. - cr.set_needs_rebalancing(good_share_hosts < len(verifiedshares)) - - cr.set_data( - count_shares_needed=self._verifycap.needed_shares, - count_shares_expected=self._verifycap.total_shares, - count_shares_good=len(verifiedshares), - count_good_share_hosts=good_share_hosts, - count_recoverable_versions=recoverable, - count_unrecoverable_versions=unrecoverable, - servers_responding=list(servers_responding), - sharemap=verifiedshares, - count_wrong_shares=0, # no such thing as wrong, for immutable - list_corrupt_shares=corruptshare_locators, - count_corrupt_shares=len(corruptshare_locators), - list_incompatible_shares=incompatibleshare_locators, - count_incompatible_shares=len(incompatibleshare_locators), - ) + needs_rebalancing = bool(good_share_hosts < len(verifiedshares)) + + cr = CheckResults(self._verifycap, SI, + healthy=healthy, recoverable=bool(recoverable), + needs_rebalancing=needs_rebalancing, + count_shares_needed=self._verifycap.needed_shares, + count_shares_expected=self._verifycap.total_shares, + count_shares_good=len(verifiedshares), + count_good_share_hosts=good_share_hosts, + count_recoverable_versions=recoverable, + count_unrecoverable_versions=unrecoverable, + servers_responding=list(servers_responding), + sharemap=verifiedshares, + count_wrong_shares=0, # no such thing, for immutable + list_corrupt_shares=corruptshare_locators, + count_corrupt_shares=len(corruptshare_locators), + list_incompatible_shares=incompatibleshare_locators, + count_incompatible_shares=len(incompatibleshare_locators), + summary=summary, + report=[], + share_problems=[], + servermap=None) return cr diff --git a/src/allmydata/immutable/filenode.py b/src/allmydata/immutable/filenode.py index 0c9b719a..a6eeab6e 100644 --- a/src/allmydata/immutable/filenode.py +++ b/src/allmydata/immutable/filenode.py @@ -124,7 +124,6 @@ class CiphertextFileNode: assert IUploadResults.providedBy(ur), ur # clone the cr (check results) to form the basis of the # prr (post-repair results) - prr = CheckResults(cr.uri, cr.storage_index) verifycap = self._verifycap servers_responding = set(cr.get_servers_responding()) @@ -142,26 +141,28 @@ class CiphertextFileNode: good_hosts = len(reduce(set.union, sm.values(), set())) is_healthy = bool(len(sm) >= verifycap.total_shares) is_recoverable = bool(len(sm) >= verifycap.needed_shares) - prr.set_data( - count_shares_needed=verifycap.needed_shares, - count_shares_expected=verifycap.total_shares, - count_shares_good=len(sm), - count_good_share_hosts=good_hosts, - count_recoverable_versions=int(is_recoverable), - count_unrecoverable_versions=int(not is_recoverable), - servers_responding=list(servers_responding), - sharemap=sm, - count_wrong_shares=0, # no such thing as wrong, for immutable - list_corrupt_shares=cr.get_corrupt_shares(), - count_corrupt_shares=len(cr.get_corrupt_shares()), - list_incompatible_shares=cr.get_incompatible_shares(), - count_incompatible_shares=len(cr.get_incompatible_shares()), - ) - prr.set_healthy(is_healthy) - prr.set_recoverable(is_recoverable) + needs_rebalancing = bool(len(sm) >= verifycap.total_shares) + prr = CheckResults(cr.uri, cr.storage_index, + healthy=is_healthy, recoverable=is_recoverable, + needs_rebalancing=needs_rebalancing, + count_shares_needed=verifycap.needed_shares, + count_shares_expected=verifycap.total_shares, + count_shares_good=len(sm), + count_good_share_hosts=good_hosts, + count_recoverable_versions=int(is_recoverable), + count_unrecoverable_versions=int(not is_recoverable), + servers_responding=list(servers_responding), + sharemap=sm, + count_wrong_shares=0, # no such thing as wrong, for immutable + list_corrupt_shares=cr.get_corrupt_shares(), + count_corrupt_shares=len(cr.get_corrupt_shares()), + list_incompatible_shares=cr.get_incompatible_shares(), + count_incompatible_shares=len(cr.get_incompatible_shares()), + summary="", + report=[], + share_problems=[], + servermap=None) crr.repair_successful = is_healthy - prr.set_needs_rebalancing(len(sm) >= verifycap.total_shares) - crr.post_repair_results = prr return crr diff --git a/src/allmydata/mutable/checker.py b/src/allmydata/mutable/checker.py index e4525d73..d76c0ca0 100644 --- a/src/allmydata/mutable/checker.py +++ b/src/allmydata/mutable/checker.py @@ -121,10 +121,7 @@ class MutableChecker: return counters def _make_checker_results(self, smap): - r = CheckResults(from_string(self._node.get_uri()), - self._storage_index) self._monitor.raise_if_cancelled() - r.set_servermap(smap.copy()) healthy = True report = [] summary = [] @@ -190,6 +187,7 @@ class MutableChecker: } corrupt_share_locators = [] + problems = [] if self.bad_shares: report.append("Corrupt Shares:") summary.append("Corrupt Shares:") @@ -205,7 +203,7 @@ class MutableChecker: report.append(" %s: %s" % (s, ft)) summary.append(s) p = (serverid, self._storage_index, shnum, f) - r.problems.append(p) + problems.append(p) msg = ("CorruptShareError during mutable verify, " "serverid=%(serverid)s, si=%(si)s, shnum=%(shnum)d, " "where=%(where)s") @@ -224,31 +222,33 @@ class MutableChecker: sharemap[shareid].append(server.get_serverid()) servers_responding = [s.get_serverid() for s in list(smap.get_reachable_servers())] - r.set_data( - count_shares_needed=counters["count-shares-needed"], - count_shares_expected=counters["count-shares-expected"], - count_shares_good=counters["count-shares-good"], - count_good_share_hosts=counters["count-good-share-hosts"], - count_recoverable_versions=len(recoverable), - count_unrecoverable_versions=len(unrecoverable), - servers_responding=servers_responding, - sharemap=sharemap, - count_wrong_shares=counters["count-wrong-shares"], - list_corrupt_shares=corrupt_share_locators, - count_corrupt_shares=len(corrupt_share_locators), - list_incompatible_shares=[], - count_incompatible_shares=0, - ) - - r.set_healthy(healthy) - r.set_recoverable(bool(recoverable)) - r.set_needs_rebalancing(needs_rebalancing) if healthy: - r.set_summary("Healthy") + summary = "Healthy" else: - r.set_summary("Unhealthy: " + " ".join(summary)) - r.set_report(report) - return r + summary = "Unhealthy: " + " ".join(summary) + + cr = CheckResults(from_string(self._node.get_uri()), + self._storage_index, + healthy=healthy, recoverable=bool(recoverable), + needs_rebalancing=needs_rebalancing, + count_shares_needed=counters["count-shares-needed"], + count_shares_expected=counters["count-shares-expected"], + count_shares_good=counters["count-shares-good"], + count_good_share_hosts=counters["count-good-share-hosts"], + count_recoverable_versions=len(recoverable), + count_unrecoverable_versions=len(unrecoverable), + servers_responding=servers_responding, + sharemap=sharemap, + count_wrong_shares=counters["count-wrong-shares"], + list_corrupt_shares=corrupt_share_locators, + count_corrupt_shares=len(corrupt_share_locators), + list_incompatible_shares=[], + count_incompatible_shares=0, + summary=summary, + report=report, + share_problems=problems, + servermap=smap.copy()) + return cr class MutableCheckAndRepairer(MutableChecker): diff --git a/src/allmydata/test/common.py b/src/allmydata/test/common.py index abf0b4fd..0cc2a918 100644 --- a/src/allmydata/test/common.py +++ b/src/allmydata/test/common.py @@ -66,27 +66,27 @@ class FakeCHKFileNode: return self.storage_index def check(self, monitor, verify=False, add_lease=False): - r = CheckResults(self.my_uri, self.storage_index) nodeid = "\x00"*20 - r.set_data( - count_shares_needed=3, - count_shares_expected=10, - count_shares_good=10, - count_good_share_hosts=10, - count_recoverable_versions=1, - count_unrecoverable_versions=0, - servers_responding=[nodeid], - sharemap={1: [nodeid]}, - count_wrong_shares=0, - list_corrupt_shares=[], - count_corrupt_shares=0, - list_incompatible_shares=[], - count_incompatible_shares=0, - ) - r.set_healthy(True) - r.set_recoverable(True) - r.problems = [] - r.set_needs_rebalancing(False) + r = CheckResults(self.my_uri, self.storage_index, + healthy=True, recoverable=True, + needs_rebalancing=False, + count_shares_needed=3, + count_shares_expected=10, + count_shares_good=10, + count_good_share_hosts=10, + count_recoverable_versions=1, + count_unrecoverable_versions=0, + servers_responding=[nodeid], + sharemap={1: [nodeid]}, + count_wrong_shares=0, + list_corrupt_shares=[], + count_corrupt_shares=0, + list_incompatible_shares=[], + count_incompatible_shares=0, + summary="", + report=[], + share_problems=[], + servermap=None) return defer.succeed(r) def check_and_repair(self, monitor, verify=False, add_lease=False): d = self.check(verify) @@ -278,27 +278,27 @@ class FakeMutableFileNode: return self.file_types[self.storage_index] def check(self, monitor, verify=False, add_lease=False): - r = CheckResults(self.my_uri, self.storage_index) nodeid = "\x00"*20 - r.set_data( - count_shares_needed=3, - count_shares_expected=10, - count_shares_good=10, - count_good_share_hosts=10, - count_recoverable_versions=1, - count_unrecoverable_versions=0, - servers_responding=[nodeid], - sharemap={"seq1-abcd-sh0": [nodeid]}, - count_wrong_shares=0, - list_corrupt_shares=[], - count_corrupt_shares=0, - list_incompatible_shares=[], - count_incompatible_shares=0, - ) - r.set_healthy(True) - r.set_recoverable(True) - r.problems = [] - r.set_needs_rebalancing(False) + r = CheckResults(self.my_uri, self.storage_index, + healthy=True, recoverable=True, + needs_rebalancing=False, + count_shares_needed=3, + count_shares_expected=10, + count_shares_good=10, + count_good_share_hosts=10, + count_recoverable_versions=1, + count_unrecoverable_versions=0, + servers_responding=[nodeid], + sharemap={"seq1-abcd-sh0": [nodeid]}, + count_wrong_shares=0, + list_corrupt_shares=[], + count_corrupt_shares=0, + list_incompatible_shares=[], + count_incompatible_shares=0, + summary="", + report=[], + share_problems=[], + servermap=None) return defer.succeed(r) def check_and_repair(self, monitor, verify=False, add_lease=False): diff --git a/src/allmydata/test/test_checker.py b/src/allmydata/test/test_checker.py index d378860e..29e1df00 100644 --- a/src/allmydata/test/test_checker.py +++ b/src/allmydata/test/test_checker.py @@ -81,10 +81,6 @@ class WebResultsRendering(unittest.TestCase, WebRenderingMixin): serverid_1 = "\x00"*20 serverid_f = "\xff"*20 u = uri.CHKFileURI("\x00"*16, "\x00"*32, 3, 10, 1234) - cr = check_results.CheckResults(u, u.get_storage_index()) - cr.set_healthy(True) - cr.set_needs_rebalancing(False) - cr.set_summary("groovy") data = { "count_shares_needed": 3, "count_shares_expected": 9, "count_shares_good": 10, @@ -98,9 +94,13 @@ class WebResultsRendering(unittest.TestCase, WebRenderingMixin): "count_corrupt_shares": 0, "list_incompatible_shares": [], "count_incompatible_shares": 0, + "report": [], "share_problems": [], "servermap": None, } - cr.set_data(**data) - + cr = check_results.CheckResults(u, u.get_storage_index(), + healthy=True, recoverable=True, + needs_rebalancing=False, + summary="groovy", + **data) w = web_check_results.CheckResultsRenderer(c, cr) html = self.render2(w) s = self.remove_tags(html) @@ -113,20 +113,25 @@ class WebResultsRendering(unittest.TestCase, WebRenderingMixin): self.failUnlessIn("Recoverable Versions: 1", s) self.failUnlessIn("Unrecoverable Versions: 0", s) - cr.set_healthy(False) - cr.set_recoverable(True) - cr.set_summary("ungroovy") + cr = check_results.CheckResults(u, u.get_storage_index(), + healthy=False, recoverable=True, + needs_rebalancing=False, + summary="ungroovy", + **data) + w = web_check_results.CheckResultsRenderer(c, cr) html = self.render2(w) s = self.remove_tags(html) self.failUnlessIn("File Check Results for SI=2k6avp", s) # abbreviated self.failUnlessIn("Not Healthy! : ungroovy", s) - cr.set_healthy(False) - cr.set_recoverable(False) - cr.set_summary("rather dead") data["count_corrupt_shares"] = 1 data["list_corrupt_shares"] = [(serverid_1, u.get_storage_index(), 2)] - cr.set_data(**data) + cr = check_results.CheckResults(u, u.get_storage_index(), + healthy=False, recoverable=False, + needs_rebalancing=False, + summary="rather dead", + **data) + w = web_check_results.CheckResultsRenderer(c, cr) html = self.render2(w) s = self.remove_tags(html) self.failUnlessIn("File Check Results for SI=2k6avp", s) # abbreviated @@ -184,11 +189,6 @@ class WebResultsRendering(unittest.TestCase, WebRenderingMixin): serverid_f = "\xff"*20 u = uri.CHKFileURI("\x00"*16, "\x00"*32, 3, 10, 1234) - pre_cr = check_results.CheckResults(u, u.get_storage_index()) - pre_cr.set_healthy(False) - pre_cr.set_recoverable(True) - pre_cr.set_needs_rebalancing(False) - pre_cr.set_summary("illing") data = { "count_shares_needed": 3, "count_shares_expected": 10, "count_shares_good": 6, @@ -202,14 +202,14 @@ class WebResultsRendering(unittest.TestCase, WebRenderingMixin): "count_corrupt_shares": 0, "list_incompatible_shares": [], "count_incompatible_shares": 0, + "report": [], "share_problems": [], "servermap": None, } - pre_cr.set_data(**data) + pre_cr = check_results.CheckResults(u, u.get_storage_index(), + healthy=False, recoverable=True, + needs_rebalancing=False, + summary="illing", + **data) - post_cr = check_results.CheckResults(u, u.get_storage_index()) - post_cr.set_healthy(True) - post_cr.set_recoverable(True) - post_cr.set_needs_rebalancing(False) - post_cr.set_summary("groovy") data = { "count_shares_needed": 3, "count_shares_expected": 10, "count_shares_good": 10, @@ -223,8 +223,13 @@ class WebResultsRendering(unittest.TestCase, WebRenderingMixin): "list_corrupt_shares": [], "list_incompatible_shares": [], "count_incompatible_shares": 0, + "report": [], "share_problems": [], "servermap": None, } - post_cr.set_data(**data) + post_cr = check_results.CheckResults(u, u.get_storage_index(), + healthy=True, recoverable=True, + needs_rebalancing=False, + summary="groovy", + **data) crr = check_results.CheckAndRepairResults(u.get_storage_index()) crr.pre_repair_results = pre_cr @@ -253,8 +258,12 @@ class WebResultsRendering(unittest.TestCase, WebRenderingMixin): crr.repair_attempted = True crr.repair_successful = False - post_cr.set_healthy(False) - post_cr.set_summary("better") + post_cr = check_results.CheckResults(u, u.get_storage_index(), + healthy=False, recoverable=True, + needs_rebalancing=False, + summary="better", + **data) + crr.post_repair_results = post_cr html = self.render2(w) s = self.remove_tags(html) @@ -265,9 +274,12 @@ class WebResultsRendering(unittest.TestCase, WebRenderingMixin): crr.repair_attempted = True crr.repair_successful = False - post_cr.set_healthy(False) - post_cr.set_recoverable(False) - post_cr.set_summary("worse") + post_cr = check_results.CheckResults(u, u.get_storage_index(), + healthy=False, recoverable=False, + needs_rebalancing=False, + summary="worse", + **data) + crr.post_repair_results = post_cr html = self.render2(w) s = self.remove_tags(html) diff --git a/src/allmydata/test/test_mutable.py b/src/allmydata/test/test_mutable.py index e0deb669..c3867ca3 100644 --- a/src/allmydata/test/test_mutable.py +++ b/src/allmydata/test/test_mutable.py @@ -1660,14 +1660,14 @@ class CheckerMixin: return r def check_expected_failure(self, r, expected_exception, substring, where): - for (peerid, storage_index, shnum, f) in r.problems: + for (peerid, storage_index, shnum, f) in r.get_share_problems(): if f.check(expected_exception): self.failUnless(substring in str(f), "%s: substring '%s' not in '%s'" % (where, substring, str(f))) return self.fail("%s: didn't see expected exception %s in problems %s" % - (where, expected_exception, r.problems)) + (where, expected_exception, r.get_share_problems())) class Checker(unittest.TestCase, CheckerMixin, PublishMixin): -- 2.37.2