Add multi-worker ResultServer: one process per VM#2923
Add multi-worker ResultServer: one process per VM#2923wmetcalf wants to merge 2 commits intokevoreilly:masterfrom
Conversation
Spawns a dedicated Python process per VM, each with its own gevent event loop on a unique port, bypassing GIL serialization of result uploads across concurrent analyses. Enabled via multiworker=yes in cuckoo.conf [resultserver] section; default off preserves existing single-port behavior unchanged. - ResultServerWorkerProcess: multiprocessing.Process per VM - SingleVMResultServerWorker: gevent server reading task_id from shared multiprocessing.Value - Auto-assigns sequential ports (base_port + index) to machines - analysis_manager rooter calls use machine.resultserver_port instead of global config port for correct per-VM firewall rules
Summary of ChangesHello @wmetcalf, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the ResultServer's ability to handle concurrent result uploads from multiple virtual machines by introducing an optional multi-worker architecture. By dedicating a separate process and gevent event loop to each VM, it effectively bypasses Python's Global Interpreter Lock (GIL) limitations, improving performance and stability under heavy load. The changes are opt-in, ensuring backward compatibility with existing single-port configurations, and include automatic port assignment for ease of deployment. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a multi-worker mode for the ResultServer, which is a significant improvement for handling concurrent analyses by dedicating a process per VM. The implementation using multiprocessing is well-structured. The changes to configuration, machine initialization, and network routing are all consistent with this new architecture.
I've found a couple of areas for improvement: a potential race condition that could lead to misattributed analysis results, and a minor code cleanup opportunity.
| if not protocol: | ||
| return |
There was a problem hiding this comment.
There is a potential race condition here. The task could be cancelled and a new one started for this VM while negotiate_protocol() is blocking on I/O. The original GeventResultServerWorker handles this by re-checking the task ID after negotiation. This check is missing here and should be added to prevent results from being associated with the wrong task. This could lead to incorrect analysis results.
if self._shared_task_id.value != task_id:
log.warning(
"Task #%d for VM %s was cancelled during negotiation, new task is %s",
task_id, self._vm_ip, self._shared_task_id.value or "none"
)
return
if not protocol:
return- Add task_id re-check after negotiate_protocol() to prevent results from being associated with wrong task if cancelled during I/O - Remove unused task_id parameter from clear_task()
1 participant
Summary
multiprocessing.Processper VM, each with its own gevent event loop on a unique portmultiworker = no) — existing single-port behavior completely unchangedbase_port + machine_index) when no explicitresultserver_portis configured per machineanalysis_manager.pyrooter calls to usemachine.resultserver_portinstead of global config port, so iptables rules open the correct per-VM portConfig
Files changed
conf/default/cuckoo.conf.defaultmultiworkerandbase_portsettingslib/cuckoo/core/resultserver.pyResultServerWorkerProcess,SingleVMResultServerWorker, refactorResultServersingletonlib/cuckoo/common/abstracts.pylib/cuckoo/core/analysis_manager.pymachine.resultserver_portin all rooter callsTest plan
multiworker = no— verify existing behavior unchangedmultiworker = yeswith single VM — confirm results arrive on per-VM portmultiworker = yeswith multiple concurrent VMs — confirm no cross-contaminationPlease test — my testing environment is limited these days so additional eyes on this would be greatly appreciated.