Skip to content

octopus: rgw: resolve empty ordered bucket listing results w/ CLS filtering *and* bucket index list produces incorrect result when non-ascii entries#45088

Merged
yuriw merged 7 commits intoceph:octopusfrom
dvanders:wip-52076-octopus
May 9, 2022
Merged

Conversation

@dvanders
Copy link
Contributor

@dvanders dvanders commented Feb 18, 2022 •
edited
Loading

@ivancich @dvanders
rgw: bucket index list produces incorrect result when non-ascii entries
7d89d07 
A recent PR that helped address the issue of non-ascii plain entries
didn't cover all the bases, allowing I/O errors to be produced in some
circumstances during a bucket index list (i.e., `radosgw-admin bi list
...`).

This fixes those issue and does some additional clean-up.

Signed-off-by: J. Eric Ivancich <ivancich@redhat.com>
(cherry picked from commit e714f0d)
(cherry picked from commit d3d8df7)
@ivancich @dvanders
rgw: de-conflate shard_id and request_id in CLSRGWConcurrentIO
9843c54 
When using asynchronous (concurrent) IO for bucket index requests,
there are two int ids that are used that need to be kept separate --
shard id and request id. In many cases they're the same -- shard 0
gets request 0, and so forth.

But in preparation for re-requests, those ids can diverge, where
request 13 maps to shard 2. The existing code maintained the OIDs that
went with each request. This PR also maintains the shard id as
well. Documentation has been beefed up to help future developers
navigate this.

Signed-off-by: J. Eric Ivancich <ivancich@redhat.com>
(cherry picked from commit 9606346)

Conflicts:
	src/cls/rgw/cls_rgw_client.cc
	src/cls/rgw/cls_rgw_client.h

In all cases I took the patch code, not mangling anything.
These were all cases of std:: or auto type.
@ivancich @dvanders
rgw: allow CLSRGWConcurrentIO to handle "advancing" retries
5b44166 
When doing an asynchronous/concurrent bucket index operation against
multiple bucket index shards, a special error code is set aside to
indicate that an "advancing" retry of a/some shard(s) is necessary. In
that case another asynchronous call is made on the indicated shard(s)
from the client (i.e., CLSRGWConcurrentIO).  It is up to the subclass
of CLSRGWConcurrentIO to handle the retry such that it "advances" and
simply doesn't get stuck, looping forever.

The retry functionality only works when the "need_multiple_rounds"
functionality is not in use.

Signed-off-by: J. Eric Ivancich <ivancich@redhat.com>
(cherry picked from commit 5d28307)

Conflicts:
	src/cls/rgw/cls_rgw_client.cc
	src/cls/rgw/cls_rgw_client.h

Resolved by taking the patch version -- all cases of auto type and std::
@ivancich @dvanders
rgw: allow ordered bucket listing to work when many filtered out entries
6b6c069 
A previous PR moved the much of the filtering that's part of bucket
listing to the CLS layer. One unanticipated result was that it is now
possible for a call to return 0 entries. In such a case we want to
retry the call with the marker moved forward (i.e., advanced),
repeatedly if necessary, in order to either retrieve some entries or
to hit the end of the entries. This PR adds that functionality.

Signed-off-by: J. Eric Ivancich <ivancich@redhat.com>
(cherry picked from commit 423c183)

Conflicts:
	src/cls/rgw/cls_rgw_ops.h
s/ceph::buffer::list/bufferlist/g
@dvanders dvanders added this to the octopus milestone Feb 18, 2022
@dvanders dvanders added the rgw label Feb 18, 2022
@dvanders
Copy link
Contributor Author

dvanders commented Feb 18, 2022

This includes the single commit from #44858.

@dvanders dvanders requested a review from ivancich February 18, 2022 21:33
@ivancich @dvanders
rgw: fix bucket index list minor calculation bug
70ba44d 
When "bucket index list" traverses the different regions in the bucket
index assembling the output, it miscalculates how many entries to ask
for at one point. This fixes that.

This fixes previous "rgw: bucket index list can produce I/O errors".

Credit for finding this bug goes to Soumya Koduri <skoduri@redhat.com>.

Signed-off-by: J. Eric Ivancich <ivancich@redhat.com>
(cherry picked from commit aa76051)
@dvanders
Copy link
Contributor Author

dvanders commented Feb 18, 2022

@ivancich as mentioned in https://tracker.ceph.com/issues/51462#note-7 we started to get bucket listing problems after upgrading to octopus. So I'm trying to collect here the backports needed to bring octopus up to the state of the art in cls_rgw bucket listing. Does this look complete and safe to try?

@dvanders dvanders mentioned this pull request Feb 23, 2022
Closed
14 tasks
@dvanders dvanders changed the title octopus: rgw: resolve empty ordered bucket listing results w/ CLS filtering octopus: rgw: resolve empty ordered bucket listing results w/ CLS filtering *and* bucket index list produces incorrect result when non-ascii entries Feb 23, 2022
@dvanders
Copy link
Contributor Author

dvanders commented Feb 23, 2022

@ivancich as mentioned in https://tracker.ceph.com/issues/51462#note-7 we started to get bucket listing problems after upgrading to octopus. So I'm trying to collect here the backports needed to bring octopus up to the state of the art in cls_rgw bucket listing. Does this look complete and safe to try?

We tested this PR on our cluster with bucket listing. It fixes!

@cbodley
Copy link
Contributor

cbodley commented Feb 28, 2022

https://pulpito.ceph.com/yuriw-2022-02-24_22:51:58-rgw-wip-yuri10-testing-2022-02-24-1329-octopus-distro-default-smithi/ shows 3 jobs failing in ceph_test_cls_rgw:

[ RUN      ] cls_rgw.bi_list
unknown file: Failure
C++ exception with description "std::bad_alloc" thrown in the test body.
[  FAILED  ] cls_rgw.bi_list (83 ms)

the exception is thrown in the cls_rgw_client code (not the cls_rgw part running in the osd). in the past, i've seen bad_alloc errors from logic errors in decode, like when it tries to decode a string, finds a really big length prefix, and tries to allocate that much memory. but i don't see any obvious mistakes in rgw_cls_list_ret here

@cbodley
Copy link
Contributor

cbodley commented Feb 28, 2022

so far it's unclear whether this also happens in the pacific backport, we didn't get a clean run there due to issues around centos 8 eol https://pulpito.ceph.com/yuriw-2022-02-24_20:49:23-rgw-wip-yuri2-testing-2022-02-23-1524-pacific-distro-default-smithi/

@dvanders
Copy link
Contributor Author

dvanders commented Feb 28, 2022 •
edited
Loading

@cbodley I think we're missing this: 1bf0581

Can we re-qa it with that?
I'll push it to the pacific PR too.

@ivancich @dvanders
test/rgw: fix and add to rgw/cls bi_list tests
dccf0a6 
Make sure marker is cleared. Put end-of-list check inside the
conditional with the rest of the test. Add some additional testing.

Signed-off-by: J. Eric Ivancich <ivancich@redhat.com>
(cherry picked from commit 1bf0581)
@dvanders
Copy link
Contributor Author

dvanders commented Mar 1, 2022

jenkins retest this please

@cbodley
Copy link
Contributor

cbodley commented Mar 1, 2022

thanks @dvanders, worth a shot!

i tried reproducing this in an old focal vm, but the test passes consistently there. so i guess it's somehow specific to centos?

@dvanders
Copy link
Contributor Author

dvanders commented Mar 1, 2022

jenkins test api

@GillesMocellin
Copy link

GillesMocellin commented Mar 2, 2022

Argh ! v15.2.16 is just released, and this PR is not in ;-(

@cbodley
Copy link
Contributor

cbodley commented Mar 3, 2022

Argh ! v15.2.16 is just released, and this PR is not in ;-(

@GillesMocellin i'm afraid that Ceph no longer have much of a backport team, and rgw has not been getting timely backports as a result. if you're interested in getting involved, you can find some resources in https://tracker.ceph.com/projects/ceph-releases/wiki and https://github.com/ceph/ceph/blob/master/SubmittingPatches-backports.rst

(and thanks to @dvanders for working on this one)

@ivancich @dvanders
rgw: fix bucket index listing count bug
4def14d 
Fix bugs surrounding calculation of number of entries returned and
whether the end of a listing range has been reached.

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

dvanders commented Mar 9, 2022 •
edited
Loading

@ivancich I have added the missing commit from #42836 here now too.
The logging changes don't apply cleanly so I'll leave them out unless you insist.

@cbodley
Copy link
Contributor

cbodley commented Mar 9, 2022

@yuriw sorry, i had requested a rerun of this PR but a new commit was added since then. this applies to #45087 also

@ivancich
Copy link
Member

ivancich commented Mar 9, 2022

@ivancich I have added the missing commit from #42836 here now too. The logging changes don't apply cleanly so I'll leave them out unless you insist.

I agree, the logging changes are not that important. Thank you, @dvanders !

@dvanders
Copy link
Contributor Author

dvanders commented Mar 10, 2022

jenkins test api

@yuriw yuriw modified the milestones: octopus, pacific Mar 16, 2022
@yuriw yuriw merged commit 3bdf9a2 into ceph:octopus May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ivancich ivancich ivancich approved these changes

Assignees

No one assigned

Labels

bug-fix feature needs-qa rgw wip-yuri3-testing

Projects

None yet

Milestone

octopus

Development

Successfully merging this pull request may close these issues.

5 participants

@dvanders @cbodley @GillesMocellin @ivancich @yuriw