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: handle non-JSON values in flux kvs get, put, dir #1257

Merged
merged 20 commits into from Oct 26, 2017

Conversation

Projects
None yet
4 participants
@garlick
Copy link
Member

garlick commented Oct 25, 2017

This PR updates the flux kvs command to handle non-JSON values. These deficiencies were discussed in #1158 and #1159.

A summary of the changes proposed here are:

  • flux kvs dir displays non-JSON values
  • flux kvs dir truncates output to fit terminal width, controlled with --width=COLS
  • flux kvs put and flux kvs get now accept the --raw, --json, and --treeobj options to allow greater control over how values are stored and interpreted.

As a reminder the KVS just stores raw values now, imposing no structure. We have convenience functions for get/put of strings (NULL terminated), and JSON (encoded as strings). The get/put subcommands now default to storing strings, as opposed to JSON-encoded strings. The --json option was added to emulate the past behavior. The --raw options was added to allow non-NULL terminated byte sequences to be stored and retrieved. The --treeobj option was added so that snapshots could be manipulated and metadata could be examined conveniently.

Man pages updated and sharness tests added.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 25, 2017

Coverage Status

Coverage increased (+0.01%) to 78.505% when pulling 8135869 on garlick:kvs_cmd_update into 2694116 on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Oct 25, 2017

just beginning to review, but a thought that comes to mind, are many options in t/kvs/basic axe-able? and tests adjusted accordingly? copy-fromkvs, copy-tokvs, get-treeobj, put-treeobj, spring to mind. Could be cleanup via another issue of course and not necessarily this PR.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 25, 2017

Hmm, it does seem like get-treeobj and put-treeobj are obviously the same as get/put with the --treeobj option. I'll see if I can do some of that cleanup here.

@@ -56,6 +56,16 @@ static void dump_kvs_dir (const flux_kvsdir_t *dir, bool Ropt, bool dopt);

This comment has been minimized.

@chu11

chu11 Oct 25, 2017

Contributor

typo in commit comment, "strong" -> "string"

This comment has been minimized.

@chu11

chu11 Oct 25, 2017

Contributor

hmm, my review comment appears to be attached to file and not a commit. This is in commit f93f776

@garlick garlick force-pushed the garlick:kvs_cmd_update branch from 8135869 to 99a1795 Oct 26, 2017

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 26, 2017

Codecov Report

Merging #1257 into master will increase coverage by 0.06%.
The diff coverage is 83.87%.

@@            Coverage Diff             @@
##           master    #1257      +/-   ##
==========================================
+ Coverage   77.86%   77.92%   +0.06%     
==========================================
  Files         154      154              
  Lines       28934    29027      +93     
==========================================
+ Hits        22530    22620      +90     
- Misses       6404     6407       +3
Impacted Files Coverage Δ
src/cmd/flux-kvs.c 80.06% <83.87%> (+0.28%) ⬆️
src/common/libutil/blobref.c 97.22% <0%> (-1.39%) ⬇️
src/common/libflux/message.c 81.13% <0%> (-0.59%) ⬇️
src/common/libkvs/kvs_txn.c 74.01% <0%> (-0.57%) ⬇️
src/common/libutil/dirwalk.c 94.28% <0%> (ø) ⬆️
src/bindings/lua/flux-lua.c 81.16% <0%> (+0.08%) ⬆️
src/common/libflux/handle.c 83.91% <0%> (+0.24%) ⬆️
src/common/libkvs/kvs.c 64.28% <0%> (+0.49%) ⬆️
src/modules/kvs/kvs.c 62.88% <0%> (+0.51%) ⬆️
src/broker/modservice.c 81.55% <0%> (+0.97%) ⬆️
... and 2 more
@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 26, 2017

Just rebased on current master, fixed commit comment, and added some commits that move functionality out of t/kvs/basic that now seem appropriate in flux-kvs:

get [--at ref]        # replaces t/kvs/basic getat
dir [--at ref]        # replaces t/kvs/basic dirat
readlink [--at ref]   # replaces t/kvs/basic readlinkat
put --raw key=-       # (read from stdin), replaces t/kvs/basic copy-tokvs
put [--no-merge]      # replaces t/kvs/basic put-no-merge

Good suggestion @chu11. All that's left in the test command is the type checking stuff.

flux kvs put $DIR.valref=$(seq -s 1 100) &&
flux kvs mkdir $DIR.dirref &&
flux kvs link foo $DIR.symlink &&
flux kvs get --treeobj $DIR.val &&

This comment has been minimized.

@chu11

chu11 Oct 26, 2017

Contributor

perhaps | grep "dir" and equivalents should be added to make sure the right treeobj is being returned

This comment has been minimized.

@garlick

garlick Oct 26, 2017

Author Member

Sure, that's a good improvement.

test_expect_success 'kvs: empty string value does not cause dir to fail' '
flux kvs unlink -Rf $DIR &&
flux kvs put $DIR.a= &&
flux kvs dir $DIR

This comment has been minimized.

@chu11

chu11 Oct 26, 2017

Contributor

should this test (and the ls one below) also make sure the directory is output?

This comment has been minimized.

@garlick

garlick Oct 26, 2017

Author Member

Yep, will fix that.

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Oct 26, 2017

@garlick awesome. A bunch of the tests can probably be moved from t1002-kvs-extra.t to the main t1000-kvs.t now that they are more "core" tests.

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Oct 26, 2017

just finished skimming. Nothing stands out to me as a problem.

Before your rebase, it felt like there were a lack of basic --raw tests. There are some now due to copy-fromkvs and copy-tokvs being gone, but some basic --raw functionality tests would probably be good.

A good test I thought of was to mix treeobj & raw. Off the top of my head:

flux kvs put --raw $DIR.a={ a tree object val }
flux kvs get --raw $DIR.a (get the treeobj above)
flux kvs get --treeobj $DIR.a (can get a treeobj)
flux kvs get $DIR.a (can get value in the treeobj)
flux kvs put $DIR.a.b=goodbye &&
flux kvs put --treeobj $DIR.b=$(flux kvs get --treeobj $DIR.a) &&
(flux kvs get $DIR.a.a && flux kvs get $DIR.a.b) >snap.expected &&
(flux kvs get $DIR.b.a && flux kvs get $DIR.b.b) >snap.actual

This comment has been minimized.

@chu11

chu11 Oct 26, 2017

Contributor

chain-lint needs &&?

This comment has been minimized.

@garlick

garlick Oct 26, 2017

Author Member

Good catch!

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 26, 2017

Just pushed some changes to address your comments, except I didn't add any new raw tests or move any tests between the two sharness scripts.

I agree the test scripts are a little haphazard in the way they've accreted tests over time and could use a cleanup pass (possibly cleanup is for another time though?)

I'll go through the raw tests including the converted ones tomorrow and see if there are any obvious gaps.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 26, 2017

restarted one builder after hang in the cron tests

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Oct 26, 2017

agreed, we should cleanup the tests another day. After this PR is done, we can make an issue for it. I'm sure after append support, the tests haphazardness will just be weirder. We may want to split it up amongst even more files instead of just "core" and "extra".

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 26, 2017

I didn't entirely understand what was meant here - maybe I am misreading

flux kvs put --raw $DIR.a={ a tree object val }
flux kvs get --raw $DIR.a (get the treeobj above)
flux kvs get --treeobj $DIR.a (can get a treeobj)
flux kvs get $DIR.a (can get value in the treeobj)

the first command would encode { tree object val } inside a tree object val, the second would pull it out again. Is nesting those meaningful?

On reorganizing - a good start would be to separate the kvs_watch tests out to their own script since they are a bit complicated and are likely to change when watch is reworked.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 26, 2017

Coverage Status

Coverage increased (+0.03%) to 78.521% when pulling 2bef84f on garlick:kvs_cmd_update into b12d027 on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Oct 26, 2017

@garlick ahh, no nesting intended. I guess I shouldn't have used brackets. First line is to just write a treeobj, second line to read it back and make sure it wrote correctly. I also realize now that the 4th line probably should have been with --json. Ugh I really messed up that suggestion.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 26, 2017

Coverage Status

Coverage increased (+0.03%) to 78.524% when pulling 18474b7 on garlick:kvs_cmd_update into b12d027 on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 26, 2017

I've pushed some raw tests, mainly exercising the kvs put --raw command line.

To be clear, --treeobj gives direct access to the RFC 11 object and --raw creates a 'val' object just like --json or no options. I'm still confused by the suggestion - maybe we should chat face to face.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 26, 2017

Coverage Status

Coverage increased (+0.07%) to 78.56% when pulling 908824a on garlick:kvs_cmd_update into b12d027 on flux-framework:master.

garlick added some commits Oct 24, 2017

cmd/flux-kvs: add [--json|--raw] options to "put"
Problem:  flux-kvs put presumes values should be stored as JSON,
but the KVS no longer requires this.

Add type options to allow the user to choose how values are stored.

If no options, value is stored as a NULL terminated string.

If --raw, value is stored as with no options, but without a
NULL terminator.  For --raw mode only, key=- may be used to
take read value from stdin.

If --json, value is stored as a NULL terminated string if it
is valid encoded JSON; otherwise it is first encoded as a JSON
string.  This mimics the old default behavior of flux-kvs put
that is expected by many tests in t1000-kvs.t and t1002-kvs-extra.t.

Add --json to flux kvs put where used in various sharness tests.

Partial fix to #1159.
cmd/flux-kvs: add [--json|--raw] options to "get"
Problem:  flux-get put presumes values should be interpreted as JSON,
but the KVS no longer requires this.

Add type options mirroring those added to the "put" subcommand
to allow the user to choose how values are interpreted.

If no options, value is interpreted as a NULL terminated string
and written to stdout with a newline.

If --raw, value is interpreted as raw data and is written to
stdout without any formatting.

If --json, value is interpreted as encoded JSON.  If the value is an
object or array, or any scalar type but string, it is displayed
in its encoded form.  If the value is a JSON string, quotes are
removed, which mimics the old default behavior of flux-kvs get
expected by many sharness tests.

Add --json to flux kvs get where used in various sharness tests.

Fixes #1159

@garlick garlick force-pushed the garlick:kvs_cmd_update branch from 908824a to 35700cb Oct 26, 2017

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 26, 2017

Squashed.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 26, 2017

Coverage Status

Coverage increased (+0.02%) to 78.51% when pulling 35700cb on garlick:kvs_cmd_update into b12d027 on flux-framework:master.

char *val, *kv, *p;
bool overflow = false;

assert (maxcol == 0 || maxcol > 3);

This comment has been minimized.

@chu11

chu11 Oct 26, 2017

Contributor

just realized, I don't think there's a check for a reasonable maxcol in callers of this function. Should a check be added somewhere? Or perhaps there should just be a generic "can't output with such small width" kind of error message?

This comment has been minimized.

@garlick

garlick Oct 26, 2017

Author Member

Yes you're right.


*put* 'key=value' ['key=value...']::
*put* [-j|-r] 'key=value' ['key=value...']::

This comment has been minimized.

@chu11

chu11 Oct 26, 2017

Contributor

need -t and -n

This comment has been minimized.

@garlick

garlick Oct 26, 2017

Author Member

I could swear I already did that one. Thanks!

If '-j', it is first encoded as JSON, then stored as a NULL-terminated string.
If '-r', it is stored as raw data with no termination. For raw mode only,
a value of "-" indicates that the value should be read from standard input.
If '-t', value is stored as a RFC 11 object.

This comment has been minimized.

@chu11

chu11 Oct 26, 2017

Contributor

and description of -n.

This comment has been minimized.

@garlick

garlick Oct 26, 2017

Author Member

Right! Adding that.

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Oct 26, 2017

other than those nits above, I think things look good. Did you want to squash?

@garlick garlick force-pushed the garlick:kvs_cmd_update branch from ee42344 to 14b3581 Oct 26, 2017

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 26, 2017

Thanks! Re-squashed.

garlick added some commits Oct 24, 2017

cmd/flux-kvs: handle non-JSON values in "dir" cmd
Problem: the KVS allows non-JSON values to be stored, but
flux kvs dir -R exits with an error if it encounters one.

For each value, if it decodes as JSON, format it like before.
But if it doesn't, print it directly instead of exiting.
If the output is a terminal, truncate long values so they fit
within the terminal width (reusing logic from the ls subcommand).
Also truncate values where they contain unprintable characters
or newlines.  Indicate truncated values by appending "...".

Fixes #1158
cmd/flux-kvs: add get --treeobj option
If --treeobj is specified, display RFC 11 object
directly instead of parsing it.

Allow directories to be listed too.
cmd/flux-kvs: add put --treeobj option
If --treeobj is specified store value argument as a
RFC 11 object.

Example, assign snapshot of 'lwj' to 'lwj.snapshot':
  flux kvs put --treeobj \
    lwj.snapshot=$(flux kvs get --treeobj lwj)
cmd/flux-kvs: get handles zero-length value
Problem: flux kvs get (no options) segfaults if key refers
to a zero-length value.

If value is zero-length, print nothing instead of handing
a NULL pointer to printf.
cmd/flux-kvs: get --json handles zero-length value
Problem: if value is zero-length, get --json displays
"null", but it should be an error like any other value
that doesn't decode as valid JSON, for example the empty
string.

output_key_json_string() is doing double duty with the
watch subcommand, which still treats a NULL value as a
non-existent key.  Don't pass a NULL value to that function;
instead, treat it as an error beforehand.
t/t1000-kvs.t: add treeobj tests
Ensure that flux kvs get --treeobj and flux kvs put --treeobj
work as designed.
t/t1000-kvs.t: add empty string value tests
Ensure that an empty string value (just a NULL) can be created
and parsed where appropriate.
t/kvs/basic: drop get-treeobj, put-treeobj subcommands
Replace "t/kvs/basic get-treeobj" with "flux kvs get --treeobj"
and "t/kvs/basic put-treeobj" with "flux kvs put --treeobj"
in sharness test.

Drop get-treeobj, put-treeobj subcommands from test program.
t/kvs/basic: drop dirsize subcommand
Replace "t/kvs/basic dirsize" with "flux kvs ls -1 | wc -l"
and drop the dirsize subcommand from the test program.
cmd/flux-kvs: add get/readlink/dir [--at REF] option
Add flux-kvs get/readlink/dir --at REF option that allows
a lookup to start with a snapshot reference.
t/kvs/basic: drop getat/readlinkat/dirat subcommands
Replace "t/kvs/basic getat" with "flux kvs get --at",
"t/kvs/basic readlinkat" with "flux kvs readlink --at",
and "t/kvs/basic dirat" with "flux kvs dir --at".

Drop getat, readlinkat, dirat subcommands from the test program.
cmd/flux-kvs: add put --no-merge flag
Add put --no-merge flag which ensures that flux_commit() is
called with the FLUX_KVS_NO_MERGE flag.
t/kvs/basic: drop put-no-merge subcommand
Replace "t/kvs/basic put-no-merge" with "flux kvs put
--no-merge --json" and drop the put-no-merge subcommand from
the test program.
t/kvs/basic: drop copy-fromkvs/copy-tokvs subcommands
Replace "t/kvs/basic copy-fromkvs" with "flux kvs get --raw"
and "t/kvs/basic copy-tokvs key -" with "flux kvs put --raw key=-",
and drop copy-fromkvs/copy-tokvs subcommands from test program.
t/t1002-kvs-extra.t: drop redundant test
Drop this test
  kvs: zero size raw value can be stored and retrieved

It is basically the same as this one in t1000-kvs.t:
  kvs: zero-length value handled by put/get --raw
doc/flux-kvs(1): update for new options
Describe get/put [--json|--raw|--treeobj] options.
Describe get/dir/readlink [--at treeobj] option.
Describe dir truncation to fit output window and [-w COL] option.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 26, 2017

Coverage Status

Coverage increased (+0.04%) to 78.528% when pulling 14b3581 on garlick:kvs_cmd_update into b12d027 on flux-framework:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 26, 2017

Coverage Status

Coverage increased (+0.05%) to 78.539% when pulling 14b3581 on garlick:kvs_cmd_update into b12d027 on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Oct 26, 2017

everything, lgtm!

@chu11 chu11 merged commit aeb79c3 into flux-framework:master Oct 26, 2017

4 checks passed

codecov/patch 83.87% of diff hit (target 77.86%)
Details
codecov/project 77.92% (+0.06%) compared to b12d027
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.05%) to 78.539%
Details

@garlick garlick deleted the garlick:kvs_cmd_update branch Oct 27, 2017

garlick added a commit to garlick/flux-sched that referenced this pull request Nov 15, 2017

t/t1004-module-load.t: use kvs get -j for lwj status
Problem: The "module-load: no jobs are lost" subtest
of sharness t1004-module-load.t is failing.

The test is comparing "status" (with quotes) to status
(without quotes).  The value in the KVS is a JSON string,
but flux-framework/flux-core#1257 changed flux-kvs so
that it displays raw values by default, in this case the
encoded JSON string with the quotes.

Add -j to the flux kvs get command in the test, to get
the old behavior.

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