python: update Flux handle location in validator plugin API #6282
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: Validator plugins access a per-thread on-deman Flux handle via the ValidatorJobInfo class. But objects of this class are not available in the `configure` method, and also this means that a handle may be created and destroyed for each job being validated, instead of persisting for the lifetime of the plugin thread. Add an on-demand handle in th ValidatorPlugin base class instead. This will allow the plugin to access a Flux handle at any time, as well as persisting for the lifetime of the plugin. Finally, this handle doesn't have to be installed in thread-local storage, since each plugin is run in a separate thread as it is.
Problem: The feasibility plugin uses the ValidatorJobInfo provided Flux handle, but the ValidatorPlugin provided one is preferred. Use the ValidatorPlugin provided handle instead.
Problem: Validator plugins should now be using the flux handle provided by the ValidatorPlugin class instead of the one provided by ValidatorJobInfo (which requires a handle be opened once per job). Remove the `flux` attribute from ValidatorJobInfo.
grondo
force-pushed
the
validator-interface-update
branch
from
2cb2d62
to
698b49b
Compare
|
Thanks! Will set MWP. |
|
@grondo can you clarify what you mean by "out of tree validators"? Is that any validator that's not stored alongside the rpm or in the builddir for the running |
By out-of-tree I meant any validator plugins being maintained outside of the flux-core source tree. If they use the flux handle in |
|
Ah, ok, thanks! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6282 +/- ##
===========================================
+ Coverage 53.78% 83.34% +29.56%
===========================================
Files 473 522 +49
Lines 78410 85985 +7575
===========================================
+ Hits 42176 71668 +29492
+ Misses 36234 14317 -21917
|
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.
Currently, the Python validator plugin interface provides an on-demand Flux handle via the
ValidatorJobInfoclass. However, in retrospect, this was a big mistake. Since theValidatorJobInfoobject is created once per job, the handle is closed and must be reopened for each job passed to a validator plugin. Also, since the job info object is passed to multiple validators running in different threads, thread-local storage needs to be used, which complicates the implementation. Finally, theValidatorJobInfoobject is obviously not available when the plugin is configuring itself, which makes it impossible for plugins to contact Flux to configure themselves (or they have to open a handle themselves)This PR instead provides a new
fluxattribute in theValidatorPluginclass which opens and returns a Flux handle on first access. Because each plugin is run in its own thread, there is no longer a need to deal with TLS, simplifying the approach greatly. Finally, the handle is conveniently available any time the plugin is active, so theconfigurecallback can now use it.The handle provided by
ValidatorJobInfois then removed once the one internal user is updated. This could be a problem if there are out-of-tree validators out there (but I don't think that is likely at this point).