feat(baremetal): add power off/on/status, and say what powering off leaves behind - #248
feat(baremetal): add power off/on/status, and say what powering off leaves behind#248Denis-hamon wants to merge 14 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>
…behind
The API has no power endpoint. There is no `/power`, no `/shutdown`, no
`/poweroff` anywhere in the 126 paths of `dedicated.server`, and a first pass
over that list concluded the CLI could not offer the verb at all.
The verb is there, spelled differently. `dedicated.server.BootTypeEnum` carries
`power` as a first-class boot type, every one of the 35 servers reachable from
this account has an entry of that type — 95083 in Europe, 95644 in Canada — and
`bootId` is writable. Powering a server off is `PUT {bootId: <power entry>}`
followed by `POST /reboot`: the machine reboots into an entry whose whole job is
to shut it down. Measured end to end on ns3070493, twice.
Three things came out of that measurement, and each one is in the code.
A completed task does not mean the machine obeyed. The `hardReboot` task
reported `done` at sixty seconds; `powerState` only flipped at t+143s, and on
the way back up at t+207s. So `--wait` polls `powerState` and never the task
status. Waiting on the task would have produced a command that returns success
while the server is still running — a gate proving the call was made, not that
anything happened.
Powering off rewrites the boot configuration, durably. The server stays on the
power-off entry and shuts down again at every later reboot, including one
triggered from the manager, with nothing on screen to explain why. `power off`
therefore records the boot the server was on before touching anything, `power
on` puts it back, and `power status` warns whenever a server is sitting on a
power-off entry.
The boot a server was on is not guessable. ns3070493 was in rescue (230242), not
on disk — I had assumed disk, and assuming it would have "restored" the machine
into a state it had never been in. So the value is read before it is changed,
and when this machine has no record of it — a `power on` run from a different
laptop than the `power off` — the command falls back to the disk entry and says
so in the message rather than presenting a guess as a memory.
Two details worth their lines. `forgetBoot` deletes its cache entry through a
new `cache.Remove` instead of writing an empty one, because `cache.Write`
purges the namespace against the ttl it is handed: forgetting one server with a
zero ttl took every other remembered server with it, so powering server A back
on made the CLI forget where B boots — while B was still off. There is a test
for exactly that. And `power on` takes `--dry-run` but no `--yes`: it interrupts
nothing, so there is no prompt to skip, and a flag that skips nothing is one
more thing to learn that means nothing.
Ten tests, on responses recorded from the live API. Each was sabotaged and
watched fail before being kept.
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.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>
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>
`power status` answers about one server and renders one object through OutputInfo. withFilterFlag binds --filter to flags.GenericFilters and nothing more, so on this command the flag was accepted by cobra, documented by docgen, and inert — the operator could believe the answer had been narrowed when nothing had looked at it. There are no rows here to filter, so the flag is removed rather than implemented. A test pins the refusal: a flag that can only be ignored is worse than an absent one. Found by a review pass over this branch, then confirmed by an audit of every withFilterFlag call site in the tree. Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`power off` remembers the boot entry the server was on, because the PUT that follows overwrites it and only `power on` knows that value mattered. It recorded whatever the API answered at that moment — including the power-off entry itself. That is not a race, it is the ordinary case. The PUT lands at once and the machine only goes dark about 143 seconds later, measured on 18 August 2026, and for that whole window the API reports powerState "poweron" sitting on the power-off bootId. Anyone typing `power off` twice, because the first one seemed to do nothing, replaced the record with the entry that shuts the machine down. `power on` then restored a power-off and announced it as "the boot it was on before it was powered off". The real entry — a rescue system, in the case that found this — was gone, and nothing on screen said so. The record is now left alone when the server is already on the power-off entry, and previousBootId reports what is actually known rather than the entry that was just written. It is absent rather than zero when nothing is known, so a caller reading -o json can tell the two apart. Two tests, both red under a sabotage that restores the unconditional write. Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`power on` puts the boot configuration back so that a later reboot does not shut the machine down again. It sent that PUT unconditionally, without ever looking at the bootId the server currently reports — so on a machine already sitting on the target entry it wrote a value that was already there, and then announced "its boot is now N", which was untrue: the boot had not moved. `power off`, three functions above, already compares state.BootID against the entry it is about to use before deciding what to record. Not doing it here was an omission, not a difference between the two commands. When neither the boot nor the power state needs changing, the command now says so and sends nothing. Dropping the record of the previous boot is itself a change, so --dry-run does not do it either: a preview that quietly forgets which boot the machine came from has the one property a preview must not have. Three tests, three sabotages, three reds — and the inverted-comparison sabotage also fails two tests that were already there. 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>
Human review — Does not workB2 · Switching off a production machine
The output : — Denis (Product Manager), through CLI sandbox review page |
Human review — Does not workB4 · Nothing to do: the command says so instead of acting
This command can write, so live mode will not run it as is. Add --dry-run: the command then describes the calls it would make, If the command has no --dry-run, it will say so itself. — Denis (Product Manager), through CLI sandbox review page |
|
Followed up on today's two "Does not work" findings from the human review (B2, B4) — both trace back to the same cause, not to this PR's code. B2 ( B4 ( Token refreshed today. Rebuilt this branch and replayed both live against the test/integration account, on Both work as intended — B4 in particular is exactly the "nothing to do, and the entry is named" behaviour the check asked for. No code change needed here. |
Human review — Does not workB3 · Powering on restores the previous boot entry
🛑 failed to read the state of ns0000001.ip-203-0-113.eu: OVHcloud API error (status code 404): Client::NotFound: "This service does not exist" (X-OVH-Query-Id: EU.ext-4.6a89a9d1.1101808.34160f0a4a397999e8833d9b0b29811a) — Denis (Product Manager), through CLI sandbox review page |
Human review — Does not workB4 · Nothing to do: the command says so instead of acting
This command can write, so live mode will not run it as is. Add --dry-run: the command then describes the calls it would make, If the command has no --dry-run, it will say so itself. — Denis (Product Manager), through CLI sandbox review page |
|
Both of today's 15:53 findings come from the review sandbox, not from this branch. Fixed on the sandbox side. B3 — 404 "This service does not exist". The check's steps offer The sandbox now rewrites, at startup, every command live mode would actually run to a real test machine ( Which is what B3 asks for: it names the entry it is on, so you can see for yourself which one it took. B4 — the "add --dry-run" guard text, again. No write call sent, the entry named, and phrased as a statement rather than an error — the three things B4 asks for. Note the check's own command still says Separately: the sandbox was running a binary built on 21/08 17:21 and has been rebuilt from the current integration stack, so what the page shows is what these branches actually produce. |
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>
The flag registration arrived here as a plain commit, so the content matched but git had no way of knowing where it came from: merging ovh#243 into main and then this branch would have replayed the same change and conflicted over it. This merge carries no content — it only states the parentage that was missing. 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-power
Signed-off-by: Denis <denis.hamon@ovhcloud.com>
1 participant
Stacked on #245 — only the last commit (
7e9fdfd) belongs to this PR; the three below it are the catalog work under review there.The API has no power endpoint. It has the verb anyway.
There is no
/power, no/shutdown, no/poweroffin the 126 paths ofdedicated.server, which is why "power a server off" reads as impossible from the CLI. It is spelled differently:dedicated.server.BootTypeEnumcarriespoweras a first-class boot type, all 35 servers reachable from my account have an entry of that type (95083 in Europe, 95644 in Canada), andbootIdis writable.The machine reboots into an entry whose only job is to shut it down.
Measured on real hardware, twice
First with a raw script, then through the CLI itself, on a spare RISE-2 with no OS installed. Starting state read before anything was touched, and returned to it:
Three measurements that shaped the code
A
donetask does not mean the machine obeyed. ThehardReboottask reporteddoneat 60 s;powerStateonly flipped at t+143 s, and at t+207 s on the way back up. So--waitpollspowerStateand never the task. Waiting on the task would have shipped a command that reports success while the server is still running.Powering off rewrites the boot configuration, durably. The server stays on the power-off entry and shuts down again at every later reboot — including one triggered from the manager, with nothing on screen explaining why. Hence:
power offrecords the previous boot,power onputs it back, andpower statuswarns whenever a server sits on a power-off entry.The previous boot is not guessable. My test server was in rescue (230242), not on disk — I had assumed disk, and restoring the assumption would have put the machine in a state it had never been in. The value is read before it is changed; when this machine has no record of it (a
power onrun from a different laptop than thepower off), the command falls back to the disk entry and says so in the message rather than presenting a guess as a memory.One defect this surfaced in
internal/cachecache.WritecallsPurgeExpired(namespace, ttl)with the ttl it is handed, so writing an empty entry with a zero ttl deletes every other entry in the namespace. Forgetting one server therefore forgot where all the others boot — while they were still powered off, which is exactly when that memory is the only thing between them and a boot they were never on. Fixed with acache.Remove, with a test for the two-server case.This is worth knowing beyond this PR:
cache.Write(ns, key, nil, 0)is not a way to delete an entry.Notes
power ontakes--dry-runbut no--yes: it interrupts nothing, so there is no prompt to skip. Caught by reading the generated docs, not the code.doc/ovhcloud_logout.mdis generated bydocgenand has never been committed onmain; it is untracked, sogit diff --exit-codein the doc gate does not see it. Left out of this PR — it is not this change's business, but the gate has a hole.🤖 Generated with Claude Code