[WIP] Temporary code changes to make crc use krunkit instead of vfkit on macOS #5040
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughThis change integrates krunkit as an alternative to vfkit by updating version references from 0.6.1 to 1.1.1, adding special-case path and version detection for krunkit, disabling the incompatible TimeSync device initialization, adjusting unix socket configuration formatting, and reducing MTU from 4000 to 1500 bytes in network configuration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related issues
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cmd/crc/cmd/daemon.go(1 hunks)pkg/crc/cache/cache.go(1 hunks)pkg/crc/cache/cache_darwin.go(1 hunks)pkg/crc/constants/constants.go(1 hunks)pkg/crc/machine/vfkit/constants.go(1 hunks)pkg/drivers/vfkit/driver_darwin.go(2 hunks)vendor/github.com/crc-org/vfkit/pkg/config/virtio.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-06T09:48:10.441Z
Learnt from: redbeam
Repo: crc-org/crc PR: 4866
File: pkg/fileserver/fs9p/shares.go:0-0
Timestamp: 2025-08-06T09:48:10.441Z
Learning: The vsock implementation in pkg/fileserver/fs9p/shares.go was temporarily removed because 9pfs doesn't fully support it yet. It will be added back when 9pfs provides proper vsock support.
Applied to files:
pkg/drivers/vfkit/driver_darwin.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: verify-devcontainer
- GitHub Check: Run OKD bundle with crc (1.24)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (4)
pkg/crc/cache/cache_darwin.go (1)
12-12: Simplification looks good, but depends on getVersionGeneric fix.The simplification to directly return
getVersionGeneric(executablePath, "-V")is cleaner. However, this relies on the krunkit-specific handling ingetVersionGeneric(cache.go).Ensure that the hardcoded path issue in cache.go is resolved (see my comment there), as this function depends on that logic.
pkg/crc/constants/constants.go (1)
146-148: Replace hardcoded krunkit path with configurable approach.Hardcoding the krunkit path to
/usr/local/bin/krunkitcreates several issues:
- Inflexibility: Users cannot use krunkit from a different location (e.g., homebrew installations may use different paths)
- Testing: Makes unit testing difficult without the exact file present
- Portability: Assumes a specific macOS installation convention
Consider these alternatives:
- Use an environment variable (e.g.,
CRC_KRUNKIT_PATH) with the hardcoded path as fallback- Add krunkit path to config file
- Search common installation locations
- Document this limitation if it's truly temporary spike code
cmd/crc/cmd/daemon.go (1)
80-80: Remove the unsubstantiated claim about MTU reduction and verify network configuration necessity.The MTU value of 1500 bytes cannot be verified as a reduction from 4000—no evidence exists in the codebase history of MTU being set to 4000 previously. Please verify:
- Whether this MTU value is appropriate for krunkit and if standard (1500) is indeed what krunkit requires
- If this network configuration should be conditional on the virtualization backend
- Whether vfkit users are affected by this configuration
vendor/github.com/crc-org/vfkit/pkg/config/virtio.go (1)
438-438: Avoid modifying vendor code; upstream this change or use a fork.Modifying vendored dependencies directly creates maintenance issues—changes will be lost during vendor updates and diverge from the upstream library. The
vfkitMagic=onparameter for krunkit compatibility should either be upstreamed to the vfkit repository or maintained in a fork until accepted upstream. If this is a temporary compatibility measure, document that this vendor modification must be preserved during future updates.
| if executablePath == "/usr/local/bin/krunkit" { | ||
| return strings.TrimSpace(strings.TrimPrefix(stdOut, "krunkit ")), nil | ||
| } |
There was a problem hiding this comment.
Replace hardcoded path check with a more flexible approach.
The hardcoded path check executablePath == "/usr/local/bin/krunkit" is brittle and will fail if krunkit is installed elsewhere. This creates several issues:
- Users with krunkit in a different location will get incorrect version parsing
- The logic couples version parsing to a specific installation path
- Makes the code harder to test
Consider instead:
- Check the executable basename (e.g.,
filepath.Base(executablePath) == "krunkit") - Or pass a flag indicating which version format to use
- Or detect the format from the output structure
Apply this diff to make the check more flexible:
- if executablePath == "/usr/local/bin/krunkit" {
+ if filepath.Base(executablePath) == "krunkit" {
return strings.TrimSpace(strings.TrimPrefix(stdOut, "krunkit ")), nil
}Add at the top of the file if not already present:
import (
"context"
"fmt"
"os"
+ "path/filepath"
"strings"Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pkg/crc/cache/cache.go around lines 62-64, replace the brittle hardcoded path
check executablePath == "/usr/local/bin/krunkit" with a flexible detection:
import path/filepath if not present, use filepath.Base(executablePath) ==
"krunkit" (or alternatively detect the version format by inspecting stdOut for a
"krunkit " prefix) and then parse the version accordingly; ensure you update
imports to include "path/filepath" if added and keep trimming logic for stdOut
only when the basename matches or the output format is detected.
| VfkitVersion = "1.1.1" | ||
| vfkitCommand = "krunkit" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's check the structure and find related files
find . -type f -name "*.go" | grep -E "(vfkit|krunkit)" | head -20Repository: crc-org/crc
Length of output: 166
🏁 Script executed:
#!/bin/bash
# Search for how these constants are used
rg "VfkitVersion|vfkitCommand" --type go -A 2 -B 2Repository: crc-org/crc
Length of output: 166
🏁 Script executed:
#!/bin/bash
# Search for URL patterns that might use these constants
rg "github.com.*vfkit.*release" --type go -A 1 -B 1Repository: crc-org/crc
Length of output: 166
🏁 Script executed:
#!/bin/bash
# Check the actual file to understand the full context
cat -n pkg/crc/machine/vfkit/constants.goRepository: crc-org/crc
Length of output: 166
🌐 Web query:
vfkit krunkit version 1.1.1 release GitHub
💡 Result:
containers/krunkit — krunkit v1.1.1 was released on Nov 4, 2025. Changes: "Fix the reference to libkrun-efi in krunkit" and version bump to 1.1.1. [1][2]
Sources:
[1] GitHub Releases — containers/krunkit (krunkit-1.1.1).
[2] GitHub repository — containers/krunkit.
🌐 Web query:
krunkit binary download v1.1.1
💡 Result:
You can download krunkit v1.1.1 from the project's GitHub Releases page (assets for v1.1.1 are attached there) or install via the Homebrew tap.
- GitHub releases (v1.1.1): https://github.com/containers/krunkit/releases (see krunkit-1.1.1). [1]
- Install via Homebrew: brew tap slp/krunkit && brew install krunkit. [2]
References:
[1] containers/krunkit — Releases (krunkit-1.1.1).
[2] krunkit installation / Lima / minikube docs (Homebrew tap instructions).
🌐 Web query:
vfkit krunkit relationship project
💡 Result:
Short summary — relationship and how the projects interact
- Krunkit is a libkrun-based VM launcher for macOS focused on lightweight VMs and guest GPU support (Mesa/Venus). It’s an alternative VM runtime/driver. [1][2]
- vfkit is the existing macOS HVF-backed VM helper used by projects like Lima/Podman/minikube as a VM backend. Krunkit and vfkit both serve as VM runtimes/drivers on macOS and are pluggable in tooling. [2][3]
- Practical relationship: projects (Lima, minikube, Podman, vmnet-helper, etc.) added krunkit as an alternative driver to vfkit so users can choose the VM backend that fits their needs (krunkit for GPU and libkrun features; vfkit for the established HVF path). Some tooling can swap or alias vfkit<>krunkit. Performance/networking tradeoffs are documented (krunkit offers GPU/offloading options; vfkit often shows better raw networking in some cases). [4][2][3]
If you want, I can list exact commands/how to switch a specific tool (minikube, lima, podman) to use krunkit instead of vfkit.
Update URLs and entitlements references for krunkit v1.1.1.
krunkit v1.1.1 is a separate release from the containers/krunkit project (not crc-org/vfkit). The URLs must be updated to point to the correct repository:
- The download URL should reference
https://github.com/containers/krunkit/releases/download/v1.1.1/instead ofcrc-org/vfkit - The entitlements reference should point to
containers/krunkitrepository, notcrc-org/vfkit
krunkit and vfkit are distinct macOS VM runtimes (libkrun-based vs HVF-backed); using vfkit repository URLs with krunkit binaries and version numbers is incorrect.
🤖 Prompt for AI Agents
In pkg/crc/machine/vfkit/constants.go around lines 12-13, the constants
reference krunkit v1.1.1 but the download and entitlements URLs point to
crc-org/vfkit; update those URLs and any entitlements references to use the
containers/krunkit repository and its release path
(https://github.com/containers/krunkit/releases/download/v1.1.1/) and change
repository identifiers from crc-org/vfkit to containers/krunkit so they
correctly reference the krunkit v1.1.1 artifacts.
| // if d.QemuGAVsockPort != 0 { | ||
| // timesync, err := config.TimeSyncNew(d.QemuGAVsockPort) | ||
| // if err != nil { | ||
| // return err | ||
| // } | ||
| // err = vm.AddDevice(timesync) | ||
| // if err != nil { | ||
| // return err | ||
| // } | ||
| // } |
There was a problem hiding this comment.
Implement conditional TimeSync support instead of commenting out.
The TimeSync device initialization has been completely disabled, which could lead to time synchronization issues in the VM. According to the PR objectives, krunkit doesn't support TimeSync, but this should be handled conditionally rather than removing the feature entirely.
Consider:
- Detect whether krunkit or vfkit is being used
- Only add TimeSync for vfkit
- Log a warning when TimeSync is skipped for krunkit
This approach maintains functionality for vfkit while supporting krunkit.
- // if d.QemuGAVsockPort != 0 {
- // timesync, err := config.TimeSyncNew(d.QemuGAVsockPort)
- // if err != nil {
- // return err
- // }
- // err = vm.AddDevice(timesync)
- // if err != nil {
- // return err
- // }
- // }
+ // krunkit doesn't support TimeSync device
+ if d.QemuGAVsockPort != 0 && !d.isKrunkit() {
+ timesync, err := config.TimeSyncNew(d.QemuGAVsockPort)
+ if err != nil {
+ return err
+ }
+ err = vm.AddDevice(timesync)
+ if err != nil {
+ return err
+ }
+ }Add a helper method to detect krunkit:
func (d *Driver) isKrunkit() bool {
return strings.Contains(d.VfkitPath, "krunkit")
}🤖 Prompt for AI Agents
In pkg/drivers/vfkit/driver_darwin.go around lines 278 to 287, replace the
commented-out TimeSync block with a conditional implementation: add a helper
method on Driver (isKrunkit) that detects krunkit via VfkitPath, then only
create and add the TimeSync device when not krunkit (i.e., for vfkit); if
krunkit is detected, skip adding the device and emit a warning via the driver
logger explaining TimeSync was skipped; ensure necessary imports (e.g., strings)
are present and propagate any errors from TimeSync creation or vm.AddDevice as
before.
| // name, err := p.Name() | ||
| // if err != nil { | ||
| // return nil, err | ||
| // } | ||
| // if !strings.HasPrefix(name, "vfkit") { | ||
| // // return InvalidExecutable error? | ||
| // log.Debugf("pid %d is stale, and is being used by %s", pid, name) | ||
| // return nil, nil | ||
| // } |
There was a problem hiding this comment.
Restore process name validation with support for both vfkit and krunkit.
Commenting out the process name validation removes an important safety check that prevents the driver from operating on unrelated processes. This could lead to:
- Sending signals to wrong processes
- Misidentifying the VM state
- Data corruption or unexpected behavior
Instead of disabling the check entirely, update it to accept both "vfkit" and "krunkit" as valid process names.
- // name, err := p.Name()
- // if err != nil {
- // return nil, err
- // }
- // if !strings.HasPrefix(name, "vfkit") {
- // // return InvalidExecutable error?
- // log.Debugf("pid %d is stale, and is being used by %s", pid, name)
- // return nil, nil
- // }
+ name, err := p.Name()
+ if err != nil {
+ return nil, err
+ }
+ if !strings.HasPrefix(name, "vfkit") && !strings.HasPrefix(name, "krunkit") {
+ // return InvalidExecutable error?
+ log.Debugf("pid %d is stale, and is being used by %s", pid, name)
+ return nil, nil
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // name, err := p.Name() | |
| // if err != nil { | |
| // return nil, err | |
| // } | |
| // if !strings.HasPrefix(name, "vfkit") { | |
| // // return InvalidExecutable error? | |
| // log.Debugf("pid %d is stale, and is being used by %s", pid, name) | |
| // return nil, nil | |
| // } | |
| name, err := p.Name() | |
| if err != nil { | |
| return nil, err | |
| } | |
| if !strings.HasPrefix(name, "vfkit") && !strings.HasPrefix(name, "krunkit") { | |
| // return InvalidExecutable error? | |
| log.Debugf("pid %d is stale, and is being used by %s", pid, name) | |
| return nil, nil | |
| } |
🤖 Prompt for AI Agents
In pkg/drivers/vfkit/driver_darwin.go around lines 446-454, restore the process
name validation: call p.Name() and handle the error (return err if non-nil),
then accept the process only if its name starts with "vfkit" or "krunkit"; if it
does not, log a debug message indicating the pid is stale/owned by another
process and return nil, nil (or the existing InvalidExecutable flow if
preferred). Ensure you use strings.HasPrefix checks for both "vfkit" and
"krunkit" and keep the logging/message consistent with surrounding code.
|
@vyasgun: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
Description
These are the temporary/informal code changes to make crc use krunkit instead of vfkit on macOS. I have hardcoded binary paths and changed the vendor files for vfkit (so krunkit sends a VFKT magic number to the daemon to establish connection).
Fixes: #4341
Relates to: #4233, #4980
Type of change
test, version modification, documentation, etc.)
Proposed changes
Testing
setupshould succeed.startshould create a krunkit processsshshould workContribution Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.