Merge pull request #1110 from tahoe-lafs/3773.just-add-lease

Get rid of renew_lease client code, in order to simplify the protocol

Fixes ticket:3773

docs/proposed/http-storage-node-protocol.rst

@@ -369,7 +369,7 @@ For example::
 ``PUT /v1/lease/:storage_index``
 !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
 
-Create a new lease on the bucket addressed by ``storage_index``.
+Either renew or create a new lease on the bucket addressed by ``storage_index``.
 The details of the lease are encoded in the request body.
 For example::
 
@@ -400,37 +400,11 @@ Several behaviors here are blindly copied from the Foolscap-based storage server
 
 * There is a cancel secret but there is no API to use it to cancel a lease (see ticket:3768).
 * The lease period is hard-coded at 31 days.
-* There are separate **add** and **renew** lease APIs (see ticket:3773).
 
 These are not necessarily ideal behaviors
 but they are adopted to avoid any *semantic* changes between the Foolscap- and HTTP-based protocols.
 It is expected that some or all of these behaviors may change in a future revision of the HTTP-based protocol.
 
-``POST /v1/lease/:storage_index``
-!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
-
-Renew an existing lease for all shares for the given storage index.
-The details of the lease are encoded in the request body.
-For example::
-
-  {"renew-secret": "abcd"}
-
-If there are no shares for the given ``storage_index``
-then ``NOT FOUND`` is returned.
-
-If there is no lease with a matching ``renew-secret`` value on the given storage index
-then ``NOT FOUND`` is returned.
-In this case,
-if the storage index refers to mutable data
-then the response also includes a list of nodeids where the lease can be renewed.
-For example::
-
-  {"nodeids": ["aaa...", "bbb..."]}
-
-Othewise,
-the matching lease's expiration time is changed to be 31 days from the time of this operation
-and ``NO CONTENT`` is returned.
-
 Immutable
 ---------
 
@@ -676,8 +650,8 @@ Immutable Data
 
 #. Renew the lease on all immutable shares in bucket ``AAAAAAAAAAAAAAAA``::
 
-     POST /v1/lease/AAAAAAAAAAAAAAAA
+     PUT /v1/lease/AAAAAAAAAAAAAAAA
-     {"renew-secret": "efgh"}
+     {"renew-secret": "efgh", "cancel-secret": "ijkl"}
 
      204 NO CONTENT
 
@@ -757,8 +731,8 @@ Mutable Data
 
 #. Renew the lease on previously uploaded mutable share in slot ``BBBBBBBBBBBBBBBB``::
 
-     POST /v1/lease/BBBBBBBBBBBBBBBB
+     PUT /v1/lease/BBBBBBBBBBBBBBBB
-     {"renew-secret": "efgh"}
+     {"renew-secret": "efgh", "cancel-secret": "ijkl"}
 
      204 NO CONTENT
 

src/allmydata/interfaces.py

@@ -154,25 +154,9 @@ class RIStorageServer(RemoteInterface):
         """
         return Any() # returns None now, but future versions might change
 
-    def renew_lease(storage_index=StorageIndex, renew_secret=LeaseRenewSecret):
-        """
-        Renew the lease on a given bucket, resetting the timer to 31 days.
-        Some networks will use this, some will not. If there is no bucket for
-        the given storage_index, IndexError will be raised.
-
-        For mutable shares, if the given renew_secret does not match an
-        existing lease, IndexError will be raised with a note listing the
-        server-nodeids on the existing leases, so leases on migrated shares
-        can be renewed. For immutable shares, IndexError (without the note)
-        will be raised.
-        """
-        return Any()
-
     def get_buckets(storage_index=StorageIndex):
         return DictOf(int, RIBucketReader, maxKeys=MAX_BUCKETS)
 
-
-
     def slot_readv(storage_index=StorageIndex,
                    shares=ListOf(int), readv=ReadVector):
         """Read a vector from the numbered shares associated with the given
@@ -343,14 +327,6 @@ class IStorageServer(Interface):
         :see: ``RIStorageServer.add_lease``
         """
 
-    def renew_lease(
-            storage_index,
-            renew_secret,
-    ):
-        """
-        :see: ``RIStorageServer.renew_lease``
-        """
-
     def get_buckets(
             storage_index,
     ):

src/allmydata/storage/server.py

@@ -49,6 +49,10 @@ from allmydata.storage.expirer import LeaseCheckingCrawler
 NUM_RE=re.compile("^[0-9]+$")
 
 
+# Number of seconds to add to expiration time on lease renewal.
+# For now it's not actually configurable, but maybe someday.
+DEFAULT_RENEWAL_TIME = 31 * 24 * 60 * 60
+
 
 @implementer(RIStorageServer, IStatsProducer)
 class StorageServer(service.MultiService, Referenceable):
@@ -62,7 +66,8 @@ class StorageServer(service.MultiService, Referenceable):
                  expiration_mode="age",
                  expiration_override_lease_duration=None,
                  expiration_cutoff_date=None,
-                 expiration_sharetypes=("mutable", "immutable")):
+                 expiration_sharetypes=("mutable", "immutable"),
+                 get_current_time=time.time):
         service.MultiService.__init__(self)
         assert isinstance(nodeid, bytes)
         assert len(nodeid) == 20
@@ -114,6 +119,7 @@ class StorageServer(service.MultiService, Referenceable):
                                    expiration_cutoff_date,
                                    expiration_sharetypes)
         self.lease_checker.setServiceParent(self)
+        self._get_current_time = get_current_time
 
     def __repr__(self):
         return "<StorageServer %s>" % (idlib.shortnodeid_b2a(self.my_nodeid),)
@@ -264,7 +270,7 @@ class StorageServer(service.MultiService, Referenceable):
         # owner_num is not for clients to set, but rather it should be
         # curried into the PersonalStorageServer instance that is dedicated
         # to a particular owner.
-        start = time.time()
+        start = self._get_current_time()
         self.count("allocate")
         alreadygot = set()
         bucketwriters = {} # k: shnum, v: BucketWriter
@@ -277,7 +283,7 @@ class StorageServer(service.MultiService, Referenceable):
         # goes into the share files themselves. It could also be put into a
         # separate database. Note that the lease should not be added until
         # the BucketWriter has been closed.
-        expire_time = time.time() + 31*24*60*60
+        expire_time = self._get_current_time() + DEFAULT_RENEWAL_TIME
         lease_info = LeaseInfo(owner_num,
                                renew_secret, cancel_secret,
                                expire_time, self.my_nodeid)
@@ -331,7 +337,7 @@ class StorageServer(service.MultiService, Referenceable):
         if bucketwriters:
             fileutil.make_dirs(os.path.join(self.sharedir, si_dir))
 
-        self.add_latency("allocate", time.time() - start)
+        self.add_latency("allocate", self._get_current_time() - start)
         return alreadygot, bucketwriters
 
     def _iter_share_files(self, storage_index):
@@ -351,26 +357,26 @@ class StorageServer(service.MultiService, Referenceable):
 
     def remote_add_lease(self, storage_index, renew_secret, cancel_secret,
                          owner_num=1):
-        start = time.time()
+        start = self._get_current_time()
         self.count("add-lease")
-        new_expire_time = time.time() + 31*24*60*60
+        new_expire_time = self._get_current_time() + DEFAULT_RENEWAL_TIME
         lease_info = LeaseInfo(owner_num,
                                renew_secret, cancel_secret,
                                new_expire_time, self.my_nodeid)
         for sf in self._iter_share_files(storage_index):
             sf.add_or_renew_lease(lease_info)
-        self.add_latency("add-lease", time.time() - start)
+        self.add_latency("add-lease", self._get_current_time() - start)
         return None
 
     def remote_renew_lease(self, storage_index, renew_secret):
-        start = time.time()
+        start = self._get_current_time()
         self.count("renew")
-        new_expire_time = time.time() + 31*24*60*60
+        new_expire_time = self._get_current_time() + DEFAULT_RENEWAL_TIME
         found_buckets = False
         for sf in self._iter_share_files(storage_index):
             found_buckets = True
             sf.renew_lease(renew_secret, new_expire_time)
-        self.add_latency("renew", time.time() - start)
+        self.add_latency("renew", self._get_current_time() - start)
         if not found_buckets:
             raise IndexError("no such lease to renew")
 
@@ -394,7 +400,7 @@ class StorageServer(service.MultiService, Referenceable):
             pass
 
     def remote_get_buckets(self, storage_index):
-        start = time.time()
+        start = self._get_current_time()
         self.count("get")
         si_s = si_b2a(storage_index)
         log.msg("storage: get_buckets %r" % si_s)
@@ -402,7 +408,7 @@ class StorageServer(service.MultiService, Referenceable):
         for shnum, filename in self._get_bucket_shares(storage_index):
             bucketreaders[shnum] = BucketReader(self, filename,
                                                 storage_index, shnum)
-        self.add_latency("get", time.time() - start)
+        self.add_latency("get", self._get_current_time() - start)
         return bucketreaders
 
     def get_leases(self, storage_index):
@@ -563,7 +569,7 @@ class StorageServer(service.MultiService, Referenceable):
         :return LeaseInfo: Information for a new lease for a share.
         """
         ownerid = 1 # TODO
-        expire_time = time.time() + 31*24*60*60   # one month
+        expire_time = self._get_current_time() + DEFAULT_RENEWAL_TIME
         lease_info = LeaseInfo(ownerid,
                                renew_secret, cancel_secret,
                                expire_time, self.my_nodeid)
@@ -599,7 +605,7 @@ class StorageServer(service.MultiService, Referenceable):
         See ``allmydata.interfaces.RIStorageServer`` for details about other
         parameters and return value.
         """
-        start = time.time()
+        start = self._get_current_time()
         self.count("writev")
         si_s = si_b2a(storage_index)
         log.msg("storage: slot_writev %r" % si_s)
@@ -640,7 +646,7 @@ class StorageServer(service.MultiService, Referenceable):
                 self._add_or_renew_leases(remaining_shares, lease_info)
 
         # all done
-        self.add_latency("writev", time.time() - start)
+        self.add_latency("writev", self._get_current_time() - start)
         return (testv_is_good, read_data)
 
     def remote_slot_testv_and_readv_and_writev(self, storage_index,
@@ -666,7 +672,7 @@ class StorageServer(service.MultiService, Referenceable):
         return share
 
     def remote_slot_readv(self, storage_index, shares, readv):
-        start = time.time()
+        start = self._get_current_time()
         self.count("readv")
         si_s = si_b2a(storage_index)
         lp = log.msg("storage: slot_readv %r %r" % (si_s, shares),
@@ -675,7 +681,7 @@ class StorageServer(service.MultiService, Referenceable):
         # shares exist if there is a file for them
         bucketdir = os.path.join(self.sharedir, si_dir)
         if not os.path.isdir(bucketdir):
-            self.add_latency("readv", time.time() - start)
+            self.add_latency("readv", self._get_current_time() - start)
             return {}
         datavs = {}
         for sharenum_s in os.listdir(bucketdir):
@@ -689,7 +695,7 @@ class StorageServer(service.MultiService, Referenceable):
                 datavs[sharenum] = msf.readv(readv)
         log.msg("returning shares %s" % (list(datavs.keys()),),
                 facility="tahoe.storage", level=log.NOISY, parent=lp)
-        self.add_latency("readv", time.time() - start)
+        self.add_latency("readv", self._get_current_time() - start)
         return datavs
 
     def remote_advise_corrupt_share(self, share_type, storage_index, shnum,

src/allmydata/storage_client.py

@@ -965,17 +965,6 @@ class _StorageServer(object):
             cancel_secret,
         )
 
-    def renew_lease(
-            self,
-            storage_index,
-            renew_secret,
-    ):
-        return self._rref.callRemote(
-            "renew_lease",
-            storage_index,
-            renew_secret,
-        )
-
     def get_buckets(
             self,
             storage_index,

src/allmydata/test/test_storage.py

@@ -24,11 +24,12 @@ import gc
 from twisted.trial import unittest
 
 from twisted.internet import defer
+from twisted.internet.task import Clock
 
 import itertools
 from allmydata import interfaces
 from allmydata.util import fileutil, hashutil, base32
-from allmydata.storage.server import StorageServer
+from allmydata.storage.server import StorageServer, DEFAULT_RENEWAL_TIME
 from allmydata.storage.shares import get_share_file
 from allmydata.storage.mutable import MutableShareFile
 from allmydata.storage.immutable import BucketWriter, BucketReader, ShareFile
@@ -168,7 +169,7 @@ class Bucket(unittest.TestCase):
         assert len(renewsecret) == 32
         cancelsecret = b'THIS LETS ME KILL YOUR FILE HAHA'
         assert len(cancelsecret) == 32
-        expirationtime = struct.pack('>L', 60*60*24*31) # 31 days in seconds
+        expirationtime = struct.pack('>L', DEFAULT_RENEWAL_TIME) # 31 days in seconds
 
         lease_data = ownernumber + renewsecret + cancelsecret + expirationtime
 
@@ -354,10 +355,11 @@ class Server(unittest.TestCase):
         basedir = os.path.join("storage", "Server", name)
         return basedir
 
-    def create(self, name, reserved_space=0, klass=StorageServer):
+    def create(self, name, reserved_space=0, klass=StorageServer, get_current_time=time.time):
         workdir = self.workdir(name)
         ss = klass(workdir, b"\x00" * 20, reserved_space=reserved_space,
-                   stats_provider=FakeStatsProvider())
+                   stats_provider=FakeStatsProvider(),
+                   get_current_time=get_current_time)
         ss.setServiceParent(self.sparent)
         return ss
 
@@ -384,8 +386,8 @@ class Server(unittest.TestCase):
         self.failUnlessIn(b'available-space', sv1)
 
     def allocate(self, ss, storage_index, sharenums, size, canary=None):
-        renew_secret = hashutil.tagged_hash(b"blah", b"%d" % next(self._lease_secret))
+        renew_secret = hashutil.my_renewal_secret_hash(b"%d" % next(self._lease_secret))
-        cancel_secret = hashutil.tagged_hash(b"blah", 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,
@@ -646,6 +648,27 @@ class Server(unittest.TestCase):
         f2 = open(filename, "rb")
         self.failUnlessEqual(f2.read(5), b"start")
 
+    def create_bucket_5_shares(
+            self, ss, storage_index, expected_already=0, expected_writers=5
+    ):
+        """
+        Given a StorageServer, create a bucket with 5 shares and return renewal
+        and cancellation secrets.
+        """
+        canary = FakeCanary()
+        sharenums = list(range(5))
+        size = 100
+
+        # Creating a bucket also creates a lease:
+        rs, cs  = (hashutil.my_renewal_secret_hash(b"%d" % next(self._lease_secret)),
+                   hashutil.my_cancel_secret_hash(b"%d" % next(self._lease_secret)))
+        already, writers = ss.remote_allocate_buckets(storage_index, rs, cs,
+                                                      sharenums, size, canary)
+        self.failUnlessEqual(len(already), expected_already)
+        self.failUnlessEqual(len(writers), expected_writers)
+        for wb in writers.values():
+            wb.remote_close()
+        return rs, cs
 
     def test_leases(self):
         ss = self.create("test_leases")
@@ -653,41 +676,23 @@ class Server(unittest.TestCase):
         sharenums = list(range(5))
         size = 100
 
-        rs0,cs0 = (hashutil.tagged_hash(b"blah", b"%d" % next(self._lease_secret)),
+        # Create a bucket:
-                   hashutil.tagged_hash(b"blah", b"%d" % next(self._lease_secret)))
+        rs0, cs0 = self.create_bucket_5_shares(ss, b"si0")
-        already,writers = ss.remote_allocate_buckets(b"si0", rs0, cs0,
-                                                     sharenums, size, canary)
-        self.failUnlessEqual(len(already), 0)
-        self.failUnlessEqual(len(writers), 5)
-        for wb in writers.values():
-            wb.remote_close()
-
         leases = list(ss.get_leases(b"si0"))
         self.failUnlessEqual(len(leases), 1)
         self.failUnlessEqual(set([l.renew_secret for l in leases]), set([rs0]))
 
-        rs1,cs1 = (hashutil.tagged_hash(b"blah", b"%d" % next(self._lease_secret)),
+        rs1, cs1 = self.create_bucket_5_shares(ss, b"si1")
-                   hashutil.tagged_hash(b"blah", b"%d" % next(self._lease_secret)))
-        already,writers = ss.remote_allocate_buckets(b"si1", rs1, cs1,
-                                                     sharenums, size, canary)
-        for wb in writers.values():
-            wb.remote_close()
 
         # take out a second lease on si1
-        rs2,cs2 = (hashutil.tagged_hash(b"blah", b"%d" % next(self._lease_secret)),
+        rs2, cs2 = self.create_bucket_5_shares(ss, b"si1", 5, 0)
-                   hashutil.tagged_hash(b"blah", b"%d" % next(self._lease_secret)))
-        already,writers = ss.remote_allocate_buckets(b"si1", rs2, cs2,
-                                                     sharenums, size, canary)
-        self.failUnlessEqual(len(already), 5)
-        self.failUnlessEqual(len(writers), 0)
-
         leases = list(ss.get_leases(b"si1"))
         self.failUnlessEqual(len(leases), 2)
         self.failUnlessEqual(set([l.renew_secret for l in leases]), set([rs1, rs2]))
 
         # and a third lease, using add-lease
-        rs2a,cs2a = (hashutil.tagged_hash(b"blah", b"%d" % next(self._lease_secret)),
+        rs2a,cs2a = (hashutil.my_renewal_secret_hash(b"%d" % next(self._lease_secret)),
-                     hashutil.tagged_hash(b"blah", b"%d" % next(self._lease_secret)))
+                     hashutil.my_cancel_secret_hash(b"%d" % next(self._lease_secret)))
         ss.remote_add_lease(b"si1", rs2a, cs2a)
         leases = list(ss.get_leases(b"si1"))
         self.failUnlessEqual(len(leases), 3)
@@ -715,10 +720,10 @@ class Server(unittest.TestCase):
                         "ss should not have a 'remote_cancel_lease' method/attribute")
 
         # test overlapping uploads
-        rs3,cs3 = (hashutil.tagged_hash(b"blah", b"%d" % next(self._lease_secret)),
+        rs3,cs3 = (hashutil.my_renewal_secret_hash(b"%d" % next(self._lease_secret)),
-                   hashutil.tagged_hash(b"blah", b"%d" % next(self._lease_secret)))
+                   hashutil.my_cancel_secret_hash(b"%d" % next(self._lease_secret)))
-        rs4,cs4 = (hashutil.tagged_hash(b"blah", b"%d" % next(self._lease_secret)),
+        rs4,cs4 = (hashutil.my_renewal_secret_hash(b"%d" % next(self._lease_secret)),
-                   hashutil.tagged_hash(b"blah", b"%d" % next(self._lease_secret)))
+                   hashutil.my_cancel_secret_hash(b"%d" % next(self._lease_secret)))
         already,writers = ss.remote_allocate_buckets(b"si3", rs3, cs3,
                                                      sharenums, size, canary)
         self.failUnlessEqual(len(already), 0)
@@ -741,6 +746,28 @@ class Server(unittest.TestCase):
         leases = list(ss.get_leases(b"si3"))
         self.failUnlessEqual(len(leases), 2)
 
+    def test_immutable_add_lease_renews(self):
+        """
+        Adding a lease on an already leased immutable with the same secret just
+        renews it.
+        """
+        clock = Clock()
+        clock.advance(123)
+        ss = self.create("test_immutable_add_lease_renews", get_current_time=clock.seconds)
+
+        # Start out with single lease created with bucket:
+        renewal_secret, cancel_secret = self.create_bucket_5_shares(ss, b"si0")
+        [lease] = ss.get_leases(b"si0")
+        self.assertEqual(lease.expiration_time, 123 + DEFAULT_RENEWAL_TIME)
+
+        # Time passes:
+        clock.advance(123456)
+
+        # Adding a lease with matching renewal secret just renews it:
+        ss.remote_add_lease(b"si0", renewal_secret, cancel_secret)
+        [lease] = ss.get_leases(b"si0")
+        self.assertEqual(lease.expiration_time, 123 + 123456 + DEFAULT_RENEWAL_TIME)
+
     def test_have_shares(self):
         """By default the StorageServer has no shares."""
         workdir = self.workdir("test_have_shares")
@@ -840,9 +867,10 @@ class MutableServer(unittest.TestCase):
         basedir = os.path.join("storage", "MutableServer", name)
         return basedir
 
-    def create(self, name):
+    def create(self, name, get_current_time=time.time):
         workdir = self.workdir(name)
-        ss = StorageServer(workdir, b"\x00" * 20)
+        ss = StorageServer(workdir, b"\x00" * 20,
+                           get_current_time=get_current_time)
         ss.setServiceParent(self.sparent)
         return ss
 
@@ -1379,6 +1407,41 @@ class MutableServer(unittest.TestCase):
               {0: ([], [(500, b"make me really bigger")], None)}, [])
         self.compare_leases_without_timestamps(all_leases, list(s0.get_leases()))
 
+    def test_mutable_add_lease_renews(self):
+        """
+        Adding a lease on an already leased mutable with the same secret just
+        renews it.
+        """
+        clock = Clock()
+        clock.advance(235)
+        ss = self.create("test_mutable_add_lease_renews",
+                         get_current_time=clock.seconds)
+        def secrets(n):
+            return ( self.write_enabler(b"we1"),
+                     self.renew_secret(b"we1-%d" % n),
+                     self.cancel_secret(b"we1-%d" % n) )
+        data = b"".join([ (b"%d" % i) * 10 for i in range(10) ])
+        write = ss.remote_slot_testv_and_readv_and_writev
+        write_enabler, renew_secret, cancel_secret = secrets(0)
+        rc = write(b"si1", (write_enabler, renew_secret, cancel_secret),
+                   {0: ([], [(0,data)], None)}, [])
+        self.failUnlessEqual(rc, (True, {}))
+
+        bucket_dir = os.path.join(self.workdir("test_mutable_add_lease_renews"),
+                                  "shares", storage_index_to_dir(b"si1"))
+        s0 = MutableShareFile(os.path.join(bucket_dir, "0"))
+        [lease] = s0.get_leases()
+        self.assertEqual(lease.expiration_time, 235 + DEFAULT_RENEWAL_TIME)
+
+        # Time passes...
+        clock.advance(835)
+
+        # Adding a lease renews it:
+        ss.remote_add_lease(b"si1", renew_secret, cancel_secret)
+        [lease] = s0.get_leases()
+        self.assertEqual(lease.expiration_time,
+                         235 + 835 + DEFAULT_RENEWAL_TIME)
+
     def test_remove(self):
         ss = self.create("test_remove")
         self.allocate(ss, b"si1", b"we1", next(self._lease_secret),