-
Couldn't load subscription status.
- Fork 12
Orchestration of various Virtio devices (reopened) #41
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
Conversation
VirtioFS: Adding necessary qemu_args and starting VirtioFSD. Installed pinned VirtioFSD as a dependency. VirtioCON: Redirecting output to a given path for console VirtioPMEM: Using a image path for the persistent memory
Removed debug stuff
Future enhancement is having a schema option for printing VirtioFSD debug messages.
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.
Virtiofs parts look great! With the few minor comments. Pmem and virtioconsole I have to do some more reading to fully understand - I'll do that!
default.nix
Outdated
|
|
||
| virtiofsd_pinned ? import (fetchTarball { | ||
| name = "nixpkgs-pinned-for-virtiofsd"; | ||
| url = "https://github.com/NixOS/nixpkgs/archive/13e8d35b7d6028b7198f8186bc0347c6abaa2701.tar.gz"; |
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.
I don't think we need this pin - we can inherit the toplevel pin, typically from IncldueOS' pinned.nix
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.
We can easily remove it. VirtioFSD should work with the latest nixpkgs. We need to check for breaking changes
when updating nixpkgs in the future.
| "type" : "number" | ||
| } | ||
| } | ||
| }, |
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.
So new device types for virtio console, fs and pmem - nice!
vmrunner/vmrunner.py
Outdated
| virtiofsd_args = ["virtiofsd", "--socket", socket, "--shared-dir", shared, "--sandbox", "none"] | ||
| self._virtiofsd_proc = subprocess.Popen(virtiofsd_args, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) # pylint: disable=consider-using-with | ||
|
|
||
| time.sleep(0.1) |
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.
We don't want to sleep - wait for a specific event if there's a race. You poll just below - there's probably a way to find out if the process is ready
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.
I reckon we can generate the socket and then wait for it to show up (the socket path).
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.
Found this TODO in the CI script for virtio-fs (virtio-fs-ci):
# TODO This is racy. Wait for the listening message on virtiofsd stderr instead.
while [ ! -e vhost-fs.sock ]; do
sleep 0.1
doneMay have changed because that belongs to an old VirtioFSD, but helpful for knowing when process is ready.
vmrunner/vmrunner.py
Outdated
|
|
||
| virtiofs_args = [] | ||
| if "virtiofs" in self._config: | ||
| socket = "/tmp/virtiofsd.sock" |
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.
This will be problematic when we run multiple vms simultaneously; I think you can just have python create a tempfile here
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.
Do you think it would be okay to use something like mktemp and unlink the socket just for getting a conflict-free name?
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.
Perhaps you could use Python's tempfile to create a temporary directory and put the socket there? Then you avoid the race and could get automatic cleanup when the script exits
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.
That seems like a clean way of achieving the wanted result.
vmrunner/vmrunner.py
Outdated
| mem_arg = [] | ||
| if "mem" in self._config: | ||
| mem_arg = ["-m", str(self._config["mem"])] | ||
| mem_arg = ["-m", f"size={self._config["mem"]},maxmem={64}G"] |
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.
1TB should be the least we support 😁
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.
Easy.
…o, made it possible with multiple pmem devices per VM
|
Ok. I have done the requested changes and made it possible to have multiple PMEM devices per VM. I have tested and it worked. The sockets are cleaned up automatically from the tip ^^· The functionality can be tested by doing this:
nix-shell --argstr unikernel <test-path> --argstr vmrunner <vmrunner-realpath> --argstr diskbuilder <diskbuilder-realpath> --run "./test.py"Note: This enables modern Virtio console and not legacy. We have a legacy VirtioCON driver (legacy boolean option possibly). In the future, we must consider multiple backends like path and socket. Socket could be hooked with socat for interactivity. |
$ nix-shell \
--argstr unikernel ./test/fs/integration/virtiofs \
--argstr vmrunner ~/repos/includeos/vmrunner \
--argstr diskbuilder ~/repos/includeos/diskbuilder \
--run ./test.py
$ nix-shell \
--argstr unikernel ./test/modern_virtio/integration \
--argstr vmrunner ~/repos/torgeiru/vmrunner \
--argstr diskbuilder ~/repos/torgeiru/diskbuilder \
--run "./test.py"both work fine on NixOS for me, but fail on Gentoo with the following errors after fixing pthread-related linking issues (see includeos/IncludeOS#2310):
The virtio-pmem test fails on both machines for me: |
|
Yes. I forgot to push private stuff will do. Changed something so that I could have multiple PMEM devices per VM. |
Sounds like a priority! Do you think you could write a mini tutorial on how one could load multiple files as an example? Perhaps a directory or repository which could potentially be merged into |
|
I'm getting similar errors as @mazunki on Ubuntu, and the issues are also picked up by Note that several networking related tests will fail until you follow the network config instructions found in shell.nix: Let me know when the virtio branch has passing tests so I can rerun this. |
Is this good enough before merging PRs? I use it myself for quick iteration, but thought traces of old stuff could mess things up. |
No, in CI I would not have this enabled. That said - I think it can give false negatives, but I don't expect false positives. So if it fails with CCache it has to be fixed, but passing with ccache does not necessarily imply it's reproducible. |
|
Okay. I know that @mazunki You got requested parts not found of gentoo, could you dump other previous errors and the command you used? The VirtioPMEM stuff I need to investigate further (diskbuilder may not be reproducible and buffer overflow issues). To test VirtioPMEM stuff, create a raw file and write to it (have demoed this for Mazunki sometime ago). Use strings on the image to observe the string. The VirtioPMEM test in Virtio demonstrates the functionality by grabbing a DAX driver object and then executes some start_addr method. Write to this and flush using the driver flush method (to be certain that changes are persistent). Alternatively, do a loop mount and build the image (dd 1MiB and format) according to the test and modify as you will. I can modify the test to do this instead of diskbuilder on request (loop mounting requires sudo). I think I will be able to fix diskbuilder but I have so many other things to think about hehe. TLDR: |
|
Ran this using <vm> [ FAT32 ] Running tests for FAT32
<vm> [x] VirtioBlk disk created
<vm> [x] Disk not empty
<vm> [x] Disk size is 2048 sectors
<vm> [ FAT ] Initializing FAT32 filesystem
<vm> [ofs=0 size=2048 (1048576 bytes)]
<vm>
<vm> [x] Filesystem auto-initializedd
<vm> [x] Filesystem recognized as FAT32
<vm> [x] Listing root directory
<vm> [x] Exactly four ents in root dir
<vm> [x] Ent is a file
<vm> [x] Ents name is 'banana.txt'
<vm> [ FAT32 ] Remounting disk.
<vm> [x] Disk not empty
<vm> [x] Disk size is 1048576 bytes
<vm> [ FAT ] Initializing FAT32 filesystem
<vm> [ofs=0 size=2048 (1048576 bytes)]
<vm>
<vm> [x] Filesystem mounted on VBR1
<vm> [ FAT32 ] Shallow banana
<vm> [x] Stat /banana.txt
<vm> [x] Stat file in root dir
<vm> [x] Entity is file
<vm> [x] Entity is not directory
<vm> [x] Name is 'banana.txt'
<vm> [ FAT32 ] Read file
<vm> [x] read_file: Read /banana.txt asynchronously
<vm> ____ ___
<vm> | _ \ ___ _ _.' _ `.
<vm> _ | [_) )' _ `._ _ ___ ! \ | | (_) | _
<vm> |:;.| _ <| (_) | \ | |' _ `| \| | _ | .:;|
<vm> | `.[_) ) _ | \| | (_) | | | | |.',..|
<vm> ':. `. /| | | | | _ | |\ | | |.' :;::'
<vm> !::, `-!_| | | |\ | | | | | \ !_!.' ':;!
<vm> !::; ":;:!.!.\_!_!_!.!-'-':;:'' '''!
<vm> ';:' `::;::;' '' ., .
<vm> `: .,. `' .::... . .::;::;'
<vm> `..:;::;:.. ::;::;:;:;, :;::;'
<vm> "-:;::;:;: ':;::;:'' ;.-'
<vm> ""`---...________...---'""
<vm>
<vm> [x] Correct shallow banana
<vm> [ FAT32 ] Deep banana
<vm> [x] Stat /dir1/dir2/dir3/dir4/dir5/dir6/banana.txt
<vm> [x] Stat file in deep dir
<vm> [x] Entity is file
<vm> [x] Entity is not directory
<vm> [x] Name is 'banana.txt'
<vm> [ FAT32 ] Read inside stat
<vm> [x] read: Read /dir1/dir2/dir3/dir4/dir5/dir6/banana.txt asynchronously
<vm> [x] Correct deep fried banana
<vm> [ FAT32 ] SUCCESS
[ SUCCESS ] <VMRunner> All tests passedThe ^^ is the VirtioPMEM test (copy of the VirtioBLK test) running (no need for the diskbuilder argstr because upstream). The FAT driver understands the image and therefore it passes. Still, it is interesting when the diskbuilder issues appear. I am running debian on my overdue laptop btw. I will investigate these things, but I am currently pretty darn close to a breakthrough with VirtioFS DAX (have succesfully built legacy QEMU and VirtioFSD with DAX patches, hellish to find because no documentation). |
|
Should I close this PR for now and send a smaller PR next time (one device per PR). Will postpone the Virtio integrations? |
No, please leave it open, I'll try to test it without your IncludeOS changes. |
|
Things will be easier with vmrunner changes upstream :) Promise to split up IncludeOS PRs for a better review experience |
Did you figure this out? If not, includeos/IncludeOS#2316 might help you. |
This worked for me now - thanks! |
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.
Thanks! I got the virtiopmem example to work now, and tested both other options manually. I can see virtiocon come up as a recognized PCI device and virtiofs spawns a virtiofsd daemon when the shared folder exists.
+--[ Simple comm. controller , VirtIO (0x1043) ]
+--[ Storage controller, VirtIO (0x105a) ]
These two entries show up in the PCI discovery when virtiofs and virtiocon entries are added to vm.json, and I think that's enough to validate these changes.
|
|
||
| info("Successfully started VirtioFSD!") | ||
|
|
||
| while not os.path.exists(socket): |
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.
This will hang with no helpful information if the shared folder doesn't exist:
* <VMRunner>: Booting ./fs_virtiopmem.elf.bin directly without bootloader (multiboot / -kernel args)
* <VMRunner>: Successfully started VirtioFSD!
You can reproduce this by adding this to vm.json
"virtiofs" : {
"shared" : "./this_is_shared"
}
And then booting with boot, when ./this_is_shared does not exist. After creating the directory this_is_shared, the unikernel starts. So we should check if the provided shared folder exists and is a folder, and if not error out. But since it works in the happy path, and only takes effect if you actually add a virtiofs entry to vm.json, I'm happy to merge this - just add the check in a separate PR.
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.
This is because poll() returns immediately and error might not have been raised before poll() have returned (this is a race). I quickly tried to add a time.sleep(0.1) and it didn't hang because poll then returns the error code and the failure exception is raised. We need another signal. Any suggestions on how to solve this? I know that tests use a "SUCCESS" to indicate a successful test. I think a similar pattern could be used here (error is printed early on error, a watch).
|
Thank you! Did you try to run the I have do some warning cleanup, constexpr and stuff but the queue implementation I think is solid and clean. |
Orchestration of VirtioFS, VirtioPMEM and VirtioCON (for testing).
Dependency used for testing upcoming modern Virtio, VirtioFS and VirtioPMEM.