fix(containerprofile): split oversized chunks on HTTP 413 instead of ending learning - #866
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe queue now detects HTTP 413 responses, splits oversized container profiles, preserves report-chain metadata with stitch chunks, limits retries and split depth, persists processing state, and reports split/drop metrics through Prometheus and OTEL implementations. ChangesContainer profile 413 handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Queue
participant ProfileCreator
participant classifyFailure
participant Splitter
participant MetricsManager
Queue->>ProfileCreator: create container profile
ProfileCreator-->>Queue: return HTTP 413
Queue->>classifyFailure: classify response
classifyFailure-->>Queue: return split category
Queue->>Splitter: split profile or create stitch chunk
Splitter-->>Queue: return bounded queue items
Queue->>MetricsManager: report split or dropped chunk
Queue->>ProfileCreator: process requeued items
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…orever
Storage's QueueManager rejects a create whose Content-Length exceeds
kindQueues.containerprofiles.maxObjectSize with an HTTP 413 and a plain-text
body. The queue classified failures by exact string equality against
file.ObjectTooLargeError and file.ObjectCompletedError, and a 413 matches
neither, so it fell through to requeueImmediate.
The consequences compound: the profile was retried every RetryInterval (5s)
forever, the dque is disk-persistent so the poison item survived pod restarts,
and because requeuing appends to the tail while enforceMaxSize evicts from the
head, it could starve newer profiles that would have been accepted. The
container was never flagged too-large, because that only happens via
OnQueueError, so the profile silently had a hole in it with no UI signal.
Replace the string comparison with classifyFailure():
- HTTP 413 is detected with apierrors.IsRequestEntityTooLargeError, which
matches on the status code and so works despite the plain-text body, and is
reported as ObjectTooLargeError. handleSaveProfileError then sets
status=too-large and ends learning with reason "too_large".
- Sentinels are matched with errors.Is plus a substring check on the message,
because a StatusError only carries the message and storage prefixes the
sentinel with the limit on some paths. Exact equality missed both shapes.
Also bound retries with MaxAttempts (default 10). A 413 is only one way to get
a permanently failing item; without a bound, any such item occupies the queue
indefinitely. Attempts is added to the persisted item, and items written before
this field existed decode with it at zero, so they get a full retry budget.
Reported by ISO Gruppe on Rancher/RKE2: contentLength=3231797 against
maxObjectSize=2500000 for cattle-system workloads.
Signed-off-by: Rotem Refael <rotem@armosec.io>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…start DefaultMaxAttempts of 10 at a 5s interval gave only 50 seconds of tolerance. storage is a single-replica apiserver backed by a PVC, so a rollout, image pull or node eviction routinely takes longer than that, and profiles would have been dropped during an ordinary restart that the previous unbounded retry survived. Raise the default to 360, roughly 30 minutes, which still sheds a permanently failing item but no longer trades the infinite-retry bug for silent data loss. Add a test asserting the budget stays above 15 minutes so it cannot be tightened back without the tradeoff being considered. Signed-off-by: Rotem Refael <rotem@armosec.io> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
b02ad07 to
a8c9bb6
Compare
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup EffectivenessNo data available. |
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Thanks for the thorough writeup — the diagnosis in #865 is right, and I verified every load-bearing claim against kubescape/storage@v0.0.290: QueueManager really does reject on Content-Length with a plain-text 413, CreateContainerProfileDirect passes the clientset error through unwrapped, and the exact-string classification on main falls through to requeueImmediate forever on a disk-persistent queue. The bounded retry and the errors.Is + relayed/prefixed StatusError matching are real, independent fixes and I'd like to keep both.
My one blocking concern is the 413 -> ObjectTooLargeError -> end learning hop. I don't think we should stop learning just because a chunk is too big — we're sending TS container profiles here, and the two conditions are genuinely different:
-
ObjectTooLargeErroris a findings-count check on the aggregated profile. InContainerProfileProcessor.PreSave, the TS branch (ReportSeriesIdMetadataKey != "") returns early and never size-checks the chunk at all — it only rejects when the existing aggregate is already flaggedtoo-large/completed. The size rule lives in the non-TS branch and counts findings, not bytes:size += len(profile.Spec.Execs) + len(profile.Spec.Opens) + len(profile.Spec.Syscalls) + len(profile.Spec.Capabilities) + len(profile.Spec.Endpoints) + len(profile.Spec.IdentifiedCallStacks) + len(profile.Spec.Ingress) + len(profile.Spec.Egress) if size > a.MaxContainerProfileSize { // set annotation but don't return an error as we want to save the profile anyway profile.Annotations[helpers.StatusMetadataKey] = helpers.TooLarge }
It doesn't even return an error — it annotates and saves. So
ObjectTooLargeErroron a chunk means "the aggregate is full, stop sending", which is a fair reason to end learning. -
The 413 is a defensive
Content-Lengthcheck on one delta. Mapping it onto the sentinel manufactures the "container is full" verdict out of a transport limit.
What I'd like instead: split the chunk and send both halves
Since saveContainerProfile calls emptyEvents() after each enqueue, every queued profile is a delta. Cutting one delta in two is exactly what the existing ProfileRequiresSplit path already does (container_operations.go:38-50 — it's literally "flush early and start a new chunk"), just reactive instead of predictive. I checked the three things that could have broken it:
- Naming works, with one trap.
GetOneTimeSlugreturns<base-slug>-<uuid-hex>, and storage'sSplitProfileNamecuts on the last hyphen to recover the base name. Each half needs its own fresh-<uuid-hex>on the base name. Do not suffix the existing chunk name (...-<uuid>-part1) —SplitProfileNamewould then returnbase-<uuid>and scatter the halves under a different aggregate key. - The halves can't collide.
time_serieshasPRIMARY KEY (kind, namespace, name, seriesID, tsSuffix)andAfterCreatewritestsSuffixfrom the per-chunk UUID, so two halves land as distinct rows even with identicalreportTimestamp/previousReportTimestamp/seriesID. Nothing needs re-stamping. - The aggregate is unchanged. Chunks merge by
patchMergeKeyand the count islen()after dedupe, so how the delta was cut doesn't affect the findings total.
What splits vs what duplicates: partition the lists (Capabilities, Execs, Opens, Syscalls, Endpoints, IdentifiedCallStacks, Ingress, Egress) and the PolicyByRuleId map; copy Architectures, ImageID, ImageTag, SeccompProfile, LabelSelector and the annotations into both halves.
Halving converges in ceil(log2(bytes/cap)) rounds — 3.23 MB against 2.5 MB is a single round — and costs nothing server-side, since QueueManager rejects before dispatch. Floor case: a chunk holding one element across all lists that still 413s is genuinely unsendable, so drop that chunk with a Warning; still not the container.
Two smaller things
- The "container is finally flagged too-large" outcome doesn't actually land on the 413 path.
handleSaveProfileErrorsets the status on the in-memorywatchedContainer, then callsdeleteContainer, which explicitly skips the termination save when status isTooLarge(lifecycle.go:287-290). We only writeStatusMetadataKeyinsidesaveContainerProfile(monitoring.go:181), so no chunk ever carriesstatus=too-largeto storage. For the registry sentinel path that's fine — storage annotates the object itself — but for a 413 the request never reaches the registry, so the series is just abandoned and the expiry sweep later marks itCompleted/Partial. Worth correcting in the description either way. - The estimator is why we get a 413 at all.
maxTsProfileSizedefaults to 2 MiB — already under storage's 2.5 MB — yet the observed body was 3.23 MB, becausesizemixes byte counts (ReportCapability->size.Of(...)) with element counts (ReportSyscall->syscalls.Append(...), the number of elements added) and never accounts for egress/ingress neighbors being expanded at serialization time after DNS resolution. That's a separate node-agent-side defect from the cap inconsistency tracked in kubescape/storage#350 — worth its own issue. Fix it and the split stays a safety net rather than the normal path.
Verification on my side: go build ./... ok, go test ./pkg/containerprofilemanager/... all pass, gofmt/vet clean. The -race failure in TestQueueWithDifferentConfigurations/small-queue does reproduce on main, so that note is accurate. I also confirmed the gob claim independently — dque uses gob (segment.go:138,221), old->new decodes with Attempts=0 and new->old ignores the extra field, so the persisted-item change is safe both ways.
| // QueueManager rejects oversized requests up front with 413 and a plain-text body, so | ||
| // there is no Status object to carry a reason. IsRequestEntityTooLargeError matches on | ||
| // the status code alone, which is what makes this work. | ||
| if apierrors.IsRequestEntityTooLargeError(err) { |
There was a problem hiding this comment.
This is the part I'd like changed. Detecting the 413 here is right — IsRequestEntityTooLargeError matching on the status code alone is exactly what makes it work against the plain-text body. Reporting it as file.ObjectTooLargeError is what I'd push back on.
The two conditions aren't the same. ObjectTooLargeError is a findings-count verdict on the aggregated profile — and note that for a TS chunk, PreSave returns early and never size-checks the chunk at all, so the sentinel only ever reaches us when the aggregate is already flagged. The 413 is a Content-Length check on a single delta.
Suggest a third failureKind here — something like failureOversized — so the queue can split the chunk and requeue both halves instead of ending learning for the container. The sentinel path stays exactly as you have it.
| // (an HTTP 413 from storage's QueueManager carries no sentinel at all). | ||
| if qd.errorCallback != nil { | ||
| qd.errorCallback.OnQueueError(queuedProfile.Profile, queuedProfile.ContainerID, err) | ||
| qd.errorCallback.OnQueueError(queuedProfile.Profile, queuedProfile.ContainerID, reported) |
There was a problem hiding this comment.
This is where the consequence lands: reported is the sentinel, so handleSaveProfileError sets status=too-large, calls deleteContainer and ends learning with reason too_large — permanently, for the whole container, on one oversized delta.
Since saveContainerProfile calls emptyEvents() after each enqueue, every queued profile is a delta, and the first report after container start carries everything observed so far — so this fires on the peak, not on a steady state. On main the container kept learning and reached Completed at max sniff time, losing only that one delta.
For the oversized case I'd like: split the profile in two, requeue both, no OnQueueError, learning continues. The ObjectCompletedError / aggregate-ObjectTooLargeError sentinels should keep going through this branch unchanged — those genuinely mean "stop sending".
| // succeeds cannot occupy the queue indefinitely: requeuing appends to the tail | ||
| // while enforceMaxSize evicts from the head, so an unbounded retry would push | ||
| // out newer profiles that could have been saved. | ||
| queuedProfile.Attempts++ |
There was a problem hiding this comment.
Once the split lands, a chunk being usefully halved shouldn't consume this budget — otherwise productive splits burn retries meant for storage being unreachable. Worth carrying a separate split-depth counter on QueuedContainerProfile alongside Attempts, with the drop happening when a chunk can't be divided any further rather than when the retry budget runs out.
The bound itself is good and I'd keep it — including the 360 default and TestDefaultRetryBudgetOutlastsARestart. One thing worth adding: this path drops silently, with no callback and no metric. With a full 1000-item queue and a >30 min outage, every item exhausts its budget in roughly the same tick, so this can emit up to 1000 Warning lines at once. A counter and rate-limited logging would make the loss visible.
| // matchesSentinel reports whether err carries the given storage sentinel, either as a | ||
| // wrapped error or as text in the message relayed by the apiserver. | ||
| func matchesSentinel(err, sentinel error) bool { | ||
| return errors.Is(err, sentinel) || strings.Contains(err.Error(), sentinel.Error()) |
There was a problem hiding this comment.
Minor: the substring arm is unanchored, so any error whose message happens to embed "object is too large" or "object is completed" becomes terminal and ends learning for the container. Low probability, but the blast radius is large for a text match.
The errors.Is arm and the prefixed-message case are both well justified — storage really does wrap as "...size exceeds the limit of %d: %w". Could we narrow the substring arm to *apierrors.StatusError (where we know the message is a relayed sentinel and errors.Is can't work) rather than applying it to every error shape?
| // TestQueueEndsLearningOnHTTP413 is the regression test for the ISO Gruppe report: an | ||
| // oversized profile rejected by storage's QueueManager must end learning instead of being | ||
| // retried every RetryInterval forever. | ||
| func TestQueueEndsLearningOnHTTP413(t *testing.T) { |
There was a problem hiding this comment.
This test pins the behaviour I'm asking to change, so it'll need reshaping: a 413 should result in the profile being split and both halves delivered, with OnQueueError never called and learning still running.
The rest of the file is genuinely good — building the errors the way client-go actually does (NewGenericServerResponse for the plain-text 413, a relayed StatusError for the sentinels) is the right level of fidelity, and TestClassifyFailure_413IsNotRequeued is a nice guard against the old equality check coming back. A couple worth adding alongside the split tests:
- the two halves carry distinct
-<uuid-hex>name suffixes thatSplitProfileNamemaps back to the same base name (a...-part1style suffix would silently re-base the aggregate key); Attemptssurvives a queueClose/reopen — the whole retry bound depends on that counter being persisted, and nothing exercises it today.
…ending learning storage's QueueManager rejects a create whose Content-Length exceeds its size cap with a bare HTTP 413 and no sentinel. Previously this was treated the same as storage's own too-large sentinel, permanently ending learning for the container. A 413 is a transport limit on one delta, not an aggregate signal, so a container that produces a single oversized chunk should not lose all future learning. classifyFailure now returns a third failureSplit kind for a bare 413 (the two genuine sentinels remain terminal, and are checked first so an authoritative signal always wins over a status code). On failureSplit, the rejected chunk is recursively halved and both halves re-enqueued until each is small enough to be accepted, bounded by MaxSplitDepth and a byte-progress guard. Halving requires care to avoid corrupting storage's time-series consolidation: giving both halves the same report-timestamp pair would permanently fork the chain and hang the profile in Learning forever, so a fresh timestamp is interposed between the parent's (previousReportTimestamp, reportTimestamp) pair to keep the chain linear. Timestamps are also stripped of their monotonic-clock suffix and parsed via a numeric-timezone-abbreviation fallback, since production timestamps come from time.Now().String(). An unsplittable chunk is dropped with a stitch chunk in its place to preserve the chain link, without ever calling back into the container's error path (which would end learning) and without ever re-stitching a dropped stitch. Addresses maintainer review feedback on #866: #866 (review) Also fixes a pre-existing data race in MockProfileCreator (CreatedProfiles/ CallCount were unsynchronized fields read across goroutines by tests), found while getting the package's -race gate green for the new tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
matthyx
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
docs/features/container-profile-split-on-413.md (1)
31-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a language to the fenced blocks.
markdownlint reports MD040 for these blocks. Use
textfor the diagrams and pseudo-code so the linter passes.Also applies to: 51-55, 67-70, 76-79, 107-114
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/features/container-profile-split-on-413.md` around lines 31 - 33, Update the fenced code blocks in the document, including the blocks around the displayed formula and the referenced sections, to declare the text language. Use text for diagrams, formulas, and pseudo-code so markdownlint MD040 passes.Source: Linters/SAST tools
pkg/containerprofilemanager/v1/queue/containerprofile_split_test.go (1)
75-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBound the chain walk to avoid a possible test hang.
The walk terminates only when a
previousReportTimestampkey is missing. If a future regression produces a cycle in the links, this loop never exits and the test hangs instead of failing. Add an iteration bound.♻️ Proposed guard
cur := origPrev visited := 0 - for { + for visited <= len(rows) { row, ok := byPrev[cur] if !ok { break } cur = row.ReportTimestamp visited++ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/containerprofilemanager/v1/queue/containerprofile_split_test.go` around lines 75 - 87, Bound the loop in the chain walk around cur, byPrev, and visited so it can iterate at most len(rows) times. Preserve the existing missing-key termination and assertions, allowing a cycle to exit through the bound and fail the chain validation instead of hanging.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/features/container-profile-split-on-413.md`:
- Around line 174-188: Update the Tests section to use the actual test
identifiers: TestChainHalves_ConsolidatesToParentInterval,
TestChainHalves_ZeroPreviousTimestamp, TestParseReportTimestamp_MonotonicSuffix,
TestSplitProfile_FloorCase, TestSplitProfile_NoProgressGuard,
TestSplitProfile_Deterministic, and
TestStitchChunk_PreservesAssignmentMergedScalars. Point the queue-level tests
TestQueueSplitsProfileOnHTTP413, TestQueueRespectsMaxSplitDepth, and
TestQueueDoesNotStitchAStitch to containerprofile_queue_errors_test.go,
preserving the existing test-category descriptions.
In `@pkg/containerprofilemanager/v1/queue/containerprofile_queue_errors.go`:
- Around line 68-72: Update matchesSentinel to apply the strings.Contains
fallback only when err is a *apierrors.StatusError; retain errors.Is for all
errors and use the status error’s relayed message for the substring comparison.
In `@pkg/containerprofilemanager/v1/queue/containerprofile_queue.go`:
- Around line 446-451: Update the first-half failure branch in the
split-container-profile enqueue flow around enqueueLocked(half(a)) to log at
Warning level, matching the second-half failure path, and increment the
corresponding loss/failure counter before returning after the parent has been
dequeued.
---
Nitpick comments:
In `@docs/features/container-profile-split-on-413.md`:
- Around line 31-33: Update the fenced code blocks in the document, including
the blocks around the displayed formula and the referenced sections, to declare
the text language. Use text for diagrams, formulas, and pseudo-code so
markdownlint MD040 passes.
In `@pkg/containerprofilemanager/v1/queue/containerprofile_split_test.go`:
- Around line 75-87: Bound the loop in the chain walk around cur, byPrev, and
visited so it can iterate at most len(rows) times. Preserve the existing
missing-key termination and assertions, allowing a cycle to exit through the
bound and fail the chain validation instead of hanging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ee9a94a3-094e-4a6c-9616-261213b47593
📒 Files selected for processing (12)
docs/features/container-profile-split-on-413.mdpkg/containerprofilemanager/v1/queue/containerprofile_queue.gopkg/containerprofilemanager/v1/queue/containerprofile_queue_errors.gopkg/containerprofilemanager/v1/queue/containerprofile_queue_errors_test.gopkg/containerprofilemanager/v1/queue/containerprofile_queue_test.gopkg/containerprofilemanager/v1/queue/containerprofile_split.gopkg/containerprofilemanager/v1/queue/containerprofile_split_test.gopkg/metricsmanager/metrics_manager_interface.gopkg/metricsmanager/metrics_manager_mock.gopkg/metricsmanager/metrics_manager_noop.gopkg/metricsmanager/otel/otel_metrics_manager.gopkg/metricsmanager/prometheus/prometheus.go
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup EffectivenessNo data available. |
Superseded: the requested split-on-413 approach landed in 49f5f19. Re-reviewed there; dismissing so this no longer blocks.
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Re-reviewed at 49f5f19. This addresses everything from my previous review, and it's a considerably more careful change than what I asked for — I've dismissed the stale CHANGES_REQUESTED so it isn't blocking. One real finding below, plus one leftover minor.
Verification on my side: go build ./... ok · go test ./pkg/containerprofilemanager/... ./pkg/metricsmanager/... all pass · gofmt/vet clean · -race on the queue package now passes, so the MockProfileCreator race that fails on main got fixed here too — worth calling out in the description, since the previous revision listed it as pre-existing and untouched.
What landed
- The 413 no longer maps to
ObjectTooLargeError. The newfailureSplitkind, and the decision to run both sentinel checks before the status-code check so an authoritative aggregate verdict still wins over a bare 413, is exactly the right split of the two meanings. - Splits inherit
Attemptsrather than consuming the retry budget. - Drops are counted, labelled with a closed
dropReasonset, and exported — that closes the "silent drop with no metric" gap I raised. TestQueuePersistsSplitDepthcoversAttempts/SplitDepth/IsStitchsurviving aClose/reopen, which was the test I'd flagged as missing.
I re-derived the storage-side claims the design rests on rather than trusting the comments, and they hold: mergeContainerProfileTS does assign SeccompProfile/ImageID/ImageTag while appending everything else, and AfterCreate does hardcode hasData=true — so stitchChunk keeping exactly those three scalars is correct, and a naively empty stitch really would have zeroed them on the aggregate.
One thing you can relax: DeflateContainerProfileSpec runs DeflateSortString(Architectures) and DeflateLabelSelectorRequirement(MatchExpressions) before the size count, so the duplication a split introduces in those fields is deduped server-side regardless. Clearing Architectures in the stitch is a nice-to-have rather than load-bearing — the SeccompProfile/ImageID/ImageTag part is the part that matters.
Leftover minor
matchesSentinel still uses an unanchored strings.Contains. I raised this last time and the check ordering is now deliberate, which makes that substring arm load-bearing for the ordering decision: a 413 whose body happened to contain "object is too large" would classify terminal and end learning. QueueManager's own body is "Request entity too large" so it doesn't bite today, but a proxy or ingress returning arbitrary text could. Narrowing the substring arm to *apierrors.StatusError — where you know the message is a relayed sentinel and errors.Is genuinely can't work — would remove the last text-matching hazard without losing the prefixed-sentinel case.
The chainHalves shim, its TEMPORARY marker and the pointer to kubescape/storage#352 are well judged, as is deriving the base name from storage's own file.SplitProfileName rather than reimplementing the suffix heuristic.
| defer qd.mu.Unlock() | ||
|
|
||
| if err := qd.enqueueLocked(half(a)); err != nil { | ||
| logger.L().Debug("failed to enqueue the first half of a split container profile", |
There was a problem hiding this comment.
The two enqueue-failure paths here are asymmetric, and the first one is the worse of the two.
If the first half fails, this logs at Debug and returns — so b is never enqueued either. The parent was already dequeued, so both halves are gone. The second-half failure loses one half and correctly logs a Warning naming the forked chain; the first-half failure loses the entire chunk and is quieter about it. Neither path increments chunksDropped or calls ReportContainerProfileChunkDropped, so the new metric can't see either.
enqueueLocked returns an error on !qd.running — reachable during Close while processAllItems is still mid-flight — or on a dque disk error. Rare, but the consequence is precisely what this PR exists to prevent: a forked chain, which by the analysis in #871 leaves the profile in Learning permanently.
Suggest treating it like the second-half case: Warning, a drop-metric bump, and ideally a stitch carrying the parent's original (previousReportTimestamp, reportTimestamp) pair — at that point nothing from the parent reached the queue, so the parent's own interval is exactly what needs repairing. That also makes the two branches read the same way, which matters for a path this hard to exercise.
- matchesSentinel's strings.Contains fallback now only fires for *apierrors.StatusError, where the message is known to be a sentinel relayed verbatim by the apiserver. Previously it ran against every error shape, so any error whose text happened to embed "object is too large" or "object is completed" (e.g. from a proxy or ingress) would misclassify as terminal and end learning for the container - now that sentinel-checks-before-413-check is a deliberate ordering, this substring arm had become load-bearing enough to matter. - requeueSplit's first-half enqueue failure was logged at Debug with no metric, even though the parent was already dequeued and so both halves are lost - a total loss, strictly worse than the second-half case next to it, which correctly logs a Warning. Both paths now log at Warning and increment the chunksDropped/drop-metric counters, and the first-half case additionally enqueues a stitch chunk carrying the parent's original (previousReportTimestamp, reportTimestamp] interval, since nothing of the parent reached the queue in any form. - Corrected the docs page's Tests section: it cited test names and a file path that don't match what was actually written. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…lit's lost-halves path Both behavior changes in 9a9ce80 shipped without a regression test, as pointed out on review: reverting either fix would have left every existing test green. - TestClassifyFailure gains a case for a plain, non-StatusError error whose text merely mentions a sentinel ("object is too large") - exactly the input whose classification changed from terminal to retryable, and the one matchesSentinel's old unanchored strings.Contains would have caught. - New TestEnqueueLocked_ReturnsErrQueueNotRunning and TestRequeueSplit_QueueNotRunningDropsBothHalvesAndAttemptsStitch exercise the previously-uncovered ErrQueueNotRunning path white-box (tests live in package queue): flipping qd.running directly reproduces the state reachable during Close while processAllItems is still mid-flight, and confirms the first-half enqueue failure is now logged, counted, and triggers a stitch attempt rather than silently returning. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup EffectivenessNo data available. |
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup EffectivenessNo data available. |
…xAttempts exhaustion (kubescape#871) enforceMaxSize and the MaxAttempts drop path discarded a queued chunk with no replacement, forking the container's report-timestamp chain and hanging the profile in Learning forever with no field-visible symptom. Both paths now go through the same stitch-repair mechanism kubescape#866 introduced for its own split-chunk drops: MaxAttempts exhaustion routes through dropChunk, and enforceMaxSize replaces an evicted item with a stitch instead of discarding it. Since replacing every eviction with a same-count stitch makes no net progress toward MaxQueueSize by itself, enforceMaxSize bounds the in-flight stitch backlog (10% of MaxQueueSize, floor 1); once exhausted, further evictions fall back to the original unrepaired drop rather than let the queue grow without bound or convert itself entirely into stitches in one call. Signed-off-by: aryanghai12 <aryanghai1205@gmail.com>
…and retry exhaustion (#884) * fix(containerprofile): repair the report chain on LRU eviction and MaxAttempts exhaustion (#871) enforceMaxSize and the MaxAttempts drop path discarded a queued chunk with no replacement, forking the container's report-timestamp chain and hanging the profile in Learning forever with no field-visible symptom. Both paths now go through the same stitch-repair mechanism #866 introduced for its own split-chunk drops: MaxAttempts exhaustion routes through dropChunk, and enforceMaxSize replaces an evicted item with a stitch instead of discarding it. Since replacing every eviction with a same-count stitch makes no net progress toward MaxQueueSize by itself, enforceMaxSize bounds the in-flight stitch backlog (10% of MaxQueueSize, floor 1); once exhausted, further evictions fall back to the original unrepaired drop rather than let the queue grow without bound or convert itself entirely into stitches in one call. Signed-off-by: aryanghai12 <aryanghai1205@gmail.com> * fix(containerprofile): bound stitch admission in dropChunk and requeueSplit too enforceMaxSize checked the in-flight stitch backlog before repairing an eviction, but dropChunk and requeueSplit's lost-first-half case built and enqueued their own replacement stitch unconditionally. Left unfixed, a sustained run of MaxAttempts exhaustions or unsplittable drops could grow the backlog past the bound the eviction loop itself relies on to terminate promptly, since only enforceMaxSize's own path was gated. Factor the check into stitchBacklogFull and call it from all three stitch-admission sites; once the backlog is spent, dropChunk and requeueSplit now fall back to the same unrepaired, forked drop (dropReasonStitchBacklogExhausted) enforceMaxSize already used. Also tightens the docs page's claim that dropReasonStitchRejected is the only drop path that can leave the chain forked - dropReasonLRUBacklogExhausted already qualified that, and now dropReasonStitchBacklogExhausted does too. Signed-off-by: aryanghai12 <aryanghai1205@gmail.com> * fix(containerprofile): clamp stitch backlog, fix double-counted drops, fresh retry budget for MaxAttempts stitches Addresses review findings on the queue-drop-lineage fix: - stitchBacklog started at zero every process start but the disk queue can already hold stitches from a prior run; decrementing for one of those (which never incremented the fresh counter) drove it negative permanently, silently widening maxStitchBacklog by up to maxQueueSize. releaseStitch clamps the decrement at zero. - chunksDropped was incremented twice for a single lost chunk whenever its repair also failed (dropChunk, requeueSplit's lost-first-half case, and enforceMaxSize's failed stitch enqueue all double-counted). Each site now increments the counter once per chunk and reports both drop reasons as separate metric samples instead. - A stitch repairing a MaxAttempts-exhaustion drop inherited the parent's already-exhausted Attempts, so it got exactly one try - a no-op against a storage outage that hasn't recovered. dropChunk now gives that stitch a fresh retry budget (newStitchFor's freshAttempts); a stitch is never re-stitched, so total exposure stays bounded at two retry windows. - dropChunk, requeueSplit's lost-first-half stitch, and enforceMaxSize's own eviction loop now all gate on stitchBacklogFull before admitting a stitch, not just enforceMaxSize's loop. - NewQueueData's startup enforceMaxSize sweep now runs under qd.mu, matching the invariant enqueueStitchNoEvict's doc comment already claimed. - Documented two residual trade-offs: the MaxAttempts repair is best-effort under a sustained outage, and a restart at capacity can now shed two deltas instead of one. Signed-off-by: aryanghai12 <aryanghai1205@gmail.com> --------- Signed-off-by: aryanghai12 <aryanghai1205@gmail.com>
…and retry exhaustion (kubescape#884) * fix(containerprofile): repair the report chain on LRU eviction and MaxAttempts exhaustion (kubescape#871) enforceMaxSize and the MaxAttempts drop path discarded a queued chunk with no replacement, forking the container's report-timestamp chain and hanging the profile in Learning forever with no field-visible symptom. Both paths now go through the same stitch-repair mechanism kubescape#866 introduced for its own split-chunk drops: MaxAttempts exhaustion routes through dropChunk, and enforceMaxSize replaces an evicted item with a stitch instead of discarding it. Since replacing every eviction with a same-count stitch makes no net progress toward MaxQueueSize by itself, enforceMaxSize bounds the in-flight stitch backlog (10% of MaxQueueSize, floor 1); once exhausted, further evictions fall back to the original unrepaired drop rather than let the queue grow without bound or convert itself entirely into stitches in one call. Signed-off-by: aryanghai12 <aryanghai1205@gmail.com> * fix(containerprofile): bound stitch admission in dropChunk and requeueSplit too enforceMaxSize checked the in-flight stitch backlog before repairing an eviction, but dropChunk and requeueSplit's lost-first-half case built and enqueued their own replacement stitch unconditionally. Left unfixed, a sustained run of MaxAttempts exhaustions or unsplittable drops could grow the backlog past the bound the eviction loop itself relies on to terminate promptly, since only enforceMaxSize's own path was gated. Factor the check into stitchBacklogFull and call it from all three stitch-admission sites; once the backlog is spent, dropChunk and requeueSplit now fall back to the same unrepaired, forked drop (dropReasonStitchBacklogExhausted) enforceMaxSize already used. Also tightens the docs page's claim that dropReasonStitchRejected is the only drop path that can leave the chain forked - dropReasonLRUBacklogExhausted already qualified that, and now dropReasonStitchBacklogExhausted does too. Signed-off-by: aryanghai12 <aryanghai1205@gmail.com> * fix(containerprofile): clamp stitch backlog, fix double-counted drops, fresh retry budget for MaxAttempts stitches Addresses review findings on the queue-drop-lineage fix: - stitchBacklog started at zero every process start but the disk queue can already hold stitches from a prior run; decrementing for one of those (which never incremented the fresh counter) drove it negative permanently, silently widening maxStitchBacklog by up to maxQueueSize. releaseStitch clamps the decrement at zero. - chunksDropped was incremented twice for a single lost chunk whenever its repair also failed (dropChunk, requeueSplit's lost-first-half case, and enforceMaxSize's failed stitch enqueue all double-counted). Each site now increments the counter once per chunk and reports both drop reasons as separate metric samples instead. - A stitch repairing a MaxAttempts-exhaustion drop inherited the parent's already-exhausted Attempts, so it got exactly one try - a no-op against a storage outage that hasn't recovered. dropChunk now gives that stitch a fresh retry budget (newStitchFor's freshAttempts); a stitch is never re-stitched, so total exposure stays bounded at two retry windows. - dropChunk, requeueSplit's lost-first-half stitch, and enforceMaxSize's own eviction loop now all gate on stitchBacklogFull before admitting a stitch, not just enforceMaxSize's loop. - NewQueueData's startup enforceMaxSize sweep now runs under qd.mu, matching the invariant enqueueStitchNoEvict's doc comment already claimed. - Documented two residual trade-offs: the MaxAttempts repair is best-effort under a sustained outage, and a restart at capacity can now shed two deltas instead of one. Signed-off-by: aryanghai12 <aryanghai1205@gmail.com> --------- Signed-off-by: aryanghai12 <aryanghai1205@gmail.com> Signed-off-by: entlein <einentlein@gmail.com>
2 participants
Fixes #865
Problem
customer of Rancher/RKE2 saw storage log this on repeat for
cattle-systemworkloads:storage's
QueueManagerrejects a create whoseContent-LengthexceedskindQueues.containerprofiles.maxObjectSizebefore it reaches the registry, replying413with a plain-text body — so there is noStatusobject and no sentinel.What changed since the last revision of this PR
@matthyx's review (#866 (review)) verified the original fix's diagnosis and bounded-retry mechanism, but flagged one blocking concern: mapping a bare 413 onto
ObjectTooLargeErrorconflates two different signals.ObjectTooLargeErroris storage's own aggregate findings-count check — an authoritative "this container's profile is genuinely full" verdict that's fair to end learning on. A bare 413 is a transport rejection of one delta based on byte size alone, and treating it the same way throws away all future learning for a container that merely produced one oversized chunk.This revision implements the reviewer's suggested fix: on a bare 413, split the rejected chunk in half and resend both halves, recursing until each piece is accepted, instead of ending learning.
Change
classifyFailure()gains a third reaction,failureSplit, alongside the existingfailureRetryable/failureTerminal. The two genuine storage sentinels (ObjectTooLargeError,ObjectCompletedError, matched viaerrors.Is+ substring) are checked first and remain terminal exactly as before — only a bare 413 detected purely by HTTP status code (apierrors.IsRequestEntityTooLargeError) now triggersfailureSplitinstead of being treated as terminal.Splitting (
containerprofile_split.go, new).splitProfilepartitions the rejected chunk's list/map fields (Capabilities,Execs,Opens,Syscalls,Endpoints,IdentifiedCallStacks,Ingress,Egress,PolicyByRuleId) roughly in half, with a non-empty guarantee so recursion always makes progress, and gives each half a fresh one-time slug over the same base name (recovered via storage's ownfile.SplitProfileName, so agreement with the server's aggregation key is true by construction — a-partNsuffix scheme was considered and rejected, sinceSplitProfileNamecuts on the last hyphen and would scatter a differently-suffixed half into a phantom aggregate). A byte-progress guard refuses to keep splitting a chunk that isn't shrinking.Preserving the report chain — the trickiest part. Storage reconstructs a container's lifecycle by walking a strictly linear
previousReportTimestamp → reportTimestampchain (consolidateContinuousTimeSeries). Naively giving both halves the parent's identical timestamp pair permanently forks that chain and leaves the profile inLearningforever — worse than today's behavior. Instead,chainHalvesinterposes a fresh timestampXso one half spans(P, X]and the other(X, T], keeping the chain linear (including for the next real report, and correctly under recursive re-splitting). This required care around: a monotonic-clock suffix on production timestamps (time.Now().String()) that needs stripping before parsing; a numeric (non-letter) timezone-abbreviation fallback for several IANA zones; and never taking a naive midpoint against a first-report's zeropreviousReportTimestamp(which would otherwise produce an implausible timestamp that storage's expiry check matches unconditionally, re-ending learning through a different route).Bounding the recursion.
MaxSplitDepth(default 4) plus the byte-progress guard cap how many times a single chunk can be halved. A chunk that can't be split further (a single oversized element, or depth exhausted) is dropped with aWarning— and replaced by a stitch chunk: a metadata-and-scalars-only stand-in that preserves the dropped chunk's(previousReportTimestamp, reportTimestamp)link so the chain doesn't fork, without carrying any of the oversized data. A stitch is never itself re-stitched. Neither drop path callsErrorCallback.OnQueueError— that's the callback that ends learning, and avoiding it on every 413 path is the entire point of this change.Observability. Two new counters (
ReportContainerProfileSplit,ReportContainerProfileChunkDropped(reason)) across allMetricsManagerimplementations, since after this change there's no remaining status signal on the 413 path at all — these counters are the only way to see the underlying estimator issue (#870) firing in the field.The bounded-retry mechanism from the previous revision (
MaxAttempts, default 360) is unchanged and still governs genuinely retryable failures.Tests
containerprofile_split_test.go(new) andcontainerprofile_queue_errors_test.go(extended) — partition correctness (no loss/duplication across halves), shared-field verbatim-copy vs. the two report-timestamp fields (deliberately not equal across halves), naming/base-name preservation, the floor case and non-empty rebalance, determinism, the timestamp-chain interposition and its zero-time/monotonic-suffix/numeric-timezone edge cases (including a golden port of storage's own consolidation algorithm so drift against the real thing is caught), recursive re-splitting staying chain-linear, the byte-progress guard, and end-to-end queue tests for split-and-resend, the unsplittable floor case (with stitch-chunk emission),MaxSplitDepthbounding, andSplitDepth/stitch persistence across a queue restart.Verified:
Also fixed a pre-existing data race in the test-only
MockProfileCreator(unsynchronized fields read across goroutines), found while getting the package's-racegate green.Not in this PR
config.MaxTsProfileSize) mixes byte-size and element-count units, which is the root cause of reaching a 413 at all. Fixing it should make splitting a rare safety net rather than something regularly exercised.MaxAttemptsexhaustion drop or LRU eviction can already break a container's report chain today, independent of this PR, permanently preventing that container's profile from completing. Found as a side effect of verifying this PR's own chain-preservation logic; appears to have never been reported.consolidateContinuousTimeSeriesshould collapse rows sharing an identical(previousReportTimestamp, reportTimestamp)pair. This is the reason node-agent has to manufacture an intermediate timestamp at all; once it lands (and once a minimum storage version can be assumed), thechainHalvesinterposition logic in this PR becomes unnecessary and can be deleted.ObjectTooLargeErrorinto an HTTP 201 with the data discarded server-side (clearSpec) is unrelated to the 413 path this PR addresses, but was a lower-priority thing worth someone eventually surfacing as a client-visible signal.Summary by CodeRabbit
New Features
Documentation
Tests