-
Notifications
You must be signed in to change notification settings - Fork 168
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
Switch to virtiofs #3428
Switch to virtiofs #3428
Conversation
Nice! Thanks for working on this. I guess we figured out if it would run without root privileges.
I always found the performance of 9p to be lacking too. Any anecdotal results you'd like to share on if you think this enhances performance? |
Does this require running with a RHEL9+ kernel? i.e. in our pipeline we're running on OpenShift which is still RHEL8 based. If so that could explain the pipeline failures. |
fa81b3c
to
5b96688
Compare
The host kernel is not relevant here AFAIK; this is just a protocol between the qemu process and the guest kernel. virtiofs is also enabled in RHEL8, so e.g. my dev workflow of doing things like e.g.
Nah, it was just racy because we were forking virtiofsd which will asynchronously create the socket, and effectively in parallel forking qemu which looks for it. It worked pretty reliably on my idle 16 core workstation, but the CI environment is much more likely to provoke races like this. I added code to synchronously wait for virtiofsd to create the socket, though a better fix would be to pre-allocate a socketpair. |
Ah I see, looks like there's another |
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.
Not tested but code LGTM. Nice to see this happening!
See coreos/coreos-assembler#3428 (comment) In `--sandbox none` scenarios, the calling process is already in an isolated container, and we may not be capable of invoking `unshare` again. Signed-off-by: Colin Walters <[email protected]>
Yep, rebased to pull in viritofsd from updates-testing! And I've sanity tested this works still locally (when running fcos/rhcos with |
That's exciting...not seeing this locally
|
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.
Didn't look at CI failures but the code change LGTM
/retest |
OK, looks like there is a bug in QEMU or in our QEMU setup in this PR:
|
FWIW I am seeing this |
In coreos#3428 I tried a wholesale switch to virtiofs. There are some kernel/KVM crashes we haven't yet debugged. It wasn't hard to factor things out so that it becomes a dynamic choice, keeping the previous 9p support. This way for e.g. local development one can now `env COSA_VIRTIOFS=1 cosa run --qemu-image rhcos.qcow2 --bind-ro ...`
One thing I hit was an error when I set FORCE_UNPRIVILEGED=1:
anyone else seeing something similar? If not I'll try to debug my setup. |
other than the |
I kicked off a |
Completed with success! |
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.
LGTM overall! Let's figure out the answers to the pending questions and we should be good to go.
Well, is it executable for you? Not reproducing this here. The continuous-integration/jenkins/pr-merge and ci/prow/rhcos jobs here also exercised the unprivileged path successfully. |
Ahh. It's not an executable file in the git repo and I have that volume mounted in. I'll open a PR to change that. This LGTM. |
In #3428 I tried a wholesale switch to virtiofs. That's a large change in one go; it wasn't hard to factor things out so that it becomes a dynamic choice, keeping the previous 9p support. This way for e.g. local development one can now `env COSA_VIRTIOFS=1 cosa run --qemu-image rhcos.qcow2 --bind-ro ...` Further work can then build on this to switch to virtiofs by default, and allow falling back to 9p. Once we're confident in virtiofs we can drop the 9p support.
Ah yes, so #3087 would have helped you... |
Likely fallout in coreos/rpm-ostree#4584 (comment) |
OK looks like this is scoped to just [rpm-ostree and bootupd](https://github.com/search?q=org%3Acoreos%20%22cosaPod(runAsUser%3A%200%2C%22&type=code). Trying to drop the Will look at a virtiofsd patch too. |
This provokes a virtiofs bug: https://gitlab.com/virtio-fs/virtiofsd/-/merge_requests/197 xref coreos/coreos-assembler#3428 (comment)
As a workaround for a virtiofs bug: https://gitlab.com/virtio-fs/virtiofsd/-/merge_requests/197 xref coreos/coreos-assembler#3428 (comment)
As a workaround for a virtiofs bug: https://gitlab.com/virtio-fs/virtiofsd/-/merge_requests/197 xref coreos/coreos-assembler#3428 (comment)
As a workaround for a virtiofs bug: https://gitlab.com/virtio-fs/virtiofsd/-/merge_requests/197 xref coreos/coreos-assembler#3428 (comment)
As a workaround for a virtiofs bug: https://gitlab.com/virtio-fs/virtiofsd/-/merge_requests/197 xref coreos/coreos-assembler#3428 (comment)
As a workaround for a virtiofs bug: https://gitlab.com/virtio-fs/virtiofsd/-/merge_requests/197 xref coreos/coreos-assembler#3428 (comment) Just like in coreos/rpm-ostree#4585.
As a workaround for a virtiofs bug: https://gitlab.com/virtio-fs/virtiofsd/-/merge_requests/197 xref coreos/coreos-assembler#3428 (comment) Just like in coreos/rpm-ostree#4585.
Looks like ostree too. Applied the same workaround as part of ostreedev/ostree#2054, but can split it out. |
As a temporary workaround for a virtiofs bug: https://gitlab.com/virtio-fs/virtiofsd/-/merge_requests/197 xref: coreos/coreos-assembler#3428
As a temporary workaround for a virtiofs bug: https://gitlab.com/virtio-fs/virtiofsd/-/merge_requests/197 xref: coreos/coreos-assembler#3428
As a temporary workaround for a virtiofs bug: https://gitlab.com/virtio-fs/virtiofsd/-/merge_requests/197 xref: coreos/coreos-assembler#3428
PR coreos#3593 was written and opened before coreos#3428 but merged after it. CI had passed but was not rerun, so this went in. This would be fixed by using a merge bot on this repo that reruns CI before merge (See also https://bors.tech/essay/2017/02/02/pitch/.) Fixes: d812406 ("kola: Add `--qemu-bind-ro`")
PR #3593 was written and opened before #3428 but merged after it. CI had passed but was not rerun, so this went in. This would be fixed by using a merge bot on this repo that reruns CI before merge (See also https://bors.tech/essay/2017/02/02/pitch/.) Fixes: d812406 ("kola: Add `--qemu-bind-ro`")
qemu: Refactor memory to actually use memfd
The previous change here didn't break anything, but didn't
actually work for virtiofs because we need to actually reference
the device. It appeared to work for me in local testing
because I accidentally duplicated the logic in the larger virtiofs
PR.
mantle/kola: Add
COSA_VIRTIOFS=1
and dual 9p/virtiofs supportIn #3428 I tried
a wholesale switch to virtiofs. That's a large change in one go;
it wasn't hard to factor things out so that it becomes a dynamic
choice, keeping the previous 9p support.
This way for e.g. local development one can now
env COSA_VIRTIOFS=1 cosa run --qemu-image rhcos.qcow2 --bind-ro ...
Further work can then build on this to switch to virtiofs by default,
and allow falling back to 9p. Once we're confident in virtiofs
we can drop the 9p support.
Switch to virtiofs by default
Closes: #1812
The key benefits here are:
parity)
we won't hit obscure bugs like the 9p OOM ones.
mantle: Drop 9p support
Per request to avoid carrying it as tech debt.