feat(baremetal): read the traffic graphs, without knowing a MAC address - #254
Draft
Denis-hamon wants to merge 18 commits into
Draft
feat(baremetal): read the traffic graphs, without knowing a MAC address#254Denis-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>
…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>
The CLI reached none of the traffic data. Three routes carry it and two are
dead ends: /mrtg and /vrack/{vrack}/mrtg were deprecated on 2017-10-23, with
deletion announced for 2018-04-23, both pointing at the per-controller route.
Only /networkInterfaceController/{mac}/mrtg is live, so it is the only one
wired here.
That route wants a MAC address, and nothing in this CLI could tell you one.
No command lists the controllers of a server; `vni list` shows the virtual
interfaces, and their own MAC list appears only under -o json. Asking for a MAC
nobody can look up would be a command nobody can run, so `traffic` resolves the
server to its controllers and reads every one of them.
ovhcloud baremetal traffic <server> [--period daily]
[--type traffic:download,traffic:upload]
[--nic <mac>]
Both directions are read by default, because "how much is this server doing" is
a question about both and answering one of them silently answers half.
The periods and types are checked against the API enums before the call, so a
typo comes back with the list of what would have worked rather than as a 400.
Numbers are printed twice on purpose. The table shows a rate somebody can read
— 33.98 Mbps, 2.87 kpps — because the raw figures are 33978092.769 and an
average of 0.16518435754189942, which are true and useless. `-o json` and
`-o peak` keep the raw values, and carry the whole series with them: a day is
173 samples, which no table can show and every script wants. Dropping it would
have made the CLI's output poorer than the API it wraps.
Every unit this API returns is a rate — bps, pps and eps, all three seen on a
real server — so one scale serves all three.
Measured against a real ADVANCE-1, on the day it was reinstalled:
nic type peak average latest
9c:6b:00:a9:c5:10 traffic:download 33.98 Mbps 233.84 kbps 46.92 kbps
9c:6b:00:a9:c5:10 traffic:upload 116.23 kbps 1.69 kbps 2.32 kbps
9c:6b:00:a9:ca:25 traffic:download 944.68 bps 774.36 bps 739.16 bps
9c:6b:00:a9:ca:25 traffic:upload 15.73 bps 0.17 bps 0 bps
The peak is the operating system image being downloaded during that reinstall.
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
--period and --type were validated against two string slices written by hand in traffic.go. The values match dedicated.server.MrtgPeriodEnum and dedicated.server.MrtgTypeEnum today, which is precisely why nobody would notice them drifting apart: a copied list does not fail, it quietly refuses something the API accepts. This repository has already paid for that. ovh#256 shipped a game-protocol list copied the same way, and it refused arkSurvivalAscended — a protocol the API returns in supportedProtocols and that a live rule on a real account was already using. Both lists are now read from the embedded schema through openapi.GetComponentEnum behind sync.OnceValues, so the day the schema is refreshed the flags follow. The refusal still lists what is accepted, and now it lists what the schema says rather than what someone remembered. The shared helper for this, common.CheckEnumFlag, does not exist yet at this point of the stack — it arrives in a later branch. A local helper does the same job here and can be collapsed into the shared one when the stack lands. Found by a third-party review pass over this branch. 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-progress
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>
…ts default Two defects in the same command. The API declares a graph sample nullable and means it. Swept over ten servers of this account, 348 of 5311 yearly samples came back null — 90 of 273 on one machine, and its very first sample was one of them. Decoded into a non-pointer struct, every one of those became a real zero, so two of the three figures the table prints were wrong and printed as facts: that machine's average read 14039 bps where the truth is 20944, understated by a third by samples that do not exist, and the unit — taken from the first sample — was the empty string, so the row read "20.94 k" with no unit at all. The sample is now a pointer. An absent sample takes part in nothing: not the peak, not the total, not the divisor of the average, and not the unit, which comes from the first sample that has one. How many were absent travels with the figures, because a window a third of which was never recorded is a caveat on every number on the line. When every sample is absent the command says so rather than printing three zeros, which would claim the interface carried no traffic. Second: --type was the only slice flag in the CLI with a non-empty default, and PostExecute puts a used slice flag back to nil rather than to its default — DefValue is "[]" for a slice, so there is nothing else it could use. The second `baremetal traffic` of a process therefore looped over an empty list and printed an empty table with exit 0. The default now lives next to the code that reads it and an empty --type resolves to it, which holds however the slice came to be empty. Fixing it here rather than in root.go keeps the change out of the one file every branch of this series touches. Three tests, three sabotages, three reds. The cross-invocation test is the one that bites: a single run cannot see this, which is how the flag shipped that way. 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/baremetal-traffic
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.
The CLI reached none of the traffic data
Three routes carry it, and two are dead ends:
/dedicated/server/{sn}/mrtg/dedicated/server/{sn}/vrack/{vrack}/mrtg/dedicated/server/{sn}/networkInterfaceController/{mac}/mrtgBoth deprecated ones name
/networkInterfaceControlleras their replacement,so only that one is wired here.
The route wants a MAC address, and nothing could tell you one
No command lists the controllers of a server.
vni listshows the virtualinterfaces, and their own MAC list appears only under
-o json. Asking anoperator for a MAC they have no way to look up would be a command nobody can
run, so
trafficresolves the server to its controllers and reads every one.Both directions are read by default: how much is this server doing is a
question about both, and answering one of them silently answers half.
Periods and types are checked against the API enums before the call, so a typo
comes back with the list of what would have worked rather than as a 400.
Numbers appear twice, on purpose
Measured against a real ADVANCE-1, on the day it was reinstalled:
That peak is the operating system image being downloaded during the reinstall.
The raw figures behind those cells are
33978092.769and an average of0.16518435754189942— true, and useless to read. So the table shows a scaledrate, while
-o jsonand-o peakkeep the raw values:-o jsonalso carries the whole series. A day is 173 samples, which notable can show and every script wants; dropping it would make the CLI's output
poorer than the API it wraps.
Every unit this API returns is a rate —
bps,ppsandeps, all three seenon a real server — so one scale serves all three:
33.98 Mbps,2.87 kpps,0 eps.Verification
go build,go vet,make wasm,go test ./..., docgen — all green.10 tests. 7 sabotages, 7 red, each on the test meant to catch it: the scale
removed, the series dropped,
latestreplaced by the peak, an empty seriesreported as zero, only the first controller read, and each of the two enum
checks disabled.
One of those sabotages first reported no test falls — it disabled the error
branch of
controllersOfrather than shortening its result, so it neverrestricted anything. Re-applied where it belonged, the test fell. The zero came
from the instrument, not from the code.
Left out deliberately
GET/PUT /dedicated/server/{sn}/burstis live and uncovered, but it answers404 "The requested object (burst) does not exist" on 8 of the 8 servers
checked. The read path could be shipped and would report that answer, but the
path that returns a burst configuration cannot be exercised on this account, so
there is no command for it here.