job-info: add JSON_DECODE & CURRENT flags, deprecate job-info.update-lookup #5633

chu11 · 2023-12-20T00:24:49Z

This PR could be split up a bit more, but for now it's one PR.

This adds several new lookup flags, and with their addition, the job-info.update-lookup RPC target can be deprecated.

The one "tricky" part was how to grab cached data from current update-watches being done. Perhaps the lookup of that "cached" info could just be dropped as it probably has minimal use.

I decided to not support job-info.update-watch within the original job-info.lookup RPC for several reasons. Most notably because lookup can take multiple keys as input and job-info.update-watch takes only one key. I could make it so a WATCH flag only works with one key. Or we could have a "re-direct to this RPC" like we do with KVS watch. But I didn't delve too much into this design yet.

~~I then updated PR #5592 to be based on this PR.~~ PR #5635 then follows this one up

TODO RFC spec 41 which I was going to hold off on until this gets some minimal review.

chu11 · 2023-12-20T21:57:59Z

hmmm, should I have used the term "UPDATED" vs "UPDATE" as the flag? UPDATE may imply "to update" vs "UPDATED" is "updates have been applied".

garlick · 2023-12-21T15:19:03Z

Instead of update(d), maybe "current"?

garlick

First pass with some comments inline.

Also: we just added job-info.update-lookup in 0.56. Would it be reasonable to just update users and remove or were you thinking that's another PR?

garlick · 2023-12-21T17:38:01Z

src/common/libjob/job.h

+enum job_lookup_flags {
+    /* decode special fields
+     * - for jobspec and R, value of response will be decoded (i.e. a
+     * json object) instead of a string.
+     */
+    FLUX_JOB_LOOKUP_DECODE = 1,
+};


This probably shouldn't be in the public header since there are no API functions that accept this flag.

hmmm. Good point, but it is useful to have around instead of hard coding. Could add a private helper header, but I'm not sure the python bindings can use it if it's a non-public header?

garlick · 2023-12-21T17:45:10Z

src/modules/job-info/lookup.c

-        if (json_object_set_new (o, keystr, str) < 0) {
-            json_decref (str);
+        if (json_object_set_new (o, keystr, val) < 0) {
+            json_decref (o);


That should be json_decref (val).

garlick · 2023-12-21T17:49:13Z

src/common/libjob/job.h

+     * - for jobspec and R, value of response will include updates
+     * from the eventlog
+     */
+    FLUX_JOB_LOOKUP_UPDATE = 2,


same comment as before - inappropriate in installed header?

garlick · 2023-12-21T17:53:13Z

src/modules/job-info/lookup.c

+    if (!(updated_str = json_dumps (update_object, 0)))
+        goto error;


Since the updated object may have to be converted back to an object again depending on flags, maybe it would make sense to return this as an object instead of converting it to a string here?

considered this, it is mildly annoying to return the object given the JSON_DECODE logic that later follows in the caller. I'll add as a fixup commit, lemme know what you think.

garlick · 2023-12-21T17:56:04Z

src/modules/job-info/lookup.c

+    if (flux_msg_get_rolemask (l->msg, &rolemask) < 0)
+        return -1;
+
+    /* if rpc from owner, no need to do guest access check */
+    if (rolemask & FLUX_ROLE_OWNER) {
+        l->allow = true;
+        return 0;


Maybe this would be a bit simpler as

if (flux_msg_authorize (l->msg, FLUX_USERID_UNKNOWN) == 0) { l->allow = true; return 0; }

garlick · 2023-12-21T18:02:03Z

src/modules/job-info/lookup.c

+    if (!l->allow || (l->flags & FLUX_JOB_LOOKUP_UPDATE)) {
+        size_t index;
+        json_t *key;
+        json_array_foreach(l->keys, index, key) {


add space before open paren

garlick · 2023-12-21T18:09:15Z

src/modules/job-info/lookup.c

+            const char *keystr;
+            if (!(keystr = json_string_value (key))) {
+                errno = EINVAL;
+                return -1;
+            }
+            if (streq (keystr, "eventlog"))
+                return 0;
        }


It would probably be good to check that the request keys is an array of strings right in the request handler rather than deeper in the code, for clarity and to avoid repeating the same check in several places. Then this could just be:

if (streq (json_string_value (key), "eventlog")) return 0;

chu11 · 2023-12-22T16:50:10Z

re-pushed going with JSON_DECODE and CURRENT as the flag names.

chu11 · 2023-12-25T16:02:17Z

re-pushed with tweaks per comments above. There's one fixup patch that adds an update that I'm a little meh on, so I left it as a fixup for the time being so it can be reviewed as a "consideration".

grondo

I realized we should probably have merged this one before I split up flux-job.c, so giving this a review now (and sorry this languished so long!).

My one thought is that unfortunately the job-info.update-lookup RPC made it into released versions of flux-core, so we have to be careful about switching flux job and other users to the new flags in case a newer flux-core is used with an older. For flux-job however, I don't know how likely it is that there would be an issue so maybe it is fine?

grondo · 2024-02-20T16:44:37Z

src/common/libjob/job.h

+     * updates for those fields
+     * - currently works for jobspec and R
+     */
+    FLUX_JOB_LOOKUP_CURRENT = 2,


A question about this commit. The comment here indicates "works for jobspec and R" but I only see R handled specifically in lookup_current. Maybe a comment in lookup_current as to why jobspec is not handled there would be helpful.

ahh comment is probably a development history mistake, see #5635

grondo · 2024-02-20T16:50:44Z

src/cmd/flux-job.c

-                                 "flags", 0))
-            || flux_rpc_get_unpack (f, "{s:o}", key, &o) < 0)
+                                 "keys", key,
+                                 "flags", FLUX_JOB_LOOKUP_CURRENT))


Use of this flag requires a current version of flux-core. I'm not sure if it is worth it, but should this call fall back to update-lookup if the RPC returns EPROTO?

ugh, It ends up I didn't check "flags" in older versions b/c flags wasn't used. So I guess a caller would just get the original version.

Could just keep on calling job-info.update-lookup? But I'm not sure the pros of that outweigh the cons. I figure most users, if they are using flux job info, base R == updated R anyways?

I figure most users, if they are using flux job info, base R == updated R anyways?

Yes, there will be an updated R only if the expiration is adjusted, which is going to pretty rare. Also, the command would have to be run against a downrev parent instance to hit the failure, and at least this would be a failure I think instead of returning wrong information. So perhaps we just keep things as is and hope for the best? Over time it won't be a problem anymore... (However, something to keep in mind when adding future interfaces)

grondo · 2024-02-20T16:51:40Z

src/modules/job-info/lookup.c

                goto error;
-            s = current_value;
+            /* tiny optimization, to skip unnecessary json_dumps() and
+             * fallthrough to code below


I'd be fine dropping this optimization for now and only address performance if it becomes an issue.

grondo

Given the discussion about the flux job info change, this can probably go in as is when the fixup commit is dropped.

Problem: Some tabbing was invalid in t2233-job-info-update.t. Make tabbing consistent.

Problem: The test utility job-info/info_lookup includes a header that is unused. Remove the unused header.

Problem: Some extraneous extra spaces exist after some function parameters. Remove the extraneous extra spaces.

Problem: The internal variable "check_eventlog" is confusing because it only means to "lookup" the eventlog. Rename variable to lookup_eventlog.

Problem: Several calls to json_array_foreach are missing a space between it and the open parenthesis. This is likely a cut and paste error. Add the extra spaces.

Problem: The job-info.lookup target does not check for invalid flags. Add check for invalid flags.

Problem: Throughout lookup code there are checks if the values within the keys array are strings. It would be simpler if this check was only done once. Check that the keys array is an array and contains only strings within the primary lookup callback.

Problem: Several bad input cases are not tested agains the job-info.lookup RPC target. Add coverage in t2230-job-info-lookup.t.

Problem: On disconnect, cleaning up update-lookups was forgotten. Add cleanup update_lookups via a new update_lookup_cleanup() function.

Problem: It can be inconvenient to lookup a key and always have to convert it to a json object / python dictionary (such as a jobspec or R). Solution: Support a new FLUX_JOB_LOOKUP_JSON_DECODE flag that will treat some special keys (currently jobspec and R) as special lookups and will returned the values as decoded JSON objects instead of strings.

Problem: There is no coverage for the new job info lookup JSON_DECODE flag. Add coverage to t2230-job-info-lookup.t and add a --json-decode flag to the utility test tool job-info/info_lookup.

Problem: It is inconvenient that there is a job-info.lookup target and job-info.update-lookup target. Support the behavior of job-info.update-lookup in job-info.lookup via a new FLUX_JOB_LOOKUP_CURRENT flag.

Problem: Now that the job-info.lookup target takes the DECODE and CURRENT flags, it can behave just like the job-info.update-lookup target. Deprecate the job-info.update-lookup RPC target by simply having it call the job-info.lookup code path with the DECODE and CURRENT flags.

Problem: There is no coverage for the job-info.lookup RPC's new CURRENT flag. Add coverage in t2233-job-info-update.t.

Problem: The job-info.update-lookup RPC target has been deprecated. It would be wise to note this in some legacy tests. Add note in t/job-info/update_lookup.c.

Problem: The job-info.update-lookup target is being deprecated over the job-info.lookup target w/ the CURRENT flag. The "flux job info" command uses the job-info.update-lookup target. Update to use job-info.lookup over job-info.update-lookup. Fallback to job-info.update-lookup if necessary.

chu11 · 2024-02-20T23:38:09Z

just re-pushed. apologies, i began cleaning up more thinking that the split of flux-job.c had already been merged into master, whoops. I hope I didn't clean up too much and make the eventual rebasing of PR #5747 more difficult than it needs to be.

removed the fixup
tweaked that one comment per @grondo comment above
noticed some stupid typos ... ad606a8
given realization above that job-info.lookup doesn't check for valid flags, added that check in cbe957d (and added appropriate updates when new flags added)
realized needed some extra coverage a43f24f given issues above

this PR is getting long, could split it up?

grondo · 2024-02-20T23:48:56Z

Since splitting up flux-job.c just copy and pastes code it should be easy to fixup after all this is merged (just recopy the relevant code I hope)

If you want to split this into a separate PR that is fine with me. Thinking about each PR as a "release note entry" though, what would that be?

chu11 · 2024-02-20T23:50:36Z

If you want to split this into a separate PR that is fine with me. Thinking about each PR as a "release note entry" though, what would that be?

It'd just be a "cleanup" PR, so I guess nothing useful. Only offering to split up if it would make reviewing easier ... but I now see that you approved the PR pre-my recent changes. So I guess that doesn't matter as much now :-)

codecov · 2024-02-21T00:43:12Z

Codecov Report

Merging #5633 (cf89676) into master (925bfc1) will increase coverage by 0.00%.
The diff coverage is 75.74%.

Additional details and impacted files

@@           Coverage Diff            @@
##           master    #5633    +/-   ##
========================================
  Coverage   83.48%   83.49%            
========================================
  Files         488      488            
  Lines       83310    83421   +111     
========================================
+ Hits        69552    69652   +100     
- Misses      13758    13769    +11

Files	Coverage Δ
src/modules/job-info/job-info.c	`75.34% <100.00%> (+0.01%)`	⬆️
src/modules/job-info/util.c	`69.64% <87.50%> (+2.97%)`	⬆️
src/cmd/flux-job.c	`87.61% <60.00%> (+0.02%)`	⬆️
src/modules/job-info/update.c	`75.48% <73.07%> (-0.73%)`	⬇️
src/modules/job-info/lookup.c	`72.37% <75.89%> (+4.77%)`	⬆️

... and 11 files with indirect coverage changes

This was referenced Dec 20, 2023

job-info: support jobspec in update lookup/watch #5592

Closed

job-info: support lookup of updated jobspec, remove manual construction of updated R / jobspec #5635

Merged

chu11 force-pushed the issue5577_job_info_lookup_decode_update branch 3 times, most recently from 9b3e62a to 51c5ccd Compare December 20, 2023 06:04

chu11 mentioned this pull request Dec 20, 2023

rfc41: add new lookup flags flux-framework/rfc#407

Merged

garlick reviewed Dec 21, 2023

View reviewed changes

chu11 force-pushed the issue5577_job_info_lookup_decode_update branch from 51c5ccd to 8571c40 Compare December 22, 2023 16:47

chu11 changed the title ~~job-info: add DECODE & UPDATE flags, deprecate job-info.update-lookup~~ job-info: add JSON_DECODE & CURRENT flags, deprecate job-info.update-lookup Dec 22, 2023

chu11 force-pushed the issue5577_job_info_lookup_decode_update branch from 8571c40 to 7e77cc8 Compare December 22, 2023 16:49

chu11 force-pushed the issue5577_job_info_lookup_decode_update branch from 7e77cc8 to 7cd0142 Compare December 25, 2023 16:00

chu11 force-pushed the issue5577_job_info_lookup_decode_update branch from 7cd0142 to a421501 Compare February 1, 2024 01:34

grondo reviewed Feb 20, 2024

View reviewed changes

grondo approved these changes Feb 20, 2024

View reviewed changes

chu11 added 12 commits February 20, 2024 14:56

testsuite: fix tabbing

bb63402

Problem: Some tabbing was invalid in t2233-job-info-update.t. Make tabbing consistent.

testsuite: remove unused header

4ea7063

Problem: The test utility job-info/info_lookup includes a header that is unused. Remove the unused header.

flux-job: remove extraneous spaces

ad606a8

Problem: Some extraneous extra spaces exist after some function parameters. Remove the extraneous extra spaces.

job-info: rename internal variable

8403439

Problem: The internal variable "check_eventlog" is confusing because it only means to "lookup" the eventlog. Rename variable to lookup_eventlog.

job-info: fix rfc7 code style violation

ed93f73

Problem: Several calls to json_array_foreach are missing a space between it and the open parenthesis. This is likely a cut and paste error. Add the extra spaces.

job-info: check for invalid flags

cbe957d

Problem: The job-info.lookup target does not check for invalid flags. Add check for invalid flags.

testsuite: add coverage for job-info.lookup input

a43f24f

Problem: Several bad input cases are not tested agains the job-info.lookup RPC target. Add coverage in t2230-job-info-lookup.t.

job-info: cleanup update lookups on disconnect

b31d706

Problem: On disconnect, cleaning up update-lookups was forgotten. Add cleanup update_lookups via a new update_lookup_cleanup() function.

testsuite: cover job-info lookup JSON_DECODE flag

244febb

Problem: There is no coverage for the new job info lookup JSON_DECODE flag. Add coverage to t2230-job-info-lookup.t and add a --json-decode flag to the utility test tool job-info/info_lookup.

job-info: support job info CURRENT lookup flag

d229f2b

Problem: It is inconvenient that there is a job-info.lookup target and job-info.update-lookup target. Support the behavior of job-info.update-lookup in job-info.lookup via a new FLUX_JOB_LOOKUP_CURRENT flag.

chu11 added 4 commits February 20, 2024 14:56

testsuite: add job-info CURRENT flag coverage

ee903ce

Problem: There is no coverage for the job-info.lookup RPC's new CURRENT flag. Add coverage in t2233-job-info-update.t.

testsuite: note job-info.update-lookup is legacy

3a0c75b

Problem: The job-info.update-lookup RPC target has been deprecated. It would be wise to note this in some legacy tests. Add note in t/job-info/update_lookup.c.

chu11 force-pushed the issue5577_job_info_lookup_decode_update branch from a421501 to cf89676 Compare February 20, 2024 23:33

chu11 added the merge-when-passing label Feb 21, 2024

mergify bot merged commit 8190b78 into flux-framework:master Feb 21, 2024
34 of 35 checks passed

chu11 deleted the issue5577_job_info_lookup_decode_update branch February 21, 2024 00:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

job-info: add JSON_DECODE & CURRENT flags, deprecate job-info.update-lookup #5633

job-info: add JSON_DECODE & CURRENT flags, deprecate job-info.update-lookup #5633

chu11 commented Dec 20, 2023 •

edited

Loading

chu11 commented Dec 20, 2023

garlick commented Dec 21, 2023

garlick left a comment

garlick Dec 21, 2023

chu11 Dec 25, 2023 •

edited

Loading

garlick Dec 21, 2023

garlick Dec 21, 2023

garlick Dec 21, 2023

chu11 Dec 25, 2023

garlick Dec 21, 2023

garlick Dec 21, 2023

garlick Dec 21, 2023

chu11 commented Dec 22, 2023

chu11 commented Dec 25, 2023

grondo left a comment

grondo Feb 20, 2024

chu11 Feb 20, 2024

grondo Feb 20, 2024

chu11 Feb 20, 2024

grondo Feb 20, 2024

grondo Feb 20, 2024

grondo left a comment

chu11 commented Feb 20, 2024

grondo commented Feb 20, 2024

chu11 commented Feb 20, 2024

codecov bot commented Feb 21, 2024

		if (!(updated_str = json_dumps (update_object, 0)))
		goto error;

job-info: add JSON_DECODE & CURRENT flags, deprecate job-info.update-lookup #5633

job-info: add JSON_DECODE & CURRENT flags, deprecate job-info.update-lookup #5633

Conversation

chu11 commented Dec 20, 2023 • edited Loading

chu11 commented Dec 20, 2023

garlick commented Dec 21, 2023

garlick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chu11 Dec 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chu11 commented Dec 22, 2023

chu11 commented Dec 25, 2023

grondo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grondo left a comment

Choose a reason for hiding this comment

chu11 commented Feb 20, 2024

grondo commented Feb 20, 2024

chu11 commented Feb 20, 2024

codecov bot commented Feb 21, 2024

Codecov Report

chu11 commented Dec 20, 2023 •

edited

Loading

chu11 Dec 25, 2023 •

edited

Loading