fix(testreport): fail an update whose host could not work out what to patch - #452
fix(testreport): fail an update whose host could not work out what to patch#452plusky wants to merge 5 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #452 +/- ##
==========================================
+ Coverage 96.28% 96.29% +0.01%
==========================================
Files 193 193
Lines 42939 43806 +867
==========================================
+ Hits 41343 42183 +840
- Misses 1596 1623 +27 ☔ View full report in Codecov by Harness. |
c70a9af to
9fc823c
Compare
|
Rebased onto the updated #446 (
Also dropped the refresh-residual paragraph #446 added to this module's docs: it duplicates the better-cited section already here, and on this branch it is partly wrong — the probe guard means a refresh failure that hides the issue repo now resurfaces on the patches probe instead of exiting Gates on the rebased tree: fmt, clippy |
mimi1vx
left a comment
mimi1vx
left a comment
There was a problem hiding this comment.
Reviewed the top commit 9fc823cb only — the two below it are #446.
Verified locally in a worktree at 9fc823cb: cargo test -p mtui-testreport
(299 lib + 151 it) passes, cargo clippy -p mtui-testreport --all-targets -D warnings is clean, cargo fmt --all --check is clean. The new shell-level
tests do execute the rendered templates under a real /bin/sh.
The core fix is correct. The pipeline-status bug is real, per-line capture with
$? read on the following line is the right POSIX lever, and the routing
rewrite is equivalent to the old all(lastexit == -1) rule on any run without a
probe failure — any(!probe_failed && lastexit != Some(-1)) reduces to
any(lastexit != Some(-1)) when no error carries the flag, the host: None
case included. The UpdateFailure match in perform_update_with_rollback is
exhaustive, so the new variant cannot fall into a rollback arm by accident.
Two things to resolve before this lands.
Rejecting 106 re-incurs the cost that leaving refresh unguarded avoids
crates/mtui-testreport/src/update_workflow/actions/update.rs:100-112
The module docs argue zypper -n refresh must stay unguarded because a refhost
routinely carries one unrelated stale/unsigned/unreachable repo alongside
working ones, and aborting there would kill updates that are fine. The same
docs then argue 106 must be rejected on the probe because zypper -n patches
goes through init_repos().
But init_repos() sets ZYPPER_EXIT_INF_REPO_SKIPPED for any skipped repo
and cannot say which — as lines 80-82 state. So on exactly that refhost the
update now aborts one line later at update.rs:190 with "could not determine
what to patch". The premises are in tension: leaving refresh unguarded buys
nothing if the next command hard-fails on the same condition.
Conditions for impact: any host with at least one repo whose metadata cannot be
loaded, where the issue repo itself is fine. Those hosts patch successfully
today and will need manual per-host repo cleanup after this change. The blast
radius is the refhost fleet rather than one code path, which is why this is
worth settling now.
The alternative the old module docs proposed resolves it without giving up the
detection: treat 106 as a warning, then assert the issue alias is actually
present (repo_manager.rs::issue_alias in zypper -n lr). That tests the thing
the guard cares about — the issue repo is there — instead of a status that
cannot distinguish it.
Note I could not verify zypper's exact 106 conditions from here; the
detection claim in the commit message depends on them holding as described.
aggregate.cancelled is unreachable and unrelated to #447
crates/mtui-testreport/src/reports/update_flow.rs:458
aggregate.cancelled = failures.iter().all(|e| e.cancelled);Every UpdateError::cancelled construction in this file is an early
return Err(...) — lines 553, 983, 1383, 1527, 1585. None is pushed into a
failures vec that reaches aggregate_failures. Check functions and
host_command_failures never set the flag either, so in production this
expression can only evaluate to false. Its only exercise is the new unit test
calling the private function directly (line 2535).
If it did become reachable, it would newly route a multi-failure aggregate to
CommandError::Cancelled through map_flow_error — a behavior change riding
in a bugfix commit, and one that conflicts with the guidance that a cancel
verdict must not mask a real host failure that merely coincided with it. Drop
the line or split it into its own commit. The probe_failed line beside it is
load-bearing and should stay.
Suggestions
The removed "residual" section is narrowed, not closed.
actions/update.rs:109 — "The case it would have caught is not lost" overstates
it. Repos are added with zypper ar and no -f (mtui-hosts
target/repo_manager.rs), so patches will not autorefresh them. If the issue
repo has a stale-but-present cache — a re-run of update after an earlier
successful prepare, with the repo now unreachable — zypper -n patches
answers 0 from cache, the list lacks the patch, and the script exits 0: the
original silent no-op. The dominant case (freshly added repo, no cache,
unreachable → 106) is genuinely covered. A one-line residual note would be
more accurate than dropping the section.
Anchor the probe-failure match to line start.
crates/mtui-testreport/src/update_workflow/checks/update.rs:342 — the gate is
a substring match over all of stdout and stderr.
args.stdout.lines().any(|l| l.starts_with(PROBE_FAILURE_MARKER)) costs the
same and removes the residual false-positive class the split-printf trick
does not cover: any host-side output reproducing the sentence mid-line.
the_probe_marker_is_matched_in_full already pins the prefix-weakening
direction; this pins the other one.
Hedge the ARG_MAX claim. actions/update.rs:50-51 — "no ARG_MAX limit
sits between them" holds only while printf resolves to a builtin. True for
dash, bash and busybox, so fine in practice, but the doc states it
unconditionally. A /usr/bin/printf resolution with a large patch table gives a
false "could not determine what to patch" — fail-safe direction, worth one
hedging word.
Rebase. Stacked on #446; assert_status_comes_from_the_patch touches lines
#446 introduces, so expect a conflict there specifically.
…emplates (openSUSE#400) The zypper and SL Micro update templates are multi-line scripts run as a single remote command, and their last line was the repo-cleanup loop — whose status is 0 whenever it finds nothing to remove. The shell reported that, and the patch's own status was discarded before anything could read it. A patch that genuinely failed with clean output passed the update check. Each template now captures the patch's status on the line after the patch, before the post-state `zypper patches` line can clobber it, and exits with it. The list of patches is captured first and the patch runs only when that list holds at least one word, so a host carrying none of the update's products is a no-op rather than a package manager complaining about its own arguments — a status that is now real. The emptiness test is `set --` plus `[ "$#" -gt 0 ]`, not `[ -n ... ]`: the latter is true for a list of whitespace, which then splits to no words at all. The check classifies the status rather than comparing it against zero. That distinction is the whole safety of this change: an update check failure drives the group-wide rollback downgrade, which reverts every host in the group and not just the one that reported, so a host that patched perfectly must not fail. zypper's informational codes pass — including 102 ("reboot needed"), the routine outcome of patching a kernel, and 107, where a package's %post script failed although the package itself is installed and registered. The classification lives in one place rather than being spelled out a fourth time. On SL Micro it is in practice just "0 passes": upstream transactional-update absorbs zypper's status and returns only 0 or 1. The templates and the check must move together — a template reporting the real status while the check still lacked the carve-out would newly fail every host that merely needs a reboot. The rendered command text changes, so `show_log` transcripts change with it. The docs that explained why the exit code could not be trusted are rewritten rather than left to contradict the code.
zypper exits 107 (ZYPPER_EXIT_INF_RPM_SCRIPT_FAILED) when a package's %post script returns an error although the package itself was, in the man page's words, "successfully unpacked to disk and registered in the rpm database". It is one of the informational codes above 100, but the install check's success set stopped at 106, so a routine kernel or dracut scriptlet hiccup was reported as "Unknown Error". Found while giving the update check the same carve-out (openSUSE#400), where the identical omission was worse: there a false failure fires the group-wide rollback downgrade.
…s label (openSUSE#400) Review of openSUSE#446 caught that short-circuiting the whole `104 | 4 | 5 | 8` class to "package not found" degrades the two failures the check exists to surface. Only `104` (`ZYPPER_EXIT_INF_CAP_NOT_FOUND`) means the capability was not found; `4`, `5` and `8` are `ERR_ZYPP`, `ERR_PRIVILEGES` and `ERR_COMMIT` — the package was found and the transaction failed. An `8` with `Error:` on stderr is the likeliest status for a genuinely failed patch, and a `5` with `System management is locked` for a busy update stack, so the label replaced both headline diagnoses with one that is not vaguer but wrong. So only `104` now outranks the markers. The other three consult them first and keep "package not found" solely on a clean transcript. The justification for the old ordering does not survive scrutiny: nothing branches on these strings — they are rendered, logged and asserted on, never matched — and on `update` the three previously reached no verdict at all, falling through to the markers, so there was no earlier behaviour for the short-circuit to preserve. The class itself stays the install check's, unsplit, so an exit code still cannot land in two classes; what a check *says* about a member is its own to choose. The install check keeps its ordering, so the same transcript can now read differently on the two. That divergence is pinned on both sides rather than left to be discovered: install's own short-circuit was only ever exercised with an empty transcript, so hoisting its markers above the set broke no test. `-1` gains `ExitClass::NotRun` rather than falling through to `Unknown`, which reported a host mtui never contacted as a failed patch. It is the message that was wrong, not the routing: `never_ran` vetoes the group-wide rollback from the target's recorded `lastexit()`, never from a reason string, so a `-1` host skipped the downgrade under the old fallthrough too. The variant also makes the distinction structural — the one match on this enum is exhaustive — and the sentinel's reason now has a single source, so the gate and the classifier arm cannot drift. The dependency-prompt marker now logs the command and stderr like its siblings. It was the one branch leaving no forensic record, which did not matter while it could only follow a success or an unrecognised status, and does now that it is reachable on the three codes an operator is most likely to be investigating. Two costs are stated where they are decided rather than left implicit: the slmicro key fails on any non-zero status, and `transactional-update` flattens an open transaction or a failed snapshot into the same `1` as a failed patch, so either downgrades and reboots every host in the group — kept, because the alternative is reporting a failed patch as a successful update. And an unguarded `zypper -n refresh` remains the one path to a clean exit with nothing installed, deferred because `refresh` returns `4` whether one repository failed or all of them did, so closing it needs a check that the issue repo is present rather than a status test. Also corrects the `UpdateError` doc, which called the reason strings a contract consumed by callers while nothing reads them; the contract is the `UpdateFailure` variant a failure routes to.
… patch (openSUSE#447) The patch list was built by a pipeline, and a pipeline reports its last stage's status. That stage is awk, which succeeds on empty input — so zypper's status was not merely discarded, it was unobservable. A ZYpp lock, a broken repo or an unreadable rpmdb produced an empty list, skipped the patch, and exited 0: an update reported as applied that installed nothing. Each probe now runs on its own line so its status can be read, and the rows are fed to awk through the printf builtin. A probe that fails prints a marker and exits with its own status — no invented sentinel, since a POSIX exit truncates to 0..255 and any chosen value would collide with the package manager's own space. awk is guarded too: it is the stage that actually produces the list, and it failed silently in exactly the same way. The probe accepts only 0, deliberately not the patch's informational set. Those codes are argued from "the transaction committed", which is a claim about the patch; on a metadata probe 106 means a repository was skipped, and if the skipped one is the issue repo the list is legitimately missing the patch we came to install. A host carrying an unrelated broken repo now fails instead of patching, which is the cost of not silently patching nothing. `zypper -n refresh` is deliberately not guarded. It returns 4 whether one repository failed or all of them did, so its status cannot separate "the issue repo went missing" from "an unrelated stale repo did", and guarding it would abort the update on a refhost that would have patched correctly. A refresh failure that really does hide the update resurfaces on the patches probe, which refreshes through the same path. The failure does not roll back. A host that could not determine what to patch never ran a patch, so nothing is half-applied for a downgrade to undo, and the rollback is group-wide — it would revert every healthy host in the group. That routes through a typed flag rather than the reason text. A run mixing this with a real check failure still rolls back, on behalf of the host the rollback can repair. A host carrying none of the update's products is still a no-op, not a failure.
…enSUSE#447) Review of openSUSE#452 caught that rejecting `106` on the patch-list probe re-incurs exactly the cost that leaving `zypper -n refresh` unguarded exists to avoid. `init_repos()` raises `ZYPPER_EXIT_INF_REPO_SKIPPED` for *any* skipped repository and cannot say which, so a refhost carrying one stale or unreachable repo alongside working ones — routine, and patching correctly today — would have aborted one line after the unguarded refresh let it through. The two rules cancelled out, and the fleet rather than one code path would have paid for it. So the status is no longer asked to carry a verdict it cannot support. On `106` the script asks the question the guard actually cares about — is the update's own repository still listed? — with `mtui_issue_repo_present`, which greps `$repa` out of `zypper -n lr`, the same selector and the same listing the cleanup loop already matches on. Repo present: the skipped one was somebody else's, so note it in the transcript and patch. Repo absent: the list cannot be trusted, and the probe fails as before. That presence test is a pipeline, and here the last-stage rule is wanted rather than worked around — awk's `END { exit !found }` is the verdict — so an `lr` that fails outright reads as absent and fails the probe, the conservative direction. A host carrying none of the update's products is untouched: no repo was added for it, `patches` answers `0`, no `106` arises, and the empty list stays the no-op it always was. The alias question is only ever asked on the `106` path, so it cannot turn that no-op into a failure. The probe marker is now matched at the start of a line rather than anywhere in the stream. Splitting it across `printf`'s format string and its argument keeps the script's own text from carrying the sentence, but cannot stop a host echoing it mid-line — a `set -x` trace, a log line quoting an earlier run. Only a line that begins with it is mtui's own verdict. Drops `aggregate.cancelled`, which was dead and unrelated to openSUSE#447: every cancellation in this module is an early `return Err`, so no cancelled error reaches a `failures` vec. Were it ever to become reachable it would newly route a multi-failure aggregate to `CommandError::Cancelled`, letting a cancel mask a real host failure that merely coincided with it. Also records the residual the removed-and-restored "not lost" claim overstated: repos are added without `-f`, so `patches` does not autorefresh them, and a repo whose cache is stale but present still answers `0` with the patch absent. Closing that needs a freshness assertion, not a status test. And the `ARG_MAX` claim is hedged to the builtin `printf` it depends on.
9fc823c to
c4dbb26
Compare
|
Both blocking items addressed in
|
2 participants
Closes #447.
Stacked on #446 — it edits the lines that PR introduces. Review the top commit; the two below are #446. I'll rebase once that merges.
The bug
The patch list was built by a pipeline, and a pipeline reports its last stage's status — awk's, which succeeds on empty input. zypper's status was not merely discarded, it was unobservable there. A ZYpp lock, a broken repo or an unreadable rpmdb gave an empty list, skipped the patch, and exited 0: an update reported as applied that installed nothing.
The fix
Each probe runs on its own line so its status can be read, and the rows reach awk through the
printfbuiltin. A failed probe prints a marker and exits with its own status — no invented sentinel, since a POSIXexittruncates to 0..255 and any chosen value would collide with the package manager's own space. There is no precedent in the tree for a template exiting with a status of its own choosing, and this does not add one.awk is guarded too. It is the stage that actually produces the list, and it failed in exactly the same silent way — the first review round found #447 still reachable through it.
Two judgement calls, both against the obvious choice
The probe accepts only
0, not the patch's informational set. Those codes are all argued from "the transaction committed", which is a claim about the patch. On a metadata probe106means a repository was skipped — and if the skipped one is the issue repo, the list is legitimately missing the patch we came to install. So the sets are deliberately different, and the code says so, so nobody unifies them later. The cost is real and stated in the CHANGELOG: a host carrying an unrelated broken repo now fails rather than patching.zypper -n refreshis deliberately not guarded. My first draft did guard it. Review caught thatrefresh.cc:337-345returns4whether one repository failed or all of them did, and that the explicitrefreshcommand never returns106(that is set only byinit_reposinside an operation command). So its status cannot separate "the issue repo went missing" from "an unrelated stale repo did", and guarding it would abort the update on a refhost that would have patched correctly — routine on QAM refhosts. The case is not lost: a refresh failure that really does hide the update resurfaces on the patches probe, which refreshes through the same path.The failure does not roll back
A host that could not determine what to patch never ran a patch. Nothing is half-applied for a downgrade to undo, and the rollback is group-wide — it would revert every healthy host in the group. New
UpdateFailure::ProbeFailed, selected from a typed flag onUpdateError, never from the reason text.ProbeFailedrather than wideningNotRunbecause the command did run to completion and did produce a verdict, soNotRun's "absence of a verdict, state unknown" doc would have become false.The predicate is now
any(repairable)→Check, elseall(probe_failed)→ProbeFailed, elseNotRun. On any run with no probe failures this is provably identical to the oldall(lastexit == -1)form, so the documented rule that a mixed run still rolls back on behalf of the host it can repair is unchanged. One case did change: a run mixing a probe failure with a lost host no longer rolls back, since neither host has anything to repair.Testing
Rendered templates are executed under a real
/bin/shwith stubs — the suite'sMockConnectionscripts one exit code per command and cannot model a shell's last-command-wins rule. 40 mutations applied, observed red and reverse-restored; each red is credited by a panic message unique to its assertion.The legitimate empty case — a host carrying none of the update's products — is pinned as first-class at both script and check level. It stays a no-op, not a failure.
Known limitations, stated rather than hidden
\>is a GNU awk extension. SUSE refhosts ship gawk, but a non-GNU awk would match nothing and skip silently; the module doc now says so, and says the fixture row is dialect-neutral so nobody mistakes the shell tests for awk coverage.zypper -n patchesnow runs twice per update, so the transcript's list is not literally the list that was patched.SLM_UPDATEhas no refresh at all, so stale metadata on SL Micro is a silent no-op no guard can see.