kvs: add namespace parameter to most kvs api functions #1977

chu11 · 2019-02-01T01:03:35Z

Add a namespace parameter to most kvs functions, such as flux_kvs_lookup(), flux_kvs_fence(), flux_kvs_commit(), flux_kvs_get_version(), flux_kvs_wait_version(), and a variety of flux_kvs_watch functions. Long ago, this was avoided to not break ABI. But recent changes, including the use of namespaces in flux_kvs_copy/move() showed the necessity of finally biting the bullet on this.

As a side effect of this, remove flux_kvs_set_namespace(), which is no longer needed as namespaces can be passed directly to all these functions. Now callers pass in a namespace to a function, or set it via an environment variable, or they get the default of primary. There is no namespace attached to a flux handle anymore.

As a side effect of removing flux_kvs_set_namespace(), the --namespace option in flux-kvs no longer works. We instead make the --namespace option in flux-kvs an option for all subcommands that can use it. In my opinion, this is much better. The --namespace option only applies to those subcommands that can use it (i.e. specify --namespace for get/put, but there is no option for dropcache). In addition, I think the flow of the command makes a lot more sense. Instead of:

flux kvs --namespace=FOO get a.key

it's now

flux kvs get --namespace=FOO get a.key

The namespace option should be specified to the "right" of the subcommand.

Also fixed a few dinky things along the way.

I will be sending a PR to update flux-sched as well on this.

chu11 · 2019-02-01T01:27:08Z

repushed, python formatting error

chu11 · 2019-02-01T02:14:52Z

hmmmm error in t2000-wreck hit.

2019-02-01T01:51:33.581578Z connector-local.err[0]: send kvs.watch response to client CF36A: Broken pipe
2019-02-01T01:51:37.390247Z job.err[0]: job16: wrexecd says: wrexecd: ev.c:3875: void ev_io_start(struct ev_loop *, ev_io *): Assertion `("libev: ev_io_start called with negative fd", fd >= 0)' failed.
2019-02-01T01:56:29.033020Z broker.err[0]: runlevel 2 timeout, sending SIGTERM
2019-02-01T01:56:29.033409Z broker.err[0]: Run level 2 Unknown signal 143 (rc=143) 300.0s
flux-broker: src/zframe.c:154: zframe_send: Assertion `dest' failed.
flux-broker: src/zframe.c:154: zframe_send: Assertion `dest' failed.
flux-broker: src/zframe.c:154: zframe_send: Assertion `dest' failed.
flux-start: 0 (pid 16508) exited with rc=143
flux-start: 3 (pid 16514) Aborted
flux-start: 1 (pid 16509) Aborted
flux-start: 2 (pid 16510) Aborted

concerning b/c I have course modified code in area of kvs.watch. Possible a bug in connector.local? Mentioned in #1641, #1386 (which points to #542).

chu11 · 2019-02-01T06:31:56Z

concerning b/c I have course modified code in area of kvs.watch

hmmm. While I have modified code in libkvs/kvs_watch, the changes are pretty simple / stupid overall. It's hard to see how I could have introduced a mem-corruption or something similarly terrible.

garlick · 2019-02-01T15:19:36Z

I wonder if wrexecd is calling flux_log() to log an error after it has already closed the flux_t handle? If so, then it could be related to this PR if some KVS operation is now failing and sending wrexecd down a heretofore untraveled error path...

Not saying that's what's happening, but maybe worth giving the KVS functions used by wrexecd one more visual check (like the flux_kvsdir_*() functions for example)

chu11 · 2019-02-01T16:22:26Z

I wonder if wrexecd is calling flux_log() to log an error after it has already closed the flux_t handle? If so, then it could be related to this PR if some KVS operation is now failing and sending wrexecd down a heretofore untraveled error path...

If the flux_t handle were closed, hopefully wrexecd isn't doing any kvs calls? :-)

The removal of flux_kvs_set_namespace() is a behavior change, but I don't believe any code was using it except for the kvs command. wrexecd is only operating on the primary kvs?

chu11 · 2019-02-01T16:32:35Z

The removal of flux_kvs_set_namespace() is a behavior change, but I don't believe any code was using it except for the kvs command. wrexecd is only operating on the primary kvs?

Actually, I forgot to point this out originally. The removal of flux_kvs_set_namespace() means that a large number of kvs operations (most notable all kvs classic functions) can only work on the primary namespace or the namespace specified via the environment variable. If some code were to mix kvs-new and kvs-classic API calls, there could be issues. For example:

f = flux_kvs_lookup (h, A_NAMESPACE, key);
...
flux_kvs_get (h, key, &json_str);

the kvs-classic get cannot be passed A_NAMESPACE. So the only way to deal with this is via the environment variable. But again, the only use of flux_kvs_set_namespace() was the flux-kvs command, so I don't believe this to be an issue.

garlick · 2019-02-01T16:34:54Z

If the flux_t handle were closed, hopefully wrexecd isn't doing any kvs calls? :-)

I was suggesting a kvs call fails in a new way, then handle is closed, then a use after free, such as a flux_log() call. That might produce the above assertion from libev. But there isn't much to go on in the log, so hypothetical!

garlick · 2019-02-01T16:47:59Z

Skimming through, I see what you mean - not a lot to worry about!

One minor thing flux_kvs_get_namespace() probably shouldn't be a public function and shouldn't take a flux_t argument since it just looks up the environment variable and substitutes primary if unset.

garlick · 2019-02-01T16:51:10Z

Is the fact that (in the kvs module) unwatch_cmp() doesn't compare namespaces a problem?

chu11 · 2019-02-01T17:17:25Z

One minor thing flux_kvs_get_namespace() probably shouldn't be a public function and shouldn't take a flux_t argument since it just looks up the environment variable and substitutes primary if unset.

Now that you mention it, I agree, it's not a useful "convenience" function to make public anymore. I'll yoink it.

chu11 · 2019-02-01T17:25:20Z

Is the fact that (in the kvs module) unwatch_cmp() doesn't compare namespaces a problem?

Hmmm.

    if (wait_destroy_msg (root->watchlist, unwatch_cmp, &p) < 0) {
        errnum = errno;
        flux_log_error (h, "%s: wait_destroy_msg", __FUNCTION__);
        goto done;
    }
    if (cache_wait_destroy_msg (ctx->cache, unwatch_cmp, &p) < 0) {
	errnum = errno;
        flux_log_error (h, "%s: cache_wait_destroy_msg", __FUNCTION__);
        goto done;
    }

The call to wait_destroy_msg() is fine, since that's a per-namespace watchlist. The cache_wait_destroy_msg() though is not per namespace. So I think it could be a problem.

BUT the cache is shared between lookups, commits, watches, etc. and not every operation requires a namespace to be specified, most notably lookupat() (in just finalized #1971).

This is probably for another issue. Or do we just punt until we remove the old watch interface?

chu11 · 2019-02-01T17:38:36Z

Actually, the call to cache_wait_destroy_msg() may not be safe at all. B/c there could be a wait sitting in the cache that was just written, and that is completely independent of of the watch. The above code may have assumed that all waiters were associated with a watch.

garlick · 2019-02-01T21:44:50Z

So based on your comment in #1980 do you think we should go ahead and merge this? (If so maybe rebase after #1982 goes in?)

chu11 · 2019-02-01T21:49:20Z

yeah, I think the unwatch issue is a separate issue. I'll re-push this after #1982 goes in.

Values being required to be JSON formatted was removed during tree object conversion in the KVS.

Add a namespace parameter to flux_kvs_commit() and flux_kvs_fence(). If the ns argument is NULL, follow prior rules on selecting a default namespace. As a side effect remove flux_kvs_commit_ns(). Update all docs, callers, and tests appropriately.

Add namespace parameter to flux_kvs_get_version() and flux_kvs_wait_version(). Update callers appropriately.

Add a namespace parameter to flux_kvs_watch family of functions. Update callers appropriately.

Add a namespace parameter to flux_kvs_lookup(). If the ns argument is NULL, follow prior rules on selecting a default namespace. As a side effect remove flux_kvs_lookup_ns(). Update all docs, callers, and tests appropriately.

With flux kvs functions now taking namespaces, remove use of flux_kvs_set_namespace() in several tests.

Support a -N ns option for any kvs commands that support namespaces. This is in preperation for the removal of the global -N option.

Use the --namespace option assigned to individual flux-kvs sub-commands and not the global --namespace option.

Remove calls to it and remove documentation as well.

Remove global --namespace option and document --namespace option in the sub-commands that it is supported in.

As namespaces can only be set via environment variable now, the need for a public function like flux_kvs_get_namespace() is no longer necessary. Create a private convenience function kvs_get_namespace() for internal use.

chu11 · 2019-02-01T23:16:00Z

re-pushed, removing flux_kvs_get_namespace(). Also realized I had forgotten to remove kvs_lookup_private.h in the prior pull request.

garlick · 2019-02-02T00:00:53Z

Thanks!

chu11 mentioned this pull request Feb 1, 2019

resource/sched/simulator: Update KVS API usage flux-framework/flux-sched#435

Merged

chu11 force-pushed the issue1435 branch from 53f590e to 4028ae3 Compare February 1, 2019 01:26

chu11 force-pushed the issue1435 branch from 4028ae3 to 4406535 Compare February 1, 2019 01:42

This was referenced Feb 1, 2019

flux_rpc_pack: NULL flux handle leads to segfaults #1979

Closed

kvs: unwatch inconsistencies / unsafe #1980

Closed

chu11 added 11 commits February 1, 2019 15:14

doc/man1/flux-kvs: Remove text about JSON values

2ff064d

Values being required to be JSON formatted was removed during tree object conversion in the KVS.

cmd/flux-kvs: Add missing options in help output

f8fc0e6

libkvs: Add namespace parameter to version funcs

831e662

Add namespace parameter to flux_kvs_get_version() and flux_kvs_wait_version(). Update callers appropriately.

libkvs: Add namespace arg to watch functions

3054bd3

Add a namespace parameter to flux_kvs_watch family of functions. Update callers appropriately.

libkvs/lookup: Add ns arg to kvs lookup

af17da3

Add a namespace parameter to flux_kvs_lookup(). If the ns argument is NULL, follow prior rules on selecting a default namespace. As a side effect remove flux_kvs_lookup_ns(). Update all docs, callers, and tests appropriately.

t/kvs: Remove use of flux_kvs_set_namespace()

c7cdc93

With flux kvs functions now taking namespaces, remove use of flux_kvs_set_namespace() in several tests.

cmd/flux-kvs: Add -N option to most commands

c96bf0e

Support a -N ns option for any kvs commands that support namespaces. This is in preperation for the removal of the global -N option.

t/kvs: Convert how kvs tests specify namespace

b3f2c96

Use the --namespace option assigned to individual flux-kvs sub-commands and not the global --namespace option.

libkvs: Remove flux_kvs_set_namespace()

a0801f2

Remove calls to it and remove documentation as well.

doc/man1/flux-kvs: Update namespace usage

051b782

Remove global --namespace option and document --namespace option in the sub-commands that it is supported in.

libkvs: Remove flux_kvs_get_namespace()

7528813

As namespaces can only be set via environment variable now, the need for a public function like flux_kvs_get_namespace() is no longer necessary. Create a private convenience function kvs_get_namespace() for internal use.

chu11 force-pushed the issue1435 branch from 4406535 to 7528813 Compare February 1, 2019 23:15

garlick merged commit 5ed7f26 into flux-framework:master Feb 2, 2019

chu11 mentioned this pull request Feb 2, 2019

kvs: improve API for namespaces #1435

Closed

chu11 mentioned this pull request Feb 13, 2019

kvs: remove watch handlers & api #2011

Merged

chu11 deleted the issue1435 branch June 5, 2021 18:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kvs: add namespace parameter to most kvs api functions #1977

kvs: add namespace parameter to most kvs api functions #1977

chu11 commented Feb 1, 2019

chu11 commented Feb 1, 2019

chu11 commented Feb 1, 2019

chu11 commented Feb 1, 2019

garlick commented Feb 1, 2019

chu11 commented Feb 1, 2019

chu11 commented Feb 1, 2019 •

edited

garlick commented Feb 1, 2019 •

edited

garlick commented Feb 1, 2019

garlick commented Feb 1, 2019

chu11 commented Feb 1, 2019

chu11 commented Feb 1, 2019

chu11 commented Feb 1, 2019

garlick commented Feb 1, 2019

chu11 commented Feb 1, 2019

chu11 commented Feb 1, 2019

garlick commented Feb 2, 2019

kvs: add namespace parameter to most kvs api functions #1977

kvs: add namespace parameter to most kvs api functions #1977

Conversation

chu11 commented Feb 1, 2019

chu11 commented Feb 1, 2019

chu11 commented Feb 1, 2019

chu11 commented Feb 1, 2019

garlick commented Feb 1, 2019

chu11 commented Feb 1, 2019

chu11 commented Feb 1, 2019 • edited

garlick commented Feb 1, 2019 • edited

garlick commented Feb 1, 2019

garlick commented Feb 1, 2019

chu11 commented Feb 1, 2019

chu11 commented Feb 1, 2019

chu11 commented Feb 1, 2019

garlick commented Feb 1, 2019

chu11 commented Feb 1, 2019

chu11 commented Feb 1, 2019

garlick commented Feb 2, 2019

chu11 commented Feb 1, 2019 •

edited

garlick commented Feb 1, 2019 •

edited