fix(bench): fail bootstrap loudly and stop recording unusable versions - #119
Merged
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three paths in the bootstrap made a failure invisible, and unpinning the tools made hitting them likelier by moving the trigger from an operator editing a pin to an upstream push. Nothing waits on cloud-init, so
terraform applyreports success either way and the failure surfaces much later as a missing binary.The rustup install was the worst of them.
su -c 'curl … | sh -s'returns the pipeline's last status,pipefaildoes not cross thesuboundary, and the installer exits zero on the EOF a failed curl feeds it. So the fetch could fail and the retry would report success on the first attempt, having installed nothing. Measured: exit 0 withoutpipefail, exit 6 with it. It is also the one failure that escaped the downstream guard, because the ocync clone needs no Rust and still succeeds, sobench-remote's[ ! -d ~/ocync ]check passes.The build block ran without
set -e. Only the final command's status escapes a multi-commandsu -c, so a failedcargo buildwas masked by thecpafter it, andbench-proxycan link even when the ocync crate fails. Separately, every verification line was a command substitution inside anechoargument, which yieldsecho's status, so no sanity check in the script could fail it. Those are assignments now.Every network fetch retries, including the
dnfcalls a cold instance hits first, which previously had none because the helper was defined below them. The final attempt no longer claims it is retrying.The clone no longer removes an existing checkout. I introduced that in #118 for retry idempotency, and it was wrong: that path holds the git-tracked run-record archive, and re-running the bootstrap through
cloud-init cleanis a normal thing to do. It clones to a scratch path and moves it into place instead.On the recording side: a binary built from a working tree stamps
(devel), which names no release, so it is no longer read as a version. Every path that ends in a recordedunknownnow says why, and says which fallback it took rather than asserting an outcome it does not control — the previous warning claimedRecording "unknown"even where the caller went on to record a real version. The bootstrap no longer parses build info in awk, which duplicated the Rust parser and disagreed with it on exactly the(devel)case the new rule exists for.One pre-existing bug in the same area:
check_toolprobedocyncthroughPATHwhilerun_toolexecutestarget/release/ocync, and the probe ran before the build. On a directcargo xtask benchthat records the release installed at instance creation, potentially months old, against numbers produced by freshly built HEAD. The build now happens first and both resolve the same way.CI is the gate. Locally
cargo fmt,clippy -D warnings,check --no-default-features --features non-fips,cargo deny,terraform fmt,terraform validate,bash -nand 151 xtask tests pass. Theocync-distributioncontainer tests fail on my machine on container startup and this diff cannot reach that crate.