krun: implement support for passt networking#1913
krun: implement support for passt networking#1913slp wants to merge 5 commits intocontainers:mainfrom
Conversation
Reviewer's GuideThe PR integrates passt-based networking by launching a passt daemon with a UNIX socketpair, attaching a virtio-net interface to the microVM via krun_add_net_unixstream, and extending FD cleanup to preserve passt sockets. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
cc/ @sbrivio-rh for awareness |
|
Ephemeral COPR build failed. @containers/packit-build please check. |
src/libcrun/handlers/krun.c
Outdated
| else | ||
| { | ||
| /* We need to make sure passt has already started before continuing. A | ||
| simple way to do it is with a blocking read on its stdout. */ |
There was a problem hiding this comment.
Actually, you can skip this whole dance by not passing -f: passt behaves like usual UNIX daemons and forks into background when it's ready, and if you give -1 / --one-off, it terminates by itself once the guest disconnects.
Conversely, getting something from its stdout doesn't guarantee that it's ready. Which is the only (minor) issue I spotted here, by the way, so if you need to stick to this approach for whatever reason it looks fine to me as well.
There was a problem hiding this comment.
Indeed, this looks much better, thanks for the suggestion!
|
I think this would be amazing to have in crun! I did apply and run your patch and I can confirm that and I can't reach the network at all. I'm launching my container like this: Do I need some extra stuff? |
With Podman and pasta(1), you get networking automatically configured because Podman passes But there's no such thing in passt(1) as it can't magically enter your VM and do stuff there, so you need to use DHCP or SLAAC/NDP to configure the network in this case ( Maybe an approach similar to AsahiLinux/muvm#111 (tiny DHCP client embedded in muvm) would make sense here as well. |
|
Ah right, that makes a lot of sense. Sadly, it doesn't work: EDIT: |
It might be that you forgot to bring the interface up first? |
That's exactly what I was planning to do, basically by translating your Rust implementation into C for |
By the way, if you're looking for some quick-and-dirty netlink implementation, without using the full libnl, with pretty much just the parts you need to do that, you can probably draw some inspiration from https://passt.top/passt/tree/netlink.c or https://archives.passt.top/passt-dev/20231206160808.3d312733@elisabeth/. It's GPL code though, so you can't use it directly in libkrun's init (init/init.c is missing licensing information by the way). But I hope it helps anyway as documentation / how-to. Or maybe it's just more convenient to go with libnl, after all... |
| stop some syscalls used by mark_or_close_fds_ge_than. | ||
| */ | ||
| ret = mark_or_close_fds_ge_than (entrypoint_args->container, entrypoint_args->context->preserve_fds + 3, true, err); | ||
| if (entrypoint_args->custom_handler->vtable->close_fds) |
There was a problem hiding this comment.
When the if statement condition evaluates to true, then no error is created.
I think using crun_error_release (err); (in line 1656) requires that we know for sure err is equal to
NULL or pointing to an allocated error. It's not clear at first sight whether that requirement is fulfilled.
One idea is to replace line 897
return mark_or_close_fds_ge_than (first_fd_to_close, true, NULL);
with
return mark_or_close_fds_ge_than (first_fd_to_close, true, err);
and add err as a function argument to libkrun_close_fds()
Some handlers may need to preserve more fds than those related to stdio. Let's allow them to provide their own hook for doing that. Signed-off-by: Sergio Lopez <slp@redhat.com>
a1d7603 to
94c06c3
Compare
|
Unlikely you missed this, but in case you did: containers/libkrun#527 (review) :) libkrun should very soon support passt's vhost-user interface as well. Not fundamental but I guess at some point krun should switch to it. |
| #define LIBKRUN_DEFAULT_RAM_MIB 1024 | ||
|
|
||
| /* The minimum amount of RAM for a viable microVM is 128 MB. */ | ||
| #define LIBKRUN_MINIMUM_RAM_MIB 128 |
There was a problem hiding this comment.
Actually I managed to boot muvm guests with 64 MiB: containers/libkrunfw#85 (comment) ...it was saving a couple of milliseconds compared to 256 MiB if I recall correctly. With 32 MiB the kernel would panic pretty early (decompressing initramfs? I don't quite remember). I guess it's not a relevant difference to 128 MiB anyway.
There was a problem hiding this comment.
64 MiB boots, but it's pretty much unusable. Realistically speaking, even 128 MiB falls short, but I reckon there's a chance (albeit slim) someone manages to use it knowing what they are actually doing ;-)
Yes, the thing is we still don't know if vhost-user is 1.x or 2.x material. So it's safer to stay with regular pipe for the moment. Also, confidential computing use cases would likely still require the good ol' pipe. |
|
Running with AWS nitro, I'm met with the following: |
src/libcrun/handlers/krun.c
Outdated
| if (errno == ENOENT) | ||
| return 0; |
There was a problem hiding this comment.
crun_error_release (err);
is needed when returning 0
Some decisions, such as whether we need to start passt or not, need to happen before we switch to the container's mount namespace. With this change we move processing the VM configuration to the HANDLER_CONFIGURE_BEFORE_USERNS phase, so we can have the information early enough. Signed-off-by: Sergio Lopez <slp@redhat.com>
Since the introduction of libkrun_configure_vm, the legacy path was only walked on very exceptional situations. Let's consolidate both configuration mechanisms in libkrun_configure_vm. While there, make GPU a configurable option, instead of relying in heuristics. Signed-off-by: Sergio Lopez <slp@redhat.com>
The minimum viable amount for RAM for a microVM is 128MB, ignore configurations asking for less than that. Signed-off-by: Sergio Lopez <slp@redhat.com>
Introduce a new configuration flag (krun.use_passt) that enables users to request the microVM to use passt based networking instead of TSI. Right now, the container needs to configure the network by itself, but the next version of libkrun will embed a DHCP client in init.c Signed-off-by: Sergio Lopez <slp@redhat.com>
I'm not sure what's leading to EEXIST, could you please trace it with |
Introduce a new configuration flag (krun.use_passt) that enables users to request the microVM to use passt based networking instead of TSI.
Right now, the container needs to configure the network by itself, but the next version of libkrun will embed a DHCP client in init.c