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.

Sign up for GitHub

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

Jump to bottom

radosgw: usage: fix bytes_sent bug. #16834

Merged

mattbenjamin merged 2 commits into ceph:master from mdw-at-linuxbox:policy

Aug 9, 2017

Contributor

mdw-at-linuxbox commented Aug 4, 2017 •

edited by smithfarm

http://tracker.ceph.com/issues/19870

The key parts of this fix are:
implement AccountingFilter::complete_request() - account for bytes reported here.
BufferingFilter::complete_request() - ignore counts from send_content_length() complete_header();

There is also some debugging stuff here. Some of it is slightly useful. But the big thing here is passing in "cct" to more things. I have an ugly fix here. A better fix would be useful, although not directly relevant to this bug.

mdw-at-linuxbox mentioned this pull request

Test bytes_sent bugs. #16835

Closed

mdw-at-linuxbox requested review from cbodley, oritwas and rzarzynski

August 5, 2017 01:16

mdw-at-linuxbox assigned mattbenjamin and cbodley

joscollin added bug-fix rgw labels

rzarzynski reviewed

View reviewed changes

Contributor

rzarzynski left a comment

@mdw-at-linuxbox: thank you for bringing this fix! Please see my comments.

src/rgw/rgw_log.cc

                 if (!s->is_err())
                   data.successful_ops = 1;
+                ldout(s->cct, 30) << "log_usage: bucket_name=" << bucket_name

Contributor

rzarzynski Aug 7, 2017

We might want to log here also tenant of the bucket. Take a look on req_state::bucket_tenant.

src/rgw/rgw_client_io.h

               public:
                 ~RGWRestfulIO() override = default;
-                RGWRestfulIO(rgw::io::RestfulClient* engine)
-                  : AccountingFilter<rgw::io::RestfulClient*>(std::move(engine)) {
+                RGWRestfulIO(CephContext *_cx, rgw::io::RestfulClient* engine)

Contributor

rzarzynski Aug 7, 2017

Usually an instance of CephContext* is named as cct.

src/rgw/rgw_client_io_filters.h Outdated

@@ @@ -14,26 +14,31 @@ @@
               namespace rgw {
               namespace io {
+              #define dout_subsys ceph_subsys_rgw

Contributor

rzarzynski Aug 7, 2017

It would be nice to have a comment about cleaning the stuff later. Alternatively, we could consider a new ldout-for-headers macro taking the dout_subsys as a parameter.

Contributor

mattbenjamin Aug 7, 2017

we can just use lsubdout, I'm pretty sure

src/rgw/rgw_client_io_filters.h Outdated

                   : DecoratedRestfulClient<T>(std::forward<U>(decoratee)),
                     enabled(false),
                     total_sent(0),
-                    total_received(0) {
+                    total_received(0), cx(_cx) {

Contributor

rzarzynski Aug 7, 2017

Prefixing with underscore is unnecessary. cx(cx) should work as well.

src/rgw/rgw_client_io_filters.h Outdated

                   : DecoratedRestfulClient<T>(std::forward<U>(decoratee)),
                     has_content_length(false),
-                    buffer_data(false) {
+                    buffer_data(false), cx(_cx) {

Contributor

rzarzynski Aug 7, 2017

As above.

src/rgw/rgw_client_io_filters.h Outdated

+                  ldout(cx, 30) <<
+                      "BufferingFilter::complete_request: !has_content_length: IGNORE: sent="
+                      << sent << dendl;
+                  sent = 0;

Contributor

rzarzynski Aug 7, 2017

Yeah, I see the pain. BufferingFilter doesn't implement (and can't without extensive changes) the rgw::io::Accounter interface, so we lack the knowledge whether the accounting is enabled or not. In consequence, we're assuming the logic of end_header here: do not account header.

Contributor

rzarzynski Aug 7, 2017

@mdw-at-linuxbox: maybe another solution exists. Can we assume the relevant part of HTTP response, from the accounting POV, is always body?

Contributor

rzarzynski Aug 7, 2017

I'm researching this possibility. The idea is to simplify the accounting in a way that takes care only of the existing use cases (HTTP Body accounting), and thus allows to strip the extra complexity we have at the moment.

I think we still should merge the patch as a stop-gap solution.

src/rgw/rgw_client_io_filters.h

+                }
+                size_t complete_request() override {
+                  const auto sent = DecoratedRestfulClient<T>::complete_request();

Contributor

rzarzynski Aug 7, 2017

For sure.

mattbenjamin requested changes

View reviewed changes

Contributor

mattbenjamin left a comment

@mdw-at-linuxbox, @rzarzynski ok, so based on discussion and prior comments, we should replace uses of ldout in .h files w/lsubdout and remove any subsys macros added here; but then take this as a stop-gap fix

rzarzynski mentioned this pull request

rgw: simplify and enable by default the transfer accounting. #16904

Closed

mdw-at-linuxbox added 2 commits

August 8, 2017 21:54


          Fix bytes_sent bugs.

0172aab

log bytes sent/received.
add cct to bufferingfilter
add cct to RGWRestfulIO
AccountingFilter - save cct for debugging output
implement AccountingFilter::complete_request() - account for bytes reported here.
BufferingFilter<T>::complete_request() - ignore counts from send_content_length() complete_header();

Code quality note:
this patch makes "cct" available for a lot of newly added debug
statements.  The debug statements are mostly not very useful (and should
go away in the future) - *But* the "cct" logic should be redone and
incorporated into some base class (such RestfulClient) so that it is
possible to easily add in debug statements such as these in the future.

Fixes: http://tracker.ceph.com/issues/19870
Signed-off-by: Marcus Watts <mwatts@redhat.com>


          Test bytes_sent bugs.

a45ab45

Rearrange logic to make it easier to measure accumulation.
Instrument the boto request/response loop to count bytes in and out.
Accumulate byte counts in usage like structure.
Compare actual usage reported by ceph against local usage measured.
Report and assert if there are any short-comings.
Remove zone placement rule that was newly added at end: tests should be rerunable.

Nit: the logic to wait for "delete_obj" is not quite right.

Fixes: http://tracker.ceph.com/issues/19870
Signed-off-by: Marcus Watts <mwatts@redhat.com>

mdw-at-linuxbox force-pushed the policy branch from fcfab70 to a45ab45 Compare

August 9, 2017 02:00

Contributor Author

mdw-at-linuxbox commented Aug 9, 2017

I've merged PR # 16385 into this one -that has the radosgw_admin.py test logic. I've also pulled out logic that had gone away in master. Good riddance - I didn't want to test that part anyways.

I've made various fixes to address the issues people raised above. I believe they're all addressed. I made and installed one build and ran radosgw_admin.py against it - it passes.

oritwas reviewed

View reviewed changes

Member

oritwas left a comment •

edited

The usage test looks good.
For multisite we will need to add tests for usage.
Maybe move the utility functions to util/rgw.py for reuse (can be done later when we add the tests to test_multi)

Contributor

mattbenjamin commented Aug 9, 2017

@mdw-at-linuxbox , @rzarzynski I'll do a suite run

mattbenjamin added the needs-qa label

Contributor

mattbenjamin commented Aug 9, 2017 •

edited

http://pulpito.ceph.com/mbenjamin-2017-08-09_11:31:15-rgw-wip-mdw-usage---basic-smithi/
this is a pass; we do have two instances of the "failed meta checkpoint for zone=a2" false positive

mattbenjamin approved these changes

View reviewed changes

mattbenjamin merged commit 0956b3a into ceph:master

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