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

Remove scanner pool in favor of single-use scanners #765

Merged
merged 2 commits into from
Jan 17, 2025

Conversation

egibs
Copy link
Member

@egibs egibs commented Jan 17, 2025

The scanner pool concept was neat in theory, but yara-x leverages WASM which exhibits unpredictable behavior when a new scanner is created. Notably, each new scanner is allocated 8GB of virtual memory for the time being and this value is not configurable. Controlling this behavior with a separate value then adds another layer of complexity.

This PR reverts to the original pattern we used for go-yara with the main downside being that we are unable to scan file descriptors with yara-x and must instead read the entire file into memory before scanning it. That said, we'll be able to directly influence how many scanners are created with c.Concurrency rather than needing to account for two values. Ultimately, not using the scanner pool has little impact on performance (though I'd much rather have stability right now anyway).

To allow for yara-x to work in environments like Cloud Run, we'll still need a change that allows for WASM tuning to drop the memory reservation to hopefully free up virtual memory overhead and prevent panics.

@egibs egibs force-pushed the remove-scanner-pool branch from a7b70fd to d343832 Compare January 17, 2025 19:19
Copy link
Member

@stevebeattie stevebeattie left a comment

Choose a reason for hiding this comment

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

malcontent/cmd/mal/mal.go

Lines 372 to 376 in d343832

&cli.IntFlag{
Name: "scanners",
Value: runtime.NumCPU(),
Usage: "Number of scanners to create",
Destination: &scannersFlag,

Do want to mark this flag as currently not being honored?

@egibs
Copy link
Member Author

egibs commented Jan 17, 2025

malcontent/cmd/mal/mal.go

Lines 372 to 376 in d343832

&cli.IntFlag{
Name: "scanners",
Value: runtime.NumCPU(),
Usage: "Number of scanners to create",
Destination: &scannersFlag,

Do want to mark this flag as currently not being honored?

Oh, I need to remove that too.

I'm working on a Wolfi PR to help with our reservation problem as in parallel to this.

Signed-off-by: egibs <[email protected]>
@egibs egibs requested a review from stevebeattie January 17, 2025 21:46
@stevebeattie stevebeattie merged commit 0a9fb05 into chainguard-dev:main Jan 17, 2025
9 checks passed
@egibs egibs deleted the remove-scanner-pool branch January 17, 2025 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants