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: allow enforcing of maximum requested in listing when possible #33864

Closed
wants to merge 2 commits into from

Conversation

ivancich
Copy link
Member

@ivancich ivancich commented Mar 10, 2020

Some Swift clients assume that the number of items requested in a bucket listing will be returned if possible. This allows the underlying code to enfore that behavior, so Swift can get that behavior, but S3 does not have to.

Tracker: https://tracker.ceph.com/issues/44554

@matthewoliver
Copy link
Contributor

Yeah the swift API and therefore clients I've seen expect up to the max container listing limit of a single req (10 000, but configurable), clients should see this value from the /info call.
If < max is returned, it's assumed we have it all, if == max, then there could be more, so client paginate.

Obivously there is also prefix, marker, end_marker, and reverse which changes potential output, but still that's up to the client to use and understand. (ie, prefix for psudo containers, or marker/end_marker to paginate by some bounds).

@ivancich
Copy link
Member Author

@matthewoliver Thank you for weighing in.

I wish this API documentation were more clear: Swift API documentation

@matthewoliver
Copy link
Contributor

matthewoliver commented Mar 17, 2020 via email

@ivancich
Copy link
Member Author

@mattbenjamin This now has both commits.

@smithfarm smithfarm added this to the octopus milestone Apr 6, 2020
Copy link
Contributor

@smithfarm smithfarm left a comment

Choose a reason for hiding this comment

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

please change the base branch from "octopus" to "master"

@smithfarm smithfarm removed this from the octopus milestone Apr 6, 2020
@ivancich ivancich changed the base branch from octopus to master April 8, 2020 15:11
@ivancich ivancich dismissed smithfarm’s stale review May 6, 2020 15:13

base changed to master 28 days ago

@ivancich ivancich added the wip-eric-testing-1 for ivancich testing label May 6, 2020
Some Swift clients assume that the number of items requested in a
bucket listing will be returned if possible. This allows the
underlying code to enfore that behavior, so Swift can get that
behavior, but S3 does not.

Signed-off-by: J. Eric Ivancich <ivancich@redhat.com>
Signed-off-by: Matt Benjamin <mbenjamin@redhat.com>
@cbodley
Copy link
Contributor

cbodley commented May 8, 2020

Yeah the swift API and therefore clients I've seen expect up to the max container listing limit of a single req (10 000, but configurable), clients should see this value from the /info call.

we advertise this value of RGW_LIST_BUCKETS_LIMIT_MAX=10000 in the swift /info api here:

void RGWInfo_ObjStore_SWIFT::list_swift_data(Formatter& formatter,
                                              const ConfigProxy& config,
                                              RGWRados& store)
{ 
  formatter.open_object_section("swift");
  formatter.dump_int("max_file_size", config->rgw_max_put_size);
  formatter.dump_int("container_listing_limit", RGW_LIST_BUCKETS_LIMIT_MAX);

i think we should return the value of rgw_max_listing_results here, and use that value for RGWListBucket_ObjStore_SWIFT::default_max as well. then we don't need to change the semantics of parse_value_and_bound(), which is used elsewhere

@ivancich
Copy link
Member Author

ivancich commented May 8, 2020

we advertise this value of RGW_LIST_BUCKETS_LIMIT_MAX=10000 in the swift /info api here:

void RGWInfo_ObjStore_SWIFT::list_swift_data(Formatter& formatter,
                                              const ConfigProxy& config,
                                              RGWRados& store)
{ 
  formatter.open_object_section("swift");
  formatter.dump_int("max_file_size", config->rgw_max_put_size);
  formatter.dump_int("container_listing_limit", RGW_LIST_BUCKETS_LIMIT_MAX);

i think we should return the value of rgw_max_listing_results here, and use that value for RGWListBucket_ObjStore_SWIFT::default_max as well. then we don't need to change the semantics of parse_value_and_bound(), which is used elsewhere

The challenge is that rgw_max_listing_results is set to 1000, which works well for s3. However, the swift api specifies that 10,000 objects are returned when that many can be returned (see: https://docs.openstack.org/api-ref/object-store/?expanded=show-container-details-and-list-objects-detail). I think technically we'd be OK as long as we "advertise" that value, but that might be problematic with users.

But ... clients might assume that without configuration changes they'll get 10,000 back.

And ... how would we configure an RGW installation with both s3 and swift users, such that s3 users get 1,000 and swift users get 10,000 entries?

The config option "rgw_max_listing_results" sets a maximum listing
count across a number of operations. Some protocols define a default
listing count for some operations. For example, Swift defines a
default container listing count of 10,000 items. When this protocol
default exceeds the number defined in the config option, the default
value should be considered the actual maximum allowed.

Signed-off-by: J. Eric Ivancich <ivancich@redhat.com>
@ivancich
Copy link
Member Author

ivancich commented May 8, 2020

@ivancich ivancich removed the wip-eric-testing-1 for ivancich testing label May 8, 2020
@ivancich
Copy link
Member Author

ivancich commented May 8, 2020

jenkins test make check

@ivancich ivancich added the wip-eric-testing-1 for ivancich testing label May 8, 2020
@ivancich
Copy link
Member Author

ivancich commented Jun 4, 2020

@cbodley : did I address your concerns adequately in #33864 (comment) ? If not, can we talk in order to move this PR forward?

@mattbenjamin
Copy link
Contributor

@ivancich @cbodley thanks for highlighting this, it's important to bring this upstream

@smithfarm
Copy link
Contributor

jenkins test make check

@ivancich ivancich removed the wip-eric-testing-1 for ivancich testing label Jun 6, 2020
@stale
Copy link

stale bot commented Aug 6, 2020

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@stale stale bot added the stale label Aug 6, 2020
@ivancich ivancich removed the stale label Aug 6, 2020
@stale
Copy link

stale bot commented Oct 10, 2020

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@stale stale bot added the stale label Oct 10, 2020
@stale stale bot removed the stale label Oct 10, 2020
@stale
Copy link

stale bot commented Dec 13, 2020

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@stale stale bot added the stale label Dec 13, 2020
@ivancich ivancich removed the stale label Dec 13, 2020
@stale
Copy link

stale bot commented Jul 21, 2021

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@stale stale bot added the stale label Jul 21, 2021
@smithfarm smithfarm removed their request for review July 21, 2021 15:38
@stale stale bot removed the stale label Jul 21, 2021
@stale
Copy link

stale bot commented Jan 9, 2022

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@stale stale bot added the stale label Jan 9, 2022
@djgalloway djgalloway changed the base branch from master to main July 9, 2022 00:00
@github-actions github-actions bot removed the stale label Jul 15, 2022
@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@github-actions github-actions bot added the stale label Sep 13, 2022
@github-actions
Copy link

This pull request has been automatically closed because there has been no activity for 90 days. Please feel free to reopen this pull request (or open a new one) if the proposed change is still appropriate. Thank you for your contribution!

@github-actions github-actions bot closed this Oct 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants