Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
graveland
force-pushed
the
graveland/multi-controller-suite
branch
2 times, most recently
from
September 25, 2026 15:56
f05ee6e to
1b474ad
Compare
graveland
commented
Sep 25, 2026
| - name: Run tests | ||
| run: go test ./... -coverprofile=./cover.out -covermode=atomic -coverpkg=./... | ||
| run: | | ||
| go test $(go list ./... | grep -v /test/suite) \ |
Contributor
Author
There was a problem hiding this comment.
This is to avoid an extra full test suite test run here adding a few minutes, keeping the suite run to its own step
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
graveland
force-pushed
the
graveland/multi-controller-suite
branch
from
September 25, 2026 17:30
1b474ad to
a8545cd
Compare
This comment has been minimized.
This comment has been minimized.
graveland
marked this pull request as ready for review
September 25, 2026 19:06
graveland
changed the title
A healthy Shard rewrote its status about ten times a second, forever.
Two server-side-apply field managers wrote the same status fields:
setStorageClassCondition's guard apply and updateStatus each behaved
correctly alone, but together they flipped status.orchReady and
status.poolsReady false/true and alternated the StorageClassValid
condition's message between two strings, with no terminal state.
The guard built its apply payload from a typed ShardStatus literal
that only set Conditions, but an SSA payload is a complete statement
of ownership: the zero values of every other field on that literal
(orchReady:false, poolsReady:false) got serialised too, and with
ForceOwnership the guard seized both from updateStatus on every
reconcile, which made updateStatus reclaim them right back next time.
The guard now applies an unstructured payload that carries only
status.conditions, so StorageClassValid has exactly one writer.
Other visible changes along the way:
- the StorageClassValid condition's messages changed: one verdict
now covers both the backup and pool StorageClass checks instead
of each reporting its own;
- pool StorageClass lookups now happen before the Multiorch
reconcile block (where the backup lookup already ran), instead of
after it;
- pools are checked in sorted name order instead of Go's randomised
map order, which was a second, independent source of the
condition message flapping;
- the missing-StorageClass warning event is now recorded in
Reconcile itself rather than inside the validation helpers, and
the FailedApply event wording for a validation error changed
accordingly.
Coverage said nothing, because both halves were executed by existing
tests and each was right in isolation: the defect only existed when
both writers hit one API server. Added coverage now pins the guard's
single-field payload and skip-if-unchanged behaviour, plus that
Reconcile itself never makes more than one guard apply per pass.
Signed-off-by: Brent Graveland <graveland@supabase.io>
graveland
force-pushed
the
graveland/multi-controller-suite
branch
from
September 26, 2026 23:09
059612b to
225dddd
Compare
This comment has been minimized.
This comment has been minimized.
Contributor
Author
|
/e2e |
|
✅ E2E tests success — View run |
graveland
added this pull request to stack #685
September 27, 2026 02:03
The operator half of the multi-controller suite: the fakes, the cluster fixture, shard identity helpers, and the scenario tests themselves, written as a consumer of github.com/multigres/testkit/ctrltest. A scenario test exercises a joint between controllers, which is what the existing tier cannot reach. The deletion protocol runs shard to tablegroup to multigrescluster; the fan-out is asserted as a sequence rather than an end state; the race test pins two reconcilers writing one object. Others cover shard lifecycle, selector impostors, transitions and round-trip equality, and thrash. Seven live operator defects are pinned with KnownDefect, so each one reproduces its own evidence on every run rather than only in a document. A pin passes while its defect is present and fails the day it is fixed, so the fix has to replace the pin with a positive assertion in the same change. The pool scale-up pin constructs its race rather than sampling it. The defect needs the new pooler to register after the shard has converged, which happens naturally about a third of the time. poolerSim can hold new registrations for one namespace, so the test holds, scales up, waits for two pool pods and one entry in status.podRoles to stay stable, then releases. The pooler then appears in etcd with no Kubernetes event to announce it. That precondition is deliberately a stability window rather than RequireQuiescent: once the defect is fixed, the shard requeues while the pooler is held and the namespace never goes quiet. Tests open with newCase(t), which allocates the namespace, registers it at the reconcile gate and attaches the failure dump, and everything hangs off that receiver: the assertions, the harness, the client verbs, and this package's own vocabulary. Sub(t) is the subtest form, keeping the namespace while binding the subtest T. Bare(t) is for the pure-logic tests that never touch the cluster. Tests scope their work to the case namespace throughout. envtest never really deletes a namespace, so an unscoped List would see everything every earlier test in the run created. Signed-off-by: Brent Graveland <graveland@supabase.io>
make test-suite runs it, and the other test targets exclude test/suite by path filter: the tier has no build tag, deliberately, so nothing can be hidden behind one. The job is advisory rather than required, because the suite pins live operator defects and a required check would block every PR on defects nobody is fixing in that PR. It runs verbosely, which is what makes a passing run readable: a pin that is doing its job logs and passes, and Go discards that output without -v, so a suite with seven live pins would otherwise print the same thing as a suite with none. Signed-off-by: Brent Graveland <graveland@supabase.io>
Nothing in this repo ran -race, which is an odd gap for the one suite where five controllers share a manager. Measured on the first run: 183s against a 166s baseline and zero data races. The 10% is cheaper than expected because this suite spends most of its wall clock waiting for controllers to converge, and the race detector does not slow down waiting. Separate from test-suite anyway. Certificate generation is the one CPU-bound step and has been measured swinging between 14 and 75 seconds under -race, which is enough to turn a wait sized against the normal run into a flake, so the timeout here is deliberately loose. The operator holds exactly one piece of state across reconcile goroutines, ShardReconciler.postureStrikes, and it is mutex-guarded with controller- runtime already serialising per object key. So this is a standing check that the answer has not changed rather than a hunt for a known race. Signed-off-by: Brent Graveland <graveland@supabase.io>
A patch without client.FieldOwner does not opt out of field management. The API server derives one from the client's User-Agent, so the write gets an owner nobody named and nobody can see, and that owner then co-owns whatever fields the patch touched. Four Shard status writes did this, and the fields they claimed are also claimed by updateStatus on every reconcile, so two managers held them jointly. That is the same structure as the status hot loop: two managers, one object, overlapping fields. It was not looping, because these are merge patches that agree on values rather than applies that disagree, but it is one changed value away from the same outcome. The Pod status write in reconcile_readiness.go takes a distinct manager rather than the Shard's. It claims one condition on a Pod whose status otherwise belongs to kubelet, and a manager name is the only record of which concern took a field. The multi-controller suite pinned this with a KnownDefect on the field-ownership check in TestTwoControllersWriteOneShard. The pin becomes the positive assertion it was waiting to be: no field on the Shard is claimed by more than one manager. Signed-off-by: Brent Graveland <graveland@supabase.io>
testkit's assertions are generic methods, which need Go 1.27, so adding it as a test dependency raised this module's go directive from 1.26.6. Pin it to the exact release and move the builder image with it: official Go images set GOTOOLCHAIN=local, so a 1.26.6 builder refuses a module that asks for 1.27. golangci-lint goes to v2.13.2. v2.12.2's bundled staticcheck IR builder cannot parse Go 1.27 syntax and panics on the standard library (buildir: package "poll": unexpected expr: *ast.KeyValueExpr), which fails every lint run on Linux. Go 1.27 support landed in v2.13.0. The newer staticcheck names deprecated symbols by full package path, so four of the existing controller-runtime deprecation exclusions stopped matching. Their patterns now accept either form rather than adding new exclusions; the deprecations and the follow-up migration are unchanged. The golangci-lint binary's name now also carries the Go version it was built with. CI restores bin/ from older caches, and a name keyed only on the linter's version reused a binary built by go1.26 after the bump, which refused to load a go1.27 module. Any future Go bump would repeat that. tools/observer is a separate module that does not use testkit and stays on 1.26.6. Signed-off-by: Brent Graveland <graveland@supabase.io>
The commit that stopped the shard controller's status hot loop ("stop the
status hot loop on a healthy shard") removed a driver that used to rerun
every shard several times a second regardless of what reconcile asked for.
That accidentally covered for a gap: once a posture observation settles
(nothing inconsistent, nothing incomplete, or an unsettled observation
accepted after its debounce), the shard requests no further reconcile at
all, even when a managed pod has never reached posture readiness, or a
shard mid-bootstrap has every pooler registered but no primary elected yet.
Nothing in Kubernetes watches the topology store, so nothing else wakes the
shard either: an accepted RPC blip or role mismatch leaves a pod NotReady or
a shard Degraded until controller-runtime's 10h resync, and a fresh cluster's
pool pods never go Ready at all.
reconcilePosture now requests a requeue for any not-converged state,
unsettled or merely not-ready, once the existing debounce for unsettled
observations ends. The delay is clamped elapsed time since the shard was
first observed not converged, five seconds to one minute, with up to 20%
upward jitter (so five seconds to about seventy-two seconds including
jitter), and resets the moment the shard converges. Elapsed time rather than
a per-reconcile count, because pod status transitions, drain requeues and
the operator's own status patches are each their own reconcile with no
predicate filtering them, and a burst of those must not by itself run the
backoff up to its ceiling.
Scopes the posture pod list to pool pods: a shard's multiorch pod carries the
same four identity labels and would otherwise count as a pod that never
becomes ready. The pod-roles, drain, and pooler-prune lists are scoped the
same way for consistency; pooler-prune's filter is the one that matters
operationally, since it decides which topology poolers this reconcile marks
LIFECYCLE_SHUTDOWN, and every pool pod has carried the component label since
the pool controller was introduced, so this is not a behaviour change for
existing clusters.
test/suite's pool-scale-up pin is now a positive assertion: the role
landing in status.podRoles after a scale-up whose pooler registers late
used to be a coin flip, and is deterministic with this fix in place.
Signed-off-by: Brent Graveland <graveland@supabase.io>
graveland
force-pushed
the
graveland/multi-controller-suite
branch
from
September 27, 2026 15:13
225dddd to
7d062a1
Compare
🔬 Go Test Coverage ReportSummary
Status✅ PASS DetailShow New Coverage |
Contributor
Author
|
/e2e |
|
✅ E2E tests success — View run |
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
1 participant
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
A scenario-test tier under
test/suite, built ongithub.com/multigres/testkit(v0.2.1).It boots all five reconcilers against one envtest API server and tests the
joints between controllers, which the per-controller tests cannot reach: the
deletion protocol from shard to tablegroup to multigrescluster, cluster fan-out
as a sequence, two reconcilers writing one object, shard lifecycle, selector
impostors, transitions and thrash.
Commits, in order:
fix(shard): stop the status hot loop on a healthy shard. A healthy Shardrewrote its status about ten times a second, forever, because two
server-side-apply field managers claimed the same status fields.
test(suite): add the scenario testsci: add an advisory job for the multi-controller suitetest: add a race-detector target for the multi-controller suitefix(shard): give every status write an explicit field owner. Four Shardstatus writes had no field owner, so a second, unnamed manager co-owned
fields
updateStatusalso claims.chore: build with Go 1.27.1. testkit needs Go 1.27 (its assertions aregeneric methods), so this raises the module's Go floor and the builder
image with it, bumps golangci-lint to v2.13.2 (v2.12.2 panics on Go 1.27
syntax), and fixes a CI cache trap the bump exposed.
fix(shard): requeue while a shard has not converged. Required bycommit 1, see below. A shard requeues while any pool pod is not yet
posture-ready or a posture observation is unsettled, backing off by
elapsed time from 5s to about 72s; a converged shard still requests
nothing.
Linear
orchReady=false/poolsReady=false. That is commit 1's defect: the StorageClass guard's server-side apply sent a full typedShardStatus, so its zero values took both fields away fromupdateStatus.Why these fixes ride along
The suite cannot land before fix 1. Without it no namespace containing a Shard
ever quiesces (about 570 state changes per 30s), so every convergence test
fails on its quiescence wait. With it,
TestShardStatusQuiescessettles inabout 10s.
Fix 7 is required by fix 1, and commit 1 alone breaks cluster bring-up.
The status hot loop was, by accident, re-running every shard several times a
second. Bring-up depended on that: while multiorch is still electing a
primary nothing in Kubernetes changes, and the shard requested no requeue, so
once the loop was gone the shard went quiet, pool pods never went Ready, and
e2e timed out. Bisected with the
minimale2e package, one package per freshkind cluster:
minimale2emainmain+ commit 1Anyone bisecting onto commit 1 alone will see e2e fail; that is expected, and
commit 7's message says so.
Fixes 5 and 7 are here to show how the suite proves a fix. Commit 2 pins each defect
with
KnownDefect; commits 5 and 7 each fix one and turn its pin into apositive assertion (commit 7's is a pooler's role never landing after a
scale-up, which also bites a brand-new shard). The scenario reproduces the
defect on
mainand does not with the fix.Pins
Seven live operator defects are pinned on
main(five after commits 5 and7). A pin
passes while its defect is present and fails the day it is fixed, with a
message saying to replace it with a positive assertion, so a fix cannot land
without updating its pin. Both pins added here were checked in both
directions: "still present" without the fix, "appears fixed" with it.
Notable details / risks
for
go.modand theDockerfilebuilder image. Driven by a testdependency, so worth an explicit yes from maintainers.
tools/observeris aseparate module and stays on 1.26.6.
symbols with their full package path, so four existing controller-runtime
deprecation exclusions in
.golangci.tomlwere widened to match both forms.No new exclusions.
bin/is now named for the Go version thatbuilt it (
golangci-lint-v2.13.2-go1.27.1). CI restoresbin/from oldercaches, and without this the lint job ran a go1.26-built linter against the
go1.27 module and refused to load.
shard_controller.go,storage_class_guard.go,reconcile_*.go,reconcile_data_plane.goandpkg/data-handler/posture. Everything else is test code, CI and theMakefile.
pod-roles, drain and prune pod lists to pool pods
(
app.kubernetes.io/component=shard-pool, which every pool pod has alwayscarried); without it a shard's own multiorch pod counts as a pod that never
becomes ready. Prune is the one worth a look: its list decides which
topology poolers get marked shut down.
StorageClassValidcondition messages change,pool StorageClass lookups move ahead of multiorch, pools are visited in
sorted order, and the missing-class event moves into
Reconcile. Details inthe commit message.
defects.
-racerun, locally on thefinal test code: 19/20 serial green,
-racegreen with zero dataraces, median 192s per run. All six remaining pins reported "still
present" in all 21 runs, no false expiries. The one failure was
TestSuiteCompressesRequeues(30s against a usual 0.3s): the shard in thatnamespace went quiet after creating its pool pods and never made the
requeue the test waits for. Root cause: the pinned
MGO-POOL-SCALEUP-ROLE-STALE defect on a fresh shard, when the first pooler
registers before the shard's first data-plane pass. The two requeue tests
now hold pooler registration so they always see the empty topology they
assert about.