You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
signal: killed is Go's default error when exec.Cmd.Cancel kills a process on context cancellation -- never something podman itself emits.
Ping (pkg/docker/helper.go) builds its podman info command with a hard-coded context.WithTimeout(ctx, 10*time.Second), and the failing run's surrounding log span reports elapsed=10.156166312s -- almost exactly that deadline.
podman info was therefore still running, not refused, when Devsy killed it and declared the daemon down.
Ping predates runCmd (added in fix: disambiguate signal-killed errors from context cancellation #896 specifically to disambiguate a self-inflicted context-cancel kill from a genuine command failure) and was never migrated onto it, so runPreflight had no way to tell "we gave up early" apart from "the daemon said no".
Confirmed by reproducing the exact pre-fix code path in isolation: a raw buildCmd(...).CombinedOutput() on a killed process returns bare err = "signal: killed" with no context.DeadlineExceeded attached -- reproducing the CI failure message verbatim.
Existing CI mitigations (--ginkgo.flake-attempts=2, dpkg-lock wait, job/test timeout bumps from #1049) target installation-time races and whole-spec timeouts; none touch this in-process 10s ping deadline, so retrying just re-rolls the same race under sustained CI resource contention.
Fix
Ping, StartPodmanMachine, and the systemctl --user start podman.socket call in StartRootlessPodmanSocket now route through runCmd (via a new runCmdCombined helper), so a self-inflicted timeout kill is wrapped with ctx.Err() and errors.Is(err, context.DeadlineExceeded) works.
pingTimeout raised from a hard-coded 10s to 30s (still well under the 90s Podman-machine-boot budget).
runPreflight now reports "daemon did not respond in time (it may just be slow to start, not necessarily down)" when the ping error carries context.DeadlineExceeded, instead of unconditionally claiming the daemon "is not reachable".
Testing
New regression tests in pkg/docker/helper_test.go and pkg/driver/docker/preflight_test.go force the internal deadlines via overridable package vars and assert errors.Is(err, context.DeadlineExceeded), plus assert a fast native refusal is not misattributed to a timeout.
All existing TestRunPreflight* table tests pass unchanged.
go build, gofmt -l, and go test ./pkg/docker/... ./pkg/driver/docker/... all pass.
Summary by CodeRabbit
Bug Fixes
Improved Docker and Podman startup and connectivity handling.
Extended Docker ping detection to allow more time for slow-starting daemons.
Error messages now distinguish between unreachable daemons and daemons that do not respond in time.
Preserved timeout details to provide clearer diagnostics when startup commands exceed their limits.
…usal
Ping killed a still-running `podman info` at its own hard-coded 10s
deadline under CI runner CPU/IO contention (CI run 32065209141,
up-provider-podman-rootless-exec), then reported the bare "signal:
killed" from that kill as an unqualified "daemon is not reachable" --
indistinguishable from a genuine refusal. This masqueraded as test
flakiness; --ginkgo.flake-attempts=2 just re-rolled the same race.
Ping, StartPodmanMachine, and the systemctl call in
StartRootlessPodmanSocket now route through runCmd (via a new
runCmdCombined helper) so a kill caused by our own context deadline is
wrapped with ctx.Err(), and pingTimeout is raised from 10s to 30s.
runPreflight reports a distinct 'did not respond in time' message when
the ping error carries context.DeadlineExceeded, instead of always
asserting the daemon is down.
No actionable comments were generated in the recent review. 🎉
ℹ️ Recent review info⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e03c4f68-3334-4faa-b4fb-de212ded77be
📥 Commits
Reviewing files that changed from the base of the PR and between 31f2112 and cb1bd09.
📒 Files selected for processing (4)
pkg/docker/helper.go
pkg/docker/helper_test.go
pkg/driver/docker/docker.go
pkg/driver/docker/preflight_test.go
Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review.
📝 Walkthrough
Walkthrough
The change adds shared command output handling, increases the ping timeout to 30 seconds, makes Podman startup timeouts configurable, and distinguishes Docker daemon timeout errors from other preflight failures.
Changes
Timeout error handling
Layer / File(s)
Summary
Shared command execution and timeout coverage pkg/docker/helper.go, pkg/docker/helper_test.go
runCmdCombined now captures command output and includes it in execution errors. Ping, Podman machine startup, and rootless Podman socket startup use the helper. Tests cover successful commands, refusal errors, and command timeouts.
Docker preflight now reports deadline failures as non-responsive daemon errors and preserves the generic unreachable-daemon message for other failures. Tests verify both classifications.
This localized change distinguishes slow daemon startup from genuine refusal and extends the ping timeout, with regression coverage and passing checks; no actionable merge-blocking risk remains.
AI Reviewer: run a review on demand. To trigger the first review automatically, go to your organization or repository integration settings. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root cause
up-provider-podman-rootless-exec(and its rootful/rootless siblings) intermittently fail with:CI run: https://github.com/devsy-org/devsy/actions/runs/32065209141
This is not a genuinely unreachable daemon:
signal: killedis Go's default error whenexec.Cmd.Cancelkills a process on context cancellation -- never something podman itself emits.Ping(pkg/docker/helper.go) builds itspodman infocommand with a hard-codedcontext.WithTimeout(ctx, 10*time.Second), and the failing run's surrounding log span reportselapsed=10.156166312s-- almost exactly that deadline.podman infowas therefore still running, not refused, when Devsy killed it and declared the daemon down.PingpredatesrunCmd(added in fix: disambiguate signal-killed errors from context cancellation #896 specifically to disambiguate a self-inflicted context-cancel kill from a genuine command failure) and was never migrated onto it, sorunPreflighthad no way to tell "we gave up early" apart from "the daemon said no".Confirmed by reproducing the exact pre-fix code path in isolation: a raw
buildCmd(...).CombinedOutput()on a killed process returns bareerr = "signal: killed"with nocontext.DeadlineExceededattached -- reproducing the CI failure message verbatim.Existing CI mitigations (
--ginkgo.flake-attempts=2, dpkg-lock wait, job/test timeout bumps from #1049) target installation-time races and whole-spec timeouts; none touch this in-process 10s ping deadline, so retrying just re-rolls the same race under sustained CI resource contention.Fix
Ping,StartPodmanMachine, and thesystemctl --user start podman.socketcall inStartRootlessPodmanSocketnow route throughrunCmd(via a newrunCmdCombinedhelper), so a self-inflicted timeout kill is wrapped withctx.Err()anderrors.Is(err, context.DeadlineExceeded)works.pingTimeoutraised from a hard-coded 10s to 30s (still well under the 90s Podman-machine-boot budget).runPreflightnow reports "daemon did not respond in time (it may just be slow to start, not necessarily down)" when the ping error carriescontext.DeadlineExceeded, instead of unconditionally claiming the daemon "is not reachable".Testing
pkg/docker/helper_test.goandpkg/driver/docker/preflight_test.goforce the internal deadlines via overridable package vars and asserterrors.Is(err, context.DeadlineExceeded), plus assert a fast native refusal is not misattributed to a timeout.TestRunPreflight*table tests pass unchanged.go build,gofmt -l, andgo test ./pkg/docker/... ./pkg/driver/docker/...all pass.Summary by CodeRabbit