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.

Sign up for GitHub

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

Jump to bottom

rgw: Check all shards for user manifest parts #37734

Merged
merged 1 commit into from Nov 20, 2020
Merged

rgw: Check all shards for user manifest parts #37734

merged 1 commit into from Nov 20, 2020

Conversation

matthewoliver
Copy link
Contributor

When we go and read an user manifest LO (or a DLO in the swift api)
there is a bug where we supply a defaulted params object that defaults
shard_id to 0. This means that when issueing a GET to the LO it'll only
check shard 0 for parts.

This is causing errors in tempest tests, and is obviously wrong.

This patch changes the shard_id to -1, so all shards are checked for
parts and therefore complete LO's are returned.

Fixes: https://tracker.ceph.com/issues/47801
Fixes: https://tracker.ceph.com/issues/47800
Signed-off-by: Matthew Oliver moliver@suse.com

Checklist

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug
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 api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

@matthewoliver
Copy link
Contributor Author

The other option could be to just default the params.shard_id to -1, so by default it'll search them all. Something like:

diff --git a/src/rgw/rgw_sal.h b/src/rgw/rgw_sal.h
index fc7dd911fe..882d562e12 100644
--- a/src/rgw/rgw_sal.h
+++ b/src/rgw/rgw_sal.h
@@ -154,7 +154,7 @@ class RGWBucket {
       RGWAccessListFilter *filter{nullptr};
       bool list_versions{false};
       bool allow_unordered{false};
-      int shard_id{0};
+      int shard_id{-1};
     };
     struct ListResults {
       vector<rgw_bucket_dir_entry> objs;

But not 100% sure of the fall out of this, so went conservative and fixed it where this bug was occuring.

@dang
Copy link
Contributor

dang commented Oct 21, 2020

This second one is a better idea. Let's do that instead.

@dang dang self-requested a review October 21, 2020 12:51
cbodley
cbodley reviewed Oct 21, 2020
View reviewed changes
src/rgw/rgw_op.cc Outdated
@@ -1615,6 +1615,7 @@ static int iterate_user_manifest_parts(CephContext * const cct,
rgw::sal::RGWBucket::ListParams params;
params.prefix = obj_prefix;
params.delim = delim;
params.shard_id = -1;
Copy link
Contributor

Choose a reason for hiding this comment

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

it feels like this should be the default behavior for ListParams, doesn't it? can we update the value int shard_id{0}; where it's declared in rgw_sal.h?

cc @dang

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, you guys are already on it! thanks

RGW: Check all shards for user manifest parts
e0d67a1 
When we go and read an user manifest LO (or a DLO in the swift api)
there is a bug where we supply a defaulted params object that defaults
shard_id to 0. This means that when issueing a GET to the LO it'll only
check shard 0 for parts.

This is causing errors in tempest tests, and is obviously wrong.

This patch defaults the ListParams.shard_id to -1, so by default
all shards are checked for parts and therefore complete LO's
are returned.

Fixes: https://tracker.ceph.com/issues/47801
Fixes: https://tracker.ceph.com/issues/47800
Signed-off-by: Matthew Oliver <moliver@suse.com>
@matthewoliver
Copy link
Contributor Author

Done, changed to option 2 :)

@dang
Copy link
Contributor

dang commented Oct 22, 2020

jenkins test make check

@harishmunjulur
Copy link
Contributor

wip-hmunjulu01-testing-2020-11-03-1721

@harishmunjulur
Copy link
Contributor

wip-hmunjulu01-testing-2020-11-05-2002

@cbodley
Copy link
Contributor

cbodley commented Nov 12, 2020

@harishmunjulur do we have any test results here?

@smithfarm smithfarm changed the title RGW: Check all shards for user manifest parts rgw: Check all shards for user manifest parts Nov 13, 2020
@smithfarm
Copy link
Contributor

@cbodley Is this eligible for backporting? If so, to which stable releases?

@cbodley
Copy link
Contributor

cbodley commented Nov 13, 2020

@smithfarm no, it was a regression from the zipper refactoring project, which is only targeted for the pacific release

@cbodley cbodley merged commit c2a1edb into ceph:master Nov 20, 2020
@cbodley
Copy link
Contributor

cbodley commented Nov 20, 2020

thanks @matthewoliver!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cbodley cbodley cbodley left review comments

@dang dang dang approved these changes

Assignees
No one assigned
Labels
needs-qa rgw
Projects
None yet
Milestone
No milestone
5 participants
@matthewoliver @dang @harishmunjulur @cbodley @smithfarm