From 3e0dc9449757054fb3a3fe3ffad8164c5fdb265d Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Wed, 25 Aug 2021 13:06:10 -0400 Subject: [PATCH 1/9] Annotate the two fakes that (at least partially) implement RIStorageServer, so they're easier to find. --- src/allmydata/test/mutable/util.py | 8 +++++++- src/allmydata/test/test_upload.py | 8 ++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/src/allmydata/test/mutable/util.py b/src/allmydata/test/mutable/util.py index 62e8d7295..7e3bd3ec7 100644 --- a/src/allmydata/test/mutable/util.py +++ b/src/allmydata/test/mutable/util.py @@ -96,8 +96,14 @@ class FakeStorage(object): shares[shnum] = f.getvalue() +# This doesn't actually implement the whole interface, but adding a commented +# interface implementation annotation for grepping purposes. +#@implementer(RIStorageServer) class FakeStorageServer(object): - + """ + A fake Foolscap remote object, implemented by overriding callRemote() to + call local methods. + """ def __init__(self, peerid, storage): self.peerid = peerid self.storage = storage diff --git a/src/allmydata/test/test_upload.py b/src/allmydata/test/test_upload.py index fc9bfd697..8d5435e88 100644 --- a/src/allmydata/test/test_upload.py +++ b/src/allmydata/test/test_upload.py @@ -122,7 +122,15 @@ class SetDEPMixin(object): } self.node.encoding_params = p + +# This doesn't actually implement the whole interface, but adding a commented +# interface implementation annotation for grepping purposes. +#@implementer(RIStorageServer) class FakeStorageServer(object): + """ + A fake Foolscap remote object, implemented by overriding callRemote() to + call local methods. + """ def __init__(self, mode, reactor=None): self.mode = mode self.allocated = [] From 6c679bd4e0292af572026353d0e7a1c74e01184f Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 2 Sep 2021 11:35:39 -0400 Subject: [PATCH 2/9] Stop using callRemoteOnly. --- src/allmydata/immutable/layout.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/allmydata/immutable/layout.py b/src/allmydata/immutable/layout.py index 3db9f096e..886a5db73 100644 --- a/src/allmydata/immutable/layout.py +++ b/src/allmydata/immutable/layout.py @@ -15,7 +15,7 @@ from zope.interface import implementer from twisted.internet import defer from allmydata.interfaces import IStorageBucketWriter, IStorageBucketReader, \ FileTooLargeError, HASH_SIZE -from allmydata.util import mathutil, observer, pipeline +from allmydata.util import mathutil, observer, pipeline, log from allmydata.util.assertutil import precondition from allmydata.storage.server import si_b2a @@ -254,8 +254,7 @@ class WriteBucketProxy(object): return d def abort(self): - return self._rref.callRemoteOnly("abort") - + self._rref.callRemote("abort").addErrback(log.err) def get_servername(self): return self._server.get_name() From 63bfff19e9be9665461b4355e56ced50a227d2a8 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 2 Sep 2021 15:05:15 -0400 Subject: [PATCH 3/9] Don't rely on Foolscap's semantics. --- src/allmydata/mutable/servermap.py | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/allmydata/mutable/servermap.py b/src/allmydata/mutable/servermap.py index 4f3226649..211b1fc16 100644 --- a/src/allmydata/mutable/servermap.py +++ b/src/allmydata/mutable/servermap.py @@ -607,13 +607,14 @@ class ServermapUpdater(object): return d def _do_read(self, server, storage_index, shnums, readv): + """ + If self._add_lease is true, a lease is added, and the result only fires + once the least has also been added. + """ ss = server.get_storage_server() if self._add_lease: # send an add-lease message in parallel. The results are handled - # separately. This is sent before the slot_readv() so that we can - # be sure the add_lease is retired by the time slot_readv comes - # back (this relies upon our knowledge that the server code for - # add_lease is synchronous). + # separately. renew_secret = self._node.get_renewal_secret(server) cancel_secret = self._node.get_cancel_secret(server) d2 = ss.add_lease( @@ -623,7 +624,16 @@ class ServermapUpdater(object): ) # we ignore success d2.addErrback(self._add_lease_failed, server, storage_index) + else: + d2 = defer.succeed(None) d = ss.slot_readv(storage_index, shnums, readv) + + def passthrough(result): + # Wait for d2, but fire with result of slot_readv() regardless of + # result of d2. + return d2.addBoth(lambda _: result) + + d.addCallback(passthrough) return d From 789a7edb56501e5e7d95c93e7570bb4fc1197507 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 2 Sep 2021 15:21:42 -0400 Subject: [PATCH 4/9] Get rid of more callRemoteOnly usage. --- src/allmydata/immutable/downloader/share.py | 4 +++- src/allmydata/storage_client.py | 4 ++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/allmydata/immutable/downloader/share.py b/src/allmydata/immutable/downloader/share.py index 86b99e99c..1e751500b 100644 --- a/src/allmydata/immutable/downloader/share.py +++ b/src/allmydata/immutable/downloader/share.py @@ -475,7 +475,9 @@ class Share(object): # there was corruption somewhere in the given range reason = "corruption in share[%d-%d): %s" % (start, start+offset, str(f.value)) - self._rref.callRemoteOnly("advise_corrupt_share", reason.encode("utf-8")) + self._rref.callRemote( + "advise_corrupt_share", reason.encode("utf-8") + ).addErrback(log.err) def _satisfy_block_hash_tree(self, needed_hashes): o_bh = self.actual_offsets["block_hashes"] diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index 22dd09bcb..c278abce3 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -1009,10 +1009,10 @@ class _StorageServer(object): shnum, reason, ): - return self._rref.callRemoteOnly( + self._rref.callRemote( "advise_corrupt_share", share_type, storage_index, shnum, reason, - ) + ).addErrback(log.err) From 04d2b27f46d9dee1122947e1f5bf50ac87a5b4aa Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 2 Sep 2021 15:24:19 -0400 Subject: [PATCH 5/9] News file. --- newsfragments/3779.minor | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 newsfragments/3779.minor diff --git a/newsfragments/3779.minor b/newsfragments/3779.minor new file mode 100644 index 000000000..e69de29bb From 148a0573dea02d67e9b05e1156afe47807402a96 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Fri, 3 Sep 2021 13:11:02 -0400 Subject: [PATCH 6/9] Replace colon on filename only, not on whole path. This would break Windows logging of corruption reports, since colon would be removed from e.g. "C:". --- src/allmydata/storage/server.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/allmydata/storage/server.py b/src/allmydata/storage/server.py index 56b47ae89..f4996756e 100644 --- a/src/allmydata/storage/server.py +++ b/src/allmydata/storage/server.py @@ -708,8 +708,10 @@ class StorageServer(service.MultiService, Referenceable): now = time_format.iso_utc(sep="T") si_s = si_b2a(storage_index) # windows can't handle colons in the filename - fn = os.path.join(self.corruption_advisory_dir, - "%s--%s-%d" % (now, str(si_s, "utf-8"), shnum)).replace(":","") + fn = os.path.join( + self.corruption_advisory_dir, + ("%s--%s-%d" % (now, str(si_s, "utf-8"), shnum)).replace(":","") + ) with open(fn, "w") as f: f.write("report: Share Corruption\n") f.write("type: %s\n" % bytes_to_native_str(share_type)) From c7e82b1640238b9e8af65d3f05a7a21647cba183 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Fri, 3 Sep 2021 13:12:57 -0400 Subject: [PATCH 7/9] Guess it's a bugfix. --- newsfragments/3779.bugfix | 1 + newsfragments/3779.minor | 0 2 files changed, 1 insertion(+) create mode 100644 newsfragments/3779.bugfix delete mode 100644 newsfragments/3779.minor diff --git a/newsfragments/3779.bugfix b/newsfragments/3779.bugfix new file mode 100644 index 000000000..c6745f1af --- /dev/null +++ b/newsfragments/3779.bugfix @@ -0,0 +1 @@ +Fixed bug where share corruption events were not recorded on Windows. \ No newline at end of file diff --git a/newsfragments/3779.minor b/newsfragments/3779.minor deleted file mode 100644 index e69de29bb..000000000 From ac9875da75c1dd42080f1d18a506a17846671df6 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Fri, 10 Sep 2021 11:39:48 -0400 Subject: [PATCH 8/9] Add explanation to new error logging. --- src/allmydata/immutable/downloader/share.py | 2 +- src/allmydata/immutable/layout.py | 2 +- src/allmydata/storage_client.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/allmydata/immutable/downloader/share.py b/src/allmydata/immutable/downloader/share.py index 1e751500b..41e11426f 100644 --- a/src/allmydata/immutable/downloader/share.py +++ b/src/allmydata/immutable/downloader/share.py @@ -477,7 +477,7 @@ class Share(object): str(f.value)) self._rref.callRemote( "advise_corrupt_share", reason.encode("utf-8") - ).addErrback(log.err) + ).addErrback(log.err, "Error from remote call to advise_corrupt_share") def _satisfy_block_hash_tree(self, needed_hashes): o_bh = self.actual_offsets["block_hashes"] diff --git a/src/allmydata/immutable/layout.py b/src/allmydata/immutable/layout.py index 886a5db73..6c7362b8a 100644 --- a/src/allmydata/immutable/layout.py +++ b/src/allmydata/immutable/layout.py @@ -254,7 +254,7 @@ class WriteBucketProxy(object): return d def abort(self): - self._rref.callRemote("abort").addErrback(log.err) + self._rref.callRemote("abort").addErrback(log.err, "Error from remote call to abort an immutable write bucket") def get_servername(self): return self._server.get_name() diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index c278abce3..e9dc8c84c 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -1015,4 +1015,4 @@ class _StorageServer(object): storage_index, shnum, reason, - ).addErrback(log.err) + ).addErrback(log.err, "Error from remote call to advise_corrupt_share") From fb61e29b5933fb9f84df4fc319e48a56f8e22659 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Fri, 10 Sep 2021 11:40:30 -0400 Subject: [PATCH 9/9] Better explanation. --- newsfragments/3779.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/newsfragments/3779.bugfix b/newsfragments/3779.bugfix index c6745f1af..073046474 100644 --- a/newsfragments/3779.bugfix +++ b/newsfragments/3779.bugfix @@ -1 +1 @@ -Fixed bug where share corruption events were not recorded on Windows. \ No newline at end of file +Fixed bug where share corruption events were not logged on storage servers running on Windows. \ No newline at end of file