jobspec: enforce restrictions on resource range keys #1341
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Problem: in a resource range, if min is 1 when the operator is '^' then the code gets stuck in an infinite loop Add a check if exponent base is 1, and return current value if so (same behaviour as if exponent or multiplication operand is 1)
Problem: certain nonsensical combinations of range keys are currently not checked for when initializing ranges Add checks ensuring that specified min/max/operator/operand combinations are self-consistent
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1341 +/- ##
======================================
Coverage 75.7% 75.7%
======================================
Files 112 112
Lines 16410 16417 +7
======================================
+ Hits 12426 12436 +10
+ Misses 3984 3981 -3
|
sam-maloney
force-pushed
the
range-keys
branch
from
3a46225 to
a33ac43
Compare
|
force-pushed changes from clang-format; sorry I forgot to run that before the first round! |
Problem: recent change to RFC 14 explicitly makes max/operator/operand co-dependent; i.e., if one is given then all three must be, but this is not currently checked or enforced Add checks ensuring that if one of max/operator/operand is specified then all are, and adjusts the testsuite to match
sam-maloney
force-pushed
the
range-keys
branch
from
a33ac43 to
d47df4a
Compare
Problem: newly enforced constraints are not checked by the testsuite Add new invalid jobspecs to the testsuite to hit the new checks
Problem: because min/max in resource counts are read in as unsinged, negative values in the jobspec may still pass validation Change the value checks for min/max to use the input values cast to signed types to ensure negative inputs are caught
sam-maloney
force-pushed
the
range-keys
branch
from
d47df4a to
17a1c5d
Compare
|
Pushed some new tests for the new constraint checks to address the Codecov report (and simplified the checks slightly). Also noticed that negative values (particularly for max) could pass validation because of integer overflow after being read into the unsigned variables, so I tweaked the checks on min/max values to hopefully catch that as well. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem: restrictions on resource range keys described in flux-framework/rfc#444 are not currently checked/enforced
This PR implements checks in libjobspec to ensure the correct combinations of keys are present and the values are self-consistent.
The initial impetus for this was a range using the exponentiation operator but having a min value of 1 would get stuck in an infinite loop, so I've added a check for that in matcher.cpp which short-circuits similarly to when an exponent or multiplier of 1 is specified. However, with the new checks in jobspec.cpp, hopefully this situation should now never be reached in the first place, and rather fail with a useful error message.
I've separated the checks for consistent values from the checks on the presence of all or none of the max/operator/operand keys into separate commits, in case you'd rather keep the behaviour of using default values.