Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

flux-job: Add context test in wait-event #2137

Merged
merged 3 commits into from Apr 24, 2019

Conversation

@chu11
Copy link
Contributor

commented Apr 19, 2019

As discussed in #2083, it would be useful for flux job wait-event to wait not just for an event but for some condition in the context. As an initial step, add the ability for the user to specify an optional key=val to check in the context too.

Initial feature supports only one key=val input, it could be > 1, but didn't see a need for > 1 at the moment.

On the way, reverted a behavior change from #2085 where strings output from contexts were quoted (i.e. data="foo" instead of data=foo). Users would have to be mindful of quoting strings in contexts, which I think is confusing. So I decided to remove those quotations everywhere.

@garlick

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

It might be a good idea to make this a real option rather than a positional one so alternative match schemes could be added later without breaking tests.

Could we avoid changing the output representation if we added an (optional?) type specifier to the match argument, like a :s suffix to indicate a string or :i to indicate an integer? Then you could say

--match-context=severity:i=0

or

--match-context=type:s=cancel
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

It might be a good idea to make this a real option rather than a positional one so alternative match schemes could be added later without breaking tests.

Good idea!

Could we avoid changing the output representation if we added an (optional?) type specifier to the match argument, like a :s suffix to indicate a string or :i to indicate an integer?

Now that I think about it, I could just check with and without quotes all of the time. That's probably easiest overall.

--match-context=severity:i=0

Now that you mention it, how about just --match-context=key:val, so it's JSON-esque.

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

Now that you mention it, how about just --match-context=key:val, so it's JSON-esque.

Oh wait, I forgot that the defualt format is still key=val output. So the equal sign is probably a good idea. But I think it's probably best I just check quote/non-quote for the user.

@garlick

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

Now that I think about it, I could just check with and without quotes all of the time. That's probably easiest overall.

Do you mean quotes on the command line force the type to string, otherwise try other scalar types? That's going to be a bit ugly in sharness tests where the script is already running in one level of quotes...

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

Do you mean quotes on the command line force the type to string, otherwise try other scalar types? That's going to be a bit ugly in sharness tests where the script is already running in one level of quotes...

Oh I meant within flux-job. Basically, flux-job should check both foo=bar and foo="bar".

@garlick

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

Are you talking about interpreting double quotes on flux-job's command line as a "this is a string"? Sorry I'm still confused.

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

Sorry. Right now in flux-job, when a context is output, it outputs with strings quoted. For example:

1556044555.138259 release ranks="all" final=true

This is because of how json_dumps() formats things for output.

So the question was what should input to --match-context be. For consistency, should quotes go around strings or not? i.e. would --match-context="type=cancel" fail, but --match-context='type="cancel"' work?

I felt that quotes around strings thing was too confusing. So that's why I initially decided to change context output to be all key=value without quotes (which matched output before contexts were JSON objects). So now it'd be a bit more clear to what input can go into --match-context. We just do key=value without quotes all the time.

If we don't want to change the output of flux-job, there's still the question of what input is acceptable from the user. Do we want to require strings be quoted?

What I was suggesting above is that it wouldn't matter if the user input --match-context="type=cancel" or --match-context='type="cancel"', they would both work. Internally, I would manage the checking of both conditions regardless of what the user input.

@garlick

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

I think principle of least surprise argues for no interpretation of quotes in arguments inside flux-job.

That being the case, there's still some ambiguity if you ask for --match-context=foo=1.2 or --match-context=bar=false whether those values are strings or not. That's what was motivating my suggestion for a type specifier.

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

That being the case, there's still some ambiguity if you ask for --match-context=foo=1.2 or --match-context=bar=false whether those values are strings or not. That's what was motivating my suggestion for a type specifier.

Do you think a type check is important? Code wise, I was initially comparing to whatever comes out of json_dumps(), so a type wasn't necessary. But with a type specifier, we would want to check that json_is_boolean(), json_is_integer(), etc.

@garlick

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

The way you are doing it, it's probably not required (only problem I can imagine is one where numbers that are equal might not be represented identically). Sorry for the noise!

@chu11 chu11 force-pushed the chu11:issue2083 branch from 44a2619 to 8fa770c Apr 23, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

re-pushed. Instead of a positional option, it's now a --match-context option. Output of flux job eventlog is no longer changed now.

Users are required to input key=value identical to what's currently output from flux job eventlog, so that means strings have to have quotes around them. So --match-context="type=\"cancel\"" will work, but --match-context="type=cancel" will not work.

Also, I stuck a random extra unit test in for libeventlog, that I suddenly realized I forgot from a prior PR.

@codecov-io

This comment has been minimized.

Copy link

commented Apr 24, 2019

Codecov Report

Merging #2137 into master will increase coverage by 0.02%.
The diff coverage is 96.87%.

@@            Coverage Diff             @@
##           master    #2137      +/-   ##
==========================================
+ Coverage    80.4%   80.42%   +0.02%     
==========================================
  Files         200      200              
  Lines       31701    31729      +28     
==========================================
+ Hits        25489    25519      +30     
+ Misses       6212     6210       -2
Impacted Files Coverage Δ
src/cmd/flux-job.c 86.23% <96.87%> (+0.94%) ⬆️
src/modules/job-ingest/worker.c 65.03% <0%> (-7.7%) ⬇️
src/common/libflux/message.c 81.39% <0%> (-0.13%) ⬇️
src/modules/connector-local/local.c 74.66% <0%> (+1.03%) ⬆️
src/common/libeventlog/eventlog.c 87.94% <0%> (+1.41%) ⬆️
src/broker/modservice.c 80.76% <0%> (+1.92%) ⬆️
src/modules/barrier/barrier.c 80.53% <0%> (+2.01%) ⬆️
@garlick

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

Users are required to input key=value identical to what's currently output from flux job eventlog, so that means strings have to have quotes around them. So --match-context="type="cancel"" will work, but --match-context="type=cancel" will not work.

So...I was hoping you were going to go the other way on that one. What purpose do the extra quotes serve? It seems like internally you don't need them to match just about anything?

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

So...I was hoping you were going to go the other way on that one. What purpose do the extra quotes serve? It seems like internally you don't need them to match just about anything?

Oh, I misread your prior comment about "I think principle of least surprise argues for no interpretation of quotes in arguments inside flux-job.".

chu11 added 2 commits Apr 18, 2019
Add --match-context option to wait-event, allowing user to check
for an arbitrary key=value in the context of an event.

Add new tests.

Fixes #2083
@garlick

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

Apologies for not making my point with clearer language!

@chu11 chu11 force-pushed the chu11:issue2083 branch from 8fa770c to 51e3d58 Apr 24, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

re-pushed, Now both --match-context="type=\"cancel\"" and --match-context=type=cancel will work.

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

hit a valgrind error in one run. I saw similar fail in #2128. No stack information or anything. Restarting builder.

==391== HEAP SUMMARY:
==391==     in use at exit: 27,632 bytes in 70 blocks
==391==   total heap usage: 25,930 allocs, 25,860 frees, 17,220,788 bytes allocated
==391== 
==391== LEAK SUMMARY:
==391==    definitely lost: 0 bytes in 0 blocks
==391==    indirectly lost: 0 bytes in 0 blocks
==391==      possibly lost: 0 bytes in 0 blocks
==391==    still reachable: 27,632 bytes in 70 blocks
==391==         suppressed: 0 bytes in 0 blocks
==391== Reachable blocks (those to which a pointer was found) are not shown.
==391== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==391== 
==391== For counts of detected and suppressed errors, rerun with: -v
==391== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
flux-start: 0 (pid 390) Killed
not ok 1 - valgrind reports no new errors on 2 broker run
@garlick

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

LGTM - thanks!

@garlick garlick merged commit a6852e3 into flux-framework:master Apr 24, 2019
2 checks passed
2 checks passed
Mergify — Summary 1 potential rule
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.