Skip to content

fix: allow after instructions for resumable uploads in a single chunk#514

Merged
cojenco merged 14 commits intogoogleapis:mainfrom
cojenco:resum-uploads-updates
Jul 19, 2023
Merged

fix: allow after instructions for resumable uploads in a single chunk#514
cojenco merged 14 commits intogoogleapis:mainfrom
cojenco:resum-uploads-updates

Conversation

@cojenco
Copy link
Copy Markdown
Contributor

@cojenco cojenco commented Jun 26, 2023 •
edited
Loading

Fixes #510

  • allow return-X-after-YK and x-goog-emulator-instructions:return-503-after-256K forced failures instructions for resumable uploads in both multiple chunks and a single chunk
  • add support to testbench for uploads to ignore request bytes already persisted, in align with GCS

[ASK]
Given that languages have different implementations on restarting an interrupted upload, verify language (conformance) tests are not adversely impacted by these new changes

integration tests & conformance tests

  • c++
  • c#
  • go
  • java
  • node.js
  • php
  • python
  • ruby
  • testbench unit tests

@codecov
Copy link
Copy Markdown

codecov bot commented Jun 29, 2023 •
edited
Loading

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01% 🎉

Comparison is base (38e7cbc) 98.65% compared to head (e8c4ef1) 98.66%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #514      +/-   ##
==========================================
+ Coverage   98.65%   98.66%   +0.01%     
==========================================
  Files          50       50              
  Lines        7717     7785      +68     
==========================================
+ Hits         7613     7681      +68     
  Misses        104      104
Flag Coverage Δ
unittests 98.66% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
testbench/common.py 96.53% <100.00%> (+0.12%) ⬆️
testbench/rest_server.py 97.84% <100.00%> (+0.05%) ⬆️
tests/test_testbench_object_upload.py 99.65% <100.00%> (+0.02%) ⬆️
tests/test_testbench_retry.py 99.50% <100.00%> (+0.02%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cojenco cojenco marked this pull request as ready for review July 10, 2023 23:39
@cojenco cojenco requested review from a team July 10, 2023 23:39
@cojenco cojenco requested a review from coryan as a code owner July 10, 2023 23:39
@coryan
Copy link
Copy Markdown
Contributor

coryan commented Jul 11, 2023

This seems to break the C++ tests:

googleapis/google-cloud-cpp#12069

Specifically these two tests:

https://github.com/googleapis/google-cloud-cpp/blob/576ea4680c54a9c3936481f43cc35072f9bd17f6/google/cloud/storage/tests/object_hash_integration_test.cc#L234-L257

https://github.com/googleapis/google-cloud-cpp/blob/576ea4680c54a9c3936481f43cc35072f9bd17f6/google/cloud/storage/tests/object_checksum_integration_test.cc#L238-L260

Both use x-goog-emulator-instructions: inject-upload-data-error, but fail with a fairly strange error.

2023-07-11T12:59:42.940753097Z [INFO] <140502397696384> UploadChunk() << UploadChunkRequest={upload_session_url=http://localhost:34887/upload/storage/v1/b/gcs-grpc-team-cloud-cpp-testing-bucket/o?uploadType=resumable&upload_id=4fb4efcfeb2deb52eb99f86617e5124336d7c2dfe2bc3a655a6eb3a4f0f8c5ad, range=<Content-Range: bytes 0-446/447>, known_object_hashes={}, x-goog-emulator-instructions: inject-upload-data-error, payload={{Lorem ipsum dolor sit amet, consectetur adipiscing.elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut e...<truncated>...}}} (/workspace/google/cloud/storage/internal/logging_client.cc:44)
2023-07-11T12:59:42.952790037Z [INFO] <140502397696384> UploadChunk() >> status={UNKNOWN: PerformWork() - CURL error [1]=Unsupported protocol} (/workspace/google/cloud/storage/internal/logging_client.cc:49)

coryan
coryan requested changes Jul 11, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@coryan coryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking for now until we figure out why the C++ tests are failing. It could be unrelated to these changes.

@cojenco
Copy link
Copy Markdown
Contributor Author

cojenco commented Jul 11, 2023

This seems to break the C++ tests:

googleapis/google-cloud-cpp#12069

Specifically these two tests:

https://github.com/googleapis/google-cloud-cpp/blob/576ea4680c54a9c3936481f43cc35072f9bd17f6/google/cloud/storage/tests/object_hash_integration_test.cc#L234-L257

https://github.com/googleapis/google-cloud-cpp/blob/576ea4680c54a9c3936481f43cc35072f9bd17f6/google/cloud/storage/tests/object_checksum_integration_test.cc#L238-L260

Both use x-goog-emulator-instructions: inject-upload-data-error, but fail with a fairly strange error.

2023-07-11T12:59:42.940753097Z [INFO] <140502397696384> UploadChunk() << UploadChunkRequest={upload_session_url=http://localhost:34887/upload/storage/v1/b/gcs-grpc-team-cloud-cpp-testing-bucket/o?uploadType=resumable&upload_id=4fb4efcfeb2deb52eb99f86617e5124336d7c2dfe2bc3a655a6eb3a4f0f8c5ad, range=<Content-Range: bytes 0-446/447>, known_object_hashes={}, x-goog-emulator-instructions: inject-upload-data-error, payload={{Lorem ipsum dolor sit amet, consectetur adipiscing.elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut e...<truncated>...}}} (/workspace/google/cloud/storage/internal/logging_client.cc:44)
2023-07-11T12:59:42.952790037Z [INFO] <140502397696384> UploadChunk() >> status={UNKNOWN: PerformWork() - CURL error [1]=Unsupported protocol} (/workspace/google/cloud/storage/internal/logging_client.cc:49)

Noted, thanks for surfacing this! I've added a checklist above to track language tests are passing.

@cojenco
Copy link
Copy Markdown
Contributor Author

cojenco commented Jul 11, 2023

We also see failed tests in Node.js with resumable upload request headers content-range set to'bytes 0-*/*'. This seems to be related to case handling in chunk_last_byte. Looking into this furthermore.

storage-testbench/testbench/rest_server.py

Lines 1007 to 1009 in a5a6592

if chunk_last_byte == "*":
x_upload_content_length = len(upload.media)
chunk_last_byte = len(upload.media) - 1

@cojenco
Copy link
Copy Markdown
Contributor Author

cojenco commented Jul 13, 2023

Hi @ddelgrosso1 the node conformance tests now pass in my local dev environment. Could you help verify again when you have a chance, thanks!

@cojenco
Copy link
Copy Markdown
Contributor Author

cojenco commented Jul 13, 2023

Hi @coryan I believe the two failed tests are related. I've updated the PR to only inspect after-bytes instructions and added a unit test. Could you help run the C++ test suite again when you have a chance, thanks!

@coryan
Copy link
Copy Markdown
Contributor

coryan commented Jul 13, 2023

The C++ tests are now passing: googleapis/google-cloud-cpp#12069 :shipit:

@cojenco
Copy link
Copy Markdown
Contributor Author

cojenco commented Jul 19, 2023

Update - confirmed that no language test is adversely impacted by this PR. Merging and cutting a new release tag shortly

@cojenco cojenco merged commit 99e3202 into googleapis:main Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coryan coryan coryan approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

PUTing a full object to a resumable session does not trigger after instructions

2 participants

@cojenco @coryan