feat(runtime): add ContainerRuntime abstraction interface#281
Conversation
Replace scattered IsPodman()/IsNerdctl() runtime detection with a ContainerRuntime interface that expresses capabilities directly. Runtimes can now be explicitly configured via provider config or auto-detected from the binary with caching.
✅ Deploy Preview for devsydev canceled.
|
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR refactors container runtime detection and capability queries from scattered if-statements into a reusable ChangesContainer Runtime Abstraction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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. 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/docker/helper.go`:
- Around line 62-63: The GPUSupportEnabled method currently calls
GetRuntime().GPUAvailable with context.TODO(), so add a cancellable context with
a sensible timeout and pass it instead: create a context via context.WithTimeout
(e.g., 5s or configurable), defer cancel(), then call
r.GetRuntime().GPUAvailable(ctx, r) and return its result; ensure
GPUSupportEnabled no longer uses context.TODO() so the runtime call cannot hang
indefinitely.
In `@pkg/docker/runtime_test.go`:
- Around line 24-28: The subtest is being created with an empty name when
tt.input == "", producing unclear test output; update the t.Run call in the loop
that exercises RuntimeFromName so it uses a non-empty descriptive name (e.g.,
compute name := tt.input; if name == "" set name = "empty" or name =
fmt.Sprintf("input=%q", tt.input)) and call t.Run(name, func(t *testing.T) { ...
}) instead of t.Run(tt.input, ...).
In `@pkg/docker/runtime.go`:
- Around line 87-95: RuntimeFromName currently defaults any unknown non-empty
name to docker, which hides typos; change RuntimeFromName(name string) to return
(ContainerRuntime, error) and implement logic: if strings.TrimSpace(name) == ""
return dockerRuntime{}, nil; switch on RuntimeName(strings.ToLower(name)) { case
RuntimePodman: return podmanRuntime{}, nil; case RuntimeNerdctl: return
nerdctlRuntime{}, nil; default: return nil, fmt.Errorf("unknown container
runtime: %q", name) } and update all callers to handle the error; reference
RuntimeFromName, RuntimeName, RuntimePodman, RuntimeNerdctl, and dockerRuntime
when making these edits.
- Around line 118-120: The detect() probe must not block indefinitely and must
not run while holding the runtimeDetectionCache mutex; change the exec call to
use exec.CommandContext with a short timeout (e.g., a couple seconds) so the
--version call is bounded, and restructure runtimeDetectionCache.get usage so
you only hold the lock to read/update the cached value — if cache is empty,
release the lock, run detect() (with the CommandContext timeout), then
re-acquire the lock to store the result (or handle concurrent in-progress
detection accordingly). Ensure you reference the detect function and the
runtimeDetectionCache get/set paths when making the change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4638a232-b5e2-4095-8f69-bc0bbf1f6a64
📒 Files selected for processing (7)
pkg/docker/helper.gopkg/docker/runtime.gopkg/docker/runtime_test.gopkg/driver/docker/build.gopkg/driver/docker/docker.gopkg/provider/provider.goproviders/podman/provider.yaml
- Add 5s timeout to GPUSupportEnabled() instead of context.TODO() - Add 5s timeout to detect() runtime probe via exec.CommandContext - Restructure runtimeDetectionCache to release mutex during exec - Make RuntimeFromName return error for unknown runtime names - Fix empty subtest name in RuntimeFromName test table - Add test for unknown runtime name error case
Summary
ContainerRuntimeinterface (pkg/docker/runtime.go) that replaces scatteredIsPodman()/IsNerdctl()detection calls with capability-based queries (SupportsInternalBuildKit(),SupportsSignalProxy(),SupportsMountConsistency(),NeedsUserNamespaceArgs(),GPUAvailable())dockerRuntime,podmanRuntime, andnerdctlRuntimewith correct capability flagsRuntimeconfig field toProviderDockerDriverConfigfor explicit runtime declaration; auto-detects from binary when not set, with per-binary cachingpkg/docker/helper.go,pkg/driver/docker/docker.go, andpkg/driver/docker/build.goto use the new abstractionruntime: podmanconfigSummary by CodeRabbit
Release Notes
New Features
Improvements