Implement CAS support for GCP test fixture #118236

nicktindall · 2024-12-09T06:58:22Z

Implemented in order that we can use it to test the fix for #116546

elasticsearchmachine · 2024-12-09T08:56:33Z

Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination)

DaveCTurner · 2024-12-10T11:15:51Z

test/fixtures/gcs-fixture/src/main/java/fixture/gcs/GoogleCloudStorageHttpHandler.java

+            try {
+                return Long.parseLong(params.get(parameterName));
+            } catch (NumberFormatException e) {
+                throw new MockGcsBlobStore.GcsRestException(RestStatus.BAD_REQUEST, "Invalid long parameter: " + parameterName);


Could we assert this doesn't happen (i.e. fail the test if it does)?

Fixed in 70c5da2 (then better in fdf8992)

DaveCTurner · 2024-12-10T11:21:52Z

test/fixtures/gcs-fixture/src/main/java/fixture/gcs/MockGcsBlobStore.java

+        });
+    }
+
+    BlobVersion updateResumableBlob(String path, String uploadId, BytesReference newContents) {


This doesn't look right to me. Resumable blobs are not visible to GET requests, and you can in principle have two different resumable uploads for the same path at the same time which should not conflict like this.

Admittedly it looks wrong in the existing code too, but we're making it more wrong here I think: a resumed upload needs to check the generation match at the point that the upload completes and becomes visible, not when deciding whether or not to start accepting new data.

Ah yep, I misinterpreted the purpose of the upload ID. I've implemented resumable uploads properly now, it was a bit of a can of worms, but I tested it with a streaming upload and it appeared to work as expected.

I restored the if-generation-match to work as you suggested, though I'm not sure if we actually use it in the fixture. In any case it was trivial to support.

…_support_gcp_test_fixture

nicktindall · 2024-12-11T03:03:35Z

test/fixtures/gcs-fixture/src/main/java/fixture/gcs/GoogleCloudStorageHttpHandler.java

        } finally {
-            int read = exchange.getRequestBody().read();
-            assert read == -1 : "Request body should have been fully read here but saw [" + read + "]";


This seemed to throw IOExceptions due to the body being closed, ExchangeImpl calls #close() when you call sendResponseHeaders with a zero-length body. None of the other HttpHandlers had this check.

DaveCTurner

LGTM except that I'd quite like to see some testing of the fixture itself, similar to S3HttpHandlerTests, as there's rather too much logic here to review by eye alone. Maybe add such a test suite in a separate PR, merge it first, and then we can include test changes just for the CAS support in this PR.

Relates #117329

nicktindall · 2024-12-16T07:17:33Z

LGTM except that I'd quite like to see some testing of the fixture itself, similar to S3HttpHandlerTests, as there's rather too much logic here to review by eye alone. Maybe add such a test suite in a separate PR, merge it first, and then we can include test changes just for the CAS support in this PR.

Relates #117329

I've put up a PR that tests the existing functionality here #118737

I think some of it is slightly wrong, so there will probably be some minor changes in the test once we merge it into this branch.

…_support_gcp_test_fixture

nicktindall · 2024-12-19T00:15:25Z

test/fixtures/gcs-fixture/src/test/java/fixture/gcs/GoogleCloudStorageHttpHandlerTests.java

        final var part1 = randomAlphaOfLength(50);
        final var uploadPart1Response = handleRequest(handler, "PUT", sessionURI, part1, contentRangeHeader(0, 50, null));
-        assertEquals(new TestHttpResponse(RESUME_INCOMPLETE, rangeHeader(0, 50)), uploadPart1Response);
+        assertEquals(new TestHttpResponse(RESUME_INCOMPLETE, rangeHeader(0, 49)), uploadPart1Response);


The returned range headers were actually wrong previously. Range is supposed to be inclusive.

DaveCTurner

Looks good, just a few tiny comments

DaveCTurner · 2024-12-19T09:03:37Z

test/framework/src/main/java/org/elasticsearch/test/fixture/HttpHeaderParser.java

+
+        @Override
+        public String toString() {
+            return "bytes=" + start + "-" + end;


toString() is generally best reserved for debugging-like uses only (and I'd rather keep the default implementation on records if possible). If we need to use a specific format for sending data over the wire, could we do so with a different method please?

Resolved in 8364c15

DaveCTurner · 2024-12-19T09:03:47Z

test/framework/src/main/java/org/elasticsearch/test/fixture/HttpHeaderParser.java

+        public String toString() {
+            final String rangeString = hasRange() ? start + "-" + end : "*";
+            final String sizeString = hasSize() ? String.valueOf(size) : "*";
+            return "bytes " + rangeString + "/" + sizeString;


Likewise here: toString() is generally best reserved for debugging-like uses only (and I'd rather keep the default implementation on records if possible). If we need to use a specific format for sending data over the wire, could we do so with a different method please?

Resolved in 8364c15

DaveCTurner · 2024-12-19T09:06:23Z

test/fixtures/gcs-fixture/src/test/java/fixture/gcs/GoogleCloudStorageHttpHandlerTests.java

+        // new file, non-zero generation
+        assertEquals(
+            RestStatus.PRECONDITION_FAILED,
+            executeResumableUpload(handler, bucket, blobName + randomIdentifier(), randomBytesReference(randomIntBetween(100, 5_000)), 1L)


nit: I know it's vanishingly unlikely, but for the sake of readability could we make sure to choose a name which is definitely different from the one used in the previous step?

Resolved in 093765d

DaveCTurner · 2024-12-19T09:07:46Z

test/fixtures/gcs-fixture/src/test/java/fixture/gcs/GoogleCloudStorageHttpHandlerTests.java

+        // update, matched generation
+        assertEquals(
+            RestStatus.OK,
+            executeResumableUpload(handler, bucket, blobName, randomBytesReference(randomIntBetween(100, 5_000)), 1L).restStatus()


nit: how do we know the generation is 1? Can we get that from the previous upload response (or a get)?

Resolved in 2796005

DaveCTurner · 2024-12-19T09:07:59Z

test/fixtures/gcs-fixture/src/test/java/fixture/gcs/GoogleCloudStorageHttpHandlerTests.java

+        // update, mismatched generation
+        assertEquals(
+            RestStatus.PRECONDITION_FAILED,
+            executeResumableUpload(handler, bucket, blobName, randomBytesReference(randomIntBetween(100, 5_000)), 13L).restStatus()


Likewise how do we know the generation is now not 13?

Resolved in 2796005

DaveCTurner · 2024-12-19T09:08:55Z

test/fixtures/gcs-fixture/src/test/java/fixture/gcs/GoogleCloudStorageHttpHandlerTests.java

+        );
+    }
+
+    public void testIfGenerationMatch_MultipartUpload() {


Similar comments here to the resumable case

Resolved in above commits

…_support_gcp_test_fixture

DaveCTurner

LGTM thanks Nick

Implement CAS support for GCP test fixture

c9892d7

elasticsearchmachine added the v9.0.0 label Dec 9, 2024

nicktindall added 2 commits December 9, 2024 18:09

Pass upload ID in when creating resumable

0598380

Remove redundant code

655d1ac

nicktindall added >test Issues or PRs that are addressing/adding tests :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs labels Dec 9, 2024

Merge branch 'main' into ES-5679_implement_cas_support_gcp_test_fixture

bd24892

nicktindall marked this pull request as ready for review December 9, 2024 08:56

elasticsearchmachine added the Team:Distributed Coordination Meta label for Distributed Coordination team label Dec 9, 2024

Tidy

e8e60e4

nicktindall requested a review from DaveCTurner December 9, 2024 09:03

Remove redundant method

685ddea

DaveCTurner reviewed Dec 10, 2024

View reviewed changes

nicktindall added 8 commits December 11, 2024 08:05

Fail test on invalid long parameter

70c5da2

Implement resumable uploads faithfully

fdf8992

Fix range arithmetic

cfb70d6

Merge remote-tracking branch 'origin/main' into ES-5679_implement_cas…

77dc333

…_support_gcp_test_fixture

Implement streaming uploads and status checks

5703b9b

Implement ifGenerationMatch for resumable uploads

41e661b

Fix javadoc

d218c31

Remove body read check (it seemed to throw?!)

f42e97d

nicktindall commented Dec 11, 2024

View reviewed changes

nicktindall added 3 commits December 11, 2024 14:53

Update test

74e1a2e

Tidy

07cced3

Merge branch 'main' into ES-5679_implement_cas_support_gcp_test_fixture

8764886

nicktindall requested a review from DaveCTurner December 11, 2024 05:28

DaveCTurner reviewed Dec 11, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into ES-5679_implement_cas…

78fb6f3

…_support_gcp_test_fixture

nicktindall added 2 commits December 18, 2024 13:50

Update GCS tests to work with new implementation

c3eea61

Add tests for ifGenerationMatch, de-dupe some of the request creation

dd20d09

nicktindall commented Dec 19, 2024

View reviewed changes

nicktindall added 3 commits December 19, 2024 11:20

Remove copy-paste artifact

b6ea31a

Use random upload type, tidy

3502439

Merge branch 'main' into ES-5679_implement_cas_support_gcp_test_fixture

c0de484

nicktindall requested a review from DaveCTurner December 19, 2024 02:30

DaveCTurner reviewed Dec 19, 2024

View reviewed changes

nicktindall added 4 commits December 20, 2024 07:44

Ask the server for blob generation

2796005

Avoid name collision for new file tests

093765d

Use headerString instead of toString

8364c15

Merge remote-tracking branch 'origin/main' into ES-5679_implement_cas…

84d0549

…_support_gcp_test_fixture

nicktindall requested a review from DaveCTurner December 19, 2024 22:22

DaveCTurner approved these changes Dec 27, 2024

View reviewed changes

Merge branch 'main' into ES-5679_implement_cas_support_gcp_test_fixture

5f46a3c

nicktindall merged commit 0dbe034 into elastic:main Jan 6, 2025
24 checks passed

nicktindall deleted the ES-5679_implement_cas_support_gcp_test_fixture branch January 6, 2025 06:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement CAS support for GCP test fixture #118236

Implement CAS support for GCP test fixture #118236

nicktindall commented Dec 9, 2024

elasticsearchmachine commented Dec 9, 2024

DaveCTurner Dec 10, 2024

nicktindall Dec 11, 2024 •

edited

Loading

DaveCTurner Dec 10, 2024

nicktindall Dec 11, 2024

nicktindall Dec 11, 2024

DaveCTurner left a comment

nicktindall commented Dec 16, 2024

nicktindall Dec 19, 2024

DaveCTurner left a comment

DaveCTurner Dec 19, 2024

nicktindall Dec 19, 2024

DaveCTurner Dec 19, 2024

nicktindall Dec 19, 2024

DaveCTurner Dec 19, 2024

nicktindall Dec 19, 2024

DaveCTurner Dec 19, 2024

nicktindall Dec 19, 2024

DaveCTurner Dec 19, 2024

nicktindall Dec 19, 2024

DaveCTurner Dec 19, 2024

nicktindall Dec 19, 2024

DaveCTurner left a comment

Implement CAS support for GCP test fixture #118236

Implement CAS support for GCP test fixture #118236

Conversation

nicktindall commented Dec 9, 2024

elasticsearchmachine commented Dec 9, 2024

Choose a reason for hiding this comment

nicktindall Dec 11, 2024 • edited Loading

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

DaveCTurner left a comment

Choose a reason for hiding this comment

nicktindall commented Dec 16, 2024

Choose a reason for hiding this comment

DaveCTurner left a 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

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

DaveCTurner left a comment

Choose a reason for hiding this comment

nicktindall Dec 11, 2024 •

edited

Loading