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 namespace prefixes in symlinks #1432

Merged
merged 11 commits into from Apr 7, 2018

Conversation

4 participants
@chu11
Copy link
Contributor

chu11 commented Apr 5, 2018

This is the final big part to issue #1341, supporting namespace prefixes in symlinks.

There are a couple of small refactoring patches in this PR as setup.

Also, the documentation for namespace prefixes was added into a patch in this PR.

@chu11 chu11 added this to To do in multi-user parallel jobs via automation Apr 5, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Apr 5, 2018

Codecov Report

Merging #1432 into master will increase coverage by 0.04%.
The diff coverage is 89.51%.

@@            Coverage Diff             @@
##           master    #1432      +/-   ##
==========================================
+ Coverage   78.68%   78.72%   +0.04%     
==========================================
  Files         163      163              
  Lines       30114    30204      +90     
==========================================
+ Hits        23694    23777      +83     
- Misses       6420     6427       +7
Impacted Files Coverage Δ
src/cmd/flux-kvs.c 81.13% <100%> (+0.22%) ⬆️
src/modules/kvs/lookup.c 82.24% <88.17%> (+1.56%) ⬆️
src/modules/kvs/kvstxn.c 79.15% <90.47%> (+0.55%) ⬆️
src/common/libflux/request.c 87.17% <0%> (-1.29%) ⬇️
src/common/libutil/base64.c 95.07% <0%> (-0.71%) ⬇️
src/broker/overlay.c 74.14% <0%> (-0.32%) ⬇️
src/broker/module.c 83.79% <0%> (-0.28%) ⬇️
src/common/libflux/message.c 81.6% <0%> (-0.12%) ⬇️
src/cmd/flux-module.c 85.36% <0%> (+0.3%) ⬆️
... and 2 more
@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 5, 2018

Coverage Status

Coverage increased (+0.04%) to 79.023% when pulling 61a3efe on chu11:issue1341-part5 into 0e0f83f on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Apr 6, 2018

Nice! I'll check out and kick the tires manually later this morning.

One quick thought - do we need the environment variable mechanism to select namespaces now that we have two other ways to do it? There might be a use case, just trying to remember what it might be...

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Apr 6, 2018

Some preliminary poking - no prob with symlinks, though some other results are a little strange. Not sure if it's the tools or the KVS implementation. I added notes with ### prefix.

$ flux kvs namespace-create foo
$ flux kvs namespace-create foo   ### kvs.err log message should be removed
2018-04-06T16:01:53.751913Z kvs.err[0]: namespace_create_request_cb: namespace_create: File exists
flux-kvs: foo: File exists
$ flux kvs put ns
$ flux kvs put ns:foo/a=42
$ flux kvs --namespace=foo dir
a = 42
$ flux kvs dir ns:foo
flux-kvs: ns:foo: Invalid argument  ### should that work?
$ flux kvs dir ns:foo/
flux-kvs: ns:foo/: Invalid argument  ### seems like that should work
$ flux kvs dir ns:foo/.  ### seems like both double dots should not be displayed
ns:foo/..a = 42
$ flux kvs ls ns:foo/.  
ns:foo/: Invalid argument  ### that should work
$ flux kvs link  ns:foo/. linktofoo
$ flux kvs dir -R linktofoo
linktofoo.a = 42
@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Apr 6, 2018

$ flux kvs dir ns:foo
flux-kvs: ns:foo: Invalid argument ### should that work?
$ flux kvs dir ns:foo/
flux-kvs: ns:foo/: Invalid argument ### seems like that should work

So the way I parse things I assume if the user invalidly specifies a namespace or only specifies a namespace, it's an error. So the correct input would be ns:foo/. (i.e. a period at the end, indicating the "root" dir).

$ flux kvs dir ns:foo/. ### seems like both double dots should not be displayed
ns:foo/..a = 42

Dunno if this is #1391, will take a look.

$ flux kvs ls ns:foo/.
ns:foo/: Invalid argument ### that should work

Hmmm. This should work. Skimming code in flux-kvs it seems the ls command strips off trailing periods, leading to ns:foo/ being the input to the server side. The client side parsing of input keys wasn't something I thought of. Hmmm. May have to tweak the ls side.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Apr 6, 2018

Possibly flux kvs just needs some tweaking. For example, cmd_dir() has this

if (optindex == argc)
        key = ".";

I guess ns:foo/ should be converted to ns:foo/.?

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Apr 6, 2018

Hmmm. What I do on the server side is detect if an namespace-prefix is present, then normalize the "key-suffix". But I consider it an error if there is no suffix, like if the key is ```ns:foo/```` (no period).

So we'd be doing different behavior on the client side.

Hmmmm.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Apr 6, 2018

I wouldn't advocate to fix that up on the server side.

On the client side though, it just seems like

$ flux kvs dir
$ flux kvs dir ns:primary/

ought to be equivalent?

You can't pass an empty string to open(2) but you can run ls with no arguments...

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Apr 6, 2018

I think my comments thus far are a little off topic for this PR, so I opened some new issues for those.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Apr 6, 2018

Whee! Passed this test with flying colors:

#!/bin/bash -e
max=20
flux kvs namespace-create foo
flux kvs namespace-create bar
flux kvs put a=42
prev=ns:primary/a
for i in $(seq 1 ${max}); do
        if test $((${i}%2)) -eq 0; then
                target=ns:foo/$i
        else
                target=ns:bar/$i
        fi
        flux kvs link  $prev $target
        prev=$target
done

This creates 20 levels of symlink, each hopping between namespaces.

$ ./script
$ flux kvs dir
a = 42
resource.
$ flux kvs --namespace bar dir
1 -> ns:primary/a
11 -> ns:foo/10
13 -> ns:foo/12
15 -> ns:foo/14
17 -> ns:foo/16
19 -> ns:foo/18
3 -> ns:foo/2
5 -> ns:foo/4
7 -> ns:foo/6
9 -> ns:foo/8
$ flux kvs --namespace foo dir
10 -> ns:bar/9
12 -> ns:bar/11
14 -> ns:bar/13
16 -> ns:bar/15
18 -> ns:bar/17
2 -> ns:bar/1
20 -> ns:bar/19
4 -> ns:bar/3
6 -> ns:bar/5
8 -> ns:bar/7
$ flux kvs --namespace foo get 20
$ flux-kvs: 20: Too many levels of symbolic links
flux kvs --namespace foo get 10
42
$ flux kvs --namespace bar get 11
flux-kvs: 11: Too many levels of symbolic links
@garlick

This comment has been minimized.

Copy link
Member

garlick commented Apr 6, 2018

Also, running flux kvs --namespace foo get 10 in a tight loop after the above doesn't budge the flux-broker-0 rss as reported by top. Same with the failing case flux kvs --namespace foo get 11.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Apr 6, 2018

Skimming through the code, I should find a way to fix:

$ flux kvs ls ns:foo/.
ns:foo/: Invalid argument

Its minimally inconsistent to the rest of the commands. The other ones can probably be dealt later with the new issues that were created.

chu11 added some commits Mar 28, 2018

modules/kvs: Add root_ref to internal walk levels
In internal walk_level_t in the lookup API, add the root reference
and the root_dirent of the walk level to each walk_level_t.

As a consequence, the "root_dirent" in the lookup_t is no
longer needed.

This refactor allows each walk level to eventually have its own
independent root reference, which could be different depending
on the namespace.
modules/kvs: Support lookup of ns prefix in links
In lookups, if a symlink contains a namespace prefix, allow the lookup
to "cross" namespaces during the lookup.
modules/kvs: In txns, check for ns prefix in links
Check for namespace crossing in symlinks in transactions.  Don't
allow crossing of namespaces.
modules/kvs: Minor refactor of kvstxn unit tests
Setup kvsroot for each set of tests and specify namespace in
verify_value checks in preparation of namespace
prefix crossing tests.
modules/kvs: Remove unnecessary goto
Remove "goto stall" in walk(), it was called only once and performed
only a simple return.
modules/kvs: Refactor symlink handling in walk()
Minor refactoring, place symlink handling into a new function in walk().
cmd/flux-kvs: Fix flux kvs ls corner case
Previously flux kvs ls would strip trailing periods in key inputs,
which can affect cases in which user specifies a namespace prefix.

Add fix for this corner case and add appropriate unit tests.

@chu11 chu11 force-pushed the chu11:issue1341-part5 branch from 8b4628b to 61a3efe Apr 6, 2018

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Apr 6, 2018

Added a simple fix for the flux kvs ls ns:foo/. case. Will address deeper questions with #1434.

Also rebased on current master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Apr 7, 2018

LGTM. I'm heading out but I'll check in and push the button when it's green.

@garlick garlick merged commit 7160a8d into flux-framework:master Apr 7, 2018

4 checks passed

codecov/patch 89.51% of diff hit (target 78.68%)
Details
codecov/project 78.72% (+0.04%) compared to 0e0f83f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.04%) to 79.023%
Details

multi-user parallel jobs automation moved this from To do to Done Apr 7, 2018

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