job-info: stream events even if job is inactive

[chu11]

Problem: If a job is inactive, all data in an eventlog will be returned
as a single response during an eventlog watch. This is because we know for
a fact that the data should never change after the job is inactive.

If this data, such as job standard output, is very large, this lookup
can be very slow. In some cases, the use of something like flux job attach
can have the apperance of a hang because the standard output response is
taking so long to lookup and return.

Solution:

When a job is inactive and the user wants to watch a job eventlog,
do not respond with all of the data in a single response. Instead stream
the response back just as if the job were active.

Utilize the FLUX_KVS_WATCH_APPEND_ONCE to ensure the stream ends once all
data in the KVS is streamed. Update all variables, functions, etc. from
"lookup" to "watch".

Fixes #6516

---

I probably need to add some new tests but wanted to throw this up here for now.

Side note on the "WATCH_APPEND_ONCE" flag, I could make this generic and apply it to all watch types (i.e. even non-appends) but didn't feel it was needed elsewhere so didn't do that.

[garlick]

Naive question: could the client just do a FLUX_KVS_WATCH_APPEND instead of the lookup and then just cancel it when the clean event is read?

[chu11]

oh i forgot, doing

> flux run flux lptest 2000000 80
<snip all that output>
> time flux job attach `flux job last`

on master the flux job attach takes about 17 seconds (seen as slow as 20 seconds). On this branch it is now down to about 13 seconds on average on corona (seen as fast as 10 seconds). And the noticeable "pause" at the beginning is no longer there.

[chu11]

Naive question: could the client just do a FLUX_KVS_WATCH_APPEND instead of the lookup and then just cancel it when the clean event is read?

Perhaps my description of what I'm trying to solve isn't clear. We already know for a fact that the 'clean' event has arleady happened. The job is already done / inactive.

So the issue is if you do a FLUX_KVS_WATCH_APPEND instead of a lookup on a job that is inactive/done, how do you know when to issue the cancel? You don't really know.

So that's why I wanted to put the "WATCH_APPEND_ONCE" flag in on the kvs-watch side to finish the stream when it knows it is done.

[garlick]

Oh duh, sorry, I was thinking about watching the primary eventlog which always ends in clean but this is about the output eventlog, which doesn't have a definitive end of file.

Since the user is just passing this to flux_kvs_lookup(), maybe the flag name and description should be divorced from the concept of "watch" (even though internally it uses the watch service), e.g.

FLUX_KVS_STREAM
    Return a potentially large value in multiple responses terminated by an ENODATA error response.

[chu11]

Since the user is just passing this to flux_kvs_lookup(), maybe the flag name and description should be divorced from the concept of "watch" (even though internally it uses the watch service), e.g.

Ahhh, I like this idea. Lemme see how to go about this.

[chu11]

still need to add tests but went with the proposed FLUX_KVS_STREAM (still need tests & documentation)

[garlick]

Nice! Just a thought, but FLUX_KVS_STREAM is a pretty big feature on its own so maybe it should be proposed in a standalone PR with its own docs and tests? Then this one could make the job-info changes?

[chu11]

just re-pushed, splitting off the FLUX_KVS_STREAM option into #6523. Removing WIP.

Edit: actually, I'll re-put WIP, just so #6523 is reviewed first.

[chu11]

rebased and re-pushed now that #6523 is in, just tweaked some commit messages.

[codecov]

Codecov Report

Attention: Patch coverage is 94.73684% with 2 lines in your changes missing coverage. Please review.

Project coverage is 83.62%. Comparing base (e10d676) to head (0118856).

Files with missing lines	Patch %	Lines
src/modules/job-info/guest_watch.c	93.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6518      +/-   ##
==========================================
- Coverage   83.63%   83.62%   -0.02%     
==========================================
  Files         522      522              
  Lines       87805    87809       +4     
==========================================
- Hits        73434    73427       -7     
- Misses      14371    14382      +11     

Files with missing lines	Coverage Δ
src/modules/job-info/watch.c	66.39% <100.00%> (+0.69%)	⬆️
src/modules/job-info/guest_watch.c	74.93% <93.33%> (+0.20%)	⬆️

... and 7 files with indirect coverage changes

[garlick]

Wondering about this name change - once the guest info is copied to the primary namespace, it is static, but "watch" implies that it is changing.

[chu11]

yeah, i did go back and forth on it. I went with watch b/c I go through job-info.eventlog-watch instead of calling kvs.lookup (similarly guest_namespace_watch also goes through job-info.eventlog-watch).

I did consider going with stream instead, but that sort of describes internal knowledge of what job-info.eventlog-watch is doing.