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

Plugin initialization and aggregation #281

Merged
merged 1 commit into from
Aug 20, 2024

Conversation

j-lanson
Copy link
Collaborator

@j-lanson j-lanson commented Aug 15, 2024

Note: This PR relies on #278 which must be merged first.

This PR makes small adjustments to and expands upon #278 . The PluginExecutor now has a start_plugins() async function which allows a set of plugins to be started concurrently. For a simple implementation, this required removing mutable state from the PluginExecutor, hence the elimination of the est_ports field.

The HcPluginCore struct was added, which is intended to manage the protocol state of connections to each plugin and handle requesting info from a different plugin on behalf of another. The HcPluginCore::new() function takes an executor, and a set of (Plugin, Configuration) pairs. This function starts all the plugins and also initializes them over gRPC by getting the schemas, setting the config, and getting the default policy expression. In order to hold important state and distinguish between an initialized and uninitialized PluginContext, the PluginTransport struct was created. It contains the context as well as handles for sending/receiving Query messages with the plugin on the other side. The goal is for the chunking portion of the protocol to be hidden within this struct

@j-lanson j-lanson self-assigned this Aug 15, 2024
@j-lanson j-lanson added the type: enhancement New feature or request label Aug 15, 2024
@j-lanson j-lanson added this to the 3.6.0 milestone Aug 15, 2024
@j-lanson j-lanson force-pushed the jlanson/plugin-context-manager branch 2 times, most recently from 7c6305c to 9a16367 Compare August 15, 2024 18:15
@j-lanson j-lanson changed the title DRAFT: Plugin initialization and aggregation Plugin initialization and aggregation Aug 15, 2024
@j-lanson j-lanson force-pushed the jlanson/plugin-context-manager branch from 850be8f to a4d67d7 Compare August 15, 2024 21:23
Copy link
Collaborator

@alilleybrinker alilleybrinker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may in the future want to consider Tokio's JoinSet instead of join_all here. The reasons are subtle and will require careful consideration. First, I recommend reading the following:

We can go forward with the current design for now, but I want us to be thinking about this.

@j-lanson
Copy link
Collaborator Author

We may in the future want to consider Tokio's JoinSet instead of join_all here. The reasons are subtle and will require careful consideration. First, I recommend reading the following:

We can go forward with the current design for now, but I want us to be thinking about this.

I had looked at tokio::JoinSet without knowing the subtlety between the two, and I didn't like that it would force the PluginExecutor to be 'static when its behavior is anticipated to be controlled by the Hipcheck config file. The alternative is a global OnceLock type situation, but it's not one I relish.

@alilleybrinker
Copy link
Collaborator

We may in the future want to consider Tokio's JoinSet instead of join_all here. The reasons are subtle and will require careful consideration. First, I recommend reading the following:

We can go forward with the current design for now, but I want us to be thinking about this.

I had looked at tokio::JoinSet without knowing the subtlety between the two, and I didn't like that it would force the PluginExecutor to be 'static when its behavior is anticipated to be controlled by the Hipcheck config file. The alternative is a global OnceLock type situation, but it's not one I relish.

Oh that's not what the 'static bound means. Any type which doesn't hold references is 'static, so what this means in practice is that if tasks want to hold pointers to state from elsewhere that access needs to be mediated by Arc and some sort of exclusion like a Mutex.

@j-lanson
Copy link
Collaborator Author

We may in the future want to consider Tokio's JoinSet instead of join_all here. The reasons are subtle and will require careful consideration. First, I recommend reading the following:

We can go forward with the current design for now, but I want us to be thinking about this.

I had looked at tokio::JoinSet without knowing the subtlety between the two, and I didn't like that it would force the PluginExecutor to be 'static when its behavior is anticipated to be controlled by the Hipcheck config file. The alternative is a global OnceLock type situation, but it's not one I relish.

Oh that's not what the 'static bound means. Any type which doesn't hold references is 'static, so what this means in practice is that if tasks want to hold pointers to state from elsewhere that access needs to be mediated by Arc and some sort of exclusion like a Mutex.

Alternatively, the PluginExecutor does not currently contain mutable state, and its member fields are all simple primitive types (minus one Range object). So cloning the PluginExecutor and moving the clone into the Future could be another strategy. That seems like it would be more performant than each Future having to wait its turn just to get access to different config fields.

@alilleybrinker
Copy link
Collaborator

Ah, since it's configuration and/or policy data that is never modified, a OnceLock or LazyLock wrapped in an Arc would be good. Reducing the duplication seems more important than removing a small amount of pointer indirection and increment / decrement for sharing (which only happens at the beginning). OnceLock / LazyLock encode the "initialize once and then never modify again" pattern and mean that each task gets to basically act like it has a normal reference without infecting the code with lifetimes that violates the JoinSet rules.

@j-lanson j-lanson force-pushed the jlanson/plugin-context-manager branch from 1e88fe8 to cc22b76 Compare August 19, 2024 19:37
@alilleybrinker
Copy link
Collaborator

LGTM! Just needs a rebase to squash to one commit. Ping me after that, I'll re-review, and we can merge!

@j-lanson j-lanson force-pushed the jlanson/plugin-context-manager branch from 9544158 to 5b67e99 Compare August 20, 2024 14:19
@alilleybrinker alilleybrinker merged commit 7be577e into main Aug 20, 2024
9 checks passed
@j-lanson j-lanson deleted the jlanson/plugin-context-manager branch August 26, 2024 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants