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: Do not decrement stats cache when the cache values are zero #16389

Merged
merged 1 commit into from Jul 27, 2017

Conversation

Projects
None yet
3 participants
@prallabh
Contributor

prallabh commented Jul 18, 2017

With RGWs configured in a load balancer, there is a possibility of
having the cached values going unbound, when PUT/DELETE operations
do not land up on the same RGW. To avoid such cases, make sure the
decrement of stats happen only when the cached values are sane.

Fixes: http://tracker.ceph.com/issues/20661
Signed-off-by: Pavan Rallabhandi PRallabhandi@walmartlabs.com

@prallabh

This comment has been minimized.

Contributor

prallabh commented Jul 19, 2017

@dang can you please take a look at this, thanks.

@prallabh

This comment has been minimized.

Contributor

prallabh commented Jul 23, 2017

@mattbenjamin @dang can you please help review

@oritwas oritwas added the rgw label Jul 24, 2017

@oritwas

you should fix the quota checks to handle negative stats correctly and not the stats calculation. The negative values are temporary and valid.

@oritwas oritwas added the bug fix label Jul 24, 2017

@prallabh

This comment has been minimized.

Contributor

prallabh commented Jul 24, 2017

@oritwas Can you please help explain about the negative values being valid. And also with such unbound values in the stats cache, how can the quota checks be validated? For example, in is_num_objs_exceeded() the check for exceeding the quota size of objects is verified against the respective value in the stats, and with an unbound (2^64) value present, the quota check would always be true. Please let me know if am missing anything.

@oritwas

This comment has been minimized.

Contributor

oritwas commented Jul 24, 2017

@prallabh , I just noticed it is a jewel issue I need to look at the quota jewel code (there were lots of changes since)

@oritwas

This comment has been minimized.

Contributor

oritwas commented Jul 24, 2017

@prallabh, it is a jewel specific fix as we use byte granularity from kraken when we are checking quota.
You will need to fix RGWQuotaStatsUpdate update method to not set a negative value for num_kb_rounded. the pr should be against jewel branch as it jewel specific

@prallabh

This comment has been minimized.

Contributor

prallabh commented Jul 24, 2017

@oritwas it's just not Jewel, it's there in master as well and that's exactly the function RGWQuotaStatsUpdate what this PR has modified. And it doesn't seem to be an issue with the byte conversion, but to do with the update_stats() from PUT/DELETE operations not necessarily landing up on the same RGW with load balancers.

entry->stats.size += added_bytes - removed_bytes;
entry->stats.size_rounded += rounded_added - rounded_removed;
entry->stats.num_objects += objs_delta;
if ((added_bytes - removed_bytes) < 0) {

This comment has been minimized.

@oritwas

oritwas Jul 24, 2017

Contributor

if ((entry->stats.size + added_bytes - removed_bytes) > 0) {
entry->stats.size += added_bytes - removed_bytes;
} else {
entry->stats.size = 0;
}

This comment has been minimized.

@prallabh

prallabh Jul 24, 2017

Contributor

yes, this check is also valid with an added equal to zero condition,

if ((entry->stats.size + added_bytes - removed_bytes) >= 0) {
  entry->stats.size += added_bytes - removed_bytes;
} else {
  entry->stats.size = 0;
}

would change it this way, if it's more readable.

@oritwas

This comment has been minimized.

Contributor

oritwas commented Jul 24, 2017

@prallabh , correct

rgw: Do not decrement stats cache when the cache values are zero
With RGWs configured in a load balancer, there is a possibility of
having the cached values going unbound, when PUT/DELETE operations
do not land up on the same RGW. To avoid such cases, make sure the
decrement of stats happen only when the cached values are sane.

Fixes: http://tracker.ceph.com/issues/20661
Signed-off-by: Pavan Rallabhandi <PRallabhandi@walmartlabs.com>
@prallabh

This comment has been minimized.

Contributor

prallabh commented Jul 25, 2017

@oritwas can you please check now

@yuriw yuriw merged commit 2357dcd into ceph:master Jul 27, 2017

4 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details

@prallabh prallabh deleted the prallabh:wip-20661 branch Jul 28, 2017

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