[SREP-3695] feat: add a warning message if mac users run podman without rosetta#930
Conversation
openshift-ci
Bot
added
the
tide/merge-method-squash
|
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:
WalkthroughAdds a macOS (Apple Silicon) non-fatal Rosetta detection that runs ChangesRosetta detection + tests
Sequence DiagramsequenceDiagram
participant App as Application
participant Console as RunConsoleContainer
participant Check as checkRosettaEnabled
participant SSH as PodmanMachineSSH
participant Binfmt as binfmt_misc
App->>Console: Start console container (macOS/arm64)
Console->>Check: Invoke Rosetta check
Check->>SSH: Execute `podman machine ssh -- sudo ls -1 /proc/sys/fs/binfmt_misc`
SSH->>Binfmt: Query binfmt registrations
Binfmt-->>SSH: Return registrations
SSH-->>Check: Return output
alt contains "rosetta"
Check-->>Console: No warning (proceed)
else missing "rosetta"
Check->>Check: Log warning about missing Rosetta
Check-->>Console: Return (proceed)
end
Console->>Console: Start container process (original run)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/container/container_podman.go`:
- Around line 16-25: checkRosettaEnabled currently calls createCommand(PODMAN,
"machine", "ssh", ...) synchronously which can block startup and also runs on
non-Apple Silicon macOS; modify checkRosettaEnabled to first short-circuit
unless runtime.GOOS == "darwin" && runtime.GOARCH == "arm64", then execute the
podman ssh check with a cancellable context/timeout (use context.WithTimeout and
exec.CommandContext or a createCommandContext helper) so the command is killed
after a short deadline and cannot hang RunConsoleContainer; alternatively
refactor RunConsoleContainer to accept a context and use CommandContext there,
but ensure checkRosettaEnabled uses a context timeout and captures stdout/stderr
non-blockingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f12c2bb5-bc03-42e2-ad30-0290128a6bf8
📒 Files selected for processing (2)
pkg/container/container_podman.gopkg/container/container_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #930 +/- ##
==========================================
- Coverage 54.00% 53.96% -0.05%
==========================================
Files 88 82 -6
Lines 6662 6319 -343
==========================================
- Hits 3598 3410 -188
+ Misses 2596 2463 -133
+ Partials 468 446 -22
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/container/container_podman.go (1)
15-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRosetta preflight can still block startup, and the platform guard is incomplete.
Despite the “non-blocking” intent (Line 16), Line 28 runs
podman machine sshsynchronously with no timeout, so Line 111 can hangRunConsoleContainer()if the command stalls. Also, the check should gate on both macOS and arm64 to match the feature scope.Suggested fix
import ( "bytes" "encoding/base64" "fmt" "os" "path/filepath" "runtime" "strings" + "time" logger "github.com/sirupsen/logrus" ) func checkRosettaEnabled() { - // Rosetta is only relevant on Apple Silicon (arm64); skip on Intel Macs - if runtime.GOARCH != "arm64" { + // Rosetta is only relevant on macOS Apple Silicon. + if runtime.GOOS != MACOS || runtime.GOARCH != "arm64" { return } checkCmd := createCommand(PODMAN, "machine", "ssh", "ls /proc/sys/fs/binfmt_misc/") var out bytes.Buffer checkCmd.Stdout = &out checkCmd.Stderr = nil - if err := checkCmd.Run(); err != nil { - // Silently skip if we can't check - return - } + done := make(chan error, 1) + go func() { done <- checkCmd.Run() }() + + select { + case err := <-done: + if err != nil { + return + } + case <-time.After(3 * time.Second): + if checkCmd.Process != nil { + _ = checkCmd.Process.Kill() + } + return + } if !strings.Contains(out.String(), "rosetta") { logger.Warnf("Rosetta does not appear to be enabled in Podman. For better compatibility with x86_64 images on Apple Silicon, please configure Rosetta. See docs/macOS.md for setup instructions.") } }As per coding guidelines,
**/*.go: “Check for proper error handling” and “Focus on Go best practices and idiomatic patterns.”Also applies to: 109-112
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/container/container_podman.go` around lines 15 - 31, checkRosettaEnabled currently only checks runtime.GOARCH and runs a synchronous podman SSH command that can hang; change the guard to require both runtime.GOARCH == "arm64" and runtime.GOOS == "darwin" and run the check with a context timeout so it cannot block startup or RunConsoleContainer; create the command with a context (use exec.CommandContext or update createCommand to accept a context), set Stdout/ Stderr to buffers, handle context.DeadlineExceeded by returning (or logging non-fatal info) and avoid blocking on long-running Run(); ensure the function name checkRosettaEnabled and PODMAN/createCommand references are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/container/container_podman.go`:
- Around line 15-31: checkRosettaEnabled currently only checks runtime.GOARCH
and runs a synchronous podman SSH command that can hang; change the guard to
require both runtime.GOARCH == "arm64" and runtime.GOOS == "darwin" and run the
check with a context timeout so it cannot block startup or RunConsoleContainer;
create the command with a context (use exec.CommandContext or update
createCommand to accept a context), set Stdout/ Stderr to buffers, handle
context.DeadlineExceeded by returning (or logging non-fatal info) and avoid
blocking on long-running Run(); ensure the function name checkRosettaEnabled and
PODMAN/createCommand references are updated accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3f434bd7-85a6-4219-9562-3c2108803f53
📒 Files selected for processing (2)
pkg/container/container_podman.gopkg/container/container_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/container/container_test.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/container/container_podman.go (1)
15-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
checkRosettaEnabledis still blocking despite the non-blocking contract.Line 16 says non-blocking, but Line 28 uses synchronous
checkCmd.Run()with no timeout, soRunConsoleContainer()can still hang ifpodman machine sshstalls.Suggested fix (bounded wait, no indefinite hang)
import ( "bytes" "encoding/base64" "fmt" "os" "path/filepath" "runtime" "strings" + "time" logger "github.com/sirupsen/logrus" ) @@ - if err := checkCmd.Run(); err != nil { - // Silently skip if we can't check - return - } + done := make(chan error, 1) + go func() { done <- checkCmd.Run() }() + + select { + case err := <-done: + if err != nil { + // Silently skip if we can't check + return + } + case <-time.After(3 * time.Second): + if checkCmd.Process != nil { + _ = checkCmd.Process.Kill() + } + return + }#!/bin/bash set -euo pipefail # Verify current Rosetta check is synchronous and lacks timeout/cancellation. rg -n --type=go -C3 'func checkRosettaEnabled\(|checkCmd := createCommand\(|checkCmd\.Run\(\)|context\.WithTimeout|CommandContext'As per coding guidelines
**/*.go: "Check for proper error handling" and "Review security implications, especially for CLI tools."
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b1f3977d-5a89-4951-87ea-4ee6ec3676f2
📒 Files selected for processing (1)
pkg/container/container_podman.go
| // This is a non-blocking check that provides a hint to the user if Rosetta is not configured | ||
| func checkRosettaEnabled() { | ||
| // Rosetta is only relevant on Apple Silicon (arm64); skip on Intel Macs | ||
| if runtime.GOARCH != "arm64" { |
There was a problem hiding this comment.
Just a nit to confirm, is rosetta macOS/Apple specific? If so, should the guard be runtime.GOOS == "darwin" && runtime.GOARCH == "arm64" rather than only checking GOARCH == "arm64" ? Thinking it could avoid running the podman machine rosetta check on non-macOS arm64 env (it might be a rare case tho)
Other than that, LGTM.
There was a problem hiding this comment.
Hi Shawn, thank you for reviewing carefully! Yes, this is Apple specific.
This checkRosettaEnable() will only be called in the MacOS implementation, it won't be called in Linux.
https://github.com/feichashao/backplane-cli/blob/d3262ba2e161d11ba58f1b35ff6bb2ff5a87bf78/pkg/container/container_podman.go#L109
There was a problem hiding this comment.
Thanks Siu Wa, looks like checkRosettaEnabled() is a package-level function, not a method, is that scoped to podmanMac only or anyone in the package could potentially add a call from checkRosettaEnabled() in podmanRunConsoleContainer() ? (the shared function used by both Linux and macOS). If so, it would execute on Linux arm64 ?
Feeling adding the GOOS == "darwin" have very low cost but benefit for defense in depth? WDYT?
There was a problem hiding this comment.
Yes, adding this is low cost and provide another layer of guard. I will add and test, will add a new commit soon.
There was a problem hiding this comment.
Added a new commit and validated in my environment.
Without Rosetta:
~/git/backplane-cli/ocm-backplane console
WARN[0035] Rosetta does not appear to be enabled in Podman. For better compatibility with x86_64 images on Apple Silicon, please configure Rosetta. See https://github.com/openshift/backplane-cli/blob/main/docs/macOS.md for setup instructions.
Error: looking up container to share net namespace with: no container with name or ID "console-2q3gp5k5in41m7rb8kuidep76v2l48rc" found: no such container
With Rosetta:
~/git/backplane-cli/ocm-backplane console
== Console is available at http://127.0.0.1:63961 ==
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pkg/container/container_podman.go (1)
15-16:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThis "non-blocking" preflight still blocks container startup.
Line 28 runs
podman machine sshsynchronously beforeRunConsoleContainer()starts the container. If the VM or SSH path wedges, the macOS console flow hangs on the warning probe itself. This should either have a short timeout or move off the critical path. As per coding guidelines,**/*.go: Check for proper error handling.Also applies to: 28-30, 109-112
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/container/container_podman.go` around lines 15 - 16, The probe in checkRosettaEnabled that runs `podman machine ssh` synchronously (which runs before RunConsoleContainer) can block startup; change it to run off the critical path or use a short timeout and swallow non-fatal failures: refactor checkRosettaEnabled (and the similar probe calls in this file) to execute the SSH probe asynchronously (goroutine) or execute the command with a context with timeout, handle context deadline/errors gracefully, and ensure RunConsoleContainer is started regardless of probe outcome while still logging the warning when the probe succeeds/fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/container/container_podman.go`:
- Around line 23-30: The remote ssh probe in container_podman.go passes the
whole command as a single argv token, causing Podman to treat it as the
executable; update the checkCmd invocation (where checkCmd :=
createCommand(PODMAN, "machine", "ssh", "ls /proc/sys/fs/binfmt_misc/") is
created) to pass the remote command and its arguments as separate tokens (e.g.,
"ls", "/proc/sys/fs/binfmt_misc/") so Podman runs the intended command on the
remote machine; leave the rest of the check (Stdout buffer, Stderr handling, Run
error handling) unchanged.
In `@pkg/container/container_test.go`:
- Line 106: Tests in container_test.go use os.Getenv("GOARCH") together with
runtime.GOOS to guard platform-specific cases; change those guards to use only
runtime.GOARCH so they match production logic in checkRosettaEnabled() and don’t
depend on CI env vars. Locate the guard expressions that currently include
os.Getenv("GOARCH") (e.g., the if conditions using runtime.GOOS == "darwin" &&
(os.Getenv("GOARCH") == "arm64" || ...)) and remove the os.Getenv branches,
replacing them with checks against runtime.GOARCH alone.
---
Duplicate comments:
In `@pkg/container/container_podman.go`:
- Around line 15-16: The probe in checkRosettaEnabled that runs `podman machine
ssh` synchronously (which runs before RunConsoleContainer) can block startup;
change it to run off the critical path or use a short timeout and swallow
non-fatal failures: refactor checkRosettaEnabled (and the similar probe calls in
this file) to execute the SSH probe asynchronously (goroutine) or execute the
command with a context with timeout, handle context deadline/errors gracefully,
and ensure RunConsoleContainer is started regardless of probe outcome while
still logging the warning when the probe succeeds/fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4605f048-fa1d-4134-9a3a-03ea1e274441
📒 Files selected for processing (2)
pkg/container/container_podman.gopkg/container/container_test.go
| checkCmd := createCommand(PODMAN, "machine", "ssh", "ls /proc/sys/fs/binfmt_misc/") | ||
| var out bytes.Buffer | ||
| checkCmd.Stdout = &out | ||
| checkCmd.Stderr = nil | ||
|
|
||
| if err := checkCmd.Run(); err != nil { | ||
| // Silently skip if we can't check | ||
| return |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify the current argv shape in the code.
rg -n -C2 'createCommand\(PODMAN,\s*"machine",\s*"ssh"' pkg/container/container_podman.go
# Verify Podman docs for podman machine ssh syntax and examples.
curl -fsSL https://docs.podman.io/en/latest/markdown/podman-machine-ssh.1.html | sed -n '/SYNOPSIS/,/DESCRIPTION/p'
curl -fsSL https://docs.podman.io/en/latest/markdown/podman-machine-ssh.1.html | sed -n '/EXAMPLES/,/SEE ALSO/p'Repository: openshift/backplane-cli
Length of output: 1413
Split the remote ssh command into separate argv tokens.
podman machine ssh is documented as [name] [command [arg …]], and the official examples pass the remote command and its arguments as separate tokens. Passing "ls /proc/sys/fs/binfmt_misc/" as one token will cause Podman to treat the entire string as the executable name, making this probe fail and silently skip the warning.
Suggested fix
- checkCmd := createCommand(PODMAN, "machine", "ssh", "ls /proc/sys/fs/binfmt_misc/")
+ checkCmd := createCommand(PODMAN, "machine", "ssh", "ls", "/proc/sys/fs/binfmt_misc/")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/container/container_podman.go` around lines 23 - 30, The remote ssh probe
in container_podman.go passes the whole command as a single argv token, causing
Podman to treat it as the executable; update the checkCmd invocation (where
checkCmd := createCommand(PODMAN, "machine", "ssh", "ls
/proc/sys/fs/binfmt_misc/") is created) to pass the remote command and its
arguments as separate tokens (e.g., "ls", "/proc/sys/fs/binfmt_misc/") so Podman
runs the intended command on the remote machine; leave the rest of the check
(Stdout buffer, Stderr handling, Run error handling) unchanged.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/container/container_test.go`:
- Around line 104-118: The tests only assert that checkRosettaEnabled invoked
podman but don't exercise the warning/no-warning branches; make the command
creation/output behavior injectable so tests can deterministically simulate
stdout containing "rosetta" and not containing it. Refactor
pkg/container/container_podman.go to expose or accept a createCommand factory
(or a package-level variable used by checkRosettaEnabled) that tests can
replace; in container_test.go set that factory to stubs that return controlled
stdout (one with "rosetta" and one without) and assert the expected warning
emission (and absence) from checkRosettaEnabled, covering both paths regardless
of runtime.GOOS/GOARCH.
- Around line 149-157: The test currently only asserts capturedCommands has at
least two entries but not that there is exactly one Rosetta preflight; update
the assertion to count occurrences of the Rosetta check (commands where
capturedCommands[i][0] == PODMAN && capturedCommands[i][1] == "machine" &&
capturedCommands[i][2] == "ssh") and assert that this count equals 1 instead of
using BeNumerically(">=", 2); keep the existing check that the last command is
the container run (runCmd := capturedCommands[len(capturedCommands)-1]) and use
the counted value to tighten the test for RunConsoleContainer()/capturedCommands
and rosettaCheckCmd validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 798c57a5-7312-4516-8fc2-31e64fa40dbb
📒 Files selected for processing (1)
pkg/container/container_test.go
| Context("when checking Rosetta on macOS Podman", func() { | ||
| It("should execute podman machine ssh command on darwin/arm64", func() { | ||
| if runtime.GOOS == "darwin" && runtime.GOARCH == "arm64" { | ||
| capturedCommands = nil | ||
| checkRosettaEnabled() | ||
| Expect(len(capturedCommands)).To(Equal(1)) | ||
| command := capturedCommands[0] | ||
| Expect(command[0]).To(Equal(PODMAN)) | ||
| Expect(command[1]).To(Equal("machine")) | ||
| Expect(command[2]).To(Equal("ssh")) | ||
| Expect(strings.Join(command[3:], " ")).To(Equal("ls /proc/sys/fs/binfmt_misc/")) | ||
| } else { | ||
| Skip("Rosetta check only runs on darwin/arm64") | ||
| } | ||
| }) |
There was a problem hiding this comment.
Exercise the warning path, not just the podman machine ssh argv.
These specs prove that checkRosettaEnabled() shells out on the current host, but they still do not cover the user-visible behavior added in pkg/container/container_podman.go:17-36: warning when stdout lacks rosetta, and no warning when it is present. With createCommand hardwired to exec.Command("true"), that branch can regress without any test here failing. Consider making the platform/output decision injectable so both warning and no-warning cases run deterministically on any runner.
As per coding guidelines, **/*_test.go: "Review test coverage and edge cases" and "Ensure tests are deterministic and not flaky".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/container/container_test.go` around lines 104 - 118, The tests only
assert that checkRosettaEnabled invoked podman but don't exercise the
warning/no-warning branches; make the command creation/output behavior
injectable so tests can deterministically simulate stdout containing "rosetta"
and not containing it. Refactor pkg/container/container_podman.go to expose or
accept a createCommand factory (or a package-level variable used by
checkRosettaEnabled) that tests can replace; in container_test.go set that
factory to stubs that return controlled stdout (one with "rosetta" and one
without) and assert the expected warning emission (and absence) from
checkRosettaEnabled, covering both paths regardless of runtime.GOOS/GOARCH.
There was a problem hiding this comment.
too heavy, skip this.
There was a problem hiding this comment.
@feichashao, understood — acknowledged and moving on. The current tests still validate that the correct podman machine ssh command is invoked, which is a reasonable coverage level for this change.
|
@feichashao: all tests passed! 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: feichashao, xiaoyu74 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
What type of PR is this?
What this PR does / Why we need it?
This PR adds a warning message to the mac users who run podman without rosetta.
We encountered compatibility issues where podman container exits with seg fault if running an x86_64 image on mac M1/M2/M3. Enabling rosetta can solve the compatibility issue.
Which Jira/Github issue(s) does this PR fix?
https://redhat.atlassian.net/browse/SREP-3695
Special notes for your reviewer
Unit Test Coverage
Guidelines
Test coverage checks
Pre-checks (if applicable)
/label tide/merge-method-squash
Summary by CodeRabbit
New Features
Tests