feat(vrack): attach and detach dedicated servers, and show what a vRack holds - #249
Draft
Denis-hamon wants to merge 18 commits into
Draft
feat(vrack): attach and detach dedicated servers, and show what a vRack holds#249Denis-hamon wants to merge 18 commits into
Denis-hamon wants to merge 18 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>
…ck holds
The vrack domain exposed three of the API's forty-seven endpoints: list, get,
rename. A customer could name their private network from the terminal but not
put a machine in it, which is the only reason the object exists.
## Attachment is by interface, and an operator does not know the interface
`POST /vrack/{n}/dedicatedServerInterface` takes a UUID. Nobody types
1688f939-b93f-47a2-9648-b270f8150a53 from memory, so the command takes a server
and resolves it: allowedServices lists what may still be attached, with the
owning server's hostname, and dedicatedServerInterfaceDetails does the same for
what already is. Measured on 71 vRacks: 9 hold interfaces, 0 hold a server
through the legacy `dedicatedServer` endpoint, which is why that path is not
used.
allowedServices rather than eligibleServices, though both claim to answer this
question. eligibleServices is an asynchronous job; its stored result was a day
old when this was measured, and reported no attachable interface at all while
allowedServices listed eleven.
Resolution refuses rather than guesses. A server with two interfaces — an
aggregation and a plain one, which do not carry the same traffic — is a choice
the operator makes with --interface, and that flag is checked against the named
server's own interfaces instead of being passed through, so a UUID belonging to
another machine cannot attach something nobody named.
## Two silences that look identical
A server absent from allowedServices either has no vRack interface at all or
has one that is already attached somewhere. Same absence, opposite actions, and
the first is not rare: 7 of the 35 servers measured have no virtual network
interface whatsoever. So the failure path asks the server itself and says which
of the two happened — including the third case, a name that matches no server,
which used to read as a hardware problem.
## Naming the machine an operator would recognise
The hostname is an address. `iam.displayName` is what its owner calls it, 23 of
the 35 servers carry one, and they read like "Yaniv - RISE-1 - LIM" or
"BMAD_Repo + Paperclip backup". Both are printed everywhere the command names a
machine, because neither replaces the other: the display name says whether this
is the right one, the hostname says it is the only one. And both are accepted
as input — a name the CLI prints and refuses to take back is half a feature.
A display name held by two machines is refused, with the hostnames listed.
The two objects store this in opposite places, which is worth knowing before
reusing the code: a server's chosen name is in IAM, while a vRack's is in its
own `name` and `description`, and its `iam.displayName` is null. Nothing is
cached — one GET is cheaper than showing an hour-old name in the sentence
somebody reads before cutting a machine off its network, or than two accounts
sharing one cache through --profile.
## get answers the question it is asked
`vrack get` printed name, description and IAM URN, so a vRack holding two
servers and a Cloud Connect looked exactly like an empty one. It now leads with
what is inside, then lists it — all eleven attachable types, not the two this
command can act on, because 32 of the 71 vRacks measured hold only a cloud
project and would each have printed as empty. A type that cannot be read is
counted and said, rather than folded into "This vRack is empty": that sentence
is a claim, and ten serial GETs give it ten chances to be false.
## Waiting
--wait polls the membership, never the task. One attach produced two tasks
(addDedicatedServerInterfaceToVrack and addVlanToVrackOnNode), so the task
returned by the POST is not the whole operation; and on a sibling command a
task reported done 83 seconds before the machine obeyed. The task is still read
for the one thing state cannot say — a cancelled operation stops the wait
instead of running to the timeout. That path needed `json.Number`: go-ovh
decodes with UseNumber, and a type switch does not widen it to float64, so
without that case the check was dead code.
Measured end to end against the live API: attached in 3 min 28 s, detached in
2 min 01 s, starting state read before and returned to after, verified by a
read that did not go through this CLI.
Reviewed adversarially by two models before opening; the display-name input
contract, the empty-vRack claim, the json.Number case, the multi-vRack server
and the already-attached message all come from that pass. Seventeen tests on
recorded responses, each 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>
This was referenced Aug 19, 2026
Denis-hamon
marked this pull request as draft
August 20, 2026 13:09
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.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>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
…iled Reading a vRack means reading eleven things: the object, the attached server interfaces, and nine other content types. The comment above that code said a failure costs the section and never the command. It did the opposite. display.OutputWarning is not a message, it is an exit: OutputWithFormat ends on ExitFunc(0) for a warning exactly as it ends on ExitFunc(1) for an error, and ExitFunc is os.Exit. Called in the middle of GetVrack, it took the process down before the vRack was printed. Whenever one of those lists failed — a 500, a rate limit, a dropped connection — `vrack get` answered nothing whatsoever and exited 0, so a script could not tell it apart from a vRack it had read. The failure is now folded into the object: it is printed under the summary, it is in -o json, and it is counted with the other unreadable sections so the summary stops short of "This vRack is empty" — the claim this command must never make about a list it could not read. Everything after an OutputWarning or an OutputError is dead code in the shipped binary, and the suite could not see it: cmd_test.go replaces ExitFunc with a no-op, so under test the command kept running and printed what an operator never saw. executeWithRealExit restores the real semantics for one command — the stop is a panic rather than os.Exit, but the point is only that execution does not continue — and the test for this fix uses it, because that stub is precisely what hid the defect. An AST pass over the whole tree found no other call of this shape introduced here, out of 1575 examined. Four remain on main and are left to their owners: three in Logout, where a warning ends the process before the local credentials are deleted, and one in formatContainerRegistryPlans. 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 attached list is one entry per vRack interface, and a dedicated server can carry several — this package knows it everywhere else: interfacesOf sorts the interfaces of one server, attach refuses when a server has more than one, and detach counts distinct servers before deciding. Only the summary counted rows. So a machine with two NICs in the vRack printed "2 dedicated servers" above a two-line table, and nothing in the output let the operator see that both lines name the same host. distinctServers already existed, ten lines away. One test, red under a sabotage that puts len() back. Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…one page
`vrack get` listed ten of the thirteen types vrack.AllowedServiceEnum declares.
One of the three left out is read by the servers section
(dedicatedServerInterface); the other two were simply absent:
- dedicatedServer, the legacy attachment — a server bound to the vRack
directly rather than through one of its interfaces. It is the only path
under which such a vRack shows up at all, so a vRack holding a production
machine that way printed "This vRack is empty."
- dedicatedCloudDatacenter.
Zero of the 71 vRacks on the account measured use the legacy attachment, which
is why the omission was invisible rather than harmless. Both paths answer 200
with a list, verified against the API.
The list of paths is now package-level and two tests hold it against the schema
enum in both directions: every declared type is listed, and nothing is listed
that the schema does not declare — an invented path errors on every vRack, and
errors are counted as "could not be read", so it would permanently stop the
summary from ever saying a vRack is empty.
Second fix, same file: serverDisplayNames read /v2/iam/resource with a plain GET.
It is a v2 route and v2 routes paginate by cursor — measured, with
X-Pagination-Size: 10 the route answers ten objects and an
X-Pagination-Cursor-Next. With 35 servers on this account a single GET happened
to see all of them, so the defect was latent rather than absent; past one page a
machine loses its display name, and that is the name an operator recognises in
the sentence asking them to cut its network. It now reads through FetchArray,
which already follows the cursor.
Four tests, four sabotages, four reds.
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
marked this pull request as ready for review
August 21, 2026 12:50
Denis-hamon
marked this pull request as draft
August 21, 2026 15:01
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/vrack-attach
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
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.
Stacked on #248 — only the last commit (
84ec8a1) belongs to this PR.The vrack domain could name a private network, not put anything in it
Three of the API's forty-seven vrack endpoints were reachable: list, get, rename. This adds the two gestures the object exists for.
Both trees, one implementation. The network view is the object's home; the server view is where somebody holding a machine looks — this CLI already has a command nobody finds for that reason,
ip reverse set, invisible frombaremetal --help.Measured before it was written
On a live account: 71 vRacks, 35 dedicated servers, read-only.
dedicatedServerendpointallowedServicesis read rather thaneligibleServices. Both claim to answer "what can be attached"; the latter is an async job whose stored result was a day old and reported nothing at all, while the former listed eleven interfaces with their owning server.Three things that shaped the code
Two silences that look the same. A server missing from
allowedServiceseither has no vRack interface or already has one attached elsewhere — and, with a typo, may not exist. One in five servers falls in the first case, so this is the command's likeliest output, not a defensive branch. It asks the server itself and says which happened.A machine's name is not its hostname.
iam.displayNameholds what its owner calls it — "Yaniv - RISE-1 - LIM", "BMAD_Repo + Paperclip backup" — while/dedicated/server/{n}'snamefield repeats the hostname on all 35. Both are printed and both are accepted as input: the display name says whether this is the right machine, the hostname says it is the only one. Note for anyone reusing this: a server keeps its chosen name in IAM, a vRack keeps its own inname/descriptionwithiam.displayNamenull. Reading "the display name" uniformly would show one of the two and silently blank the other.--waitpolls the membership, never the task. One attach produced two tasks (addDedicatedServerInterfaceToVrack,addVlanToVrackOnNode), so the task returned by the POST is not the operation. The task is still read for the one thing state cannot report — a cancelled operation ends the wait instead of running to the timeout.get now answers the question it is asked
It used to print name, description and IAM URN, so a vRack with two servers and a Cloud Connect looked identical to an empty one. It leads with a summary, then lists all eleven attachable types — not the two this command drives, because 32 of the 71 vRacks hold only a cloud project and each would have printed as empty. A type that fails to list is counted and said rather than folded into "This vRack is empty", which is a claim, and ten serial GETs give it ten chances to be wrong.
The Name column appears only when at least one server has a name of its own; otherwise it repeated its neighbour and cost the width that pushes a row onto two lines.
Verified on real hardware
Attached in 3 min 28 s, detached in 2 min 01 s, on an empty vRack and a spare RISE-2. Starting state recorded before and returned to after, confirmed by a read that did not go through this CLI.
Notes
json.Number: go-ovh decodes withUseNumber, and a type switch does not widen it tofloat64. Without that case the cancelled-task check was dead code — the same class of bug this repo already fixed once inbaremetal.go.--profile.doc/ovhcloud_logout.mdis generated bydocgenand has never been committed onmain; it is untracked, so the doc gate'sgit diff --exit-codedoes not see it. Not this change's business, but the gate has a hole.🤖 Generated with Claude Code