feat(backup-services): the Veeam backup surface, reached from the machine it protects - #265
Draft
Denis-hamon wants to merge 28 commits into
Draft
feat(backup-services): the Veeam backup surface, reached from the machine it protects#265Denis-hamon wants to merge 28 commits into
Denis-hamon wants to merge 28 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>
…hine it protects Seventeen v2 paths manage the Veeam Backup product — tenants, storage vaults, the Service Provider Console that drives the agents — and none had a command. Not skipped: invisible, because this repository could only fetch v1 schemas until ovh#262. Every operation is badged "Beta version" upstream. ovhcloud backup-services tenant|vault|vspc list|get|edit ovhcloud backup-services vault buckets <vault> ovhcloud backup-services policies ovhcloud backup-services deploy-script ovhcloud backup-services licenses list|servers ovhcloud baremetal backup-agent show|create|edit|delete <server> The hierarchy is three UUIDs deep and nobody knows any of them, so the levels are resolved when they are not ambiguous — one tenant is taken, several are refused with their names and identifiers, none is an answer rather than an error. --tenant and --vspc exist for the accounts where there is a choice. An agent is reached from the machine it protects. backup.AgentProductTypeEnum lists OVHCLOUD_BAREMETAL as a first-class product type and the agent names its server in productResourceName, so `baremetal backup-agent` starts where an operator starts. Measured on 20 August 2026: all nine agents of this account are OVHCLOUD_BAREMETAL, named agent-<server>, with ips equal to the server's own address in a /32 and region equal to the server's region — nine out of nine. That correspondence is why `create` needs nothing but the server name. Three things the measurement decided. An agent is created NOT_INSTALLED, and that is where it stops: the object exists and the software is not on the machine. All nine agents of this account are in that state, with an empty policy, while two retention policies sit beside them — nine provisioned, none deployed, none retaining. So NOT_INSTALLED is a settled status for --wait, not a transition, and the success message says what is left to do rather than "created". The PUT replaces the target spec, so `edit` carries over what it is not changing. Sending only --policy would blank the display name and the addresses, which is the trap `iam resource edit --tag` still has. `vault bucket credentials` is deliberately absent. The schema declares a GET and the API answers 405 on all three buckets of this account, with no Allow header, while the same path answers 404 for a bucket that does not exist and the parent bucket route answers 200. The route resolves and then refuses the method, so the command would always fail — the same reason install/hardwareRaidSize was left out in ovh#250. Two columns exist because the raw object hides what somebody is looking for. A resource carries the tasks running on it and there is no task route to poll, so the tasks column is the only place a failure appears; it names the operation, because "ERROR" answers "error doing what?" with nothing. And allowedIps is deliberately not a vault column: it looks like a field of a vault and is not, and reading it off the vault answers 0 where the truth is 9. That column found something. On this account three BACKUP_VAULT_CREATE tasks are in ERROR and a VSPC_AGENT_UPDATE beside them, with two VSPC_TENANT_UPDATE stuck in PENDING — sampled six times over five minutes, same identifiers, same statuses — on resources that all report READY. Nothing surfaced them before. 10 unit tests, 15 cobra tests, 15 sabotages, 15 red. Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Checked against the acceptance criteria of LVL2-19664 (Backup Agent - CLI
Integration). Four of the six MUSTs were already covered by the previous
commit; these are the three that were not.
MUST "List Agent" was answered per server by `baremetal backup-agent show`,
which is the question an operator asks about a machine. `backup-services agents`
is the other question — what the backup posture of the estate is — and it is the
one that makes this account's state readable in one screen: nine agents
provisioned, none deployed, none on a retention policy.
MUST "Download Agent" is `deploy-script`, which prints the install command and
the per-platform links. It gains `download-agent` as an alias so the name in the
product brief reaches the command; one command under two names rather than two
commands.
MUST "List billing / invoices pricing / consumption" was not covered at all, and
the reason is that the backup API carries no price: a v2 resource has no plan,
no billing date and no renewal mode. Those live in the account's service router,
keyed by the very same identifiers — measured, /v1/services?resourceName=<uuid>
answers with exactly one service for the backup tenant, for the VSPC tenant and
for each vault. So the join is one lookup per resource rather than a sweep of
the 826 services on this account, and `backup-services billing` reads:
kind name plan price period consumption renew state
tenant 15d3f0dd-… backup-tenant 0.00 € P1M none yet automatic active
vspc vspc-tenant-610301 vspc-tenant 0.00 € P1M none yet automatic active
vault backup-vault-paygo-…-sbg-… backup-vault-paygo 0.00 € P1M none yet automatic active
Consumption is joined by serviceId from the account's current usage. Nothing has
been stored on this account — every agent is NOT_INSTALLED — so every line reads
"none yet", which is a fact and not an empty cell. Consumption that could not be
read reads "unknown" instead: a failed read is not a reading of zero.
A backup resource with no billable service behind it does not stop the command:
that is what an included component looks like, and the price column says so.
Account-level invoices (/v1/me/bill) are deliberately not here. They are an
account-wide surface no command exposes yet, and scoping them to one product
belongs to an `account` lot rather than to this one.
3 unit tests, 4 cobra tests, 7 sabotages, 7 red.
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
The IP of the machine these tests were written against ended up in the fixtures, five times. It is not a secret -- it is in the DNS name of the server -- but it ties this repository to one account for no benefit, and the same habit put a live migration token in a test once already (fixed in ovh#257). Replaced with 203.0.113.7, which RFC 5737 reserves for documentation. Signed-off-by: Denis <denis.hamon@ovhcloud.com>
Denis-hamon
added a commit
to Denis-hamon/ovhcloud-cli
that referenced
this pull request
Aug 20, 2026
A real invoice identifier read from the account under test ended up in the refund fixture, and the server address of ovh#265 came along with the rebase. Neither is a secret and neither buys anything: replaced with invented values of the same shape, and with 203.0.113.7 from the range RFC 5737 reserves for documentation. Signed-off-by: Denis <denis.hamon@ovhcloud.com>
Denis-hamon
marked this pull request as draft
August 20, 2026 13:09
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>
Denis-hamon
added a commit
that referenced
this pull request
Aug 20, 2026
A real invoice identifier read from the account under test ended up in the refund fixture, and the server address of #265 came along with the rebase. Neither is a secret and neither buys anything: replaced with invented values of the same shape, and with 203.0.113.7 from the range RFC 5737 reserves for documentation. Signed-off-by: Denis <denis.hamon@ovhcloud.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Ten commands of this group registered the flag and rendered their rows directly: tenant, vault, bucket, vspc, policies, agents, billing, licenses, licence servers, and the per-server agent view. withFilterFlag only binds --filter to flags.GenericFilters; display.RenderTable does not read it, and ManageListRequest — which normally passes it to filters.FilterLines — is not on this path because the rows are assembled here. So the flag was accepted by cobra, written into doc/ by docgen, and inert. An operator reading a list they believe was narrowed is worse served than one who was never offered the flag. These go through common.RenderFilteredTable, added for the same defect earlier in the stack. The test asserts the row the filter EXCLUDES is absent, not merely that the kept one is present: the second passes just as well with the filtering removed. Found by a review pass over this branch, then confirmed by an audit of every withFilterFlag call site in the tree — twenty-one commands this series added carried the same defect. 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>
The tenant and the VSPC tenant are both resolved when the account holds only one, and named with --tenant / --vspc when it holds several. That resolution is shared: `backup-services` uses it, and so does `baremetal backup-agent`, which reaches the same resources starting from the machine they protect. The two flags were only registered on the `backup-services` tree. On an account with more than one backup tenant — or more than one VSPC tenant inside it — every `baremetal backup-agent` command therefore failed with "name one with --tenant", and cobra then answered "unknown flag: --tenant". The whole subtree was unreachable, and its refusal told the operator to do something the command could not do. Verified against the built binary before and after. Registered through one helper so the two trees cannot drift apart again, and the comment says why it is a function rather than two lines. Two tests. The first runs the command rather than inspecting a FlagSet, because what matters is what cobra accepts on the command line; the existing test asserted only that the refusal *named* the flag, which is what let this through. The second pins both trees to the same pair. A compiling sabotage that drops the registration turns both red. 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>
…it had not seen **The status column was always empty.** `licenses servers` asked for the column "status", while rowsOf writes the value under "resourceStatus" — the two-word form in every other list here is key first, header second. So the column was blank on every line, and a blank status column reads as "nothing to report" on exactly the resources somebody is checking on. **`backup-agent --wait` could not see a failure.** This generation of the API has no task route and no failed value in its status enumeration: currentTasks is the only place a failure appears. The sibling package reads it; this one did not have the field at all, so a creation whose task ended in ERROR came back as a created agent. Measured on this account: three BACKUP_VAULT_CREATE and one VSPC_AGENT_UPDATE sat in ERROR for five minutes on resources every status field called READY. The tasks are read now, in both directions — a failed removal leaves the agent in place, so the loop would otherwise poll a corpse for its whole timeout and then blame its own patience. WAITING_USER_INPUT is treated as terminal: it is not an error, but only a person can clear it, and holding the terminal open for that tells nobody anything. An empty status is no longer taken for a finished one either: backupAgentTransient has no entry for "", so an answer carrying no status at all counted as settled. **Billing emitted two JSON documents.** When the consumption read failed, ShowBilling printed an OutputInfo document and then the table, so `-o json` put two documents on stdout back to back — which no parser accepts. A script asking for the prices got a decode error precisely on the runs where half the data was missing, defeating the whole point of still answering. The sentence goes to the log, which is stderr, and the fact goes into the consumption cell of every row, where a script will find it. "unreadable" and "none yet" are kept apart: an empty usage list is the normal answer for a freshly ordered vault. Found while testing that: currentUsage was a sync.OnceValues over a live API read — a process-wide cache over data that changes. The first call decided the answer for the life of the process, so a transient 500 on that first read made every later billing command in the same process report the consumption as unreadable. Every other once-value in this repository wraps the embedded schema, which cannot change. It is a plain function now. This is also why one test passed alone and failed in its suite, which is how it was found. Nine tests, seven sabotages, seven reds. Two of the seven first came back green for the wrong reason: one broke the build, and the two-documents one could not be seen at all through the return value of Execute, because every writer overwrites display.ResultString — the two documents only exist on real stdout. That test now captures os.Stdout, which is the only place this defect was ever visible. 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>
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/backup-services
Signed-off-by: Denis <denis.hamon@ovhcloud.com>
Contributor
Author
Human review — Works as intendedM4 ·
Dont call it "Veeam backup agent", call it "Backup Agent" — Antoine Ruelle (PU Lead), through CLI sandbox review page |
1 similar comment
Contributor
Author
Human review — Works as intendedM4 ·
Dont call it "Veeam backup agent", call it "Backup Agent" — Antoine Ruelle (PU Lead), through CLI sandbox review page |
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 is
Seventeen v2 paths manage the Veeam Backup product — tenants, storage vaults, the Service Provider Console that drives the agents — and none had a command. Not skipped: invisible, because this repository could only fetch v1 schemas until #262.
All 25 operations are badged Beta version upstream.
The hierarchy is three UUIDs deep, so it is resolved
A tenant holds a VSPC tenant which holds the agents. Nobody knows any of those identifiers. One of each is taken silently; several are refused with an identifier to paste and a name to recognise; none is an answer, not a stack of failed lookups.
--tenantand--vspcexist for the accounts where there is a choice.An agent is reached from the machine it protects
backup.AgentProductTypeEnumlistsOVHCLOUD_BAREMETALas a first-class product type, and an agent names its server inproductResourceName. Measured on the real account:OVHCLOUD_BAREMETALagent-<server>ips= the server's own address in a /32region= the server's own regionThat correspondence is why
createneeds nothing but a server name:Three things the measurement decided
NOT_INSTALLEDis where a created agent stops, not a step on the way. The object exists; the software is not on the machine. All nine agents of this account are in that state, with an empty policy, while two retention policies sit beside them — nine provisioned, none deployed, none retaining anything. So it counts as settled for--wait, and the success message says what is left to do instead of "created":The PUT replaces the target spec, so
editcarries over what it is not changing. Sending only--policywould blank the display name and the addresses — the trapiam resource edit --tagstill has.vault bucket credentialsis deliberately absent. The schema declares aGET; the API answers 405 on all three buckets of this account, with noAllowheader. Controls, same client and token:GET .../bucket/{realId}/credentialsAllowGET .../bucket/{fakeId}/credentialsGET .../bucket/{realId}OPTIONS .../credentialsThe route resolves the bucket and then refuses the method, so the command would always fail. Same reason
install/hardwareRaidSizewas left out of #250. Worth reporting to the owner of the route — sixth API defect found by this audit.Two columns exist because the object hides what you are looking for
Tasks. This generation of the API has no task route to poll: a resource carries the tasks running on it, and that list is the only place a failure appears. The column names the operation, because
ERRORanswers "error doing what?" with nothing.Not
allowedIps. It looks like a field of a vault and it is not — the addresses allowed to reach one are listed in the VSPC tenant's own view of its vaults. Reading it off the vault resource answers 0 where the truth is 9, and a column that is wrong reads worse than a column that is missing. A test pins its absence.The tasks column found something on the first run
Three failed
BACKUP_VAULT_CREATE, one failedVSPC_AGENT_UPDATE, twoVSPC_TENANT_UPDATEstuck inPENDING— sampled six times over five minutes, identical identifiers and statuses each time — on resources that all reportREADY. Nothing surfaced them before.Two things about those tasks are worth noting upstream:
errorsis declared on a task in the schema and is empty even on a failed one, andlink— documented as "Link to the task details" — is the empty string. A failure with no message and nowhere to look.Verified against the API
Every read exercised live: tenants, vaults (3), buckets, VSPC tenant, policies (
14d_retention,30d_retention), licences (none, which is an answer), deploy script, and the agents of a server that has one and of one that has not.Refusals executed live: unknown retention policy, edit and delete on a server with no agent, and
--dry-runon create and delete.The write cycle, run end to end
Authorised by the account owner and run on
ns3118333.ip-51-68-100.eu, a server with no agent:create --wait --yesNOT_INSTALLED, name / address / region derived from the server51.68.100.165/32added — the confirmation prompt was rightedit --policy 14d_retentionedit --policy ''(a write that changes nothing)delete --wait --yesTwo things came out of it that no amount of reading the schema would have shown.
The
PUTanswers 500. Twice, with two query ids, and the second call is the control: it sets the target spec to exactly what it already was, so the failure is not about the value being applied. The command reports it and claims nothing — the state re-read outside the CLI is unchanged. Root cause is almost certainly BKP-1517, filed by the backup team the day before; the API-side evidence has been added there as a comment.Deleting an agent does not take back its storage authorisation. After the delete, the three vaults still allow
51.68.100.165/32— 10 addresses for 9 agents, and the orphan is exactly the deleted agent's. Sampled three times over 90 seconds and again later: not eventual consistency. Before the cycle the correspondence was exact, 9 ↔ 9, zero orphans. Filed as BKP-1523; on a customer account every agent ever deleted leaves a permanent entry in the allow-list of the backup vaults.Against LVL2-19664
The acceptance criteria of the product ticket, and where each one lands:
backup-services agentsbaremetal backup-agent show <server>per machinebaremetal backup-agent create <server>baremetal backup-agent edit --policybackup-services deploy-script(aliasdownload-agent)backup-services vault list/get/bucketsbackup-services billingThe billing view needed a join the backup API does not offer: a v2 resource carries no price at all. Measured —
/v1/services?resourceName=<uuid>answers with exactly one billable service for the backup tenant, for the VSPC tenant and for each vault, so it is one lookup per resource rather than a sweep of the 826 services on this account.none yetis a fact — nothing has been stored, every agent isNOT_INSTALLED. Consumption that could not be read saysunknowninstead: a failed read is not a reading of zero.What is not covered, and why. Ordering (the SHOULD) goes through the order cart, which is its own surface and its own lot. Account-level invoices (
/v1/me/bill) are account-wide and no command exposes them yet; scoping them to one product belongs to anaccountlot rather than to this one.Gate
go build,go vet,make wasm,go test ./..., docgen — all clean.backup-servicesis added to the hand-written command index, whichmake docdiscards and therefore never updates on its own.36 tests (13 unit, 19 cobra) and 22 sabotages, 22 red — each on the test it was meant to exercise, restoration checked by digest.
Stacked on #264 → #263 → #262 → #261.