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 modify commit protocol in preparation for append #1243

Merged
merged 13 commits into from Oct 20, 2017

Conversation

Projects
None yet
6 participants
@garlick
Copy link
Member

garlick commented Oct 19, 2017

This PR changes the request message format for KVS commit/fence to incorporate a flags parameter in each of the operation objects in the "ops" array. This prepares for a KVS append operation, as discussed in #1193.

Since operations were encoded/decoded in several places, I added new (internal to KVS client and server) functions txn_encode_op() and txn_decode_op(), and used them in the client transaction code, and server commit code. The decoding function uses the JSON_STRICT ! flag to tighten things up a bit.

While adding this to the commit code, I fixed an error handling oversight that would allow a commit request to silently ignore a malformed operation and return success to the caller.

Finally, the KVS flags definitions were moved to kvs.h from kvs_lookup.h. They are not lookup specific. I didn't add an append flag yet though - we can do that when we have support on the server end for it.

@garlick garlick force-pushed the garlick:kvs_commit_proto branch 3 times, most recently from 181b84a to 902841f Oct 19, 2017

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

garlick added some commits Oct 19, 2017

libkvs/txn: add flags to commit operation object
As discussed in #1193, the operation object (an array
element in the ordered array of operations sent with
commit/fence requests) needs to expand to support a
future KVS append operation.

Add 'flags', an integer value, to each operation,
always 0 in this commit.

Request parsing on the KVS service end currently ignores
the extra 'flags' value.
libkvs/kvs: move flags to kvs.h
Problem: KVS flags are defined in kvs_lookup.h in a
'kvs_lookup_flags' enum, but the FLUX_KVS_TREEOBJ flag
is also used by the kvs_txn.h functions.

Move flags to kvs.h and make the enum anonymous.
libkvs/txn: [cleanup] tidy private ops interfaces
Problem: private txn interface for accessing ops
array and iterating through individual operations
use void * arguments and are a bit awkward.

There is no need for that in the current context.
Use more straightfoward interfaces to access the
ops array and its members.
@@ -332,7 +318,7 @@ int flux_kvs_txn_unlink (flux_kvs_txn_t *txn, int flags,
}
if (validate_flags (flags, 0) < 0)
goto error;
if (flux_kvs_txn_put_treeobj (txn, flags, key, NULL) < 0)
if (flux_kvs_txn_put_treeobj (txn, flags, key, json_null ()) < 0)

This comment has been minimized.

@chu11

chu11 Oct 20, 2017

Contributor

I think we'll mem-leak here, because txn_encode_op packs using s:O (that's upper case O). I think we need to save a reference to the object returned by json_null () and decref it.

This comment has been minimized.

@garlick

garlick Oct 20, 2017

Author Member

The jansson manual says json_null() is special, so I think the json_decref would be a no-op but it's good form at least. I'll go ahead and add it.

null = json_null ();
json_object_set_new (op, "dirent", null);
}
txn_encode_op (key, flags, dirent, &op);
json_array_append (array, op);

This comment has been minimized.

@chu11

chu11 Oct 20, 2017

Contributor

don't care too much since this is test code, but I think we should decref dirent?

This comment has been minimized.

@garlick

garlick Oct 20, 2017

Author Member

Yep thanks!

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 20, 2017

travis is choking here:

ffi.api.FFIError: multiple declarations of anonymous $enum_$1 (for interactive usage, try cdef(xx, override=True))

Guess I'll have to name that flags enum.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 20, 2017

If it is only travis do we just need to update the python-cffi version in there (it seems to currently be 1.1)

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 20, 2017

Do we care about breaking TOSS 2 systems these days? /opt on hype is python2.7-cffi-1.1.2...
I guess if we do another flux release there we could push out another python2.7-cffi.

my desktop (Ubuntu 16.04.3 LTS) has python-cffi-1.5.2 and is working.

ipa (TOSS 3) has python-cffi-1.6.0-5

I'll go ahead and change travis to 1.5 and see how that goes.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 20, 2017

Do we care about breaking TOSS 2 systems these says? /opt on hype is python2.7-cffi-1.1.2...
I guess if we do another flux release there we could push out another python2.7-cffi.

We didn't build a new package for TOSS 2 last time, not sure we have any users on that platform. But I agree we can just updated the cffi package if we need it in the future.

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Oct 20, 2017

With C++11 requirements for newer components, I am not sure if TOSS2 can be supported easily in the future...

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 20, 2017

Just pushed some commits to address comments so far, plus a couple incidentals. I do want to try to add a test for 902841f that commits a malformed operation. Once we're in a satisfactory state, I'll squash the incremental development.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 20, 2017

Coverage Status

Changes Unknown when pulling e349449 on garlick:kvs_commit_proto into ** on flux-framework:master**.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 20, 2017

Codecov Report

Merging #1243 into master will decrease coverage by 0.03%.
The diff coverage is 82.35%.

@@            Coverage Diff             @@
##           master    #1243      +/-   ##
==========================================
- Coverage   78.02%   77.99%   -0.04%     
==========================================
  Files         154      154              
  Lines       28933    28961      +28     
==========================================
+ Hits        22576    22588      +12     
- Misses       6357     6373      +16
Impacted Files Coverage Δ
src/modules/kvs/lookup.c 84.18% <ø> (ø) ⬆️
src/modules/kvs/fence.c 82.5% <ø> (ø) ⬆️
src/modules/kvs/commit.c 76.6% <100%> (-0.08%) ⬇️
src/common/libkvs/kvs_commit.c 76.47% <50%> (-10.2%) ⬇️
src/common/libkvs/kvs_txn.c 76.11% <81.81%> (-0.96%) ⬇️
src/common/libflux/content.c 86.66% <0%> (-3.34%) ⬇️
src/common/libflux/rpc.c 92.56% <0%> (-1.66%) ⬇️
src/common/libutil/dirwalk.c 93.57% <0%> (-1.43%) ⬇️
src/cmd/flux-event.c 67.74% <0%> (-1.08%) ⬇️
src/common/libkvs/kvs_watch.c 86.34% <0%> (-0.89%) ⬇️
... and 15 more
@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Oct 20, 2017

overall looks good to me. I especially like the changes with the txn_get functions. Looks a lot cleaner now.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 20, 2017

Bad travis day! I restarted 5 builders that got write errors, and one that hung in the sharness cron test.

Also restarted a hung master build from the last merge that is preventing codecov from reporting results.

I have that test queued up and will submit after that master build finishes.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 20, 2017

Coverage Status

Coverage decreased (-0.02%) to 78.595% when pulling 536e25a on garlick:kvs_commit_proto into 2095397 on flux-framework:master.

garlick added some commits Oct 19, 2017

libkvs/txn: [cleanup] add txn_encode_op, txn_decode_op
Problem: the txn operation encode/decode sequence is
repeated in several parts of the code.

Factor out private functions txn_encode_op() and
txn_decode_op().  Use it in kvs_txn.c and test/kvs_txn.c.
modules/kvs/commit: use new op encode/decode funcs
Use txn_decode_op() in commit_process() when applying the
operations in a commit, instead of direct jansson calls.

Update commit unit test to use txn_encode_op().
modules/kvs/commit: assert on array access error
Problem: in commit_process(), while applying commits, if we
get NULL back from json_get_array() while iterating over the
commit operations (using a known good array index), it is
silently skipped.

Change this to an assertion.
modules/kvs/commit: op decode should fail commit
Problem: in commit_process(), while applying commits, if
an operation cannot be decoded while iterating over the
commit operations, it is silently skipped.

Change error handling to fail the commit with EPROTO.
test/kvs/commit: [cleanup] fix func braces style
Use K&R brace alignment per RFC 7.
travis-ci: update pip-installed cffi 1.1->1.5
Problem: cffi 1.1 gets unhappy when it sees more
than one anonymous enum in a header it's ingesting.

Update to 1.5.
libkvs/txn: fix memleak in flux_kvs_txn_symlink()
Problem: flux_kvs_txn_symlink() leaks 'dirent' on success.

Call json_decref() on non-error exit path.
test/kvs/commit: fix memleak in ops_append()
Problem: ops_append() creates an operation,
appends it to the ops array with json_array_append(),
and exits the function, leaking the operation
since json_array_append() takes a reference.

Use json_array_append_new () which steals the
reference.
test/kvs/commit: add test for malformed operation
Add a commit test which submits a commit with a malformed
operation and verifies that the commit fails with EPROTO.

@garlick garlick force-pushed the garlick:kvs_commit_proto branch from 536e25a to d9002a1 Oct 20, 2017

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 20, 2017

All the tests passed that time. Resubmitting with incremental development squashed.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 20, 2017

Coverage Status

Coverage decreased (-0.02%) to 78.595% when pulling d9002a1 on garlick:kvs_commit_proto into 2095397 on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 20, 2017

This is good to go (IMHO).

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Oct 20, 2017

lgtm

@chu11 chu11 merged commit 8c34735 into flux-framework:master Oct 20, 2017

4 checks passed

codecov/patch 82.35% of diff hit (target 78.02%)
Details
codecov/project Absolute coverage decreased by -0.03% but relative coverage increased by +4.32% compared to 2095397
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.02%) to 78.595%
Details

@garlick garlick deleted the garlick:kvs_commit_proto branch Oct 20, 2017

@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.