feat(ip): move a failover IP, and say where it can go - #255
Draft
Denis-hamon wants to merge 18 commits into
Draft
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>
An additional IP is bought to be moved. That is what makes it additional: it
follows a service rather than a machine, so a failover survives the server
under it. The CLI could list those IPs and show what each was routed to, and
could move none of them — `/ip/{ip}/move` and `/ip/{ip}/park` were among the
66 routes of this domain it did not reach.
ovhcloud ip destinations <ip>
ovhcloud ip move <ip> <service> [--nexthop] [--wait] [--yes|--dry-run]
ovhcloud ip park <ip> [--wait] [--yes|--dry-run]
ovhcloud ip tasks <ip>
`destinations` is what makes the rest usable. The API answers a map keyed by
service family, and on a real account one address accepted a hundred of them —
33 cloud projects, 33 VPS, 22 dedicated servers, 12 load balancers. Families
that accept nothing come back as empty lists rather than being absent, so they
are dropped: four empty sections answer the question worse than three full ones.
`move` validates the destination against that same list before sending
anything. A service that does not accept the IP is refused locally, naming the
families and their counts, because the alternative is a 400 several seconds
later with no hint of which names would have worked. The individual services
are deliberately not printed — there can be a hundred — so the refusal names
the command that lists them.
The guard is `Disruptive`: the traffic that stops is the traffic of the service
the IP is leaving, and that service is the one thing the operator cannot see
from the command they typed, so the prompt names it.
`--wait` reads where the IP is routed rather than following the task. Measured
on a real move: a minute after the API accepted the request, the task existed
and `routedTo` still pointed at the old server. A wait that trusts the task it
was handed can report success while the work is unfinished — the same shape as
the vRack attach in ovh#249, where one request produced two tasks.
Verified on a real failover address, moved and put back:
move to ns3118333.ip-51-68-100.eu 1 min 32 s, exit 0
move back to ns3018397… 1 min 06 s, exit 0
final state, read outside the CLI identical to the start
The four refusal paths were run against the live API: unknown destination,
--dry-run, an IP that serves nothing, and refusal without --yes.
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>
routedTo swallowed every read error into an empty service name, and an empty
service name is what this file uses to mean "this IP serves nothing". The two
are opposites, and all three callers acted on the difference:
- `park` answered "X is not routed to any service, so there is nothing to
park" and exited 0. The IP may well have been carrying production traffic;
what actually happened is that nobody could tell.
- the move prompt lost the "this stops the traffic it carries for Y" warning
at the one moment it matters — the last point where a wrong move can still
be stopped.
- worst, the wait loop. Parking waits for the routing to become empty, so a
failed read WAS the answer it was waiting for: `park --wait` would report
the park done because the check verifying it had failed. This whole stack
is built on probing the state rather than trusting a task status, and this
turned that principle against itself.
routedTo now returns an error. park refuses, the prompt says the reading
failed instead of implying the IP is free, and the wait loop does not count a
failure as arrival.
The test pins the distinction the prompt has to make: the two cases must not
produce the same sentence.
This commit also carries the case-variant fixture sweep for this file — the
uppercase spelling of a real hostname lived in the same test.
Found by a third-party review pass over this branch, which named the prompt.
The wait loop was worse and is what the fix is really about.
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>
waitForRouting had no terminal-failure branch. A move or park whose task ends in
cancelled, customerError or ovhError leaves the IP where it was, so the state
reads "not there yet" for as long as anyone asks: the wait ran its full ten
minutes and finished with "is not routed to X yet, follow it with ovhcloud ip
tasks", which reads as work still in progress. Ten minutes and a misleading
sentence for something the API knew in seconds.
The task is now consulted, and consulted as a complement — never a substitute.
The file already argued why: a vRack attach measured earlier in this CLI created
two tasks for one request, so a wait that concluded from the task alone could
report success while the other half was still running. So the state is read
first every round, the task is asked only about failure, and the state is read
once more before any failure is announced — if the IP has arrived in the
meantime, that wins.
The three statuses treated as terminal are retyped in Go because the schema lists
the seven without saying which are failures; a guard test holds them against the
enum so a rename shows up red instead of silently disabling the branch.
`park` was said to have no task to watch. It has: POST /ip/{ip}/park answers with
an ip.IpTask, which the code discarded. Keeping it makes both halves symmetric.
The identifier is read through a json.Number case, because go-ovh decodes with
UseNumber — a type switch handling only float64 is dead code, which this
repository has already shipped twice.
Also here, the two behavioural halves of eb56fab that had no test: a park is never
concluded from a failed read, and an IP whose routing cannot be read is never
reported as one that serves nothing. "Nothing to park" reads as "already done".
Eight tests, five sabotages, five reds — after two of the five first came back
green for the wrong reasons. One broke the build, which proves nothing about a
test's teeth; the other exposed a genuinely vacuous test, whose first state read
already matched, so the re-read it was written for never ran. Both rewritten.
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>
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/ip-failover-lifecycle
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.
An additional IP is bought to be moved
That is what makes it additional: it follows a service rather than a machine,
so a failover survives the server under it. The CLI could list those IPs and
show what each was routed to, and could move none of them.
destinationsis what makes the rest usableThe API answers a map keyed by service family. On a real account, one address
accepted a hundred destinations — 33 cloud projects, 33 VPS, 22 dedicated
servers, 12 load balancers. Families that accept nothing come back as empty
lists rather than being absent, so they are dropped: four empty sections answer
the question worse than three full ones.
The destination is checked before the request
The alternative is a 400 several seconds later with no hint of which names
would have worked, and the CLI is holding that list already. The individual
services are deliberately not printed — there can be a hundred — so the
refusal names the command that lists them instead.
The guard is
Disruptive. What stops is the traffic of the service the IP isleaving, and that service is the one thing the operator cannot see from the
command they typed, so the prompt names it.
--waitreads the IP, not the taskMeasured during the live move: a minute after the API accepted the request,
the task existed with status
todoandroutedTostill pointed at the oldserver. A wait that trusts the task it was handed can report success while the
work is unfinished — the same shape as #249, where one vRack attach produced
two tasks.
Verified on a real failover address, moved and put back
movetons3118333.ip-51-68-100.eu--waitmoveback tons3018397…--waitThe four refusal paths were run against the live API: unknown destination,
--dry-run, an IP that serves nothing, and refusal without--yes.go build,go vet,make wasm,go test ./..., docgen — all green.13 tests. 5 sabotages, 5 red, each on the test meant to catch it.
One of those sabotages first reported red for the wrong reason — it left an
unbalanced brace, so the failure came from the compiler rather than from a
test. Re-applied in a form that compiles, it fell on the intended test. Every
restore in this lot is verified by checksum against the pre-sabotage file.
Note for whoever merges this with #242
common.Callgains aDetailfield in #242, which is not in this branch'sancestry. The move's destination therefore travels inside the endpoint string
here; when both land, it belongs in
Detailso that-o jsonkeepsendpointa path. There is a comment saying so at the call site.