This repository was archived by the owner on Aug 16, 2026. It is now read-only.
[Draft] feat performance 2 Parallelize genericbinder reconciles and add latency optimization plan - #61
Draft
kneumoin wants to merge 53 commits into
Draft
[Draft] feat performance 2 Parallelize genericbinder reconciles and add latency optimization plan#61kneumoin wants to merge 53 commits into
kneumoin wants to merge 53 commits into
Conversation
Raise genericbinder MaxConcurrentReconciles from 1 to 4 with a bounded rate limiter so a capture wave is not serialized through a single worker, and correct the stale comment about the SnapshotContent reverse-watch. Add the snapshot creation latency analysis, optimization and implementation-plan docs.
… latency The ManifestCaptureRequest controller watched only its own MCR and polled at a fixed 500ms RequeueAfter while waiting for SnapshotContent to adopt the ManifestCheckpoint (ownerRef handoff). On the tree scheme this 500ms gap multiplies with manifest count and dominates ManifestsArchived latency. Add a reverse watch on ManifestCheckpoint keyed by spec.manifestCaptureRequestRef (mapManifestCheckpointToMCR) so the MCR reconciles on the checkpoint Ready flip and the ownerRef handoff immediately instead of polling. The checkpoint is controller-owned by the execution ObjectKeeper, not the MCR, so Owns cannot route it; the spec back-reference is the stable link. The handler only enqueues and the 500ms self-requeue remains as a safety net.
…napshot latency Add dual-path routing for the ManifestCheckpoint wake-up (L9a): route by SnapshotContent ownerRef once adopted, otherwise resolve the owning content via a new cache field index on status.manifestCheckpointName. This removes the 500ms adoption-poll gap on the snapshotcontent side, mirroring the MCR-side L8 fix, without changing the ownership model. Also treat finalizer-add 409 conflicts in snapshotcontent as benign (requeue instead of Reconciler error), so concurrent-reconcile races no longer trigger rate-limited backoff (L9c).
…t latency Raise ManifestCaptureRequest/checkpoint controller concurrency from implicit 1 to 4 with the existing bounded rate limiter. Guard the shared Config manifest fields (MaxChunkSizeBytes, DefaultTTL, DefaultTTLStr) with an RWMutex since loadConfigFromConfigMap rewrites them on every reconcile and concurrency makes that a data race. Scoped to state-snapshotter; storage-foundation VCR concurrency is unchanged and may become the next bottleneck.
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…validation Co-authored-by: Cursor <cursoragent@cursor.com>
Switch the mirror-path SnapshotContent reads to the cached client: the genericbinder Ready mirror and the SnapshotReconciler Ready/ManifestsArchived mirrors read a watched object, so a stale cache costs at most one extra reconcile and removes a direct apiserver GET on every mirror pass. Add a dedicated cached getter so safe-to-delete and read-after-write callers keep the uncached APIReader, and pin the routing with split-client tests. The declared-child owner read stays on APIReader (correctness of the one-way ManifestsArchived latch).
Co-authored-by: Cursor <cursoragent@cursor.com>
Split TREES vs SETS benchmarks, flag SETS=10 as open, add QPS caveat and content-side diagnostic plan, move APIReader audit to an appendix; record T-mcr-wake SETS=10 validation and the remaining content-manifest tail.
…nostics Replace the full unstructured List+decode in the three genericbinder reverse-watch mappers with direct O(1) routing: bound content via spec.snapshotRef, MCR via its snapshot ownerRef, and parent content via the parent snapshot status.childrenSnapshotRefs. The existing RequeueAfter paths remain the only fallback; reconcile contract and statuses are unchanged. Add gated per-mapper invoked/enqueued counters (STATE_SNAPSHOTTER_WATCH_MAP_STATS) and make the controller-runtime metrics endpoint explicit on :8080, so enqueue and reconcile counts can be measured after the reroute (mapper cost vs critical path).
Emit V(1) exit-result trace on the Snapshot reconcile and branch markers on the root capture path (subtree-pending, mcr-present, mcr-created, mcp-not-ready, mcp-ready) plus a volume-publish requeue-override marker, so the root manifest leg gap (children-archived to root MCR, and root MCP-Ready to ManifestsArchived) can be attributed to a specific requeue/branch without guessing. Diagnostic only, no behavior change. Records the B diagnosis in the latency plan.
Metrics show root Snapshot reconciles run up to ~15s (sum 42s over 10), so a slow pass locks the key and delays servicing child-archive / MCP-ready wakes. Add total reconcile durMs to the exit trace and per-section timers (volume-leg, mcp-already-ready-check, rbac-sar, namespace-list-manifest-planning) logged when a section exceeds 150ms, to localize the slow section to the root manifest leg (in scope) versus the volume leg (out of scope for B). Diagnostic only, no behavior change.
Section timers so far account for ~3.5s (volume-leg, mcp-already-ready-check) of the observed 8-15s root reconcile passes. Add timers for the untimed sections (child-graph-planning, root-object-keeper, finalize-after-manifest- capture) so the dominant cost of a slow root reconcile can be attributed before fixing. Diagnostic only, no behavior change.
Add per-section wall-clock accumulation (childGraphPlanningTimings) to the root child-graph planning pass: resolveMappings, listSources (with List call / source-object counts), coverageWalk, ensureChildren, priorityReady, publish. Logged once per pass via defer so the hot priority-layer-pending early return is covered, at the same 150ms threshold as the caller total. Diagnosis-only: no behaviour, data-model, or status-contract change. Records the corrected latency classification in snapshot-creation-latency.md: H1 leaf staircase is absorbed by H3 (delayed leaf creation gated by repeated root re-plan), leaf-skip is a rejected no-op (planning runs only on the root Snapshot; demo snapshots are reconciled by the domain-controller), and H3 is the single remaining open issue with its internal cost distribution still to be attributed by this instrumentation.
Permanent always-apply rule capturing the evidence-first investigation methodology: the six pre-code questions and 15 principles (identify the real bottleneck, build timelines, correlate by stable identity, understand the execution model, separate observations from conclusions, falsifiable hypotheses, classify findings, one variable at a time, cause vs symptom, validate against acceptance criteria, document negative results, minimal explanations, correctness over optimization, continuous doc updates).
…st leg Co-authored-by: Cursor <cursoragent@cursor.com>
…instead of apiserver field selector
Co-authored-by: Cursor <cursoragent@cursor.com>
Ensure SnapshotContent tests reference an existing owning Snapshot so the ManifestsArchived Ready-gate can latch, latch residual volume capture in the content Ready-contract test, clarify the redeploy commit/push precondition, add the agent-must-not-push rule, and include dynamic-watch PoC test and unified-snapshot flow/import notes.
reconcileParentOwnedChildGraph listed CustomSnapshotDefinition via the uncached APIReader on every planning pass, producing repeated apiserver LISTs per snapshot tree. The CSD informer is already running, so route the list through the cached manager client. Mapping resolution is unchanged.
…ng view Record the hot-LIST attribution outcome: child-graph planning now lists CustomSnapshotDefinition through the manager cache instead of the uncached APIReader (~208 apiserver LISTs/tree removed), as a correctness-neutral load optimization. Add a Deferred note explaining why the registry-derived planning view was not implemented and the criteria for revisiting it. Also includes the previously written H5 pre-MCR sweep-race diagnosis. Co-authored-by: Cursor <cursoragent@cursor.com>
Concurrent reconciles of the same root Snapshot all passed the MCR-gate NotFound before any created the ManifestCaptureRequest, so each ran the full, identical pre-MCR namespace sweep and the losers' Creates landed on AlreadyExists. Gate the sweep with a non-blocking per-UID in-process lock: one reconcile plans and the others requeue briefly and take the frozen mcr-present branch once the MCR exists. Concurrency dedup only; the MCR-gate keeps temporal dedup and the plan result is not cached across time. Distinct Snapshots still plan in parallel.
Mark H5 closed: the per-Snapshot-UID single-flight is implemented and fan-out validation (SETS=10 x3, SETS=20) shows sweeps/root 2->1 and 3->1, redundant sweeps eliminated, and the concurrent MCR AlreadyExists race gone with Ready unchanged. Record the before/after table and update the checklist.
Add a Current validated state paragraph summarizing the two validated correctness-neutral load cleanups (CSD planning list cache, H5 pre-MCR sweep single-flight) and naming child-subtree API-cost as the next open scalability item, distinct from the stopped wall-clock work. Co-authored-by: Cursor <cursoragent@cursor.com>
…ndexed lookup Add SnapshotChildrenRefFieldIndex over Snapshot.status.childrenSnapshotRefs and use it for the reverse parent lookup in findParentsReferencingChildSnapshot, replacing the full-namespace APIReader SnapshotList issued on every child event. Child freshness read stays on APIReader; only relationship enumeration moves to the cache index. Namespace isolation comes from InNamespace on the namespace-less key, covered by a bidirectional test.
…tion Record the childrenSnapshotRefs field-index fix: primary relay reverse-lookup LIST eliminated (storage/snapshots ~440 -> ~2 LIST/tree, 0 field-label errors, 0 restarts, root Ready ~21s, no regression). Reframe the next open item from "find another APIReader" to "remaining repeated full-collection operations" (child-subtree enumerations), since the five closed load fixes share one pattern: replace search-by-full-traversal with direct addressing (index / direct-ref / cache).
Document Case B: childrenSnapshotRefs before ChildrenSnapshotReady=True/Completed is not a frozen membership set. It is a full recompute per pass and grows/shrinks across priority layers, and the first publish can be partial. The only valid freeze point is ChildrenSnapshotReady=True, Reason=Completed, ObservedGeneration==Generation, already exploited by childGraphReplanSkippable. Narrow the next open candidate to the freshness-gated readiness-cache question (prove staleness tolerance before implementing). Co-authored-by: Cursor <cursoragent@cursor.com>
Controller manager, capture dynamic client and domain-controller manager client now read QPS/Burst from env vars with unchanged defaults (50/100 and capture 100/200). Values are parsed once at startup with fail-fast on invalid input.
…pter Record Case B for the last large child-snapshot LIST candidate: the shared per-GVK list in childSnapshotReadCache feeds coverage/dedup and existence-before-create, not just readiness, so moving it to the informer cache makes create/dedup stale (stale-NotFound -> Create -> AlreadyExists, not ignored -> GraphPlanningFailed / root Ready flapping). Not correctness-neutral. Document the future Case A prerequisites and close the API-load/scalability chapter: all obvious hot LIST candidates are fixed or rejected; the remaining child-snapshot LIST path is freshness-bound and intentionally left on the APIReader.
Co-authored-by: Cursor <cursoragent@cursor.com>
…pagation Co-authored-by: Cursor <cursoragent@cursor.com>
…oll-paced Co-authored-by: Cursor <cursoragent@cursor.com>
Record the STOP decision (QPS was the only throughput lever that moved the wall; remainder is structural disk-layer serialization; domain sweep optional) and add a code-agnostic, grep-able carry-over section so the latency findings can be re-applied on a substantially reworked main.
AleksZimin
marked this pull request as draft
July 29, 2026 14:52
kneumoin
changed the title
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Raise genericbinder MaxConcurrentReconciles from 1 to 4 with a bounded rate limiter so a capture wave is not serialized through a single worker, and correct the stale comment about the SnapshotContent reverse-watch. Add the snapshot creation latency analysis, optimization and implementation-plan docs.
Description
Why do we need it, and what problem does it solve?
What is the expected result?
Checklist