feat: add configurable vhost option for monitors#415
feat: add configurable vhost option for monitors#415cmainas merged 1 commit intourunc-dev:main-pr415from
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
95f8e5c to
5a3d153
Compare
|
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.
5a3d153 to
8fce760
Compare
|
@cmainas can you please review the changes.Thanks |
cmainas
left a comment
There was a problem hiding this comment.
Thank you @vinayakjeet .
The changes look good, Please rebase over main to resolve the conflicts, so we can merge it.
8fce760 to
d544e15
Compare
|
Hi @cmainas |
|
Hello @vinayakjeet , could you also please squash your commits? |
d544e15 to
7a4b16b
Compare
Signed-off-by: vinayakjeet <vinayakjeetog@gmail.com>
|
hi @cmainas ,commits are squashed. Thanks |
cmainas
left a comment
There was a problem hiding this comment.
Thank you @vinayakjeet for this addition.
PR: #415 Signed-off-by: vinayakjeet <vinayakjeetog@gmail.com> Reviewed-by: Charalampos Mainas <cmainas@nubificus.co.uk> Approved-by: Charalampos Mainas <cmainas@nubificus.co.uk>
PR: #415 Signed-off-by: vinayakjeet <vinayakjeetog@gmail.com> Reviewed-by: Charalampos Mainas <cmainas@nubificus.co.uk> Approved-by: Charalampos Mainas <cmainas@nubificus.co.uk>
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).