From a1b4fbc47c2ad7f09ab86c4f271123d9ee7b9119 Mon Sep 17 00:00:00 2001 From: Bryant Biggs Date: Fri, 14 Aug 2026 07:58:41 -0500 Subject: [PATCH] fix(bench): fail bootstrap loudly and stop recording unusable versions Every network fetch in the bootstrap now goes through one retry helper that aborts after three attempts, including the dnf calls that a cold instance hits first. The four go install calls had no retry at all, and unpinning moved the trigger for a transient failure from an operator editing a pin to an upstream push. Nothing waits on cloud-init, so apply reports success either way and the failure would otherwise appear when bench-remote finds no binaries. Three paths made a failure invisible. The rustup pipeline returned the installer's status, and the installer exits zero on the EOF a failed curl feeds it, so the retry reported success having installed nothing. The build block ran without set -e, so only the last command's status escaped and a failed cargo build was masked by the copy after it. And every verification was a command substitution inside an echo argument, which yields echo's status, so none of them could fail the script. The clone no longer deletes an existing checkout. That path holds the git-tracked run-record archive, and re-running the bootstrap through cloud-init clean is a normal thing to do. Versions are recorded more honestly. A binary built from a working tree stamps (devel), which names no release, so it is not read as a version. Every path ending in a recorded "unknown" says why, and says which fallback it took rather than asserting an outcome it does not control. The bootstrap no longer parses build info in awk, which duplicated the Rust parser and disagreed with it. ocync's version probe reads the binary the run executes rather than whatever PATH offers, since a PATH lookup finds the release installed at instance creation and would credit it with HEAD's numbers. --- bench/CLAUDE.md | 4 +- bench/terraform/aws/user-data.sh | 101 +++++++++++++----- xtask/src/bench/mod.rs | 16 +-- xtask/src/bench/remote.rs | 4 +- xtask/src/bench/runner.rs | 177 +++++++++++++++++++++++++------ 5 files changed, 228 insertions(+), 74 deletions(-) diff --git a/bench/CLAUDE.md b/bench/CLAUDE.md index f5827e3e..e8a4d2a6 100644 --- a/bench/CLAUDE.md +++ b/bench/CLAUDE.md @@ -133,7 +133,9 @@ A benchmark run is a point in time, not a fixed rig. Nothing is pinned: competit Three resolution details are easy to get wrong. skopeo's module path is `go.podman.io/skopeo` since v1.23.0, and the old `github.com/containers/skopeo` path still serves tags whose `go.mod` declares the new path, so installing from it fails on a path mismatch rather than a missing version. `@latest` never crosses a major version boundary under semantic import versioning, so when one of these tags v2 the install silently sticks on the highest v1 until the path gains a `/v2` suffix. And dregsy tags releases without a `v` prefix, so the proxy reads no valid semver and `@latest` resolves to a pseudo-version off the default branch rather than to a release. -Versions are read from the binaries rather than assumed. `go install` sets none of the ldflags a release build uses, so the ECR credential helper reports `development` and dregsy has no version flag at all. Both are recovered with `go version -m`, which reads the module version stamped into the binary. +Versions are read from the binaries rather than assumed. `go install` sets none of the ldflags a release build uses, so the ECR credential helper reports `development` and dregsy has no version flag at all. Both are recovered with `go version -m`, which reads the module version stamped into the binary. That can still come up empty, for a binary built from a working tree (Go stamps `(devel)`, which names no release) or where Go is absent, and the record then reads `unknown` with the reason on stderr. Treat an `unknown` as a run whose provenance was lost rather than as a harness bug. + +The bootstrap log is `/var/log/user-data.log` on the instance, not `cloud-init-output.log`: `user-data.sh` redirects its own output there. dregsy transfers nothing itself. Its config sets `relay: skopeo`, so skopeo moves every byte dregsy is credited with, and the run record captures skopeo's version alongside dregsy's for that reason. diff --git a/bench/terraform/aws/user-data.sh b/bench/terraform/aws/user-data.sh index 2723cbb9..87d2e17a 100644 --- a/bench/terraform/aws/user-data.sh +++ b/bench/terraform/aws/user-data.sh @@ -17,10 +17,36 @@ if ! swapon --show | grep -q '/swapfile'; then echo '/swapfile none swap sw 0 0' >> /etc/fstab fi +# ── Retry helper ────────────────────────────────────────────────────────────── + +# Every network fetch below goes through this. Exhausting the attempts is a +# hard failure: nothing waits on cloud-init, so `terraform apply` reports +# success either way, and continuing past a failed fetch turns a clear error +# here into a confusing missing-binary error when bench-remote runs later. +retry() { + local what="$1" + shift + local attempt + for attempt in 1 2 3; do + if "$@"; then + return 0 + fi + if [ "$attempt" -lt 3 ]; then + echo " $what attempt $attempt failed, retrying in 15s..." + sleep 15 + fi + done + echo "ERROR: $what failed after 3 attempts, aborting bootstrap" >&2 + return 1 +} + # ── System packages ─────────────────────────────────────────────────────────── -dnf update -y -dnf install -y \ +# A cold instance's first mirror contact is the most transient-failure-prone +# fetch here, and gpgme-devel and openssl-devel are what skopeo's cgo build +# needs later. +retry "dnf update" dnf update -y +retry "dnf install" dnf install -y \ git \ cmake \ gcc \ @@ -47,17 +73,15 @@ chown ec2-user:ec2-user /home/ec2-user/.bashrc echo "--- Installing Rust via rustup (as ec2-user)" -for attempt in 1 2 3; do - if su - ec2-user -c 'curl --proto "=https" --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y --no-modify-path'; then - break - fi - echo " rustup attempt $attempt failed, retrying in 15s..." - sleep 15 -done +# pipefail is set inside the su, not inherited from this shell: without it the +# pipeline's status is the installer's, and curl failing feeds it EOF, which it +# exits zero on. retry would then report success having installed nothing. +retry rustup su - ec2-user -c 'set -o pipefail; curl --proto "=https" --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y --no-modify-path' su - ec2-user -c 'echo "export PATH=\"\$HOME/.cargo/bin:\$PATH\"" >> ~/.bashrc' -echo "Rust: $(su - ec2-user -c '/home/ec2-user/.cargo/bin/rustc --version')" +RUSTC_VERSION="$(su - ec2-user -c '/home/ec2-user/.cargo/bin/rustc --version')" +echo "Rust: $${RUSTC_VERSION}" # ── Go ──────────────────────────────────────────────────────────────────────── @@ -65,13 +89,7 @@ echo "--- Installing Go 1.26.x" GO_VERSION="1.26.2" GO_ARCHIVE="go$${GO_VERSION}.linux-amd64.tar.gz" -for attempt in 1 2 3; do - if curl -fsSL "https://go.dev/dl/$${GO_ARCHIVE}" -o "/tmp/$${GO_ARCHIVE}"; then - break - fi - echo " Go download attempt $attempt failed, retrying in 15s..." - sleep 15 -done +retry "Go download" curl -fsSL "https://go.dev/dl/$${GO_ARCHIVE}" -o "/tmp/$${GO_ARCHIVE}" tar -C /usr/local -xzf "/tmp/$${GO_ARCHIVE}" rm -f "/tmp/$${GO_ARCHIVE}" @@ -86,7 +104,8 @@ EOF chown ec2-user:ec2-user /home/ec2-user/.bashrc -echo "Go: $(go version)" +GO_VERSION_LINE="$(go version)" +echo "Go: $${GO_VERSION_LINE}" # Go env for root builds (dregsy, regsync, skopeo, ecr-credential-helper) export HOME=/root GOPATH=/root/go GOCACHE=/root/.cache/go-build @@ -101,7 +120,7 @@ export PATH="/usr/local/go/bin:$GOPATH/bin:$PATH" # ── ECR credential helper ──────────────────────────────────────────────────── echo "--- Installing ECR credential helper" -go install github.com/awslabs/amazon-ecr-credential-helper/ecr-login/cli/docker-credential-ecr-login@latest +retry "ecr-credential-helper install" go install github.com/awslabs/amazon-ecr-credential-helper/ecr-login/cli/docker-credential-ecr-login@latest cp /root/go/bin/docker-credential-ecr-login /usr/local/bin/ mkdir -p /home/ec2-user/.docker @@ -114,7 +133,8 @@ chown -R ec2-user:ec2-user /home/ec2-user/.docker # the only version several of these report. The credential helper leaves its # own version at "development" because go install sets none of the ldflags its # Makefile uses, and dregsy has no version flag at all. -echo "ecr-credential-helper: $(go version -m /usr/local/bin/docker-credential-ecr-login | awk '$1 == "mod" { print $3; exit }')" +ECR_HELPER_MOD="$(go version -m /usr/local/bin/docker-credential-ecr-login | grep -m1 -E '^[[:space:]]*mod[[:space:]]' || echo 'build info unavailable')" +echo "ecr-credential-helper: $${ECR_HELPER_MOD}" # ── skopeo (dregsy transfer backend) ───────────────────────────────────────── @@ -122,12 +142,13 @@ echo "--- Installing skopeo" # skopeo moved its module path to go.podman.io/skopeo at v1.23.0. The github.com # path still serves tags, but their go.mod declares the new path, so installing # from it fails on a module path mismatch rather than a missing version. -CGO_ENABLED=1 go install \ +retry "skopeo install" env CGO_ENABLED=1 go install \ -tags "exclude_graphdriver_btrfs containers_image_openpgp" \ go.podman.io/skopeo/cmd/skopeo@latest cp /root/go/bin/skopeo /usr/local/bin/skopeo -echo "skopeo: $(skopeo --version 2>&1)" +SKOPEO_VERSION_LINE="$(skopeo --version)" +echo "skopeo: $${SKOPEO_VERSION_LINE}" # ── dregsy ──────────────────────────────────────────────────────────────────── @@ -135,18 +156,20 @@ echo "--- Installing dregsy" # dregsy tags releases without the v prefix, so the module proxy cannot read # them as versions and @latest resolves to a pseudo-version off the default # branch rather than to a release. -go install github.com/xelalexv/dregsy/cmd/dregsy@latest +retry "dregsy install" go install github.com/xelalexv/dregsy/cmd/dregsy@latest cp /root/go/bin/dregsy /usr/local/bin/dregsy -echo "dregsy: $(go version -m /usr/local/bin/dregsy | awk '$1 == "mod" { print $3; exit }')" +DREGSY_MOD="$(go version -m /usr/local/bin/dregsy | grep -m1 -E '^[[:space:]]*mod[[:space:]]' || echo 'build info unavailable')" +echo "dregsy: $${DREGSY_MOD}" # ── regsync ─────────────────────────────────────────────────────────────────── echo "--- Installing regsync" -go install github.com/regclient/regclient/cmd/regsync@latest +retry "regsync install" go install github.com/regclient/regclient/cmd/regsync@latest cp /root/go/bin/regsync /usr/local/bin/regsync -echo "regsync: $(regsync version 2>&1 || true)" +REGSYNC_VERSION_LINE="$(regsync version)" +echo "regsync: $${REGSYNC_VERSION_LINE}" # ── Clean up Go build cache ────────────────────────────────────────────────── @@ -156,17 +179,37 @@ rm -rf /root/go/pkg/mod/cache /root/.cache/go-build echo "--- Cloning and building ocync (as ec2-user)" +# Cloning is separate from building so it can retry. It clones to a scratch +# path and moves it into place, so a retry after a partial clone is idempotent +# without ever deleting an existing checkout: that path holds the git-tracked +# run-record archive, and re-running this script through `cloud-init clean` +# is a normal way to retry a bootstrap. The build is not retried, since a +# compile failure is deterministic and cargo retries its own fetches. +retry "ocync clone" su - ec2-user -c ' + set -e + rm -rf $HOME/.ocync-clone + git clone https://github.com/clowdhaus/ocync.git $HOME/.ocync-clone + [ -e $HOME/ocync ] || mv $HOME/.ocync-clone $HOME/ocync + rm -rf $HOME/.ocync-clone +' + +# set -e inside the block: without it only the last command's status escapes, +# so a failed cargo build would be masked by the cp that follows it. su - ec2-user -c " + set -e source \$HOME/.cargo/env - git clone https://github.com/clowdhaus/ocync.git \$HOME/ocync cd \$HOME/ocync cargo build --release --package ocync --package bench-proxy cp target/release/ocync \$HOME/.cargo/bin/ocync cp target/release/bench-proxy \$HOME/.cargo/bin/bench-proxy " -echo "ocync: $(su - ec2-user -c '/home/ec2-user/.cargo/bin/ocync version')" -echo "bench-proxy: built" +# Assigned rather than echoed inline: a command substitution inside an echo +# argument yields echo's status, so set -e can never fire on a failed probe. +OCYNC_VERSION="$(su - ec2-user -c '/home/ec2-user/.cargo/bin/ocync version')" +echo "ocync: $${OCYNC_VERSION}" +BENCH_PROXY_PATH="$(su - ec2-user -c 'command -v /home/ec2-user/.cargo/bin/bench-proxy')" +echo "bench-proxy: $${BENCH_PROXY_PATH}" # ── Generate bench-proxy CA and install into system trust store ────────────── diff --git a/xtask/src/bench/mod.rs b/xtask/src/bench/mod.rs index 7e0fde91..39dfb2b2 100644 --- a/xtask/src/bench/mod.rs +++ b/xtask/src/bench/mod.rs @@ -299,12 +299,20 @@ pub(crate) async fn run(args: BenchArgs) -> Result<(), Box = BTreeMap::new(); let mut relay_versions: BTreeMap = BTreeMap::new(); for &tool in &tools { - let version = runner::check_tool(tool).await?; + let version = runner::check_tool(tool, workspace_root).await?; eprintln!(" {}: {version}", tool); tool_versions.insert(tool.to_string(), version); @@ -314,12 +322,6 @@ pub(crate) async fn run(args: BenchArgs) -> Result<(), Box/dev/null; then fi if [ ! -d ~/ocync ]; then - echo "bench-remote: error: ~/ocync not found after cloud-init. Check user-data logs:" - echo " ssh $USER@$(hostname -I | awk '{{print $1}}') 'sudo cat /var/log/cloud-init-output.log | tail -50'" + echo "bench-remote: error: ~/ocync not found after cloud-init. Check the bootstrap log:" + echo " ssh $USER@$(hostname -I | awk '{{print $1}}') 'sudo tail -50 /var/log/user-data.log'" exit 1 fi diff --git a/xtask/src/bench/runner.rs b/xtask/src/bench/runner.rs index 04c8cd15..fb4cba69 100644 --- a/xtask/src/bench/runner.rs +++ b/xtask/src/bench/runner.rs @@ -179,10 +179,16 @@ async fn probe_version( ) } +/// Version Go stamps into a binary built from a local working tree. +/// +/// It names no released artifact, so it is not a usable record of what ran. +const DEVEL_VERSION: &str = "(devel)"; + /// Extracts the module version from `go version -m` output. /// /// The build info lists one `mod` line holding the main module's path and -/// version, followed by a `dep` line per dependency. +/// version, followed by a `dep` line per dependency. Returns `None` for a +/// binary built from a working tree, whose stamped version identifies nothing. fn module_version_from_build_info(output: &str) -> Option<&str> { output.lines().find_map(|line| { let mut fields = line.split_whitespace(); @@ -190,30 +196,71 @@ fn module_version_from_build_info(output: &str) -> Option<&str> { return None; } fields.next()?; // module path - fields.next() + fields.next().filter(|v| *v != DEVEL_VERSION) }) } +/// Explains why a `go version -m` probe yielded no module version. +/// +/// The probe runs under `sh`, so a missing Go toolchain and a binary missing +/// from `PATH` both surface as a non-zero exit rather than a spawn failure. +/// The tool's own text separates them, so it is passed through verbatim. +fn no_module_version_reason(success: bool, stdout: &str, stderr: &str) -> String { + if success { + return "build info carries no released module version".to_string(); + } + + let detail = stderr + .lines() + .chain(stdout.lines()) + .find(|l| !l.trim().is_empty()) + .unwrap_or("no output") + .trim(); + format!("go version -m failed: {detail}") +} + /// Reads the module version stamped into a Go binary on `PATH`. /// -/// This is the only version several of these tools carry. `go install` sets -/// none of the ldflags a project's release build uses, so a tool that reports -/// its version from a linker-injected variable reports a placeholder, and -/// dregsy has no version flag at all. -async fn go_module_version(binary: &str) -> Option { +/// Returns the reason on failure rather than swallowing it, so a caller can +/// say what it recorded instead. Callers differ: one falls back to asking the +/// binary, the other records that no version is known. +async fn go_module_version(binary: &str) -> Result { let output = tokio::process::Command::new("sh") .arg("-c") .arg(format!("go version -m \"$(command -v {binary})\"")) .output() .await - .ok()?; + .map_err(|e| format!("could not run sh: {e}"))?; - if !output.status.success() { - return None; + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + + match module_version_from_build_info(&stdout) { + Some(version) => Ok(version.to_string()), + None => Err(no_module_version_reason( + output.status.success(), + &stdout, + &stderr, + )), } +} - let stdout = String::from_utf8_lossy(&output.stdout); - module_version_from_build_info(&stdout).map(str::to_string) +/// Resolves the binary a tool is executed as. +/// +/// ocync runs from the workspace release build rather than `PATH`, since that +/// is what `build_ocync` just produced. A `PATH` lookup would find whatever +/// was installed when the instance was created, which can be months older, so +/// version probes must resolve the same way runs do or the record names a +/// binary that produced none of the numbers. +fn tool_path(tool: Tool, workspace_root: &Path) -> std::borrow::Cow<'static, str> { + match tool { + Tool::Ocync => workspace_root + .join("target/release/ocync") + .to_string_lossy() + .into_owned() + .into(), + Tool::Dregsy | Tool::Regsync => tool.binary().into(), + } } /// Runs a benchmarked tool's version probe. @@ -223,16 +270,23 @@ async fn go_module_version(binary: &str) -> Option { /// fallback keys off the probe reporting nothing rather than off a sentinel /// string, so a tool that prints the word `unknown` is still recorded as /// having reported it. -pub(crate) async fn check_tool(tool: Tool) -> Result { - if let Some(version) = - probe_version(tool.binary(), version_args(tool), reports_version(tool)).await? +pub(crate) async fn check_tool(tool: Tool, workspace_root: &Path) -> Result { + let binary = tool_path(tool, workspace_root); + + if let Some(version) = probe_version(&binary, version_args(tool), reports_version(tool)).await? { return Ok(version); } - Ok(go_module_version(tool.binary()) - .await - .unwrap_or_else(|| UNKNOWN_VERSION.to_string())) + match go_module_version(&binary).await { + Ok(version) => Ok(version), + Err(reason) => { + eprintln!( + "WARNING: no version for {binary}: {reason}. Recording \"{UNKNOWN_VERSION}\"." + ); + Ok(UNKNOWN_VERSION.to_string()) + } + } } /// Returns the version of every binary `tool` relays its transfers through. @@ -246,13 +300,20 @@ pub(crate) async fn check_relays(tool: Tool) -> Result, St // credential helper prints "development" whatever it was built from, // because `go install` sets none of its Makefile's ldflags. let version = match go_module_version(binary).await { - Some(version) => version, - // Still probe, so a missing binary fails here rather than midway - // through a run. A relay need not accept `--version`, so a - // non-zero exit is not itself an error. - None => probe_version(binary, &["--version"], false) - .await? - .unwrap_or_else(|| UNKNOWN_VERSION.to_string()), + Ok(version) => version, + Err(reason) => { + // Still probe, so a missing binary fails here rather than + // midway through a run. A relay need not accept `--version`, + // so a non-zero exit is not itself an error. Announced, + // because this is the weaker source and may be a placeholder. + eprintln!( + "WARNING: no module version for {binary}: {reason}. \ + Falling back to asking the binary." + ); + probe_version(binary, &["--version"], false) + .await? + .unwrap_or_else(|| UNKNOWN_VERSION.to_string()) + } }; versions.push((binary.to_string(), version)); } @@ -351,16 +412,7 @@ pub(crate) async fn run_tool( Tool::Regsync => vec!["once", "-c", &config_str], }; - // Use the workspace release binary for ocync (just built by build_ocync), - // PATH lookup for external tools (dregsy, regsync). - let binary: std::borrow::Cow<'_, str> = match tool { - Tool::Ocync => workspace_root - .join("target/release/ocync") - .to_string_lossy() - .into_owned() - .into(), - _ => tool.binary().into(), - }; + let binary = tool_path(tool, workspace_root); // For ocync, set OCYNC_TIMING_FILE so the engine writes phase timing JSONL. let timing_path = if tool == Tool::Ocync { @@ -608,6 +660,46 @@ mod tests { ); } + #[test] + fn failure_reason_passes_through_the_tools_own_text() { + // The probe runs under sh, so a missing Go toolchain and a binary + // missing from PATH both arrive as a non-zero exit. Only the tool's + // text tells them apart, so it must survive verbatim. + let missing_go = no_module_version_reason(false, "", "sh: go: command not found"); + assert!(missing_go.contains("command not found"), "{missing_go}"); + + let missing_binary = + no_module_version_reason(false, "", "stat : no such file or directory"); + assert!( + missing_binary.contains("no such file or directory"), + "{missing_binary}" + ); + + // Go writing its diagnostic to stdout must not read as "no output". + let on_stdout = no_module_version_reason(false, "some diagnostic", ""); + assert!(on_stdout.contains("some diagnostic"), "{on_stdout}"); + } + + #[test] + fn failure_reason_distinguishes_a_binary_without_build_info() { + let reason = no_module_version_reason(true, "", ""); + assert!( + reason.contains("no released module version"), + "a zero-exit probe with no mod line is not a probe failure: {reason}" + ); + } + + #[test] + fn locally_built_binary_reports_no_module_version() { + // Go stamps (devel) for a build from a working tree. It names no + // released artifact, so recording it would give a run's provenance the + // same authority as a real version. + let devel = "/usr/local/bin/dregsy: go1.26.2\n\ + \tmod\tgithub.com/xelalexv/dregsy\t(devel)\t\n"; + + assert_eq!(module_version_from_build_info(devel), None); + } + #[test] fn module_version_absent_without_a_mod_line() { // `go version -m` writes nothing to stdout for a non-Go binary, and @@ -622,6 +714,21 @@ mod tests { assert_eq!(module_version_from_build_info(deps_only), None); } + #[test] + fn ocync_is_probed_at_the_binary_the_run_executes() { + // A PATH lookup would find the ocync installed when the instance was + // built, which can be months older than the one just compiled, and + // the record would credit it with HEAD's numbers. + let root = Path::new("/w"); + assert_eq!( + tool_path(Tool::Ocync, root), + "/w/target/release/ocync", + "ocync must resolve to the workspace build, matching run_tool" + ); + assert_eq!(tool_path(Tool::Dregsy, root), "dregsy"); + assert_eq!(tool_path(Tool::Regsync, root), "regsync"); + } + #[test] fn relays_cover_every_binary_that_moves_a_tools_numbers() { // dregsy delegates every transfer to skopeo, and both Go tools reach