rgw/sal: list_buckets() returns RGWBucketEnts#50611
Conversation
|
|
f5d8b3f to
d499953
Compare
dang
left a comment
There was a problem hiding this comment.
Seems a good cleanup. Just a question about span
| 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, |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Okay, it just seem redundant to pass in both a vector and a span representing the same data.
ivancich
left a comment
There was a problem hiding this comment.
Code isn't building on ceph-ci. One error is provided.
|
|
||
| class StoreBucket : public Bucket { | ||
| protected: | ||
| RGWBucketEnt ent; |
There was a problem hiding this comment.
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
|
This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved |
d499953 to
3b89846
Compare
|
jenkins test make check |
3b89846 to
259171f
Compare
|
qa looked mostly good, but caused a single failure in the tempest tests: |
| 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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);There was a problem hiding this comment.
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.
1a0db53 to
84eb856
Compare
|
This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved |
84eb856 to
9229ec9
Compare
9229ec9 to
7fbb757
Compare
|
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>
7fbb757 to
16e896a
Compare
16e896a to
1db32f6
Compare
|
jenkins test make check |
1 similar comment
|
jenkins test make check |
|
fyi @idryomov, seeing several different failures in recent rbd unit tests, here and elsewhere: |
|
@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. |
|
jenkins test make check |
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 |
`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>
1db32f6 to
958578a
Compare
|
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
i think this is finally good enough to merge |
sal::User::list_buckets()no longer returns a map ofsal::Buckethandles. it now usesstd::span<RGWBucketEnt>for input and output.RGWBucketEntcontains all of the information we need to satisfy ListBuckets requests, and also stores thergw_bucketkey for use withDriver::get_bucket()where asal::Buckethandle is necessarysal::BucketListcontains the span of results and thenext_marker. theis_truncatedflag was removed in favor of!next_marker.empty()this means that
sal::Bucketno longer needs to wrapRGWBucketEntto support these listings, and can be represented byRGWBucketInfoalone. the bucket stats interfaces that relied onRGWBucketEntinternally now return their result as eitherRGWBucketEntorRGWStorageStatsthe checks for
user->get_max_buckets()on bucket creation now use a paginatedcheck_user_max_buckets()helper function that limits the number of allocated entries torgw_list_buckets_max_chunkShow available Jenkins commands
jenkins retest this pleasejenkins test classic perfjenkins test crimson perfjenkins test signedjenkins test make checkjenkins test make check arm64jenkins test submodulesjenkins test dashboardjenkins test dashboard cephadmjenkins test apijenkins test docsjenkins render docsjenkins test ceph-volume alljenkins test ceph-volume toxjenkins test windows