rgw/sal: list_buckets() returns RGWBucketEnts #50611

cbodley · 2023-03-21T17:56:11Z

sal::User::list_buckets() no longer returns a map of sal::Bucket handles. it now uses std::span<RGWBucketEnt> for input and output. RGWBucketEnt contains all of the information we need to satisfy ListBuckets requests, and also stores the rgw_bucket key for use with Driver::get_bucket() where a sal::Bucket handle is necessary

sal::BucketList contains the span of results and the next_marker. the is_truncated flag was removed in favor of !next_marker.empty()

this means that sal::Bucket no longer needs to wrap RGWBucketEnt to support these listings, and can be represented by RGWBucketInfo alone. the bucket stats interfaces that relied on RGWBucketEnt internally now return their result as either RGWBucketEnt or RGWStorageStats

the checks for user->get_max_buckets() on bucket creation now use a paginated check_user_max_buckets() helper function that limits the number of allocated entries to rgw_list_buckets_max_chunk

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows

cbodley · 2023-03-21T18:15:29Z

~~sorry for the massive commit. i'll try to split the User::list_buckets() change from the one that removes RGWBucketEnt from StoreBucket~~

dang

Seems a good cleanup. Just a question about span

dang · 2023-03-22T14:25:14Z

src/rgw/driver/rados/rgw_sal_rados.cc

-			       const std::string& end_marker, uint64_t max, bool need_stats,
-			       BucketList &buckets, optional_yield y)
+                            const std::string& end_marker, bool need_stats,
+                            std::span<RGWBucketEnt> storage,


So is the point of using both span<> and storage just that the resulting length of the span<> can be different? If so, storage would only be truncated at the end, so it won't result in more allocations, and you can just pass a marker reference. Or, vector has a distinction between size and capacity, so that could be used. (I've never dealt with span<> before, so I'm curious)

thanks for the review!

std::span is new in c++20, and represents a non-owning reference to a contiguous range. it's similar to std::string_view, except it can either be a mutable span<T> or an immutable span<const T>

i like using it in interfaces because it's very flexible; the caller can either point directly at an array on the stack, or to a preallocated vector. i used this pattern with sal::ConfigStore and was happy with the result. in most cases like get_zones_pool_set(), i was able to use a std::array for the storage. i ended up using std::vector for all of the list_buckets() callers just because we have a config option rgw_list_buckets_max_chunk that can vary

and you're right that BucketList::buckets is a separate span because we may return fewer entries than the requested storage.size(). we might instead return the number of actual entries, but i think span is nice there too because it allows the caller to loop over the results directly with for (auto& e : listing.buckets)

Okay, it just seem redundant to pass in both a vector and a span representing the same data.

ivancich

Code isn't building on ceph-ci. One error is provided.

ivancich · 2023-06-21T17:21:34Z

src/rgw/rgw_sal_store.h

@@ -71,7 +71,6 @@ class StoreUser : public User {

 class StoreBucket : public Bucket {
  protected:
-    RGWBucketEnt ent;


I'm trying to do a QA run, and I'm getting compiler errors because ent was removed and there are still references to it.

/home/jenkins-build/build/workspace/ceph-dev-new-build/ARCH/x86_64/AVAILABLE_ARCH/x86_64/AVAILABLE_DIST/centos9/DIST/centos9/MACHINE_SIZE/gigantic/release/18.0.0-4522-gc1b6c062/rpm/el9/BUILD/ceph-18.0.0-4522-gc1b6c062/src/rgw/rgw_sal_store.h:104:7: error: ‘ent’ was not declared in this scope; did you mean ‘int’? 104 | ent.count = _count; | ^~~ | int

cbodley · 2023-06-21T17:26:44Z

Code isn't building on ceph-ci. One error is provided.

thanks @ivancich! i'll rebase this once #50206 merges

github-actions · 2023-06-22T00:21:27Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

cbodley · 2023-08-03T18:24:47Z

jenkins test make check

cbodley · 2023-08-08T12:11:38Z

qa looked mostly good, but caused a single failure in the tempest tests:

2023-08-04T14:45:09.977 INFO:teuthology.orchestra.run.smithi092.stdout:{0} tempest.api.object_storage.test_account_services.AccountTest.test_list_containers_with_limit_and_marker [0.058874s] ... FAILED
2023-08-04T14:45:09.978 INFO:teuthology.orchestra.run.smithi092.stdout:
2023-08-04T14:45:09.978 INFO:teuthology.orchestra.run.smithi092.stdout:Captured traceback:
2023-08-04T14:45:09.978 INFO:teuthology.orchestra.run.smithi092.stdout:~~~~~~~~~~~~~~~~~~~
2023-08-04T14:45:09.978 INFO:teuthology.orchestra.run.smithi092.stdout:    Traceback (most recent call last):
2023-08-04T14:45:09.978 INFO:teuthology.orchestra.run.smithi092.stdout:
2023-08-04T14:45:09.978 INFO:teuthology.orchestra.run.smithi092.stdout:      File "/home/ubuntu/cephtest/tempest/tempest/api/object_storage/test_account_services.py", line 225, in test_list_containers_with_limit_and_marker
2023-08-04T14:45:09.979 INFO:teuthology.orchestra.run.smithi092.stdout:    self.assertLessEqual(len(container_list), limit,
2023-08-04T14:45:09.979 INFO:teuthology.orchestra.run.smithi092.stdout:
2023-08-04T14:45:09.979 INFO:teuthology.orchestra.run.smithi092.stdout:      File "/usr/lib/python3.10/unittest/case.py", line 1238, in assertLessEqual
2023-08-04T14:45:09.979 INFO:teuthology.orchestra.run.smithi092.stdout:    self.fail(self._formatMessage(msg, standardMsg))
2023-08-04T14:45:09.979 INFO:teuthology.orchestra.run.smithi092.stdout:
2023-08-04T14:45:09.979 INFO:teuthology.orchestra.run.smithi092.stdout:      File "/usr/lib/python3.10/unittest/case.py", line 675, in fail
2023-08-04T14:45:09.980 INFO:teuthology.orchestra.run.smithi092.stdout:    raise self.failureException(msg)
2023-08-04T14:45:09.980 INFO:teuthology.orchestra.run.smithi092.stdout:
2023-08-04T14:45:09.980 INFO:teuthology.orchestra.run.smithi092.stdout:    AssertionError: 6 not less than or equal to 0 : [b"tempest-b'a'--260816155", b"tempest-b'b'--282857927", b"tempest-b'c'--1651578176", b"tempest-b'd'--960450078", b"tempest-b'e'--1109719051", b"tempest-b'f'--656717176"]

dang · 2023-08-21T13:52:13Z

src/rgw/driver/rados/rgw_user.cc

-      ret = it->second->remove_bucket(dpp, true, false, nullptr, y);
+    for (const auto& ent : listing.buckets) {
+      std::unique_ptr<rgw::sal::Bucket> bucket;
+      ret = driver->get_bucket(dpp, user, ent.bucket, &bucket, y);


This now double-loads the bucket, since get_bucket() will load it and remove_bucket() will load it. Not sure it makes much of a difference, but I thought I'd mention it.

thanks @dang. it's probably not a big deal, as long as the metadata cache is enabled. do you think it's worth switching this to the non-loading overload that takes RGWBucketInfo? ie

std::unique_ptr<rgw::sal::Bucket> bucket; const RGWBucketInfo info{.bucket = ent.bucket}; ret = driver->get_bucket(user, info, &bucket);

In this particular case, it may be, since bucket is only use do call remove_bucket(), and that does a load as the first operation.

github-actions · 2023-09-13T14:17:31Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

github-actions · 2023-09-21T17:35:53Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

callers use Bucket::read_stats() to load bucket stats Signed-off-by: Casey Bodley <cbodley@redhat.com>

Signed-off-by: Casey Bodley <cbodley@redhat.com>

cbodley · 2023-09-21T22:37:25Z

jenkins test make check

cbodley · 2023-09-22T13:37:05Z

jenkins test make check

cbodley · 2023-09-22T15:34:49Z

fyi @idryomov, seeing several different failures in recent rbd unit tests, here and elsewhere:

idryomov · 2023-09-22T16:01:42Z

@cbodley These are known, they pop up from time to time in Jenkins but I haven't had time to squash them for good. None of them are recent -- at least two of them definitely predate me taking over RBD.

idryomov · 2023-09-22T16:01:47Z

jenkins test make check

cbodley · 2023-09-22T16:26:25Z

qa looked mostly good, but caused a single failure in the tempest tests:

2023-08-04T14:45:09.977 INFO:teuthology.orchestra.run.smithi092.stdout:{0} tempest.api.object_storage.test_account_services.AccountTest.test_list_containers_with_limit_and_marker [0.058874s] ... FAILED
2023-08-04T14:45:09.978 INFO:teuthology.orchestra.run.smithi092.stdout:
2023-08-04T14:45:09.978 INFO:teuthology.orchestra.run.smithi092.stdout:Captured traceback:
2023-08-04T14:45:09.978 INFO:teuthology.orchestra.run.smithi092.stdout:~~~~~~~~~~~~~~~~~~~
2023-08-04T14:45:09.978 INFO:teuthology.orchestra.run.smithi092.stdout:    Traceback (most recent call last):
2023-08-04T14:45:09.978 INFO:teuthology.orchestra.run.smithi092.stdout:
2023-08-04T14:45:09.978 INFO:teuthology.orchestra.run.smithi092.stdout:      File "/home/ubuntu/cephtest/tempest/tempest/api/object_storage/test_account_services.py", line 225, in test_list_containers_with_limit_and_marker
2023-08-04T14:45:09.979 INFO:teuthology.orchestra.run.smithi092.stdout:    self.assertLessEqual(len(container_list), limit,
2023-08-04T14:45:09.979 INFO:teuthology.orchestra.run.smithi092.stdout:
2023-08-04T14:45:09.979 INFO:teuthology.orchestra.run.smithi092.stdout:      File "/usr/lib/python3.10/unittest/case.py", line 1238, in assertLessEqual
2023-08-04T14:45:09.979 INFO:teuthology.orchestra.run.smithi092.stdout:    self.fail(self._formatMessage(msg, standardMsg))
2023-08-04T14:45:09.979 INFO:teuthology.orchestra.run.smithi092.stdout:
2023-08-04T14:45:09.979 INFO:teuthology.orchestra.run.smithi092.stdout:      File "/usr/lib/python3.10/unittest/case.py", line 675, in fail
2023-08-04T14:45:09.980 INFO:teuthology.orchestra.run.smithi092.stdout:    raise self.failureException(msg)
2023-08-04T14:45:09.980 INFO:teuthology.orchestra.run.smithi092.stdout:
2023-08-04T14:45:09.980 INFO:teuthology.orchestra.run.smithi092.stdout:    AssertionError: 6 not less than or equal to 0 : [b"tempest-b'a'--260816155", b"tempest-b'b'--282857927", b"tempest-b'c'--1651578176", b"tempest-b'd'--960450078", b"tempest-b'e'--1109719051", b"tempest-b'f'--656717176"]

most passing qa in https://pulpito.ceph.com/cbodley-2023-09-22_13:12:11-rgw-wip-rgw-sal-list-buckets-distro-default-smithi/ but still hitting this tempest failure. it seems to happen only when limit = random.randint(0, self.containers_count - 1) returns limit=0 and we treat 0 as no limit

`sal::User::list_buckets()` no longer returns a map of `sal::Bucket` handles. it now uses `std::span<RGWBucketEnt>` for input and output. `RGWBucketEnt` contains all of the information we need to satisfy ListBuckets requests, and also stores the `rgw_bucket` key for use with `Driver::get_bucket()` where a `sal::Bucket` handle is necessary `sal::BucketList` contains the span of results and the `next_marker`. the `is_truncated` flag was removed in favor of `!next_marker.empty()` the checks for `user->get_max_buckets()` on bucket creation now use a paginated `check_user_max_buckets()` helper function that limits the number of allocated entries to `rgw_list_buckets_max_chunk` Signed-off-by: Casey Bodley <cbodley@redhat.com>

`sal::Bucket` no longer needs to wrap `RGWBucketEnt` to support user bucket listings, so can be represented by `RGWBucketInfo` alone. the bucket stats interfaces that relied on RGWBucketEnt internally now return their result as either `RGWBucketEnt` or `RGWStorageStats` Signed-off-by: Casey Bodley <cbodley@redhat.com>

cbodley · 2023-09-22T21:47:36Z

the rerun in https://pulpito.ceph.com/cbodley-2023-09-22_20:53:19-rgw-wip-rgw-sal-list-buckets-distro-default-smithi/ passed the tempest job, but i can't tell whether it used limit=0. i was able to reproduce that issue and confirm that it's now fixed by commit rgw/sal: list_buckets() returns RGWBucketEnts using this curl command:

curl -i http://localhost:8000/swift/v1?format=json\&marker=container\&limit=0 -X GET -H "X-Auth-Token: AUTH_rgwtk0b000000746573743a7465737465721429e6a420c1a92f40100f65c63dce0b2740fdc449d151fa3323c28073815b3acd13838d" -H "Accept-Encoding: gzip"

i think this is finally good enough to merge

cbodley added cleanup rgw labels Mar 21, 2023

cbodley requested review from a team as code owners March 21, 2023 17:56

github-actions bot added documentation tests labels Mar 21, 2023

cbodley force-pushed the wip-rgw-sal-list-buckets branch 2 times, most recently from f5d8b3f to d499953 Compare March 21, 2023 19:05

dang reviewed Mar 22, 2023

View reviewed changes

dang approved these changes Mar 22, 2023

View reviewed changes

cbodley added the needs-qa label Apr 18, 2023

ivancich added the wip-eric-testing-1 for ivancich testing label Jun 21, 2023

ivancich requested changes Jun 21, 2023

View reviewed changes

ivancich added needs-review and removed needs-qa wip-eric-testing-1 for ivancich testing labels Jun 21, 2023

github-actions bot added the needs-rebase label Jun 22, 2023

cbodley force-pushed the wip-rgw-sal-list-buckets branch from d499953 to 3b89846 Compare August 1, 2023 16:26

cbodley requested a review from ivancich August 1, 2023 16:26

github-actions bot removed the needs-rebase label Aug 1, 2023

cbodley added needs-qa and removed needs-review labels Aug 1, 2023

cbodley added the wip-cbodley-testing label Aug 3, 2023

cbodley force-pushed the wip-rgw-sal-list-buckets branch from 3b89846 to 259171f Compare August 3, 2023 21:56

cbodley removed the needs-qa label Aug 8, 2023

dang approved these changes Aug 21, 2023

View reviewed changes

cbodley force-pushed the wip-rgw-sal-list-buckets branch 4 times, most recently from 1a0db53 to 84eb856 Compare August 23, 2023 17:23

github-actions bot added the needs-rebase label Sep 13, 2023

cbodley force-pushed the wip-rgw-sal-list-buckets branch from 84eb856 to 9229ec9 Compare September 18, 2023 17:22

github-actions bot removed the needs-rebase label Sep 18, 2023

cbodley force-pushed the wip-rgw-sal-list-buckets branch from 9229ec9 to 7fbb757 Compare September 18, 2023 22:07

github-actions bot added the needs-rebase label Sep 21, 2023

cbodley added 2 commits September 21, 2023 13:37

rgw: remove Bucket::update_container_stats()

d9dfd38

callers use Bucket::read_stats() to load bucket stats Signed-off-by: Casey Bodley <cbodley@redhat.com>

rgw/admin: 'buckets list' takes --marker

b75cc5c

Signed-off-by: Casey Bodley <cbodley@redhat.com>

cbodley force-pushed the wip-rgw-sal-list-buckets branch from 7fbb757 to 16e896a Compare September 21, 2023 17:38

github-actions bot removed the needs-rebase label Sep 21, 2023

cbodley force-pushed the wip-rgw-sal-list-buckets branch from 16e896a to 1db32f6 Compare September 21, 2023 20:11

cbodley added 2 commits September 22, 2023 12:49

cbodley force-pushed the wip-rgw-sal-list-buckets branch from 1db32f6 to 958578a Compare September 22, 2023 17:06

cbodley added the TESTED label Sep 22, 2023

cbodley merged commit 1378703 into ceph:main Sep 22, 2023
10 of 11 checks passed

cbodley deleted the wip-rgw-sal-list-buckets branch September 22, 2023 22:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rgw/sal: list_buckets() returns RGWBucketEnts #50611

rgw/sal: list_buckets() returns RGWBucketEnts #50611

cbodley commented Mar 21, 2023

cbodley commented Mar 21, 2023 •

edited

dang left a comment

dang Mar 22, 2023

cbodley Mar 22, 2023

dang Mar 22, 2023

ivancich left a comment

ivancich Jun 21, 2023

cbodley commented Jun 21, 2023

github-actions bot commented Jun 22, 2023

cbodley commented Aug 3, 2023

cbodley commented Aug 8, 2023

dang Aug 21, 2023

cbodley Aug 21, 2023

dang Aug 21, 2023

github-actions bot commented Sep 13, 2023

github-actions bot commented Sep 21, 2023

cbodley commented Sep 21, 2023

cbodley commented Sep 22, 2023

cbodley commented Sep 22, 2023

idryomov commented Sep 22, 2023

idryomov commented Sep 22, 2023

cbodley commented Sep 22, 2023

cbodley commented Sep 22, 2023

rgw/sal: list_buckets() returns RGWBucketEnts #50611

rgw/sal: list_buckets() returns RGWBucketEnts #50611

Conversation

cbodley commented Mar 21, 2023

cbodley commented Mar 21, 2023 • edited

dang left a comment

Choose a reason for hiding this comment

dang Mar 22, 2023

Choose a reason for hiding this comment

cbodley Mar 22, 2023

Choose a reason for hiding this comment

dang Mar 22, 2023

Choose a reason for hiding this comment

ivancich left a comment

Choose a reason for hiding this comment

ivancich Jun 21, 2023

Choose a reason for hiding this comment

cbodley commented Jun 21, 2023

github-actions bot commented Jun 22, 2023

cbodley commented Aug 3, 2023

cbodley commented Aug 8, 2023

dang Aug 21, 2023

Choose a reason for hiding this comment

cbodley Aug 21, 2023

Choose a reason for hiding this comment

dang Aug 21, 2023

Choose a reason for hiding this comment

github-actions bot commented Sep 13, 2023

github-actions bot commented Sep 21, 2023

cbodley commented Sep 21, 2023

cbodley commented Sep 22, 2023

cbodley commented Sep 22, 2023

idryomov commented Sep 22, 2023

idryomov commented Sep 22, 2023

cbodley commented Sep 22, 2023

cbodley commented Sep 22, 2023

cbodley commented Mar 21, 2023 •

edited