-
Notifications
You must be signed in to change notification settings - Fork 18
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
Change architecture of Workers pool #329
Merged
Merged
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
This depends on kubewarden/policy-evaluator#186 |
flavio
force-pushed
the
change-workers-architecture
branch
from
September 23, 2022 07:05
5db5eb9
to
d8fa433
Compare
viccuad
approved these changes
Oct 4, 2022
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.
LGTM! Nice PR, as the other!
Do not duplicate in memory the contents of a Wasm module, just to read its metadata. Signed-off-by: Flavio Castelli <[email protected]>
Small refactoring, consolidate methods and functions inside of the `settings` module. Signed-off-by: Flavio Castelli <[email protected]>
The latest Burrego releases breaks some APIs. Signed-off-by: Flavio Castelli <[email protected]>
The same policy can be instantiated multiple times. Prior to this commit, the Downloader would download and verify the same WebAssembly module over and over again. Now the Download avoids doing that. Moreover, prior to this commit, the code mutated the list of policies provided as a parameter. That was done to add the local path to the WebAssembly module. The new code avoids doing this mutation, which was a bit ugly. Signed-off-by: Flavio Castelli <[email protected]>
Signed-off-by: Flavio Castelli <[email protected]>
Signed-off-by: Flavio Castelli <[email protected]>
Ensure the worker_pool receives the list of fetched policies. This is needed because now the `settings::Policy` structs do no longer have an attribute that holds this information. See previous commit. Signed-off-by: Flavio Castelli <[email protected]>
This is a big refactoring of the WorkerPool and the Worker structs. The major changes done to WorkerPool: * Perform the validation of policies settings only once per policy. Prior to that, each worker validated the settings of all the policies. * Precompile all the WebAssembly modules once, then give the precompiled modules to the workers. This reduces the bootstrap time **a lot**. About the latter point, creating a `wasmtime::Module` by reading the contents of the `.wasm` file is an expensive operation. Prior to that, the same WebAssembly module was compiled by each worker. Note, we cannot rely on Wasmtime `cache` feature because, given the number of workers we start, there would be quite some resource contention among them. Changes done to the Worker: * Create one single `wasmtime::Engine` to be shared among all the Modules hosted by the Worker. This reduces resource usage and is needed to implement the "limit execution time of the policies" feature. * Instantiate the `wasmtime::Module` instances by using the precompiled objects prepared by the WorkerPool. This saves an insane amount of time at bootstrap time * Do not perform settings validation anymore. They have already been validated prior to the creation of the Worker Signed-off-by: Flavio Castelli <[email protected]>
Use latest version of policy_evaluator, plus require the rayon crate which is now being used by the worker pool. Signed-off-by: Flavio Castelli <[email protected]>
flavio
force-pushed
the
change-workers-architecture
branch
from
October 17, 2022 08:58
d8fa433
to
8bea4bd
Compare
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.
This is a big refactoring of the WorkerPool and the Worker structs.
The major changes done to WorkerPool:
About the latter point, creating a
wasmtime::Module
by reading the contents of the.wasm
file is an expensive operation. Prior to that, the same WebAssembly module was compiled by each worker.Note, we cannot rely on Wasmtime
cache
feature because, given the number of workers we start, there would be quite some resource contention among them.Changes done to the Worker:
wasmtime::Engine
to be shared among all the Modules hosted by the Worker. This reduces resource usage and is needed to implement the "limit execution time of the policies"feature.
wasmtime::Module
instances by using the precompiled objects prepared by the WorkerPool. This saves an insane amount of time at bootstrap timeFinally, this is required to allow us to implement #254