[WIP] Experimental krunkit support #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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughAdds Krunkit as an alternative provider to Vfkit on macOS, introduces provider selection via CRC_PROVIDER, updates vfkit APIs to provider-aware calls, adjusts driver initialization and preflight, and adds Krunkit-specific signing during cache extraction. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CRC CLI / Env
participant Cache as cache layer
participant Driver as vfkit/krunkit driver
participant Daemon as CRC daemon
participant Codesign as macOS codesign
Note over CLI: User/installer triggers fetch/install
CLI->>Cache: request download for Provider() (vfkit/krunkit)
Cache->>Cache: determine DownloadURL() and Version()
Cache->>Cache: extract file (supports .tar.gz/.tgz)
alt Executable == KrunkitCommand
Cache->>Codesign: create entitlements plist & run codesign on final path
Codesign-->>Cache: signing result
end
Cache->>Driver: place helper at ResolveHelperPath(Provider())
CLI->>Daemon: launch with env CRC_PROVIDER=Provider()
Daemon->>Driver: start provider-specific helper
Driver->>Driver: provider-specific startup (no TimeSync for krunkit)
Driver-->>Daemon: ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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.
vyasgun
force-pushed
the
spike/krun
branch
2 times, most recently
from
0ca061e to
7189720
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/crc/machine/vfkit/constants.go (1)
1-1: Critical: Build tag allows non-Darwin builds to access Darwin-only constants.The build tag
//go:build darwin || buildallows this file to be compiled on non-Darwin platforms when thebuildtag is used. However, this file referencesconstants.Provider()which is only defined inconstants_darwin.go(Darwin-only), causing build failures on Windows as shown in static analysis.🔎 Recommended approach
Either remove the
|| buildfrom the tag to make this strictly Darwin-only:-//go:build darwin || build +//go:build darwinOr create platform-specific stub implementations of
constants.Provider()for non-Darwin platforms if cross-compilation is needed.pkg/crc/cache/cache.go (1)
62-67: Fix documentation-code mismatch and verify version output formats.The comment documenting
getVersionGenericstates the version format issomething: <version>(colon-separated), but the implementation splits on space at line 62. This inconsistency must be resolved.Additionally, verify that the space-based splitting is compatible with actual version outputs from:
- vfkit (called with
-v)- libvirt machine driver (called with
version)Note: The AdminHelper parsing (lines 70-85) uses a different strategy—it takes the last element from space-split output, not the second element. Consider whether this inconsistency warrants unification.
♻️ Duplicate comments (1)
pkg/drivers/vfkit/driver_darwin.go (1)
437-441: Process name validation only checks the current provider, missing the other.Line 437 validates that the process name starts with
constants.Provider(). However, when loading an existing VM that was created with a different provider, this check will fail. For example, if a VM was created with vfkit but the current environment hasCRC_PROVIDER=krunkit, the validation will incorrectly reject the running vfkit process.🔎 Proposed fix
- if !strings.HasPrefix(name, constants.Provider()) { + 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 }This allows the driver to recognize both vfkit and krunkit processes regardless of the current provider setting.
Based on learnings, this issue was previously identified in past review comments.
🧹 Nitpick comments (1)
pkg/crc/constants/constants_darwin.go (1)
19-33: Consider returning an error instead of log.Fatalf for invalid providers.The
log.Fatalfcall terminates the process immediately when an invalid provider is specified. This prevents graceful error handling and might be surprising to users. Consider returning an error instead and letting the caller decide how to handle it.🔎 Proposed alternative approach
-func Provider() string { +func Provider() (string, error) { provider := os.Getenv("CRC_PROVIDER") if provider == "" { provider = VfkitCommand } switch provider { case VfkitCommand: - return VfkitCommand + return VfkitCommand, nil case KrunkitCommand: - return KrunkitCommand + return KrunkitCommand, nil default: - log.Fatalf("Invalid provider: %s. Choose between %s or %s", provider, VfkitCommand, KrunkitCommand) - return "" + return "", fmt.Errorf("invalid provider: %s. Choose between %s or %s", provider, VfkitCommand, KrunkitCommand) } }Note: This would require updating all call sites to handle the error.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
pkg/crc/cache/cache.go(3 hunks)pkg/crc/cache/cache_darwin.go(1 hunks)pkg/crc/constants/constants_darwin.go(2 hunks)pkg/crc/machine/vfkit/constants.go(1 hunks)pkg/crc/machine/vfkit/driver_darwin.go(1 hunks)pkg/crc/preflight/preflight_darwin.go(1 hunks)pkg/drivers/vfkit/driver_darwin.go(3 hunks)pkg/libmachine/load_darwin.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/crc/cache/cache_darwin.go
🧰 Additional context used
🧬 Code graph analysis (5)
pkg/libmachine/load_darwin.go (2)
pkg/drivers/vfkit/driver_darwin.go (1)
NewDriver(55-74)pkg/crc/constants/constants_darwin.go (1)
Provider(19-33)
pkg/crc/constants/constants_darwin.go (1)
pkg/crc/machine/vfkit/constants.go (2)
VfkitCommand(13-13)KrunkitCommand(15-15)
pkg/crc/cache/cache.go (2)
pkg/crc/constants/constants_darwin.go (1)
KrunkitCommand(16-16)pkg/os/exec.go (1)
RunWithDefaultLocale(63-65)
pkg/drivers/vfkit/driver_darwin.go (2)
pkg/crc/constants/constants.go (1)
ResolveHelperPath(145-150)pkg/crc/constants/constants_darwin.go (1)
Provider(19-33)
pkg/crc/machine/vfkit/constants.go (2)
pkg/crc/constants/constants_darwin.go (3)
VfkitCommand(15-15)Provider(19-33)KrunkitCommand(16-16)pkg/crc/constants/constants.go (1)
ResolveHelperPath(145-150)
🪛 GitHub Actions: Build Windows chocolatey
pkg/crc/cache/cache.go
[error] 147-147: undefined: constants.KrunkitCommand
🪛 GitHub Actions: Test OKD bundle
pkg/crc/cache/cache.go
[error] 147-147: undefined: constants.KrunkitCommand
🪛 GitHub Check: build (ubuntu-latest, 1.24)
pkg/crc/cache/cache.go
[failure] 147-147:
undefined: constants.KrunkitCommand
🪛 GitHub Check: build (windows-2022, 1.24)
pkg/crc/cache/cache.go
[failure] 147-147:
undefined: constants.KrunkitCommand
🪛 GitHub Check: build-installer (windows-2022, 1.24)
pkg/crc/machine/vfkit/constants.go
[failure] 40-40:
undefined: constants.Provider
[failure] 29-29:
undefined: constants.Provider
[failure] 25-25:
undefined: constants.Provider
🪛 GitHub Check: Run OKD bundle with crc (1.24)
pkg/crc/cache/cache.go
[failure] 147-147:
undefined: constants.KrunkitCommand
⏰ 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). (13)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build (macOS-26, 1.24)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: build (macOS-15-intel, 1.24)
- GitHub Check: build (ubuntu-22.04, 1.24)
- GitHub Check: verify-devcontainer
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (8)
pkg/crc/cache/cache.go (1)
224-224: LGTM! Added .tgz extension support for krunkit tarball.The krunkit download URL uses
.tgzformat, so adding this extension to the tarball detection is correct.pkg/libmachine/load_darwin.go (2)
6-7: LGTM! Proper imports for provider abstraction.Added the constants package and aliased the vfkit driver package for clarity.
12-12: LGTM! Correct usage of dynamic provider selection.Using
constants.Provider()allows the driver to be instantiated with either vfkit or krunkit based on theCRC_PROVIDERenvironment variable. This properly implements the provider abstraction.Also applies to: 33-33
pkg/crc/constants/constants_darwin.go (1)
15-16: LGTM! Provider command constants properly defined.The constants clearly identify the two supported providers on Darwin.
pkg/crc/preflight/preflight_darwin.go (1)
36-36: LGTM! Dynamic provider names in user-facing messages.The preflight check descriptions now correctly reflect the selected provider (vfkit or krunkit) in user-facing messages, improving clarity when krunkit is used.
Also applies to: 38-38, 44-44
pkg/crc/machine/vfkit/constants.go (2)
12-16: LGTM! Provider constants clearly defined.The constants for both vfkit and krunkit versions and commands are well-organized.
28-48: LGTM! Provider-based helper functions correctly implement abstraction.The
DownloadURL()andVersion()functions properly switch based on the provider, enabling dynamic selection between vfkit and krunkit.pkg/drivers/vfkit/driver_darwin.go (1)
55-55: LGTM! NewDriver signature updated for provider abstraction.The constructor now accepts
executableNameand uses it to resolve the helper path viaconstants.ResolveHelperPath(). This enables dynamic provider selection.Also applies to: 72-72
| if c.GetExecutableName() == constants.KrunkitCommand { | ||
| err = signKrunkit(finalExecutablePath) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Critical: Platform-specific constant used in non-platform-specific file causes build failures.
The code references constants.KrunkitCommand which is only defined in constants_darwin.go. This causes build failures on Windows and Linux platforms as shown in the pipeline failures and static analysis hints.
🔎 Proposed fix
Add build tags to this file or move the krunkit-specific logic to a Darwin-only file:
Option 1: Add build tags to this section
//go:build darwin
func (c *Cache) cacheExecutable() error {
// ... existing code ...
if c.GetExecutableName() == constants.KrunkitCommand {
err = signKrunkit(finalExecutablePath)
if err != nil {
return err
}
}
return nil
}Option 2: Move signing to a platform-specific file
Create cache_darwin.go:
//go:build darwin
package cache
func (c *Cache) platformSpecificPostCache(finalExecutablePath string) error {
if c.GetExecutableName() == constants.KrunkitCommand {
return signKrunkit(finalExecutablePath)
}
return nil
}Then call it from the main cacheExecutable function (with a no-op implementation in cache_other.go for other platforms).
🧰 Tools
🪛 GitHub Actions: Build Windows chocolatey
[error] 147-147: undefined: constants.KrunkitCommand
🪛 GitHub Actions: Test OKD bundle
[error] 147-147: undefined: constants.KrunkitCommand
🪛 GitHub Check: build (ubuntu-latest, 1.24)
[failure] 147-147:
undefined: constants.KrunkitCommand
🪛 GitHub Check: build (windows-2022, 1.24)
[failure] 147-147:
undefined: constants.KrunkitCommand
🪛 GitHub Check: Run OKD bundle with crc (1.24)
[failure] 147-147:
undefined: constants.KrunkitCommand
🤖 Prompt for AI Agents
In pkg/crc/cache/cache.go around lines 147-155, the code references
constants.KrunkitCommand (defined only in constants_darwin.go) causing
cross-platform build failures; to fix, move the krunkit-specific signing into a
Darwin-only implementation or guard the function with a darwin build tag: create
a darwin-specific helper (e.g., cache_darwin.go) that implements signing logic
for Krunkit and expose a no-op stub for other platforms (or mark the entire
cacheExecutable function with //go:build darwin and provide an alternative/no-op
in a non-darwin file), then call the platform-specific helper from the main flow
so non-Darwin builds no longer reference the darwin-only constant.
| func signKrunkit(executablePath string) error { | ||
| const krunkitEntitlements = `<?xml version="1.0" encoding="UTF-8"?> | ||
| <!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd"> | ||
| <plist version="1.0"> | ||
| <dict> | ||
| <key>com.apple.security.cs.disable-library-validation</key> | ||
| <true/> | ||
| <key>com.apple.security.hypervisor</key> | ||
| <true/> | ||
| </dict> | ||
| </plist> | ||
| ` | ||
| tmpFile, err := os.CreateTemp("", "krunkit-entitlements-*.plist") | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create temp entitlements file: %w", err) | ||
| } | ||
| defer os.Remove(tmpFile.Name()) | ||
|
|
||
| if _, err := tmpFile.WriteString(krunkitEntitlements); err != nil { | ||
| tmpFile.Close() | ||
| return fmt.Errorf("failed to write entitlements: %w", err) | ||
| } | ||
| tmpFile.Close() | ||
|
|
||
| _, _, err = crcos.RunWithDefaultLocale("codesign", "-s", "-", "--entitlements", tmpFile.Name(), "--force", executablePath) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| logging.Debugf("Signed %s", executablePath) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Code signing implementation looks correct but needs platform isolation.
The krunkit signing logic with entitlements for hypervisor access is appropriate for macOS. However, this function should be in a Darwin-specific file (e.g., cache_darwin.go) with appropriate build tags since it's only relevant on macOS.
🤖 Prompt for AI Agents
In pkg/crc/cache/cache.go lines 157-187 the macOS-only signKrunkit function must
be moved to a Darwin-specific file: create pkg/crc/cache/cache_darwin.go with
the proper build tags at the top (//go:build darwin and the legacy +build darwin
line), copy the signKrunkit implementation into that file, ensure required
imports (os, fmt, logging, crcos) are declared there, and remove the function
from cache.go so non-mac builds don’t reference macOS APIs; keep the function
signature unchanged and verify no unused imports remain in either file.
| VfkitDownloadURL = fmt.Sprintf("https://github.com/crc-org/vfkit/releases/download/v%s/%s", VfkitVersion, VfkitCommand) | ||
| VfkitEntitlementsURL = fmt.Sprintf("https://raw.githubusercontent.com/crc-org/vfkit/v%s/vf.entitlements", VfkitVersion) | ||
| KrunkitDownloadURL = fmt.Sprintf("https://github.com/containers/krunkit/releases/download/v%s/krunkit-podman-unsigned-%s.tgz", KrunkitVersion, KrunkitVersion) | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "constants.go" -path "*/vfkit/*" -type f 2>/dev/null | head -5Repository: crc-org/crc
Length of output: 91
🏁 Script executed:
cat -n ./pkg/crc/machine/vfkit/constants.goRepository: crc-org/crc
Length of output: 1479
Krunkit download URL references non-existent GitHub release v1.1.1.
The URL attempts to download from https://github.com/containers/krunkit/releases/download/v1.1.1/krunkit-podman-unsigned-1.1.1.tgz, but the containers/krunkit repository on GitHub only has releases v0.1.1 and v0.1.2. Release v1.1.1 does not exist, so the download will fail with a 404 error. Update KrunkitVersion to match an actual GitHub release, or use an alternative distribution method if v1.1.1 is only available elsewhere (e.g., Homebrew).
🤖 Prompt for AI Agents
In pkg/crc/machine/vfkit/constants.go around lines 19 to 22, the
KrunkitDownloadURL is built using KrunkitVersion v1.1.1 which does not exist on
the containers/krunkit GitHub releases (only v0.1.1 and v0.1.2 exist); update
KrunkitVersion to a valid release (for example "0.1.2") or change the URL to a
supported distribution (Homebrew or alternate host), regenerate the
KrunkitDownloadURL accordingly, and add a short comment noting the chosen source
and version so future changes reference a known existing release.
|
|
||
| func CreateHost(machineConfig config.MachineConfig) *vfkit.Driver { | ||
| vfDriver := vfkit.NewDriver(machineConfig.Name, constants.MachineBaseDir) | ||
| vfDriver := vfkit.NewDriver(machineConfig.Name, constants.MachineBaseDir, VfkitCommand) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consider using dynamic provider selection instead of hardcoded VfkitCommand.
Line 14 passes VfkitCommand directly to the driver constructor, which hardcodes the provider to vfkit. Since the PR introduces a provider abstraction via constants.Provider(), this should likely use that function instead to support both vfkit and krunkit dynamically.
🔎 Proposed fix
- vfDriver := vfkit.NewDriver(machineConfig.Name, constants.MachineBaseDir, VfkitCommand)
+ vfDriver := vfkit.NewDriver(machineConfig.Name, constants.MachineBaseDir, constants.Provider())📝 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.
| vfDriver := vfkit.NewDriver(machineConfig.Name, constants.MachineBaseDir, VfkitCommand) | |
| vfDriver := vfkit.NewDriver(machineConfig.Name, constants.MachineBaseDir, constants.Provider()) |
🤖 Prompt for AI Agents
In pkg/crc/machine/vfkit/driver_darwin.go around line 14, the driver is being
constructed with a hardcoded VfkitCommand which forces the vfkit provider;
replace that argument with the dynamic provider lookup (use constants.Provider()
or the equivalent provider value introduced in constants) so the driver is
created using the runtime-selected provider instead of the hardcoded
VfkitCommand; ensure the provider value matches the expected type of the
NewDriver constructor and update any imports or variable names accordingly.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/crc/machine/vfkit/constants.go (1)
1-1: Build tagdarwin || buildcauses cross-platform compilation failures.The build tag
//go:build darwin || buildallows this file to compile on non-Darwin platforms when the genericbuildtag is present (which is common). However, this file callsconstants.Provider()which is only defined inconstants_darwin.go. This causes the Windows and Linux build failures shown in static analysis.Either:
- Change the build tag to
//go:build darwinonly, or- Ensure
constants.Provider()has stubs for other platformsProposed fix (Option 1)
-//go:build darwin || build +//go:build darwin
♻️ Duplicate comments (4)
pkg/crc/machine/vfkit/driver_darwin.go (1)
14-14: Past review concern remains unaddressed.The previous review comment correctly identified that
VfkitCommandis hardcoded, which prevents dynamic provider selection between vfkit and krunkit. Since the PR introduces a provider abstraction (constants.Provider()), this line should use that function instead to support runtime provider selection.pkg/crc/machine/vfkit/constants.go (1)
19-21: Krunkit download URL may reference a non-existent release.The
KrunkitDownloadURLreferences version1.1.1fromcontainers/krunkit. Per previous review comments, only versions0.1.1and0.1.2exist on the GitHub releases page. Verify the correct version and update accordingly.containers/krunkit GitHub releases latest versionpkg/crc/cache/cache.go (2)
147-155: Critical: Platform-specific constant causes cross-platform build failures.
constants.KrunkitCommandis only defined inconstants_darwin.go, but this file (cache.go) compiles on all platforms. This causes the build failures on Windows, Linux, and in the OKD bundle tests as shown in the pipeline failures.Move the krunkit-specific signing logic to a Darwin-only file (e.g., keep it in
cache_darwin.goor create a new platform-specific file).Proposed approach
Option 1: Move to cache_darwin.go
Add a platform-specific post-cache hook in
cache_darwin.go:// In cache_darwin.go func (c *Cache) platformPostCache(finalExecutablePath string) error { if c.GetExecutableName() == constants.KrunkitCommand { return signKrunkit(finalExecutablePath) } return nil }Option 2: Create cache_signing_darwin.go
Move
signKrunkitand the conditional call to a new Darwin-only file with appropriate build tags.
157-187: macOS-specific signing function in cross-platform file.
signKrunkitusescodesignwhich is a macOS-only tool. This function should be in a Darwin-specific file with//go:build darwinbuild tag.
🧹 Nitpick comments (3)
pkg/crc/machine/vfkit/constants.go (1)
12-15: Duplicate constant definitions withpkg/crc/constants/constants_darwin.go.
VfkitCommandandKrunkitCommandare defined here and also inpkg/crc/constants/constants_darwin.go(lines 16-17 per the relevant snippets). This duplication can lead to maintenance issues and potential inconsistencies.Consider removing the duplicates here and importing from the
constantspackage, or consolidating all provider-related constants in one location.pkg/crc/constants/constants_darwin.go (2)
17-18: Consider consolidating duplicate constants.These constants appear to be duplicated in
pkg/crc/machine/vfkit/constants.go. To maintain a single source of truth and follow DRY principles, consider either importing them from the vfkit package or defining them in a shared location.
32-33: Unreachable return statement after fatal error.The
return ""on line 33 is unreachable becauseFatalfterminates the program. While this satisfies the compiler's requirement for a return value, it's misleading. Consider adding a comment to clarify this, or restructure the function to make the intent clearer.Alternative approach
default: logging.Fatalf("Invalid provider: %s. Choose between %s or %s", provider, VfkitCommand, KrunkitCommand) - return "" + return "" // unreachable: Fatalf terminates the program }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
pkg/crc/cache/cache.go(3 hunks)pkg/crc/cache/cache_darwin.go(1 hunks)pkg/crc/constants/constants_darwin.go(2 hunks)pkg/crc/machine/vfkit/constants.go(1 hunks)pkg/crc/machine/vfkit/driver_darwin.go(1 hunks)pkg/crc/preflight/preflight_darwin.go(1 hunks)pkg/drivers/vfkit/driver_darwin.go(3 hunks)pkg/libmachine/load_darwin.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/crc/preflight/preflight_darwin.go
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/crc/constants/constants_darwin.go (2)
pkg/crc/machine/vfkit/constants.go (2)
VfkitCommand(13-13)KrunkitCommand(15-15)pkg/crc/logging/logging.go (1)
Fatalf(108-110)
pkg/crc/machine/vfkit/constants.go (2)
pkg/crc/constants/constants_darwin.go (3)
VfkitCommand(17-17)Provider(21-35)KrunkitCommand(18-18)pkg/crc/constants/constants.go (1)
ResolveHelperPath(145-150)
🪛 GitHub Actions: Build Windows chocolatey
pkg/crc/cache/cache.go
[error] 147-147: undefined: constants.KrunkitCommand. Build failed while running: GOOS=windows GOARCH=amd64 go build -tags "containers_image_openpgp" -ldflags="-X github.com/crc-org/crc/v2/pkg/crc/version.crcVersion=2.57.0 -X github.com/crc-org/crc/v2/pkg/crc/version.ocpVersion=4.20.6 -X github.com/crc-org/crc/v2/pkg/crc/version.okdVersion=4.20.0-okd-scos.11 -X github.com/crc-org/crc/v2/pkg/crc/version.microshiftVersion=4.20.0 -X github.com/crc-org/crc/v2/pkg/crc/version.commitSha=cfc0a5 " -o out/windows-amd64/crc.exe ./cmd/crc
🪛 GitHub Actions: Test OKD bundle
pkg/crc/cache/cache.go
[error] 147-147: undefined: constants.KrunkitCommand
🪛 GitHub Check: build (macOS-15, 1.24)
pkg/crc/cache/cache.go
[failure] 147-147:
undefined: constants.KrunkitCommand
🪛 GitHub Check: build (ubuntu-22.04, 1.24)
pkg/crc/cache/cache.go
[failure] 147-147:
undefined: constants.KrunkitCommand
🪛 GitHub Check: build (ubuntu-latest, 1.24)
pkg/crc/cache/cache.go
[failure] 147-147:
undefined: constants.KrunkitCommand
🪛 GitHub Check: build (windows-2022, 1.24)
pkg/crc/cache/cache.go
[failure] 147-147:
undefined: constants.KrunkitCommand
🪛 GitHub Check: build-installer (windows-2022, 1.24)
pkg/crc/machine/vfkit/constants.go
[failure] 40-40:
undefined: constants.Provider
[failure] 29-29:
undefined: constants.Provider
[failure] 25-25:
undefined: constants.Provider
🪛 GitHub Check: Run OKD bundle with crc (1.24)
pkg/crc/cache/cache.go
[failure] 147-147:
undefined: constants.KrunkitCommand
⏰ 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). (11)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: build (macOS-26, 1.24)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (macOS-15-intel, 1.24)
- GitHub Check: verify-devcontainer
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (8)
pkg/crc/machine/vfkit/driver_darwin.go (1)
30-42: LGTM!The shared directory configuration logic is correct. The function properly maps each directory with sequential tags and the appropriate virtiofs type.
pkg/crc/cache/cache_darwin.go (1)
9-18: Changes align with new provider abstraction.The updates to use
vfkit.DownloadURL()andvfkit.Version()correctly integrate with the new provider-aware API. The version parsing using--versionand trimming thevprefix is appropriate.Consider renaming
NewVfkitCacheandgetVfkitVersionto provider-agnostic names (e.g.,NewProviderCache,getProviderVersion) for consistency with the provider abstraction, though this can be deferred.pkg/crc/cache/cache.go (2)
224-224: Good addition of.tgzextension for tarball detection.This aligns with the krunkit download URL which uses
.tgzformat.
62-67: Comment/code mismatch in newgetVersionGenericfunction.The function comment (lines 50-54) documents the expected format as
something: <version>(colon-separated), but the implementation splits on space and returns element [1]. This inconsistency exists from the initial commit.Additionally,
NewAdminHelperCacheuses a different parsing strategy (takes the last element after space split), suggesting version output formats may vary across executables. Without test cases or documentation of the actual version output formats for vfkit, admin-helper, and krunkit, the correctness of this generic parser cannot be verified.Either update the comment to match the implementation, or fix the implementation to match the documented format.
pkg/libmachine/load_darwin.go (2)
6-12: Provider-aware driver initialization looks correct.The changes properly integrate
constants.Provider()into the driver construction, enabling dynamic selection between vfkit and krunkit. The import aliasing (vfkitDriver) improves readability.
27-38: Consistent provider handling in Load function.The
Loadfunction mirrors theNewHostchanges, ensuring consistent provider-aware driver initialization across both code paths.pkg/drivers/vfkit/driver_darwin.go (2)
55-74: Constructor properly parameterized for provider selection.The
NewDriverfunction now acceptsexecutableNameand usesconstants.ResolveHelperPath(executableName)to setVfkitPath. This enables the same driver code to work with both vfkit and krunkit.
433-441: Use the provider from driver state instead of reading from environment.The validation at line 437 relies on
constants.Provider(), which reads theCRC_PROVIDERenvironment variable. If this variable changes between VM start and state-check (or defaults to vfkit if unset), the validation could incorrectly reject or accept a stale process.Since
VfkitPathis persisted in the driver state and contains the actual provider that was used (e.g.,/path/to/bin/vfkitor/path/to/bin/krunkit), extract the executable name fromd.VfkitPathinstead of callingconstants.Provider()to validate the process name.
| package constants | ||
|
|
||
| import ( | ||
| "log" |
There was a problem hiding this comment.
Use the project's logging package instead of the standard library.
The standard library log package is imported, but the project has its own logging package at pkg/crc/logging with a Fatalf function. Line 32 should use logging.Fatalf instead of log.Fatalf to maintain consistency with the rest of the codebase.
🔎 Proposed fix
import (
- "log"
"os"
"path/filepath"
+
+ "github.com/crc-org/crc/v2/pkg/crc/logging"
)Then update line 32:
default:
- log.Fatalf("Invalid provider: %s. Choose between %s or %s", provider, VfkitCommand, KrunkitCommand)
+ logging.Fatalf("Invalid provider: %s. Choose between %s or %s", provider, VfkitCommand, KrunkitCommand)
return ""Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pkg/crc/constants/constants_darwin.go around line 6 and line 32, the file
imports the standard library "log" and calls log.Fatalf; replace the import with
the project's logging package "pkg/crc/logging" and change the call to use
logging.Fatalf instead of log.Fatalf so the project-wide logger is used
consistently.
| func DownloadURL() string { | ||
| switch constants.Provider() { | ||
| case VfkitCommand: | ||
| return VfkitDownloadURL | ||
| case KrunkitCommand: | ||
| return KrunkitDownloadURL | ||
| default: | ||
| return "" | ||
| } |
There was a problem hiding this comment.
Default case returns empty string silently.
When constants.Provider() returns an unexpected value, DownloadURL() returns an empty string without logging or error indication. This could cause silent failures downstream when attempting to download.
Proposed fix
func DownloadURL() string {
switch constants.Provider() {
case VfkitCommand:
return VfkitDownloadURL
case KrunkitCommand:
return KrunkitDownloadURL
default:
+ // This should not happen as Provider() validates the value
return ""
}
}📝 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.
| func DownloadURL() string { | |
| switch constants.Provider() { | |
| case VfkitCommand: | |
| return VfkitDownloadURL | |
| case KrunkitCommand: | |
| return KrunkitDownloadURL | |
| default: | |
| return "" | |
| } | |
| func DownloadURL() string { | |
| switch constants.Provider() { | |
| case VfkitCommand: | |
| return VfkitDownloadURL | |
| case KrunkitCommand: | |
| return KrunkitDownloadURL | |
| default: | |
| // This should not happen as Provider() validates the value | |
| return "" | |
| } | |
| } |
🧰 Tools
🪛 GitHub Check: build-installer (windows-2022, 1.24)
[failure] 29-29:
undefined: constants.Provider
🤖 Prompt for AI Agents
In pkg/crc/machine/vfkit/constants.go around lines 28-36, the switch default
silently returns an empty string when constants.Provider() is unexpected; update
the default branch to log or warn the unexpected provider value (including the
actual value returned by constants.Provider()) before returning the empty string
so downstream callers have diagnostic context. Add the necessary import for the
chosen logger (e.g., fmt or log) if not present and ensure the log message
clearly states it is an unexpected provider and includes the provider string.
Add CRC_PROVIDER env var to switch between vfkit and krunkit. Default provider: vfkit Set the env var to use krunkit Usage: ``` export CRC_PROVIDER=krunkit crc setup crc start ``` Note: Krunkit binary is signed with embedded hypervisor entitlements at cache time.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
pkg/drivers/vfkit/driver_darwin.go (2)
176-305: TimeSync device setup should be conditional, not removed.The TimeSync device initialization has been completely removed from the
Start()method. According to the PR objectives, krunkit doesn't support TimeSync, but this functionality should be preserved for vfkit. Removing it entirely breaks time synchronization for vfkit users.The code should conditionally add the TimeSync device only when using vfkit, not krunkit.
🔎 Proposed fix
After line 274 (after adding the unixgram/vsock device), add conditional TimeSync setup:
+ // TimeSync is not supported by krunkit, only add it for vfkit + if d.QemuGAVsockPort != 0 && strings.Contains(d.VfkitPath, "vfkit") { + timesync, err := config.TimeSyncNew(d.QemuGAVsockPort) + if err != nil { + return err + } + err = vm.AddDevice(timesync) + if err != nil { + return err + } + } + args, err := vm.ToCmdLine()
437-437: Process name validation should check for both vfkit and krunkit, not just current provider.Line 437 validates the process name using
constants.Provider(), which returns the currently configured provider. This creates a problem when the provider is switched:
- VM is started with vfkit (process name is "vfkit")
- User sets
CRC_PROVIDER=krunkitfindVfkitProcess()checks if process starts with "krunkit"- Validation fails even though a valid vfkit process is running
The validation should accept both "vfkit" and "krunkit" as valid process names, not just the current provider.
🔎 Proposed fix
- if !strings.HasPrefix(name, constants.Provider()) { + 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 }pkg/crc/machine/vfkit/driver_darwin.go (1)
14-14: Use dynamic provider selection instead of hardcoded VfkitCommand.Line 14 passes
VfkitCommanddirectly to the driver constructor, which hardcodes the provider to vfkit. This defeats the purpose of the provider abstraction introduced in this PR. The code should useconstants.Provider()to support dynamic selection between vfkit and krunkit based on theCRC_PROVIDERenvironment variable.🔎 Proposed fix
- vfDriver := vfkit.NewDriver(machineConfig.Name, constants.MachineBaseDir, VfkitCommand) + vfDriver := vfkit.NewDriver(machineConfig.Name, constants.MachineBaseDir, constants.Provider())
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
pkg/crc/cache/cache.gopkg/crc/cache/cache_darwin.gopkg/crc/constants/constants_darwin.gopkg/crc/constants/constants_linux.gopkg/crc/constants/constants_windows.gopkg/crc/machine/vfkit/constants.gopkg/crc/machine/vfkit/driver_darwin.gopkg/crc/preflight/preflight_darwin.gopkg/drivers/vfkit/driver_darwin.gopkg/libmachine/load_darwin.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/crc/cache/cache.go
- pkg/crc/machine/vfkit/constants.go
- pkg/crc/preflight/preflight_darwin.go
- pkg/crc/constants/constants_darwin.go
- pkg/crc/cache/cache_darwin.go
🧰 Additional context used
🧬 Code graph analysis (4)
pkg/crc/constants/constants_linux.go (2)
pkg/crc/constants/constants_darwin.go (1)
Provider(21-35)pkg/crc/constants/constants_windows.go (1)
Provider(13-15)
pkg/crc/constants/constants_windows.go (2)
pkg/crc/constants/constants_darwin.go (1)
Provider(21-35)pkg/crc/constants/constants_linux.go (1)
Provider(16-18)
pkg/libmachine/load_darwin.go (3)
pkg/drivers/vfkit/driver_darwin.go (1)
NewDriver(55-74)pkg/crc/constants/constants_darwin.go (1)
Provider(21-35)pkg/crc/constants/constants.go (1)
ResolveHelperPath(145-150)
pkg/crc/machine/vfkit/driver_darwin.go (3)
pkg/crc/constants/constants.go (1)
MachineBaseDir(117-117)pkg/crc/constants/constants_darwin.go (1)
VfkitCommand(17-17)pkg/crc/machine/vfkit/constants.go (1)
VfkitCommand(13-13)
⏰ 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-qe (darwin, arm64)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build-installer (windows-2022, 1.24)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (4)
pkg/crc/constants/constants_windows.go (1)
10-10: LGTM! Appropriate stubs for Windows platform.The empty
KrunkitCommandconstant andProvider()function returning an empty string are appropriate for Windows, as krunkit/vfkit are macOS-specific virtualization providers.Also applies to: 13-15
pkg/crc/constants/constants_linux.go (1)
11-11: LGTM! Appropriate stubs for Linux platform.The empty
KrunkitCommandconstant andProvider()function returning an empty string are appropriate for Linux, as krunkit/vfkit are macOS-specific virtualization providers.Also applies to: 16-18
pkg/libmachine/load_darwin.go (1)
6-7: LGTM! Proper integration with provider abstraction.The import additions and the use of
constants.Provider()in theNewDriverconstructor calls correctly integrate with the provider abstraction layer, allowing runtime selection between vfkit and krunkit.Also applies to: 12-12, 33-33
pkg/drivers/vfkit/driver_darwin.go (1)
55-55: LGTM! Proper signature extension for provider abstraction.The
NewDriversignature extension to acceptexecutableNameas a third parameter, along with usingconstants.ResolveHelperPath(executableName)forVfkitPathinitialization, correctly implements the provider abstraction layer.Also applies to: 72-72
| if err := json.Unmarshal(h.RawDriver, &driver); err != nil { | ||
| return nil, err | ||
| } | ||
| driver.VfkitPath = constants.ResolveHelperPath(constants.Provider()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find the relevant files and understand the structure
echo "=== Searching for Driver struct definition ==="
fd -e go -e json | head -20
# Find load_darwin.go and examine context around line 37
echo -e "\n=== Examining load_darwin.go ==="
find . -name "load_darwin.go" -type f
# Search for Driver struct definition
echo -e "\n=== Searching for Driver struct ==="
rg "type Driver struct" -A 20 --type go | head -50
# Search for VfkitPath usage
echo -e "\n=== Searching for VfkitPath usage ==="
rg "VfkitPath" --type goRepository: crc-org/crc
Length of output: 5080
🏁 Script executed:
#!/bin/bash
# Let's look at the actual load_darwin.go file
cat -n pkg/libmachine/load_darwin.goRepository: crc-org/crc
Length of output: 1390
🏁 Script executed:
#!/bin/bash
# Search for how driver configuration is saved/loaded
echo "=== Searching for JSON serialization of Driver ==="
rg "json:" --type go | rg -i "driver\|vfkit\|provider" | head -30
# Search for constants.Provider usage
echo -e "\n=== Searching for constants.Provider usage ==="
rg "constants.Provider" --type goRepository: crc-org/crc
Length of output: 1096
🏁 Script executed:
#!/bin/bash
# Let's find the Driver struct more carefully
echo "=== Finding Driver struct in the codebase ==="
fd -e go -type f | xargs rg "type Driver struct" -l
echo -e "\n=== Full Driver struct definition ==="
fd -e go -type f | xargs grep -l "type Driver struct" | xargs cat | rg "type Driver struct" -A 50Repository: crc-org/crc
Length of output: 665
🏁 Script executed:
#!/bin/bash
# Let's examine the vfkit driver struct with field tags
cat -n pkg/drivers/vfkit/driver_darwin.go | head -50Repository: crc-org/crc
Length of output: 1642
🏁 Script executed:
#!/bin/bash
# Search for json tags on VfkitPath and nearby fields
rg "VfkitPath|json:" pkg/drivers/vfkit/driver_darwin.go -B 2 -A 2Repository: crc-org/crc
Length of output: 401
🏁 Script executed:
#!/bin/bash
# Let's check the base driver struct to understand field persistence
rg "type.*VMDriver|type.*BaseDriver" vendor/github.com/crc-org/machine/drivers/ -A 30 | head -100Repository: crc-org/crc
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Check what fields are actually stored in the JSON by looking at the base driver
find . -path ./vendor -prune -o -name "driver.go" -type f -print | xargs grep -l "VMDriver\|BaseDriver" | head -5Repository: crc-org/crc
Length of output: 146
🏁 Script executed:
#!/bin/bash
# Let's check if provider is stored anywhere in the configuration
rg "Provider\(\)" pkg/drivers/vfkit/driver_darwin.go -B 3 -A 3
# Also check if there's any provider field in the driver
rg "Provider" pkg/drivers/vfkit/driver_darwin.goRepository: crc-org/crc
Length of output: 321
🏁 Script executed:
#!/bin/bash
# Let's see the full Driver struct definition with all fields and potential json tags
cat -n pkg/drivers/vfkit/driver_darwin.go | sed -n '42,100p'Repository: crc-org/crc
Length of output: 2188
🏁 Script executed:
#!/bin/bash
# Check the VMDriver base class to see what fields it has and if they're serialized
cat -n vendor/github.com/crc-org/machine/drivers/base_driver.go | head -100Repository: crc-org/crc
Length of output: 143
🏁 Script executed:
#!/bin/bash
# Let's see if VfkitPath gets serialized by checking what happens when we marshal
# and if there are any struct tags
rg "type Driver struct" -A 50 pkg/drivers/vfkit/driver_darwin.go | head -60Repository: crc-org/crc
Length of output: 1434
🏁 Script executed:
#!/bin/bash
# Check what constants.Provider() returns and if it's configurable
cat -n pkg/crc/constants/constants.go | grep -A 20 "func Provider"Repository: crc-org/crc
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Find the constants file with Provider function
find . -path ./vendor -prune -o -name "*.go" -type f -print | xargs grep -l "func Provider" | grep -v vendorRepository: crc-org/crc
Length of output: 174
🏁 Script executed:
#!/bin/bash
# Let's check the constants_darwin.go file for Provider function
cat -n pkg/crc/constants/constants_darwin.go | grep -A 10 "func Provider"Repository: crc-org/crc
Length of output: 362
🏁 Script executed:
#!/bin/bash
# Check if Provider is a global function or method
rg "Provider\(\)" pkg/crc/constants/ -B 2 -A 2Repository: crc-org/crc
Length of output: 868
🏁 Script executed:
#!/bin/bash
# Let's examine what files exist in the constants directory
ls -la pkg/crc/constants/Repository: crc-org/crc
Length of output: 499
🏁 Script executed:
#!/bin/bash
# Now check the actual load_darwin.go again in context - see if VfkitPath is expected to be in JSON
# and look at what happens during unmarshal
cat -n pkg/libmachine/load_darwin.goRepository: crc-org/crc
Length of output: 1390
🏁 Script executed:
#!/by
# Check what fields would be unmarshalled from JSON in the VMDriver base
find . -path ./vendor -prune -o -name "*.go" -type f -print | xargs grep -l "type VMDriver" | head -3Repository: crc-org/crc
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Let's search for where Provider is defined and what it returns
rg "^func Provider|^var Provider" --type go | grep -v vendor | head -20Repository: crc-org/crc
Length of output: 243
🏁 Script executed:
#!/bin/bash
# Check all go files in constants package
ls -la pkg/crc/constants/*.goRepository: crc-org/crc
Length of output: 470
🏁 Script executed:
#!/bin/bash
# Check the full Provider() function to understand its behavior
cat -n pkg/crc/constants/constants_darwin.goRepository: crc-org/crc
Length of output: 1209
🏁 Script executed:
#!/bin/bash
# Let's verify if VfkitPath is actually used when starting the VM
# and what happens with different providers
rg "VfkitPath" pkg/drivers/vfkit/driver_darwin.go -B 5 -A 5Repository: crc-org/crc
Length of output: 852
🏁 Script executed:
#!/bin/bash
# Check if the VM configuration persists any provider information
# or if switching providers would actually cause issues
rg "krunkit|Provider" pkg/drivers/vfkit/driver_darwin.goRepository: crc-org/crc
Length of output: 107
🏁 Script executed:
#!/bin/bash
# Check the NewDriver function more carefully - see what executableName param does
cat -n pkg/drivers/vfkit/driver_darwin.go | sed -n '55,75p'Repository: crc-org/crc
Length of output: 893
🏁 Script executed:
#!/bin/bash
# Let's check if there are tests or documentation about provider switching
rg "CRC_PROVIDER" --type go -B 3 -A 3 | head -50Repository: crc-org/crc
Length of output: 437
🏁 Script executed:
#!/bin/bash
# Check what ResolveHelperPath does
rg "func ResolveHelperPath" -A 15 --type go pkg/crc/constants/Repository: crc-org/crc
Length of output: 1021
Unconditionally overwriting VfkitPath with current provider breaks existing VMs.
When loading an existing VM, line 37 overwrites VfkitPath with the currently configured provider via constants.Provider() (which reads the CRC_PROVIDER environment variable). Since VfkitPath is serialized to JSON by default and contains the filesystem path to the hypervisor executable, switching providers causes the VM's startup to use the wrong hypervisor:
- VM created and stored with vfkit → VfkitPath saved pointing to vfkit executable
- User sets
CRC_PROVIDER=krunkitand loads the VM - Line 37 overwrites VfkitPath to point to krunkit executable
- VM startup executes with krunkit's path instead of vfkit's → failure or incorrect behavior
The VM's original provider should be recorded and validated on load, not silently switched. Either:
- Persist the provider in the VM configuration and validate consistency
- Skip overwriting VfkitPath if it was already restored from JSON
- Document that switching providers requires VM recreation
🤖 Prompt for AI Agents
In pkg/libmachine/load_darwin.go around line 37, the code unconditionally sets
driver.VfkitPath = constants.ResolveHelperPath(constants.Provider()), which
overwrites the VfkitPath restored from the VM JSON and can change the hypervisor
used for existing VMs; change this to preserve an existing VfkitPath when the
driver was loaded from disk (i.e., only call ResolveHelperPath and assign
VfkitPath if driver.VfkitPath is empty), and also add a persisted Provider field
to the VM config (or use an existing field) and validate that the current
constants.Provider() matches the stored provider on load — if they differ, log a
clear error and refuse to silently switch the VfkitPath or fail fast with
guidance to recreate the VM.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/macos-installer.yml (1)
39-43: Remove debug step before merge.This debugging step should be removed before the PR is merged to production. While it's acceptable for work-in-progress troubleshooting, it adds no value to the final CI pipeline.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/macos-installer.yml
⏰ 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). (17)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: Run OKD bundle with crc (1.24)
- GitHub Check: build-installer (windows-2022, 1.24)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: verify-devcontainer
- GitHub Check: build (macOS-15-intel, 1.24)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: build (ubuntu-22.04, 1.24)
- GitHub Check: build (macOS-26, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
| uses: actions/setup-go@v6 | ||
| with: | ||
| go-version: ${{ matrix.go }} | ||
| cache: false |
There was a problem hiding this comment.
Reconsider disabling Go cache before merge.
Disabling the Go module cache will significantly increase CI build times. If this was added temporarily for debugging purposes during development, please revert it before the final merge.
🤖 Prompt for AI Agents
.github/workflows/macos-installer.yml around line 38: the workflow explicitly
sets "cache: false" which disables Go module caching and will slow CI; revert
this change by removing the "cache: false" line or restoring the original
caching setting (e.g., set to true or the previous cache key/config), or add a
clear TODO comment and limit the disablement to a temporary debug branch only
before merging.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/crc/constants/constants.go (1)
145-150: Logic correctly exempts krunkit from installer path resolution.The condition
executableName != KrunkitCommandensures that krunkit is always resolved fromCrcBinDirrather than the installer path, which aligns with the PR's approach of signing krunkit at cache time rather than bundling it with the installer.KrunkitCommand definitions are platform-specific via build tags: Each constant is defined in a platform-specific file (darwin, windows, linux) with appropriate build constraints, so they are not duplicates across platforms. However, on macOS there is a redundant definition—both
pkg/crc/machine/vfkit/constants.goandpkg/crc/constants/constants_darwin.godefineKrunkitCommand = "krunkit". Consider consolidating these to a single definition.Optionally, add a comment explaining why krunkit receives special treatment:
func ResolveHelperPath(executableName string) string { + // krunkit is signed at cache time with embedded entitlements, so it's always in CrcBinDir if version.IsInstaller() && executableName != KrunkitCommand { return filepath.Join(version.InstallPath(), executableName) } return filepath.Join(CrcBinDir, executableName) }
🧹 Nitpick comments (2)
pkg/crc/machine/vfkit/constants.go (2)
25-31: Consider adding a comment explaining the path difference.The function uses different path resolution strategies for krunkit (CrcBinDir) versus vfkit (ResolveHelperPath). While this aligns with the PR's notes about krunkit's embedded entitlements and distinct deployment model, a brief comment would help future maintainers understand why these providers are handled differently.
🔎 Example comment
func ExecutablePath() string { provider := constants.Provider() + // krunkit is placed in CrcBinDir with embedded entitlements, + // while vfkit uses the standard helper path if provider == KrunkitCommand { return filepath.Join(constants.CrcBinDir, provider) } return constants.ResolveHelperPath(provider) }
44-53: Default case returns empty string silently (same as DownloadURL()).This function has the same silent empty string return in the default case as
DownloadURL()(already flagged in past comments). For consistency and defensive coding, consider applying the same fix pattern—log or document the unexpected provider value before returning.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/crc/constants/constants.gopkg/crc/machine/vfkit/constants.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/crc/constants/constants.go (4)
pkg/crc/version/version.go (1)
IsInstaller(69-71)pkg/crc/machine/vfkit/constants.go (1)
KrunkitCommand(16-16)pkg/crc/constants/constants_darwin.go (1)
KrunkitCommand(18-18)pkg/crc/constants/constants_windows.go (1)
KrunkitCommand(10-10)
⏰ 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). (9)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: build-installer (windows-2022, 1.24)
- GitHub Check: Run OKD bundle with crc (1.24)
|
@vyasgun: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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
Add experimental krunkit provider support for macOS
Add CRC_PROVIDER env var to switch between vfkit and krunkit.
Default provider: vfkit
Set the env var to use krunkit
Usage:
Note: Krunkit binary is signed with embedded hypervisor entitlements at cache time.
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
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.