fix(sbommanager): decouple shared-data wait from single-worker pool to stop node-wide SBOM stall (#850)#859
Conversation
…o stop node-wide SBOM stall (kubescape#850) Signed-off-by: Aaradhya-07 <aaradhya07n@gmail.com>
📝 WalkthroughWalkthroughThe SBOM manager now bounds shared-container-data waits to ten minutes, propagates caller cancellation through retry logic, and delays worker-pool submission until shared data is available. Tests cover timeout handling and cached-data success. ChangesSBOM shared-data wait
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ContainerCallback
participant waitForSharedContainerData
participant SbomManagerWorkerPool
ContainerCallback->>waitForSharedContainerData: wait with timeout context
waitForSharedContainerData-->>ContainerCallback: shared container data
ContainerCallback->>SbomManagerWorkerPool: submit processContainerWithMetadata
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/sbommanager/v1/sbom_manager.go (1)
260-264: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInclude context and error details in the log message.
To maintain consistency with the rest of the file and aid in debugging, consider passing the context to the logger, logging the actual error, and including the namespace and pod name (available on
notif.Container.K8s).♻️ Proposed log enhancement
if err != nil { - logger.L().Error("SbomManager - container not found in shared data", + logger.L().Ctx(ctx).Error("SbomManager - container not found in shared data", + helpers.Error(err), + helpers.String("namespace", notif.Container.K8s.Namespace), + helpers.String("pod", notif.Container.K8s.PodName), + helpers.String("container", notif.Container.K8s.ContainerName), helpers.String("container ID", notif.Container.Runtime.ContainerID)) return }🤖 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/sbommanager/v1/sbom_manager.go` around lines 260 - 264, Update the error logging in the notification handling flow to pass the available context, include the original error details, and add the container namespace and pod name from notif.Container.K8s alongside the container ID. Preserve the existing early return after logging.
🤖 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.
Nitpick comments:
In `@pkg/sbommanager/v1/sbom_manager.go`:
- Around line 260-264: Update the error logging in the notification handling
flow to pass the available context, include the original error details, and add
the container namespace and pod name from notif.Container.K8s alongside the
container ID. Preserve the existing early return after logging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b5560279-10f7-40dc-a1c3-b90e5aeda00c
📒 Files selected for processing (2)
pkg/sbommanager/v1/sbom_manager.gopkg/sbommanager/v1/sbom_manager_sharedwait_test.go
…xt, error, and pod fields Signed-off-by: Aaradhya-07 <aaradhya07n@gmail.com>
|
@Aaradhya-07 can you solve the conflicts? |
Signed-off-by: Aaradhya-07 <aaradhya07n@gmail.com>
|
@matthyx sorry I didnt see that, resolved . |
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Reviewed at 8511422 (post-merge-with-main). Build, go vet and go test ./pkg/sbommanager/... all pass locally, and the direction is right: moving the wait off the pool-of-1 does remove the node-wide head-of-line block, and keeping workerpool.New(1) is the correct call.
Three things I'd want addressed before merge, plus two notes. Details are in the line comments; summary:
Blocking
-
The wait window is now shorter than the producer's.
maxWaitForSharedContainerData = 10mis compared againstcontainerprofilemanager, but the thing that actually populates this data isContainerWatcher.setSharedWatchedContainerData, which retries ongithub.com/cenkalti/backoffv2 withDefaultMaxElapsedTime = 15m. Before this PR the SBOM side also ran to 15m (backoff v5's ownDefaultMaxElapsedTime), so the two matched. Now there is a 10–15m window where the producer still succeeds but the consumer has already given up, andContainerCallbackfires once per container add with no retry path — so that container silently never gets an SBOM. -
Nothing ever cancels these goroutines.
s.ctxtraces back toctx := context.Background()incmd/main.go:78— it is never cancelled, socontext.WithTimeout(s.ctx, 10m)is always a flat 10-minute wall clock. Combined with the fact that shared data is deleted on container-remove, every short-lived container — the exact population that motivated #850 — parks a goroutine that is guaranteed to fail, for the full 10 minutes, holdingnotif,mountsand aVerbose: trueImageStatusResponse. -
Neither new test covers the actual fix. Both exercise
waitForSharedContainerDatain isolation. Revert thego func(){...}wrapper (keep the ctx parameter) and both still pass, while the node-wide stall is back.
Non-blocking
-
The PR description says the benign "container not found in shared data" log from #848 is unchanged — it isn't; it gained
.Ctx()and four fields, and it now fires for every short-lived container atErrorlevel. Worth demoting when the cause iscontext.DeadlineExceeded, and worth correcting the description. -
Pre-existing, but it interacts with the premise here:
containerCallbackAsyncrunsgo cw.setSharedWatchedContainerData(...)on add and callsDeleteSharedContainerDatasynchronously on remove, with no ordering between them. A lateSetcan land after theDelete, which both leaks a permanent cache entry and lets this wait succeed for a container that is already gone — the subsequent scan then runs against stale/proc/<pid>lowerdirs and burns afailureRetriesslot. Not for this PR, but it means "data is deleted on remove, so the wait will time out" isn't reliably true.
| maxFailureRetryEntries = 1000 | ||
| failureRetryTTL = 30 * time.Minute | ||
| // maxWaitForSharedContainerData bounds the wait for a container's shared data; mirrors containerprofilemanager's MaxWaitForSharedContainerData. | ||
| maxWaitForSharedContainerData = 10 * time.Minute |
There was a problem hiding this comment.
Blocking — this shortens the effective wait from 15m to 10m and can silently drop SBOMs.
The comment justifies 10m by mirroring containerprofilemanager, but the relevant counterpart is the producer, not another consumer. Shared container data is written by ContainerWatcher.setSharedWatchedContainerData (pkg/containerwatcher/v2/containercallback.go:84), which retries with:
err := backoff.Retry(func() error { ... }, backoff.NewExponentialBackOff())using github.com/cenkalti/backoff v2.2.1, whose NewExponentialBackOff() sets MaxElapsedTime = DefaultMaxElapsedTime = 15 * time.Minute.
Before this PR the consumer side ran on context.Background() and was bounded by backoff v5's own DefaultMaxElapsedTime, also 15m — so producer and consumer windows matched exactly. After this PR the consumer gives up at 10m while the producer keeps retrying to 15m.
Concretely: a pod whose GetWorkload calls fail for ~11 minutes (API server pressure, etcd slowness — i.e. exactly the degraded cluster where you least want to lose data) will have its shared data populated successfully, but waitForSharedContainerData has already returned context.DeadlineExceeded and the goroutine returned. ContainerCallback only fires on EventTypeAddContainer and there is no re-arm path, so that container never gets an SBOM. For a singleton long-lived workload that is permanent until the next restart.
Suggestions, in order of preference:
- Bound the wait to at least the producer's window (15m), or
- Export the producer's budget as a single shared constant so the two can't drift, or
- Keep 10m but re-arm: on the deadline path, re-check the cache once when data eventually shows up (e.g. via the remove/add event) rather than dropping the container.
Whatever the number ends up being, please add a comment tying it to setSharedWatchedContainerData's budget, since that is the constraint that actually matters.
| } | ||
| s.processContainerWithMetadata(notif, mounts, imageStatus, sharedData.ImageTag, sharedData.ImageID) | ||
| // Wait for shared data off the single-worker pool so a short-lived container whose data never arrives can't head-of-line-block SBOM generation node-wide (#850); only real work is submitted to the pool. | ||
| go func() { |
There was a problem hiding this comment.
Blocking — unbounded goroutines, none of which can be cancelled, and most of which are provably futile.
Two facts that together make this worse than it looks:
-
s.ctxiscontext.Background(). It comes fromCreateSbomManager(ctx, ...)atcmd/main.go:430, and thatctxiscontext.Background()fromcmd/main.go:78— never wrapped inWithCancel, never cancelled on theSIGTERMpath atcmd/main.go:524. Socontext.WithTimeout(s.ctx, maxWaitForSharedContainerData)is not "bounded by shutdown or 10m", it is always a flat 10-minute timer. -
As the PR description itself notes, shared data is deleted on the container-remove event (
containercallback.go,EventTypeRemoveContainer→DeleteSharedContainerData). So for any container that exits before its data is populated, this wait cannot succeed — it will always burn the entire 10 minutes and then log an error.
That is precisely the #850 population. A node with a CronJob firing every 10s accumulates ~600 live goroutines, each retaining notif (*containercollection.Container), mounts, and an ImageStatusResponse fetched with Verbose: true — that response carries the full image config in its Info map, routinely tens of KB. Nothing bounds this; maxPendingScans doesn't apply on this path.
I don't think "cancel on container-remove" is a follow-up here — it's what keeps the fix from trading one unbounded resource for another, and the signal is already in hand: ContainerCallback receives EventTypeRemoveContainer and currently early-returns at the top. A map[containerID]context.CancelFunc guarded by a mutex, populated here and drained in the remove branch, is roughly 15 lines and turns the worst case from 10 minutes into the container's actual lifetime.
At minimum, please add a concurrency bound (a buffered-channel semaphore around the goroutine) so a burst can't fan out without limit.
Separately, and much smaller: s.pool.Submit at the end has no s.ctx.Done() check, so work is still enqueued during shutdown. That's moot while s.ctx is Background(), but it will matter the moment someone makes it cancellable.
|
|
||
| assert.Error(t, err) | ||
| assert.Nil(t, data) | ||
| assert.Less(t, elapsed, 5*time.Second, "wait must be bounded by the context deadline, not backoff's 15m default") |
There was a problem hiding this comment.
Blocking — the tests don't guard the regression the PR is about, and this one fails badly when it does fail.
Three separate points:
a) The assertion doesn't prove why the wait returned. assert.Error + assert.Less(elapsed, 5s) is also satisfied if the backoff returns Stop, or if the operation returns a *backoff.PermanentError, or if the mock is misconfigured — none of which are the behavior under test. Add assert.ErrorIs(t, err, context.DeadlineExceeded) so the test actually pins "the context deadline is what stopped it."
b) The 5s bound is a 25x slack on a 200ms deadline, and the failure mode is ugly. If the regression comes back (ctx → context.Background()), this test does not fail in 5 seconds — elapsed is only measured after waitForSharedContainerData returns, so the call sits there for backoff v5's full 15m DefaultMaxElapsedTime, blows past go test's 10-minute default panic timeout, and takes down the whole package's test binary with a stack dump instead of producing a readable assertion failure. Please run the wait in a goroutine and select on time.After(2*time.Second) so the regression reports as a clean failure, and tighten the bound while you're there.
c) The actual fix is untested. Both tests exercise waitForSharedContainerData in isolation, i.e. the context plumbing. Neither asserts the behavioral claim in the PR title — that the wait no longer occupies the single pool worker. Delete the go func(){...} wrapper in ContainerCallback and call waitForSharedContainerData(ctx, ...) inline: both tests still pass, and the node-wide stall is fully restored.
ContainerCallback is awkward to test directly (it needs /proc and a live CRI socket), so the cheap seam is to extract the goroutine body into something like func (s *SbomManager) awaitAndSubmit(notif, mounts, imageStatus) and assert that (i) it returns to the caller without blocking and (ii) with container A's data never arriving and container B's already present, B's processContainerWithMetadata runs promptly. That's the regression guard #850 needs.
| defer cancel() | ||
| sharedData, err := s.waitForSharedContainerData(ctx, notif.Container.Runtime.ContainerID) | ||
| if err != nil { | ||
| logger.L().Ctx(s.ctx).Error("SbomManager - container not found in shared data", |
There was a problem hiding this comment.
Nit, non-blocking. The PR description says "the benign container not found in shared data log (#848) is unchanged" — it did change here: it gained .Ctx(s.ctx) and three extra fields. The added fields are a clear improvement and the .Ctx() is consistent with the rest of the file (and currently a no-op, since s.ctx carries no span).
The severity is the part worth reconsidering. Post-PR this fires once for every short-lived container that exits before its data lands — a benign, expected outcome that #848 already tracks as noise — and it fires at Error. Consider demoting to Warning, or to Debug specifically when errors.Is(err, context.DeadlineExceeded), keeping Error for genuinely unexpected failures. Either way the description should be corrected.
|
Thanks for the thorough review . Pushed a commit addressing them:
Nit (log severity). Corrected — the log is now Debug when the cause is context.DeadlineExceeded/Canceled (the benign #848 outcome for a container that exits before its data lands) inely unexpected failures. You're also right the PR description was wrong that the log was "unchanged"; I've updated the description. go test -race ./pkg/sbommanager/v1/ passes. |
2 participants
Overview
Current behavior: A single short-lived container can silently halt SBOM generation node-wide. SbomManager runs a single-worker pool, and the shared-container-data wait executed on that worker via backoff.Retry(context.Background(), …) with no timeout — running to backoff v5's 15m DefaultMaxElapsedTime on a context that is never cancelled. Since shared data is deleted on the container-remove event, a container that exits before its data is populated holds the sole worker for the full window, head-of-line-blocking SBOM generation for every other container on the node.
Future behavior: The shared-data wait is decoupled from the single-worker pool. ContainerCallback now performs the wait in a per-container goroutine — bounded by a timeout and cancelled early on the container-remove event — and submits only the actual SBOM work to the pool once metadata is available. A stuck/short-lived container can no longer occupy the worker (so other containers' SBOMs proceed unblocked) nor park a goroutine past its own lifetime.
Additional Information
How to Test
Related issues/PRs:
Checklist before requesting a review
Summary by CodeRabbit