Skip to content

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

Merged
yuriw merged 9 commits intoceph:pacificfrom
dvanders:wip-52075-pacific
Mar 11, 2022

Conversation

@dvanders
Copy link
Contributor

@dvanders dvanders commented Feb 18, 2022

@dvanders dvanders added this to the pacific milestone Feb 18, 2022
@dvanders dvanders added the rgw label Feb 18, 2022
@github-actions github-actions bot added the tests label Feb 18, 2022
@dvanders dvanders added the DNM label Feb 18, 2022
@dvanders
Copy link
Contributor Author

To do the backport cleanly I needed the commit from #44815, which is currently DNM, so I marked this DNM too.

@dvanders dvanders requested a review from ivancich February 18, 2022 21:14
@dvanders dvanders removed the DNM label Feb 23, 2022
@dvanders dvanders changed the title pacific: rgw: resolve empty ordered bucket listing results w/ CLS filtering pacific: 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

jenkins retest this please

@dvanders
Copy link
Contributor Author

dvanders commented Mar 1, 2022

This was missing a fixed bi_list test. Re-qa it please. (I've rebased on latest pacific)

ivancich added 6 commits March 1, 2022 11:31
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 3473bf3)
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)
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)
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)
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)
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 dvanders force-pushed the wip-52075-pacific branch from c561b3e to 0d103e7 Compare March 1, 2022 10:31
@cfsnyder
Copy link
Contributor

cfsnyder commented Mar 9, 2022

@dvanders looks like this should also include this follow-up fix: fa90bc0

@ivancich
Copy link
Member

ivancich commented Mar 9, 2022

@dvanders : Wondering if you could incorporate some fixes from #42836 , particularly this commit: fa90bc0 .

[edited to add: @cfsnyder and I were discussing this on irc, which is why you got to requests in quick succession]

Copy link
Member

@ivancich ivancich left a comment

Choose a reason for hiding this comment

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

This looks good. Thank you for undertaking the effort! As mentioned in another comment, adding one more commit would fix some additional bugs and would save us another PR. Thank you for considering!

ivancich added 3 commits March 9, 2022 18:15
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>
…stent

This provides more thorough and consistent function tracing in CLS/RGW
when logging is set to 10 or higher.

Signed-off-by: J. Eric Ivancich <ivancich@redhat.com>
Adding more level 20 logging for bucket index listing.

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

dvanders commented Mar 9, 2022

This looks good. Thank you for undertaking the effort! As mentioned in another comment, adding one more commit would fix some additional bugs and would save us another PR. Thank you for considering!

Done, I also brought in a dependent logging change to make 5b8af18 apply cleanly.

@dvanders
Copy link
Contributor Author

dvanders commented Mar 9, 2022

jenkins retest this please

@yuriw yuriw merged commit 84a4dc6 into ceph:pacific Mar 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants