From ccfcd4de376f00e9aa9928780267e8cf555698aa Mon Sep 17 00:00:00 2001
From: Brian Warner <warner@lothar.com>
Date: Fri, 25 May 2012 00:13:23 -0700
Subject: [PATCH] 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.
---
 src/allmydata/check_results.py      | 22 +++++++-
 src/allmydata/immutable/checker.py  | 44 +++++++--------
 src/allmydata/immutable/filenode.py | 22 ++++++--
 src/allmydata/mutable/checker.py    | 86 ++++++++++++++++-------------
 src/allmydata/test/common.py        | 56 ++++++++++---------
 src/allmydata/test/test_checker.py  | 78 ++++++++++++++------------
 6 files changed, 179 insertions(+), 129 deletions(-)

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
-- 
2.45.2