From 7faec6e5a0bb53f5d58a4253795047144a58d62d Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 10 Nov 2021 15:48:58 -0500 Subject: [PATCH 1/9] news fragment --- newsfragments/3842.minor | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 newsfragments/3842.minor diff --git a/newsfragments/3842.minor b/newsfragments/3842.minor new file mode 100644 index 000000000..e69de29bb From 9af81d21c5af232c8e02e874b09ac33202cb5158 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 10 Nov 2021 16:08:40 -0500 Subject: [PATCH 2/9] add a way to turn off implicit bucket lease renewal too --- src/allmydata/storage/server.py | 50 ++++++++++++++++++++----- src/allmydata/test/test_storage.py | 59 +++++++++++++++++++++++++++++- 2 files changed, 98 insertions(+), 11 deletions(-) diff --git a/src/allmydata/storage/server.py b/src/allmydata/storage/server.py index 3e2d3b5c6..d142646a8 100644 --- a/src/allmydata/storage/server.py +++ b/src/allmydata/storage/server.py @@ -57,9 +57,23 @@ DEFAULT_RENEWAL_TIME = 31 * 24 * 60 * 60 @implementer(RIStorageServer, IStatsProducer) class StorageServer(service.MultiService, Referenceable): + """ + A filesystem-based implementation of ``RIStorageServer``. + + :ivar bool _implicit_bucket_lease_renewal: If and only if this is ``True`` + then ``allocate_buckets`` will renew leases on existing shares + associated with the storage index it operates on. + + :ivar bool _implicit_slot_lease_renewal: If and only if this is ``True`` + then ``slot_testv_and_readv_and_writev`` will renew leases on shares + associated with the slot it operates on. + """ name = 'storage' LeaseCheckerClass = LeaseCheckingCrawler + _implicit_bucket_lease_renewal = True + _implicit_slot_lease_renewal = True + def __init__(self, storedir, nodeid, reserved_space=0, discard_storage=False, readonly_storage=False, stats_provider=None, @@ -135,6 +149,29 @@ class StorageServer(service.MultiService, Referenceable): def __repr__(self): return "" % (idlib.shortnodeid_b2a(self.my_nodeid),) + def set_implicit_bucket_lease_renewal(self, enabled): + # type: (bool) -> None + """ + Control the behavior of implicit lease renewal by *allocate_buckets*. + + :param enabled: If and only if ``True`` then future *allocate_buckets* + calls will renew leases on shares that already exist in the bucket. + """ + self._implicit_bucket_lease_renewal = enabled + + def set_implicit_slot_lease_renewal(self, enabled): + # type: (bool) -> None + """ + Control the behavior of implicit lease renewal by + *slot_testv_and_readv_and_writev*. + + :param enabled: If and only if ``True`` then future + *slot_testv_and_readv_and_writev* calls will renew leases on + shares that still exist in the slot after the writev is applied + and which were touched by the writev. + """ + self._implicit_slot_lease_renewal = enabled + def have_shares(self): # quick test to decide if we need to commit to an implicit # permutation-seed or if we should use a new one @@ -319,8 +356,9 @@ class StorageServer(service.MultiService, Referenceable): # file, they'll want us to hold leases for this file. for (shnum, fn) in self._get_bucket_shares(storage_index): alreadygot.add(shnum) - sf = ShareFile(fn) - sf.add_or_renew_lease(lease_info) + if self._implicit_bucket_lease_renewal: + sf = ShareFile(fn) + sf.add_or_renew_lease(lease_info) for shnum in sharenums: incominghome = os.path.join(self.incomingdir, si_dir, "%d" % shnum) @@ -625,15 +663,10 @@ class StorageServer(service.MultiService, Referenceable): secrets, test_and_write_vectors, read_vector, - renew_leases, ): """ Read data from shares and conditionally write some data to them. - :param bool renew_leases: If and only if this is ``True`` and the test - vectors pass then shares in this slot will also have an updated - lease applied to them. - See ``allmydata.interfaces.RIStorageServer`` for details about other parameters and return value. """ @@ -673,7 +706,7 @@ class StorageServer(service.MultiService, Referenceable): test_and_write_vectors, shares, ) - if renew_leases: + if self._implicit_slot_lease_renewal: lease_info = self._make_lease_info(renew_secret, cancel_secret) self._add_or_renew_leases(remaining_shares, lease_info) @@ -690,7 +723,6 @@ class StorageServer(service.MultiService, Referenceable): secrets, test_and_write_vectors, read_vector, - renew_leases=True, ) def _allocate_slot_share(self, bucketdir, secrets, sharenum, diff --git a/src/allmydata/test/test_storage.py b/src/allmydata/test/test_storage.py index 460653bd0..efa889f8d 100644 --- a/src/allmydata/test/test_storage.py +++ b/src/allmydata/test/test_storage.py @@ -608,6 +608,61 @@ class Server(unittest.TestCase): for i,wb in writers.items(): wb.remote_abort() + def test_allocate_without_lease_renewal(self): + """ + ``remote_allocate_buckets`` does not renew leases on existing shares if + ``set_implicit_bucket_lease_renewal(False)`` is called first. + """ + first_lease = 456 + second_lease = 543 + storage_index = b"allocate" + + clock = Clock() + clock.advance(first_lease) + ss = self.create( + "test_allocate_without_lease_renewal", + get_current_time=clock.seconds, + ) + ss.set_implicit_bucket_lease_renewal(False) + + # Put a share on there + already, writers = self.allocate(ss, storage_index, [0], 1) + (writer,) = writers.values() + writer.remote_write(0, b"x") + writer.remote_close() + + # It should have a lease granted at the current time. + shares = dict(ss._get_bucket_shares(storage_index)) + self.assertEqual( + [first_lease], + list( + lease.get_grant_renew_time_time() + for lease + in ShareFile(shares[0]).get_leases() + ), + ) + + # Let some time pass so we can tell if the lease on share 0 is + # renewed. + clock.advance(second_lease) + + # Put another share on there. + already, writers = self.allocate(ss, storage_index, [1], 1) + (writer,) = writers.values() + writer.remote_write(0, b"x") + writer.remote_close() + + # The first share's lease expiration time is unchanged. + shares = dict(ss._get_bucket_shares(storage_index)) + self.assertEqual( + [first_lease], + list( + lease.get_grant_renew_time_time() + for lease + in ShareFile(shares[0]).get_leases() + ), + ) + def test_bad_container_version(self): ss = self.create("test_bad_container_version") a,w = self.allocate(ss, b"si1", [0], 10) @@ -1408,9 +1463,10 @@ class MutableServer(unittest.TestCase): def test_writev_without_renew_lease(self): """ The helper method ``slot_testv_and_readv_and_writev`` does not renew - leases if ``False`` is passed for the ``renew_leases`` parameter. + leases if ``set_implicit_bucket_lease_renewal(False)`` is called first. """ ss = self.create("test_writev_without_renew_lease") + ss.set_implicit_slot_lease_renewal(False) storage_index = b"si2" secrets = ( @@ -1429,7 +1485,6 @@ class MutableServer(unittest.TestCase): sharenum: ([], datav, None), }, read_vector=[], - renew_leases=False, ) leases = list(ss.get_slot_leases(storage_index)) self.assertEqual([], leases) From 2742de6f7c1fa6cf77e35ecc5854bcf7db3e5963 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Wed, 10 Nov 2021 16:08:53 -0500 Subject: [PATCH 3/9] drop some ancient cruft allocated_size not used anywhere, so why have it --- src/allmydata/storage/server.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/allmydata/storage/server.py b/src/allmydata/storage/server.py index d142646a8..36cf06d0e 100644 --- a/src/allmydata/storage/server.py +++ b/src/allmydata/storage/server.py @@ -617,10 +617,8 @@ class StorageServer(service.MultiService, Referenceable): else: if sharenum not in shares: # allocate a new share - allocated_size = 2000 # arbitrary, really share = self._allocate_slot_share(bucketdir, secrets, sharenum, - allocated_size, owner_num=0) shares[sharenum] = share shares[sharenum].writev(datav, new_length) @@ -726,7 +724,7 @@ class StorageServer(service.MultiService, Referenceable): ) def _allocate_slot_share(self, bucketdir, secrets, sharenum, - allocated_size, owner_num=0): + owner_num=0): (write_enabler, renew_secret, cancel_secret) = secrets my_nodeid = self.my_nodeid fileutil.make_dirs(bucketdir) From c3cb0ebaeaa196c24272ac1fd834ed3c30baa377 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 12 Nov 2021 16:20:27 -0500 Subject: [PATCH 4/9] Switch to per-call parameter for controlling lease renewal behavior This is closer to an implementation where you could have two frontends, say a Foolscap frontend and an HTTP frontend or even just two different HTTP frontends, which had different opinions about what the behaviour should be. --- src/allmydata/storage/server.py | 51 ++++++------------------ src/allmydata/test/test_storage.py | 63 +++++++++++++++++++++++------- 2 files changed, 61 insertions(+), 53 deletions(-) diff --git a/src/allmydata/storage/server.py b/src/allmydata/storage/server.py index 36cf06d0e..70d71f841 100644 --- a/src/allmydata/storage/server.py +++ b/src/allmydata/storage/server.py @@ -59,21 +59,10 @@ DEFAULT_RENEWAL_TIME = 31 * 24 * 60 * 60 class StorageServer(service.MultiService, Referenceable): """ A filesystem-based implementation of ``RIStorageServer``. - - :ivar bool _implicit_bucket_lease_renewal: If and only if this is ``True`` - then ``allocate_buckets`` will renew leases on existing shares - associated with the storage index it operates on. - - :ivar bool _implicit_slot_lease_renewal: If and only if this is ``True`` - then ``slot_testv_and_readv_and_writev`` will renew leases on shares - associated with the slot it operates on. """ name = 'storage' LeaseCheckerClass = LeaseCheckingCrawler - _implicit_bucket_lease_renewal = True - _implicit_slot_lease_renewal = True - def __init__(self, storedir, nodeid, reserved_space=0, discard_storage=False, readonly_storage=False, stats_provider=None, @@ -149,29 +138,6 @@ class StorageServer(service.MultiService, Referenceable): def __repr__(self): return "" % (idlib.shortnodeid_b2a(self.my_nodeid),) - def set_implicit_bucket_lease_renewal(self, enabled): - # type: (bool) -> None - """ - Control the behavior of implicit lease renewal by *allocate_buckets*. - - :param enabled: If and only if ``True`` then future *allocate_buckets* - calls will renew leases on shares that already exist in the bucket. - """ - self._implicit_bucket_lease_renewal = enabled - - def set_implicit_slot_lease_renewal(self, enabled): - # type: (bool) -> None - """ - Control the behavior of implicit lease renewal by - *slot_testv_and_readv_and_writev*. - - :param enabled: If and only if ``True`` then future - *slot_testv_and_readv_and_writev* calls will renew leases on - shares that still exist in the slot after the writev is applied - and which were touched by the writev. - """ - self._implicit_slot_lease_renewal = enabled - def have_shares(self): # quick test to decide if we need to commit to an implicit # permutation-seed or if we should use a new one @@ -314,9 +280,12 @@ class StorageServer(service.MultiService, Referenceable): def _allocate_buckets(self, storage_index, renew_secret, cancel_secret, sharenums, allocated_size, - owner_num=0): + owner_num=0, renew_leases=True): """ Generic bucket allocation API. + + :param bool renew_leases: If and only if this is ``True`` then + renew leases on existing shares in this bucket. """ # owner_num is not for clients to set, but rather it should be # curried into the PersonalStorageServer instance that is dedicated @@ -356,7 +325,7 @@ class StorageServer(service.MultiService, Referenceable): # file, they'll want us to hold leases for this file. for (shnum, fn) in self._get_bucket_shares(storage_index): alreadygot.add(shnum) - if self._implicit_bucket_lease_renewal: + if renew_leases: sf = ShareFile(fn) sf.add_or_renew_lease(lease_info) @@ -399,7 +368,7 @@ class StorageServer(service.MultiService, Referenceable): """Foolscap-specific ``allocate_buckets()`` API.""" alreadygot, bucketwriters = self._allocate_buckets( storage_index, renew_secret, cancel_secret, sharenums, allocated_size, - owner_num=owner_num, + owner_num=owner_num, renew_leases=True, ) # Abort BucketWriters if disconnection happens. for bw in bucketwriters.values(): @@ -661,12 +630,17 @@ class StorageServer(service.MultiService, Referenceable): secrets, test_and_write_vectors, read_vector, + renew_leases, ): """ Read data from shares and conditionally write some data to them. See ``allmydata.interfaces.RIStorageServer`` for details about other parameters and return value. + + :param bool renew_leases: If and only if this is ``True`` then renew + leases on all shares mentioned in ``test_and_write_vectors` that + still exist after the changes are made. """ start = self._get_current_time() self.count("writev") @@ -704,7 +678,7 @@ class StorageServer(service.MultiService, Referenceable): test_and_write_vectors, shares, ) - if self._implicit_slot_lease_renewal: + if renew_leases: lease_info = self._make_lease_info(renew_secret, cancel_secret) self._add_or_renew_leases(remaining_shares, lease_info) @@ -721,6 +695,7 @@ class StorageServer(service.MultiService, Referenceable): secrets, test_and_write_vectors, read_vector, + renew_leases=True, ) def _allocate_slot_share(self, bucketdir, secrets, sharenum, diff --git a/src/allmydata/test/test_storage.py b/src/allmydata/test/test_storage.py index efa889f8d..a6c1ac2c2 100644 --- a/src/allmydata/test/test_storage.py +++ b/src/allmydata/test/test_storage.py @@ -468,14 +468,19 @@ class Server(unittest.TestCase): sv1 = ver[b'http://allmydata.org/tahoe/protocols/storage/v1'] self.failUnlessIn(b'available-space', sv1) - def allocate(self, ss, storage_index, sharenums, size, canary=None): + def allocate(self, ss, storage_index, sharenums, size, renew_leases=True): + """ + Call directly into the storage server's allocate_buckets implementation, + skipping the Foolscap layer. + """ renew_secret = hashutil.my_renewal_secret_hash(b"%d" % next(self._lease_secret)) cancel_secret = hashutil.my_cancel_secret_hash(b"%d" % next(self._lease_secret)) - if not canary: - canary = FakeCanary() - return ss.remote_allocate_buckets(storage_index, - renew_secret, cancel_secret, - sharenums, size, canary) + return ss._allocate_buckets( + storage_index, + renew_secret, cancel_secret, + sharenums, size, + renew_leases=renew_leases, + ) def test_large_share(self): syslow = platform.system().lower() @@ -611,7 +616,7 @@ class Server(unittest.TestCase): def test_allocate_without_lease_renewal(self): """ ``remote_allocate_buckets`` does not renew leases on existing shares if - ``set_implicit_bucket_lease_renewal(False)`` is called first. + ``renew_leases`` is ``False``. """ first_lease = 456 second_lease = 543 @@ -623,10 +628,11 @@ class Server(unittest.TestCase): "test_allocate_without_lease_renewal", get_current_time=clock.seconds, ) - ss.set_implicit_bucket_lease_renewal(False) # Put a share on there - already, writers = self.allocate(ss, storage_index, [0], 1) + already, writers = self.allocate( + ss, storage_index, [0], 1, renew_leases=False, + ) (writer,) = writers.values() writer.remote_write(0, b"x") writer.remote_close() @@ -647,7 +653,9 @@ class Server(unittest.TestCase): clock.advance(second_lease) # Put another share on there. - already, writers = self.allocate(ss, storage_index, [1], 1) + already, writers = self.allocate( + ss, storage_index, [1], 1, renew_leases=False, + ) (writer,) = writers.values() writer.remote_write(0, b"x") writer.remote_close() @@ -684,8 +692,17 @@ class Server(unittest.TestCase): def test_disconnect(self): # simulate a disconnection ss = self.create("test_disconnect") + renew_secret = b"r" * 32 + cancel_secret = b"c" * 32 canary = FakeCanary() - already,writers = self.allocate(ss, b"disconnect", [0,1,2], 75, canary) + already,writers = ss.remote_allocate_buckets( + b"disconnect", + renew_secret, + cancel_secret, + sharenums=[0,1,2], + allocated_size=75, + canary=canary, + ) self.failUnlessEqual(already, set()) self.failUnlessEqual(set(writers.keys()), set([0,1,2])) for (f,args,kwargs) in list(canary.disconnectors.values()): @@ -717,8 +734,17 @@ class Server(unittest.TestCase): # the size we request. OVERHEAD = 3*4 LEASE_SIZE = 4+32+32+4 + renew_secret = b"r" * 32 + cancel_secret = b"c" * 32 canary = FakeCanary() - already, writers = self.allocate(ss, b"vid1", [0,1,2], 1000, canary) + already, writers = ss.remote_allocate_buckets( + b"vid1", + renew_secret, + cancel_secret, + sharenums=[0,1,2], + allocated_size=1000, + canary=canary, + ) self.failUnlessEqual(len(writers), 3) # now the StorageServer should have 3000 bytes provisionally # allocated, allowing only 2000 more to be claimed @@ -751,7 +777,14 @@ class Server(unittest.TestCase): # now there should be ALLOCATED=1001+12+72=1085 bytes allocated, and # 5000-1085=3915 free, therefore we can fit 39 100byte shares canary3 = FakeCanary() - already3, writers3 = self.allocate(ss, b"vid3", list(range(100)), 100, canary3) + already3, writers3 = ss.remote_allocate_buckets( + b"vid3", + renew_secret, + cancel_secret, + sharenums=list(range(100)), + allocated_size=100, + canary=canary3, + ) self.failUnlessEqual(len(writers3), 39) self.failUnlessEqual(len(ss._bucket_writers), 39) @@ -1463,10 +1496,9 @@ class MutableServer(unittest.TestCase): def test_writev_without_renew_lease(self): """ The helper method ``slot_testv_and_readv_and_writev`` does not renew - leases if ``set_implicit_bucket_lease_renewal(False)`` is called first. + leases if ``renew_leases```` is ``False``. """ ss = self.create("test_writev_without_renew_lease") - ss.set_implicit_slot_lease_renewal(False) storage_index = b"si2" secrets = ( @@ -1485,6 +1517,7 @@ class MutableServer(unittest.TestCase): sharenum: ([], datav, None), }, read_vector=[], + renew_leases=False, ) leases = list(ss.get_slot_leases(storage_index)) self.assertEqual([], leases) From 85977e48a7dde8ea29e196a6d466ae8685c2f6fc Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 12 Nov 2021 16:23:15 -0500 Subject: [PATCH 5/9] put this comment back and merge info from the two versions --- src/allmydata/storage/server.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/allmydata/storage/server.py b/src/allmydata/storage/server.py index 70d71f841..9b73963ae 100644 --- a/src/allmydata/storage/server.py +++ b/src/allmydata/storage/server.py @@ -635,12 +635,13 @@ class StorageServer(service.MultiService, Referenceable): """ Read data from shares and conditionally write some data to them. + :param bool renew_leases: If and only if this is ``True`` and the test + vectors pass then shares mentioned in ``test_and_write_vectors`` + that still exist after the changes are made will also have an + updated lease applied to them. + See ``allmydata.interfaces.RIStorageServer`` for details about other parameters and return value. - - :param bool renew_leases: If and only if this is ``True`` then renew - leases on all shares mentioned in ``test_and_write_vectors` that - still exist after the changes are made. """ start = self._get_current_time() self.count("writev") From dece67ee3ac8d2bd06b42e07a01492e3c4497ae6 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 12 Nov 2021 16:24:29 -0500 Subject: [PATCH 6/9] it is not the remote interface that varies anymore --- src/allmydata/test/test_storage.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/allmydata/test/test_storage.py b/src/allmydata/test/test_storage.py index a6c1ac2c2..076e9f3d1 100644 --- a/src/allmydata/test/test_storage.py +++ b/src/allmydata/test/test_storage.py @@ -615,8 +615,8 @@ class Server(unittest.TestCase): def test_allocate_without_lease_renewal(self): """ - ``remote_allocate_buckets`` does not renew leases on existing shares if - ``renew_leases`` is ``False``. + ``StorageServer._allocate_buckets`` does not renew leases on existing + shares if ``renew_leases`` is ``False``. """ first_lease = 456 second_lease = 543 From 6c2e85e99145652625ff7a4d6791a410ce13c742 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 12 Nov 2021 16:25:36 -0500 Subject: [PATCH 7/9] put the comment back --- src/allmydata/test/test_storage.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/allmydata/test/test_storage.py b/src/allmydata/test/test_storage.py index 076e9f3d1..4e40a76a5 100644 --- a/src/allmydata/test/test_storage.py +++ b/src/allmydata/test/test_storage.py @@ -1496,7 +1496,7 @@ class MutableServer(unittest.TestCase): def test_writev_without_renew_lease(self): """ The helper method ``slot_testv_and_readv_and_writev`` does not renew - leases if ``renew_leases```` is ``False``. + leases if ``False`` is passed for the ``renew_leases`` parameter. """ ss = self.create("test_writev_without_renew_lease") From ad6017e63df94dbac7916f4673332b33deb8d5be Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Mon, 15 Nov 2021 08:08:14 -0500 Subject: [PATCH 8/9] clarify renew_leases docs on allocate_buckets --- 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 9b73963ae..bfbc10b59 100644 --- a/src/allmydata/storage/server.py +++ b/src/allmydata/storage/server.py @@ -284,8 +284,10 @@ class StorageServer(service.MultiService, Referenceable): """ Generic bucket allocation API. - :param bool renew_leases: If and only if this is ``True`` then - renew leases on existing shares in this bucket. + :param bool renew_leases: If and only if this is ``True`` then renew a + secret-matching lease on (or, if none match, add a new lease to) + existing shares in this bucket. Any *new* shares are given a new + lease regardless. """ # owner_num is not for clients to set, but rather it should be # curried into the PersonalStorageServer instance that is dedicated From 84c19f5468b04279e1826ed77dc1e7d4b4ae00e8 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Mon, 15 Nov 2021 08:12:07 -0500 Subject: [PATCH 9/9] clarify renew_leases docs on slot_testv_and_readv_and_writev --- src/allmydata/storage/server.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/allmydata/storage/server.py b/src/allmydata/storage/server.py index bfbc10b59..ee2ea1c61 100644 --- a/src/allmydata/storage/server.py +++ b/src/allmydata/storage/server.py @@ -639,8 +639,9 @@ class StorageServer(service.MultiService, Referenceable): :param bool renew_leases: If and only if this is ``True`` and the test vectors pass then shares mentioned in ``test_and_write_vectors`` - that still exist after the changes are made will also have an - updated lease applied to them. + that still exist after the changes are made will also have a + secret-matching lease renewed (or, if none match, a new lease + added). See ``allmydata.interfaces.RIStorageServer`` for details about other parameters and return value.