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.

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

rgw: Increase the default number of RGW bucket shards #32660

Merged
merged 7 commits into from Mar 2, 2020

Conversation

cbodley
Copy link
Contributor

@cbodley cbodley commented Jan 15, 2020

each zone in the zonegroup has a "bucket_index_max_shards" field, but the only way to change it is by manually editing the json format using radosgw-admin zonegroup get/set

this change extends the zone modify and zonegroup modify commands to accept the --max-bucket-index-shards option to set this field - either for an individual zone, or for all zones in a given zonegroup

it also pulls in the change from #30875 to raise its default value to 11

@cbodley
Copy link
Contributor Author

cbodley commented Jan 29, 2020

jenkins test make check

@cbodley cbodley changed the title radosgw-admin: allow zone[group] modify to configure index shards rgw: Increase the default number of RGW bucket shards Jan 30, 2020
@cbodley
Copy link
Contributor Author

cbodley commented Feb 3, 2020

@ivancich could you please help review this for octopus?

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.

I have some suggestions that are more on the nit level. Please consider, but I won't hold up the approval.

@@ -350,6 +350,7 @@ void usage()
cout << " set list of zones to sync from\n";
cout << " --sync-from-rm=[zone-name][,...]\n";
cout << " remove zones from list of zones to sync from\n";
cout << " --bucket-index-max-shards override a zone/zonegroup's default bucket index shard count\n";
Copy link
Member

Choose a reason for hiding this comment

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

does this need to indicate that there is a number included in the command-line-argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'm torn on this. some options do specify that they require a certain argument, and others don't. in this case, it would feel overly verbose and need a line break. and if there is confusion, the error message is pretty clear:

$ radosgw-admin zone modify --bucket-index-max-shards
Option --bucket-index-max-shards requires an argument.

src/rgw/rgw_zone.cc Outdated Show resolved Hide resolved
@@ -568,7 +568,7 @@ struct RGWZone {
bool sync_from_all;
set<std::string> sync_from; /* list of zones to sync from */

RGWZone() : log_meta(false), log_data(false), read_only(false), bucket_index_max_shards(0),
RGWZone() : log_meta(false), log_data(false), read_only(false), bucket_index_max_shards(11),
Copy link
Member

Choose a reason for hiding this comment

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

what about making 11 a constexpr?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point! turning this into a constexpr variable also allows us to document it

@@ -245,6 +245,7 @@
set list of zones to sync from
--sync-from-rm=[zone-name][,...]
remove zones from list of zones to sync from
--bucket-index-max-shards override a zone/zonegroup's default bucket index shard count
Copy link
Member

Choose a reason for hiding this comment

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

again, indicate a value is expected

src/rgw/rgw_zone.h Outdated Show resolved Hide resolved
doc/man/8/radosgw-admin.rst Outdated Show resolved Hide resolved
@cbodley cbodley force-pushed the wip-rgw-admin-zone-shards branch 2 times, most recently from 6e80a25 to af65bf0 Compare February 3, 2020 18:49
Mark Nelson and others added 5 commits February 27, 2020 15:12
Signed-off-by: Mark Nelson <mnelson@redhat.com>
Modified-by: Casey Bodley <cbodley@redhat.com>
Signed-off-by: Casey Bodley <cbodley@redhat.com>
Signed-off-by: Casey Bodley <cbodley@redhat.com>
Signed-off-by: Casey Bodley <cbodley@redhat.com>
Signed-off-by: Casey Bodley <cbodley@redhat.com>
@cbodley
Copy link
Contributor Author

cbodley commented Feb 27, 2020

http://pulpito.ceph.com/cbodley-2020-02-27_18:44:45-rgw-wip-cbodley-testing-distro-basic-smithi/

i'm seeing a number of s3test failures around listing sharded buckets:

s3tests_boto3.functional.test_s3.test_bucket_list_delimiter_prefix ... FAIL
s3tests_boto3.functional.test_s3.test_bucket_listv2_delimiter_prefix ... FAIL
s3tests_boto3.functional.test_s3.test_bucket_list_delimiter_prefix_underscore ... FAIL
s3tests_boto3.functional.test_s3.test_bucket_listv2_delimiter_prefix_underscore ... FAIL
s3tests_boto3.functional.test_s3.test_bucket_list_delimiter_not_skip_special ... FAIL

i think these are real bugs that we just haven't hit because we always test with num_shards=1

@ivancich
Copy link
Member

http://pulpito.ceph.com/cbodley-2020-02-27_18:44:45-rgw-wip-cbodley-testing-distro-basic-smithi/

i'm seeing a number of s3test failures around listing sharded buckets:

s3tests_boto3.functional.test_s3.test_bucket_list_delimiter_prefix ... FAIL
s3tests_boto3.functional.test_s3.test_bucket_listv2_delimiter_prefix ... FAIL
s3tests_boto3.functional.test_s3.test_bucket_list_delimiter_prefix_underscore ... FAIL
s3tests_boto3.functional.test_s3.test_bucket_listv2_delimiter_prefix_underscore ... FAIL
s3tests_boto3.functional.test_s3.test_bucket_list_delimiter_not_skip_special ... FAIL

i think these are real bugs that we just haven't hit because we always test with num_shards=1

I suspect you're right. This area of code has been touched by a number of recent PRs.

Would you like me to take a look?

@cbodley
Copy link
Contributor Author

cbodley commented Feb 27, 2020

@ivancich i'll take a stab first and keep you posted

@cbodley
Copy link
Contributor Author

cbodley commented Feb 28, 2020

i pushed a fix to #33628, and i'll test these two prs together

Signed-off-by: Casey Bodley <cbodley@redhat.com>
Signed-off-by: Casey Bodley <cbodley@redhat.com>
@@ -0,0 +1 @@
.qa/rgw_bucket_sharding
Copy link
Member

Choose a reason for hiding this comment

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

Cool!

@cbodley
Copy link
Contributor Author

cbodley commented Mar 2, 2020

jenkins test make check

@cbodley
Copy link
Contributor Author

cbodley commented Mar 2, 2020

http://pulpito.ceph.com/cbodley-2020-02-28_20:21:50-rgw-wip-cbodley-testing-distro-basic-smithi/

some of the many-sharded tests died with osd "out of order op" crashes, but those appear to have been resolved on master with https://tracker.ceph.com/issues/42328

the bucket listing tests are all succeeding now with #33628

@cbodley cbodley merged commit affc79b into ceph:master Mar 2, 2020
@cbodley cbodley deleted the wip-rgw-admin-zone-shards branch March 2, 2020 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants