feat: add configurable vhost option for monitors#415
feat: add configurable vhost option for monitors#415vinayakjeet wants to merge 2 commits intourunc-dev:mainfrom
Conversation
✅ Deploy Preview for urunc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Hello @vinayakjeet , thank you for your contribution. Please fill the PR template correctly and fully. Also, please test your changes before submitting a PR. |
TestingThe following tests were run locally on WSL2 (Ubuntu 24.04, amd64):
I attempted to run All setup steps (Docker, containerd, systemd) were verified locally. Let me know if you would like me to re-test this on a native Linux host. |
cmainas
left a comment
There was a problem hiding this comment.
Hello @vinayakjeet ,
thank you for this PR, I have added a few initial comments. However, you can at least execute the tests that do not use devmapper and specifically check if the new option in the configuration does work as expected. In the e2e test cases you can see which are the ones that do not require devmapper https://github.com/urunc-dev/urunc/blob/main/tests/e2e/test_cases.go
You can run these tests with make test_ctr_{name_of_test}, same for any other tool
cmainas
left a comment
There was a problem hiding this comment.
Thank you @vinayakjeet for the changes.
A few comments:
- vhost can be enabled for virtio block too. Therefore, the block related cli options will need to get updated too
- Would it be possibly to check the value of vhost in more unit tests. The ones that check qemu, so we can also verify the default value.
|
@cmainas - Regarding vhost for block devices:
also should this use the same |
|
Hello @vinayakjeet , for the time being, vhost-blk-pci would be enough. We can later make the configuration more verbose. |
315df73 to
95f8e5c
Compare
|
@cmainas can you please review the changes, thanks. |
cmainas
left a comment
There was a problem hiding this comment.
Hello @vinayakjeet ,
first of all, I apologize because I did not pay enough attention to the virtio block vhost. Unfortunately, there is a need for a helper program running alongside the monitor for vhost and block. Therefore, more changes are required which can be done in the future. We can keep the network part in this PR.
Moreover, I have added a few comments about the testing. Overall the qemu_test.go does not seem to test the code in qemu.go, but in vmm.go. Also, the tests in urunc config are fine, except the last one, which can be simplified and moved to TestUruncConfigMap
Signed-off-by: vinayakjeet <vinayakjeetog@gmail.com>
95f8e5c to
5a3d153
Compare
Signed-off-by: vinayakjeet <vinayakjeetog@gmail.com>
|
hi @cmainas can you please review the changes. Thanks |
cmainas
left a comment
There was a problem hiding this comment.
Hello @vinayakjeet ,
the changes look ok. However, in the current form the unit tests in the hypervisor package will not be executed in CI or locally. Therefore, would it be possible to add a new target in the Makefile (see test_metrics for example https://github.com/urunc-dev/urunc/blob/main/Makefile#L234)
Also, please make sure to rebase over the main branch.
Description
This change adds a configurable
vhostoption to monitors.When enabled for QEMU, it allows the use of
vhost-netto improve network performance.Related issues
Fixes
How was this tested?
make lintmake test_ctrLLM usage
Yes – LLMs were used for drafting code and clarifying implementation details.
All changes were reviewed and verified manually.
Checklist
make lint).make test_ctr).