✨ feat(controller,api): report a live collected resource count on KollectTarget (PERF-FIX-05) - #300
Merged
Merged
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…lectTarget PERF-FIX-05 / finding F-05. `KollectTarget` reported collection scale only as prose inside the Ready condition message, built from a count captured on the spec reconcile path. Objects entering or leaving the matched set never re-ran it, and nothing marked the number stale. On the Talos lab the status read `collecting 1000 resource(s)` throughout a window in which the inventory demonstrably moved 10 000 -> 10 200 and back within 20 s. - add `status.collectedCount` (`*int64`) as the machine-readable source of truth, plus `status.collectedCountUpdatedAt` recording when the number last changed. A null count means never measured; a measured zero is a real zero. A degraded or failing target keeps its last known count with an old timestamp instead of reporting a fresh-looking number, so staleness is explicit rather than implied. - add COLLECTED / UPDATED / AGE printer columns so `kubectl get kollecttargets` shows the count without `-o yaml`. - requeue a Ready target on `--target-count-resync` (default 60s, Helm `controller.targetCountResync`). Nothing else enqueues a Target when a watched object enters or leaves its set. Re-registration with unchanged state is free by design and the status write is skipped when the number did not move, so the steady-state cost is one cached read per Target per interval. - derive the Ready message from the stored count rather than restating it. The write path depends on it: `setTargetCondition` skips the API write for a byte-identical condition, so a count that never reached the message would never reach the API server. The status message is unchanged in shape and no existing field is removed or renamed — the CRD change is additive. Chart CRDs: `hack/helm-sync-crds.sh` also picks up pre-existing drift under `charts/kollect/crds/` that predates this lane. `kollectclusterinventories`, `kollectclusterscopes` and `kollectinventories` were behind `config/crd/bases`, and `kollecttargets` was missing `extractionFailures`/`lastExtractionError`. Nothing in CI gates that sync today — only `hack/release-assets.sh` calls it. Tests: an envtest drives a real collection engine and asserts the persisted count grows as ConfigMaps enter the selector, shrinks as they leave, reaches a genuine zero, and that each reconcile returns a positive RequeueAfter. Unit tests pin the persistence of a count-only change through the fake client, the unchanged-count timestamp stability, and the printer columns in both the kubebuilder and Helm chart CRD copies.
…message Independent review finding F1. Deriving the Ready message from the count made the message change whenever the number changed — but that is not the same as the number always reaching the API server. `setTargetCondition` skips the write for a byte-identical condition, so a target upgraded from a binary that already wrote `collecting 0 resource(s)` and genuinely collecting zero never persisted its count: the condition matched, the write was skipped, and `status.collectedCount` stayed absent for every subsequent resync. `kubectl get` then showed `COLLECTED <none>`, which the shipped CRD documentation defines as "never computed" — so a measured zero was indistinguishable from an unevaluated target, breaking the invariant `TestSetReadyPersistsShrinkingCollectedCount` asserts. `setTargetCondition` now reports whether it issued the status write, and `setReady` follows up with an explicit `Status().Update` when the count moved and the condition write was skipped. Persistence no longer depends on the condition text at all, and the extra call is only paid when it is the only way the number reaches the API server. The reviewer's probe is kept as a guard test: ten consecutive `setReady` calls on a zero-count target upgraded mid-flight must leave a non-nil, zero `status.collectedCount`. It fails without this change.
Independent review finding F2. The lane claimed the count resync was "free by design" and cost "one cached read per target per interval". Both were false. `RegisterTarget` refreshed the namespace cache unconditionally, and that refresh is a live, uncached, unpaginated cluster-wide namespace LIST against the real clientset — issued before the fingerprint gate, so the "unchanged state is free" property never applied to it. Making every Ready target reconcile on a timer turned that into a LIST per target per interval that did not exist before. Only the recompute branch actually reads the namespace cache, and both reconcilers supply `EffectiveNamespaces`, so the refresh now happens only when the engine has to derive the namespace set itself. `RegisterTargetOptions.EffectiveNamespaces` documents the contract that comes with it: supplying the set makes the caller responsible for cache freshness. `KollectTarget` already satisfies that — `resolveTargetFilterStatus` refreshes immediately before registering. `KollectClusterTarget` did not: it resolves namespaces from the cached client and relied on `RegisterTarget` to populate the snapshot that backs resource-rule evaluation and the namespace watch opt-in/opt-out in `ShouldCollect`. It now refreshes once per reconcile ahead of its registration loop, which also collapses the previous one-LIST-per-registered-namespace into one per reconcile. Remaining cost, stated accurately in `runtime_options.go`, `performance.md` and ADR-0603 instead of the previous claim: one cluster-wide namespace LIST per target per resync, two when a KollectScope is enforced. A regression test counts namespace LISTs through a fake clientset reactor: zero when `EffectiveNamespaces` is supplied (including on a re-registration, the resync shape), one on the recompute branch. It fails with the unconditional refresh restored.
…s churning Independent review findings F3 and F6. (F4, the missing `--target-count-resync` entry in ADR-0603, landed in the preceding commit alongside the corrected cost statement.) - The printer-column contract test only checked `Collected` and `Updated`. Declaring any `additionalPrinterColumns` suppresses the apiserver's default AGE column, so a regeneration that dropped the explicit `Age` entry would silently remove AGE from `kubectl get kollecttargets` with every test still green. `Age` is now asserted too, in both the kubebuilder and Helm chart CRD copies. - `setTargetCondition` treats a changed message as a transition, and the Ready message restates the live count — so `Ready.lastTransitionTime` now churns whenever the count moves, as often as the resync interval, while Ready never leaves True. The original F-05 report used "lastTransitionTime hours old" as evidence of staleness; that signal no longer means what it did. The conditions reference now says so in both directions: do not alert on it as flapping, and do not read an old value as a stall.
…pends on Follow-up to the F2 gate. Skipping the namespace LIST inside RegisterTarget when the caller supplies EffectiveNamespaces makes the KollectTarget path depend on an ordering invariant nothing asserted: `resolveTargetFilterStatus` must run before RegisterTarget, and must not be skipped when the filter status looks unchanged, because it is the only thing refreshing the engine's namespace cache on that path. That cache backs resource-rule evaluation and the namespace watch opt-in/opt-out in ShouldCollect, so losing the refresh drops objects silently rather than failing. The contract itself lives in `collect.RegisterTargetOptions`, where a controller-side editor would not see it — so state it at the call site too. The test asserts the resolved namespace set, not the cache. Asserting only that the cache ends up populated proves nothing: the engine's recompute branch refreshes it as a fallback precisely when the effective set arrives empty, so that version passed with the refresh deleted — confirmed by mutation, and the reason for the comment on the test. Asserting that a target in a freshly created namespace resolves its OWN namespace into `status.effectiveNamespaces` does discriminate, and fails when the refresh is removed. That fallback is also worth recording: the ordering dependency degrades rather than corrupts, because an empty effective set sends RegisterTarget down the recompute branch, which refreshes. Suspended targets never populated the cache before this change either — RegisterTarget's Suspend early-return has always preceded the refresh — so that path is unchanged.
Independent review finding F-E, plus the red `codecov/patch` that pointed at the same code. `RefreshNamespaces` in `syncEngineTargets` is the compensating control this lane's F2 gate made necessary — RegisterTarget no longer refreshes when the caller supplies EffectiveNamespaces, and this controller always does. Deleting the call left the whole suite green: the line was executed, never asserted. The envtest asserts the observable outcome instead of the cache. A namespace carrying `kollect.dev/namespace-watch: disabled` sits inside the cluster target's selector, so it IS registered — asserted explicitly, so a zero count cannot pass for "never in scope" — and a ConfigMap in a sibling namespace is collected as a positive control that the pipeline drained. Only the annotation, which reaches the engine exclusively through the namespace cache, keeps the opted-out namespace's object out. MUTATION-VERIFIED: with the refresh removed the opted-out namespace collects 1 instead of 0, which is the exact failure mode — an operator's explicit opt-out silently ignored, over-collection, no error anywhere. The "cache is populated" shape was deliberately avoided; it is the tautology this lane already hit once, because an empty effective set falls through to the recompute branch which refreshes as a fallback. Two error paths that `codecov/patch` flagged are covered on merit, both mutation-verified to fail when the error is swallowed: - the forced count write in `setReady` must surface its failure as a reconcile error and retry, or the number is dropped silently — the class of defect this lane exists to fix; - a failed namespace refresh must fail the cluster-target sync rather than register against a stale cache, since registering anyway reproduces the F-E failure mode. Patch coverage goes from 6 uncovered statements to 2. The remaining two are the `refreshNamespaceCache` error inside RegisterTarget's recompute branch, which is logged rather than returned — pre-existing behaviour this lane only relocated, so it now counts as patch lines. Left uncovered deliberately: asserting a log line would be coverage for its own sake, and changing it to return would alter registration behaviour on transient list failures, which is out of scope here. Also reconciles the third cost statement: ADR-0603 and performance.md now both carry the "two under an enforced KollectScope" qualifier that runtime_options.go already had.
konih
force-pushed
the
feat/perf-fix-05-live-collected-count
branch
from
August 16, 2026 08:18
e3e7ca8 to
38f8dde
Compare
SonarCloud flagged the only new issue on this lane: cognitive complexity 18 against a limit of 15 in TestKollectTargetPrinterColumns (go:S3776). The quality gate passed anyway, but the nesting was mine and worth removing — reading the manifest, unmarshalling it and folding every version's columns into one map all sat inside the subtest closure alongside the assertions. Parsing moves to a printerColumns helper, leaving the test body as the want/got comparison it is meant to be. Behaviour is unchanged, and the guard still discriminates: dropping the Age column from the chart CRD copy fails the test with `printer column "Age" = "", want ".metadata.creationTimestamp"` — verified by mutation.
|
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.
PERF-FIX-05
setReadybuiltcollecting %d resource(s)from a count passed on the spec reconcile path, so objects entering or leaving the matched set never refreshed it.Observed on the Talos lab: status read
collecting 1000 resource(s)while it was demonstrably collecting 1200 — inventory moved 10,000 → 10,200 and back within 20s, the message never changed,observedGenerationstuck at 2,lastTransitionTimehours old. Nothing marked the number stale.What changed
status.collectedCount(*int64) +status.collectedCountUpdatedAt— machine-readable, additive, no field removed or renamed.kubectl get kollecttargetshows it without-o yaml.Ageis re-added explicitly becauseadditionalPrinterColumnssuppresses the apiserver default — and that is now pinned by a contract test.--target-count-resync(default 60s), exposed ascontroller.targetCountResync.Two P1s from independent review, both fixed and mutation-verified
A measured zero never persisted.
setTargetConditionearly-returns on a byte-identical condition, so when the stored message already readcollecting 0 resource(s)the field stayednilforever — indistinguishable from "never computed". Persistence is now independent of the condition text. Instrumented write counts: 0 on a steady resync, 1 when the count moves, 1 total across 5 resyncs on the upgrade edge. No write amplification.The cost claim was false. "One cached read per target per interval" was wrong — it was 2 live, uncached, cluster-wide namespace LISTs (3 under a
KollectScope). Rather than reword it, the refresh moved onto the recompute branch only. Net effect is a reduction: KollectTarget 2 → 1 LIST (3 → 2 scoped), and KollectClusterTarget N LISTs (one per namespace) → 1 per reconcile.A naive gate would have broken
KollectClusterTarget, which relied on the unconditional refresh to populate the snapshot backingResourceMatchesRulesand the namespacekollect.dev/watchopt-out — silently disabling it.RefreshNamespacesis now called once per reconcile ahead of the registration loop, and the reviewer independently confirmed both the hole and the compensation.Verification
task lint,task verify,task test:run,task coverage(90.6%, floor 90),task helm-test,task vulncheck(exit 0) — all green. Envtest drives a real collect engine and asserts the persisted count 2 → 5 → 2 → 0; red-confirmed before the fix on both assertions independently.