feat(baremetal): one command for what quietly breaks a server - #259
Draft
Denis-hamon wants to merge 27 commits into
Draft
feat(baremetal): one command for what quietly breaks a server#259Denis-hamon wants to merge 27 commits into
Denis-hamon wants to merge 27 commits into
Conversation
…to change
`vps service-info edit myvps --renew-period 12` used to send this:
{"renew":{"automatic":false,"deleteAtExpiration":false,"forced":false,
"manualPayment":false,"period":12}}
The renewal settings are booleans bound to a shared struct carrying no
`omitempty`, so every one of them was marshalled at its zero value and won the
merge against the fetched resource. Changing the renewal period therefore also
switched automatic renewal off — on a service that had been renewing itself for
years, without a word in the output saying so.
Webhosting already built its payload from `cmd.Flags().Changed`, and did not
have the defect. This promotes that builder into `common`, so `vps` gets it too
and the next `service-info edit` cannot reintroduce the bug by reaching for the
struct.
Reading `Changed` rather than the values keeps `--renew-automatic=false`
working: pflag records a flag as changed whatever value it was given, so an
explicit false is still sent while an absent flag stays absent. Both cases are
covered by a test, and each test was checked against the failure it exists to
catch.
The shared mutable `ServiceInfoSpec` goes away with the last thing that read
it, and the five flag registrations repeated across four commands become one
call, which also settles the two spellings of the period's help text.
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
…ayer Declaring cobra flags is the command layer's job, and internal/services/common was the only service package doing it — the shared flag helpers all live in internal/cmd. Raised in review of this PR. The registration moves; the table does not. Both halves need the flag name — one to declare it, the other to read whether the operator set it — and that name is the only thing tying them together. Splitting it into two copies would mean a rename could touch one side and leave the other silently no longer sending a setting, which is the exact failure this PR exists to fix. So common.ServiceInfoRenewFlags becomes the exported description, internal/cmd registers from it, and the payload builder keeps reading it. Checked by renaming an entry in that table and watching the service-info tests fall: the two halves still move together. Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
What silently breaks a dedicated server is spread across five routes and none of them is where somebody would look. A machine left on the rescue system runs no service and says nothing. Monitoring switched off means OVHcloud will not call when a disk dies. A renewal switched off means the server goes back at expiry. `doctor` reads them together, on one server or on every server of the account. On a real fleet: 35 servers, 8.3 seconds, 27 findings — six machines booted on a rescue system, eleven with monitoring off, one refusing hardware intervention, five whose renewal state is not what it looks like. Testing bootType == "rescue" finds only half of them. Three of the six carried bootType "internal" on a boot entry whose kernel is "rescue-customer" and whose description ends with "[REMOVAL ON 2025-06-23]" — a rescue image retired fourteen months ago. The type is the API's and it is wrong for that entry; the kernel is not. Both are read, and the retired image is its own finding. And renew.automatic does not hold still. Twenty sequential reads of the same serviceInfos object returned different values on five servers of the eight tested — 10/10, 9/11, 8/12, 6/14, 5/15 — with the domain field proving each answer belonged to the server asked about, while monitoring on the server object was stable ten out of ten. The first version of this command read it once and reported "automatic renewal is off" on three servers whose renewal was on. It reads five times now: when the readings disagree, that is the finding, because it is both true and the more serious problem; when they agree, the finding says how it was established and asks for confirmation, since this route cannot give certainty. Three readings agree by chance one time in four, five about one in sixteen. Two rules the command is built on. A server that could not be read is not a healthy server, so one unreadable server withholds the clean bill of health from the whole run — this thing exists to be trusted when it says nothing is wrong. And the exit code stays 0 when findings are reported, because the command ran and answered; --strict is for the pipeline gating on it and has to be asked for. Every command named in a fix was checked against this binary's command tree. The first draft sent operators to set-boot, list-boots and list-planned-interventions, none of which exist. Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…them Under `-o json --strict`, doctor rendered the table and then called OutputError. Those are two documents on one stdout, and only the last one survives: the pipeline that asked for --strict — the only caller the flag exists for — received "27 finding(s) on 35 server(s)" where it had come for the 27 findings. The flag was destroying exactly the output its user wanted. --strict now changes the exit code and nothing else. The table is the answer; the exit status is the verdict. Found by a third-party review pass over this branch, then confirmed by running the command: the JSON was well-formed, complete and about nothing. Two things surfaced while fixing it. The first draft guarded it as `if DoctorStrict && len(findings) > 0`, which cannot be false — the no-finding case returns earlier in the function, so that half of the condition was dead code and the test written to cover it exercised a path nothing reaches. The guard now tests DoctorStrict alone, and the test pins the order that actually matters: moving the exit above the early return would fail a healthy fleet, and that is what goes red. And ExitFunc existed only in the non-wasm half of display. This is the first caller of it from outside that package, so the wasm build stopped compiling while every test stayed green — the same trap ovh#244 hit when OutputRaw was added to one implementation of display and not the other. The wasm counterpart is added here. It cannot call os.Exit, which would take the Go runtime down with the page, so it reports on ResultError the way OutputError already does there. ResultError and ResultString are separate globals, so the findings survive the failure signal — which is the same property this commit restores for stdout. Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A hostname, an address, an order id and a colleague's name read off a live account had been used as fixtures and as examples in comments. This repository is public: what goes in stays in. The values are replaced with synthetic ones of the same shape — RFC 5737 documentation addresses, hostnames built on them, identifiers of the same length — so the tests keep exercising the same parsing. This is the fourth time in this series that live account data reached a commit, after a live IP migration token, a test server's address, and a real invoice id. The first three were fixed one at a time as they were noticed; this is the sweep that should have followed the first one. Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
The first sweep replaced only the exact spelling. A test that checks a name can be copied back without reproducing its case carried the uppercase form of a real hostname, which therefore survived — and, once its neighbour was replaced, made that test fail. A real value does not stop being one because one of its spellings was removed. Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
This command exists to be believed when it says nothing is wrong, and it says so itself: a server that could not be read is not a healthy server, so one unreadable server withdraws the verdict from the whole pass. Only the first of five reads could do that. The boot entry, the renewal, the running tasks and the planned interventions each discarded their error, so a server whose four checks had all failed printed "✅ Nothing to report" — and under --strict exited 0, which is a pipeline reading four dead checks as a green gate. A check that could not run is now a finding of its own rather than a counter: it sorts with the rest, it is in -o json, --filter reaches it, and it makes the finding list non-empty, which is what mechanically removes both the clean bill and the zero exit. Nothing has to remember to consult a second list. It is a warning and not a critical, because a check that did not run is not evidence of a problem — it is the withdrawal of the guarantee, and the guarantee is the whole product of this command. The renewal check needed one more thing. It reads serviceInfos five times because that field disagrees with itself between consecutive reads of the same object, and three readings agree by chance about once in four. On a failure the read returned whatever it had collected, without saying so, and the check decided on it — for a failure on the second read, that is a verdict reached on a single coin toss, which is the exact thing those five readings exist to refuse. A short read is now no answer rather than a weaker one. Three tests. Two sabotages, both compiling: one that discards the four errors again, one that only makes the renewal vote permissive. Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
`baremetal edit --no-intervention=false` sent no such field. `omitempty` on a Go bool drops false, so the command-line map came out empty for that key; then EditResource reads the object, merges the map into it and PUTs the result, so the current true went straight back — and the command printed "✅ Resource updated successfully". Not a no-op: a no-op that claimed to have worked. This is the command the doctor names as the remedy for a server refusing hardware intervention, so the advice was inoperative and falsely confirmed. `--monitoring=false` had the same defect and looked healthy, because the only caller that names it happens to pass true. Both fields are pointers now, and EditBaremetal asks cobra whether the flag was typed rather than asking the value — a boolean has no absent value of its own. The assignment runs both ways round on every invocation: the wasm build keeps one process across commands and resets the flag's Changed bit between them, so a pointer left from an earlier edit would have sent a value nobody asked for, with "was it typed" answering no while the pointer said yes. Four tests, on the body of the PUT: the explicit false travels, for both fields; an untyped flag sends back what was read; and two commands in one process do not bleed into each other. Two compiling sabotages, one per half — the second was written only after the first version of these tests stayed green without the reset, which is what showed the reset needed its own test. Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
agreedRenewal returned the same (false, false) when the readings disagreed and when renew.automatic was simply not in the response, and checkRenewal had one branch for both. So a route that answered the same thing five times — nothing — was reported as "the API gave different answers about automatic renewal across 5 reads", which sends the operator looking for an instability that has not happened. Same for no readings at all. Three outcomes now, named: agreed, disagreed, absent. The absent case says what is true — this route does not return the field for this server — and keeps the same remedy, because the answer is still in the Manager. The unit test already covered "an absent field is not an agreement"; it could not have caught this, because absent and disagreed were the same value. It now distinguishes the three, and covers the empty slice. One compiling sabotage, merging absent back into disagreed: red on the unit test and on the end-to-end one. Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
ovh#243 moved the Cobra registration of the renewal flags out of internal/services/common and into the command layer, and exported the descriptor table so both halves keep reading one list. That commit landed after this branch had already taken its parent, so this branch still carried the version where a service package declares flags — and being the more recent side of the merge, it would have won and quietly undone the refactor. Merged rather than rebased: nothing is rewritten, so the review threads on this PR stay attached to their lines. Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
A --dry-run already carries the parameters in its message, and a
log.Println sitting just above the branch repeated the same JSON behind
a Go timestamp no other command in this CLI emits:
🔍 Dry run: nothing was sent. This would have been posted to …
{ "operatingSystem": "debian12_64" }
2026/08/23 23:47:22 Final parameters:
{ "operatingSystem": "debian12_64" }
The log line moves below the branch. A real run still logs what it is
about to send, which is what it was for; a dry run logs nothing, because
it sends nothing.
The line goes to stderr, so no assertion on stdout could ever have seen
it — which is why it survived every green run. Two tests now redirect the
logger: one that a dry run does not log, and its positive control that a
real run still does, so deleting the line outright would not pass.
Signed-off-by: Denis <denis.hamon@ovhcloud.com>
Mechanical, not a change of behaviour. Placing them just after TestBaremetalReinstallDryRun put them in the one region of baremetal_test.go that 23 downstream branches also append to, and in the import block they all touch as well. Merging the parent into those branches failed 23 times out of 23, every one of them on adjacency rather than on a disagreement. baremetal_test.go goes back to what it was before the previous commit, so this branch now leaves that file untouched. A new file can only clash with a file of the same name, and nothing else carries this one. Signed-off-by: Denis <denis.hamon@ovhcloud.com>
…' into feat/baremetal-doctor
Signed-off-by: Denis <denis.hamon@ovhcloud.com>
I4, from the review sandbox: --strict exited 1 on any finding, notes included. `expiry` reports a note for every server renewing inside the next 30 days, which on a real account is most of them, most of the time — so --strict was red permanently, and a gate that is always red is read exactly like no gate at all. The three severities existed and the exit code ignored them. --strict now fails on a warning or a critical. It reads the FILTERED rows, so --filter 'severity=="critical"' narrows the gate as well as the table, instead of failing on rows the caller asked not to see. The suite only ever exercised --strict against a warning, so nothing said what a note should do — which is how the behaviour stayed wrong and green. Two tests now pin both directions: a note alone does not exit, a warning beside the same note still does. Signed-off-by: Denis <denis.hamon@ovhcloud.com>
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.
What this adds
The things that quietly break a dedicated server are spread across five routes, and none of them is
where somebody would look. A machine left on the rescue system runs no service and says nothing;
monitoring switched off means OVHcloud will not call when a disk dies; a renewal switched off means
the server goes back at expiry.
With no argument it checks every server of the account. Stacked on #258; only the last commit
belongs to this pull request.
What it found on a real fleet
35 servers, 8.3 seconds, 27 findings:
Every one of these was verified against the API before being written as a check. Three more —
state != ok,powerState != poweron, planned interventions — fire on nothing here and areincluded because they cost one field each and are exactly what this command exists to catch on a
fleet that is not this one.
Two things the fleet had to be asked
Testing
bootType == "rescue"misses half of them. Six servers were booted on a rescue system:three carried
bootType: "rescue", and three carriedbootType: "internal"on a boot entrywhose kernel is
rescue-customerand whose description readsCustomer rescue system (Debian-10-based)[REMOVAL ON 2025-06-23]— a rescue image whose removal wasannounced fourteen months ago. The type is the API's and it is wrong for that entry; the kernel is
not. So both are read, and the retired image is its own finding.
renew.automaticdoes not hold still. Twenty sequential reads of the sameserviceInfosobjectreturned different values on five servers of the eight tested:
The
domainfield proved each answer belonged to the server asked about, andmonitoringon theserver object was stable ten out of ten — so it is that one field, on that one route. A health
check built on a single read of it reports a coin toss, and the first version of this command did
exactly that: it reported "automatic renewal is off" on three servers whose renewal was on.
It reads the field five times now. When the readings disagree, that is the finding — "the API gave
different answers about automatic renewal across 5 reads, so whether this server renews cannot be
established from here" — which is both true and the more serious problem. When they agree, the
finding says how it was established and asks the operator to confirm, because this route cannot give
certainty. Three readings would agree by chance one time in four; five bring that to about one in
sixteen. This is worth a look by the route's owner.
Design
command says so and reports no clean bill of health for the rest. It exists to be trusted when it
says nothing is wrong.
fixwas checked against this binary's command tree. The first draftsent operators to
set-boot,list-bootsandlist-planned-interventions, none of which exist —the same defect an earlier lot of this audit shipped in its own error message.
every other command of this CLI reads a non-zero code as a failure.
--strictturns findings intoa failure for the pipeline gating on it, and has to be asked for rather than discovered.
--filterworks on them(
--filter 'severity=="critical"').Verified
11 sabotages against the checks, 11 red — including one that reverts the rescue detection to the
type alone, one that decides the renewal from a single read, and one that lets an unreadable server
pass for healthy.