python: KVSDir object should write to path it is initialized with

[garlick]

Problem: a KVSDir object acts like a dictionary for a KVS directory when reading from the KVS, but it discards its relative path and operates directly on the root when writing to the KVS.

For example, this program uses a KVSDir to print a key from a job directory and then update the same key:

#!/usr/bin/env python3

import sys
import flux.job
import flux.job.JobID as JobID

jobid = JobID(sys.argv[1])
key = sys.argv[2]
val = sys.argv[3]

handle = flux.Flux()
jobdir = flux.job.job_kvs(handle, jobid)
# This  refers to a key name in the job's KVS directory
print(jobdir[key])

# But this refers to a key name in the root directory
jobdir[key] = val
jobdir.commit()

$ flux ./foo.py  ƒ6acjkLQf jobspec xyz
{'resources': [{'type': 'slot', 'count': 1, 'with': [{'type': 'core', 'count': 1}], 'label': 'task'}], 'tasks': [{'command': ['hostname'], 'slot': 'task', 'count': {'per_slot': 1}}], 'attributes': {'system': {'duration': 0, 'cwd': '/nfshome/garlick', 'shell': {'options': {'rlimit': {'cpu': -1, 'fsize': -1, 'data': -1, 'stack': -1, 'core': 16384, 'nofile': 128000, 'as': -1, 'rss': -1, 'nproc': 8192}}}}}, 'version': 1}

# hmm no update
$ flux job info ƒ6acjkLQf jobspec
{"resources":[{"type":"slot","count":1,"with":[{"type":"core","count":1}],"label":"task"}],"tasks":[{"command":["hostname"],"slot":"task","count":{"per_slot":1}}],"attributes":{"system":{"duration":0,"cwd":"/nfshome/garlick","shell":{"options":{"rlimit":{"cpu":-1,"fsize":-1,"data":-1,"stack":-1,"core":16384,"nofile":128000,"as":-1,"rss":-1,"nproc":8192}}}}},"version":1}

# oops unexpected jobspec key in the root!
$ flux kvs get jobspec
"xyz"

[vsoch]

Thanks for opening this @garlick !

Update: for future reference, my test cases are here: https://github.com/researchapps/test-kvs-commit

[garlick]

Thanks for finding it!

[chu11]

Looking through code history, this appears to be an oversight from the initial implementation in 2015. Surprised it took so long to find :-) Also no path = .... tests in t/python/, so should do that too.

[vsoch]

That's me, the bug finder!! 😆 🐛 🐞 Actually more like I'm really good at breaking things. 🔨

[vsoch]

Workaround - works! researchapps/test-kvs-commit@9a8d1be

Thanks for the help! This should work for our current needs- it works for both the case that the key exists and the case that it does not.

[chu11]

note: looking through code, I think fill() and mkdir() on KVSDir probably also don't work.

[chu11]

thinking about this a bit ... this change has the potential to break a ton of code, if people are using it. Which I don't know if people are.

i.e.

dir = KVSDir(handle, path="foo.bar")
dir["foo.bar.a"] = 1
dir.commit()

before this created "foo.bar.a" and if we change it'll create "foo.bar.foo.bar.a".

Perhaps we could ping a few code teams and see if they use KVSDir with a path argument and do writes? If we can confirm a few teams don't use the path argument, then I'd feel better that this isn't going to break the world.

If a few teams are using it, then we perhaps should have a migration path to warn them of the change.

Edit: ugh and likewise if people use the results from job_kvs() like example above

[vsoch]

If the bug hasn't been caught for 8 years, do you think people are using it?

[chu11]

If the bug hasn't been caught for 8 years, do you think people are using it?

I agree low probability. Two things made me pause.

1. With using KVSDir directly, I think this issue is a little less obvious. You going through job_kvs() makes it a bit more obvious. So it may have not been an obvious issue to any users using KVSDir directly.

2. (this is the bigger one I paused on) there's a KVSDir function called fill() which allows the user to pass in a Mapping and put each "key=val" mapping into the KVS. It's not a dumb setter/getter that one would "just add" to a newly created class. I think there's a decent chance it was added by a user request OR added b/c of a specific need.

Edit: also should mention mkdir() also allows you to pass in a Mapping to write stuff to directory post-mkdir. Again IMO not an obvious thing to just add, so added by user request or specific need?

Edit2: scratch the mkdir() part, it didn't work at all given the initial "relative path" problem ...