Fix non-deterministic ordering of the partition capacity response

[muhittink]

Description

TestPartitionCapacity/non_filter_considers_all_machines fails in roughly one out of six runs. It is an ordering problem, not an arithmetic one.

calcPartitionCapacity collects the partitions in a map and appends them to the result slice by ranging over it, with no sort before the return. Go randomizes map iteration and the assertion compares with cmp.Diff, which is order sensitive. As soon as a fixture holds two partitions, the test is a coin flip.

Measured on master (afae26f, go1.26.0 linux/amd64), 100 runs of that subtest: 83 PASS, 17 FAIL. With this change: 100 PASS, 0 FAIL.

This is not a regression of #632. That pull request only swapped Machines: ms for allMs; the map and the unsorted append are older. What #632 added was the first fixture with two partitions in want - the other 15 cases hold one and are trivially stable.

It is not only the test

SearchMachines builds its term from machineTable() and chains Filter calls only; OrderBy appears nowhere in the datastore package. The row order RethinkDB returns is therefore unspecified, and four fields of the response inherit it:

field	order comes from	stable before this change
[]PartitionCapacity	Go map iteration	no
pc.ServerCapacities	first appearance of each size in the machine list	no
cap.FaultyMachines	append order	no
cap.OtherMachines	append order	no

An operator does not see the first two, because metalctl repairs them on the client: partitionCapacity() calls PartitionCapacitySorter().SortBy(...), whose default key is the partition ID, and then sorts pc.Servers by size. That has been in place since metalctl #157 (aac9723, 2022-08-24). The sort keys in this pull request are deliberately the same two, so the API starts returning what metalctl has been re-sorting for three years.

What no client repairs is FaultyMachines and OtherMachines, and what no client should have to repair is the response itself. Inside the organisation, metal-metrics-exporter keys its gauges by partition and size and is indifferent to order, but metal-python, direct API consumers and anything else reading the endpoint get an order that can change between two identical calls.

That is also the reason the sort sits in the service and not in the test. Sorting the two slices in partition-service_test.go before comparing would fix the flake just as well, and leave the response as unspecified as it is today. If you prefer the endpoint to stay unordered and the test to do the sorting, say so and I will turn it around.

This changes the ordering clients see, although for metalctl the printed table stays exactly as it is. Partitions come back sorted by ID, server capacities by size, the machine ID lists ascending.

Verification

$ go test ./cmd/metal-api/internal/service/ \
    -run 'TestPartitionCapacity/non_filter_considers_all_machines' -count=100
on afae26f:   100 RUN   83 PASS   17 FAIL
with change:  100 RUN  100 PASS    0 FAIL

$ make test-unit
8 packages ok, service package at 45.8% coverage

$ make build && git diff --exit-code spec pkg
(no change to spec/metal-api.json)

$ make test-integration
9 packages ok, 938 tests and subtests, 0 FAIL

The sort over ServerCapacities is not passing vacuously: the fixtures already list size-a before size-b, so reversing the machine order handed to mockMachines in that case (m5, m4, m3, m2, m1) puts size-b first. That reversed fixture fails 5 of 5 runs on afae26f and passes 5 of 5 with this change.

Known limits

• slices.Sort on FaultyMachines and OtherMachines is defensive. Every fixture holds at most one entry in those slices, so no assertion can make either line fail. They are argued correct, not shown correct.
• Nothing was run against a live RethinkDB. The statement about row order is read off the query path, not observed.
• That sorting cannot change a computed number follows from the code - Reservations and UsedReservations accumulate with +=, every delta is non-negative, Free clamps at zero - not from a measurement. No fixture holds two reservations for the same size and partition.
• The integration suite passes, but none of it exercises calcPartitionCapacity. It is evidence that nothing else broke, not evidence for this change. It was run locally; the integration job on this pull request failed while pulling testcontainers/ryuk from Docker Hub, before a single test started.
• Consumers were checked inside the metal-stack organisation only (metalctl re-sorts, metal-metrics-exporter keys its gauges by partition and size). Consumers outside it were not.

Used AI-Tools ✨

• Claude Code (Opus 5)
• deepseek-v4-flash

Closes #634.

[majst01]

Thanks for taking care of this, but we do not want to introduce much new code here, please try instead fixing it here: https://github.com/metal-stack/metal-apiserver

[muhittink]

OK, thx for the hint. I will prepare the PR for v2 and close this one as soon as the PR is ready.

[muhittink]

Closing in favour of metal-stack/metal-apiserver#303, as suggested. Thanks for the quick pointer.

For anyone finding this later: in metal-apiserver the partition list is already sorted (// Prevent flaky tests, f219074), so the axis that is genuinely random here — Go map iteration — is covered there. The new pull request only adds the three sibling fields that derive from the unordered row order of Machine().List(), in six lines, using the sort package the file already imports.

#634 stays open: TestPartitionCapacity/non_filter_considers_all_machines still fails in 17 of 100 runs on afae26f with go1.26.0. Happy to leave that as is if metal-api is not meant to receive changes any more.