[WIP]: Support for libkrun using krunkit
#4137
Conversation
Signed-off-by: Ansuman Sahoo <[email protected]>
| "--cpus", fmt.Sprintf("%d", *inst.Config.CPUs), | ||
| "--device", fmt.Sprintf("virtio-serial,logFilePath=%s", filepath.Join(inst.Dir, filenames.SerialLog)), | ||
| "--krun-log-level", logLevelInfo, | ||
| "--restful-uri", fmt.Sprintf("unix://%s", restfulSocketPath(inst)), |
There was a problem hiding this comment.
The restful service is not very useful in krunkit:
- It always return "running"
- It may fail to respond if krunkit is terminating
- You cannot depend on it for stopping the vm since the vm may ignore the ACPI request
- Sending Stop request send an async event to libkrun and return immediately without waiting for ack from the library
For these reasons in minikube
- we use
--restfu-uri none://(which is also the default in latest krunkit, which is required to for using --device virtio-net,type=unixstream,path=...). - the vm state is the process state if the process exist the vm is running, if the process does not exist the vm is stopped
- we terminate the vm with SIGTERM
When the service will improved, it can make sense to use it for stopping the service gracefully, and terminate krunkit after a timeout if it failed to stop within a timeout.
Checking the status will always be the existence of the process. If the process exist you cannot start another instance, and if you want to stop it you must terminate the process anyway.
| "--device", fmt.Sprintf("virtio-serial,logFilePath=%s", filepath.Join(inst.Dir, filenames.SerialLog)), | ||
| "--krun-log-level", logLevelInfo, | ||
| "--restful-uri", fmt.Sprintf("unix://%s", restfulSocketPath(inst)), | ||
| "--bootloader", fmt.Sprintf("efi,variable-store=%s,create", filepath.Join(inst.Dir, KrunEfi)), |
There was a problem hiding this comment.
This is not needed - krunkit ignore this parameter. It exists for compatibility with vfkit.
| "--krun-log-level", logLevelInfo, | ||
| "--restful-uri", fmt.Sprintf("unix://%s", restfulSocketPath(inst)), | ||
| "--bootloader", fmt.Sprintf("efi,variable-store=%s,create", filepath.Join(inst.Dir, KrunEfi)), | ||
| "--device", fmt.Sprintf("virtio-blk,path=%s,format=raw", filepath.Join(inst.Dir, filenames.DiffDisk)), |
There was a problem hiding this comment.
Worth a comment that this is the boot disk. The order of the device matters.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| networkArg := fmt.Sprintf("virtio-net,type=unixstream,path=%s,mac=%s,offloading=true", |
There was a problem hiding this comment.
offloading=true cannot not work with socket_vment since it does not enable offloading in the vment interface, and we really don't want to enable offloading in vment since it lead to horrible performance.
See https://github.com/containers/krunkit/blob/main/docs/usage.md#offloading-performance-implications
Remove the offloating=true parameter to make the network work. I tested it with socket_vment here:
containers/krunkit#63 (comment)
| } | ||
| if err = diskUtil.ConvertToRaw(ctx, baseDisk, diffDisk, &diskSize, false); err != nil { | ||
| return fmt.Errorf("failed to convert %q to a raw disk %q: %w", baseDisk, diffDisk, err) | ||
| } |
There was a problem hiding this comment.
krunkit support qcow2 disks but using raw disk is likely to perform better. We can add a comment here.
| // SPDX-FileCopyrightText: Copyright The Lima Authors | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package krun |
There was a problem hiding this comment.
I'm not sure about the name. The program we use here is named krunkit, and the library is called libkrun. In minikube the driver is called krunkit similar to other drivers (vfkit, qemu). In lima we call the qemu driver qemu, so this driver should call krunkit for consistency.
There was a problem hiding this comment.
The "Kit" suffix is carried over from Docker, where it was HyperKit and LinuxKit and BuildKit and so on
| Instance *limatype.Instance | ||
| SSHLocalPort int | ||
|
|
||
| krunCmd *exec.Cmd |
There was a problem hiding this comment.
krunCmd is not a good name since the command is krunkit. This is internal detail not visible to users, but it makes the code more confusing.
| "github.com/lima-vm/lima/v2/pkg/ptr" | ||
| ) | ||
|
|
||
| type LimaKrunDriver struct { |
There was a problem hiding this comment.
Why do we need the Lima prefix? Do we use LimaQemuDriver for the qemu driver?
There was a problem hiding this comment.
Do we use LimaQemuDriver for the qemu driver?
Yes
| return nil | ||
| } | ||
|
|
||
| if err := l.krunCmd.Process.Signal(os.Interrupt); err != nil { |
There was a problem hiding this comment.
Use SIGTERM for terminating a process
| return nil | ||
| case <-timeout: | ||
| return l.krunCmd.Process.Kill() | ||
| } |
There was a problem hiding this comment.
You need to wait for the process after killing it. Otherwise it will remain a zombie until this process terminates.
| import "github.com/lima-vm/lima/v2/pkg/registry" | ||
|
|
||
| func init() { | ||
| registry.Register(New()) |
There was a problem hiding this comment.
This is only needed for built-in internal drivers, it is not needed for external drivers.
Important
This PR is not yet ready for reviewing. Networking and file sharing is yet to be implemented. Many things are also hardcoded directly and that needs to be fixed too.