Changeset fadfbca in trunk

Timestamp:

2021-09-28T14:37:56Z (4 years ago)

Author:

GitHub <noreply@…>

Branches:

master

Children:

0a072a9, a02d5f4

Parents:

eb5b6c5 (diff), f66f3e6 (diff)
Note: this is a merge changeset, the changes displayed below correspond to the merge itself.
Use the (diff) links above to see all the changes relative to each parent.

git-author:

Itamar Turner-Trauring <itamar@…> (2021-09-28 14:37:56)

git-committer:

GitHub <noreply@…> (2021-09-28 14:37:56)

Message:

Merge pull request #1130 from tahoe-lafs/3801-no-overlapping-writes-immutable-upload

Disallow conflicting overlapping writes when doing an immutable upload.

Fixes ticket:3801

Files:

• docs/proposed/http-storage-node-protocol.rst (modified) (2 diffs)
• newsfragments/3801.bugfix (added)
• nix/collections-extended.nix (added)
• nix/overlays.nix (modified) (1 diff)
• nix/tahoe-lafs.nix (modified) (1 diff)
• setup.py (modified) (1 diff)
• src/allmydata/interfaces.py (modified) (1 diff)
• src/allmydata/storage/common.py (modified) (1 diff)
• src/allmydata/storage/immutable.py (modified) (3 diffs)
• src/allmydata/test/test_istorageserver.py (modified) (2 diffs)
• src/allmydata/test/test_storage.py (modified) (6 diffs)

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

@@ -498 +498 @@
 The server must recognize when all of the data has been received and mark the share as complete
 (which it can do because it was informed of the size when the storage index was initialized).
-Clients should upload chunks in re-assembly order.
 
 * When a chunk that does not complete the share is successfully uploaded the response is ``OK``.
-* When the chunk that completes the share is successfully uploaded the response is ``CREATED``.
-* If the *Content-Range* for a request covers part of the share that has already been uploaded the response is ``CONFLICT``.
   The response body indicates the range of share data that has yet to be uploaded.
   That is::
@@ -514 +511 @@
       ]
     }
+
+* When the chunk that completes the share is successfully uploaded the response is ``CREATED``.
+* If the *Content-Range* for a request covers part of the share that has already,
+  and the data does not match already written data,
+  the response is ``CONFLICT``.
+  At this point the only thing to do is abort the upload and start from scratch (see below).
+
+``PUT /v1/immutable/:storage_index/:share_number/abort``
+!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
+
+This cancels an *in-progress* upload.
+
+The response code:
+
+* When the upload is still in progress and therefore the abort has succeeded,
+  the response is ``OK``.
+  Future uploads can start from scratch with no pre-existing upload state stored on the server.
+* If the uploaded has already finished, the response is 405 (Method Not Allowed)
+  and no change is made.
 
 

nix/overlays.nix ¶

@@ -19 +19 @@
       # Need a newer version of Twisted, too.
       twisted = python-super.callPackage ./twisted.nix { };
+
+      # collections-extended is not part of nixpkgs at this time.
+      collections-extended = python-super.callPackage ./collections-extended.nix { };
     };
   };

nix/tahoe-lafs.nix ¶

@@ -98 +98 @@
     service-identity pyyaml magic-wormhole treq
     eliot autobahn cryptography netifaces setuptools
-    future pyutil distro configparser
+    future pyutil distro configparser collections-extended
   ];
 

setup.py ¶

@@ -138 +138 @@
     # Backported configparser for Python 2:
     "configparser ; python_version < '3.0'",
+
+    # For the RangeMap datastructure.
+    "collections-extended",
 ]
 

src/allmydata/interfaces.py ¶

@@ -52 +52 @@
 LeaseRenewSecret = Hash # used to protect lease renewal requests
 LeaseCancelSecret = Hash # was used to protect lease cancellation requests
+
+
+class DataTooLargeError(Exception):
+    """The write went past the expected size of the bucket."""
+
+
+class ConflictingWriteError(Exception):
+    """Two writes happened to same immutable with different data."""
 
 

src/allmydata/storage/common.py ¶

@@ -14 +14 @@
 from allmydata.util import base32
 
-class DataTooLargeError(Exception):
-    pass
+# Backwards compatibility.
+from allmydata.interfaces import DataTooLargeError  # noqa: F401
+
 class UnknownMutableContainerVersionError(Exception):
     pass

src/allmydata/storage/immutable.py ¶

@@ -14 +14 @@
 import os, stat, struct, time
 
+from collections_extended import RangeMap
+
 from foolscap.api import Referenceable
 
 from zope.interface import implementer
-from allmydata.interfaces import RIBucketWriter, RIBucketReader
+from allmydata.interfaces import (
+    RIBucketWriter, RIBucketReader, ConflictingWriteError,
+    DataTooLargeError,
+)
 from allmydata.util import base32, fileutil, log
 from allmydata.util.assertutil import precondition
 from allmydata.util.hashutil import timing_safe_compare
 from allmydata.storage.lease import LeaseInfo
-from allmydata.storage.common import UnknownImmutableContainerVersionError, \
-     DataTooLargeError
+from allmydata.storage.common import UnknownImmutableContainerVersionError
 
 # each share file (in storage/shares/$SI/$SHNUM) contains lease information
@@ -218 +222 @@
         # added by simultaneous uploaders
         self._sharefile.add_lease(lease_info)
+        self._already_written = RangeMap()
 
     def allocated_size(self):
@@ -227 +232 @@
         if self.throw_out_all_data:
             return
+
+        # Make sure we're not conflicting with existing data:
+        end = offset + len(data)
+        for (chunk_start, chunk_stop, _) in self._already_written.ranges(offset, end):
+            chunk_len = chunk_stop - chunk_start
+            actual_chunk = self._sharefile.read_share_data(chunk_start, chunk_len)
+            writing_chunk = data[chunk_start - offset:chunk_stop - offset]
+            if actual_chunk != writing_chunk:
+                raise ConflictingWriteError(
+                    "Chunk {}-{} doesn't match already written data.".format(chunk_start, chunk_stop)
+                )
         self._sharefile.write_share_data(offset, data)
+
+        self._already_written.set(True, offset, end)
         self.ss.add_latency("write", time.time() - start)
         self.ss.count("write")

src/allmydata/test/test_istorageserver.py ¶

@@ -25 +25 @@
 from twisted.internet.defer import inlineCallbacks
 
-from foolscap.api import Referenceable
+from foolscap.api import Referenceable, RemoteException
 
 from allmydata.interfaces import IStorageServer
@@ -249 +249 @@
         )
 
-    @skipIf(True, "https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3801")
-    def test_overlapping_writes(self):
-        """
-        The policy for overlapping writes is TBD:
-        https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3801
-        """
+    @inlineCallbacks
+    def test_non_matching_overlapping_writes(self):
+        """
+        When doing overlapping writes in immutable uploads, non-matching writes
+        fail.
+        """
+        storage_index, renew_secret, cancel_secret = (
+            new_storage_index(),
+            new_secret(),
+            new_secret(),
+        )
+        (_, allocated) = yield self.storage_server.allocate_buckets(
+            storage_index,
+            renew_secret,
+            cancel_secret,
+            sharenums={0},
+            allocated_size=30,
+            canary=Referenceable(),
+        )
+
+        yield allocated[0].callRemote("write", 0, b"1" * 25)
+        # Overlapping write that doesn't match:
+        with self.assertRaises(RemoteException):
+            yield allocated[0].callRemote("write", 20, b"2" * 10)
+
+    @inlineCallbacks
+    def test_matching_overlapping_writes(self):
+        """
+        When doing overlapping writes in immutable uploads, matching writes
+        succeed.
+        """
+        storage_index, renew_secret, cancel_secret = (
+            new_storage_index(),
+            new_secret(),
+            new_secret(),
+        )
+        (_, allocated) = yield self.storage_server.allocate_buckets(
+            storage_index,
+            renew_secret,
+            cancel_secret,
+            sharenums={0},
+            allocated_size=25,
+            canary=Referenceable(),
+        )
+
+        yield allocated[0].callRemote("write", 0, b"1" * 10)
+        # Overlapping write that matches:
+        yield allocated[0].callRemote("write", 5, b"1" * 20)
+        yield allocated[0].callRemote("close")
+
+        buckets = yield self.storage_server.get_buckets(storage_index)
+        self.assertEqual(set(buckets.keys()), {0})
+
+        self.assertEqual((yield buckets[0].callRemote("read", 0, 25)), b"1" * 25)
 
     @inlineCallbacks

src/allmydata/test/test_storage.py ¶

@@ -9 +9 @@
 from __future__ import unicode_literals
 
-from future.utils import native_str, PY2, bytes_to_native_str
+from future.utils import native_str, PY2, bytes_to_native_str, bchr
 if PY2:
     from future.builtins import filter, map, zip, ascii, chr, hex, input, next, oct, open, pow, round, super, bytes, dict, list, object, range, str, max, min  # noqa: F401
@@ -21 +21 @@
 import shutil
 import gc
+from uuid import uuid4
 
 from twisted.trial import unittest
@@ -26 +27 @@
 from twisted.internet import defer
 from twisted.internet.task import Clock
+
+from hypothesis import given, strategies
 
 import itertools
@@ -34 +37 @@
 from allmydata.storage.mutable import MutableShareFile
 from allmydata.storage.immutable import BucketWriter, BucketReader, ShareFile
-from allmydata.storage.common import DataTooLargeError, storage_index_to_dir, \
+from allmydata.storage.common import storage_index_to_dir, \
      UnknownMutableContainerVersionError, UnknownImmutableContainerVersionError, \
      si_b2a, si_a2b
@@ -48 +51 @@
                                      VERIFICATION_KEY_SIZE, \
                                      SHARE_HASH_CHAIN_SIZE
-from allmydata.interfaces import BadWriteEnablerError
+from allmydata.interfaces import (
+    BadWriteEnablerError, DataTooLargeError, ConflictingWriteError,
+)
 from allmydata.test.no_network import NoNetworkServer
 from allmydata.storage_client import (
@@ -147 +152 @@
         self.failUnlessEqual(br.remote_read(25, 25), b"b"*25)
         self.failUnlessEqual(br.remote_read(50, 7), b"c"*7)
+
+    def test_write_past_size_errors(self):
+        """Writing beyond the size of the bucket throws an exception."""
+        for (i, (offset, length)) in enumerate([(0, 201), (10, 191), (202, 34)]):
+            incoming, final = self.make_workdir(
+                "test_write_past_size_errors-{}".format(i)
+            )
+            bw = BucketWriter(self, incoming, final, 200, self.make_lease(),
+                              FakeCanary())
+            with self.assertRaises(DataTooLargeError):
+                bw.remote_write(offset, b"a" * length)
+
+    @given(
+        maybe_overlapping_offset=strategies.integers(min_value=0, max_value=98),
+        maybe_overlapping_length=strategies.integers(min_value=1, max_value=100),
+    )
+    def test_overlapping_writes_ok_if_matching(
+            self, maybe_overlapping_offset, maybe_overlapping_length
+    ):
+        """
+        Writes that overlap with previous writes are OK when the content is the
+        same.
+        """
+        length = 100
+        expected_data = b"".join(bchr(i) for i in range(100))
+        incoming, final = self.make_workdir("overlapping_writes_{}".format(uuid4()))
+        bw = BucketWriter(
+            self, incoming, final, length, self.make_lease(),
+            FakeCanary()
+        )
+        # Three writes: 10-19, 30-39, 50-59. This allows for a bunch of holes.
+        bw.remote_write(10, expected_data[10:20])
+        bw.remote_write(30, expected_data[30:40])
+        bw.remote_write(50, expected_data[50:60])
+        # Then, an overlapping write but with matching data:
+        bw.remote_write(
+            maybe_overlapping_offset,
+            expected_data[
+                maybe_overlapping_offset:maybe_overlapping_offset + maybe_overlapping_length
+            ]
+        )
+        # Now fill in the holes:
+        bw.remote_write(0, expected_data[0:10])
+        bw.remote_write(20, expected_data[20:30])
+        bw.remote_write(40, expected_data[40:50])
+        bw.remote_write(60, expected_data[60:])
+        bw.remote_close()
+
+        br = BucketReader(self, bw.finalhome)
+        self.assertEqual(br.remote_read(0, length), expected_data)
+
+
+    @given(
+        maybe_overlapping_offset=strategies.integers(min_value=0, max_value=98),
+        maybe_overlapping_length=strategies.integers(min_value=1, max_value=100),
+    )
+    def test_overlapping_writes_not_ok_if_different(
+            self, maybe_overlapping_offset, maybe_overlapping_length
+    ):
+        """
+        Writes that overlap with previous writes fail with an exception if the
+        contents don't match.
+        """
+        length = 100
+        incoming, final = self.make_workdir("overlapping_writes_{}".format(uuid4()))
+        bw = BucketWriter(
+            self, incoming, final, length, self.make_lease(),
+            FakeCanary()
+        )
+        # Three writes: 10-19, 30-39, 50-59. This allows for a bunch of holes.
+        bw.remote_write(10, b"1" * 10)
+        bw.remote_write(30, b"1" * 10)
+        bw.remote_write(50, b"1" * 10)
+        # Then, write something that might overlap with some of them, but
+        # conflicts. Then fill in holes left by first three writes. Conflict is
+        # inevitable.
+        with self.assertRaises(ConflictingWriteError):
+            bw.remote_write(
+                maybe_overlapping_offset,
+                b'X' * min(maybe_overlapping_length, length - maybe_overlapping_offset),
+            )
+            bw.remote_write(0, b"1" * 10)
+            bw.remote_write(20, b"1" * 10)
+            bw.remote_write(40, b"1" * 10)
+            bw.remote_write(60, b"1" * 40)
 
     def test_read_past_end_of_share_data(self):