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: Do not fetch bucket stats by default upon bucket listing #15834

Merged
merged 1 commit into from Jul 12, 2017

Conversation

Projects
None yet
6 participants
@prallabh
Contributor

prallabh commented Jun 22, 2017

Fixes: http://tracker.ceph.com/issues/20377
Signed-off-by: Pavan Rallabhandi PRallabhandi@walmartlabs.com

@theanalyst theanalyst requested a review from rzarzynski Jun 22, 2017

@@ -43,7 +43,7 @@ class RGWListBuckets_ObjStore_SWIFT : public RGWListBuckets_ObjStore {
return 0;
}
public:
RGWListBuckets_ObjStore_SWIFT() : need_stats(true) {}
RGWListBuckets_ObjStore_SWIFT() : need_stats(false) {}

This comment has been minimized.

@rzarzynski

rzarzynski Jun 22, 2017

Contributor

I don't think that just switching to false is a way to go. The patch causes to fail two Tempest tests:

tempest.api.object_storage.test_account_services.AccountTest.test_list_containers_with_format_json
tempest.api.object_storage.test_account_services.AccountTest.test_list_containers_with_format_xml

I'm aware about the performance impact imposed by the current implementation. Though, it's still essential to:

  • provide the count and bytes properties for XML/JSON-formatted container listings,
  • calculate the X-Account-Object-Count, X-Account-Bytes-Used and X-Account-Bytes-Used-Actual headers for all types of container listings.

The first point could be addressed relatively easy (e.g. need_stats = (s->format != RGW_FORMAT_PLAIN)), but the second one is a different story. I'm afraid it would require deeper changes. Alternatively, we may consider delegating the choice to an administrator through introduction of a new configurable.

This comment has been minimized.

@prallabh

prallabh Jun 22, 2017

Contributor

@rzarzynski Thanks for the quick review. I was referring to http://tracker.ceph.com/issues/11285 where this check need_stats = (s->format != RGW_FORMAT_PLAIN) was removed.

With this patch this is what I see for my curl queries, am I missing anything here.

curl -i 'http://localhost:8000/swift/v1?stats=true&format=json' -X GET -H "X-Auth-Token: "
HTTP/1.1 200 OK
X-Timestamp: 1498141604.33927
X-Account-Container-Count: 1
X-Account-Object-Count: 1
X-Account-Bytes-Used: 5872
X-Account-Bytes-Used-Actual: 8192
X-Account-Access-Control: {}
X-Trans-Id: tx0000000000000000037c1-00594bd3a4-101b-default
X-Openstack-Request-Id: tx0000000000000000037c1-00594bd3a4-101b-default
Content-Type: application/json; charset=utf-8
Content-Length: 40
Date: Thu, 22 Jun 2017 14:26:44 GMT


curl -i 'http://localhost:8000/swift/v1?stats=true' -X GET -H "X-Auth-Token: "
HTTP/1.1 200 OK
X-Timestamp: 1498141609.86924
X-Account-Container-Count: 1
X-Account-Object-Count: 1
X-Account-Bytes-Used: 5872
X-Account-Bytes-Used-Actual: 8192
X-Account-Access-Control: {}
X-Trans-Id: tx0000000000000000037c2-00594bd3a9-101b-default
X-Openstack-Request-Id: tx0000000000000000037c2-00594bd3a9-101b-default
Content-Type: text/plain; charset=utf-8
Content-Length: 4
Date: Thu, 22 Jun 2017 14:26:49 GMT

This comment has been minimized.

@rzarzynski

rzarzynski Jun 22, 2017

Contributor

I'm afraid that the stats parameter is our private extension. Tempest doesn't use it but require the stats to be always present:

$ sudo ngrep -W byline port 8000 -d lo

...

########
T 127.0.0.1:33146 -> 127.0.0.1:8000 [AP]
GET /v1/KEY_9775d1b07dc245849dfddf3f64857b34/?format=json HTTP/1.1.
Host: 127.0.0.1:8000.
Accept-Encoding: identity.
connection: close.
X-Auth-Token: gAAAAABZS9cb7Oh41tfiRucOVG88cS7rGnfcgmcz9siBlVZPkoKx5k76vcVbMmMcdc_RdRyAyLrwPe7R5BMUhyh0p5v9HbtisHkZ-rkClWqAT0rK35lr-AilpefEU3r916ajnHpjBDSQ9LSqkj7If3cNAB3Hg_RG458Dqh-8iQXwbMFvSnKca7g.
.

##
T 127.0.0.1:8000 -> 127.0.0.1:33146 [AP]
HTTP/1.1 200 OK.
Content-Length: 196.
X-Timestamp: 1498142492.21979.
X-Account-Container-Count: 6.
X-Account-Object-Count: 0.
X-Account-Bytes-Used: 0.
X-Account-Bytes-Used-Actual: 0.
X-Account-Access-Control: {}.
X-Trans-Id: tx00000000000000000002d-00594bd71c-106b-default.
X-Openstack-Request-Id: tx00000000000000000002d-00594bd71c-106b-default.
Accept-Ranges: bytes.
Content-Type: application/json; charset=utf-8.
Date: Thu, 22 Jun 2017 14:41:32 GMT.
Connection: close.
.

##
T 127.0.0.1:8000 -> 127.0.0.1:33146 [AP]
[{"name":"tempest-a--501653990"},{"name":"tempest-b--1185429765"},{"name":"tempest-c--475862726"},{"name":"tempest-d--118749091"},{"name":"tempest-e--1032378481"},{"name":"tempest-f--1618574056"}]
########

This comment has been minimized.

@prallabh

prallabh Jun 22, 2017

Contributor

FWIW, the stats is added via http://tracker.ceph.com/issues/4759

@rzarzynski rzarzynski self-assigned this Jun 22, 2017

@rzarzynski

This comment has been minimized.

Contributor

rzarzynski commented Jun 22, 2017

The second thing: please correct the commit author's name and e-mail. Try:

git config user.name "John Smith"
git config user.email "user@host"
@mattbenjamin

This comment has been minimized.

Contributor

mattbenjamin commented Jun 22, 2017

@rzarzynski can this be implemented as an extension?

@rzarzynski

This comment has been minimized.

Contributor

rzarzynski commented Jun 22, 2017

@mattbenjamin: I think so. We could introduce a configurable for the default value of need_stats.

@prallabh

This comment has been minimized.

Contributor

prallabh commented Jun 22, 2017

@rzarzynski So would you want a new config to be introduced with the default value for stats set to true? Can you please advise further, thanks.

@rzarzynski

This comment has been minimized.

Contributor

rzarzynski commented Jun 23, 2017

@prallabh: yeah, I think this would be the quicker solution for the perfomance-vs-compliance dilemma.

@prallabh

This comment has been minimized.

Contributor

prallabh commented Jun 23, 2017

@rzarzynski can you please check now, thanks!

@rzarzynski

This comment has been minimized.

Contributor

rzarzynski commented Jun 25, 2017

jenkins retest this please (unrelated test failures)

@rzarzynski

LGTM. Could you please amend the commit auth's name and e-mail? I see:

root committed with prallabh 3 days ago

rgw: Have an option to not to fetch bucket stats upon bucket listing
Fixes: http://tracker.ceph.com/issues/20377
Signed-off-by: Pavan Rallabhandi <PRallabhandi@walmartlabs.com>
@prallabh

This comment has been minimized.

Contributor

prallabh commented Jun 27, 2017

@rzarzynski can you please check now, have updated the commit message as well appropriately, thanks.

@rzarzynski

LGTM. Thanks, @prallabh!

@prallabh

This comment has been minimized.

Contributor

prallabh commented Jun 28, 2017

@rzarzynski can this now be merged?

@rzarzynski

This comment has been minimized.

Contributor

rzarzynski commented Jun 28, 2017

It will be merged after passing a Teuthology run. I will schedule one soon.

@rzarzynski

This comment has been minimized.

@prallabh

This comment has been minimized.

Contributor

prallabh commented Jul 6, 2017

@rzarzynski I see some failures in that run, is that holding this to be merged.

@oritwas oritwas added the needs-qa label Jul 6, 2017

@rzarzynski

This comment has been minimized.

Contributor

rzarzynski commented Jul 10, 2017

@prallabh: no worries, it looks they are unrelated. Just in case I'm scheduling a new run.

@yuriw yuriw merged commit ed075b0 into ceph:master Jul 12, 2017

4 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
default Build finished.
Details
make check make check succeeded
Details
@yuriw

This comment has been minimized.

@prallabh prallabh deleted the prallabh:wip-20377 branch Jul 12, 2017

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