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 symlinks with namespaces within them #1949

Merged
merged 10 commits into from Jan 29, 2019

Conversation

@chu11
Copy link
Contributor

commented Jan 23, 2019

This is the third part to issue #1860, implementing namespace symlink support as described by RFC change flux-framework/rfc#147.

Basically, we're changing the treeobj of a symlink to:

{ "ver":1,
  "type":"symlink",
  "data":{"namespace":"a","target":"b.b"},
}

where the "namespace" field is optional. Adjustments to all functions that relate to symlinks are done (treeobj_create_symlink(), flux_kvs_lookup_get_symlink(), and flux_kvs_txn_symlink()
are the obvious ones). Subsequently new options to flux kvs link and flux kvs readlink.

The one part of this PR I really like is how so little changed in modules/kvs and there were no changes in modules/kvs-watch. So I think this was a much better choice than the ns prefix we did earlier.

The one part of this PR that folks may find weird is in the function treeobj_get_symlink_namespace(). If the symlink does not contain a namespace, I elect to return NULL and set errno to ENODATA, instead of EINVAL. So that callers have a way to differentiate between no namespace and an invalid treeobj. I'm not 100% if this is the right errno or the right pattern to use. Thoughts?

Also up for discussion may be the "ns::key" output format I use in flux-kvs.

@codecov-io

This comment has been minimized.

Copy link

commented Jan 23, 2019

Codecov Report

Merging #1949 into master will increase coverage by <.01%.
The diff coverage is 82.69%.

@@            Coverage Diff             @@
##           master    #1949      +/-   ##
==========================================
+ Coverage      80%   80.01%   +<.01%     
==========================================
  Files         195      195              
  Lines       34913    34987      +74     
==========================================
+ Hits        27933    27994      +61     
- Misses       6980     6993      +13
Impacted Files Coverage Δ
src/bindings/lua/flux-lua.c 82.32% <100%> (ø) ⬆️
src/common/libkvs/kvs_lookup.c 81.86% <100%> (+1.09%) ⬆️
src/common/libkvs/kvs_txn.c 73.26% <100%> (ø) ⬆️
src/common/libkvs/kvs_classic.c 88.62% <100%> (ø) ⬆️
src/modules/kvs/kvstxn.c 77.38% <100%> (+0.22%) ⬆️
src/common/libkvs/treeobj.c 83.15% <67.74%> (-2%) ⬇️
src/cmd/flux-kvs.c 82.81% <83.33%> (+0.01%) ⬆️
src/modules/kvs/lookup.c 80.84% <88.23%> (+0.59%) ⬆️
src/common/libflux/message.c 81.27% <0%> (-0.25%) ⬇️
... and 1 more
@garlick

This comment has been minimized.

Copy link
Member

commented Jan 23, 2019

If the symlink does not contain a namespace, I elect to return NULL and set errno to ENODATA, instead of EINVAL

ENODATA might be a bad choice because it is the RFC 6 end-of-stream indicator. Some code, e.g. see lookup_continuation() in cmd/flux-kvs.c, needs to treat this as a non-error.

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2019

ENODATA might be a bad choice because it is the RFC 6 end-of-stream indicator. Some code, e.g. see lookup_continuation() in cmd/flux-kvs.c, needs to treat this as a non-error.

Doh! I totally forgot about that. Hmmm I guess ENOENT might be the safer choice.

@garlick

This comment has been minimized.

Copy link
Member

commented Jan 23, 2019

treeobj_get_symlink_namespace()

Or maybe just change

const char *treeobj_get_symlink (const json_t *obj);

to

int treeobj_get_symlink (const json_t *obj, const char **namespace, const char **target);

and set namespace to NULL if there isn't one? (not an error)

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2019

int treeobj_get_symlink (const json_t *obj, const char **namespace, const char **target);

Oh, that's a good idea. Lets go with that.

@chu11 chu11 force-pushed the chu11:issue1860_part3 branch from ee1588d to 69dac65 Jan 23, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2019

re-pushed with new int treeobj_get_symlink (const json_t *obj, const char **namespace, const char **target); implementation.

@garlick

This comment has been minimized.

Copy link
Member

commented Jan 25, 2019

This might be slightly confusing:

Usage: flux-kvs link [-L ns] target linkname
Create a new name for target
  -L, --link-namespace   Specify link's namespace
  -h, --help             Display this message.

So if I want blah -> a::b, I would say

$ flux kvs link -L a b blah

A minor tweak would be to change "Specify link's namespace" with "specify target's namespace".

Or, would it be better if flux kvs dir is going to show links in the above double-colon form, maybe that's what the optional namespace ought to look like in flux-kvs link? E.g.

$ flux kvs link a::b blah

Thoughts?

@garlick

This comment has been minimized.

Copy link
Member

commented Jan 25, 2019

It actually might make sense for the tool (not the API) to always take a ns:: prefix whereever there is a key? Then drop the "global" --namespace=ns option?

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2019

A minor tweak would be to change "Specify link's namespace" with "specify target's namespace".

Yeah, "target namespace" makes a lot more sense.

It actually might make sense for the tool (not the API) to always take a ns:: prefix whereever there is a key? Then drop the "global" --namespace=ns option?

I dislike the idea that the user would have to know a special format to specify the namespace. I feel like there should always be an option.

As for supporting a ns::key format as an option, I'm hesitant b/c there's always the corner cases that end up popping up.

ns::, error or does this assume the root dir? (there was ns prefix similar issues in #1434)
ns:::key, error or is the key :key? Or is the namespace ns:?

And we get into some of the same trappings we had before.

Perhaps the issue is the output of ns::key in dir or ls. Should the output be different? A format that a user would (probably) never try to use as input?

blah -> (MYNS) key
blah -> key (MYNS)
blah -> { namespace = MYNS, target = key }

As an aside, I admittedly dislike the global "--namespace" option b/c it's used somewhat weirdly. i.e.

flux kvs --namespace=FOO get a.key

The --namespace comes before the get. I think it would be more natural for

flux kvs get --namespace=FOO a.key

but that would mean every sub-commands would have to handle the namespace option. Perhaps my desire to have cleaner coding got in the way. Perhaps each sub-command should simply parse the namespace option instead.

@garlick

This comment has been minimized.

Copy link
Member

commented Jan 25, 2019

I dislike the idea that the user would have to know a special format to specify the namespace. I feel like there should always be an option.

OK, that sounds like a reasonable position. I withdraw that suggestion :-)

I don't necessarily have a strong opinion on the output format. ns::key seems fine to me.

Since we both don't like the global --namespace option, maybe we can open that up as a separate issue.

@garlick

This comment has been minimized.

Copy link
Member

commented Jan 25, 2019

Was noticing vim colorizes the word namespace. Didn't we before decide that this name should be avoided in variable names since it's a C++ keyword?

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2019

Was noticing vim colorizes the word namespace. Didn't we before decide that this name should be avoided in variable names since it's a C++ keyword?

I don't recall agreeing to that, but it does seem like a good idea. I'll adjust code accordingly.

@garlick

This comment has been minimized.

Copy link
Member

commented Jan 25, 2019

See 658e4f5 and 113c997

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2019

Was going to try and correct namespace variable name usage everywhere in this PR, but it's in more places than I realized. So I only corrected usage in this PR. Opened #1963 to deal with it elsewhere.

@chu11 chu11 force-pushed the chu11:issue1860_part3 branch from 69dac65 to 588f5c8 Jan 25, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2019

re-pushed with --link-namespace renamed to --target-namespace. In addition, all variables namespace have been renamed ns, with appropriate documentation tweaks in manpages.

@garlick

This comment has been minimized.

Copy link
Member

commented Jan 26, 2019

One builder failed in t1004-kvs-namespace.t. Looks like all the tests passed but the broker had to be killed with a SIGTERM? Not a lot to go on.

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Jan 26, 2019

Hmmm, it's worrisome since this PR is related to the KVS. But it appears this has shown up a few times before, including recently (#1936, #1931, #1846) where the tests pass, so the error is from the shutdown of the broker. Of the few log entries we have with details (including this one):

flux-broker: module 'content-sqlite' was not cleanly shutdown
flux-broker: module 'kvs' was not cleanly shutdown

it always appears to be these two modules not cleanly being shutdown. @grondo suggests grace timeout issue in #1931. I guess I don't quite know the behavior of the broker in these circumstances. If a module unload takes too long, does it SIGTERM?

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Jan 26, 2019

It seems if a runlevel takes too long it gets a SIGTERM ... possible travis is just taking to long to run rc3?

@garlick

This comment has been minimized.

Copy link
Member

commented Jan 28, 2019

The "not cleanly shutdown" messages are hints that it was indeed the rc3 timeout. Whether it's travis being slow or one of the module unloads getting stuck (kvs unloads before content-sqlite) so prob kvs), it's hard to tell.

So maybe it's worth pondering whether anything in this PR could conceivably cause the kvs module to get stuck, and then move on if not?

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Jan 28, 2019

So maybe it's worth pondering whether anything in this PR could conceivably cause the kvs module to get stuck, and then move on if not?

I can't think of anything. And the fact it occurred in several other issues recently suggests it's something outside of these changes.

I wasn't too familiar with how an unload of a module works, but it appears that a module unload sends a rpc to <modulename>.shutdown? The kvs module doesn't have a shutdown callback, so I assume the module unload should just be quick?

I also noticed in rc3:

flux module remove -r all -x 0 kvs
if test -n "$PERSISTDIR"; then
    flux kvs getroot >${PERSISTDIR}/kvsroot.final
    flux content flush
fi
flux module remove -r 0 kvs
flux module remove -r 0 content-sqlite

the fact we only see "not cleanly shutdown" once for the kvs module suggest to me that the first flux module remove -r all -x 0 kvs probably succeeded but the latter kvs module remove did not run. The default shutdown grace is only 1 second for most of the tests (size < 16). The addition of the content flush sounds like something that could take up a nice chunk of time? Perhaps we should increase the grace shutdown time (another issue of course)

@garlick

This comment has been minimized.

Copy link
Member

commented Jan 28, 2019

The default shutdown handler calls flux_reactor_stop() so should be quick, unless the module is doing something that blocks (like a blocking RPC).

Let's consider that failure unrelated to this PR then.

@chu11 chu11 force-pushed the chu11:issue1860_part3 branch from 588f5c8 to ca64dd7 Jan 28, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Jan 28, 2019

rebased and re-pushed

@chu11 chu11 force-pushed the chu11:issue1860_part3 branch from 7dcc877 to 05f6cac Jan 29, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2019

oops, rebase bug, repushed

Copy link
Member

left a comment

I made a few minor review comments - see what you think.
Overall this is looking good. Nice work!

`flux_kvs_txn_symlink()` sets _key_ to a symbolic link pointing to a
namespace _ns_ and a _target_ key within that namespace. Neither
_ns_ nor _target_ must exist. The namespace _ns_ is optional, if
set to NULL the _target_'s current namespace is assumed.

This comment has been minimized.

Copy link
@garlick

garlick Jan 29, 2019

Member

Should it be the key's target namespace is assumed?

This comment has been minimized.

Copy link
@chu11

chu11 Jan 29, 2019

Author Contributor

ahh, you're right!

This comment has been minimized.

Copy link
@chu11

chu11 Jan 29, 2019

Author Contributor

what I really meant to say was, "the target is assumed to be in the key's current namespace."

@@ -32,7 +32,8 @@ SYNOPSIS
const char *key);

This comment has been minimized.

Copy link
@garlick

garlick Jan 29, 2019

Member

Not critical but "common" is not that helpful in the commit title. Maybe go with "libkvs/txn:" for this one?

@@ -171,9 +171,9 @@ void _treeobj_insert_entry_val (json_t *obj, const char *name,
* created symlink can be properly dereferenced

This comment has been minimized.

Copy link
@garlick

garlick Jan 29, 2019

Member

Just a thought (feel free to ignore) but kvs/lookup test is pretty huge. Should it be split?

This comment has been minimized.

Copy link
@chu11

chu11 Jan 29, 2019

Author Contributor

It's smaller than when we had ns prefix support :-) But yeah, it is quite big. And the kvstxn.c tests too. I'll take a look in the future to splitting it up. I never split it up before b/c there wasn't an obvious split in the tests.

Update symlink treeobj to have object for data instead of
a string.  The object has a target and an optional namespace.

Add tests and update callers.
chu11 added 9 commits Jan 8, 2019
Support namespace in symlinks in kvs txn requests.  Add unit tests
appropriately.
Add namespace parameter to flux_kvs_lookup_get_symlink().

Update callers appropriately.
Add ability for readlink to read symlink tree objects with
namespaces.  Add options to limit output to only keys or only
namespaces if the user desires.
Support namespaces in symlinks in txns and lookups.  Add / update
unit tests appropriately.
@chu11 chu11 force-pushed the chu11:issue1860_part3 branch from 05f6cac to bae80e4 Jan 29, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2019

repushed with suggested changes to commit comments and tweak to documentation

@garlick

This comment has been minimized.

Copy link
Member

commented Jan 29, 2019

LGTM - thanks!

@garlick garlick merged commit 3202bb1 into flux-framework:master Jan 29, 2019
3 checks passed
3 checks passed
codecov/patch 82.69% of diff hit (target 80%)
Details
codecov/project 80.01% (+<.01%) compared to 4f7a31c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.