fix: clean up expired/deleted containerprofile time series database entries#338
fix: clean up expired/deleted containerprofile time series database entries#338matthyx wants to merge 1 commit into
Conversation
|
Warning Review limit reached
More reviews will be available in 38 minutes and 50 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdds ChangesContainerProfile Time-Series Cleanup and Expired-Status Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
matthyx
force-pushed
the
fix/expired-ts-profiles-zombies
branch
from
9e6cf82 to
ab45bcf
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
pkg/registry/file/sqlite.go (1)
289-291: ⚡ Quick winCentralize container-profile kind normalization/matching in one helper.
The alias logic is now split across
pkg/registry/file/sqlite.go,pkg/registry/file/storage.go, andpkg/registry/file/utils.go. Please extract shared helpers (for example,NormalizeContainerProfileKind+IsContainerProfileKind) and reuse them to avoid drift.🤖 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/registry/file/sqlite.go` around lines 289 - 291, The container-profile kind normalization logic (checking for "containerprofiles" and "containerprofile-merged" aliases) is duplicated across multiple files: sqlite.go, storage.go, and utils.go, creating maintenance risk and potential drift. Extract shared helper functions such as NormalizeContainerProfileKind and IsContainerProfileKind in a common location (likely utils.go), then replace all instances of the kind alias logic throughout these files with calls to these centralized helpers. This ensures consistent behavior and a single source of truth for kind normalization.
🤖 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 `@pkg/registry/file/containerprofile_processor.go`:
- Around line 719-727: The early return with skipFurtherProcessing=true in the
ContainerProfileProcessor.updateProfileStatus function at the expired full-path
check prevents remaining series from being processed and added to the processed
set, which causes deleteProcessedTimeSeries to miss cleaning up TS profile
objects. Instead of returning true to skip further processing, return false to
allow the remaining series in the loop to complete their processing, ensuring
all series are properly tracked in the processed set so they can be fully
deleted later.
In `@pkg/registry/file/dynamicpathdetector/analyzer.go`:
- Around line 124-129: The fast-reject logic in the analyzer function does not
properly handle the root-protected prefix "/". When "/" is a protected prefix,
paths like "/etc" have topDir("/etc") = "/etc", which fails the pinAncestors
lookup and returns false prematurely, bypassing the descendant check that should
succeed. Ensure that when "/" is included as a protected prefix, it is properly
represented in the ua.pinAncestors map so that the fast-reject check does not
incorrectly exclude valid paths that should be protected as descendants of the
root. This may involve ensuring "/" is added to pinAncestors during
initialization, or handling the root case specially in the pinAncestors lookup
logic.
In `@pkg/registry/file/storage.go`:
- Around line 376-382: The StorageImpl.delete method currently logs the error
from DeleteTimeSeriesContainerEntries but continues execution and returns
success, leaving orphaned data in the database. When the error check for
DeleteTimeSeriesContainerEntries fails in the containerprofile cleanup block,
return the error immediately instead of just logging it. This ensures callers
are aware the operation failed and can implement retry logic, preventing the API
from reporting a successful deletion when time-series cleanup is incomplete.
In `@pkg/registry/file/utils.go`:
- Around line 59-64: The error from DeleteTimeSeriesContainerEntries in the
containerprofile cleanup logic is being only logged without being propagated to
the caller, which prevents structured error handling and retry mechanisms.
Instead of just logging the error, return it from the deleteMetadata method so
that the caller can appropriately handle the failure, implement retries, or
signal that the cleanup operation did not fully succeed. This ensures orphaned
time-series rows are not silently left behind without a path to recovery.
---
Nitpick comments:
In `@pkg/registry/file/sqlite.go`:
- Around line 289-291: The container-profile kind normalization logic (checking
for "containerprofiles" and "containerprofile-merged" aliases) is duplicated
across multiple files: sqlite.go, storage.go, and utils.go, creating maintenance
risk and potential drift. Extract shared helper functions such as
NormalizeContainerProfileKind and IsContainerProfileKind in a common location
(likely utils.go), then replace all instances of the kind alias logic throughout
these files with calls to these centralized helpers. This ensures consistent
behavior and a single source of truth for kind normalization.
🪄 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
Run ID: ed35a624-9a09-46c1-80d9-b99033190c2e
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
go.modmain.gopkg/apiserver/apiserver.gopkg/cmd/server/start.gopkg/config/config.gopkg/registry/file/containerprofile_processor.gopkg/registry/file/containerprofile_processor_test.gopkg/registry/file/dynamicpathdetector/analyzer.gopkg/registry/file/dynamicpathdetector/protection.gopkg/registry/file/dynamicpathdetector/protection_test.gopkg/registry/file/dynamicpathdetector/types.gopkg/registry/file/openprotection.gopkg/registry/file/openprotection_test.gopkg/registry/file/sqlite.gopkg/registry/file/storage.gopkg/registry/file/utils.go
matthyx
force-pushed
the
fix/expired-ts-profiles-zombies
branch
from
ab45bcf to
1cec3df
Compare
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent “zombie” SQLite time_series rows by cascading deletions when container profiles are removed (REST delete / cleanup walks) and by tightening consolidation behavior for expired time-series profiles.
Changes:
- Cascade-delete
time_seriesrows when container profile metadata/payload is deleted in storage and cleanup flows. - Normalize kind handling in
DeleteTimeSeriesContainerEntriesto match howtime_series.kindis stored. - Update expired time-series consolidation to finalize status and clear consolidated series; add a unit test for the expired partial path.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/registry/file/utils.go | Cleanup-walk metadata deletion now also deletes related time_series rows for container profiles. |
| pkg/registry/file/storage.go | REST delete path now also deletes related time_series rows for container profiles. |
| pkg/registry/file/sqlite.go | Adjusts DeleteTimeSeriesContainerEntries kind normalization before deleting time_series rows. |
| pkg/registry/file/containerprofile_storage.go | Introduces a plural kind constant for container profiles. |
| pkg/registry/file/containerprofile_processor.go | Refactors expired time-series status handling and adds deletion behavior in the completed/full path. |
| pkg/registry/file/containerprofile_processor_test.go | Fixes connection handling in an existing test and adds a test for expired status finalization (partial). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
matthyx
force-pushed
the
fix/expired-ts-profiles-zombies
branch
from
1cec3df to
5969768
Compare
matthyx
force-pushed
the
fix/expired-ts-profiles-zombies
branch
from
5969768 to
242c325
Compare
|
Summary:
|
…ntries Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
matthyx
force-pushed
the
fix/expired-ts-profiles-zombies
branch
from
242c325 to
da6dde6
Compare
|
Summary:
|
This PR fixes a gap in the consolidation pipeline where expired time series profiles were not deleted from SQLite, leaving zombie records. It also purges time series records when container profiles are deleted via the REST API or cleanup walked paths.
Summary by CodeRabbit
Release Notes
Bug Fixes
Improvements
Tests