Properly fix GCS when HMAC is used #49390

antonio2368 · 2023-05-02T10:52:48Z

Changelog category (leave one):

Bug Fix (user-visible misbehavior in an official stable release)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Correctly modify GCS requests when HMAC key is used.

Fix #49195

cc @Algunenano @vitlibar

Documentation entry for user-facing changes

Documentation is written (mandatory for new features)

Information about CI checks: https://clickhouse.com/docs/en/development/continuous-integration/

robot-ch-test-poll1 · 2023-05-02T10:53:18Z

This is an automated comment for commit edf6580 with description of existing statuses. It's updated for the latest CI running
The full report is available here
The overall status of the commit is 🔴 error

Check name	Description	Status
AST fuzzer	Runs randomly generated queries to catch program errors. The build type is optionally given in parenthesis. If it fails, ask a maintainer for help	🟢 success
Bugfix validate check	Checks that either a new test (functional or integration) or there some changed tests that fail with the binary built on master branch	🔴 error
CI running	A meta-check that indicates the running CI. Normally, it's in success or pending state. The failed status indicates some problems with the PR	🟢 success
ClickHouse build check	Builds ClickHouse in various configurations for use in further steps. You have to fix the builds that fail. Build logs often has enough information to fix the error, but you might have to reproduce the failure locally. The cmake options can be found in the build log, grepping for cmake. Use these options and follow the general build process	🟢 success
Compatibility check	Checks that clickhouse binary runs on distributions with old libc versions. If it fails, ask a maintainer for help	🟢 success
Docker image for servers	The check to build and optionally push the mentioned image to docker hub	🟢 success
Fast test	Normally this is the first check that is ran for a PR. It builds ClickHouse and runs most of stateless functional tests, omitting some. If it fails, further checks are not started until it is fixed. Look at the report to see which tests fail, then reproduce the failure locally as described here	🟢 success
Flaky tests	Checks if new added or modified tests are flaky by running them repeatedly, in parallel, with more randomization. Functional tests are run 100 times with address sanitizer, and additional randomization of thread scheduling. Integrational tests are run up to 10 times. If at least once a new test has failed, or was too long, this check will be red. We don't allow flaky tests, read the doc	🟢 success
Install packages	Checks that the built packages are installable in a clear environment	🟢 success
Integration tests	The integration tests report. In parenthesis the package type is given, and in square brackets are the optional part/total tests	🔴 failure
Mergeable Check	Checks if all other necessary checks are successful	🟢 success
Performance Comparison	Measure changes in query performance. The performance test report is described in detail here. In square brackets are the optional part/total tests	🟢 success
Push to Dockerhub	The check for building and pushing the CI related docker images to docker hub	🟢 success
SQLancer	Fuzzing tests that detect logical bugs with SQLancer tool	🟢 success
Sqllogic	Run clickhouse on the sqllogic test set against sqlite and checks that all statements are passed	🟢 success
Stateful tests	Runs stateful functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	🟢 success
Stateless tests	Runs stateless functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	🔴 failure
Stress test	Runs stateless functional tests concurrently from several clients to detect concurrency-related errors	🟢 success
Style Check	Runs a set of checks to keep the code style clean. If some of tests failed, see the related log from the report	🟢 success
Unit tests	Runs the unit tests for different release types	🟢 success
Upgrade check	Runs stress tests on server version from last release and then tries to upgrade it to the version from the PR. It checks if the new server can successfully startup without any errors, crashes or sanitizer asserts	🔴 failure

Algunenano · 2023-05-04T14:55:11Z

I've done a couple of quick tests with GCS (setting up an s3 disk and reading a parquet file) with this PR and didn't see any problem so far.

jrdi · 2023-05-05T07:28:42Z

src/IO/S3/Client.cpp

+bool Client::supportsMultiPartCopy() const
 {
-    return provider_type;
+    return provider_type != ProviderType::GCS;
+}


Nitpicking: Unrelated to this PR but worth understanding some implications when using GCS. Why do we disable multipart upload in GCS? I thought this was already supported and the "only" missing big difference between GCS and S3 was the multi-delete.

This page discusses XML API multipart uploads in Cloud Storage. This upload method uploads files in parts and then assembles them into a single object using a final request. XML API multipart uploads are compatible with Amazon S3 multipart uploads.

https://cloud.google.com/storage/docs/multipart-uploads

We don't disable multipart upload but multipart native copy.
This is not supported by GCS so we have to try to do single part native copy and if it fails fallback to upload instead of multipart native copy as we do for AWS.
You can read some things left to do in the previous PR description #48981

The only difference is that GCS support team confirmed that they don't have a limit of 5GB for a single part native copy so we will probably remove the limit if GCS is used.
Additional problem is the need to compose a file uploaded with multi part upload before it can be copied natively but it returns 503 in some cases. This was also confirmed to be a problem on their side but I prepared a PR for it and will push it once it's resolved.

Until all the improvements are made it's important to be aware that backup could be slower than AWS in some cases.

Great, thank you @antonio2368!

@antonio2368 maybe worth to auto-detect this feature, like DeleteObjects?

ClickHouse/src/Disks/ObjectStorages/S3/registerDiskS3.cpp

Line 41 in 353791b

static bool checkBatchRemove(S3ObjectStorage & storage, const String & key_with_trailing_slash)

Since there can be other provides that will not support some features.

yes, that sounds like a better solution
maybe combine it and do it if the provider type is UNKNOWN because we already know best on URL for AWS and GCS

vitlibar · 2023-05-05T15:00:50Z

src/IO/S3/ProviderType.h

+enum class ApiMode : uint8_t
+{
+    AWS,
+    GCS


Can you please provide some comments about the enum and its constants?

Properly fix GCS with keys

8c91dbd

robot-ch-test-poll1 added the pr-bugfix Pull request with bugfix, not backported by default label May 2, 2023

antonio2368 added 3 commits May 2, 2023 11:08

Fix whitespace

468ed32

Merge branch 'master' into fix-gcs-headers

e8e9906

Better

9825531

jrdi reviewed May 5, 2023

View reviewed changes

vitlibar self-assigned this May 5, 2023

vitlibar approved these changes May 5, 2023

View reviewed changes

vitlibar reviewed May 5, 2023

View reviewed changes

Add comments for enums

edf6580

antonio2368 merged commit 212c57c into master May 8, 2023
257 of 261 checks passed

antonio2368 deleted the fix-gcs-headers branch May 8, 2023 06:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Properly fix GCS when HMAC is used #49390

Properly fix GCS when HMAC is used #49390

antonio2368 commented May 2, 2023 •

edited

robot-ch-test-poll1 commented May 2, 2023 •

edited by robot-ch-test-poll4

Algunenano commented May 4, 2023

jrdi May 5, 2023

antonio2368 May 5, 2023

antonio2368 May 5, 2023

jrdi May 5, 2023

azat May 16, 2023

antonio2368 May 17, 2023

vitlibar May 5, 2023

antonio2368 May 5, 2023

Properly fix GCS when HMAC is used #49390

Properly fix GCS when HMAC is used #49390

Conversation

antonio2368 commented May 2, 2023 • edited

Changelog category (leave one):

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Documentation entry for user-facing changes

robot-ch-test-poll1 commented May 2, 2023 • edited by robot-ch-test-poll4

Algunenano commented May 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonio2368 commented May 2, 2023 •

edited

robot-ch-test-poll1 commented May 2, 2023 •

edited by robot-ch-test-poll4