Conversation
Sets up the test infrastructure for the upcoming pod-pool partition work. NewControllerPodAt lets a single fake clientset hold multiple controller pods with distinct PodIPs so their hash rings can agree on N peers. The multiControllerFixture helper builds N Controllers that share one fake clientset and starts informers in that pre-seeded state. BenchmarkAssignPodContention exercises the assignment hot path under concurrent calls from N controllers and reports ns/op, p99-ms, and patches/success. The fake clientset does not simulate optimistic-concurrency on Patch, so the numbers are not directly comparable to production tail latency; this baseline exists to catch regressions and to set the forward-looking acceptance bar for the partitioned implementation.
|
This change is part of the following stack: Change managed by git-spice. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ad5e57d. Configure here.
| for range poolPerFunction { | ||
| poolObjs = append(poolObjs, benchAvailablePod(fns[c], sharedHost, int32(sharedPort))) | ||
| } | ||
| } |
There was a problem hiding this comment.
All functions share same deployment, making pools indistinct
Medium Severity
fixture.NewFunction always sets Deployment to the constant "test". Since getUnassignedPods selects candidates using the key.Deployment.Label selector matching fn.GetDeployment(), all three functions query the same label value "test", making all 60K pods (3 × 20K) a single shared pool. The benchmark claims to drive "distinct functions" but pods aren't actually partitioned per function — every controller contends for the same undifferentiated pool, which misrepresents the contention pattern the benchmark is designed to measure.
Reviewed by Cursor Bugbot for commit ad5e57d. Configure here.
| _, err := ctrl.assignPod(ctx, fn) | ||
| elapsed := time.Since(start) | ||
| if err != nil { | ||
| b.Fatalf("assignPod failed (goroutine %d): %v", gid, err) |
There was a problem hiding this comment.
b.Fatalf called from non-test goroutine in RunParallel
Medium Severity
b.Fatalf is called inside the b.RunParallel callback, which runs in goroutines spawned by RunParallel, not the benchmark's main goroutine. The Go testing docs state that FailNow/Fatal/Fatalf "must be called from the goroutine running the test or benchmark function, not from other goroutines created during the test." On Go 1.24+ (this project uses Go 1.26), this can panic or produce unreliable behavior where only the calling goroutine exits while other goroutines continue.
Reviewed by Cursor Bugbot for commit ad5e57d. Configure here.
This branch has not been deployed
1 participant
Sets up the test harness for the partitioned pod-assignment work that follows. No behavior change in this PR.
fixture.NewControllerPodAt(name, ip)lets a single fake clientset seed multiple controller pods with distinctPodIPs so their hash rings can agree on N peers.multiControllerFixture(ininternal/controller/multicontroller_test.go) builds NControllerinstances that share one fake clientset, with peer seeding done beforestartInformersso the controllers pick the peers up via initial-list without overflowing the fake watcher's small event channel.BenchmarkAssignPodContention(ininternal/controller/pod_bench_test.go) drives the assignment hot path with N=3 controllers callingassignPodconcurrently for distinct functions. It reportsns/op, ap99-mspercentile, and apatches/successratio computed via apatchreactor on the fake clientset. The fake serializesPatchthrough a tracker mutex and does not implement optimistic concurrency onResourceVersion, so the wall-clock numbers are not directly comparable to production tail latency; this bench exists to set the forward-looking acceptance bar for the partition work that comes next and to catch regressions in the assignment hot path between commits.Stacked on #143 (router-side in-flight counter decoupling), which is the prerequisite for this work.