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

osd: extend OMAP_GETKEYS and GETVALS to include a 'more' output field #12950

Merged
merged 9 commits into from Jan 22, 2017

Conversation

Projects
None yet
2 participants
@liewegas
Member

liewegas commented Jan 16, 2017

The current omap get methods cannot communicate to the client that their
results were truncated due to hitting a limit on the number of keys or
bytes, making those server-side limits extremely dangerous to
enforce.

Extend the API to implement a new set of methods that explicitly expose a
"more" flag.

The old interfaces are marked deprecated.

Open question: should we change the old implementations to loop on the client
side?

@liewegas liewegas changed the title from osd: extend OMAP_GETKEYS and GETVALS to include a 'more' output field to DNM: osd: extend OMAP_GETKEYS and GETVALS to include a 'more' output field Jan 16, 2017

@liewegas

This comment has been minimized.

Member

liewegas commented Jan 16, 2017

@jdurgin Should we make the old omap_get_{keys,vals} loop on the client? I'm thinking yes...

@liewegas

This comment has been minimized.

Member

liewegas commented Jan 16, 2017

Okay, updated to do the looping on the client in the cases where we can.

This introduces a bunch of new warnings across the code base for various users of the deprecated API. We can fix those before or after merge (I don't feel strongly either way). I could also separate out the bit that marks them deprecated so that we don't "break" master with those warnings until they are fixed. But I think having them there will be the right level of motivation to get them fixed...

The two critical users are rgw and the mds, @mattbenjamin @yehudasa @jcsp

@@ -266,7 +266,8 @@ void librados::ObjectReadOperation::omap_get_vals(
int *prval)
{
::ObjectOperation *o = &impl->o;
o->omap_get_vals(start_after, filter_prefix, max_return, out_vals, prval);

This comment has been minimized.

@jdurgin

jdurgin Jan 18, 2017

Member

looks like these librados test changes slipped into the wrong commit - they wouldn't build yet here

This comment has been minimized.

@liewegas

liewegas Jan 18, 2017

Member

these are calling the Objecter::ObjectOperation, not the librados structure

* @param filter_prefix list only keys beginning with filter_prefix
* @param max_return list no more than max_return key/value pairs
* @param iter where to store the iterator
* @param prval where to store the return value from this action

This comment has been minimized.

@jdurgin

jdurgin Jan 18, 2017

Member

should describe pmore for each of these

This comment has been minimized.

@liewegas
@@ -3124,7 +3124,28 @@ CEPH_RADOS_API void rados_read_op_omap_get_vals(rados_read_op_t read_op,
const char *filter_prefix,
uint64_t max_return,
rados_omap_iter_t *iter,
int *prval);
int *prval)
__attribute__((deprecated)); /* use v2 below */

This comment has been minimized.

@jdurgin

jdurgin Jan 18, 2017

Member

seems like the originals are useful convenience functions that shouldn't be deprecated now that they're looping on the client side

This comment has been minimized.

@liewegas

liewegas Jan 18, 2017

Member

These read_op* methods are used to build up a multi-op transaction.. we can't do magic client-side looping for them.

number of keys or bytes that can be returned by a single omap request. These
limits were introduced by kraken but were effectively disabled (by setting a
very large limit of 1 GB) because users of the newly deprecated interface
cannot tell whether they should fetch more keys or not.

This comment has been minimized.

@jdurgin

jdurgin Jan 18, 2017

Member

mention looping for the originals, and imo no need to deprecate them, but warn about old clients + newer osd behavior here

This comment has been minimized.

@liewegas

liewegas Jan 18, 2017

Member

Adding warning.

The reason to deprecated them is to make sure the developer is aware they're using a fundamentally broken/unsafe interface...

}
if (more) {
if (!out.empty()) {
return -EINVAL; // wth

This comment has been minimized.

@jdurgin

jdurgin Jan 18, 2017

Member

what's this case for?

This comment has been minimized.

@liewegas

liewegas Jan 18, 2017

Member

it's just to guard the out.rbegin() dereference on the next line. it's not supposed to happen, but an assert would be rude.

liewegas added some commits Jan 15, 2017

osd/PrimaryLogPG: tell client if we truncate results
If we truncate the results of the omap read commands,
provide a flag so that the caller knows there is more
to be read.  We don't need to provide the name of the
next key because the interface is defined as
"start_after" (not "start_with"), allowing them to use
the last key they received.

Signed-off-by: Sage Weil <sage@redhat.com>
osdc/Objecter: add pmore argument to omap_get_{keys,vals}
Note that the MDS callers have new #warnings indicating that they
are not providing the pmore argument and are thus broken.  (They
were already broken.)

Signed-off-by: Sage Weil <sage@redhat.com>
@jdurgin

looping only for the IoCtx methods makes sense to me

@liewegas liewegas added the needs-qa label Jan 20, 2017

@liewegas liewegas changed the title from DNM: osd: extend OMAP_GETKEYS and GETVALS to include a 'more' output field to osd: extend OMAP_GETKEYS and GETVALS to include a 'more' output field Jan 20, 2017

liewegas added some commits Jan 16, 2017

librados: add omap_get_{keys,vals}2 with pmore output arg
Expose public methods that include a new output argument to indicate
whether there are more keys to fetch or not.

Mark the old interfaces deprecated.

Signed-off-by: Sage Weil <sage@redhat.com>
osdc/Objecter: infer ptruncated on old OSDs via max_entries
If we do not get an explicit 'more' value from the OSD, infer it by
checking whether we got the max requested entries.  On old OSDs, which
don't enforce a limit, this will work.  On new OSDs, we will get the
explicit result.

Signed-off-by: Sage Weil <sage@redhat.com>
ceph_test_rados_*: use new omap methods
Make these tests pass nullptr for pmore.  This codifies our
assumption that the OSD limits are high enough.

Signed-off-by: Sage Weil <sage@redhat.com>
rados: omap bench: pass null for pmore for omap fetch
Assume OSD limits are high enough for us.

Signed-off-by: Sage Weil <sage@redhat.com>
ceph_test_rados: pass nullptr for pmore for omap fetch
Assume OSD limits are hugh enough.

Signed-off-by: Sage Weil <sage@redhat.com>
rados: use bare omap_get_keys op
This handles the client-side looping on 'more' if the OSD limits
the response size.

Signed-off-by: Sage Weil <sage@redhat.com>
PendingReleaseNotes: mention old clients vs new OSDs
Signed-off-by: Sage Weil <sage@redhat.com>

@liewegas liewegas merged commit 55b96a3 into ceph:master Jan 22, 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

@liewegas liewegas deleted the liewegas:wip-omap-getkeys branch Jan 22, 2017

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