Skip to content
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

Rgw master fix plus #17040

Merged
merged 3 commits into from Aug 17, 2017

Conversation

@mdw-at-linuxbox
Copy link
Contributor

commented Aug 16, 2017

This is actually a "jumbo" patch of 3 different fixes. I'm combining them here just to simplify the testing/review process.

  1. rgw: replace '+'. This is an evolution of PR#16116
  2. fix zero sized chunk uploads. New fix.
  3. fix small sized chunk uploads. New fix.

With a ceph built with these fixes, I was able to get "hadoop 2.8.1" s3a tests to pass.

zhangsw and others added 3 commits Jul 4, 2017
rgw: replace '+' with "%20" in canonical query string for s3 v4 auth.
( note: this patch modified: now replaces "+" with " ".  mwatts@redhat.com )

Fixes: http://tracker.ceph.com/issues/20501

Signed-off-by: Zhang Shaowen <zhangshaowen@cmss.chinamobile.com>
radosgw: fix zero sized chunked uploads with s3
s3 allows for zero sized objects, which can be
created using chunked uploads.  It is important to
call "recv_body (and ChunkMeta::create_next()) even
on zero sized objects, so that the proper chunk signature
can be presented at signature verification time.

Fixes: http://tracker.ceph.com/issues/21000

Signed-off-by: Marcus Watts <mwatts@redhat.com>
radosgw: fix small sized chunked uploads with s3
The logic to compute the number of bytes to copy with a chunked file
upload was failing to take into account the amount of data that had been
consumed, but was still pending in the parse buffer.  This would cause
strange behavior ranging from "just works" to "fails".  There was also a
strange "works, one byte at a time" mode.  Using the correct stream_pos
offset eliminates the bad behavior.

This fix also adds debug statements to make the correct behavior easier to see.

Fixes: http://tracker.ceph.com/issues/21003

Signed-off-by: Marcus Watts <mwatts@redhat.com>
@mattbenjamin
Copy link
Contributor

left a comment

woot

/* Optimize the typical flow. */
return std::string();
}
if (params->find_first_of('+') != std::string::npos) {
copy_params = *params;
boost::replace_all(copy_params, "+", " ");

This comment has been minimized.

Copy link
@mattbenjamin

This comment has been minimized.

Copy link
@ZVampirEM77

ZVampirEM77 Aug 16, 2017

Contributor

Is it all right in here to replace the "+" to " "? Why not "20%"?

This comment has been minimized.

Copy link
@mdw-at-linuxbox

mdw-at-linuxbox Aug 16, 2017

Author Contributor

I believe there is later logic that translates " " into %20 where necessary.

@joscollin joscollin added the rgw label Aug 16, 2017

@rzarzynski
Copy link
Contributor

left a comment

LGTM.

size_t to_extract = \
std::min(chunk_meta.get_data_size(stream_pos), buf_max);
std::min(chunk_meta.get_data_size(stream_pos_was), buf_max);
dout(30) << "AWSv4ComplMulti: stream_pos_was=" << stream_pos_was << ", to_extract=" << to_extract << dendl;

This comment has been minimized.

Copy link
@rzarzynski

rzarzynski Aug 16, 2017

Contributor

AWSv4ComplMulti has the CephContext* available as cct, so ldout could be used here.

This comment has been minimized.

Copy link
@mdw-at-linuxbox

mdw-at-linuxbox Aug 16, 2017

Author Contributor

The last debug comment in the function used "dout". I thought it best to stick with the prevailing style. A better fix would be a separate patch to handle dout/ldout.

This comment has been minimized.

Copy link
@rzarzynski

rzarzynski Aug 16, 2017

Contributor

Sure, this is cosmetics only. Separated patch/pull request works for me.

@@ -986,8 +993,11 @@ size_t AWSv4ComplMulti::recv_body(char* const buf, const size_t buf_max)
std::begin(parsing_buf) + consumed);
}

size_t stream_pos_was = stream_pos - parsing_buf.size();

This comment has been minimized.

Copy link
@rzarzynski

rzarzynski Aug 16, 2017

Contributor

Thanks!

@@ -1138,7 +1138,7 @@ int RGWPutObj_ObjStore::get_data(bufferlist& bl)
}

int len = 0;
if (cl) {
{

This comment has been minimized.

Copy link
@rzarzynski

rzarzynski Aug 16, 2017

Contributor

OK.

@rzarzynski

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2017

jenkins retest this please (Makefile failed to find target 'tests')

@rzarzynski rzarzynski added the bug fix label Aug 16, 2017

@mattbenjamin

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2017

jenkins retest this please

@theanalyst

This comment has been minimized.

Copy link
Member

commented Aug 17, 2017

jenkins test docs

@mattbenjamin mattbenjamin merged commit e7b14d0 into ceph:master Aug 17, 2017

4 of 5 checks passed

make check (arm64) running make check
Details
Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.