Repository navigation
Conversation
anant10
force-pushed
the
feat/owned-series-tracking
branch
from
August 11, 2026 17:28
97d8157 to
305bc32
Compare
|
|
||
| // If ring tokens are loaded, check ownership before creating. | ||
| // This prevents tracking series we don't own (e.g., stale distributor routes). | ||
| if len(ringTokens) > 0 && !isOwnedByInstance(key, ringTokens, instanceTokens) { |
Contributor
There was a problem hiding this comment.
we should maintain active behaviour the same. we should make the check impact on the owned metric which is the new one
Author
There was a problem hiding this comment.
Addressed this issue in the next revision
| // This test just validates ActiveSeries itself works correctly. | ||
| } | ||
|
|
||
| func BenchmarkActiveSeries_UpdateSeries(b *testing.B) { |
Contributor
There was a problem hiding this comment.
Did we replace tests removing benchmark to add new test?
We should still have benchmark and would be nice to run to see how it perform with owned active
Author
There was a problem hiding this comment.
Yes fixing it in the next revision. Removed it by mistake
| // if the responsible token belongs to this instance. | ||
| func isOwnedByInstance(key uint32, ringTokens []uint32, instanceTokens map[uint32]struct{}) bool { | ||
| i := ring.SearchToken(ringTokens, key) | ||
| _, found := instanceTokens[ringTokens[i]] |
Contributor
There was a problem hiding this comment.
lets add a check here to make sure ringTokens is not empty otherwise it will crash.
Author
There was a problem hiding this comment.
Added. SearchToken wraps to 0 on an empty slice, so the index panics. it wasn't reachable since all three call sites guarded with len(ringTokens) > 0, but the check belonged in the function.
It now returns true when the token list is empty or the ownership data doesn't match it, so unknown ownership counts as owned rather than under-counting against the limit.
| * [CHANGE] Cache: Setting `-blocks-storage.bucket-store.metadata-cache.bucket-index-content-ttl` to 0 will disable the bucket-index cache. #7446 | ||
| * [CHANGE] HA Tracker: Move `-distributor.ha-tracker.failover-timeout` from a global config to a per-tenant runtime config. The flag name and default value (30s) remain the same. #7481 | ||
| * [FEATURE] Ingester: Add owned series tracking to prevent false customer throttling during ingester scale-up and ring resharding. When enabled, the ingester tracks which series it currently owns according to the ring and uses that count (instead of total in-memory series) for limit enforcement. Eliminates a up-to-2-hour window of incorrect throttling after any ring change. Controlled by `-ingester.owned-series-metrics-enabled` (metric emission) and `-ingester.owned-series-limit-enforcement-enabled` (limit enforcement). #7509 | ||
| * [ENHANCEMENT] Ring: Consolidate sharding functions (`TokenForLabels`, `ShardByMetricName`, etc.) into `pkg/ring/token.go` for reuse by both distributor and ingester. Export `SearchToken`. #7509 |
Contributor
There was a problem hiding this comment.
We dont need this entries. Just one for feature is enough
anant10
force-pushed
the
feat/owned-series-tracking
branch
from
October 7, 2026 01:23
35b9338 to
640b980
Compare
… during ring changes When ingesters scale up, the per-ingester local series limit drops immediately but stale series data remains in TSDB head for up to 2 hours. This causes PreCreation() to incorrectly reject new writes. This PR introduces owned series tracking in ActiveSeries: - Each series stores its ring token (computed via TokenForLabels) - Ownership is evaluated against current ring state on each update cycle - When ring changes, unowned series are excluded from limit enforcement Two feature flags for safe rollout: - owned_series_metrics_enabled: enables cortex_ingester_owned_series metric - owned_series_limit_enforcement_enabled: switches PreCreation to use owned count for both per-user and instance-level max_series limits Key design decisions: - Zone-local ownership check via SearchToken + instance token map lookup - Ring state stored behind atomic.Pointer[ringState] for lock-free reads on the hot push path - instanceOwnedCount recalculated every ~1 min (not incremental) to avoid drift from edge cases - Startup fallback: when instanceOwnedCount==0, uses instanceSeriesCount Code consolidation: - FNV hash functions consolidated into pkg/util/fnv.go (single source) - Sharding functions moved to pkg/ring/token.go (eliminates duplication between distributor and ring packages) Production validation: tested with 5M active series, scale-up 9->18 ingesters showed owned_series=984K vs memory_series=1.8M (813K stale series correctly excluded). Zero throttle errors. Fixes cortexproject#7509 Signed-off-by: Anant Shanbhag <anantvas@amazon.com>
Ring.Get determined the replica set for a key by inlining the ring walk directly in its body. The walk is the single definition of "which instances hold this data", and the owned-series work needs to ask that same question from a second place: for every token position, rather than for one key. Duplicating the walk there would let the two copies disagree, and a disagreement is exactly the bug that matters, because an ingester would then count series against its limit that the distributor does not route to it. Move the walk into ringTopology.replicaSetAt, keyed on a token position rather than a key, and have Ring.Get call it after resolving the key to a position with SearchToken. ringTopology is a read-only view that copies only slice and map headers, so it can be built per call. replicaSetAt also returns the instance IDs alongside the descriptors. InstanceDesc carries no ID of its own and an instance's ID is not interchangeable with its Addr, so a caller asking "am I in this set?" can only answer that correctly by comparing IDs. The per-zone cap is now skipped when the ring reports no zones at all, rather than dividing by the zone count unconditionally. Ring.Get cannot reach that state for a non-empty ring, so this is a robustness change for the new caller, not a behaviour change here. No behaviour change. The existing pkg/ring suite passes unmodified, and BenchmarkRing_Get is unchanged at ~607 ns/op and 0 allocs/op. Signed-off-by: Anant Shanbhag <anantvas@amazon.com>
Add OwnedTokenPositions, which returns the ring's sorted token list plus a parallel bitmap of the positions owned by a given instance. Ownership is computed with replicaSetAt, the same walk Ring.Get uses, so an instance's view of what it owns cannot drift from where the ring routes. This replaces the "am I the first instance in this token range, among the token list for my own zone?" check the owned-series tracking previously used. That check has exactly one owner per token range, which is only the same thing as the replica set in one configuration: zone awareness enabled with a replication factor equal to the zone count. In every other shape, including the default of zone awareness disabled, it undercounts by roughly the replication factor. Ownership deliberately does not apply the replication strategy's health filter. It has to stay stable across heartbeat flapping, because a series does not stop being this instance's responsibility when a peer misses a heartbeat. The lookup is O(tokens x replicationFactor) and is intended to be called once per ring change, so that the per-series question becomes owned[SearchToken(tokens, key)]: a binary search plus an array index. BenchmarkOwnedTokenPositions covers that precompute, at ~29.7 ms for 100 instances holding 512 tokens each. Tests assert the load-bearing property directly: across ten ring shapes covering zone awareness on and off and replication factors above, below, equal to and not divisible by the zone count, an instance owns a token position if and only if Ring.Get for a key in that position includes it. A conservation test asserts that every position is owned by exactly replicationFactor instances, which is what makes the per-ingester counts sum correctly against a limit that is itself scaled by the replication factor. One test builds a ring whose instance IDs and addresses deliberately differ. The shared fixtures set Addr equal to the instance ID, so an implementation comparing addresses would pass every other test here and then own nothing at all in production. Signed-off-by: Anant Shanbhag <anantvas@amazon.com>
Replace the ownership check used by owned-series tracking. It previously asked "is the first token at or after my series' token, within the token list for my own zone, one of my tokens?". That has exactly one owner per token range, which only coincides with the ring's replica set when zone awareness is enabled and the replication factor equals the zone count. In every other configuration, including the default of zone awareness disabled, it is wrong. With zone awareness off, every instance falls into one unnamed zone, the "zone-filtered" token list becomes the whole ring, and the check degenerates to "am I the single global primary?". That counts roughly 1/replicationFactor of the series the instance actually holds, while the limit it is compared against is scaled up by the replication factor. The net effect is a limit that is too permissive by about the replication factor, so an ingester runs out of memory instead of throttling. That is a worse failure than the stale-count problem this feature exists to fix. Ownership now comes from ring.OwnedTokenPositions, which walks the ring with the same function Ring.Get uses, so the set of series an ingester counts is by construction the set the distributor routes to it. Because the walk honours the operation's replica-set extension rules, instances in JOINING, LEAVING and READONLY are handled exactly as the write path handles them, rather than via a separate READONLY special case. The lifecycler computes the bitmap in updateCounters, alongside the healthy-instance count that the limiter already uses as the denominator of the same comparison. Both therefore come from one ring snapshot taken at one instant under one lock, so numerator and denominator cannot be drawn from different views of the ring. The walk is too expensive to repeat on every heartbeat, and heartbeats are by far the most common reason updateCounters runs, so it is gated on a fingerprint covering instances, zones, states and tokens. Desc.RingCompare cannot serve here: it reports a state change and a timestamp change as the same result, and ownership depends on state. The recompute deliberately runs outside countersLock, since HealthyInstancesCount is on the push path and must not block behind it. Per-series cost drops from a map lookup to a binary search plus an array index, and isOwned no longer indexes the token slice without checking its length, which panicked on an empty ring. Tested with the upstream CI invocation, which matters here: go test -tags "netgo slicelabels" -race. Without the slicelabels tag pkg/ingester panics in cortexpb.CopyLabels, and without -race the //go:build !race file is compiled in and panics too; both reproduce on unmodified upstream. Signed-off-by: Anant Shanbhag <anantvas@amazon.com>
Owned-series tracking previously refused to track a series the ring did not assign to this ingester, and deleted entries when a ring change took ownership away. That made cortex_ingester_active_series silently drop those series, changing the meaning of a long-standing metric, and it made owned and active identical by construction, which left the staged rollout across the two feature flags unable to show any difference. Ownership now gates exactly one counter. Every series is tracked and counts towards active exactly as before; only owned is conditional. Losing ownership moves a series out of owned and leaves it active, which is accurate, because the series is still held in this ingester's head. Retention is now two-tier. The periodic cycle recounts but removes nothing, and entries are released by a purge after head compaction, keyed on the head's new minimum time. An entry therefore lives for as long as the series it describes is in memory. This is what makes the feature work for a tenant with high churn. Tying owned to the idle timeout meant a tenant with a 3M-series head but only 500K recently active series reported owned as 500K, so a 3M limit would admit 2.5M more series on top of the 3M already resident: more permissive than the unmodified code it replaces. Anchoring to the head instead makes owned a measure of what the ingester is actually storing, which is what a series limit exists to bound. The consequence is that owned may exceed active for such a tenant, which is intended and is the reason the two counts are maintained separately rather than one being derived from the other. Totals are cached per tenant and maintained as series are created, because PreCreation consults them for every new series and previously took a read lock on all 512 stripes to do so. The periodic cycle recomputes them authoritatively, so any drift is corrected within one cycle. Limit enforcement follows. The per-tenant check no longer needs its "owned > 0" guard: the cached total is accurate from the first sample rather than only after the first periodic cycle, so there is no startup window to paper over, and the guard had a cliff where a genuinely empty tenant silently fell back to the head count. The instance-level check keeps a fallback, because the instance total really is unavailable until the first cycle runs, but it is now keyed on an explicit negative sentinel rather than on zero, so a true zero is no longer confused with "not yet computed". Measured on 100k series: the periodic recount is 1.67ms with no allocations when the ring is unchanged and 2.42ms when every entry's ownership is re-evaluated. The push path is unchanged within noise, ~440ns/op with the ring loaded against ~400-457ns/op without it. Signed-off-by: Anant Shanbhag <anantvas@amazon.com>
The owned-series work replaced this file wholesale, removing 139 lines:
three tests covering the active count and purge behaviour, and four
benchmarks covering the push and purge paths. Removing the benchmarks meant
there was no longer a baseline to compare the new per-series ownership cost
against, which is what the review asked about.
All of them are restored, with the ring token argument added. They pass a
token of zero with no ring loaded, so ownership is unknown and every series
counts as owned, which means they exercise precisely the behaviour that
predates this feature.
Restoring them surfaced that BenchmarkActiveSeries_UpdateSeries does not
run at all. It calls b.Loop() in two separate loops, which is not allowed,
and sizes its series slice from b.N before b.Loop() has established an
iteration count. It panics with "index out of range [1] with length 1", or
fails with "B.Loop called with timer stopped" when an explicit -benchtime is
given. This reproduces on unmodified upstream at this branch's merge base,
and the same code is present verbatim on master, so it has been broken since
the b.Loop() migration rather than by this change. It is converted to the
b.N form, which is the correct shape when the iteration count is needed
before the timed loop begins.
New coverage for the owned-series semantics:
- active is unaffected by ownership. The same series are pushed into two
trackers, one with a ring where half the tokens belong elsewhere and one
with no ring, and the active counts must agree exactly. This is the
regression test for the metric's meaning being preserved.
- owned exceeds active for a tenant whose series have gone idle but are
still held in the head, which is the behaviour the limit fix depends on.
- the periodic cycle retains expired entries, repeated cycles are stable
rather than progressively dropping entries, and only a purge releases
them.
- losing and regaining ownership moves series between owned and not-owned
without touching active and without deleting anything, so ownership can
return without the series being re-pushed.
- the cached totals match the per-stripe counters after creation, after a
periodic cycle, after a purge and after a clear, since those totals are
what limits are enforced against.
- ownership change detection fires when the bitmap changes even though the
token list does not, which is the case a token-list hash cannot see.
- isOwned handles an empty ring and a mismatched bitmap without panicking,
which the previous ownership check did not.
Two benchmarks are added for the new paths: the push path with and without
a ring loaded, to show the per-series ownership cost, and the periodic
recount separating the common unchanged-ring case from a full ownership
re-evaluation.
Signed-off-by: Anant Shanbhag <anantvas@amazon.com>
Ownership is computed from the raw replica-set walk and deliberately does not apply the replication strategy's health filter. That makes ownership differ from routing while an instance is transitioning, and until now that difference was only described in a comment rather than pinned by a test. For each of JOINING, LEAVING and READONLY, assert that the transitioning instance still owns its token ranges, that those ranges end up with more owners than the replication factor because the replica set is extended, and that writes still land on exactly replicationFactor healthy instances and never on the transitioning one. Both the transitioning instance and the instance the walk extends to really are holding that data, so both counting it is correct rather than double counting: the departing instance still has the series in its head, and the distributor really is writing to the extension. Filtering for health here would instead make an instance's own series count depend on whether its peers happened to miss a heartbeat, which is why the asymmetry with Ring.Get is intentional. Note that the aggregate invariant asserted by TestOwnedTokenPositions_Conservation, that every position has exactly replicationFactor owners, holds only for a ring where every instance is ACTIVE. This test covers the complementary case. Signed-off-by: Anant Shanbhag <anantvas@amazon.com>
The CHANGELOG carried three entries for this change. The two ENHANCEMENT lines described internal refactoring, moving the sharding helpers into pkg/ring/token.go and the FNV helpers into pkg/util/fnv.go, which is not something an operator reading release notes needs. Collapsed to the single FEATURE entry, reworded to describe the effect rather than the mechanism, marked experimental to match how the flags are listed in v1-guarantees, and with the "a up-to-2-hour" typo removed. It now says "up to one head compaction cycle", which is what the window actually is rather than assuming the default block range. The two new flags were never added to the generated configuration documentation, so `make check-doc` fails: it regenerates the docs and asserts `git diff --exit-code` over the config reference, the blocks-storage pages and the JSON config schema. Regenerated, which adds the twelve expected lines to docs/configuration/config-file-reference.md and the matching entries to schemas/cortex-config-schema.json. No other generated file changes. Also listed the feature under experimental features in docs/configuration/v1-guarantees.md, alongside the other opt-in ingester metrics, so the stability expectation is explicit while it is behind flags. Signed-off-by: Anant Shanbhag <anantvas@amazon.com>
Head-anchored retention compares two different clocks. An entry's timestamp is the wall-clock time a sample for it arrived, while Head().MinTime() is a sample timestamp. They agree for real-time ingestion, which is why this works in the common case, but they diverge for a tenant whose samples are backdated or whose clock is skewed. Only one of the two directions is dangerous. A cutoff that lands too early retains entries for longer than necessary, which over-counts owned series and errs towards enforcing limits sooner: wasteful, not harmful. A cutoff that lands too late drops entries for series which are still resident, which under-counts owned series and lets a tenant exceed its limit. A tenant whose sample timestamps run ahead of real time would hit exactly that, and in the extreme every entry would be released on each compaction, since no arrival time can be in the future. Clamp the cutoff so it is never more recent than one block range ago. Anything still in the head must have arrived within roughly that window, so this cannot release an entry for a resident series however far ahead a tenant's timestamps run. Backdated timestamps are left alone, since they already err in the safe direction. This is a bound rather than a translation between the clocks. Making the comparison exact would mean recording each series' last sample timestamp on its entry, which costs memory per series on a structure whose retention cost is already the open question here, so it is deliberately left for the design discussion on the pull request rather than decided unilaterally. The cutoff is computed by a free function so the behaviour can be asserted without standing up a TSDB. Tests cover each direction of skew and state the property the clamp exists for: a series which arrived within the last block range is never released, whatever the head reports. Signed-off-by: Anant Shanbhag <anantvas@amazon.com>
Retaining entries past the idle window introduced one way for the active count to behave differently from before, which I had not handled. Previously a series going idle had its entry removed, so a later sample created a fresh entry and the active count reflected the series at once. Now the entry survives and merely stops being counted, so a later sample updates it in place and creates nothing. The cached total is only bumped on creation, so a returning series went unnoticed until the next periodic recount, under-reporting active for up to one update period, a minute by default. The exported cortex_ingester_active_series gauge was not affected, because it is set inside the periodic loop immediately after the recount. The two other readers were: the native histogram limit consulted by PreCreation on every new series, and the active series figure in the per-tenant stats endpoint. Both would have read low, making that limit marginally too permissive. UpdateMetrics now publishes the idle cutoff it used, and the push path uses it to notice an entry crossing back into the window. The compare-and-swap that moves the timestamp across the cutoff decides which caller counts the series, so concurrent samples for the same returning series count it once. Whether the series is a native histogram is taken from the entry rather than from the incoming sample, since an entry's kind is fixed when it is created. Only the active counts change on reactivation. An idle series never stopped being owned, because owned deliberately ignores the window. The cutoff is published as zero until a cycle has run with one, so when entries are released by the idle timeout rather than retained, the reactivation path never engages and the original behaviour of creating a fresh entry is preserved exactly. Tests assert active reflects the returning series before any recount, that a second sample inside the window does not count it twice, that the subsequent authoritative recount agrees with what the push path did, and that the cached totals still match the per-stripe counters. Verified to fail without the change. A concurrency test pushes the same returning series from 64 goroutines and asserts it is counted once. The push path is unchanged within noise at ~470ns/op. Signed-off-by: Anant Shanbhag <anantvas@amazon.com>
Three behaviours were only visible by reading the implementation. Enabling owned series tracking is documented as turning on a metric, but it also changes how long active series entries are retained: they are released at head compaction rather than after the idle timeout, so that the owned count reflects what is held in memory rather than only what is recently active. That increases ingester memory roughly in proportion to the ratio between series in the TSDB head and recently active series. An operator enabling what reads like an observability flag should not have to discover that from a heap profile. The owned count may exceed the active count for a tenant with high churn, because it includes idle series that are still held. This is intended, and cortex_ingester_active_series keeps its existing value, but anything asserting that owned is a subset of active will be surprised. Limit enforcement applies to the instance-wide max_series limit as well as to per-tenant limits, which a single flag name does not convey. The consequence worth stating is that max_series then stops counting series reassigned to another ingester which are still resident until the next head compaction, so it no longer reflects everything the ingester is holding. No behaviour change. Flag help text, the regenerated configuration reference and schema, and the experimental features list. Signed-off-by: Anant Shanbhag <anantvas@amazon.com>
cortex_ingester_owned_series carries a user label but was absent from both places that clean up per-tenant metrics: deletePerUserMetrics, and the TSDB close path. Its three sibling per-tenant gauges are removed in both. A stale gauge therefore remained for every tenant whose TSDB was ever closed, by idle cleanup or deletion. On an ingester that sees tenants come and go the label cardinality of its own /metrics grows without bound, and dashboards keep reporting owned series for tenants that no longer exist. Signed-off-by: Anant Shanbhag <anantvas@amazon.com>
numZones was derived as len(getTokensByZone()), which merge-sorts every token in the ring into one slice per zone just to take the length of the resulting map. Counting distinct Zone values over the instances is proportional to the number of instances rather than the number of tokens, and allocates nothing. For 100 instances holding 512 tokens each this takes the once-per-ring-change precompute from 29.7ms to 26.4ms and from 715 allocations to 489. It also removes one of the two calls that sort each instance's token slice in place on the shared Desc as a side effect. Signed-off-by: Anant Shanbhag <anantvas@amazon.com>
…aching The previous revision cached one cross-stripe total per counter and had the push path increment it, while the periodic recount summed the stripes and stored the result. That store is not atomic with respect to the scan it was computed from, so a series created between a stripe being scanned and the total being stored was lost, and one created just before its stripe was scanned could be counted twice. The next complete recount repaired it, so the drift was transient, but the counts were only eventually correct rather than correct. It also diverged from the behaviour it replaced even with both feature flags off. Previously Active and ActiveNativeHistogram summed the stripes under a read lock and were exact at all times; with the cache they could drift during each purge pass, which is a change nobody opting out of this feature asked for. Make the per-stripe counters atomics and total them in the accessors. Every mutation still happens while holding that stripe's lock, so nothing can be lost, and readers no longer need to acquire 512 locks: totalling 512 atomics costs about 290ns with no allocations. That is cheaper than the read-locked version it replaces as well as being exact, so the cache was solving a problem that did not need solving at the cost of one that did. The cross-stripe cache, and the plumbing that reported back from updateSeriesTimestamp purely to maintain it, are gone. Reactivation still has to be counted explicitly, because an entry crossing back into the active window is not a creation, but it now increments the stripe counter under the stripe lock like every other mutation. A test asserts the counts stay exact while a recount runs concurrently with sixteen writers. It does not reproduce the transient drift of the cached design, since that repairs itself before any assertion can observe it; it pins the steady-state invariant. Signed-off-by: Anant Shanbhag <anantvas@amazon.com>
purge was given one cutoff and used it both to decide what to delete and to
decide what counts as active. That is correct for its original caller, which
passes the idle timeout, but owned-series tracking added a second caller that
passes a cutoff derived from the head's minimum time, which can be hours old.
Every retained entry then counted as active, so after each head compaction the
active count jumped to the owned count until the next periodic recount.
Two readers see that window, and neither waits for the recount:
- PreCreation consults ActiveNativeHistogram on every new series, so a tenant
with max_native_histogram_series_per_user set would get spurious
limit-exceeded errors for up to a minute after each head compaction.
- The per-tenant stats endpoint reports the inflated active count, which also
contradicts the documentation added in this revision stating that the active
series value is unchanged.
Take the two cutoffs separately: deleteBefore decides what is still held,
activeCutoff decides what is inside the idle window. The original caller passes
the idle timeout for both, which is exactly its previous behaviour, so nothing
changes with the feature disabled.
The regression test fails if the cutoffs are conflated again.
Signed-off-by: Anant Shanbhag <anantvas@amazon.com>
Active series entries are only created when a sample arrives: UpdateSeries is
called from the push path and nowhere else, and nothing seeds the tracker from a
restored head. An ingester that has just replayed its WAL therefore has a full
head and an empty tracker, so Owned() reads zero.
The previous revision removed the "owned > 0" guard that had been covering this,
on the reasoning that the count is maintained as series are created and so is
accurate from the first sample. That is true for series created by a push and
false for every series restored from the WAL. With the guard gone, a tenant could
create a full limit's worth of series on top of everything already resident,
roughly doubling the intended memory, until enough of the restored series
happened to receive a sample. Unmodified Cortex is not exposed to this, because
it compares the limit against Head().NumSeries(), which is correct the moment
replay finishes.
The instance-wide max_series limit had the same hole, and the negative sentinel
added in this revision did not close it: one cycle after a restart the periodic
loop stored a genuine zero, so the OOM guard passed everything.
Track the number of entries held, and only use the owned count when it covers
what is in memory:
- per tenant, use Owned() only when Tracked() is at least Head().NumSeries()
- instance wide, publish the sentinel rather than a total whenever the trackers
between them hold fewer entries than there are series in memory
Falling back to the in-memory series count over-counts, which delays a scale-up
from taking effect but cannot let a tenant past its limit. Guarding on coverage
rather than on the count being zero also removes the cliff the old guard had,
where a genuinely empty tenant was indistinguishable from an unknown one.
Tracked is a useful number in its own right: it is the size of the structure
whose retention this feature extends, which was otherwise not observable.
Signed-off-by: Anant Shanbhag <anantvas@amazon.com>
Head-anchored retention decided which active series entries to release at head compaction by comparing each entry's timestamp against a cutoff derived from the head's minimum time. Those are two different clocks: an entry's timestamp is the wall-clock time a sample arrived, while the head's minimum time is a sample timestamp. An earlier change clamped the cutoff to at most one block range before now, to avoid releasing entries for resident series when sample timestamps run ahead of real time. Beta testing showed the clamp binds on every compaction and over-counts substantially. Compaction runs about an hour after a block boundary, so the head is truncated to that boundary while the cutoff is pinned an hour earlier. Every series that went idle in between leaves the head but keeps its entry. In beta us-west-2 cell 3, with a churning 2.4M-series tenant, owned exceeded the head by 15-22% on 17 of 18 ingesters after each compaction and stayed there until the next one. That makes the limit stricter than with the feature off, which is the same class of false throttling the feature exists to remove. Narrowing the clamp to the creation grace period would shrink the error but not remove it, and would depend on a per-tenant setting and on distributor and ingester clocks agreeing. Instead, stop estimating. The head already reports exactly which series it deleted, through the PostDeletion lifecycle callback Cortex uses to keep its in-memory series count. Release precisely those entries there. Each entry now records the head series reference it describes, refreshed on every sample. On deletion, an entry is removed only if it still carries the deleted reference. The head invokes the callback after releasing its own lock, so a sample can re-create the same labels as a new head series before the callback runs; that sample moves the entry to the new reference, and the stale deletion leaves it alone. References are never reused within a TSDB. The push path now keeps the reference returned by every successful append, rather than discarding it when a cached reference was used, so a series re-created between GetRef and Append is still recorded correctly. The time-based purge of retained entries and its cutoff function are removed. A backstop purge remains at compaction for entries idle longer than two block ranges, longer than any series can stay in the head without a sample, so it cannot under-count and only guards against an entry outliving its series by a path the callback does not cover. With owned-series tracking disabled nothing changes: references are not recorded or allocated, the callback does not touch the tracker, and entries are still released by the idle-timeout purge. TestIngester_OwnedSeriesFollowsHeadThroughCompaction drives a real head compaction with explicit sample timestamps, including a series idle for five minutes before the truncation point, and asserts the tracker holds exactly the series left in the head. It fails on the previous code with 3 entries for 1 head series, reproducing the beta result, and passes with this change. Unit tests cover release on a matching reference, retention across re-creation, unknown references, and adopting a reference late. Signed-off-by: Anant Shanbhag <anantvas@amazon.com>
…ding owned The limit check used the owned count only when the tracker held an entry for every series in the head, and otherwise fell back to the head count. That guard was meant for the window after a restart, but every new series enters the head a moment before its tracker entry is written. Under concurrent pushes the guard therefore failed during any burst of series creation, and an ingester that had just lost ownership of most of its head was throttled against the stale head count: the false throttling the owned count exists to prevent. Count series in the head without a tracker entry as owned instead. After a restart this equals the head count, as before; in normal running it adds only the series in flight. Apply the same to the instance-level count. In beta, about 37% of new series on the pre-existing ingesters were rejected after a scale-up. The new concurrent test rejects 300 of 640 series without this change and none with it. Signed-off-by: Anant Shanbhag <anantvas@amazon.com>
anant10
force-pushed
the
feat/owned-series-tracking
branch
from
October 10, 2026 01:14
640b980 to
3720c7b
Compare
This branch has not been deployed
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
2 participants
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 this PR does:
Adds per-ingester series ownership tracking to prevent false throttling during ingester scale-up and ring resharding.
The problem: When ingesters scale up, the per-ingester local series limit drops immediately (recalculated based on new ingester count), but stale series data remains in the TSDB head for up to 2 hours until head compaction.
PreCreation()usesHead().NumSeries()for limit checks, so it incorrectly rejects new writes during this window — the ingester appears over its new lower limit, but many of those series have been resharded to other ingesters and will be cleaned up at next compaction.The solution: Track which series each ingester actually owns according to the ring, and use that count for limit enforcement. The owned count drops immediately when the ring changes (within 1 minute), eliminating the 2-hour dependency on head compaction.
How it works:
ActiveSeriesupdateActiveSeriescycle), if the ring changed, re-scan all entries and remove series whose token no longer maps to this ingesterPreCreation()usesactiveSeries.Owned()instead ofHead().NumSeries()for the limit checkDesign decisions:
-ingester.owned-series-metrics-enabled: enablescortex_ingester_owned_seriesmetric emission only (no enforcement change)-ingester.owned-series-limit-enforcement-enabled: switches limit enforcement to use owned count (requires first flag)SearchToken(zoneTokens, key)→ is responsible token in this instance's set?atomic.Pointer[ringState]— zero lock contentionmax_series:instanceOwnedCountrecalculated every ~1 min (not incremental, avoids drift). Startup fallback toinstanceSeriesCountwhen count is 0pkg/util/fnv.go, sharding functions →pkg/ring/token.go(eliminates duplication between distributor and ring)Validation:
Tested in a multi-zone deployment under sustained write load. After scale-up:
memory_serieson old ingesters remained high (stale data in head)owned_serieson old ingesters dropped proportionally to ring redistributionNew configuration flags (experimental):
yaml
Which issue(s) this PR fixes: Fixes #7509