GCP Support over the S3 XML API #2176

poodlewars · 2025-02-12T15:58:39Z

This adds support for GCP over the S3-compatible XML API.

GCP support did not work out of the box because Google does not support any of the S3 "batch" APIs. We used batch delete for all our deletion operations, which broke. Instead I use the unary delete operation here, using the AWS SDK's async API to reduce the performance impact of sending single object deletes.

Users connect to GCP like:

ac = Arctic("gcpxml://storage.googleapis.com:aseaton_bucket?access=...&secret=...")
ac = Arctic("gcpxmls://storage.googleapis.com:aseaton_bucket?access=...&secret=...")
# Kept aws_auth naming as this will use ~/.aws/credentials
ac = Arctic("gcpxml://storage.googleapis.com:aseaton_bucket?aws_auth=true&aws_profile=blah")
ac = Arctic("gcpxml://storage.googleapis.com:aseaton_bucket?aws_auth=true&prefix=my_prefix")

We have a URL of gcpxml so that if we do a full GCP implementation later, users can get it with gcp://. I've added a GCP storage proto object so that users on the gcpxml API will later be able to move to a gcp:// API without updating any library configs. The proto is tiny, just storing the library's prefix, which is the only information we need to serialize.

This is implemented as a small sublass of our existing S3 implementation, with its deletion functionality overridden. The Python library adapter creates the NativeVersionStore with a GCPXMLSettings in memory object (in the NativeVariantStorage) which makes us create a GCPXMLStorage C++ adapter rather than the normal S3Storage.

The interesting part of this PR is the threading model in do_remove_no_batching_impl. This is called from an IO executor as part of the RemoveTask so it is not safe (due to the risk of deadlocks) to submit any work to the IO executor from it. Instead:

The storage client s3_client_impl.cpp submits delete operations with the S3 API. The S3 SDK has its own event loop to handle these. When they complete, they resolve a folly::Promise, and the client returns a folly::Future out of that promise.
Our storage adapter (detail-inl.hpp) collects these futures, on an inline executor. This is fine because there is no work for this executor to do, other than waiting to be notified that promises have been resolved by the AWS SDK.

For testing, I created a moto based test fixture that returns errors when batch delete operations are invoked. Also tested manually against a real GCP backend.

There's some duplication between the gcpxml and the s3 Python adapters, but since hopefully the gcpxml adapter will be temporary until a full GCP implementation, I didn't see the value in refactoring it away.

Monday: 8450684276

poodlewars · 2025-02-12T16:00:36Z

python/tests/conftest.py

@@ -368,9 +384,11 @@ def mem_storage() -> Generator[InMemoryStorageFixture, None, None]:
 @pytest.fixture(
    scope="function",
    params=[


Do not merge the commented out fixtures

poodlewars · 2025-02-17T15:03:51Z

cpp/arcticdb/storage/common.hpp

    void update(const s3::S3Settings& config) {
        config_ = config;
    }
+
+    std::string to_string() {


This is just slightly helpful for debugging in the Python layer.

G-D-Petrov

LGTM I just have a couple of comments that don't need to be addressed in this PR:

As far as I can see, it is still possible to connect to GCP via the S3Storage directly, rather than GCPXMLStorage, this would be a user error ofc but an annoying one. I think that we should either:

have some logic that forbids this
add a fall back in the S3Storage when the batch delete fails

We should add some tests with a real GCP, not just a simulator

phoebusm · 2025-02-18T09:41:49Z

cpp/arcticdb/storage/config_cache.hpp

@@ -89,6 +89,12 @@ class ConfigCache {
                    storage_conf.config().UnpackTo(&s3_storage);
                    storages.emplace_back(create_storage(path, mode, s3::S3Settings(settings).update(s3_storage)));
                },
+                [&storage_conf, &storages, &path, mode] (const s3::GCPXMLSettings& settings) {
+                    util::check(storage_conf.config().Is<arcticdb::proto::gcp_storage::Config>(), "Only support S3 native settings");


"S3" -> "GCP"

phoebusm · 2025-02-18T09:58:30Z

python/tests/integration/storage_fixtures/test_s3.py

+def test_gcp_no_batch_delete(gcp_storage_factory: MotoGcpS3StorageFixtureFactory):
+    # Given
+    s3 = gcp_storage_factory
+    bucket = s3.create_fixture()


"s3" to "gcp" maybe to lessen the confusion?

phoebusm · 2025-02-18T10:20:18Z

cpp/arcticdb/storage/python_bindings.cpp

+                return py::make_tuple(settings.aws_auth());
+            },
+            [](py::tuple t) {
+                util::check(t.size() == 1, "Invalid GCPXMLSettings pickle objects");


If I have read the code correctly, gcpxml settings will be stored in this native class, instead of in protobuf, like we used to do.
If so, I think the rest of settings in the class are needed to be pickled as well.
Nice to have it test on test_fork.py

phoebusm · 2025-02-18T10:34:35Z

cpp/proto/arcticc/pb2/gcp_storage.proto

+package arcticc.pb2.gcp_storage_pb2;
+
+message Config {
+    string prefix = 1;


I wonder why prefix is the special kid, gets to stay in the proto, instead of living in native config like its sibilings 🤔

Because the path of a library is determined when the library is created, adding on the creation timestamp. We have to store it in _arctic_cfg or we'd never be able to find the library again.

phoebusm · 2025-02-18T10:38:02Z

python/arcticdb/storage_fixtures/s3.py

+                b'</Error>'
+            )
+            start_response(
+                "503 Slow Down", [("Content-Type", "text/xml"), ("Content-Length", str(len(response_body)))]


I think the http code is not 503 Slow Down for the response of not-implemented batch delete right?

Yeah, fixed in the Windows fixes commit

poodlewars · 2025-02-18T10:47:48Z

LGTM I just have a couple of comments that don't need to be addressed in this PR:

1. As far as I can see, it is still possible to connect to GCP via the `S3Storage` directly, rather than `GCPXMLStorage`, this would be a user error ofc but an annoying one. I think that we should either:


* have some logic that forbids this

* add a fall back in the `S3Storage` when the batch delete fails


2. We should add some tests with a real GCP, not just a simulator

Thanks!

Agreed on 2, we'll do that as a follow up. I have done a lot of manual testing with real GCP though.

On 1, to start with this will be a bit of a "hidden" feature and we can talk people through how to use it. I'd rather not implement any fancy "is it GCP" detection or fallbacks until we know that people get bitten by this.

phoebusm

LGTM apart from the pickle handling
Nice async on delete

…ssue with test fixture cleanup

poodlewars commented Feb 12, 2025

View reviewed changes

poodlewars force-pushed the aseaton/8450684276/gcp-support branch from b938fc1 to b798d13 Compare February 13, 2025 18:05

poodlewars commented Feb 17, 2025

View reviewed changes

poodlewars changed the title ~~aseaton/8450684276/gcp-support~~ GCP Support over the S3 XML API Feb 17, 2025

poodlewars marked this pull request as ready for review February 17, 2025 17:35

poodlewars requested review from alexowens90 and willdealtry as code owners February 17, 2025 17:35

poodlewars requested review from G-D-Petrov and phoebusm February 17, 2025 17:57

G-D-Petrov approved these changes Feb 18, 2025

View reviewed changes

phoebusm reviewed Feb 18, 2025

View reviewed changes

phoebusm requested changes Feb 18, 2025

View reviewed changes

phoebusm approved these changes Feb 18, 2025

View reviewed changes

Add support for GCP via the S3 compatible XML API

d189bdd

poodlewars force-pushed the aseaton/8450684276/gcp-support branch 5 times, most recently from a53e89c to e3787c6 Compare February 20, 2025 09:32

GCP XML support - change GCP fixture scope for now as there is some i…

5463f96

…ssue with test fixture cleanup

poodlewars force-pushed the aseaton/8450684276/gcp-support branch from e3787c6 to 5463f96 Compare February 20, 2025 10:28

poodlewars merged commit 5958781 into master Feb 20, 2025
152 checks passed

poodlewars deleted the aseaton/8450684276/gcp-support branch February 20, 2025 15:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GCP Support over the S3 XML API #2176

GCP Support over the S3 XML API #2176

poodlewars commented Feb 12, 2025 •

edited

Loading

poodlewars Feb 12, 2025

poodlewars Feb 17, 2025

G-D-Petrov left a comment

phoebusm Feb 18, 2025

phoebusm Feb 18, 2025

phoebusm Feb 18, 2025 •

edited

Loading

phoebusm Feb 18, 2025

poodlewars Feb 18, 2025 •

edited

Loading

phoebusm Feb 18, 2025

poodlewars Feb 18, 2025

poodlewars commented Feb 18, 2025

phoebusm left a comment •

edited

Loading

GCP Support over the S3 XML API #2176

GCP Support over the S3 XML API #2176

Conversation

poodlewars commented Feb 12, 2025 • edited Loading

poodlewars Feb 12, 2025

Choose a reason for hiding this comment

poodlewars Feb 17, 2025

Choose a reason for hiding this comment

G-D-Petrov left a comment

Choose a reason for hiding this comment

phoebusm Feb 18, 2025

Choose a reason for hiding this comment

phoebusm Feb 18, 2025

Choose a reason for hiding this comment

phoebusm Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

phoebusm Feb 18, 2025

Choose a reason for hiding this comment

poodlewars Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

phoebusm Feb 18, 2025

Choose a reason for hiding this comment

poodlewars Feb 18, 2025

Choose a reason for hiding this comment

poodlewars commented Feb 18, 2025

phoebusm left a comment • edited Loading

Choose a reason for hiding this comment

poodlewars commented Feb 12, 2025 •

edited

Loading

phoebusm Feb 18, 2025 •

edited

Loading

poodlewars Feb 18, 2025 •

edited

Loading

phoebusm left a comment •

edited

Loading