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: frontend subsystem rework #10767

Merged
merged 76 commits into from Nov 1, 2016

Conversation

rzarzynski
Copy link
Contributor

@rzarzynski rzarzynski commented Aug 17, 2016

TODO:

  • Document the new interfaces.
  • Test frontends and RadosGW's APIs. The test matrix will be quite big.
  • Apply the new filters over RGWRestfulIO to all frontends.
  • Move the AWS Auth v4 signature generator to a dedicated filter. (introduced AWS_AUTHv4_IO; the rest of the work will be accomplished in the continuation of the auth rework - I think that the AWSv4 auth engine would be able to decorate req_state::cio with a filter for signature calculation).
  • Seek and fix API clients which violate the call order of RGWRestfulIO. This is not the highest priority as we have RGWRestfulIOReorderingEngine now that could be used together with the new ASIO frontend. (applied the reordering filter).

@yuyuyu101
Copy link
Member

OH, the boost asio......

@rzarzynski rzarzynski force-pushed the wip-rgw-frontend-rework branch 2 times, most recently from 96a1f28 to d26ce25 Compare August 17, 2016 17:22
Copy link
Contributor

@cbodley cbodley left a comment

Choose a reason for hiding this comment

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

overall, looks really good! i love how the decorator pattern separates out the logic for all of these different behaviors, and allows frontends to mix/match the pieces they care about

uint64_t get_bytes_received() { return bytes_received; }
}; /* RGWStreamIO */
static inline RGWClientIOAccounter* ACCOUNTING_IO(struct req_state* s) {
return dynamic_cast<RGWClientIOAccounter*>(s->cio);
Copy link
Contributor

@cbodley cbodley Sep 21, 2016

Choose a reason for hiding this comment

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

if this needs to use dynamic_cast, callers will need to check its return value against null

i.e. this is not allowed ACCOUNTING_IO(s)->get_bytes_sent();

edit: now that RGWRestfulIO inherits from RGWRestfulIOAccountingEngine inherits from RGWClientIOAccounter, a static_cast should be perfectly safe here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE.


if (buffer_data) {
if (data.length()) {
sent += RGWDecoratedRestfulIO<T>::send_body(data.c_str(),
Copy link
Contributor

Choose a reason for hiding this comment

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

if a bufferlist has more than one segment, calling bufferlist::c_str() forces it to reallocate and copy those segments into a single contiguous buffer. we should avoid that!

instead, you can write each segment with something like this:

  if (buffer_data) {
    for (const auto& ptr : data.buffers()) {
      sent += RGWDecoratedRestfulIO<T>::send_body(ptr.c_str(), ptr.length());
    }
    data.clear();
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely TODO.

Thanks a lot for the snippet!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE.

#include "rgw_client_io.h"

template <typename T>
class RGWRestfulIOAccountingEngine : public RGWDecoratedRestfulIO<T>,
Copy link
Contributor

Choose a reason for hiding this comment

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

the naming scheme for these decorators seems a bit verbose. maybe just RGWIOAccounting, RGWIOContentLength, etc? or bring in namespaces, and go with something like rgw::io::Accounting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the moment we have rgw::io::AccountingEngine (the filter) and rgw::io::Accounter (the interface). I'm still looking for better names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, maybe just rgw::io::AccountingFilter? What do you think, Casey?

template<typename T> friend class RGWDecoratedRestfulIO;

public:
class Exception : public std::exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

any objection to just using std::system_error for this? using Exception = std::system_error;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE.

}
};

virtual std::size_t send_status(int status, const char *status_name) = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

why does size_t need to be qualified with std::?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because in C++, unlike in C, size_t lives in the std namespace. Although std::size_t looks really ugly, I would prefer to avoid putting global using in headers.

Copy link
Contributor

Choose a reason for hiding this comment

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

a global using isn't needed, though. ::size_t is already available through system headers, and is widely used by ceph and rgw already

i don't think it's necessary to qualify integer types like std::uint32_t, and i don't see what makes size_t any different

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great. I don't like std::size_t too. :-) TODO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE.

Copy link
Contributor

Choose a reason for hiding this comment

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

can we just use size_t instead of ::size_t please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, sorry - I've misunderstood you. I will rework this soon.

char buf[etag.size() + 2 + 1];
const auto len = snprintf(buf, sizeof(buf), "\"%s\"", etag.data());

return dump_header(s, "ETag: \"%s\"\r\n", boost::string_ref(buf, len));
Copy link
Contributor

Choose a reason for hiding this comment

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

return dump_header(s, "ETag", boost::string_ref(buf, len));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE.

else {
STREAM_IO(s)->print("Location: \"%s\"\r\n", s->info.request_uri.c_str());
} else {
dump_header(s, "Location", "\"" + s->info.request_uri + "\"");
Copy link
Contributor

Choose a reason for hiding this comment

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

can we find a way to avoid operator+() on strings for stuff like this? maybe add a dump_quoted_header() function for cases like this and lo_etag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this idea! TODO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE.

/* Receive body. On success Returns number of bytes sent to the direct
* client of RadosGW. On failure throws int containing errno. */
virtual std::size_t recv_body(char* buf, std::size_t max) = 0;
virtual std::size_t send_body(const char* buf, std::size_t len) = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

can you explain the rationale behind splitting recv/send_body from read/write_data? is it just to support RGWRestfulIOAccountingEngine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

recv_body and send_body are parts of the public interface for doing restful IO whilewrite_data and read_data live in the stream domain and are actually implementation' details. Although some engines are stream-oriented (both headers and body are handled the same way; FastCGI might be seen as an example here), others aren't. This applies especially to filters (accounting, chunking, buffering, reordering). Having to implement a machine state to differentiate headers and body would unnecessarily increase the complexity. Thus, I believe it's better to leverage the fact that the interface' usage pattern is truly restful.

dump_header(s, "Bucket",
url_encode(s->bucket_tenant + "/" + s->bucket_name));
} else {
dump_header(s, "Bucket", url_encode(s->bucket_name));
Copy link
Contributor

Choose a reason for hiding this comment

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

are you sure it's safe to pass a temporary std::string into dump_header() as a const string_ref& argument? it seems like the string_ref would end up with a pointer to freed memory

Copy link
Contributor Author

Choose a reason for hiding this comment

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

url_encode(s->bucket_name) should return a temporary std::string living till the end of the full expression. I think the same applies to temporary boost::string_ref created from it.

I would say the dangling pointer/reference might occur only if an engine buries boost::string_ref somewhere inside. That's the reason why RGWRestfulIOReorderingEngine::send_header() stores headers as a std::pair of std::string.

* mixing interfaces with implementation. Additionally, those classes derive
* from RGWRestfulIO defined here. I believe that including in the middle of
* file is still better than polluting it directly. */
#include "rgw_client_io_decoimpl.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

this in unfortunate, but i definitely think it's worth the tradeoff. and thanks for the explanation

@cbodley cbodley self-assigned this Sep 23, 2016
public:
virtual ~Accounter() {}

virtual void set_account(bool enabled) = 0;
Copy link
Contributor

@cbodley cbodley Oct 4, 2016

Choose a reason for hiding this comment

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

this part of the interface seems unnecessary to me.. i only see one caller, at the end of end_header() in rgw_rest.cc. can we just have AccountingEngine::complete_header() set enabled = true; instead?

edit: and are we really only accounting for the body, and not headers?

if (ACCOUNTING_IO(s)) {
bytes_sent = ACCOUNTING_IO(s)->get_bytes_sent();
bytes_received = ACCOUNTING_IO(s)->get_bytes_received();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

consider storing the result of ACCOUNTING_IO(s) in a temporary so we only dynamic_cast once

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE.

@rzarzynski
Copy link
Contributor Author

@cbodley: to prevent issues with empty-valued HTTP headers (frequently used especially in the Swift API) we might need vinniefalco/beast@2fc27fb for Beast.

@cbodley
Copy link
Contributor

cbodley commented Oct 6, 2016

to prevent issues with empty-valued HTTP headers (frequently used especially in the Swift API) we might need vinniefalco/beast@2fc27fb for Beast.

okay, yeah. we might need to set up a ceph fork for that repo to use for our submodule. feel free to retarget the submodule to whatever commit you need in the meantime

Copy link
Contributor

@cbodley cbodley left a comment

Choose a reason for hiding this comment

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

looking good. i like rgw::io::Exception much better than the nested one


endif # WITH_RADOSGW
endif # WITH_RADOS
endif # ENABLE_CLIENT
Copy link
Contributor

Choose a reason for hiding this comment

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

autotools is dead 💀 please don't resurrect it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE.

// vim: ts=8 sw=2 smarttab

#ifndef CEPH_RGW_CLIENT_IO_DECOIMPL_H
#define CEPH_RGW_CLIENT_IO_DECOIMPL_H
Copy link
Contributor

Choose a reason for hiding this comment

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

since all of these classes are named Filters, maybe rename this header to rgw_client_io_filters.h?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE.

public:
template <typename U>
AccountingFilter(U&& decoratee)
: DecoratedRestfulClient<T>(std::move(decoratee)),
Copy link
Contributor

Choose a reason for hiding this comment

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

U&& decoratee here is a universal reference - it will bind to either rvalue- or lvalue refs - so the std::move() should be std::forward(). same goes with other filters' constructors and their add_x() factory functions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE.

auto& env = DecoratedRestfulClient<T>::get_env();
auto ruri = env.get("REQUEST_URI", cct->_conf->rgw_request_uri.c_str());
auto suri = env.get("SCRIPT_URI", cct->_conf->rgw_request_uri.c_str());
env.set("REQUEST_URI", uri_prefix + ruri);
Copy link
Contributor

Choose a reason for hiding this comment

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

how does this interact with s3 signature verification? wouldn't it cause us to calculate a different signature than the client?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out, Casey. I'm looking for better way for prefixing than poking with REQUEST_URI. Maybe we can accomplish the goal by small modifications to RGWRESTMgr interface. I will get back to you soon.

ldout(s->cct, 0) << "ERROR: s->cio->send_content_length() returned err="
<< r << dendl;
<< e.code().value() << dendl;
Copy link
Contributor

Choose a reason for hiding this comment

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

printing e.what() would be more descriptive

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done here and in other places as well.

uint64_t get_bytes_received() { return bytes_received; }
}; /* RGWStreamIO */
static inline RGWRestfulIO* AWS_AUTHv4_IO(struct req_state* s) {
return static_cast<RGWRestfulIO*>(s->cio);
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm curious why the upcast from rgw::io::BasicClient to RGWRestfulIO is allowed here without dynamic_cast

and i suppose the same goes for RESTFUL_IO - how do we guarantee that the BasicClient *cio is actually a RestfulClient and not some other derived class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The casts are now protected with assert according to our discussion.

@rzarzynski rzarzynski force-pushed the wip-rgw-frontend-rework branch 2 times, most recently from 344a6e3 to d277a67 Compare October 12, 2016 15:09
@cbodley
Copy link
Contributor

cbodley commented Oct 12, 2016

I pushed this branch to ceph/wip-rgw-frontend-rework, but the centos gitbuilders didn't like it: http://gitbuilder.sepia.ceph.com/gitbuilder-ceph-rpm-centos7-amd64-basic/log.cgi?log=d277a67ec14cd7e33dc451c220f64f48221f30c0

@rzarzynski
Copy link
Contributor Author

@cbodley: fixed and rebased. I hope that after merging the Matt's pull request for in-source Boost we will be able to switch back to boost::string_ref::to_string just by reverting e9c2b49.

@rzarzynski rzarzynski changed the title [DNM] rgw: frontend subsystem rework rgw: frontend subsystem rework Oct 14, 2016
@rzarzynski rzarzynski force-pushed the wip-rgw-frontend-rework branch 2 times, most recently from baf393c to 0051815 Compare October 14, 2016 17:52
@rzarzynski
Copy link
Contributor Author

Pushed to ceph/wip-rgw-frontend-rework for gitbuilders.

@rzarzynski
Copy link
Contributor Author

rzarzynski commented Oct 15, 2016

Oops, it looks we have an issue with Beast and Boost version on the CentOS 7 gitbuilder:

 [100%] Building CXX object src/rgw/CMakeFiles/radosgw.dir/rgw_asio_frontend.cc.o
Linking CXX executable ../../bin/test_cors
Linking CXX executable ../../../bin/unittest_rgw_bencode
Linking CXX executable ../../../bin/unittest_http_manager
[100%] Built target ceph_test_rgw_obj
[100%] Building CXX object src/rgw/CMakeFiles/radosgw.dir/rgw_fcgi_process.cc.o
In file included from /srv/autobuild-ceph/gitbuilder.git/build/rpmbuild/BUILD/ceph-11.0.0/src/rgw/../Beast/include/beast/http/read.hpp:13:0,
from /srv/autobuild-ceph/gitbuilder.git/build/rpmbuild/BUILD/ceph-11.0.0/src/rgw/rgw_asio_frontend.cc:16:
/srv/autobuild-ceph/gitbuilder.git/build/rpmbuild/BUILD/ceph-11.0.0/src/rgw/../Beast/include/beast/core/async_completion.hpp:12:39: fatal error: boost/asio/async_result.hpp: No such file or directory
#include
^
compilation terminated. 

@rzarzynski rzarzynski force-pushed the wip-rgw-frontend-rework branch 2 times, most recently from 8cc1c33 to 071f0d3 Compare October 19, 2016 08:18
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
The method has been introduced in Boost 1.54. However,
some systems we are targeting (like CentOS 7) offer
older version of the library.

Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
This is a potential fix for partial writes and handling of error
codes that might come from mg_write() of the CivetWeb front-end.
None of the potential issues has been observed in testing.

The commit also documents the same aspects regarding the ASIO
and FastCGI front-ends.

Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
This is because of versioning issues between Boost and Beast
on CentOS 7. It is intended that this patch will be reverted
after merging the in-tree Boost facility.

Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
@rzarzynski
Copy link
Contributor Author

@cbodley: done - updated 273e540 to accommodate ceph/Beast. Absolutely no other changes.

@cbodley
Copy link
Contributor

cbodley commented Oct 24, 2016

investigating some performance regressions with the civetweb frontend. i ran cosbench on a simple vstart cluster (one mon, one osd, one radosgw). against master, it got ~650 read ops/s and 160 writes/sec. with this pr, it was ~440 reads and 110 writes

* for low-level buffering without relying on dynamic memory allocations.
* The buffer is carried entirely on stack. This narrows down applicability
* to those situations where buffers are relatively small. This perfectly
* fits the needs of composing an HTTP heade. Without that a front-end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FIXME: typo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE.

std::streambuf::setp(buffer, buffer + len);
}
};

Copy link
Contributor

Choose a reason for hiding this comment

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

have you looked at using common/PrebufferedStreambuf.h for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm afraid that PrebufferedStreambuf won't help us. IIRC (Adam Kupczyk made an optimization there) it's rather for efficient log handling than buffering network IO. Basically its overflow method enlarges the std::string instead of flushing the buffered content.

@cbodley
Copy link
Contributor

cbodley commented Oct 24, 2016

@rzarzynski thanks, after that commit to buffer the headers again, the regression is gone 👍

template <size_t BufferSizeV = 4096>
class StaticOutputBufferer : public std::streambuf {
static_assert(BufferSizeV >= sizeof(std::streambuf::char_type),
"The buffer must be bigger than one its char_type");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FIXME.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE.

@rzarzynski
Copy link
Contributor Author

@cbodley: thanks a lot for the immediate testing! I will apply similar modifications to other front-ends and also try to avoid the additional strlen in CivetWeb on a header's value.

@rzarzynski
Copy link
Contributor Author

@cbodley: the buffered header composition has been implemented in other front-ends as well. Using strlen in frontends' send_header methods isn't necessary anymore as the attributes are now sanitized to not include the trailing zero to string's content.

Both s3tests and Tempest haven't found a regression.

* to store metadata values (x-amz-meta-*, X-Container-Meta-*, etags).
* Otherwise we might send 0x00 to clients. */
const size_t len = raw.length();
return boost::string_ref(raw.c_str(), len ? len - 1 : 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

okay, i see that we're converting strings to bufferlist like this: bl.append(etag.c_str(), etag.size() + 1); i'm not sure why we need a null terminator in these bufferlists, because we shouldn't be trying to print them out

can you make this trimming logic conditional, so that we only trim if they're \0s? that way we have the option to start removing those trailing nulls

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'm not sure why we need a null terminator in these bufferlists, because we shouldn't be trying to print them out

I guess it was a workaround for ceph::buffer::list::c_str() that AFAIK doesn't guarantee its caller to obtain a null-terminated C-string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE.

*petag = string(etagbl.c_str(), etagbl.length());
const auto iter = attrs.find(RGW_ATTR_ETAG);
if (iter != std::end(attrs)) {
*petag = iter->second;
Copy link
Contributor

Choose a reason for hiding this comment

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

would you mind sticking to the attrs.end() version for consistency? as i understand it, std::end() is for generic code - it just makes it possible to pass in arrays instead of containers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE.

std::string and thus boost::string_ref ARE OBLIGED to carry multiple
0x00 and count them to the length of a string. We need to take that
into consideration and sanitize the size of a ceph::buffer::list used
to store metadata values (x-amz-meta-*, X-Container-Meta-*, etags).
Otherwise we might send 0x00 to clients.

Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
Without the patch front-ends issueed a lot of small IO operations
leading to increased overhead on syscalls and to the fragmentation
of an HTTP message across multiple TCP segments. The later was
occuring when the Nagle's algorithm hadn't been able to form
a single TCP segment (usually when running on extremely fast
network interfaces like loopback).

Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
@rzarzynski
Copy link
Contributor Author

@cbodley, @mattbenjamin: the two last commits have been modified to address Casey's comments. No changes besides that.

Tempest and s3-tests haven't found any regression. Swift's TempAUTH implementation has been tested manually - looks good.

@cbodley
Copy link
Contributor

cbodley commented Oct 25, 2016

looks good 👍
Reviewed-by: Casey Bodley <cbodley@redhat.com>

@liewegas
Copy link
Member

Forked to https://github.com/ceph/Beast

@mattbenjamin
Copy link
Contributor

@liewegas thanks!

if (sent_header)
goto send_data;

if (custom_http_ret) {
Copy link
Contributor

Choose a reason for hiding this comment

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

hi @cbodley , when i test radosgw-admin usage , i get object

from boto3.session import Session
import boto3
access_key = "yly"
secret_key = "yly"
url = 'http://127.0.0.1:7480'
session = Session(access_key, secret_key)
s3_client = session.client('s3', endpoint_url=url )

resp = s3_client.get_object(Bucket="test1", Key="512M")
with open('local-backup.bin','wb') as f:
    f.write(resp['Body'].read())

before it complete download ,i press ctrl+c to cancal ,and found that the success op =1 and op = 1, i found the op_ret = -5 , in the current master branch ,it will go send_header and goto send_data, which will not call set_req_state_err and resorting to 500, and in the /var/log/ceph/rgw.log file , the message will be GET /bucket/object 200, this is not correct, so should we remove this patch ?

Copy link
Contributor

Choose a reason for hiding this comment

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

jewel branch

2018-08-27 08:15:22.571433 7faa16f25700  1 ====== starting new request req=0x7faa16f1f710 =====
2018-08-27 08:15:22.890051 7faa16f25700  0 ERROR: flush_read_list(): d->client_c->handle_data() returned -5
2018-08-27 08:15:22.893102 7faa16f25700  0 WARNING: set_req_state_err err_no=5 resorting to 500
2018-08-27 08:15:22.893474 7faa16f25700  0 ERROR: s->cio->send_content_length() returned err=-5
2018-08-27 08:15:22.893482 7faa16f25700  0 ERROR: s->cio->print() returned err=-5
2018-08-27 08:15:22.893485 7faa16f25700  0 ERROR: STREAM_IO(s)->print() returned err=-5
2018-08-27 08:15:22.893502 7faa16f25700  0 ERROR: STREAM_IO(s)->complete_header() returned err=-5
2018-08-27 08:15:22.893670 7faa16f25700  1 ====== req done req=0x7faa16f1f710 op status=-5 http_status=500 ======
2018-08-27 08:15:22.893770 7faa16f25700  1 civetweb: 0x7fae5c000bb0: 127.0.0.1 - - [27/Aug/2018:08:15:22 +0800] "GET /ylybucket/100M HTTP/1.1" 500 0 - Boto3/1.7.6 Python/2.7.5 Linux/3.10.0-327.el7.x86_64 Botocore/1.10.6

master branch

2018-08-27 08:41:59.843 7f26d34f6700  1 ====== starting new request req=0x7f26d34ef870 =====
2018-08-27 08:41:59.895 7f26d34f6700  0 ERROR: flush_read_list(): d->client_cb->handle_data() returned -5
2018-08-27 08:41:59.895 7f26d34f6700  1 ====== req done req=0x7f26d34ef870 op status=-5 http_status=200 ======
2018-08-27 08:41:59.895 7f26d34f6700  1 civetweb: 0x7f270a75c000: 127.0.0.1 - - [27/Aug/2018:08:41:59 +0800] "GET /ylybucket/100M HTTP/1.1" 200 4866108 - Boto3/1.8.1 Python/2.7.5 Linux/3.10.0-327.28.3.el7.x86_64 Botocore/1.11.1


"user": "yly",
            "categories": [
                {
                    "category": "get_obj",
                    "bytes_sent": 4194304,
                    "bytes_received": 0,
                    "ops": 1,
                    "successful_ops": 1
                }
            ],

Copy link
Contributor

Choose a reason for hiding this comment

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

a084b82 , if agree with it ,i will pull requests t fix this,

Copy link
Contributor

Choose a reason for hiding this comment

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

the message will be GET /bucket/object 200, this is not correct

hi @joke-lee - in this case, haven't we already responded to the client with 200 OK?

@joke-lee
Copy link
Contributor

@mattbenjamin @cbodley ping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants