From: Brian Warner Date: Fri, 25 May 2012 07:13:23 +0000 (-0700) Subject: change CheckResults to use a fat set_data() X-Git-Url: https://git.rkrishnan.org/reliability?a=commitdiff_plain;h=ccfcd4de376f00e9aa9928780267e8cf555698aa;p=tahoe-lafs%2Ftahoe-lafs.git change CheckResults to use a fat set_data() i.e. change set_data() to accept lots of parameters, instead of taking a single dictionary with lots of keys. Also Convert all CheckResults creators to use it. --- diff --git a/src/allmydata/check_results.py b/src/allmydata/check_results.py index 445eed73..c1b791c5 100644 --- a/src/allmydata/check_results.py +++ b/src/allmydata/check_results.py @@ -30,7 +30,27 @@ class CheckResults: self.healthy = False def set_needs_rebalancing(self, needs_rebalancing): self.needs_rebalancing_p = bool(needs_rebalancing) - def set_data(self, data): + 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): + data = {"count-shares-needed": count_shares_needed, + "count-shares-expected": count_shares_expected, + "count-shares-good": count_shares_good, + "count-good-share-hosts": count_good_share_hosts, + "count-recoverable-versions": count_recoverable_versions, + "count-unrecoverable-versions": count_unrecoverable_versions, + "servers-responding": servers_responding, + "sharemap": sharemap, + "count-wrong-shares": count_wrong_shares, + "list-corrupt-shares": list_corrupt_shares, + "count-corrupt-shares": count_corrupt_shares, + "list-incompatible-shares": list_incompatible_shares, + "count-incompatible-shares": count_incompatible_shares, + } self._data = data def set_summary(self, summary): assert isinstance(summary, str) # should be a single string diff --git a/src/allmydata/immutable/checker.py b/src/allmydata/immutable/checker.py index 4b7a0059..fe03f5eb 100644 --- a/src/allmydata/immutable/checker.py +++ b/src/allmydata/immutable/checker.py @@ -739,9 +739,6 @@ class Checker(log.PrefixingLogMixin): def _format_results(self, results): SI = self._verifycap.get_storage_index() cr = CheckResults(self._verifycap, SI) - d = {} - d['count-shares-needed'] = self._verifycap.needed_shares - d['count-shares-expected'] = self._verifycap.total_shares verifiedshares = dictutil.DictOfSets() # {sharenum: set(serverid)} servers = {} # {serverid: set(sharenums)} @@ -761,8 +758,7 @@ class Checker(log.PrefixingLogMixin): if responded: servers_responding.add(server_id) - d['count-shares-good'] = len(verifiedshares) - d['count-good-share-hosts'] = len([s for s in servers.keys() if servers[s]]) + good_share_hosts = len([s for s in servers.keys() if servers[s]]) assert len(verifiedshares) <= self._verifycap.total_shares, (verifiedshares.keys(), self._verifycap.total_shares) if len(verifiedshares) == self._verifycap.total_shares: @@ -776,29 +772,33 @@ class Checker(log.PrefixingLogMixin): self._verifycap.total_shares)) if len(verifiedshares) >= self._verifycap.needed_shares: cr.set_recoverable(True) - d['count-recoverable-versions'] = 1 - d['count-unrecoverable-versions'] = 0 + recoverable = 1 + unrecoverable = 0 else: cr.set_recoverable(False) - d['count-recoverable-versions'] = 0 - d['count-unrecoverable-versions'] = 1 - - d['servers-responding'] = list(servers_responding) - d['sharemap'] = verifiedshares - # no such thing as wrong shares of an immutable file - d['count-wrong-shares'] = 0 - d['list-corrupt-shares'] = corruptshare_locators - d['count-corrupt-shares'] = len(corruptshare_locators) - d['list-incompatible-shares'] = incompatibleshare_locators - d['count-incompatible-shares'] = len(incompatibleshare_locators) - + 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(d['count-good-share-hosts'] < d['count-shares-good']) - - cr.set_data(d) + 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), + ) return cr diff --git a/src/allmydata/immutable/filenode.py b/src/allmydata/immutable/filenode.py index dbd09e0b..d8735189 100644 --- a/src/allmydata/immutable/filenode.py +++ b/src/allmydata/immutable/filenode.py @@ -128,6 +128,7 @@ class CiphertextFileNode: prr = CheckResults(cr.uri, cr.storage_index) prr_data = copy.deepcopy(cr.get_data()) + verifycap = self._verifycap servers_responding = set(prr_data['servers-responding']) sm = prr_data['sharemap'] assert isinstance(sm, DictOfSets), sm @@ -136,14 +137,25 @@ class CiphertextFileNode: sm.add(shnum, s.get_serverid()) servers_responding.add(s.get_serverid()) servers_responding = sorted(servers_responding) - prr_data['servers-responding'] = servers_responding - prr_data['count-shares-good'] = len(sm) + good_hosts = len(reduce(set.union, sm.itervalues(), set())) - prr_data['count-good-share-hosts'] = good_hosts - prr.set_data(prr_data) - verifycap = self._verifycap 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=prr_data["list-corrupt-shares"], + count_corrupt_shares=prr_data["count-corrupt-shares"], + list_incompatible_shares=prr_data["list-incompatible-shares"], + count_incompatible_shares=prr_data["count-incompatible-shares"], + ) prr.set_healthy(is_healthy) prr.set_recoverable(is_recoverable) crr.repair_successful = is_healthy diff --git a/src/allmydata/mutable/checker.py b/src/allmydata/mutable/checker.py index 6b629eca..d02cfb0e 100644 --- a/src/allmydata/mutable/checker.py +++ b/src/allmydata/mutable/checker.py @@ -126,14 +126,11 @@ class MutableChecker: self._monitor.raise_if_cancelled() r.set_servermap(smap.copy()) healthy = True - data = {} report = [] summary = [] vmap = smap.make_versionmap() recoverable = smap.recoverable_versions() unrecoverable = smap.unrecoverable_versions() - data["count-recoverable-versions"] = len(recoverable) - data["count-unrecoverable-versions"] = len(unrecoverable) if recoverable: report.append("Recoverable Versions: " + @@ -164,7 +161,6 @@ class MutableChecker: report.append("Best Recoverable Version: " + smap.summarize_version(best_version)) counters = self._count_shares(smap, best_version) - data.update(counters) s = counters["count-shares-good"] k = counters["count-shares-needed"] N = counters["count-shares-expected"] @@ -180,45 +176,44 @@ class MutableChecker: # find a k and N from somewhere first = list(unrecoverable)[0] # not exactly the best version, but that doesn't matter too much - data.update(self._count_shares(smap, first)) + counters = self._count_shares(smap, first) # leave needs_rebalancing=False: the file being unrecoverable is # the bigger problem else: # couldn't find anything at all - data["count-shares-good"] = 0 - data["count-shares-needed"] = 3 # arbitrary defaults - data["count-shares-expected"] = 10 - data["count-good-share-hosts"] = 0 - data["count-wrong-shares"] = 0 + counters = { + "count-shares-good": 0, + "count-shares-needed": 3, # arbitrary defaults + "count-shares-expected": 10, + "count-good-share-hosts": 0, + "count-wrong-shares": 0, + } + corrupt_share_locators = [] if self.bad_shares: - data["count-corrupt-shares"] = len(self.bad_shares) - data["list-corrupt-shares"] = locators = [] report.append("Corrupt Shares:") summary.append("Corrupt Shares:") - for (server, shnum, f) in sorted(self.bad_shares): - serverid = server.get_serverid() - locators.append( (serverid, self._storage_index, shnum) ) - s = "%s-sh%d" % (server.get_name(), shnum) - if f.check(CorruptShareError): - ft = f.value.reason - else: - ft = str(f) - report.append(" %s: %s" % (s, ft)) - summary.append(s) - p = (serverid, self._storage_index, shnum, f) - r.problems.append(p) - msg = ("CorruptShareError during mutable verify, " - "serverid=%(serverid)s, si=%(si)s, shnum=%(shnum)d, " - "where=%(where)s") - log.msg(format=msg, serverid=server.get_name(), - si=base32.b2a(self._storage_index), - shnum=shnum, - where=ft, - level=log.WEIRD, umid="EkK8QA") - else: - data["count-corrupt-shares"] = 0 - data["list-corrupt-shares"] = [] + for (server, shnum, f) in sorted(self.bad_shares): + serverid = server.get_serverid() + locator = (serverid, self._storage_index, shnum) + corrupt_share_locators.append(locator) + s = "%s-sh%d" % (server.get_name(), shnum) + if f.check(CorruptShareError): + ft = f.value.reason + else: + ft = str(f) + report.append(" %s: %s" % (s, ft)) + summary.append(s) + p = (serverid, self._storage_index, shnum, f) + r.problems.append(p) + msg = ("CorruptShareError during mutable verify, " + "serverid=%(serverid)s, si=%(si)s, shnum=%(shnum)d, " + "where=%(where)s") + log.msg(format=msg, serverid=server.get_name(), + si=base32.b2a(self._storage_index), + shnum=shnum, + where=ft, + level=log.WEIRD, umid="EkK8QA") sharemap = {} for verinfo in vmap: @@ -227,14 +222,27 @@ class MutableChecker: if shareid not in sharemap: sharemap[shareid] = [] sharemap[shareid].append(server.get_serverid()) - data["sharemap"] = sharemap - data["servers-responding"] = [s.get_serverid() for s in - list(smap.get_reachable_servers())] + 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) - r.set_data(data) if healthy: r.set_summary("Healthy") else: diff --git a/src/allmydata/test/common.py b/src/allmydata/test/common.py index ff9db980..abf0b4fd 100644 --- a/src/allmydata/test/common.py +++ b/src/allmydata/test/common.py @@ -67,23 +67,25 @@ class FakeCHKFileNode: def check(self, monitor, verify=False, add_lease=False): r = CheckResults(self.my_uri, self.storage_index) - data = {} - data["count-shares-needed"] = 3 - data["count-shares-expected"] = 10 - data["count-good-share-hosts"] = 10 - data["count-wrong-shares"] = 0 nodeid = "\x00"*20 - data["count-corrupt-shares"] = 0 - data["list-corrupt-shares"] = [] - data["sharemap"] = {1: [nodeid]} - data["servers-responding"] = [nodeid] - data["count-recoverable-versions"] = 1 - data["count-unrecoverable-versions"] = 0 + 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) - data["count-shares-good"] = 10 r.problems = [] - r.set_data(data) r.set_needs_rebalancing(False) return defer.succeed(r) def check_and_repair(self, monitor, verify=False, add_lease=False): @@ -277,23 +279,25 @@ class FakeMutableFileNode: def check(self, monitor, verify=False, add_lease=False): r = CheckResults(self.my_uri, self.storage_index) - data = {} - data["count-shares-needed"] = 3 - data["count-shares-expected"] = 10 - data["count-good-share-hosts"] = 10 - data["count-wrong-shares"] = 0 - data["count-corrupt-shares"] = 0 - data["list-corrupt-shares"] = [] nodeid = "\x00"*20 - data["sharemap"] = {"seq1-abcd-sh0": [nodeid]} - data["servers-responding"] = [nodeid] - data["count-recoverable-versions"] = 1 - data["count-unrecoverable-versions"] = 0 + 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) - data["count-shares-good"] = 10 r.problems = [] - r.set_data(data) r.set_needs_rebalancing(False) return defer.succeed(r) diff --git a/src/allmydata/test/test_checker.py b/src/allmydata/test/test_checker.py index 816c57fd..585a3e43 100644 --- a/src/allmydata/test/test_checker.py +++ b/src/allmydata/test/test_checker.py @@ -85,19 +85,21 @@ class WebResultsRendering(unittest.TestCase, WebRenderingMixin): 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, - "count-good-share-hosts": 11, - "count-corrupt-shares": 0, - "list-corrupt-shares": [], - "count-wrong-shares": 0, + data = { "count_shares_needed": 3, + "count_shares_expected": 9, + "count_shares_good": 10, + "count_good_share_hosts": 11, + "count_recoverable_versions": 1, + "count_unrecoverable_versions": 0, + "servers_responding": [], "sharemap": {"shareid1": [serverid_1, serverid_f]}, - "count-recoverable-versions": 1, - "count-unrecoverable-versions": 0, - "servers-responding": [], + "count_wrong_shares": 0, + "list_corrupt_shares": [], + "count_corrupt_shares": 0, + "list_incompatible_shares": [], + "count_incompatible_shares": 0, } - cr.set_data(data) + cr.set_data(**data) w = web_check_results.CheckResultsRenderer(c, cr) html = self.render2(w) @@ -122,9 +124,9 @@ class WebResultsRendering(unittest.TestCase, WebRenderingMixin): 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) + data["count_corrupt_shares"] = 1 + data["list_corrupt_shares"] = [(serverid_1, u.get_storage_index(), 2)] + cr.set_data(**data) html = self.render2(w) s = self.remove_tags(html) self.failUnlessIn("File Check Results for SI=2k6avp", s) # abbreviated @@ -187,38 +189,42 @@ class WebResultsRendering(unittest.TestCase, WebRenderingMixin): 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, - "count-good-share-hosts": 7, - "count-corrupt-shares": 0, - "list-corrupt-shares": [], - "count-wrong-shares": 0, + data = { "count_shares_needed": 3, + "count_shares_expected": 10, + "count_shares_good": 6, + "count_good_share_hosts": 7, + "count_recoverable_versions": 1, + "count_unrecoverable_versions": 0, + "servers_responding": [], "sharemap": {"shareid1": [serverid_1, serverid_f]}, - "count-recoverable-versions": 1, - "count-unrecoverable-versions": 0, - "servers-responding": [], + "count_wrong_shares": 0, + "list_corrupt_shares": [], + "count_corrupt_shares": 0, + "list_incompatible_shares": [], + "count_incompatible_shares": 0, } - pre_cr.set_data(data) + pre_cr.set_data(**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, - "count-good-share-hosts": 11, - "count-corrupt-shares": 0, - "list-corrupt-shares": [], - "count-wrong-shares": 0, + data = { "count_shares_needed": 3, + "count_shares_expected": 10, + "count_shares_good": 10, + "count_good_share_hosts": 11, + "count_recoverable_versions": 1, + "count_unrecoverable_versions": 0, + "servers_responding": [], "sharemap": {"shareid1": [serverid_1, serverid_f]}, - "count-recoverable-versions": 1, - "count-unrecoverable-versions": 0, - "servers-responding": [], + "count_wrong_shares": 0, + "count_corrupt_shares": 0, + "list_corrupt_shares": [], + "list_incompatible_shares": [], + "count_incompatible_shares": 0, } - post_cr.set_data(data) + post_cr.set_data(**data) crr = check_results.CheckAndRepairResults(u.get_storage_index()) crr.pre_repair_results = pre_cr