search

firecracker-microvm / firecracker

Secure and fast microVMs for serverless computing.
http://firecracker-microvm.io
Apache License 2.0
26.22k stars 1.82k forks source link

Refactor `vmm` builder code to simplify logic that creates the microVM to boot #4547

Open bchalios opened 7 months ago

bchalios commented 7 months ago

Description

Currently, the logic to create a Vmm object is scattered across vmm/lib.rs and vmm.builder.rs files and it is quite convoluted and some times difficult to follow. Moreover, there is a lot of architecture specific code inserted in arbitrary places which further increases the un-readability.

Apart from the aesthetics and readability aspect, the state of the code makes it quite difficult to unit test. There are functions that take 7 arguments just to pass them (way) down the stack. That makes unit-testing very hard, since we often need to construct dummy versions of objects (like EventManager) even though they are not used in the part of the code we are trying to test.

Solution

Re-work the vmm construction code to simplify code paths and isolate architecture-specific code.

tommady commented 3 weeks ago

Hi, may I take this task to practice?

tommady commented 2 days ago

Hi @bchalios in the current main branch

the logic to create a Vmm object is scattered across vmm/lib.rs and vmm.builder.rs files

I don’t see any Vmm object returned in the vmm/lib.rs file. Please let me know if I’ve misunderstood.

since we often need to construct dummy versions of objects (like EventManager) even though they are not used in the part of the code we are trying to test.

Could you point me to an example of this? Or are you referring to

event_manager = EventManager::new()

as the so-called dummy version? and in the create_vmm_and_vcpus function, for instance, event_manager is passed to allow subscription to serial_device. This makes it unclear why event_manager could be omitted.

Could you share your thoughts on how you would approach avoiding the need to pass event_manager, as you mentioned?

After all, may I ask why the create_vmm_and_vcpus function is specified only for aarch64? It seems to be used in several places, even with x86_64.

Otherwise, I completely agree with your points and will work on addressing them.

thanks.

tommady commented 1 day ago

I just found this https://github.com/firecracker-microvm/firecracker/issues/4411 seems related