Sketch of advise_corrupt_share support for immutables.

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

@@ -615,7 +615,7 @@ From RFC 7231::
 !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
 
 Advise the server the data read from the indicated share was corrupt.
-The request body includes an human-meaningful string with details about the corruption.
+The request body includes an human-meaningful (Unicode) string with details about the corruption.
 It also includes potentially important details about the share.
 
 For example::
@@ -624,6 +624,8 @@ For example::
 
 .. share-type, storage-index, and share-number are inferred from the URL
 
+The response code is OK, or 404 not found if the share couldn't be found.
+
 Reading
 ~~~~~~~
 

src/allmydata/storage/http_client.py

@@ -354,3 +354,31 @@ class StorageClientImmutables(object):
             return
         else:
             raise ClientException(response.code)
+
+    @inlineCallbacks
+    def advise_corrupt_share(
+        self,
+        storage_index: bytes,
+        share_number: int,
+        reason: str,
+    ):
+        """Indicate a share has been corrupted, with a human-readable message."""
+        assert isinstance(reason, str)
+        url = self._client.relative_url(
+            "/v1/immutable/{}/{}/corrupt".format(
+                _encode_si(storage_index), share_number
+            )
+        )
+        message = dumps({"reason": reason})
+        response = yield self._client.request(
+            "POST",
+            url,
+            data=message,
+            headers=Headers({"content-type": ["application/cbor"]}),
+        )
+        if response.code == http.OK:
+            return
+        else:
+            raise ClientException(
+                response.code,
+            )

src/allmydata/storage/http_server.py

@@ -455,3 +455,22 @@ class HTTPServer(object):
 
         request.setResponseCode(http.NO_CONTENT)
         return b""
+
+    @_authorized_route(
+        _app,
+        set(),
+        "/v1/immutable/<storage_index:storage_index>/<int(signed=False):share_number>/corrupt",
+        methods=["POST"],
+    )
+    def advise_corrupt_share(self, request, authorization, storage_index, share_number):
+        """Indicate that given share is corrupt, with a text reason."""
+        # TODO 3879 test success path
+        try:
+            bucket = self._storage_server.get_buckets(storage_index)[share_number]
+        except KeyError:
+            # TODO 3879 test this path
+            raise _HTTPError(http.NOT_FOUND)
+
+        info = loads(request.content.read())
+        bucket.advise_corrupt_share(info["reason"].encode("utf-8"))
+        return b""

src/allmydata/storage/server.py

@@ -743,8 +743,9 @@ class StorageServer(service.MultiService):
 
     def advise_corrupt_share(self, share_type, storage_index, shnum,
                              reason):
-        # This is a remote API, I believe, so this has to be bytes for legacy
-        # protocol backwards compatibility reasons.
+        # Previously this had to be bytes for legacy protocol backwards
+        # compatibility reasons. Now that Foolscap layer has been abstracted
+        # out, we can probably refactor this to be unicode...
         assert isinstance(share_type, bytes)
         assert isinstance(reason, bytes), "%r is not bytes" % (reason,)
 

src/allmydata/storage_client.py

@@ -77,7 +77,7 @@ from allmydata.util.hashutil import permute_server_hash
 from allmydata.util.dictutil import BytesKeyDict, UnicodeKeyDict
 from allmydata.storage.http_client import (
     StorageClient, StorageClientImmutables, StorageClientGeneral,
-    ClientException as HTTPClientException,
+    ClientException as HTTPClientException
 )
 
 
@@ -1094,7 +1094,10 @@ class _HTTPBucketReader(object):
         )
 
     def advise_corrupt_share(self, reason):
-       pass  # TODO in later ticket
+       return self.client.advise_corrupt_share(
+           self.storage_index, self.share_number,
+           str(reason, "utf-8", errors="backslashreplace")
+       )
 
 
 # WORK IN PROGRESS, for now it doesn't actually implement whole thing.
@@ -1124,7 +1127,7 @@ class _HTTPStorageServer(object):
             cancel_secret,
             sharenums,
             allocated_size,
-            canary,
+            canary
     ):
         upload_secret = urandom(20)
         immutable_client = StorageClientImmutables(self._http_client)
@@ -1148,7 +1151,7 @@ class _HTTPStorageServer(object):
     @defer.inlineCallbacks
     def get_buckets(
             self,
-            storage_index,
+            storage_index
     ):
         immutable_client = StorageClientImmutables(self._http_client)
         share_numbers = yield immutable_client.list_shares(
@@ -1165,9 +1168,24 @@ class _HTTPStorageServer(object):
         self,
         storage_index,
         renew_secret,
-        cancel_secret,
+        cancel_secret
     ):
         immutable_client = StorageClientImmutables(self._http_client)
         return immutable_client.add_or_renew_lease(
             storage_index, renew_secret, cancel_secret
         )
+
+    def advise_corrupt_share(
+        self,
+        share_type,
+        storage_index,
+        shnum,
+        reason: bytes
+    ):
+        if share_type == b"immutable":
+            imm_client = StorageClientImmutables(self._http_client)
+            return imm_client.advise_corrupt_share(
+                storage_index, shnum, str(reason, "utf-8", errors="backslashreplace")
+            )
+        else:
+            raise NotImplementedError()  # future tickets

src/allmydata/test/test_istorageserver.py

@@ -1149,12 +1149,6 @@ class HTTPImmutableAPIsTests(
 ):
     """HTTP-specific tests for immutable ``IStorageServer`` APIs."""
 
-    # These will start passing in future PRs as HTTP protocol is implemented.
-    SKIP_TESTS = {
-        "test_advise_corrupt_share",
-        "test_bucket_advise_corrupt_share",
-    }
-
 
 class FoolscapMutableAPIsTests(
     _FoolscapMixin, IStorageServerMutableAPIsTestsMixin, AsyncTestCase