-
Notifications
You must be signed in to change notification settings - Fork 1k
Fixes #38986 - Support runtime evaluation of Procs in plugin ForemanContext metadata #10814
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
base: develop
Are you sure you want to change the base?
Fixes #38986 - Support runtime evaluation of Procs in plugin ForemanContext metadata #10814
Conversation
Allow plugins to register metadata as lambdas that are evaluated at runtime instead of at registration time. This enables dynamic state changes without requiring a server restart. Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
On each request, it will also traverse all the metadata key-value pairs recursively checking if they're callable or not. That sounds like a relatively expensive operation to do over and over again. Have you tried measuring how long a single evaluation takes with a full plugin set? |
@adamruzicka If I apply Ewoud's suggestion, will this satisfy your concern? |
|
Yes, it will |
Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
|
Okay, updated. I had to keep one level of nesting, but it's better than recursion. otherwise, it wouldn't work because the keys of |
Convert if-elsif to case-when for better code style. Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
0f0c332 to
38a31af
Compare
|
@ekohl updated. This seems to work for my IoP rh_cloud use case. |
| core_app_metadata.merge( | ||
| ::Foreman::Plugin.app_metadata_registry.all_plugin_metadata | ||
| ) | ||
| ::Foreman::Plugin::AppMetadataRegistry.resolve_procs(core_app_metadata) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Core doesn't have procs in app metadata. Do we need to support this or can we add it when we need it? I'd lean to that just for the performance benefit, even if it's probably tiny.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just don't like the idea of treating core and plugins differently, as a principle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also a strong argument. @adamruzicka (or someone else): mind being the tie breaker?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Jeremy, based on the same reasoning.
I can't really think of an example right now, but as Foreman transforms more and more into an SPA, the context will only become more important in the future. I think it does make sense to have the same behaviour for plugin and core metadata because of that.
I tried testing the performance impact with Benchmark and got these results:
original code:
user system total real
Metadata 0.001473 0.000000 0.001473 ( 0.001501)
user system total real
Metadata 0.000424 0.001173 0.001597 ( 0.001621)
user system total real
Metadata 0.000000 0.001692 0.001692 ( 0.001545)
user system total real
Metadata 0.001469 0.000000 0.001469 ( 0.001429)
user system total real
Metadata 0.001409 0.000188 0.001597 ( 0.001584)
resolve_procs called on core_app_metadata:
user system total real
Metadata 0.001375 0.000189 0.001564 ( 0.001366)
user system total real
Metadata 0.000000 0.001520 0.001520 ( 0.001496)
user system total real
Metadata 0.001566 0.000000 0.001566 ( 0.001375)
user system total real
Metadata 0.000482 0.001278 0.001760 ( 0.001770)
user system total real
Metadata 0.002283 0.000000 0.002283 ( 0.002234)
Given the variance, these values are pretty useless in absolute terms, but they do tell me that the performance impact is basically non-existent. On a pretty under-spec'd VM no less...
Summary
Plugins can now register ForemanContext metadata as lambdas that are evaluated at runtime instead of at registration time. This enables plugin metadata to reflect dynamic state changes without requiring a server restart.
Enables theforeman/foreman_rh_cloud#1138
User Impact
When plugins register metadata as Procs, the values will now be evaluated fresh on each request. This fixes issues where plugin metadata could become stale when system state changes (e.g., when a smart proxy is created or destroyed).
Technical Details
evaluate_metadata_hashhelper that evaluates Proc values in metadata hashesImplementation Notes
This PR addresses PR feedback from @ekohl and @adamruzicka:
Testing
For manual testing steps, see theforeman/foreman_rh_cloud#1138
Added 4 comprehensive tests covering: