-
Notifications
You must be signed in to change notification settings - Fork 280
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Wip cbodley multipart nostreaming #564
base: master
Are you sure you want to change the base?
Wip cbodley multipart nostreaming #564
Conversation
Signed-off-by: Casey Bodley <cbodley@redhat.com>
this removes a Pytest warning during execution Signed-off-by: Matt Benjamin <mbenjamin@redhat.com>
…-multipart As described in https://tracker.ceph.com/issues/65746, retrying complete-multipart after having attempted to complete the same upload with a bad checksum argument fails with an internal error. The status code is 500, but I'm unsure if it can be retried again, or whether the upload can be aborted later. Signed-off-by: Matt Benjamin <mbenjamin@redhat.com>
tests a full multipart upload cycle with 3 unique parts, which verifies composite checksum computation and the logic to propagate parts_count to ComleteMultipart Signed-off-by: Matt Benjamin <mbenjamin@redhat.com>
response = client.complete_multipart_upload(Bucket=bucket, Key=key, UploadId=upload_id, ChecksumSHA256=composite_sha256sum, MultipartUpload={'Parts': [ | ||
{'ETag': etag1, 'ChecksumSHA256': response['ChecksumSHA256'], 'PartNumber': 1}, | ||
{'ETag': etag2, 'ChecksumSHA256': response['ChecksumSHA256'], 'PartNumber': 2}, | ||
{'ETag': etag3, 'ChecksumSHA256': response['ChecksumSHA256'], 'PartNumber': 3}]}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is passing the same ChecksumSHA256 value for each part, which comes from the last upload_part() response. CompleteMultipart should check that these checksums match the parts and reject such a request
you should be able to pass part1_sha256sum etc for these values
# should reject the missing part checksum | ||
e = assert_raises(ClientError, client.complete_multipart_upload, Bucket=bucket, Key=key, UploadId=upload_id, ChecksumSHA256='bad', MultipartUpload={'Parts': [ | ||
{'ETag': response['ETag'].strip('"'), 'PartNumber': 1}]}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, i'd made a mistake here - above we already tested that ChecksumSHA256='bad'
leads to 400 InvalidRequest
this one was meant to test the case where ChecksumSHA256 is missing from the Parts array
ChecksumSHA256='bad'
here should pass the correct value ChecksumSHA256=composite_sha256sum
(as below) so that we correctly test the Parts behavior
since test_multipart_checksum_3parts is passing with the wrong checksum values in Parts, i think ceph/ceph#55076 is missing the logic to validate those
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, true; it validates the checksums of the parts as actually uploaded, though
this tests a two-megabyte binary upload with validated (awscli-computed) SHA256 checksum, and also verifies failure when a bad checksum is provided Signed-off-by: Matt Benjamin <mbenjamin@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I might be not the right person to review, but just wanted to add small comments
size = 1024 | ||
body = FakeWriteFile(size, 'A') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
size = 1024 | |
body = FakeWriteFile(size, 'A') | |
body = FakeWriteFile(1024, 'A') |
Since it is not used inside the test except this place, can some some memory to not assign a variable to it :)
assert 'SHA256' == response['ChecksumAlgorithm'] | ||
upload_id = response['UploadId'] | ||
|
||
size = 1024 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
size = 1024 |
Can use size
variable from 13466 line
assert 'SHA256' == response['ChecksumAlgorithm'] | ||
upload_id = response['UploadId'] | ||
|
||
size = 1024 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
size = 1024 |
Same here
No description provided.