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: update Beast for streaming reads in asio frontend #14273

Merged
merged 3 commits into from May 5, 2017

Conversation

Projects
None yet
4 participants
@cbodley
Contributor

cbodley commented Mar 31, 2017

tracking an upstream Beast development branch that adds support for streaming reads (see issue https://github.com/vinniefalco/Beast/issues/154)

passes all swift functional tests (which cover chunked encoding), and all s3tests except for 12 that send unreadable header values (some tests expect success, and others expect 403). the beast parser rejects these headers, so we reply with 400 Bad Request. you can run s3tests with the tag !fails_strict_rfc2616 to skip these

for early review - DNM until the development branch is finalized

@oritwas

This comment has been minimized.

Contributor

oritwas commented Apr 3, 2017

jenkins test this please

@cbodley cbodley requested a review from rzarzynski Apr 27, 2017

@cbodley

This comment has been minimized.

Contributor

cbodley commented May 2, 2017

i was able to run this through teuthology as part of https://github.com/ceph/ceph-ci/commits/wip-rgw-beast-new-parser-qa, which adds an extra commit qa/rgw: add beast frontend to rgw suites (i'm leaving that out of this pr until we're committed to maintaining this frontend)

the results with --filter frontend/beast.yaml are available at http://pulpito.ceph.com/cbodley-2017-05-01_15:23:40-rgw-wip-rgw-beast-new-parser-qa---basic-mira/:
three unrelated valgrind failures, and one s3tests failure that somehow ran and failed 7 tests that should have been filtered out by !fails_strict_rfc2616

@rzarzynski

LGTM. The comments regard less-than-minor things. Thanks, @cbodley!

void RGWAsioClientIO::init_env(CephContext *cct)
void ClientIO::init_env(CephContext *cct)

This comment has been minimized.

@rzarzynski

rzarzynski May 2, 2017

Contributor

I know it isn't new but maybe:

CephContext* cct
boost::system::error_code ec;
dout(30) << this << " read_data for " << max << " with "
<< buffer.size() << " bytes buffered" << dendl;

This comment has been minimized.

@rzarzynski

rzarzynski May 2, 2017

Contributor

Indentation.

RGWRequest req{env.store->get_new_req_id()};
RGWAsioClientIO real_client{std::move(socket), std::move(request)};
rgw::asio::ClientIO real_client{socket, parser, buffer};

This comment has been minimized.

@rzarzynski

rzarzynski May 2, 2017

Contributor

OK, rgw::asio::ClientIO stores references now.

@@ -7,13 +7,11 @@
#include <vector>
#include <boost/asio.hpp>
#include <boost/optional.hpp>

This comment has been minimized.

@rzarzynski

rzarzynski May 2, 2017

Contributor

Thanks!

#include <beast/http/body_type.hpp>
#include <beast/http/concepts.hpp>
#include <beast/http/message_v1.hpp>
#include <boost/optional.hpp>

This comment has been minimized.

@rzarzynski

rzarzynski May 2, 2017

Contributor

I'm not sure whether the header is really needed.

@cbodley

This comment has been minimized.

Contributor

cbodley commented May 2, 2017

thanks @rzarzynski. i pushed an update that removes the extra optional.hpp include

@cbodley cbodley changed the title from [DNM] rgw: update Beast for streaming reads in asio frontend to rgw: update Beast for streaming reads in asio frontend May 2, 2017

@rzarzynski

This comment has been minimized.

Contributor

rzarzynski commented May 2, 2017

jenkins test this please

cbodley added some commits Feb 18, 2017

rgw: update Beast for streaming reads in asio frontend
Signed-off-by: Casey Bodley <cbodley@redhat.com>
rgw: rename frontend from asio to beast
Signed-off-by: Casey Bodley <cbodley@redhat.com>
rgw: update Beast submodule
Signed-off-by: Casey Bodley <cbodley@redhat.com>
@cbodley

This comment has been minimized.

Contributor

cbodley commented May 5, 2017

new streaming read interfaces merged to Beast's upstream master. i pulled up https://github.com/ceph/Beast to match, and updated the submodule to point there. still building and passing tests

@cbodley cbodley merged commit 9667761 into ceph:master May 5, 2017

2 of 3 checks passed

Unmodifed Submodules Approval needed: modified submodules found
Details
Signed-off-by all commits in this PR are signed
Details
default Build finished.
Details

@cbodley cbodley deleted the cbodley:wip-rgw-beast-new-parser branch May 5, 2017

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented May 22, 2017

This breaks the s390x build :-(

[  228s] CMake Error at /usr/share/cmake/Modules/FindBoost.cmake:1657 (message):
[  228s]   Unable to find the requested Boost libraries.
[  228s] 
[  228s]   Boost version: 1.63.0
[  228s] 
[  228s]   Boost include path:
[  228s]   /home/abuild/rpmbuild/BUILD/ceph-12.0.3+git.1495491843.99463ac8f3/build/boost/include
[  228s] 
[  228s] 
[  228s]   Could not find the following static Boost libraries:
[  228s] 
[  228s]           boost_context
[  228s] 
[  228s]   Some (but not all) of the required Boost libraries were found.  You may
[  228s]   need to install these additional Boost libraries.  Alternatively, set
[  228s]   BOOST_LIBRARYDIR to the directory containing Boost libraries or BOOST_ROOT
[  228s]   to the location of Boost.
[  228s] Call Stack (most recent call first):
[  228s]   CMakeLists.txt:595 (find_package)

@cbodley Any ideas?

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented May 22, 2017

@cbodley The s390x build is mandatory for us. . . I guess we'll have to conditionalize all the Beast stuff so the s390x builds don't see it. Either that or enable the context module to build on s390x :-/

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented May 22, 2017

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented May 23, 2017

Testing a fix [1] for the s390x build issue.

[1] #15225

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment