fix: don't fabricate time_to_ready_ms when rediscovering an already-ready container#500
Merged
Merged
Conversation
…eady container trackStartupLifecycle created a fresh tracker entry whenever it observed a (podUID, containerName, restartCount) combination with no live entry, using the pod's original PodScheduled condition timestamp as pendingAt. If the container was ALREADY Running and Ready in that same observation (no genuine Pending->ContainerCreating->Running->Ready transition watched), the Ready branch computed time_to_ready_ms = now.Sub(pendingAt) and emitted immediately, then deleted the entry -- guaranteeing the next unrelated status update for the same stable pod would repeat the exact cycle, each time computing an even larger, wrong duration. This fires for any long-lived, already-Ready pod every time zxporter (re)starts, or a previously emitted entry gets rediscovered on a later, unrelated status update (e.g. a routine kubelet condition/heartbeat refresh) -- not just on collector restart. Confirmed against live preprod ClickHouse data: 10,953 of the affected rows (100% of the implausible time_to_ready_ms values, up to 58+ hours) all had container_creating_at IS NULL, i.e. never observed the ContainerCreating transition -- exactly the signature of this bug. See devzero-inc/services#8891 for full investigation. Fix: when creating a new tracker entry, skip it entirely if the container is already Running and Ready at first observation. That container's genuine startup was either already captured by snapshotStartupLifecycles (which correctly uses real historical condition timestamps, not now()) during the initial informer sync, or was already correctly reported by an earlier real transition and this is a stale rediscovery with no reliable data to report. Added pod_collector_test.go (new file for this package) with a failing test that reproduces the bug against the pre-fix code (confirmed: produced time_to_ready_ms=172800000 for a pod scheduled 48h earlier) and a control test proving genuine fresh Pending->ContainerCreating-> Running->Ready transitions still correctly emit a small, real duration.
Code Review ✅ ApprovedPrevents the fabrication of implausible Was this helpful? React with 👍 / 👎 | Gitar |
Tzvonimir
approved these changes
Jul 11, 2026
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
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.
Why
Fixes the root cause investigated in devzero-inc/services#8891:
container_startup_lifecyclesrows in ClickHouse showed implausibly largetime_to_ready_msvalues (up to 58+ hours) for long-lived, stable pods, whiletime_to_running_msfor the same containers stayed correct. This corruptedavgTimeToReadyMsaggregates consumed by both the existingContainerHealthService/GetWorkloadHealthSummariesfrontend RPC and the new startup-timing fields streamed to the tunrWorkloadProfileCRD (devzero-inc/services#8886).Root cause
trackStartupLifecyclecreates a fresh tracker entry whenever it observes a(podUID, containerName, restartCount)key with no live entry, seedingpendingAtfrom the pod'sPodScheduledcondition — which can be days old for a long-lived pod. If the container is alreadyRunningandReadyin that same observation (no genuine watched transition throughPending -> ContainerCreating -> Running -> Ready), the code computedtime_to_ready_ms = now.Sub(pendingAt)and emitted immediately, then deleted the entry. This guarantees the next unrelated status update for the same stable pod (a routine kubelet condition/heartbeat refresh, not necessarily a collector restart) repeats the exact cycle — recreating the entry, immediately matching Ready again, and computing an even larger, wrong duration each time.Confirmed against live preprod ClickHouse data: of rows in the last 7 days, 10,953 had an implausible
time_to_ready_ms(>10 min) — 100% of them also hadcontainer_creating_at IS NULL(never observed theContainerCreatingtransition), and zero rows withcontainer_creating_atpopulated showed the anomaly. That's the exact signature of this bug.Fix
When creating a new tracker entry, skip it entirely if the container is already
RunningandReadyat first observation — there is no reliable data available to compute a real duration in that case. That container's genuine startup was either already captured correctly bysnapshotStartupLifecycles(which uses real historical condition timestamps, notnow(), specifically for containers observed as already-running during the initial informer cache sync), or was already correctly reported by an earlier real transition and this is a stale rediscovery.Test plan
TestTrackStartupLifecycle_DoesNotFabricateReadyDurationForStaleRediscoveryreproduces the bug against the pre-fix code (confirmed: producedtime_to_ready_ms=172800000— exactly 48h — for a pod scheduled 48h earlier) and passes against the fix (no event emitted, no tracker entry left behind).TestTrackStartupLifecycle_EmitsCorrectDurationForGenuineFreshStartupproves a genuinePending -> ContainerCreating -> Running -> Readytransition (the normal, working path) still correctly emits a small, real duration — this fix doesn't affect legitimate fresh startups.go build ./...,go vet ./internal/collector/...,gofmt -lall clean.go test ./internal/...passes (full suite, excludingtest/e2ewhich requires a live kind cluster + cert-manager/prometheus-operator not available in this environment — pre-existing, unrelated to this change).golangci-lintnot run locally (repeatedly OOM-killed in the sandbox this was developed in, even scoped to a single package) — please confirm it passes in CI.