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: add bucket size limit check to radosgw-admin #11796

Merged
merged 1 commit into from Apr 20, 2017

Conversation

Projects
None yet
6 participants
@mattbenjamin
Contributor

mattbenjamin commented Nov 5, 2016

http://tracker.ceph.com/issues/17925

The change adds a new list of all buckets * all users, with
fields for bucket name, tenant name, current num_objects,
current num_shards, current objects per shard, and the
corresponding fill_status--the latter consisting of 'OK',
'WARN %', or 'OVER %.'

The warning check is relative to two new tunables. The threshold
max objects per shard is set as rgw_bucket_safe_max_objects_per_shard,
which defaults to 100K. The value rgw_bucket_warning_threshold is
a percent of the current safe max at which to warn (defaults to
10).

Signed-off-by: Matt Benjamin mbenjamin@redhat.com

@mattbenjamin

This comment has been minimized.

Contributor

mattbenjamin commented Nov 5, 2016

@yehudasa could you give this a look over? tx

if (opt_cmd == OPT_BUCKETS_LIMIT_CHECK) {
void *handle;
metadata_key = "user";

This comment has been minimized.

@yehudasa

yehudasa Nov 5, 2016

Member

indentation?

This comment has been minimized.

@mattbenjamin

mattbenjamin Nov 5, 2016

Contributor

ack

This comment has been minimized.

@mattbenjamin
uint16_t shard_warn_pct =
store->ctx()->_conf->rgw_shard_warning_threshold;
if (shard_warn_pct > 100)
shard_warn_pct = 10;

This comment has been minimized.

@yehudasa

yehudasa Nov 5, 2016

Member

is it 10 on purpose here? If > 100 then reset to 100 I think

This comment has been minimized.

@mattbenjamin

mattbenjamin Nov 5, 2016

Contributor

ok, that's what I had originally, then changed it...

This comment has been minimized.

@yehudasa

yehudasa Nov 28, 2016

Member

@mattbenjamin should probably set it to 90

string marker;
bool is_truncated;
ret = rgw_read_user_buckets(store, user_id, buckets,

This comment has been minimized.

@yehudasa

yehudasa Nov 5, 2016

Member

@mattbenjamin need to make sure that this works correctly with users under non-default tenants

This comment has been minimized.

@mattbenjamin
&master_ver, stats, nullptr);
if (! ret) {
for (const auto& s : stats) {
num_objects = s.second.num_objects;

This comment has been minimized.

@yehudasa

yehudasa Nov 5, 2016

Member

num_objects += s.second.num_objects;

This comment has been minimized.

@mattbenjamin

mattbenjamin Nov 5, 2016

Contributor

ack

if (safe_fill_pct <= shard_warn_pct) {
ss << boost::format("WARN %4f%%") % safe_fill_pct;
} else {
ss << "OK";

This comment has been minimized.

@yehudasa

yehudasa Nov 5, 2016

Member

@mattbenjamin maybe we need to have two modes of operations? one that just dumps everything (like here), another one that just dumps warnings and errors?

This comment has been minimized.

@mattbenjamin
* limit-check on each group */
do {
std::list<std::string> user_ids;
ret = store->meta_mgr->list_keys_next(handle, max, user_ids,

This comment has been minimized.

@yehudasa

yehudasa Nov 5, 2016

Member

@mattbenjamin maybe if uid is passed in the command line, just show stats of a specific user?

This comment has been minimized.

@mattbenjamin

mattbenjamin Nov 5, 2016

Contributor

yeah, ok

This comment has been minimized.

@mattbenjamin
@ktdreyer

This comment has been minimized.

Member

ktdreyer commented Nov 7, 2016

If we plan to backport this to Jewel, we'll need a ticket at http://tracker.ceph.com/

@mattbenjamin

This comment has been minimized.

Contributor

mattbenjamin commented Nov 15, 2016

@ktdreyer Ok; @yehudasa is this version ready for merge, removing DNM?

@mattbenjamin mattbenjamin changed the title from [DNM] rgw: add bucket size limit check to radosgw-admin to rgw: add bucket size limit check to radosgw-admin Nov 15, 2016

@@ -1518,6 +1518,10 @@ OPTION(rgw_realm_reconfigure_delay, OPT_DOUBLE, 2) // seconds to wait before rel
OPTION(rgw_period_push_interval, OPT_DOUBLE, 2) // seconds to wait before retrying "period push"
OPTION(rgw_period_push_interval_max, OPT_DOUBLE, 30) // maximum interval after exponential backoff
OPTION(rgw_safe_max_objects_per_shard, OPT_INT, 100*1024) // safe max loading
OPTION(rgw_shard_warning_threshold, OPT_DOUBLE, 10) // pct of safe max

This comment has been minimized.

@yehudasa

yehudasa Nov 15, 2016

Member

@mattbenjamin let's change this configurable to be the inverse of this one (100-x)

This comment has been minimized.

@mattbenjamin

mattbenjamin Nov 16, 2016

Contributor

ack, otw

@yehudasa

This comment has been minimized.

Member

yehudasa commented Nov 15, 2016

@mattbenjamin did you push a newer version? I still see my comments unaddressed

@ktdreyer

This comment has been minimized.

Member

ktdreyer commented Nov 15, 2016

It would be great to have the ticket URL in the commit log

@mattbenjamin

This comment has been minimized.

Contributor

mattbenjamin commented Nov 16, 2016

@ktdreyer ack

@mattbenjamin

This comment has been minimized.

Contributor

mattbenjamin commented Nov 16, 2016

@yehudasa updated

@mattbenjamin mattbenjamin changed the title from rgw: add bucket size limit check to radosgw-admin to [DNM] rgw: add bucket size limit check to radosgw-admin Nov 17, 2016

@mattbenjamin

This comment has been minimized.

Contributor

mattbenjamin commented Nov 17, 2016

@yehudasa I broke a submodule commit (ec corpus), will repush...again
(fixed)

@mattbenjamin mattbenjamin changed the title from [DNM] rgw: add bucket size limit check to radosgw-admin to rgw: add bucket size limit check to radosgw-admin Nov 17, 2016

@mattbenjamin

This comment has been minimized.

Contributor

mattbenjamin commented Nov 18, 2016

@yehudasa updated radosgw-admin help.t
The Jenkins build now verifies the radosgw-admin usage output which it had stopped on previously. The current make check failures are unrelated to this change.

@yehudasa this does remind me that I still haven't updated doc/radosgw/admin.rst. I think it makes sense to do so, but it looks like we independently need writeup on bucket (re-)sharding.

@ghost

This comment has been minimized.

ghost commented Nov 21, 2016

jenkins test this please (eio now ignored in master)

@@ -495,6 +496,8 @@ static int get_cmd(const char *cmd, const char *prev_cmd, const char *prev_prev_
} else if (strcmp(prev_cmd, "buckets") == 0) {
if (strcmp(cmd, "list") == 0)
return OPT_BUCKETS_LIST;
if (strcmp(cmd, "limitcheck") == 0)

This comment has been minimized.

@yehudasa

yehudasa Nov 28, 2016

Member

radosgw-admin limit-check? or radosgw-admin limit check (handle it like radosgw-admin sync status)

This comment has been minimized.

@mattbenjamin
uint16_t shard_warn_pct =
store->ctx()->_conf->rgw_shard_warning_threshold;
if (shard_warn_pct > 100)
shard_warn_pct = 10;

This comment has been minimized.

@yehudasa

yehudasa Nov 28, 2016

Member

@mattbenjamin should probably set it to 90

@yehudasa

This comment has been minimized.

Member

yehudasa commented Nov 28, 2016

@mattbenjamin sorry for failing to send these comments earlier, new github review workflow was a bit confusing

@mattbenjamin

This comment has been minimized.

Contributor

mattbenjamin commented Dec 9, 2016

@yehudasa pushed with requested changes; I haven't run w/tenants yet, fyi.

@smithfarm smithfarm added the feature label Jan 30, 2017

@yehudasa

This comment has been minimized.

Member

yehudasa commented Mar 10, 2017

jenkins test this please

@oritwas

This comment has been minimized.

Contributor

oritwas commented Apr 3, 2017

@mattbenjamin please rebase

@mattbenjamin

This comment has been minimized.

Contributor

mattbenjamin commented Apr 13, 2017

@Orit done

@mattbenjamin mattbenjamin requested a review from cbodley Apr 19, 2017

std::list<std::string> user_ids;
metadata_key = "user";
int max = 1000;
int ret = store->meta_mgr->list_keys_init(metadata_key, &handle);

This comment has been minimized.

@cbodley

cbodley Apr 19, 2017

Contributor

this call isn't needed in the if (! user_id.empty()) { case (and will leak memory because list_keys_complete() is only called in the } else { block)

This comment has been minimized.

@mattbenjamin
if (ret < 0 && ret != -ENOENT) {
cerr << "ERROR: buckets limit check lists_keys_next(): "
<< cpp_strerror(-ret) << std::endl;
return -ret;

This comment has been minimized.

@cbodley

cbodley Apr 19, 2017

Contributor

an early return here will leak memory allocated by list_keys_init(). break; should work instead

This comment has been minimized.

@mattbenjamin
@ktdreyer

This comment has been minimized.

Member

ktdreyer commented Apr 20, 2017

Looks like a real test failure in src/test/cli/radosgw-admin/help.t ?

} else {
/* list users in groups of max-keys, then perform user-bucket
* limit-check on each group */
int ret = store->meta_mgr->list_keys_init(metadata_key, &handle);

This comment has been minimized.

@cbodley

cbodley Apr 20, 2017

Contributor

it looks like this ret is shadowing a variable from an enclosing scope. the return -ret at the end won't return this one

map<string, RGWBucketEnt>& m_buckets = buckets.get_buckets();
for (const auto& iter : m_buckets) {
auto& bucket = const_cast<rgw_bucket&>(iter.second.bucket);

This comment has been minimized.

@cbodley

cbodley Apr 20, 2017

Contributor

why the const_cast? can you just make const auto& iter non-const?

This comment has been minimized.

@mattbenjamin

mattbenjamin Apr 20, 2017

Contributor

I guess, but I think this is overreaching

if (! ret) {
if (info.num_shards > 0)
num_shards = info.num_shards;
}

This comment has been minimized.

@cbodley

cbodley Apr 20, 2017

Contributor

why ignore errors from get_bucket_info() and get_bucket_stats() below?

i see that a continue; here would skip the marker = bucket.name; statement at the end of this for loop. but if you were to move that assignment above this call to get_bucket_info(), it would be safe to change these to:

if (ret < 0) {
  continue;
}

This comment has been minimized.

@mattbenjamin

mattbenjamin Apr 20, 2017

Contributor

this comment may be important, but I do think it would be more helpful to more clearly state the problem; I don't need help picking between return continue, or other operators

@@ -231,6 +232,9 @@
--categories=<list> comma separated list of categories, used in usage show
--caps=<caps> list of caps (e.g., "usage=read, write; user=read")
--yes-i-really-mean-it required for certain operations
--warnings-only when specified with bucket limit check, list
only buckets nearing or over the current max
objects per shard value

This comment has been minimized.

@cbodley

cbodley Apr 20, 2017

Contributor

indentation here

rgw: add bucket size limit check to radosgw-admin
The change adds a new list of all buckets x all users, with
fields for bucket name, tenant name, current num_objects,
current num_shards, current objects per shard, and the
corresponding fill_status--the latter consisting of 'OK',
'WARN <n>%', or 'OVER <n>%.'

The warning check is relative to two new tunables.  The threshold
max objects per shard is set as rgw_bucket_safe_max_objects_per_shard,
which defaults to 100K.  The value rgw_bucket_warning_threshold is
a percent of the current safe max at which to warn (defaults to
90% of full).

From review:

* fix indentation (rgw_admin)
* if user a user_id is provided, check only buckets for that user
* update shard warn pct to be pct-of-fill (not 100 - pct-of-fill)
* print only buckets near or over per-shard limit, if --warnings-only
* s/bucket limitcheck/bucket limit check */
* sanity shard limit should be 90, not 10 (because that changed)
* fixes for memleaks and other points found by cbodley

Fixes: http://tracker.ceph.com/issues/17925

Signed-off-by: Matt Benjamin <mbenjamin@redhat.com>
@mattbenjamin

This comment has been minimized.

Contributor

mattbenjamin commented Apr 20, 2017

@cbodley updated again, left a marker assignment at bol

@mattbenjamin mattbenjamin merged commit dcbc28f into ceph:master Apr 20, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment