Issue #4881 Add Rosetta 2 support for Apple Silicon#5181
Conversation
|
Hi @cpepper96. Thanks for your PR. I'm waiting for a crc-org member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (8)
WalkthroughAdds optional Rosetta 2 support for Apple Silicon macOS: new config key with validation, flag propagation into start/machine configs and VFKit driver, VM-side Rosetta virtiofs device and in-VM binfmt setup during startup, and Darwin preflight checks and test updates. ChangesRosetta 2 integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 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/preflight/preflight_windows.go (1)
214-216:⚠️ Potential issue | 🔴 CriticalCompilation error: Missing argument in
getAllPreflightChecks.The call to
getPreflightCheckson line 215 passes only 5 arguments, but the function signature on line 234 requires 6 parameters. This will fail to compile.🐛 Proposed fix
func getAllPreflightChecks() []Check { - return getPreflightChecks(true, network.UserNetworkingMode, constants.GetDefaultBundlePath(crcpreset.OpenShift), crcpreset.OpenShift, false) + return getPreflightChecks(true, network.UserNetworkingMode, constants.GetDefaultBundlePath(crcpreset.OpenShift), crcpreset.OpenShift, false, false) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/crc/preflight/preflight_windows.go` around lines 214 - 216, getAllPreflightChecks calls getPreflightChecks with only five arguments but the getPreflightChecks signature expects six; update the call in getAllPreflightChecks to pass the missing sixth parameter (match the exact type and semantics required by getPreflightChecks) so the argument list and types align with the function signature — locate getPreflightChecks and add the appropriate value (constant, variable, or literal) to the call in getAllPreflightChecks to fix the compilation error.
🧹 Nitpick comments (3)
pkg/crc/preflight/preflight_darwin_test.go (1)
26-28: Consider adding the Rosetta case for the alternate first-flag path too.You now cover Rosetta-enabled counts for
getPreflightChecks(false, ...). Adding the correspondinggetPreflightChecks(true, ..., true)assertion would make this regression-proof if behavior diverges by that first flag later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/crc/preflight/preflight_darwin_test.go` around lines 26 - 28, The test currently asserts Rosetta-enabled preflight check counts only for getPreflightChecks(false, ...); add the matching assertions for the alternate first-flag path by calling getPreflightChecks(true, network.SystemNetworkingMode, constants.GetDefaultBundlePath(preset.OpenShift), preset.OpenShift, false, true) and getPreflightChecks(true, network.UserNetworkingMode, constants.GetDefaultBundlePath(preset.OpenShift), preset.OpenShift, false, true) and assert their lengths match the expected 21 and 20 respectively so both code paths are covered.pkg/drivers/vfkit/driver_darwin.go (1)
236-236: Use a shared constant for the Rosetta mount tag.Line 236 hardcodes
"rosetta". Since guest-side mount/config must match this tag exactly, a shared constant helps avoid drift across files.Suggested refactor
+const rosettaMountTag = "rosetta" ... - rosettaDev, err := config.RosettaShareNew("rosetta") + rosettaDev, err := config.RosettaShareNew(rosettaMountTag)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/drivers/vfkit/driver_darwin.go` at line 236, Replace the hardcoded "rosetta" string in the call rosettaDev, err := config.RosettaShareNew("rosetta") with a shared constant (e.g., config.RosettaTag) so guest-side mount/config matches the same identifier everywhere; update this call to use the constant from the config package and, if the constant does not yet exist, add it in the config package (name it clearly like RosettaTag or RosettaMountTag) and update any other callers to use that constant to prevent drift.pkg/crc/machine/start.go (1)
270-325: Consider using POSIX-compatible shell syntax forbinfmtconfiguration.The code uses here-string syntax (
<<<) on line 314, which is a bash-specific feature and will fail on systems using only POSIXsh. While RHEL/Fedora-based systems typically include bash, this reduces portability.♻️ Suggested improvement for broader shell compatibility
// Write Rosetta binfmt config pointing to the bind mount path rosettaBinfmt := `:rosetta:M::\x7fELF\x02\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x3e\x00:\xff\xff\xff\xff\xff\xfe\xfe\x00\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff\xff:/var/lib/rosetta/rosetta:CFP` if _, _, err := sshRunner.RunPrivileged("Writing Rosetta binfmt config", - fmt.Sprintf("tee /etc/binfmt.d/rosetta.conf <<< '%s'", rosettaBinfmt)); err != nil { + "sh", "-c", fmt.Sprintf("echo '%s' > /etc/binfmt.d/rosetta.conf", rosettaBinfmt)); err != nil { return err }Note: Similar bash-ism appears at line 509 for chrony config; consider applying the same fix there.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/crc/machine/start.go` around lines 270 - 325, The use of a bash-only here-string in configureRosetta should be replaced with a POSIX-compatible write (e.g., use a heredoc or printf piped to tee) so that rosettaBinfmt is written to /etc/binfmt.d/rosetta.conf without relying on <<<; update the RunPrivileged call that currently uses fmt.Sprintf("tee /etc/binfmt.d/rosetta.conf <<< '%s'", rosettaBinfmt) to instead pipe the content via a POSIX-safe form (for example: printf '%s\n' '<content>' | tee /etc/binfmt.d/rosetta.conf or tee /etc/binfmt.d/rosetta.conf <<'EOF' ... EOF) and apply the same change to the similar chrony config write elsewhere in the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/crc/config/validations.go`:
- Around line 159-162: The current platform check blocks any setting of the
rosetta flag on non-Apple-Silicon hosts; instead first validate the boolean
using ValidateBool(value), and only block when the validated value is true on
unsupported platforms. Concretely: call ValidateBool(value) (same symbol) to get
the parsed boolean and message, if parsing failed return that failure; if parsed
value is false return success (allow explicit false); if parsed value is true
then check runtime.GOOS and runtime.GOARCH and, if not darwin/arm64, return the
existing error message "Rosetta is only supported on Apple Silicon Macs",
otherwise return success.
In `@pkg/crc/preflight/preflight_checks_darwin.go`:
- Around line 232-234: The os.Stat(rosettaPath) error is currently always
reported as "Rosetta is not installed", which hides other failures; change the
handling so that if os.IsNotExist(err) you return the existing "Rosetta is not
installed" error, otherwise return a distinct error that includes the underlying
error (e.g., "failed to stat rosettaPath: <err>") so permission/I/O errors are
preserved; locate the os.Stat(rosettaPath) call in preflight_checks_darwin.go
and update the conditional to use os.IsNotExist(err) and return the
wrapped/original error for non-ENOENT cases.
---
Outside diff comments:
In `@pkg/crc/preflight/preflight_windows.go`:
- Around line 214-216: getAllPreflightChecks calls getPreflightChecks with only
five arguments but the getPreflightChecks signature expects six; update the call
in getAllPreflightChecks to pass the missing sixth parameter (match the exact
type and semantics required by getPreflightChecks) so the argument list and
types align with the function signature — locate getPreflightChecks and add the
appropriate value (constant, variable, or literal) to the call in
getAllPreflightChecks to fix the compilation error.
---
Nitpick comments:
In `@pkg/crc/machine/start.go`:
- Around line 270-325: The use of a bash-only here-string in configureRosetta
should be replaced with a POSIX-compatible write (e.g., use a heredoc or printf
piped to tee) so that rosettaBinfmt is written to /etc/binfmt.d/rosetta.conf
without relying on <<<; update the RunPrivileged call that currently uses
fmt.Sprintf("tee /etc/binfmt.d/rosetta.conf <<< '%s'", rosettaBinfmt) to instead
pipe the content via a POSIX-safe form (for example: printf '%s\n' '<content>' |
tee /etc/binfmt.d/rosetta.conf or tee /etc/binfmt.d/rosetta.conf <<'EOF' ...
EOF) and apply the same change to the similar chrony config write elsewhere in
the code.
In `@pkg/crc/preflight/preflight_darwin_test.go`:
- Around line 26-28: The test currently asserts Rosetta-enabled preflight check
counts only for getPreflightChecks(false, ...); add the matching assertions for
the alternate first-flag path by calling getPreflightChecks(true,
network.SystemNetworkingMode, constants.GetDefaultBundlePath(preset.OpenShift),
preset.OpenShift, false, true) and getPreflightChecks(true,
network.UserNetworkingMode, constants.GetDefaultBundlePath(preset.OpenShift),
preset.OpenShift, false, true) and assert their lengths match the expected 21
and 20 respectively so both code paths are covered.
In `@pkg/drivers/vfkit/driver_darwin.go`:
- Line 236: Replace the hardcoded "rosetta" string in the call rosettaDev, err
:= config.RosettaShareNew("rosetta") with a shared constant (e.g.,
config.RosettaTag) so guest-side mount/config matches the same identifier
everywhere; update this call to use the constant from the config package and, if
the constant does not yet exist, add it in the config package (name it clearly
like RosettaTag or RosettaMountTag) and update any other callers to use that
constant to prevent drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 07a9fe6e-c8bf-475b-b0b9-71d0f1971f16
📒 Files selected for processing (15)
cmd/crc/cmd/start.gopkg/crc/config/settings.gopkg/crc/config/validations.gopkg/crc/machine/config/config.gopkg/crc/machine/start.gopkg/crc/machine/types/types.gopkg/crc/machine/vfkit/driver_darwin.gopkg/crc/preflight/preflight.gopkg/crc/preflight/preflight_checks_darwin.gopkg/crc/preflight/preflight_darwin.gopkg/crc/preflight/preflight_darwin_test.gopkg/crc/preflight/preflight_linux.gopkg/crc/preflight/preflight_windows.gopkg/crc/preflight/preflight_windows_test.gopkg/drivers/vfkit/driver_darwin.go
| if runtime.GOOS != "darwin" || runtime.GOARCH != "arm64" { | ||
| return false, "Rosetta is only supported on Apple Silicon Macs" | ||
| } | ||
| return ValidateBool(value) |
There was a problem hiding this comment.
Allow explicit false for unsupported platforms.
Line 159 currently rejects the setting regardless of value on non-Apple-Silicon hosts. This prevents users from explicitly setting use-rosetta=false (useful for config portability/reset flows).
Suggested fix
func validateRosetta(value interface{}) (bool, string) {
- if runtime.GOOS != "darwin" || runtime.GOARCH != "arm64" {
- return false, "Rosetta is only supported on Apple Silicon Macs"
- }
- return ValidateBool(value)
+ enabled, err := cast.ToBoolE(value)
+ if err != nil {
+ return false, "must be true or false"
+ }
+ if !enabled {
+ return true, ""
+ }
+ if runtime.GOOS != "darwin" || runtime.GOARCH != "arm64" {
+ return false, "Rosetta is only supported on Apple Silicon Macs"
+ }
+ return true, ""
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/crc/config/validations.go` around lines 159 - 162, The current platform
check blocks any setting of the rosetta flag on non-Apple-Silicon hosts; instead
first validate the boolean using ValidateBool(value), and only block when the
validated value is true on unsupported platforms. Concretely: call
ValidateBool(value) (same symbol) to get the parsed boolean and message, if
parsing failed return that failure; if parsed value is false return success
(allow explicit false); if parsed value is true then check runtime.GOOS and
runtime.GOARCH and, if not darwin/arm64, return the existing error message
"Rosetta is only supported on Apple Silicon Macs", otherwise return success.
| if _, err := os.Stat(rosettaPath); err != nil { | ||
| return fmt.Errorf("Rosetta is not installed, which is required for x86_64 emulation") | ||
| } |
There was a problem hiding this comment.
Handle non-ENOENT errors separately in Rosetta preflight.
Line 232 currently treats every os.Stat failure as “not installed”. That can mask permission/I/O errors and mislead remediation.
Suggested fix
func checkRosettaInstalled() error {
- if _, err := os.Stat(rosettaPath); err != nil {
- return fmt.Errorf("Rosetta is not installed, which is required for x86_64 emulation")
+ if _, err := os.Stat(rosettaPath); err != nil {
+ if os.IsNotExist(err) {
+ return fmt.Errorf("Rosetta is not installed, which is required for x86_64 emulation")
+ }
+ return fmt.Errorf("failed to verify Rosetta installation at %s: %w", rosettaPath, err)
}
return 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.
| if _, err := os.Stat(rosettaPath); err != nil { | |
| return fmt.Errorf("Rosetta is not installed, which is required for x86_64 emulation") | |
| } | |
| func checkRosettaInstalled() error { | |
| if _, err := os.Stat(rosettaPath); err != nil { | |
| if os.IsNotExist(err) { | |
| return fmt.Errorf("Rosetta is not installed, which is required for x86_64 emulation") | |
| } | |
| return fmt.Errorf("failed to verify Rosetta installation at %s: %w", rosettaPath, err) | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/crc/preflight/preflight_checks_darwin.go` around lines 232 - 234, The
os.Stat(rosettaPath) error is currently always reported as "Rosetta is not
installed", which hides other failures; change the handling so that if
os.IsNotExist(err) you return the existing "Rosetta is not installed" error,
otherwise return a distinct error that includes the underlying error (e.g.,
"failed to stat rosettaPath: <err>") so permission/I/O errors are preserved;
locate the os.Stat(rosettaPath) call in preflight_checks_darwin.go and update
the conditional to use os.IsNotExist(err) and return the wrapped/original error
for non-ENOENT cases.
Could you share more details about your usecase for this? Last time we checked, running amd64 images in an openshift/microshift arm64 cluster was not supported, and there were complications with running images from other arches |
We have arm64 macs as our dev laptops but our target environment is an amd64 openshift cluster. We are receiving amd64 container images from an upstream vendor (we don't control the build process). So our use case is local development and testing of these amd64 images on our arm64 macs prior to deploying to our amd64 openshift cluster. |
can you be more specific about how you pull these images in the cluster and then run them? is this done using |
sure! we push the x86 images to the internal image registry in the openshift local cluster and configure the deployments to pull from there. |
@cpepper96 This target environment is not the mac-M1
So when you testing those amd64 images before pushing/deploying to your amd64 cluster, does it really required the OCP cluster locally or you just need podman to test those on M1? I am thinking you might just need podman desktop on your dev machine to test the images locally before push to cluster which does support running the amd64 images.
|
Hi @praveenkumar! We currently use podman/podman desktop and have used the workaround you suggested. There is no hard requirement to testing using OCP local but we have found it quite helpful to be able to do so and these changes enabled us to do that. |
@cpepper96 Happy to hear that you find OCP local helpful. I am just trying to understand the usecase which you described with OCP local because to me that is really just running a different arch container for which you really don't need a cluster but container runtime which podman desktop provides. Are you using this OCP local for podman operations also and not consuming the podman-machine? |
Yeah let me try to be more specific here. We are building applications to integrate with an existing platform that gets delivered to us as a Helm chart + images. Can't get too specific but its about 10 different microservices. So our two big use cases are:
So the use case is not just running a different arch container on mac but being able to develop/test against a OCP based platform locally that is comprised of different arch containers. |
|
make cross fails with following error |
|
@cpepper96 can you update this PR and make sure the CI succeed? |
Hey @praveenkumar! Apologies for the late response but yes I can update this and make sure CI is passing |
|
@praveenkumar Updated. I think y'all will have to kick off that CI job |
|
/ok-to-test |
|
/retest |
1 similar comment
|
/retest |
|
Hi @praveenkumar, any thing I can do to move this PR along? |
One thing you need to do is squash all the commits to single one since it contains around 16 commits and mostly around syncing the branch or a normal fixup. |
Add the enable-rosetta configuration path and plumb it through preflight, machine configuration, vfkit, and startup so Apple Silicon hosts can use Rosetta for x86_64 container support.
|
[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 |
|
According to https://developer.apple.com/documentation/apple-silicon/about-the-rosetta-translation-environment/ looks like mac support for rosetta-2 is going to end when macOS-27 is released. |
|
https://developer.apple.com/documentation/apple-silicon/about-the-rosetta-translation-environment/ says
|
3 participants
Description
Fixes: #4881
Hi folks! This PR adds support for enabling Rosetta 2 on MacOS. The issue linked above does a great a job of describing the problem and solution.
TLDR: CRC currently uses QEMU on ARM64 MacOS to emulate x86 instructions. This has known limitations and some x86 containers are unable to run on CRC on Mac. Swapping Rosetta for QEMU should result in faster and more stable performance when trying to run x86 containers with CRC on Mac.
My team has been using this solution for the last couple of weeks with no issues except for one edge case. x86 binaries that use reexec() (e.g., buildah) have a known limitation under Rosetta due to kernel-level binfmt_misc + memfd interactions.
Type of change
test, version modification, documentation, etc.)
Proposed changes
use-rosettaconfig field to enable this featureTesting
crc config set use-rosetta trueContribution Checklist
Summary by CodeRabbit
New Features
Validation
Tests