flux-job: get updated version of jobspec #5428

chu11 · 2023-09-06T22:28:03Z

The jobspec returned by flux job info is the one stored in the KVS, which may differ from what is internally "viewed" by flux components due to jobspec-update events. Notably, the jobspec returned may not be consistent to what is returned via flux jobs.

This PR will make flux job info return an updated jobspec by default.

Added a --noupdate option to get the version stored in the KVS. I'm not super in love with this option name. I considered --kvs, but that was confusing give Python binding function names like ("job_kvs_lookup()"). --stored seemed dumb too. Ehhh ... any better ideas? I didn't want the option to be --update b/c I wanted the default to be the updated one.

Also added support in the Python bindings.

Update flux-update, as it no longer needs to apply jobspec-update events manually.

src/bindings/python/flux/job/kvslookup.py

grondo · 2023-09-06T23:41:53Z

Ehhh ... any better ideas? I didn't want the option to be --update b/c I wanted the default to be the updated one.

What about --base?

chu11 · 2023-09-07T00:12:07Z

What about --base?

That's not bad ... illustrates the idea well. Lets go with it.

chu11 · 2023-09-07T04:33:15Z

repushed using the term "base" over "noupdate"

garlick · 2023-09-08T14:12:39Z

Silly question but we have flux job info --original ID jobspec which unwraps the jobspec in J. Do we need --base also?

Edit: the --original version includes the environment but not frobnicator changes...

grondo · 2023-09-08T14:37:36Z

Do we need --base also?

It seems useful at least for testing if nothing else?

chu11 · 2023-09-08T15:10:17Z

Do we need --base also?

I did ponder this. I added --base with the consideration:

the output of --original and --base is different, one is the "original redacted" jobspec while the other is the "original original" retrieved via J.
--base can also be used in the future with flux job info <JOBID> R, which get the updated R once resource-update is supported. I'm not sure --original makes sense for R (given how it is used for J since there is no equivalent for R).

Not to say that these are the strongest reasons, just some reasons :-)

garlick · 2023-09-08T15:12:15Z

OK I'm fine with that. Just wanted to hash that out.

grondo

LGTM. A couple nits inline.

grondo · 2023-09-08T22:17:25Z

src/cmd/flux-job.c

+         * event user specified duplicate keys or options that lead to
+         * duplicate keys: e.g.
+         * - --original and both "jobspec" and "J"
+         * - --base and both "jobspec" and "eventlog"


From the code it appears that "jobspec" and "eventlog" are both fetched only if not --base not with --base.
Is this comment correct?

grondo · 2023-09-08T22:21:01Z

src/bindings/python/flux/job/kvslookup.py

@@ -53,16 +53,11 @@ def job_info_lookup(flux_handle, jobid, keys=["jobspec"]):
    return rpc


-def _original_setup(keys, original):


commit message: is "inputted" a word?

https://english.stackexchange.com/questions/30684/inputted-or-input

LOL

https://www.merriam-webster.com/grammar/is-inputted-a-word

the issue of how to treat the past participle is a contentious one, with much blood being shed on both sides.

But it seems folks have accepted it as a word

🤷

👍 However from now on I will think of it as a golf term.

Problem: Internally within flux, the jobspec can be updated through jobspec-update events. When the user runs "flux job info" to get the jobspec, it is getting the jobspec stored in the KVS and it may not represent what is actually used. In addition, the jobspec returned from "flux job info" will be inconsistent to the information returned from "flux jobs". Solution: When retrieving the jobspec, also retrieve the job eventlog. Internally update the jobspec through "jobspec-update" events and output the final result to give the user a representation of what the jobspec looks like within flux. Add a --base option to perform the old behavior, which is to just get the jobspec stored in the KVS. Fixes flux-framework#5411

Problem: There is no coverage for the new output style of "flux job info" and the jobspec (output represents the internal view of the jobspec) and there is no coverage for the flux job info --base option. Add coverage in t2230-job-info-lookup.t.

Problem: The job kvslookup code uses a number of flags to manage special case "jobspec" lookups. While this is fine when there is one single lookup special case, future special cases make the addition of more flags very cumbersome. Refactor the logic to remove flags. Instead, maintain a copy of the inputted lookup keys and a copy of the keys used for the actual lookup. Use this information instead of the multiple created flags to determine special case handling.

Problem: The "flux job info" command supports a "base" option, but this is not supported in tne similar Python bindings. Support a "base" flag in the job_kvs_lookup() function and the JobKVSLookup class.

Problem: The tests in t0014-job-kvslookup.py are numbered, making it annoying to add new tests and renumber all of the tests. To avoid excessive re-numbering of tests when new tests are added, logically group similar tests together and numerically number tests only within a group.

Problem: There is no coverage for the new "base" option for job kvs lookups via Python. Add coverage in python/t0014-job-kvslookup.py.

Problem: flux-update manually updates a jobspec with jobspec-update events from the eventlog. However, this is no longer necessary as the "job_kvs_lookup()" function does it by default. Remove manual updates to jobspec from flux-update.

chu11 · 2023-09-08T23:15:02Z

@grondo Thanks, good catch on the comment. Fixed that up, will set MWP.

codecov · 2023-09-08T23:40:26Z

Codecov Report

Merging #5428 (304ad80) into master (eccb4d2) will increase coverage by 0.02%.
The diff coverage is 93.80%.

@@            Coverage Diff             @@
##           master    #5428      +/-   ##
==========================================
+ Coverage   83.61%   83.64%   +0.02%     
==========================================
  Files         480      480              
  Lines       80678    80744      +66     
==========================================
+ Hits        67460    67539      +79     
+ Misses      13218    13205      -13

Files Changed	Coverage Δ
src/cmd/flux-update.py	`97.80% <ø> (-0.16%)`	⬇️
src/cmd/flux-job.c	`87.52% <89.39%> (-0.03%)`	⬇️
src/bindings/python/flux/job/kvslookup.py	`97.76% <100.00%> (+0.41%)`	⬆️

... and 7 files with indirect coverage changes

github-advanced-security bot found potential problems Sep 6, 2023

View reviewed changes

src/bindings/python/flux/job/kvslookup.py Fixed Show fixed Hide fixed

chu11 force-pushed the issue5411_flux_job_info_redacted_jobspec branch from 80f6c7f to 511ead8 Compare September 7, 2023 04:32

chu11 force-pushed the issue5411_flux_job_info_redacted_jobspec branch from 511ead8 to d88af86 Compare September 7, 2023 15:41

chu11 force-pushed the issue5411_flux_job_info_redacted_jobspec branch from d88af86 to 6e58d56 Compare September 8, 2023 18:30

grondo approved these changes Sep 8, 2023

View reviewed changes

chu11 force-pushed the issue5411_flux_job_info_redacted_jobspec branch from 6e58d56 to 6b5ed20 Compare September 8, 2023 23:01

chu11 added 7 commits September 8, 2023 16:02

testsuite: cover flux job info jobspec

6a32d0a

Problem: There is no coverage for the new output style of "flux job info" and the jobspec (output represents the internal view of the jobspec) and there is no coverage for the flux job info --base option. Add coverage in t2230-job-info-lookup.t.

python: support base flag in job kvslookup

53c16cf

Problem: The "flux job info" command supports a "base" option, but this is not supported in tne similar Python bindings. Support a "base" flag in the job_kvs_lookup() function and the JobKVSLookup class.

testsuite: add job kvs lookup 'base' coverage

ab4e96e

Problem: There is no coverage for the new "base" option for job kvs lookups via Python. Add coverage in python/t0014-job-kvslookup.py.

flux-update: do not manually update jobspec

304ad80

Problem: flux-update manually updates a jobspec with jobspec-update events from the eventlog. However, this is no longer necessary as the "job_kvs_lookup()" function does it by default. Remove manual updates to jobspec from flux-update.

chu11 force-pushed the issue5411_flux_job_info_redacted_jobspec branch from 6b5ed20 to 304ad80 Compare September 8, 2023 23:02

chu11 added the merge-when-passing label Sep 8, 2023

mergify bot merged commit 9d92166 into flux-framework:master Sep 8, 2023
30 checks passed

chu11 deleted the issue5411_flux_job_info_redacted_jobspec branch September 8, 2023 23:52

chu11 mentioned this pull request Sep 13, 2023

Idea: job-info: add streaming RPC to "watch" R #5451

Closed

chu11 mentioned this pull request Sep 22, 2023

flux-job: get updated version of R #5464

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

flux-job: get updated version of jobspec #5428

flux-job: get updated version of jobspec #5428

chu11 commented Sep 6, 2023

grondo commented Sep 6, 2023

chu11 commented Sep 7, 2023

chu11 commented Sep 7, 2023

garlick commented Sep 8, 2023 •

edited

Loading

grondo commented Sep 8, 2023

chu11 commented Sep 8, 2023

garlick commented Sep 8, 2023

grondo left a comment

grondo Sep 8, 2023

grondo Sep 8, 2023

garlick Sep 8, 2023

chu11 Sep 8, 2023 •

edited

Loading

grondo Sep 9, 2023

chu11 commented Sep 8, 2023

codecov bot commented Sep 8, 2023

		@@ -53,16 +53,11 @@ def job_info_lookup(flux_handle, jobid, keys=["jobspec"]):
		return rpc


		def _original_setup(keys, original):

flux-job: get updated version of jobspec #5428

flux-job: get updated version of jobspec #5428

Conversation

chu11 commented Sep 6, 2023

grondo commented Sep 6, 2023

chu11 commented Sep 7, 2023

chu11 commented Sep 7, 2023

garlick commented Sep 8, 2023 • edited Loading

grondo commented Sep 8, 2023

chu11 commented Sep 8, 2023

garlick commented Sep 8, 2023

grondo left a comment

Choose a reason for hiding this comment

grondo Sep 8, 2023

Choose a reason for hiding this comment

grondo Sep 8, 2023

Choose a reason for hiding this comment

garlick Sep 8, 2023

Choose a reason for hiding this comment

chu11 Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

grondo Sep 9, 2023

Choose a reason for hiding this comment

chu11 commented Sep 8, 2023

codecov bot commented Sep 8, 2023

Codecov Report

garlick commented Sep 8, 2023 •

edited

Loading

chu11 Sep 8, 2023 •

edited

Loading