diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 03c4bb55..b8e1efd6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -31,7 +31,22 @@ jobs: # It is deliberately NOT the measured 78.96%: a floor set exactly at # today's number turns any unrelated refactor that moves one region into a # failed build. - coverage-threshold: 75 + # 79, not the org standard's 90, and not because podup is short of it. + # + # Measured 2026-08-03: the full suite covers **91.52%** of lines. This job + # cannot run it — the integration tests need a live Podman and skip + # themselves on a runner without one — so what it measures is `--lib + # --bins` alone, which is **79.39%**. The two numbers describe different + # things and only one of them is checkable here. + # + # 75 left four points of silent slack on a figure that is already a + # subset: coverage could fall from 79 to 75 with nothing saying so. This + # locks in what holds today rather than demanding new work, which is what + # the testing standard asks for when adopting a floor. + # + # The 90 gate belongs where the tests can actually run — the `podman-vm` + # lane — and that is tracked separately rather than papered over here. + coverage-threshold: 79 msrv: "1.85" package-check: true semver-check: true diff --git a/.github/workflows/podman-lane.yml b/.github/workflows/podman-lane.yml index 89a8c1c9..b6828798 100644 --- a/.github/workflows/podman-lane.yml +++ b/.github/workflows/podman-lane.yml @@ -23,7 +23,16 @@ jobs: runs-on: ubuntu-24.04 # Raised for the one-thread experiment: serialising the suite costs wall # clock, and a run that dies on the timeout answers nothing (#1039). - timeout-minutes: 75 + # + # Raised again for the coverage path. Capping the coverage run to the same + # thread count as the gating run (#1338) means the Podman 6 leg now runs two + # serialised suites, and it came in at 66m27s of the 75 on run 30879785232 — + # 89% of the budget. That is not a failure yet, which is the only useful + # moment to change it: a slower runner or a heavier rawhide image tips it + # over, and the run that dies tells you nothing about the coverage it was + # measuring. Costs nothing on the pull-request path, where coverage is off + # and the leg finishes in about half this. + timeout-minutes: 95 strategy: fail-fast: false matrix: @@ -95,6 +104,15 @@ jobs: # flawless 155-pass run and clear the floor comfortably. export PODUP_REQUIRE_PODMAN=1 cd "$HOME/podup" || exit 90 + # Coverage runs on the scheduled and manual paths only — never on a + # pull request. The number is worth having where the integration + # tests can actually run (#1326: the CI coverage job sees 79% because + # every integration test skips itself without Podman, while the full + # suite covers 91.5%), but instrumenting doubles nothing here for + # free: it is a second full build inside the VM. Gating on it is + # deliberately NOT done yet — a threshold set from a number never + # observed in this environment is how a phantom check gets born. + COVERAGE=__COVERAGE__ # Capture the FULL output: the per-failure detail (where the flake # marker lives) precedes the summary, so a `tail` on the console # would truncate it and hide why a test failed. @@ -129,11 +147,18 @@ jobs: # this hypothesis dies too and the search moves elsewhere. # # `--test-threads` caps how many streaming tests hit the libpod - # socket at once. The nested-virt (VM-in-runner) transport is what - # drops connections mid-stream — measured: on a single-level VM - # (Podman 5.8.1 and 6.0.1) the same suite never drops a byte, so the - # flake is the double-virt socket under concurrent load, not a podup - # or libpod bug. Fewer concurrent streams means less peak pressure, + # socket at once. + # + # This used to say the drops were specific to the nested-virt + # (VM-in-runner) transport, and that a single-level VM "never + # drops a byte". **That is measured false** (2026-08-03, #1322): + # on a plain KVM guest with Podman 6.0.1, the same suite fails + # 17 of 27 at eight test threads and 3 at one — a clean + # dose-response curve in thread count, reproducible across + # repetitions. Nesting makes it worse; it does not cause it. + # + # What holds is the lever: fewer concurrent streams means less + # peak pressure, # which is the lever to shrink Podman 6's variable flaky tail toward # a small, stable set that an identity list can gate (#1039). Kept at # 2 rather than 1 so the suite still fits the VM's time budget. @@ -156,6 +181,67 @@ jobs: FLAKY=$(grep -cE "$DROPPED" /tmp/cargo.log) echo "PODUP_TESTS_RC=$RC" echo "PODUP_SUMMARY pass=${PASS:-0} fail=${FAIL:-0} flaky=${FLAKY:-0}" + # Coverage, measured where the integration tests can actually run. + # Reported, never gated — see the note where COVERAGE is set. A + # failure here must not redden the leg: this is an observation, and + # the suite's own result above is what gates. + if [ "$COVERAGE" = "1" ]; then + # Nothing here redirects to /dev/console. This script runs as + # `tester`, which may not open that device, and a denied + # redirection aborts the command it was attached to and is + # reported as that command's own failure. The first run said + # `install-failed` for exactly that reason: cargo never ran. + # stdout and stderr already point at the console, inherited + # from the root caller that launched this script. + # cargo-llvm-cov looks for the `llvm-tools-preview` rustup + # component, and there is no rustup here — Rust comes from dnf. + # It accepts the two tools directly instead, and Fedora's `llvm` + # package (already installed above) carries both. Taking them + # from the distribution is also what keeps the versions in step: + # rustc and llvm are built for the same Fedora release, so the + # profile format matches, which a separately-pinned LLVM would + # not guarantee. + # + # Exactly ONE PODUP_COVERAGE line is emitted on every path. Two + # would be worse than none: whoever greps the log gets an answer + # either way and no reason to suspect there was another. + export LLVM_COV=/usr/bin/llvm-cov + export LLVM_PROFDATA=/usr/bin/llvm-profdata + # Print both versions unconditionally. A profile-format mismatch + # is the next thing likely to go wrong here, and it is only + # diagnosable against the pair of numbers that produced it. + echo "llvm tools: $("$LLVM_COV" --version 2>&1 | grep -i version | head -1)" + echo "rustc llvm: $(rustc --version --verbose | grep -i '^LLVM')" + if [ ! -x "$LLVM_COV" ] || [ ! -x "$LLVM_PROFDATA" ]; then + # Say so here rather than letting cargo-llvm-cov fail with its + # own wording several minutes and one full build later. + echo "PODUP_COVERAGE=llvm-tools-missing" + elif cargo install cargo-llvm-cov --locked; then + # Same thread cap as the gating run above. cargo-llvm-cov + # drives its own `cargo test` and does NOT inherit it, which + # is measurable rather than theoretical: on the Podman 6 leg + # the gating run took 1743s and passed 178/178, and the + # coverage run of the same code in the same boot took 441s — + # four times faster because it was parallel — and failed 5 + # with hyper IncompleteMessage. Uncapped, this step measures + # a different suite than the one that gates. + cargo llvm-cov --all-features --summary-only \ + -- --test-threads=__THREADS__ >/tmp/cov.log 2>&1 + PCT=$(awk '$1=="TOTAL" { for (i=1;i<=NF;i++) if ($i ~ /%$/) { print $i; exit } }' /tmp/cov.log) + if [ -n "$PCT" ]; then + echo "PODUP_COVERAGE=$PCT" + else + # Ran, produced nothing parseable. Print what it actually + # said: a second 15-minute boot is an expensive way to + # find out, and each distinct marker below names one + # cause instead of collapsing every failure into one word. + echo "PODUP_COVERAGE=no-total-line" + tail -30 /tmp/cov.log + fi + else + echo "PODUP_COVERAGE=install-failed" + fi + fi # Emit the IDENTITIES of the failing tests, not just the count. # The floor gate can only tighten to a per-test known-flaky # allowlist once the flaky set is small and stable (#1039), and @@ -169,7 +255,7 @@ jobs: echo "PODUP_FAILED_TESTS=${FAILED_TESTS}" runcmd: - bash -c 'F=$(findmnt -no FSTYPE /); case $F in btrfs) btrfs filesystem resize max / ;; xfs) xfs_growfs / ;; ext4) resize2fs $(findmnt -no SOURCE /) ;; esac >/dev/console 2>&1; echo "DISK=$(df -h / | tail -1)" >/dev/console' - - bash -c 'dnf install -y podman rust cargo gcc git >/dev/console 2>&1' + - bash -c 'dnf install -y podman rust cargo gcc git llvm >/dev/console 2>&1' # Prove the driver took, so a silently-ignored config cannot put the # logs tests back to passing without reading anything. - bash -c 'echo "LOGDRIVER=$(podman info --format {{.Host.LogDriver}} 2>/dev/null)" >/dev/console' @@ -186,6 +272,13 @@ jobs: [ "${{ matrix.podman }}" = "6" ] && THREADS=1 echo "test-threads for Podman ${{ matrix.podman }}: $THREADS" sed -i "s/__THREADS__/$THREADS/" "$RUNNER_TEMP/user-data" + # Coverage on the scheduled and manual paths only. A pull request must + # not pay for a second instrumented build, and must not be able to fail + # for a reason that has nothing to do with its own change. + COVERAGE=0 + case "${{ github.event_name }}" in schedule|workflow_dispatch) COVERAGE=1 ;; esac + echo "coverage pass for Podman ${{ matrix.podman }}: $COVERAGE" + sed -i "s/__COVERAGE__/$COVERAGE/" "$RUNNER_TEMP/user-data" cloud-localds "$RUNNER_TEMP/seed.iso" "$RUNNER_TEMP/user-data" - name: Boot VM (9p = repo only) + capture run: | @@ -228,6 +321,16 @@ jobs: # log (the console here is tail-truncated, so don't recount from it). SUM=$(grep -oE 'PODUP_SUMMARY pass=[0-9]+ fail=[0-9]+ flaky=[0-9]+' "$L" | tail -1) [ -n "$SUM" ] || { echo "::error::no PODUP_SUMMARY from VM on Podman $VER — suite did not report"; exit 1; } + # Coverage, when this run asked for it. Reported into the job summary and + # never gated: the org standard's 90% floor belongs here rather than on + # the Podman-less job that can only see 79% (#1326), but a threshold set + # from a number never observed in THIS environment would be a phantom + # check. Observe first, then gate, in a separate change. + COV=$(grep -oE 'PODUP_COVERAGE=[^ ]+' "$L" | tail -1 | cut -d= -f2) + if [ -n "$COV" ]; then + echo "coverage on Podman $VER (full suite, integration tests included): $COV" + echo "- **Podman $VER coverage:** $COV" >> "$GITHUB_STEP_SUMMARY" + fi PASS=$(echo "$SUM" | grep -oE 'pass=[0-9]+' | cut -d= -f2) FAIL=$(echo "$SUM" | grep -oE 'fail=[0-9]+' | cut -d= -f2) FLAKY=$(echo "$SUM" | grep -oE 'flaky=[0-9]+' | cut -d= -f2) diff --git a/Cargo.lock b/Cargo.lock index fd7af1e5..f66ea496 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -124,9 +124,9 @@ checksum = "9330f8b2ff13f34540b44e946ef35111825727b38d33286ef986142615121801" [[package]] name = "clap" -version = "4.6.4" +version = "4.6.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d91e0c145792ef73a6ad36d27c75ac09f1832222a3c209689d90f534685ee5b7" +checksum = "301b56658598e48f3648647ac6fc887be7e7108eddfa4e9b63fcf3ec58c0cadf" dependencies = [ "clap_builder", "clap_derive", @@ -134,9 +134,9 @@ dependencies = [ [[package]] name = "clap_builder" -version = "4.6.2" +version = "4.6.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f09628afdcc538b57f3c6341e9c8e9970f18e4a481690a64974d7023bd33548b" +checksum = "94a65403d1a1bd28f7dc68eb8506e8874808ee5eecb59298de588e2e1407a078" dependencies = [ "anstream", "anstyle", @@ -714,7 +714,7 @@ dependencies = [ [[package]] name = "podup" -version = "3.6.0" +version = "3.6.1" dependencies = [ "anstream", "anstyle", diff --git a/Cargo.toml b/Cargo.toml index 9afb84de..844ff477 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "podup" -version = "3.6.0" +version = "3.6.1" edition = "2021" rust-version = "1.85" description = "Translate and run docker-compose files on rootless Podman" diff --git a/debian/changelog b/debian/changelog index 3dbfe06c..9b17e356 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,3 +1,22 @@ +podup (3.6.1) unstable; urgency=medium + + Fixed + + * A lifecycle operation whose response the daemon drops is no longer reported + as a failure when it actually completed. `restart`, `start`, `kill`, `stop` + and `rm` now confirm the container reached the state the command was for, + and only then report success; a container that did not reach it, or a state + that cannot be re-read, still fails. Podman 6 severs these responses under + concurrency, which made `up`, `down` and `restart` fail on multi-service + projects for a reason that was never the command. + * `exec` retries once when the daemon drops the response to its session + create. That request is where the drops land most, and it cannot be resolved + by re-checking the container, because what is lost is the session id. + Retrying is safe: an exec that is created and never started allocates no + process and is discarded with the container. + + -- Jaro-c <75870284+Jaro-c@users.noreply.github.com> Tue, 04 Aug 2026 04:35:55 -0500 + podup (3.6.0) unstable; urgency=medium Incompatible diff --git a/internal/engine/fake_podman.rs b/internal/engine/fake_podman.rs index 74135907..3595a47d 100644 --- a/internal/engine/fake_podman.rs +++ b/internal/engine/fake_podman.rs @@ -42,6 +42,19 @@ pub(super) enum FakeReply { /// closes. The other place a severed stream can land, and — measured — hyper /// classifies the two differently, which is why both exist here. ChunkedCutMidPayload(String), + /// The request is read and accepted, and then the connection closes with **no + /// response at all** — not even a status line. + /// + /// This is the shape `PodmanError::is_incomplete_message` names: hyper's + /// `IncompleteMessage` is about the message *head*, so it is the one reply + /// here that produces it, and the severed-body variants above do not. + /// + /// It is what libpod does on Podman 6 to the container-archive PUT (#1097, + /// applies the archive then hangs up) and to state-changing POSTs under + /// concurrency (#1339). Both are handled by re-checking the observable out of + /// band, and until this existed neither discriminator had a test that could + /// reach it. + ClosedWithoutResponse, } /// A test's routing rule: `(method, target) -> reply`, where `target` is the @@ -181,6 +194,9 @@ async fn serve_one( .await?; stream.flush().await?; } + FakeReply::ClosedWithoutResponse => { + // Write nothing. The shutdown below is the entire reply. + } } stream.shutdown().await?; Ok(()) diff --git a/internal/engine/lifecycle/commands.rs b/internal/engine/lifecycle/commands.rs index 520b07e0..6503152d 100644 --- a/internal/engine/lifecycle/commands.rs +++ b/internal/engine/lifecycle/commands.rs @@ -3,6 +3,7 @@ use crate::compose::types::ComposeFile; use crate::error::{ComposeError, Result}; +use super::drop_recheck::LifecycleGoal; use super::filter_services; use super::parallel::{ filter_levels, first_error, join_bounded, restart_service_set, retain_levels, @@ -81,6 +82,7 @@ impl Engine { path: &str, container: &str, done: &str, + goal: LifecycleGoal, ) -> Result { match self.client.post_empty_ok(path).await { Ok(()) => { @@ -91,6 +93,21 @@ impl Engine { tracing::debug!("{container}: {done} skipped ({e})"); Ok(false) } + // The server closed before completing the response. That is not an + // answer: the operation may have run to completion and lost only its + // reply. Measured on Podman 6 under concurrency, where the drops land + // on exactly these state-changing POSTs and follow a slow one — a + // restart that burned its full stop grace, then a drop on the next + // (#1339). It is not a client deadline (READ_TIMEOUT is 120s) and not + // a pooled-connection race (there is no pool; every request gets a + // fresh socket), so the transport genuinely cannot say. + // + // Resolve it the way `cp` and `stats` already do: ask the observable + // the transport cannot see. If the container reached the state the + // operation was for, it succeeded. + Err(e) if e.is_incomplete_message() => { + self.confirm_lost_response(container, done, goal, e).await + } Err(e) => Err(ComposeError::Podman(e)), } } @@ -148,6 +165,13 @@ impl Engine { tracing::debug!("{container}: stop skipped ({e})"); Ok(()) } + // `stop` is one of the four state-changing calls the drops were + // measured on (#1339), and it does not go through `run_lifecycle_op`, + // so it needs the re-check on its own. + Err(e) if e.is_incomplete_message() => self + .confirm_lost_response(container, "Stopped", LifecycleGoal::NotRunning, e) + .await + .map(|_| ()), Err(e) if e.is_timeout() => { tracing::warn!( "{container}: stop did not complete within the grace window; escalating to SIGKILL" diff --git a/internal/engine/lifecycle/drop_recheck.rs b/internal/engine/lifecycle/drop_recheck.rs new file mode 100644 index 00000000..4b095c2b --- /dev/null +++ b/internal/engine/lifecycle/drop_recheck.rs @@ -0,0 +1,121 @@ +//! Deciding whether an operation whose response was dropped actually landed. +//! +//! The transport cannot answer it. A libpod call that is severed before its +//! response completes looks identical whether the operation ran or not (#1104), +//! and on Podman 6 under concurrency that happens on exactly the state-changing +//! calls — `exec`, `restart`, `stop`, container `DELETE` — after a slow one +//! (#1339). It is not a client deadline and not a pooled-connection race; both +//! were ruled out by measurement. +//! +//! So it is answered the way `cp` and `stats` already answer theirs: by asking +//! the observable the transport cannot see. + +use crate::error::{ComposeError, Result}; + +use crate::engine::Engine; +use crate::libpod::API_PREFIX; + +/// What a lifecycle operation was trying to achieve. +/// +/// The transport cannot say whether a dropped response means the operation +/// failed or completed and lost only its reply — the two are indistinguishable +/// at HTTP (#1104). This names the observable that answers it out of band. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(super) enum LifecycleGoal { + /// `start`, `restart` — the container should be running afterwards. + Running, + /// `kill`, `stop` — the container should not be running afterwards. + NotRunning, + /// `rm` — the container should not exist afterwards. Distinct from + /// [`Self::NotRunning`]: a stopped-but-present container satisfies that one + /// and would read a failed removal as a success. + Gone, +} + +impl LifecycleGoal { + /// Whether libpod's `State` satisfies this goal. `None` means the container + /// no longer exists — which reaches `NotRunning` and `Gone`, and fails + /// `Running`. + pub(super) fn reached(self, state: Option<&str>) -> bool { + match self { + Self::Running => state == Some("running"), + Self::NotRunning => state != Some("running"), + Self::Gone => state.is_none(), + } + } +} + +impl Engine { + /// Decide whether an operation whose response was dropped actually landed. + /// + /// Shared by every state-changing call that can lose its reply, so they + /// cannot drift into different answers to the same question — the way the + /// lane's retry list and its flake counter drifted apart in #1104. + /// + /// Success requires the container to have reached `goal`. Both other shapes + /// fail closed: a container that did not reach it, and a re-check that could + /// not be read. Neither is confirmation, and reporting success without one is + /// the failure this exists to prevent. + pub(super) async fn confirm_lost_response( + &self, + container: &str, + done: &str, + goal: LifecycleGoal, + e: crate::libpod::PodmanError, + ) -> Result { + match self.container_state(container).await { + Ok(state) if goal.reached(state.as_deref()) => { + tracing::warn!( + "{container}: {done} lost its response [{}] but the container reached \ + {goal:?}, so the operation landed", + e.stream_end_kind() + ); + crate::ui::progress_line("Container", container, done); + Ok(true) + } + Ok(state) => { + tracing::warn!( + "{container}: {done} lost its response [{}] and the container is \ + {state:?}, not {goal:?}", + e.stream_end_kind() + ); + Err(ComposeError::Podman(e)) + } + Err(recheck) => { + tracing::warn!( + "{container}: {done} lost its response [{}] and the state could not be \ + re-checked: {recheck}", + e.stream_end_kind() + ); + Err(ComposeError::Podman(e)) + } + } + } + + /// libpod's `State` for one container, or `None` when it no longer exists. + /// + /// Listed rather than inspected: the list carries `State` directly and the + /// inspect response is several times the size for the one field wanted here + /// (#1298). libpod's `name` filter matches on substring, so the exact name is + /// picked out of the results rather than trusted from the query. + pub(super) async fn container_state(&self, container: &str) -> Result> { + let filters = serde_json::json!({ "name": [container] }); + let path = format!( + "{API_PREFIX}/containers/json?all=true&filters={}", + crate::libpod::urlencoded(&filters.to_string()), + ); + let entries = self + .client + .get_json::>(&path) + .await + .map_err(ComposeError::Podman)?; + Ok(entries + .into_iter() + .find(|e| { + e.names + .iter() + .any(|n| n.trim_start_matches('/') == container) + }) + .map(|e| e.state)) + } +} diff --git a/internal/engine/lifecycle/drop_recheck_tests.rs b/internal/engine/lifecycle/drop_recheck_tests.rs new file mode 100644 index 00000000..fc0ab6e7 --- /dev/null +++ b/internal/engine/lifecycle/drop_recheck_tests.rs @@ -0,0 +1,201 @@ +//! A lifecycle POST whose response is dropped is resolved out of band (#1339). +//! +//! Measured on Podman 6 under concurrency: the drops land on state-changing +//! POSTs — `exec`, `restart`, `stop`, container `DELETE` — and follow a slow +//! one, a restart that burned its full stop grace before the next request lost +//! its response. It is not a client deadline (`READ_TIMEOUT` is 120s) and not a +//! pooled-connection race (there is no pool; every request opens a fresh +//! socket), so the transport genuinely cannot say whether the operation ran. +//! +//! These pin the three answers: the container reached the goal, it did not, and +//! the re-check itself could not be read. Only the first is success. + +use super::drop_recheck::LifecycleGoal; + +#[test] +fn a_goal_is_reached_only_by_the_state_that_satisfies_it() { + assert!(LifecycleGoal::Running.reached(Some("running"))); + assert!(!LifecycleGoal::Running.reached(Some("exited"))); + assert!(!LifecycleGoal::Running.reached(Some("paused"))); + // A container that no longer exists never satisfies `Running`, and always + // satisfies `NotRunning` — `rm` and a lost `kill` response both land here. + assert!(!LifecycleGoal::Running.reached(None)); + assert!(LifecycleGoal::NotRunning.reached(None)); + assert!(LifecycleGoal::NotRunning.reached(Some("exited"))); + assert!(!LifecycleGoal::NotRunning.reached(Some("running"))); +} + +#[cfg(unix)] +mod over_the_wire { + use super::super::drop_recheck::LifecycleGoal; + use crate::engine::fake_podman::{self, FakeReply}; + use crate::engine::Engine; + use crate::libpod::API_PREFIX; + + pub(super) fn engine_with(client: crate::libpod::Client, project: &str) -> Engine { + Engine::with_base_dir(client, project.into(), std::env::temp_dir()) + } + + /// Drop the response to the lifecycle POST, and answer the state re-check + /// with `state`. `None` reports the container as absent. + pub(super) fn fake_dropping_the_op(state: Option<&'static str>) -> fake_podman::FakePodman { + fake_podman::start_replying(move |method, target| { + // Both shapes the drops were measured on: the state-changing POSTs + // and the container DELETE. Dropping only the POSTs let a removal + // fall through to the 404 arm and be read as an idempotent no-op, + // which is a test that measures nothing. + let touches_it = target.contains("/proj-web-1"); + if touches_it && (method == "POST" || method == "DELETE") { + // Accept, then hang up without a response — the one shape that + // produces hyper's `IncompleteMessage`. + return FakeReply::ClosedWithoutResponse; + } + if method == "GET" && target.contains("/containers/json") { + let body = match state { + Some(s) => format!( + r#"[{{"Id":"abc","Names":["/proj-web-1"],"Image":"i","Status":"","State":"{s}"}}]"# + ), + None => "[]".to_string(), + }; + return FakeReply::Body(200, body); + } + FakeReply::Body(404, r#"{"message":"not found"}"#.to_string()) + }) + } + + fn start_path() -> String { + format!("{API_PREFIX}/containers/proj-web-1/start") + } + + #[tokio::test] + async fn a_lost_response_succeeds_when_the_container_reached_the_goal() { + let fake = fake_dropping_the_op(Some("running")); + let engine = engine_with(fake.client(), "proj"); + let acted = engine + .run_lifecycle_op( + &start_path(), + "proj-web-1", + "Started", + LifecycleGoal::Running, + ) + .await + .expect("the container is running, so the operation landed"); + assert!(acted, "a confirmed operation counts as having acted"); + } + + #[tokio::test] + async fn a_lost_response_fails_when_the_container_did_not_reach_the_goal() { + let fake = fake_dropping_the_op(Some("exited")); + let engine = engine_with(fake.client(), "proj"); + engine + .run_lifecycle_op( + &start_path(), + "proj-web-1", + "Started", + LifecycleGoal::Running, + ) + .await + .expect_err("the container is not running, so nothing confirms the start"); + } + + /// Fail closed. An unreadable re-check is not confirmation, and reporting + /// success without one is the defect this whole path exists to prevent. + #[tokio::test] + async fn a_lost_response_fails_when_the_state_cannot_be_re_checked() { + let fake = fake_podman::start_replying(|method, target| { + if method == "POST" && target.contains("/proj-web-1/") { + return FakeReply::ClosedWithoutResponse; + } + // The re-check itself errors. + FakeReply::Body(500, r#"{"message":"boom"}"#.to_string()) + }); + let engine = engine_with(fake.client(), "proj"); + engine + .run_lifecycle_op( + &start_path(), + "proj-web-1", + "Started", + LifecycleGoal::Running, + ) + .await + .expect_err("an unreadable re-check must not be read as success"); + } + + /// A gone container satisfies `NotRunning`, which is what a lost `kill` + /// response looks like when the kill actually worked. + #[tokio::test] + async fn a_lost_kill_response_succeeds_when_the_container_is_gone() { + let fake = fake_dropping_the_op(None); + let engine = engine_with(fake.client(), "proj"); + let acted = engine + .run_lifecycle_op( + &format!("{API_PREFIX}/containers/proj-web-1/kill?signal=SIGKILL"), + "proj-web-1", + "Killed", + LifecycleGoal::NotRunning, + ) + .await + .expect("the container is gone, so the kill landed"); + assert!(acted); + } +} + +/// `Gone` is not `NotRunning`. A removal that lost its response must not be read +/// as success just because the container stopped — it has to be absent. +#[test] +fn gone_needs_absence_not_merely_a_stopped_container() { + assert!(LifecycleGoal::Gone.reached(None)); + assert!(!LifecycleGoal::Gone.reached(Some("exited"))); + assert!(!LifecycleGoal::Gone.reached(Some("running"))); + // The distinction that matters: `exited` satisfies NotRunning and not Gone. + assert!(LifecycleGoal::NotRunning.reached(Some("exited"))); +} + +#[cfg(unix)] +mod stop_and_remove { + use super::over_the_wire::{engine_with, fake_dropping_the_op}; + + /// `stop` does not go through `run_lifecycle_op`, so it carries the re-check + /// itself. A container that is no longer running confirms the stop landed. + #[tokio::test] + async fn a_lost_stop_response_succeeds_when_the_container_is_not_running() { + let fake = fake_dropping_the_op(Some("exited")); + let engine = engine_with(fake.client(), "proj"); + engine + .stop_container("proj-web-1", 10) + .await + .expect("the container is not running, so the stop landed"); + } + + #[tokio::test] + async fn a_lost_stop_response_fails_while_the_container_still_runs() { + let fake = fake_dropping_the_op(Some("running")); + let engine = engine_with(fake.client(), "proj"); + engine + .stop_container("proj-web-1", 10) + .await + .expect_err("still running is not a stop"); + } + + /// And removal, which needs the container **absent** — a stopped-but-present + /// container would satisfy `NotRunning` and read a failed removal as success. + #[tokio::test] + async fn a_lost_removal_response_fails_when_the_container_is_merely_stopped() { + let fake = fake_dropping_the_op(Some("exited")); + let engine = engine_with(fake.client(), "proj"); + engine + .teardown_one_container("proj-web-1", 10, &[], false) + .await + .expect_err("a container that is still there was not removed"); + } + + #[tokio::test] + async fn a_lost_removal_response_succeeds_when_the_container_is_gone() { + let fake = fake_dropping_the_op(None); + let engine = engine_with(fake.client(), "proj"); + engine + .teardown_one_container("proj-web-1", 10, &[], false) + .await + .expect("the container is absent, so the removal landed"); + } +} diff --git a/internal/engine/lifecycle/mod.rs b/internal/engine/lifecycle/mod.rs index d6a687e1..f63ff955 100644 --- a/internal/engine/lifecycle/mod.rs +++ b/internal/engine/lifecycle/mod.rs @@ -2,6 +2,7 @@ mod commands; mod down_label; +mod drop_recheck; mod images; // Visible within the engine, not beyond it: the secret pre-creation stage // (#1219) fans out against the same `MAX_LIFECYCLE_CONCURRENCY` ceiling rather @@ -771,5 +772,7 @@ pub(super) fn container_rm_path(name: &str, remove_volumes: bool) -> String { ) } +#[cfg(test)] +mod drop_recheck_tests; #[cfg(test)] mod tests; diff --git a/internal/engine/lifecycle/parallel.rs b/internal/engine/lifecycle/parallel.rs index 9274aafe..66b3d7ec 100644 --- a/internal/engine/lifecycle/parallel.rs +++ b/internal/engine/lifecycle/parallel.rs @@ -18,6 +18,7 @@ use crate::engine::Engine; use crate::error::{ComposeError, Result}; use crate::libpod::{urlencoded, API_PREFIX}; +use super::drop_recheck::LifecycleGoal; use super::targets::{stop_deadline, stop_timeout_param}; /// Upper bound on the number of same-level services a lifecycle command acts on @@ -181,7 +182,7 @@ impl Engine { urlencoded(&container_name), ); if let Err(e) = self - .run_lifecycle_op(&path, &container_name, "Started") + .run_lifecycle_op(&path, &container_name, "Started", LifecycleGoal::Running) .await { first_err.get_or_insert(e); @@ -210,7 +211,7 @@ impl Engine { stop_timeout_param(grace), ); match self - .run_lifecycle_op(&restart_path, &container_name, done) + .run_lifecycle_op(&restart_path, &container_name, done, LifecycleGoal::Running) .await { Ok(true) => acted.store(true, std::sync::atomic::Ordering::Relaxed), @@ -239,7 +240,7 @@ impl Engine { urlencoded(signal), ); match self - .run_lifecycle_op(&path, &container_name, "Killed") + .run_lifecycle_op(&path, &container_name, "Killed", LifecycleGoal::NotRunning) .await { Ok(true) => acted.store(true, std::sync::atomic::Ordering::Relaxed), @@ -382,6 +383,13 @@ impl Engine { Ok(()) } Err(e) if e.is_status(404) => Ok(()), + // The other state-changing call the drops were measured on (#1339). + // `Gone` rather than `NotRunning`: a stopped-but-present container + // would satisfy the latter and read a failed removal as a success. + Err(e) if e.is_incomplete_message() => self + .confirm_lost_response(container_name, "Removed", LifecycleGoal::Gone, e) + .await + .map(|_| ()), Err(e) => { tracing::warn!("could not remove {container_name}: {e}"); Err(ComposeError::Podman(e)) diff --git a/internal/engine/query/exec.rs b/internal/engine/query/exec.rs index fa773c71..da6abc66 100644 --- a/internal/engine/query/exec.rs +++ b/internal/engine/query/exec.rs @@ -258,11 +258,35 @@ impl Engine { "{API_PREFIX}/containers/{}/exec", urlencoded(&container_name), ); - let resp: ExecCreateResponse = self - .client - .post_json(&create_path, &exec_cfg) - .await - .map_err(|e| map_not_running(e, service_name))?; + let resp: ExecCreateResponse = match self.client.post_json(&create_path, &exec_cfg).await { + Ok(resp) => resp, + // The exec create is where the drops measured in #1339 land most + // often — twice of six, more than any other endpoint. Unlike the + // lifecycle calls, a lost response here cannot be resolved by looking + // at the container: what is lost is the exec id, and a container + // running several execs at once cannot say which of its `ExecIDs` was + // this one. + // + // Retrying is safe instead, because creating an exec changes nothing. + // Measured on Podman 5.7.0: five execs created and never started leave + // the container `running` on the same pid with no extra process + // inside — they are inert handles that die with it. So the worst case + // of a retry is one leaked handle, against an `exec` that fails for a + // reason that was never about the command. + // + // Once only: a second drop is a daemon problem, not a transient. + Err(e) if e.is_incomplete_message() => { + tracing::warn!( + "{container_name}: the exec create lost its response [{}]; retrying once", + e.stream_end_kind() + ); + self.client + .post_json(&create_path, &exec_cfg) + .await + .map_err(|e| map_not_running(e, service_name))? + } + Err(e) => return Err(map_not_running(e, service_name)), + }; let exec_id = resp.id; // `-d/--detach`: start the exec and return without streaming output or @@ -573,3 +597,92 @@ mod tests { } } } + +/// The exec create is the endpoint the #1339 drops land on most, and a lost +/// response there cannot be resolved by looking at the container — what is lost +/// is the exec id. Retrying is safe because creating an exec changes nothing: +/// measured on Podman 5.7.0, five unstarted execs leave the container running on +/// the same pid with no extra process inside. +#[cfg(all(test, unix))] +mod exec_create_retry_tests { + use std::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::Arc; + + use super::ExecOptions; + use crate::compose::parse_str; + use crate::engine::fake_podman::{self, FakeReply}; + use crate::engine::Engine; + + /// Answer the exec create by dropping the connection the first `drops` times + /// and replying normally after that. Returns the fake and the call counter. + fn fake_dropping_creates(drops: usize) -> (fake_podman::FakePodman, Arc) { + let creates = Arc::new(AtomicUsize::new(0)); + let seen = creates.clone(); + let fake = fake_podman::start_replying(move |method, target| { + if method == "POST" && target.ends_with("/exec") { + let n = seen.fetch_add(1, Ordering::SeqCst); + if n < drops { + return FakeReply::ClosedWithoutResponse; + } + return FakeReply::Body(201, r#"{"Id":"exec-1"}"#.to_string()); + } + if method == "POST" && target.contains("/exec/") { + return FakeReply::Body(200, String::new()); + } + if method == "GET" && target.contains("/containers/json") { + return FakeReply::Body( + 200, + r#"[{"Id":"c1","Names":["/proj-web-1"],"Image":"i","Status":"","State":"running"}]"# + .to_string(), + ); + } + FakeReply::Body(404, r#"{"message":"not found"}"#.to_string()) + }); + (fake, creates) + } + + /// Drive the real `exec` entry point, detached so the hijacked streaming path + /// is out of the picture — the retry under test is on the CREATE, which both + /// paths share. Driving `test_exec_capture` instead would have measured + /// nothing: it builds its own request and never reaches this code. + async fn run_exec(fake: &fake_podman::FakePodman) -> crate::error::Result<()> { + let engine = Engine::with_base_dir(fake.client(), "proj".into(), std::env::temp_dir()); + let file = parse_str("services:\n web:\n image: alpine:latest\n").unwrap(); + engine + .exec_with_options( + &file, + "web", + vec!["true".to_string()], + ExecOptions::default() + .with_no_tty_for_test(true) + .with_detach_for_test(true), + ) + .await + } + + #[tokio::test] + async fn a_dropped_exec_create_is_retried_once_and_succeeds() { + let (fake, creates) = fake_dropping_creates(1); + run_exec(&fake).await.expect("the retry answers"); + assert_eq!( + creates.load(Ordering::SeqCst), + 2, + "the create must be attempted twice: once dropped, once retried" + ); + } + + /// Once only. A second drop is a daemon problem, not a transient, and + /// retrying forever would turn a broken socket into a hang. + #[tokio::test] + async fn a_second_dropped_exec_create_is_not_retried_again() { + let (fake, creates) = fake_dropping_creates(2); + run_exec(&fake) + .await + .expect_err("two drops in a row is a failure, not something to keep retrying"); + assert_eq!( + creates.load(Ordering::SeqCst), + 2, + "exactly two attempts, never a third" + ); + } +} diff --git a/internal/engine/stream_end_tests.rs b/internal/engine/stream_end_tests.rs index a337b344..1dcc2c9f 100644 --- a/internal/engine/stream_end_tests.rs +++ b/internal/engine/stream_end_tests.rs @@ -38,6 +38,10 @@ async fn read_stream(reply: FakeReply) -> (Vec, Option FakeReply::ChunkedEnd(c.clone()), FakeReply::ChunkedTruncated(c) => FakeReply::ChunkedTruncated(c.clone()), FakeReply::ChunkedCutMidPayload(c) => FakeReply::ChunkedCutMidPayload(c.clone()), + // Not a stream *ending* — it never becomes a stream. `get_stream` fails + // at the response head rather than reaching the parser this measures, so + // this shape belongs to the lifecycle re-check tests instead. + FakeReply::ClosedWithoutResponse => FakeReply::ClosedWithoutResponse, }); let client = fake.client(); let resp = client diff --git a/internal/engine/watch/mod.rs b/internal/engine/watch/mod.rs index 13e69cc4..9a0092cf 100644 --- a/internal/engine/watch/mod.rs +++ b/internal/engine/watch/mod.rs @@ -445,6 +445,36 @@ impl Engine { self.list_project_container_names(None).await } + /// The network aliases a container answers to, flattened across every + /// network it is attached to. + /// + /// The seam that lets a test check **podup's** contribution to service-name + /// resolution — registering the compose service name as an alias — without + /// depending on the runtime's DNS server being up to answer for it. Those + /// are two layers, and a test that only measures the second blames podup for + /// the first's failures (#1330). + pub async fn test_container_aliases(&self, container: &str) -> Result> { + let path = format!( + "{}/containers/{}/json", + crate::libpod::API_PREFIX, + crate::libpod::urlencoded(container) + ); + let inspect: crate::libpod::types::container::ContainerInspect = self + .client + .get_json(&path) + .await + .map_err(crate::error::ComposeError::Podman)?; + Ok(inspect + .network_settings + .map(|n| { + n.networks + .into_values() + .flat_map(|a| a.aliases) + .collect::>() + }) + .unwrap_or_default()) + } + /// Run a command in the named container and return its captured stdout. /// /// Integration tests use this to observe the effect of a watch action (e.g. diff --git a/internal/libpod/types/container/response.rs b/internal/libpod/types/container/response.rs index ca109b26..53e5527e 100644 --- a/internal/libpod/types/container/response.rs +++ b/internal/libpod/types/container/response.rs @@ -187,6 +187,23 @@ pub struct NetworkSettings { /// exposed but not published. #[serde(rename = "Ports", default)] pub ports: HashMap>>, + /// Per-network attachment details, keyed by the on-host network name. + /// + /// Carried for the aliases: a compose service is reachable by its service + /// name because podup registers that name as a network alias, and that + /// registration is the part podup owns. Whether a lookup for it then + /// *answers* is the container runtime's DNS, which is a different layer and + /// fails for its own reasons (#1330). + #[serde(rename = "Networks", default)] + pub networks: HashMap, +} + +/// One network a container is attached to. +#[derive(Deserialize, Default, Clone)] +pub struct NetworkAttachment { + /// Names this container answers to on the network, when DNS is working. + #[serde(rename = "Aliases", default, deserialize_with = "null_default")] + pub aliases: Vec, } /// Host port binding from container inspect network settings. diff --git a/internal/update/install.rs b/internal/update/install.rs index 211d4377..f2c68512 100644 --- a/internal/update/install.rs +++ b/internal/update/install.rs @@ -248,6 +248,16 @@ fn write_temp(tmp: &Path, new_bytes: &[u8], target: &Path) -> crate::Result<()> // `create_new` (O_EXCL) + O_NOFOLLOW: never follow or clobber a pre-planted // symlink in a shared/attacker-writable install directory, so the verified // bytes can only land in our own freshly created file. + // + // **Neither flag is reachable from a test, and that is a property of what + // they guard rather than a gap.** The `remove_file` above already unlinks + // any symlink planted beforehand — measured: after it, the link is gone + // and its victim is untouched — so what is left for these flags is the + // window *between* that unlink and this open. Closing a race is exactly + // the thing an in-process test cannot enter. Mutations removing either + // one survive the suite; the third property of this call, the 0600 mode, + // is reachable and is pinned by + // `write_temp_creates_the_file_private_to_this_user`. std::fs::OpenOptions::new() .write(true) .create_new(true) @@ -390,6 +400,50 @@ mod tests { assert_eq!(std::fs::read(&target).unwrap(), b"new version"); } + /// A special bit on the target is never propagated onto the new binary. + /// + /// `write_temp` copies the target's permissions so an install keeps whatever + /// mode the operator chose, and masks with `& 0o777` on the way. Without the + /// mask, a target that had been made setuid — by tampering, or by an + /// operator who did it on purpose once — would hand the freshly installed + /// podup the same bit, on a binary that has just been fetched over the + /// network. That is a privilege-escalation footgun, and it is the one + /// property of this function a test can actually observe. + /// + /// The other three are window guards and cannot be reached in process: the + /// 0600 create mode is overwritten by this very copy before the function + /// returns, and `O_EXCL`/`O_NOFOLLOW` close a race between the unlink above + /// and the open. Their comments say so where they live. + #[cfg(unix)] + #[test] + fn write_temp_never_propagates_a_special_bit_from_the_target() { + use std::os::unix::fs::PermissionsExt; + let dir = tempfile::tempdir().unwrap(); + let target = dir.path().join("podup"); + std::fs::write(&target, b"old").unwrap(); + std::fs::set_permissions(&target, std::fs::Permissions::from_mode(0o4755)).unwrap(); + // Only meaningful if the filesystem kept the bit; some do not. + let target_mode = std::fs::metadata(&target).unwrap().permissions().mode(); + if target_mode & 0o4000 == 0 { + return; + } + + let tmp = dir.path().join("podup.tmp"); + super::write_temp(&tmp, b"freshly downloaded", &target).unwrap(); + + let mode = std::fs::metadata(&tmp).unwrap().permissions().mode(); + assert_eq!( + mode & 0o7000, + 0, + "a special bit rode from the target onto the new binary: {mode:o}" + ); + assert_eq!( + mode & 0o777, + 0o755, + "the ordinary permission bits should still be carried over: {mode:o}" + ); + } + #[test] fn install_at_creates_when_absent() { let dir = tempfile::tempdir().unwrap(); diff --git a/tests/cli_diagnostics.rs b/tests/cli_diagnostics.rs index 69ad2843..726fee6a 100644 --- a/tests/cli_diagnostics.rs +++ b/tests/cli_diagnostics.rs @@ -318,3 +318,143 @@ fn update_rejects_compose_only_global_flags() { ); } } + +/// The `-f -` form enforces the same 16 MiB read cap the file path does. +/// +/// The cap exists so a pathological compose document cannot exhaust memory, and +/// the generic reader that implements it is unit-tested — but with an explicit +/// limit passed in. **Nothing checked that the stdin call site passes +/// `MAX_FILE_BYTES` rather than something larger**, which a mutation replacing it +/// with `u64::MAX` proved by surviving the whole suite. +/// +/// It cannot be closed at unit level: the function reads the real stdin, so the +/// only honest test is the one a user would perform. This feeds the binary more +/// than the cap through a pipe and asserts it is refused. +#[test] +fn stdin_is_refused_past_the_read_cap() { + use std::io::Write; + use std::process::Stdio; + + // One byte over 16 MiB, and valid YAML up to the point it is rejected, so a + // refusal cannot be mistaken for a parse error. The padding lives in a + // comment for the same reason. + const CAP: usize = 16 * 1024 * 1024; + let mut document = String::from("services:\n web:\n image: alpine\n# "); + document.push_str(&"x".repeat(CAP + 1 - document.len())); + document.push('\n'); + assert!(document.len() > CAP, "the fixture must exceed the cap"); + + let mut child = Command::new(bin()) + .args(["-f", "-", "config"]) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .unwrap(); + // A refused read closes the pipe, so the write can fail with EPIPE before + // the whole document is sent. That is the success path, not an error. + let _ = child.stdin.take().unwrap().write_all(document.as_bytes()); + let out = child.wait_with_output().unwrap(); + + let stderr = String::from_utf8_lossy(&out.stderr); + assert!( + !out.status.success(), + "oversized stdin was accepted: stdout {} bytes, stderr {stderr:?}", + out.stdout.len() + ); + assert!( + stderr.contains("larger than") && stderr.contains("limit"), + "refused for some other reason than the cap: {stderr:?}" + ); +} + +/// The same document one byte under the cap is accepted, so the test above +/// pins a threshold rather than "big inputs fail". +#[test] +fn stdin_just_under_the_cap_is_accepted() { + use std::io::Write; + use std::process::Stdio; + + const CAP: usize = 16 * 1024 * 1024; + let mut document = String::from("services:\n web:\n image: alpine\n# "); + document.push_str(&"x".repeat(CAP - document.len() - 1)); + document.push('\n'); + assert!(document.len() <= CAP, "the fixture must fit under the cap"); + + let mut child = Command::new(bin()) + .args(["-f", "-", "config"]) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .unwrap(); + child + .stdin + .take() + .unwrap() + .write_all(document.as_bytes()) + .unwrap(); + let out = child.wait_with_output().unwrap(); + assert!( + out.status.success(), + "a document under the cap was refused: {}", + String::from_utf8_lossy(&out.stderr) + ); +} + +/// A compose file past the 16 MiB cap is refused, the same as stdin. +/// +/// The cap is a bound on what a single trusted-but-unbounded input can make +/// podup allocate. `read_capped_from` implements it and is unit-tested — with an +/// explicit limit passed in — so **what was untested is that the real call sites +/// pass `MAX_FILE_BYTES`**. A mutation raising the constant to a terabyte left +/// the whole lib and bins suite green, which is what a control nothing exercises +/// looks like from outside. +#[test] +fn a_compose_file_past_the_read_cap_is_refused() { + const CAP: usize = 16 * 1024 * 1024; + let dir = TempDir::new().unwrap(); + let path = dir.path().join("docker-compose.yml"); + let mut document = String::from("services:\n web:\n image: alpine\n# "); + document.push_str(&"x".repeat(CAP + 1 - document.len())); + document.push('\n'); + fs::write(&path, &document).unwrap(); + + let out = Command::new(bin()) + .args(["-f", path.to_str().unwrap(), "config"]) + .output() + .unwrap(); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!( + !out.status.success(), + "an oversized compose file was accepted: {} bytes of stdout", + out.stdout.len() + ); + assert!( + stderr.contains("larger than") && stderr.contains("limit"), + "refused for some other reason than the cap: {stderr:?}" + ); +} + +/// The same file one byte under the cap is accepted, so the test above pins the +/// threshold rather than "large files fail". +#[test] +fn a_compose_file_just_under_the_read_cap_is_accepted() { + const CAP: usize = 16 * 1024 * 1024; + let dir = TempDir::new().unwrap(); + let path = dir.path().join("docker-compose.yml"); + let mut document = String::from("services:\n web:\n image: alpine\n# "); + document.push_str(&"x".repeat(CAP - document.len() - 1)); + document.push('\n'); + fs::write(&path, &document).unwrap(); + + let out = Command::new(bin()) + .args(["-f", path.to_str().unwrap(), "config"]) + .output() + .unwrap(); + assert!( + out.status.success(), + "a compose file under the cap was refused: {}", + String::from_utf8_lossy(&out.stderr) + ); +} diff --git a/tests/engine_integration.rs b/tests/engine_integration.rs index ba53d722..05f03f5c 100644 --- a/tests/engine_integration.rs +++ b/tests/engine_integration.rs @@ -72,6 +72,40 @@ fn bin() -> &'static str { env!("CARGO_BIN_EXE_podup") } +/// Run the built `podup` and hand back whatever it did, checking nothing. +/// +/// For calls whose outcome the test does not depend on — teardown, mostly. When +/// a later assertion depends on this command having worked, use [`run_ok`]. +#[allow(dead_code)] +fn run(args: &[&str]) -> std::process::Output { + std::process::Command::new(bin()) + .args(args) + .output() + .unwrap() +} + +/// Run the built `podup` and fail with its own words if it did not succeed. +/// +/// Setting a test up with [`run`] and then asserting on the effect throws away +/// the evidence of what went wrong. `create_makes_containers_without_starting_them` +/// discarded an `up -d` and reported `left: 0, right: 1` — true, and unable to +/// say whether `up` failed or whether it worked and the container died (#1340). +/// +/// The failure is invisible while the environment is healthy and surfaces +/// exactly when something else is already broken, which is when the diagnosis +/// is worth the most. +#[allow(dead_code)] +fn run_ok(args: &[&str]) -> std::process::Output { + let out = run(args); + assert!( + out.status.success(), + "podup {args:?} exited {}: {}", + out.status, + String::from_utf8_lossy(&out.stderr) + ); + out +} + /// Poll until reading `path` inside `container` yields exactly `expect` once /// trimmed, or `secs` elapse. Returns whether it matched. /// @@ -136,6 +170,8 @@ mod build_resources; mod commands_networking; #[path = "engine_integration/cp_flags.rs"] mod cp_flags; +#[path = "engine_integration/dns_resolution.rs"] +mod dns_resolution; #[path = "engine_integration/exec_flags.rs"] mod exec_flags; #[path = "engine_integration/health_targeting.rs"] @@ -171,6 +207,24 @@ mod create_ls; mod lifecycle_output; #[path = "engine_integration/multi_file.rs"] mod multi_file; +/// A free loopback port, chosen by binding zero and releasing it. +/// +/// Shared because three tests hard-coded `18081` and a fourth `18080`, so any +/// two of them running at once fought over the same bind and the loser failed +/// with `pasta failed ... Address already in use`. That is not flakiness: at +/// eight test threads it is close to certain. +/// +/// There is a window between releasing the port and the container binding it. +/// It is small, and far smaller than the certainty of a shared constant. +#[allow(dead_code)] +fn free_port() -> u16 { + std::net::TcpListener::bind("127.0.0.1:0") + .expect("no loopback port") + .local_addr() + .unwrap() + .port() +} + #[path = "engine_integration/push_registry.rs"] mod push_registry; #[path = "engine_integration/scale.rs"] diff --git a/tests/engine_integration/cli_commands.rs b/tests/engine_integration/cli_commands.rs index ba368043..77014b9e 100644 --- a/tests/engine_integration/cli_commands.rs +++ b/tests/engine_integration/cli_commands.rs @@ -413,9 +413,15 @@ async fn cli_port_subcommand() { let dir = tempdir().unwrap(); let compose = dir.path().join("docker-compose.yml"); let proj = format!("t{}-clprt", std::process::id()); + // A port chosen at run time, not a constant: three tests shared 18081 and a + // fourth 18080, so any two running at once lost the bind and failed with + // `pasta failed ... Address already in use`. + let port = super::free_port(); fs::write( &compose, - "services:\n web:\n image: alpine:latest\n command: [\"sleep\", \"infinity\"]\n ports:\n - \"127.0.0.1:18081:80\"\n", + format!( + "services:\n web:\n image: alpine:latest\n command: [\"sleep\", \"infinity\"]\n ports:\n - \"127.0.0.1:{port}:80\"\n" + ), ) .unwrap(); diff --git a/tests/engine_integration/cli_flags.rs b/tests/engine_integration/cli_flags.rs index 85943614..ea536cee 100644 --- a/tests/engine_integration/cli_flags.rs +++ b/tests/engine_integration/cli_flags.rs @@ -60,10 +60,6 @@ async fn cli_logs_tail_limits_output() { .unwrap(); } -fn run(args: &[&str]) -> std::process::Output { - Command::new(bin()).args(args).output().unwrap() -} - fn ps_all_count(compose: &str, proj: &str) -> usize { String::from_utf8_lossy(&run(&["-f", compose, "-p", proj, "ps", "-a", "-q"]).stdout) .lines() @@ -89,7 +85,7 @@ async fn cli_down_remove_orphans_drops_undeclared_containers() { fs::write(&one, format!("services:\n web:\n {svc}\n")).unwrap(); let (two, one) = (two.to_str().unwrap(), one.to_str().unwrap()); - run(&["-f", two, "-p", &proj, "up", "-d"]); + run_ok(&["-f", two, "-p", &proj, "up", "-d"]); assert_eq!(ps_all_count(two, &proj), 2); // Down against the one-service file: --remove-orphans must also drop `extra`. @@ -117,7 +113,7 @@ async fn cli_restart_no_deps_succeeds() { .unwrap(); let c = compose.to_str().unwrap(); - run(&["-f", c, "-p", &proj, "up", "-d"]); + run_ok(&["-f", c, "-p", &proj, "up", "-d"]); let restart = run(&["-f", c, "-p", &proj, "restart", "--no-deps", "web"]); assert!( restart.status.success(), @@ -170,7 +166,7 @@ async fn cli_up_pull_never_starts_present_image() { .unwrap(); let c = compose.to_str().unwrap(); // Ensure the image is present, then `--pull never` must still start it. - run(&["-f", c, "-p", &proj, "up", "-d"]); + run_ok(&["-f", c, "-p", &proj, "up", "-d"]); run(&["-f", c, "-p", &proj, "down"]); let up = run(&["-f", c, "-p", &proj, "up", "-d", "--pull", "never"]); assert!( @@ -220,7 +216,7 @@ async fn cli_down_rmi_all_succeeds_and_removes_containers() { .unwrap(); let c = compose.to_str().unwrap(); - run(&["-f", c, "-p", &proj, "up", "-d"]); + run_ok(&["-f", c, "-p", &proj, "up", "-d"]); let down = run(&["-f", c, "-p", &proj, "down", "--rmi", "all"]); assert!( down.status.success(), @@ -259,7 +255,7 @@ async fn cli_rm_volumes_removes_container() { .unwrap(); let c = compose.to_str().unwrap(); - run(&["-f", c, "-p", &proj, "up", "-d"]); + run_ok(&["-f", c, "-p", &proj, "up", "-d"]); run(&["-f", c, "-p", &proj, "stop"]); let rm = run(&["-f", c, "-p", &proj, "rm", "-v", "-f"]); assert!(rm.status.success(), "rm -v failed: {:?}", rm.stderr); @@ -284,7 +280,7 @@ async fn cli_kill_remove_orphans_drops_undeclared() { fs::write(&one, format!("services:\n web:\n {svc}\n")).unwrap(); let (two, one) = (two.to_str().unwrap(), one.to_str().unwrap()); - run(&["-f", two, "-p", &proj, "up", "-d"]); + run_ok(&["-f", two, "-p", &proj, "up", "-d"]); let kill = run(&["-f", one, "-p", &proj, "kill", "--remove-orphans"]); assert!(kill.status.success(), "kill failed: {:?}", kill.stderr); // The orphan `extra` is removed; the declared `web` is killed but remains. @@ -367,7 +363,7 @@ async fn cli_rm_stop_removes_running_container() { .unwrap(); let c = compose.to_str().unwrap(); - run(&["-f", c, "-p", &proj, "up", "-d"]); + run_ok(&["-f", c, "-p", &proj, "up", "-d"]); assert_eq!(ps_all_count(c, &proj), 1, "container should exist after up"); // `rm -s` (no -f) must stop the running container first, then remove it. @@ -399,7 +395,7 @@ async fn cli_start_wait_returns_after_starting() { // Create the container without starting, then `start --wait` must start it // and return (no healthcheck → ready once started). - run(&["-f", c, "-p", &proj, "up", "--no-start"]); + run_ok(&["-f", c, "-p", &proj, "up", "--no-start"]); let start = run(&[ "-f", c, diff --git a/tests/engine_integration/commands_networking.rs b/tests/engine_integration/commands_networking.rs index c904d269..18e593c2 100644 --- a/tests/engine_integration/commands_networking.rs +++ b/tests/engine_integration/commands_networking.rs @@ -167,9 +167,13 @@ async fn engine_port_resolves_a_published_port() { }; let proj = proj("prt"); let engine = Engine::new(client, proj.clone()); - let file = parse_str( - "services:\n web:\n image: alpine:latest\n command: [\"sleep\", \"infinity\"]\n ports:\n - \"127.0.0.1:18080:80\"\n", - ) + // A port chosen at run time, not a constant: three tests shared 18081 and a + // fourth 18080, so any two running at once lost the bind and failed with + // `pasta failed ... Address already in use`. + let port = super::free_port(); + let file = parse_str(&format!( + "services:\n web:\n image: alpine:latest\n command: [\"sleep\", \"infinity\"]\n ports:\n - \"127.0.0.1:{port}:80\"\n" + )) .unwrap(); engine.up(&file).await.unwrap(); @@ -496,92 +500,6 @@ async fn up_is_idempotent_over_existing_named_volume() { second.expect("second up over an existing named volume must be idempotent"); } -// --------------------------------------------------------------------------- -// A sibling resolves a service by its service name on a shared network -// --------------------------------------------------------------------------- - -#[cfg(feature = "test-helpers")] -#[tokio::test] -async fn sibling_resolves_service_by_name_on_shared_network() { - let client = match podman().await { - Some(d) => d, - None => return, - }; - let proj = proj("dns"); - let engine = Engine::new(client, proj.clone()); - let file = parse_str( - "services:\n server:\n image: busybox:latest\n command: [\"sh\", \"-c\", \"mkdir -p /www; echo ok > /www/index.html; exec httpd -f -p 80 -h /www\"]\n networks:\n - appnet\n client:\n image: busybox:latest\n command: [\"sleep\", \"infinity\"]\n networks:\n - appnet\nnetworks:\n appnet:\n", - ) - .unwrap(); - - engine.up(&file).await.unwrap(); - // The client must reach the server by its compose service name (`server`), - // not only by the container name — the service name has to be registered as - // a network alias. Retry briefly while the server's httpd comes up. - let out = engine - .test_exec_capture( - &format!("{proj}-client-1"), - vec![ - "sh".into(), - "-c".into(), - "for i in $(seq 1 30); do wget -q -O - http://server:80/ && exit 0; sleep 0.3; done; exit 1".into(), - ], - ) - .await; - engine.down(&file).await.unwrap(); - let out = out.expect("exec in client container failed"); - assert!( - out.contains("ok"), - "service `server` was not reachable by its service name: {out:?}" - ); -} - -// --------------------------------------------------------------------------- -// With NO `networks:` block, services still reach each other by service name -// (the synthesized `default` network — docker-compose parity, #417) -// --------------------------------------------------------------------------- - -#[cfg(feature = "test-helpers")] -#[tokio::test] -async fn sibling_resolves_service_by_name_without_networks_block() { - let client = match podman().await { - Some(d) => d, - None => return, - }; - let proj = proj("dnsdef"); - let engine = Engine::new(client, proj.clone()); - - // No top-level `networks:` and no per-service `networks:` — the common case. - // Parse through the real CLI entry point so the implicit `default` network - // is synthesized; `parse_str` deliberately does not normalize. - let dir = tempfile::tempdir().unwrap(); - let compose = dir.path().join("docker-compose.yml"); - fs::write( - &compose, - "services:\n server:\n image: busybox:latest\n command: [\"sh\", \"-c\", \"mkdir -p /www; echo ok > /www/index.html; exec httpd -f -p 80 -h /www\"]\n client:\n image: busybox:latest\n command: [\"sleep\", \"infinity\"]\n", - ) - .unwrap(); - let file = parse_files_with_env_files(&[compose], &[]).unwrap(); - - engine.up(&file).await.unwrap(); - let out = engine - .test_exec_capture( - &format!("{proj}-client-1"), - vec![ - "sh".into(), - "-c".into(), - "for i in $(seq 1 30); do wget -q -O - http://server:80/ && exit 0; sleep 0.3; done; exit 1".into(), - ], - ) - .await; - engine.down(&file).await.unwrap(); - let out = out.expect("exec in client container failed"); - assert!( - out.contains("ok"), - "service `server` was not reachable by name without a networks: block: {out:?}" - ); -} - // --------------------------------------------------------------------------- // up -V/--renew-anon-volumes and up --timestamps // --------------------------------------------------------------------------- diff --git a/tests/engine_integration/create_ls.rs b/tests/engine_integration/create_ls.rs index 3a1ea092..f36da12c 100644 --- a/tests/engine_integration/create_ls.rs +++ b/tests/engine_integration/create_ls.rs @@ -1,15 +1,10 @@ //! Integration tests for `create` (containers without starting) and `ls` //! (project discovery) against a real Podman daemon. Skip when unreachable. use std::fs; -use std::process::Command; use tempfile::tempdir; use super::*; -fn run(args: &[&str]) -> std::process::Output { - Command::new(bin()).args(args).output().unwrap() -} - /// Count non-empty lines of a `-q` listing. fn count(out: &std::process::Output) -> usize { String::from_utf8_lossy(&out.stdout) @@ -48,7 +43,7 @@ async fn create_makes_containers_without_starting_them() { ); // `up` then starts the already-created container. - run(&["-f", c, "-p", &proj, "up", "-d"]); + run_ok(&["-f", c, "-p", &proj, "up", "-d"]); assert_eq!(count(&run(&["-f", c, "-p", &proj, "ps", "-q"])), 1); run(&["-f", c, "-p", &proj, "down"]); @@ -69,7 +64,7 @@ async fn ls_lists_running_projects() { .unwrap(); let c = compose.to_str().unwrap(); - run(&["-f", c, "-p", &proj, "up", "-d"]); + run_ok(&["-f", c, "-p", &proj, "up", "-d"]); // `ls -q` (running only) lists the project by name. let names = String::from_utf8_lossy(&run(&["-p", &proj, "ls", "-q"]).stdout).into_owned(); assert!( diff --git a/tests/engine_integration/dns_resolution.rs b/tests/engine_integration/dns_resolution.rs new file mode 100644 index 00000000..14aac0de --- /dev/null +++ b/tests/engine_integration/dns_resolution.rs @@ -0,0 +1,187 @@ +//! Service-name resolution between siblings, split by layer (#1330). +//! +//! Reaching a service by its compose name needs three things to work, and only +//! the first belongs to podup: +//! +//! 1. podup registers the service name as a network alias, +//! 2. netavark wires the network, +//! 3. aardvark-dns answers the lookup. +//! +//! These tests used to assert only the end-to-end outcome, so a runtime whose +//! DNS server had died reported `service \`server\` was not reachable by its +//! service name` — which reads as a podup defect and cost real debugging time +//! before measurement showed aardvark-dns was simply not running. Each layer is +//! now asserted on its own, and a failure names the layer that produced it. + +use super::*; + +/// Ask whether the container runtime's DNS is answering at all. +/// +/// Only ever called after a lookup has already failed, so a passing test pays +/// nothing for it. +/// +/// The probe is the *container's own name*, which the runtime registers itself +/// with no involvement from podup. That makes it a clean discriminator: a name +/// podup never touched failing to resolve cannot be podup's alias handling. +/// +/// The signatures come from the measurement in #1330 rather than from guessing +/// at what busybox prints — a dead server times out, while a live server that +/// does not know the name answers NXDOMAIN, and only the first is the runtime's +/// fault. `test_exec_capture` attaches stderr and does not inspect the exit +/// code, so a failed lookup arrives as `Ok` carrying its own complaint. +async fn runtime_dns_is_down(engine: &Engine, from: &str, own_name: &str) -> bool { + match engine + .test_exec_capture(from, vec!["nslookup".into(), own_name.into()]) + .await + { + Ok(out) => { + out.contains("no servers could be reached") || out.contains("connection timed out") + } + // The exec failing says nothing about DNS. Staying quiet here keeps the + // original assertion's message, which is the honest one when the cause + // is unknown. + Err(_) => false, + } +} + +/// The body both tests share: bring the project up, assert podup's layer, then +/// the runtime's, and tear down whatever happened. +/// +/// `alias_context` distinguishes the two topologies in the failure message — +/// an explicit `networks:` block versus the synthesized `default` network — +/// because "the alias is missing" has a different cause in each. +async fn assert_sibling_resolves_by_service_name( + engine: &Engine, + file: &podup::compose::types::ComposeFile, + proj: &str, + alias_context: &str, +) { + let server = format!("{proj}-server-1"); + let client = format!("{proj}-client-1"); + + engine.up(file).await.unwrap(); + + // **podup's layer**, checkable with no DNS involved: the compose service + // name is registered as a network alias. This is the whole of podup's + // contribution to service-name resolution, and the part a podup regression + // would break. + let aliases = engine + .test_container_aliases(&server) + .await + .expect("could not read the server's network aliases"); + + // **The runtime's layer**: the alias actually answers. Retry briefly while + // the server's httpd comes up. + let out = engine + .test_exec_capture( + &client, + vec![ + "sh".into(), + "-c".into(), + "for i in $(seq 1 30); do wget -q -O - http://server:80/ && exit 0; sleep 0.3; done; exit 1".into(), + ], + ) + .await; + + // Only probe DNS when the lookup did not answer, and do it before `down` + // removes the containers the probe needs. + let dns_down = match &out { + Ok(o) if o.contains("ok") => false, + _ => runtime_dns_is_down(engine, &client, &server).await, + }; + + engine.down(file).await.unwrap(); + + assert!( + aliases.iter().any(|a| a == "server"), + "podup did not register the service name as a network alias {alias_context}: {aliases:?}" + ); + + // A runtime whose DNS is down cannot answer this question, and a test that + // could not run is not a test that failed. It skips — but only where the + // environment does not promise Podman works. + // + // Where it does (the nested-virt lane sets PODUP_REQUIRE_PODMAN), the skip + // becomes a hard failure, for the same reason `podman()` refuses to skip + // there: a lane that reports `ok` for tests it never ran is the failure mode + // this whole mechanism exists to prevent. The message names the runtime so + // the next reader does not start by suspecting podup. + if dns_down { + assert!( + std::env::var_os("PODUP_REQUIRE_PODMAN").is_none(), + "the alias `server` is registered, so podup did its part, but the container \ + runtime's DNS server did not answer a lookup for the container's own name \ + either — aardvark-dns is down. PODUP_REQUIRE_PODMAN is set, so this is a \ + broken environment rather than a test to skip." + ); + eprintln!( + "skipping: the container runtime's DNS is not answering (aardvark-dns), so \ + service-name resolution cannot be measured here" + ); + return; + } + + let out = out.expect("exec in client container failed"); + assert!( + out.contains("ok"), + "the alias `server` is registered and the runtime's DNS is answering, so the \ + lookup failing is a real service-name resolution defect: {out:?}" + ); +} + +// --------------------------------------------------------------------------- +// A sibling resolves a service by its service name on a shared network +// --------------------------------------------------------------------------- + +#[cfg(feature = "test-helpers")] +#[tokio::test] +async fn sibling_resolves_service_by_name_on_shared_network() { + let client = match podman().await { + Some(d) => d, + None => return, + }; + let proj = proj("dns"); + let engine = Engine::new(client, proj.clone()); + let file = parse_str( + "services:\n server:\n image: busybox:latest\n command: [\"sh\", \"-c\", \"mkdir -p /www; echo ok > /www/index.html; exec httpd -f -p 80 -h /www\"]\n networks:\n - appnet\n client:\n image: busybox:latest\n command: [\"sleep\", \"infinity\"]\n networks:\n - appnet\nnetworks:\n appnet:\n", + ) + .unwrap(); + + assert_sibling_resolves_by_service_name(&engine, &file, &proj, "on the shared network").await; +} + +// --------------------------------------------------------------------------- +// With NO `networks:` block, services still reach each other by service name +// (the synthesized `default` network — docker-compose parity, #417) +// --------------------------------------------------------------------------- + +#[cfg(feature = "test-helpers")] +#[tokio::test] +async fn sibling_resolves_service_by_name_without_networks_block() { + let client = match podman().await { + Some(d) => d, + None => return, + }; + let proj = proj("dnsdef"); + let engine = Engine::new(client, proj.clone()); + + // No top-level `networks:` and no per-service `networks:` — the common case. + // Parse through the real CLI entry point so the implicit `default` network + // is synthesized; `parse_str` deliberately does not normalize. + let dir = tempfile::tempdir().unwrap(); + let compose = dir.path().join("docker-compose.yml"); + fs::write( + &compose, + "services:\n server:\n image: busybox:latest\n command: [\"sh\", \"-c\", \"mkdir -p /www; echo ok > /www/index.html; exec httpd -f -p 80 -h /www\"]\n client:\n image: busybox:latest\n command: [\"sleep\", \"infinity\"]\n", + ) + .unwrap(); + let file = parse_files_with_env_files(&[compose], &[]).unwrap(); + + assert_sibling_resolves_by_service_name( + &engine, + &file, + &proj, + "on the synthesized default network", + ) + .await; +} diff --git a/tests/engine_integration/niche.rs b/tests/engine_integration/niche.rs index ea5d3974..3517e71e 100644 --- a/tests/engine_integration/niche.rs +++ b/tests/engine_integration/niche.rs @@ -1,14 +1,10 @@ //! Niche-command CLI integration tests (wait/export/commit), split for the //! source line limit. use std::fs; -use std::process::Command; use tempfile::tempdir; use super::*; -fn run(args: &[&str]) -> std::process::Output { - Command::new(bin()).args(args).output().unwrap() -} #[tokio::test] async fn cli_wait_names_the_container_and_its_exit_code() { if super::podman().await.is_none() { @@ -24,7 +20,7 @@ async fn cli_wait_names_the_container_and_its_exit_code() { .unwrap(); let c = compose.to_str().unwrap(); - run(&["-f", c, "-p", &proj, "up", "-d"]); + run_ok(&["-f", c, "-p", &proj, "up", "-d"]); let out = run(&["-f", c, "-p", &proj, "wait", "job"]); assert!(out.status.success(), "wait failed: {:?}", out.stderr); // One line per container, naming it (#1248). This used to assert a line @@ -56,7 +52,7 @@ async fn cli_export_writes_tar() { let c = compose.to_str().unwrap(); let tar = dir.path().join("rootfs.tar"); - run(&["-f", c, "-p", &proj, "up", "-d"]); + run_ok(&["-f", c, "-p", &proj, "up", "-d"]); let out = run(&[ "-f", c, @@ -89,7 +85,7 @@ async fn cli_commit_creates_image() { .unwrap(); let c = compose.to_str().unwrap(); - run(&["-f", c, "-p", &proj, "up", "-d"]); + run_ok(&["-f", c, "-p", &proj, "up", "-d"]); let out = run(&["-f", c, "-p", &proj, "commit", "web", &img]); run(&["-f", c, "-p", &proj, "down"]); let exists = std::process::Command::new("podman") @@ -126,7 +122,7 @@ async fn cli_attach_streams_output_until_exit() { .unwrap(); let c = compose.to_str().unwrap(); - run(&["-f", c, "-p", &proj, "up", "-d"]); + run_ok(&["-f", c, "-p", &proj, "up", "-d"]); let out = run(&["-f", c, "-p", &proj, "attach", "web"]); run(&["-f", c, "-p", &proj, "down"]); assert!(out.status.success(), "attach failed: {:?}", out.stderr); diff --git a/tests/engine_integration/push_registry.rs b/tests/engine_integration/push_registry.rs index f76460f3..91a079ae 100644 --- a/tests/engine_integration/push_registry.rs +++ b/tests/engine_integration/push_registry.rs @@ -18,17 +18,6 @@ use tempfile::tempdir; use super::*; -/// A free loopback port, chosen by binding zero and releasing it. -/// -/// There is a window between releasing and the registry binding it. It is small -/// and the readiness poll below fails loudly rather than silently if it is lost, -/// which is the honest trade against hard-coding a port two concurrent runs -/// would fight over. -fn free_port() -> u16 { - let listener = std::net::TcpListener::bind("127.0.0.1:0").expect("no loopback port"); - listener.local_addr().unwrap().port() -} - /// One HTTP GET over a plain TCP socket, returning the body. /// /// Raw rather than a client library on purpose: this asks a local registry for a diff --git a/tests/engine_integration/scale.rs b/tests/engine_integration/scale.rs index bc492fae..aae407e1 100644 --- a/tests/engine_integration/scale.rs +++ b/tests/engine_integration/scale.rs @@ -18,10 +18,6 @@ fn running_count(compose: &str, proj: &str) -> usize { .count() } -fn run(args: &[&str]) -> std::process::Output { - Command::new(bin()).args(args).output().unwrap() -} - /// Podman container id for `name`, or empty when it does not exist. fn container_id(name: &str) -> String { let out = Command::new("podman") @@ -80,7 +76,7 @@ async fn scale_subcommand_scales_up_then_down() { .unwrap(); let c = compose.to_str().unwrap(); - run(&["-f", c, "-p", &proj, "up", "--detach"]); + run_ok(&["-f", c, "-p", &proj, "up", "--detach"]); assert_eq!(running_count(c, &proj), 1); let up = run(&["-f", c, "-p", &proj, "scale", "worker=3"]); diff --git a/tests/engine_integration/stats_flags.rs b/tests/engine_integration/stats_flags.rs index 45ecfe3b..020612e8 100644 --- a/tests/engine_integration/stats_flags.rs +++ b/tests/engine_integration/stats_flags.rs @@ -129,7 +129,15 @@ async fn cli_stats_streaming_json_is_ndjson() { .output() .expect("run podup") }; - run(&["up", "-d"]); + // The closure above only checks that podup started, not that it exited 0, + // so a failed `up` would surface as a confusing assertion further down + // (#1340). + let up = run(&["up", "-d"]); + assert!( + up.status.success(), + "up -d failed: {}", + String::from_utf8_lossy(&up.stderr) + ); // Take a couple of frames, then stop: the stream never ends on its own. let out = Command::new("timeout") @@ -187,7 +195,15 @@ async fn cli_port_without_a_binding_exits_nonzero() { .output() .expect("run podup") }; - run(&["up", "-d"]); + // The closure above only checks that podup started, not that it exited 0, + // so a failed `up` would surface as a confusing assertion further down + // (#1340). + let up = run(&["up", "-d"]); + assert!( + up.status.success(), + "up -d failed: {}", + String::from_utf8_lossy(&up.stderr) + ); let out = run(&["port", "web", "80"]); assert!( @@ -210,9 +226,15 @@ async fn cli_port_prints_the_published_binding() { let dir = tempdir().unwrap(); let compose = dir.path().join("docker-compose.yml"); let proj = format!("t{}-prtb", std::process::id()); + // A port chosen at run time, not a constant: three tests shared 18081 and a + // fourth 18080, so any two running at once lost the bind and failed with + // `pasta failed ... Address already in use`. + let port = super::free_port(); fs::write( &compose, - "services:\n web:\n image: alpine:latest\n command: [\"sleep\", \"infinity\"]\n ports:\n - \"127.0.0.1:18081:80\"\n", + format!( + "services:\n web:\n image: alpine:latest\n command: [\"sleep\", \"infinity\"]\n ports:\n - \"127.0.0.1:{port}:80\"\n" + ), ) .unwrap(); let c = compose.to_str().unwrap(); @@ -223,7 +245,15 @@ async fn cli_port_prints_the_published_binding() { .output() .expect("run podup") }; - run(&["up", "-d"]); + // The closure above only checks that podup started, not that it exited 0, + // so a failed `up` would surface as a confusing assertion further down + // (#1340). + let up = run(&["up", "-d"]); + assert!( + up.status.success(), + "up -d failed: {}", + String::from_utf8_lossy(&up.stderr) + ); let out = run(&["port", "web", "80"]); let stdout = String::from_utf8_lossy(&out.stdout).to_string(); @@ -235,7 +265,7 @@ async fn cli_port_prints_the_published_binding() { "port failed for a published port: {stdout:?}" ); assert!( - stdout.contains("127.0.0.1:18081"), + stdout.contains(&format!("127.0.0.1:{port}")), "port did not print the host binding it was asked for: {stdout:?}" ); }