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

KVS: Support reading valref objects with multiple blobrefs #1227

Merged
merged 7 commits into from Oct 17, 2017

Conversation

Projects
None yet
4 participants
@chu11
Copy link
Contributor

chu11 commented Oct 9, 2017

To support #1193 and #1202, this PR adds support to read valref treeobjs with multiple blobrefs in it. Note that outside of manual creation of multiple valref objects, there is no present easy mechanism to write multi-blobref valref objects. The "write" half of these features is for later subtasks in #1193 and #1202.

There are two major parts to this PR. One was to refactor the internal KVS lookup API to support multiple missing references being returned to callers. The second major part was to support loading multiple blobrefs in the internal KVS lookup API and creating a single val object to return the value to the caller.

@chu11 chu11 requested a review from garlick Oct 9, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 9, 2017

Coverage Status

Coverage decreased (-0.06%) to 78.59% when pulling 11dd00e on chu11:issue1202-part1 into 9d3b4b1 on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 10, 2017

Codecov Report

Merging #1227 into master will decrease coverage by <.01%.
The diff coverage is 79.69%.

@@            Coverage Diff            @@
##           master   #1227      +/-   ##
=========================================
- Coverage   78.11%   78.1%   -0.01%     
=========================================
  Files         154     154              
  Lines       28702   28812     +110     
=========================================
+ Hits        22420   22504      +84     
- Misses       6282    6308      +26
Impacted Files Coverage Δ
src/modules/kvs/kvs.c 63.52% <54.54%> (-0.63%) ⬇️
src/modules/kvs/lookup.c 84.18% <88%> (+0.3%) ⬆️
src/common/libutil/blobref.c 97.22% <0%> (-1.39%) ⬇️
src/common/libflux/request.c 87.17% <0%> (-1.29%) ⬇️
src/common/libflux/mrpc.c 85.49% <0%> (-1.18%) ⬇️
src/common/libkvs/kvs_watch.c 86.34% <0%> (-0.89%) ⬇️
src/common/libkvs/kvs.c 64.87% <0%> (-0.63%) ⬇️
src/broker/module.c 83.79% <0%> (-0.28%) ⬇️
src/common/libflux/message.c 81.48% <0%> (+0.23%) ⬆️
src/common/libflux/future.c 89.25% <0%> (+0.46%) ⬆️
... and 4 more
@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Oct 10, 2017

struct lookup_ref_data ld = { 0 };

I guess i need to specifically set each field to zero to avoid warning error, added fix, will squash later.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 10, 2017

Coverage Status

Coverage decreased (-0.05%) to 78.597% when pulling 2532e1b on chu11:issue1202-part1 into 9d3b4b1 on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Oct 10, 2017

boo, "write error" fails, restarting

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 10, 2017

Great!

I'll try running and poking at this tomorrow. Couple quick comments: function pointers should end in _f (per RFC 7) and the word "appropriate" is comically overused in the commit message for 08910cf :-)

I was vaguely wondering if the creation of a single val object from multiple blobs was something that could be cleverly abstracted in treeobj.c. I'm not sure what interface would work best here though, may not be worth it.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Oct 10, 2017

just pushed bunch of tiny things, minor bug fixes, bunch of extra coverage tests, and renaming the callback function to have _f instead of _cb. Hopefully can get coverage to around 78%, but may be hard given many "impossible" to reach error paths.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 10, 2017

Coverage Status

Coverage decreased (-0.006%) to 78.64% when pulling a1fbc51 on chu11:issue1202-part1 into 9d3b4b1 on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Oct 10, 2017

hmmm, on two builds my EOVERFLOW test fails as it returns ENOMEM. My assumption is this check:

        total += len;
        if (total < len) {
            lh->errnum = EOVERFLOW;
            goto done;
        }

is not working and a malloc below it is subsequently failing. Looking online it appears this is not safe behavior for signed ints. Will have to research.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 10, 2017

Coverage Status

Coverage decreased (-0.06%) to 78.59% when pulling 019b7f4 on chu11:issue1202-part1 into 9d3b4b1 on flux-framework:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 10, 2017

Coverage Status

Coverage decreased (-0.06%) to 78.583% when pulling 14c1b51 on chu11:issue1202-part1 into 9d3b4b1 on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Oct 10, 2017

more tiny fixes pushed to increase coverage, fix a minor issue. It's a ton of tiny things. If you haven't started reviewing yet, I can squash and get things back to a nice point.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 10, 2017

Coverage Status

Coverage increased (+0.03%) to 78.671% when pulling e42c744 on chu11:issue1202-part1 into 9d3b4b1 on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 10, 2017

I was holding off until you settled down :-) Squashing would be great.

@chu11 chu11 force-pushed the chu11:issue1202-part1 branch from e42c744 to 85732bf Oct 10, 2017

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Oct 10, 2017

ok, just squashed. Last go-around I was at 76.8% diff coverage. Lets see if my last tweak can get me past 78%. If it doesn't, this might be as close as I can get. There's a lot of "impossible" paths in the main kvs.c file that can't be reached.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 10, 2017

Coverage Status

Coverage increased (+0.01%) to 78.655% when pulling 85732bf on chu11:issue1202-part1 into 9d3b4b1 on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 10, 2017

I tried modifying your sharness test to make the first blob empty and got a hang and:

content_load_completion: cache_entry_set_raw: Invalid argument

Since we allow an empty raw value to be stored in the KVS, presumably we should allow it to be appended to?

@garlick
Copy link
Member

garlick left a comment

I added some inline comments which I hope aren't too off base, but please feel free to tell me if so.

I would like to see more sharness tests, variants on the one you already added, that include things like zero length blobs in different positions, and blobrefs that aren't in the store to make sure those corner cases are handled right.

Also, I think we spoke about this before but I am not sure how we resolved it - EPERM seems like the wrong error code to return for things like dangling blobrefs. Do you remember where we left that?


/* return 0 on success, -1 on failure. On success, stall should be
* check */
static int get_multi_blobref_valref_value (lookup_t *lh, int refcount,

This comment has been minimized.

@garlick

garlick Oct 10, 2017

Member

Just a suggestion and may just be a style thing: would it be clearer if determining the aggregate size and copying to the aggregate buffer were split into two functions?

This comment has been minimized.

@chu11

chu11 Oct 10, 2017

Author Contributor

yeah, that could probably help.

@@ -804,6 +812,13 @@ static void get_request_cb (flux_t *h, flux_msg_handler_t *w,

if (lookup_iter_missing_refs (lh, lookup_load_cb, &cbd) < 0) {
errno = cbd.errnum;

This comment has been minimized.

@garlick

garlick Oct 10, 2017

Member

sharness test for this? (one bad blobref in valref array?)

@@ -36,6 +36,12 @@ bool lookup_validate (lookup_t *lh);
* an error occurred or not */
int lookup_get_errnum (lookup_t *lh);

/* if user wishes to stall, but needs future knowledge to fail and

This comment has been minimized.

@garlick

garlick Oct 10, 2017

Member

is that a real use case? is there ever a reason to stall the request if you've hit an error? Seems like you would want to immediately abort and get the response to the user.

This comment has been minimized.

@chu11

chu11 Oct 10, 2017

Author Contributor

So the way I've handled it in the past (with the commit API) was that if multiple RPCs are sent successfully then one fails, I wait for the in-flight rpcs to complete then return the error to the user. So these get/set aux errnums are the "flag" for the callback function to return an error to the user. See commit_apply() function in kvs.c and you'll see what I do.

This perhaps could be dealt with an alternate way. Perhaps an error could be returned to the user immediately and get/set aux errnum could be a flag informing the callback function "this rpc already errored, don't send user a response". It'd simply be a logic change. Perhaps for another issue though?

This comment has been minimized.

@garlick

garlick Oct 10, 2017

Member

Ah, sorry, I think that's fine for now. It's an error case, so trading a little latency for simplicity is probably a wise move. If you feel that change could actually simplify the code here and in the commit API, then I'd suggest opening a bug for later.

This comment has been minimized.

@chu11

chu11 Oct 10, 2017

Author Contributor

I think code wise it's not too much different each way. Of course returning the error to the user earlier shortens the latency a bit. I'll create an issue so we don't forget about this.

This comment has been minimized.

@chu11

chu11 Oct 10, 2017

Author Contributor

Actually, thinking about it now, the code logic would probably be more confusing. Because I'd have to keep some auxiliary-data structures around longer than they are currently used.

For example in commit_apply(), after we reply to the user we call commit_mgr_remove_commit() to remove/destroy a commit context. If I were to change the logic to return an error to the user immediately, I can't destroy this. It still needs to exist for the rpcs that haven't finished.

Think it'd be better to keep it the way it is right now.

@@ -57,6 +57,11 @@ typedef struct {
zlist_t *pathcomps;
} walk_level_t;

typedef struct {

This comment has been minimized.

@garlick

garlick Oct 10, 2017

Member

I am probably not understanding the big picture, but why do we need the zlist of these structs when we have the json_t ref itself, which contains a JSON array of hash keys into the cache?

This comment has been minimized.

@chu11

chu11 Oct 10, 2017

Author Contributor

I think you're right, we don't. I was sort of mimicking what was done in the internal commit API.

This comment has been minimized.

@chu11

chu11 Oct 10, 2017

Author Contributor

doh! changing up the code right now, I realize there was a reason. We can't assume every blobref in the valref treeobj is missing from the cache. Some could be missing while others aren't.

It perhaps wouldn't have much performance impact to pass all references back to the caller, b/c the main kvs module would already recognize that some of those missing blobrefs are already in the cache. It'd simply be an API style change to say I'm passing back a list of references, atleast 1 of which is missing. Instead of all of them are missing.

I'll have to ponder this. As the change from the list to just using the valref object is much cleaner.

This comment has been minimized.

@chu11

chu11 Oct 11, 2017

Author Contributor

duh, thought of an obvious way to fix

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Oct 10, 2017

hey jim, you're right. I had not thought of the 0 length blobs, so some tests should definitely be added and code fixed accordingly.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Oct 10, 2017

As for EPERM, I think we just sort of left it the last time it was discussed, as EPERM is the catch all "internally not consistent/bad" kind of errno. I'll create an issue for this so we don't forget.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 12, 2017

@chu11 - are you waiting on me for anything here? Your comments above all make sense to me. Let me know when you're ready for me to make another review pass.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Oct 12, 2017

I'm working on #1232 right now. It sort of needs to be done first, b/c without that I can't really do the multi-blob support w/ empty data.

@chu11 chu11 force-pushed the chu11:issue1202-part1 branch from 85732bf to 1df93cd Oct 17, 2017

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Oct 17, 2017

re-pushed rebased with master and changes from #1232.

lookup no longer uses a list for returning missing refs to the user, just uses the valref object itself, this is way cleaner now. Thanks Jim.

some code cleanup in lookup API

some multi-blobref tests added that have zero-content length blobs

need to add bad-blobref amongst the good blob-ref tests, forgot about that one. Forth coming soon.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Oct 17, 2017

oh yeah, I forgot, the test for 1 illegal blobref in a valref array hangs b/c of #792. So that test will probably have to be for another day. I noted in #792 that this test should be added.

But we can add a blobref that points to the wrong type of data (i.e. a directory or something). Will add that test.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 17, 2017

Coverage Status

Coverage increased (+0.005%) to 78.689% when pulling 1df93cd on chu11:issue1202-part1 into 9f03dda on flux-framework:master.

@chu11 chu11 force-pushed the chu11:issue1202-part1 branch from 1df93cd to edfcac2 Oct 17, 2017

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Oct 17, 2017

re-pushed, added a valref w/ a single-blobref test in which the blobref points to the wrong type of data.

then added a multi-blobref valref test, in which one of the blobrefs points to the wrong type of data.

But when I wrote the tests, I realized a problem. Imagine ...

a directory has blobref to another dir, say that blob is sha1-XXX
valref has sha1-XXX as a reference to a raw piece of data, which is invalid/bad in this case

How sha1-XXX is stored in the KVS cache (stored as json or raw) will depend on how the reference is first loaded into the KVS cache. If the valref is loaded first, it'll be loaded as raw data. If it's loaded as a directory object first, it'll be loaded as json. This is b/c the data in the content store is not "typed" in any way. An error would occur on whatever tries to read sha1-XXX second.

Not sure if this needs to be solved in some way. The only way this occurs is if the user manually creates tree objects and modifies them with bad data.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 17, 2017

Coverage Status

Coverage decreased (-0.02%) to 78.667% when pulling edfcac2 on chu11:issue1202-part1 into 9f03dda on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Oct 17, 2017

hmmm, lots of failures. It appears with my new test in 116f6b8. Tests after it don't run, suggesting segfault or assert. Hmmm.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Oct 17, 2017

ugh, dumb re base error, re-pushed (per twitter - github + travis is currently having a problem, so i guess CI will run whenever that is fixed)

@chu11 chu11 force-pushed the chu11:issue1202-part1 branch 2 times, most recently to f2e405e Oct 17, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 17, 2017

Coverage Status

Coverage increased (+0.06%) to 78.739% when pulling f2e405e on chu11:issue1202-part1 into 9f03dda on flux-framework:master.

@chu11 chu11 force-pushed the chu11:issue1202-part1 branch from f2e405e to b66ad57 Oct 17, 2017

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Oct 17, 2017

re-push, fixing chain-lint

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Oct 17, 2017

hit write errors and #731, restarting builds

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 17, 2017

Coverage Status

Coverage decreased (-0.009%) to 78.675% when pulling b66ad57 on chu11:issue1202-part1 into 9f03dda on flux-framework:master.

chu11 added some commits Oct 5, 2017

modules/kvs: Refactor lookup_get_missing_ref
Refactor lookup_get_missing_ref() into lookup_iter_missing_refs(),
in which user passes a callback function to retrieve the missing
reference and raw boolean instead of via the function itself.
This is to prepare for the KVS lookup API returning multiple
missing references back to the user.

Update code appropriately in main KVS module.

Update unit tests and add additional tests as necessary.
modules/kvs: Refactor lookup handling of valrefs
Refactor internal lookup API to be able to handle returning multiple
missing references from a valref to the caller.  This is predominantly
infrastructure support for future multiple missing references support
and nothing in the internal KVS lookup API currently uses this.

As a fallout of this refactoring, the `missing_ref_raw` flag is no
longer necessary and has been removed.
modules/kvs: Add get/set aux_errnum in lookup API
Add lookup_get_aux_errnum() and lookup_set_aux_errnum() in internal
KVS lookup API.  This is convenience for future error handling needs.

Add unit tests appropriately.
modules/kvs: Handle lookup multi-load error
In main KVS module, handle errors on multiple reference lookup loads
if an error occurs after several rpcs have already been sent.
modules/kvs: Add multi-blobref valref read support
In internal lookup API, return val appropriately if valref
has multiple blobrefs within it.  This is done by loading each
blobref appropriately and constructing a concatenated result.

Update unit tests appropriately.  Add more tests for coverage.

@chu11 chu11 force-pushed the chu11:issue1202-part1 branch from b66ad57 to a1233f6 Oct 17, 2017

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Oct 17, 2017

re-pushed, eek out a couple extra lines of code coverage.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 17, 2017

Coverage Status

Coverage increased (+0.02%) to 78.701% when pulling a1233f6 on chu11:issue1202-part1 into 9f03dda on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 17, 2017

I'm for merging this if you're ready @chu11.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Oct 17, 2017

it's good to go

@garlick garlick merged commit eebb5e7 into flux-framework:master Oct 17, 2017

4 checks passed

codecov/patch 79.69% of diff hit (target 78.11%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +1.58% compared to 9f03dda
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 78.701%
Details

@grondo grondo referenced this pull request May 10, 2018

Closed

0.9.0 Release #1479

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.