[SREP-3695] feat: add a warning message if mac users run podman without rosetta#930
[SREP-3695] feat: add a warning message if mac users run podman without rosetta#930feichashao wants to merge 3 commits intoopenshift:mainfrom
Conversation
openshift-ci
Bot
added
the
tide/merge-method-squash
WalkthroughAdds a macOS (Apple Silicon) non-fatal Rosetta detection that runs a Changes
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` (inspect 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🎯 3 (Moderate) | ⏱️ ~20 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)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: feichashao 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 |
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.94% -0.07%
==========================================
Files 88 88
Lines 6662 6674 +12
==========================================
+ Hits 3598 3600 +2
- Misses 2596 2605 +9
- Partials 468 469 +1
🚀 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
|
@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. |
2 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