Conversation
This comment has been minimized.
This comment has been minimized.
Contributor
Author
|
/e2e |
|
❌ E2E tests failure — View run |
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>
Mechanical, produced entirely by the tool rather than by hand, after
bumping github.com/multigres/testkit to v0.2.0 (the release that adds
the methods the tool now converts to):
go install github.com/multigres/testkit/tools/assertfix@v0.2.2
go fix -fixtool=assertfix ./...
go fix -tags=integration,verbose -fixtool=assertfix ./...
go fix -tags=e2e -fixtool=assertfix ./...
gofmt -w <changed files>
golangci-lint fmt <changed files>
The three runs are needed because go fix only sees files matching the
active build tags. The last step rewraps converted lines that exceed
golines' limit; the same files needed no formatting before conversion.
167 test files, 91% of assertion sites (3,954 of 4,337 outside
test/suite and tools/observer), and no test file outside test/suite
imports testify any more. The rest is left untouched on purpose: the
tool declines any site it cannot map with certainty, for example a
compound condition whose failure message would panic or have side
effects if evaluated on the passing path.
No test changes behaviour: t.Errorf continues and t.Fatalf aborts, so
a scope with both gets assert.NewCollecting and its aborting sites are
written c.Require().X(...). Top-level test counts are identical under
all three tag sets, and re-running the tool over the result changes
nothing.
Signed-off-by: Brent Graveland <graveland@supabase.io>
graveland
force-pushed
the
graveland/test/assert-conversion
branch
from
September 27, 2026 01:00
80dbe4f to
ea8b5fb
Compare
🔬 Go Test Coverage ReportSummary
Status✅ PASS DetailShow New Coverage |
graveland
added this pull request to stack #685
September 27, 2026 02:03
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
Converts the operator's existing tests from hand-written
if cond { t.Errorf(...) }checks and testify togithub.com/multigres/testkit/assert, and bumps testkit tov0.2.0. One commit, generated entirely by theassertfixtool (v0.2.2); nothing in it was edited by hand. The exact commands are in the commit message.Stacked on #679, which adds the testkit dependency. The base is #679's branch, so this diff shows only the conversion; it gets retargeted to
mainonce #679 merges.Why
ifchecks,testify/assert,testify/require). 91% of assertion sites convert (3,954 of 4,337), and no test file outsidetest/suiteimports testify any more.c.Eq(1, int64(1))is a compile error, not a runtime failure as in testify._test.godrops by about 3,170 lines (3.6%).How to review it
It is a generated diff, so the useful review questions are about the tool's guarantees, not individual lines:
t.Errorfcontinues andt.Fatalfaborts. A scope using both getsassert.NewCollecting, and its aborting sites becomec.Require().X(...).ifblock, with the same message and the same abort/continue behaviour.Verification
Run against the unconverted tree under the default,
integrationande2ebuild tags:go vetclean.golangci-lint(uncapped) reports zero new findings; 18 and 12 pre-existing findings underintegrationande2ego away.make testandmake test-integrationgreen.test/suitegreen, all sixKnownDefectpins still reporting "present".Notable details / risks
vet -tags=e2e) but not run, since they need kind; CI's e2e job is the real check there.v0.2.0also changes(*C).Eventuallyon a collecting receiver to report and continue instead of aborting. Nothing in this repo calls it that way (test/suiteuses aborting receivers throughout), and the suite run above covers it.assertfixv0.2.2 after its release: v0.2.0 deleted comments inside theifblocks it replaced, which lost 4 comments here (a// Empty structnote and three// Needed for the parallel test runs). v0.2.2 keeps them; that is the only difference from the earlier push.