feat(baremetal): say what a reinstall is doing while it does it - #253
Draft
Denis-hamon wants to merge 16 commits into
Draft
feat(baremetal): say what a reinstall is doing while it does it#253Denis-hamon wants to merge 16 commits into
Denis-hamon wants to merge 16 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>
`reinstall --wait` followed the task, and the task API answers whether the work
is finished and nothing else. So a reinstall that runs for twenty minutes
printed the same sentence sixty times:
Still waiting for task 559351769 to complete (status=doing)…
install/status has the missing half — one entry per step, with the running one
marked — and the CLI used it nowhere. It does now, in two places:
- `baremetal install-status <server>` reads it, for somebody who started the
installation in another terminal, or who answered the confirmation, walked
away, and wants to know whether to keep waiting.
- `reinstall --wait` prints the step it is on instead of repeating itself.
Measured against a real reinstall of an ADVANCE-1:
step 5/21: Preparing disks for new Partitioning
step 10/21: Applying Partitioning
step 13/21: Downloading OS image
step 16/21: Checking Partitioning
Two things the API does that the code has to know about.
Outside an installation the route answers 404 with "Server is not being
installed or reinstalled at the moment". That is an answer, not a breakdown,
and it is also why this endpoint cannot carry the verdict of a --wait: it says
the same thing before an installation starts and after it ends. The task stays
the authority on whether the work is done; install/status only says where it
has got to, and a progress read that fails is an empty line rather than a
failed command.
And elapsedTime is not the age of the installation. Sampled through one real
run: -1935 during the hardware reboot, +42 four minutes later, -87 again after
the final reboot, counting up at exactly one per second in each stretch and
never resetting between steps. It is a continuous counter whose origin is
rebased every time the machine restarts. So --wait times itself from the
request it sent, and the read command attributes the figure to the API instead
of asserting it — it was reporting 1m47s while the install had been running
for six minutes.
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>
Denis-hamon
changed the base branch from
main
to
feat/baremetal-install-discovery
August 20, 2026 15:19
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com> # Conflicts: # internal/cmd/baremetal_install_test.go
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>
Contributor
Author
Human review — Does not workC1 ·
🛑 failed to fetch the installation status of ns0000001.ip-203-0-113.eu: OVHcloud API error (status code 401): Client::Unauthorized: "You must login first" (X-OVH-Query-Id: EU.ext-4.6a89762d.1101808.07340469ae1ad3e9577d931e9921a0d9) — Denis (Product Manager), through CLI sandbox review page |
Contributor
Author
|
Followed up on today's "Does not work" finding (C1, Same cause as the identical 401 reported today on #248: the review sandbox's Token refreshed. Rebuilt this branch and replayed live against the test/integration account: Works as intended. No code change needed here. |
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>
Contributor
Author
Human review — Does not workC3 · Does the elapsed time come from the CLI or from the API?
🛑 error reinstalling server: mandatory field "operatingSystem" is missing in the parameters Usage: — Denis (Product Manager), through CLI sandbox review page |
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-install-progress
Signed-off-by: Denis <denis.hamon@ovhcloud.com>
This was referenced Aug 26, 2026
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
1 participant
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.
A reinstall said nothing for twenty minutes
reinstall --waitfollowed the task, and the task API answers whether the workis finished and nothing else. So this was the whole of it, repeated every
thirty seconds:
install/statushas the missing half — one entry per step, with the runningone marked — and the CLI used it nowhere.
What it does now
Measured on a real reinstall of an ADVANCE-1 (EPYC 4245P, rbx8) — 11 min 05 s
end to end, exit 0:
and
install-status, against that same running installation:Two things the API does that the code has to know about
Outside an installation the route answers 404, with "Server is not being
installed or reinstalled at the moment" — verified on three servers. That is
an answer, not a breakdown, so it is reported as one.
It is also why this endpoint cannot carry the verdict of a
--wait: it saysthe same thing before an installation starts and after it ends. The task stays
the authority on whether the work is done;
install/statusonly says where ithas got to, and a progress read that fails is an empty line rather than a
failed command.
elapsedTimeis not the age of the installation. Sampled every 30 s throughone real run:
+42+72+102+133-106-76-15It counts up at exactly one per second and never resets between steps, but its
origin is rebased at every machine restart — one discontinuity of 269
seconds at the final reboot, and an earlier
-1935during the hardware reboot.It was reporting
1m47swhile the install had been running for six minutes.So
--waittimes itself from the request it sent, andinstall-statusattributes the figure to the API (
· API reports 1m47s elapsed) rather thanasserting it as a duration measurement has shown it is not.
Verification
go build,go vet,make wasm,go test ./..., docgen — all green.11 tests. 5 sabotages, 5 red, each on the test meant to catch it:
PrintsTheNoteInsteadOfRepeatingItselfTestMockSuiteRefusesToInventANegativeDurationCurrentStepIsTheOneBeingWorkedOn+IsAbsentWhenNothingIsRunningFailedStepCarriesItsReasonThe empty-note path is covered too: a progress read that fails must leave the
wait saying what it always said, not fail it.
One thing found while running this, unrelated to the change
On
main,reinstall --waitends by callingGetBaremetalAuthenticationSecrets,which prints the new SSH password in cleartext, unconditionally. Anyone
redirecting that output to a build log has the root password in it.
#233 already fixes this — it replaces that call with a masked
fetchAuthenticationSecretsbehind--reveal. Verified that #233 and thischange merge cleanly in
baremetal.go; the only conflicts are the two docpages and the cobra wiring where both add commands side by side.