job-list: do not assume alloc event context always contains annotations #4907
Conversation
chu11
force-pushed
the
issue4906_job_list_alloc_bypass
branch
2 times, most recently
from
e764a97
to
1d0386c
Compare
|
Sounds good to me! |
|
added a coverage test on top, instead of the dump file or jobtap plugin approach as mentioned in #4904, I just "hand" wrote an eventlog over an existing one. That way we can easily add extra events in the future (vs the dump file approach). |
| @@ -0,0 +1,25 @@ | |||
| #!/bin/bash -e | |||
There was a problem hiding this comment.
Since you have the follow on commit which adds a regression test to the job-list module, maybe this separate regression test isn't required? The reason I ask is that the t4000-issues test runs all the reproducers serially and at this point is really slow, so avoiding another flux start test therein would keep the testsuite a little leaner.
There was a problem hiding this comment.
actually the follow on test doesn't test it perfectly, as the regression that tests the event alloc {"bypass":1}, whereas the new test added alloc {"annotations":<something>, "etc": 1}.
But I could tweak it accordingly.
There was a problem hiding this comment.
Ah, that might be good just to save some unnecessary work for the issues test driver.
There was a problem hiding this comment.
ahhh this was a good idea, ended up there was a corner case with exception notes (which are optional).
chu11
force-pushed
the
issue4906_job_list_alloc_bypass
branch
from
2575eb7
to
38dc79f
Compare
|
re-pushed, removing the regression test and adding a new test in t2260-job-list.t instead. Decided to add another test to check for optional context fields. The only other one I found was userid/note in exceptions, and it ended up there was a corner case in job-list for that case, so fixed that too. |
Problem: When replaying the eventlog from the KVS, the job-list module assumes an alloc event context must have an annotations field. This is invalid, an alloc context may not contain an annoations field. It may contain other fields. Solution: Do not return EPROTO error if an alloc event context does not contain annotations. Fixes flux-framework#4906
Problem: Exception notes are optional, but this is not handled when retrieving an exception note for a job-list request. Solution: Ensure that an exception note is non-NULL when returning it to the user.
Problem: There are several eventlog parsing corner cases that are not covered. Solution: Add several eventlog parsing corner case tests. Specifically add: - missing annotations in alloc event context - missing note in exception event context - extraneous extra keys added to all event contexts
chu11
force-pushed
the
issue4906_job_list_alloc_bypass
branch
from
38dc79f
to
c498ad8
Compare
Codecov Report
@@ Coverage Diff @@
## master #4907 +/- ##
=======================================
Coverage 82.90% 82.90%
=======================================
Files 426 426
Lines 75251 75251
=======================================
Hits 62386 62386
Misses 12865 12865 |
Per discussion in #4906
I only added a regression test, no additional wider coverage tests yet per discussion in #4906.
Hopefully will have those wider extra coverage tests in soon. But just in case that extra wider coverage can't be completed in a timely manner, just wanted to make sure we atleast get this fix in before the next release, we can always add the extra tests later.