Skip to content

Sort the partition capacity response deterministically - #303

Merged
majst01 merged 1 commit into
mainfrom
fix/partition-capacity-size-ordering
Sep 3, 2026
Merged

Sort the partition capacity response deterministically#303
majst01 merged 1 commit into
mainfrom
fix/partition-capacity-size-ordering

Conversation

@muhittink

Copy link
Copy Markdown
Contributor

Description

Follow-up to metal-stack/metal-api#656. That pull request fixed the flaky TestPartitionCapacity in metal-api by sorting the capacity response; @majst01 asked for it to be done here instead. This is the same fix, reduced to what is actually missing in this repository.

Capacity already sorts the partition list before returning it:

// Prevent flaky tests
sort.SliceStable(res, func(i, j int) bool {
	return res[i].Partition < res[j].Partition
})

Three sibling fields are not sorted, and they come from the same place. Machine().List() builds its term without OrderBy — the string appears nowhere in pkg/db — so the row order RethinkDB returns is unspecified, and MachineSizeCapacities (order of first appearance of each size), FaultyMachines and OtherMachines (append order) inherit it.

The test is green by accident, not by design

Test_partitionServiceServer_Capacity/non_filter_considers_all_machines has one partition with two sizes and expects c1-large-x86 before n1-medium-x86.

Measured, with a temporary probe printing the ids as Machine().List() returns them: the order is [3 4 2 1 5]. Not the insertion order (reversing the fixture changes nothing), not id order — arbitrary, and identical across 6 separate processes with a fresh RethinkDB container each. The test passes because machine …0003 happens to be a c1-large-x86 machine.

Relabelling the fixture — giving n1-medium-x86 the id …0003 and the third c1-large-x86 machine the id …0004, the same scenario with the ids moved — makes the subtest fail:

"machine_size_capacities": []protocmp.Message{
-   s`{size:"c1-large-x86", total:3, phoned_home:3, allocated:3, faulty:1, ...}`,
    {... "size": "n1-medium-x86" ...},
+   s`{size:"c1-large-x86", total:3, phoned_home:3, allocated:3, faulty:1, ...}`,
},

Every counter is intact; only the position moved. With this change the relabelled fixture passes 3 of 3.

What this does not claim

There is no reproducible flake here today, and this fixes no failing test. The genuinely random axis — Go map iteration over the partitions — is the one already covered by the existing sort. What is left is a response order that nothing specifies and that happens to be stable for the current set of document ids.

metal-stack/cli already re-sorts two of the three axes on the client (cmd/admin/v2/partition.go sorts MachineSizeCapacities by size, then PartitionCapacitySorter by partition), so an operator does not see this. Nothing re-sorts FaultyMachines and OtherMachines.

Size of the change

Six lines, in the loop that already exists, using the sort package the file already imports. No new dependency, no new import, no existing expectation touched.

Verification

$ go build ./... && go vet ./pkg/repository/
$ go test ./pkg/service/admin/partition/ -run Test_partitionServiceServer_Capacity -count=1
ok  	17 subtests, 0 FAIL

Two pre-existing failures, both reproduced on main without this change:

  • -count=3 on the capacity test fails in one_allocated_machine on the second and third iteration - a state leak between iterations of the test function.
  • make test fails in TestMachineCreateIntegration with too many parallel machine allocations taking place: unable to acquire mutex: partition-1. 55 packages pass, that one does not - on main as well. It is a parallel-allocation test under -race and it does not touch Capacity.

Known limits

  • The sort.Strings calls on FaultyMachines and OtherMachines are defensive. No fixture holds more than one entry in either slice, so no assertion can make them fail. They are argued correct, not shown correct.
  • Nothing was run against a production RethinkDB. The statement about row order is read off the query path and measured against the test container, not observed in a cluster.
  • That sorting cannot change a computed number follows from the code — the counters accumulate with ++/+=, every reservation delta is non-negative and Free clamps at zero — not from a measurement.
  • The response order changes for consumers that are not metal-stack/cli. No such consumer was checked.

Used AI-Tools ✨

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

References:

The partition list is already sorted before the response is returned, with
the comment "Prevent flaky tests". MachineSizeCapacities, FaultyMachines and
OtherMachines are not. All three derive from the row order of
Machine().List(), which builds its term without OrderBy, so the order
RethinkDB returns is unspecified.

Test_partitionServiceServer_Capacity passes today only because the document
ids in the fixture happen to place a c1-large machine first. Swapping the ids
of the n1-medium machine and one c1-large machine - the same scenario, only
relabelled - makes the subtest fail on the ordering alone, with every counter
intact.

Uses the sort package that the file already imports, no new dependency and no
change to any existing expectation.

Generated-By: Claude Code (Opus 5)
Generated-By: deepseek-v4-flash
@codecov

codecov Bot commented Sep 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.92%. Comparing base (bb7c6ce) to head (208d468).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #303      +/-   ##
==========================================
- Coverage   79.93%   79.92%   -0.01%     
==========================================
  Files         199      199              
  Lines       14800    14804       +4     
==========================================
+ Hits        11830    11832       +2     
- Misses       2969     2971       +2     
  Partials        1        1              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@muhittink
muhittink marked this pull request as ready for review September 2, 2026 17:45
@muhittink
muhittink requested a review from a team as a code owner September 2, 2026 17:45
@majst01
majst01 merged commit 88073b1 into main Sep 3, 2026
17 checks passed
@majst01
majst01 deleted the fix/partition-capacity-size-ordering branch September 3, 2026 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: control-plane Affects the metal-stack control-plane area.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants