fix(bpf): cleanup lpm map when no paths are configured - #1326
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1326 +/- ##
==========================================
- Coverage 35.42% 34.78% -0.64%
==========================================
Files 22 22
Lines 3241 3300 +59
Branches 3241 3300 +59
==========================================
Hits 1148 1148
- Misses 2088 2147 +59
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthrough
ChangesPath prefix reload management
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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)
fact/src/bpf/mod.rs (2)
135-206: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd unit tests for the new reload/cleanup logic.
Per the PR objectives, Codecov flagged 0% patch coverage on these changed lines.
cleanup_lpm_map,take_path_prefix, and the reload path inload_pathsimplement the core fix for issue#1322and would benefit from tests covering: empty-path cleanup, partial-set reload (removed prefixes actually removed), and insert/remove failure handling.🤖 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 `@fact/src/bpf/mod.rs` around lines 135 - 206, Add unit tests covering the reload and cleanup behavior in cleanup_lpm_map and load_paths: verify empty configuration detaches programs, clears prefixes, and resets the globset; verify partial reload removes prefixes absent from the new configuration; and exercise both insertion and removal failure handling. Also cover take_path_prefix for successful extraction and invalid or missing map cases, using test fixtures or mocks consistent with the existing BPF test infrastructure.
135-146: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
take_path_prefixpanics instead of returning aResult, unlike its siblings.
take_inode_map,take_metrics, andtake_ringbufferall returnanyhow::Result<T>and usebail!when a map is missing/mistyped.take_path_prefixinstead usesunreachable!(), which will abort the process on any mismatch between the compiled BPF object and this code. Given this PR's stated goal is to reduce fact crashes, panicking here (rather than propagating aResultup throughBpf::new) is an inconsistent and avoidable crash path.♻️ Suggested refactor for consistency
- fn take_path_prefix( - obj: &mut Ebpf, - ) -> LpmTrie<MapData, [c_char; LPM_SIZE_MAX as usize], c_char> { - let Some(paths_lpm_prefix) = obj.take_map("path_prefix") else { - unreachable!("path_prefix map not found"); - }; - - match paths_lpm_prefix.try_into() { - Ok(map) => map, - Err(_) => unreachable!("path_prefix map is not LpmTrie"), - } - } + fn take_path_prefix( + obj: &mut Ebpf, + ) -> anyhow::Result<LpmTrie<MapData, [c_char; LPM_SIZE_MAX as usize], c_char>> { + let Some(paths_lpm_prefix) = obj.take_map("path_prefix") else { + bail!("path_prefix map not found"); + }; + Ok(paths_lpm_prefix.try_into()?) + }and adjust the call site (
let paths_lpm_map = Bpf::take_path_prefix(&mut obj)?;).🤖 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 `@fact/src/bpf/mod.rs` around lines 135 - 146, Update take_path_prefix to return anyhow::Result<LpmTrie<...>> like take_inode_map, take_metrics, and take_ringbuffer; replace both unreachable! paths with bail! errors. Propagate the result through Bpf::new by calling Bpf::take_path_prefix with ?, preserving successful map extraction while avoiding panics for missing or mistyped maps.
🤖 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 `@fact/src/bpf/mod.rs`:
- Around line 156-162: The cleanup_lpm_map method must retain any path prefix
whose paths_lpm_map.remove operation fails, so it can be retried later. Update
the surrounding self.paths update logic to preserve failed removals instead of
unconditionally replacing tracking with new_paths, while continuing to remove
successfully deleted prefixes.
- Around line 177-203: Make prefix reload in the paths update flow keep
self.paths consistent with paths_lpm_map when any iteration or globset build
fails. Update the logic around the new_paths construction, paths_lpm_map.insert,
and final self.paths assignment to either record each successfully inserted
prefix incrementally or remove all prefixes inserted during the failed reload
before returning the error; preserve successful reload behavior and ensure
cleanup_lpm_map cannot miss partially inserted entries.
- Around line 187-191: Update the glob construction in the builder loop to
propagate the contextualized invalid-pattern error with the function’s existing
anyhow result flow instead of calling unwrap. Preserve the current “invalid
glob” context and let the surrounding fallible operations continue using ?.
---
Nitpick comments:
In `@fact/src/bpf/mod.rs`:
- Around line 135-206: Add unit tests covering the reload and cleanup behavior
in cleanup_lpm_map and load_paths: verify empty configuration detaches programs,
clears prefixes, and resets the globset; verify partial reload removes prefixes
absent from the new configuration; and exercise both insertion and removal
failure handling. Also cover take_path_prefix for successful extraction and
invalid or missing map cases, using test fixtures or mocks consistent with the
existing BPF test infrastructure.
- Around line 135-146: Update take_path_prefix to return
anyhow::Result<LpmTrie<...>> like take_inode_map, take_metrics, and
take_ringbuffer; replace both unreachable! paths with bail! errors. Propagate
the result through Bpf::new by calling Bpf::take_path_prefix with ?, preserving
successful map extraction while avoiding panics for missing or mistyped maps.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 2b139f62-14b0-400c-87bd-9fb8ab688c1a
📒 Files selected for processing (1)
fact/src/bpf/mod.rs
7534e09 to
ee2012c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@fact/src/bpf/mod.rs`:
- Around line 132-144: Update take_path_prefix to return
anyhow::Result<LpmTrie<MapData, [c_char; LPM_SIZE_MAX as usize], c_char>>,
replacing both unreachable! paths with descriptive bail! errors for a missing or
mismatched path_prefix map. Update its call site to propagate the Result using
the surrounding error-handling convention, matching take_inode_map,
take_metrics, and take_ringbuffer.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 07f131ad-7416-421b-99da-565a1032af27
📒 Files selected for processing (2)
fact-ebpf/src/lib.rsfact/src/bpf/mod.rs
ee2012c to
0ae62ac
Compare
This was causing a bug where the map was not being cleaned up, potentially leading to it being completely full and crashing fact.
0ae62ac to
ed3aee8
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
fact/src/bpf/mod.rs (1)
153-180: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider adding test coverage for the new cleanup/reload logic.
The PR's Codecov report flagged 0% patch coverage across the changed lines in this file, including
cleanup_lpm_mapand the reworkedload_paths. Since this is the exact logic that fixes the LPM-map-fills-up crash (issue#1322), a regression here would silently reintroduce the original bug. A unit/integration test exercising a reload cycle (insert prefixes → reload with a subset → assert stale prefixes are removed from the trie, plus the empty-config wipe case) would meaningfully de-risk this path.Also applies to: 189-211
🤖 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 `@fact/src/bpf/mod.rs` around lines 153 - 180, Add test coverage for the reload behavior implemented by cleanup_lpm_map and load_paths: insert multiple prefixes, reload with a subset, and assert removed prefixes no longer exist in the LPM trie; also verify that reloading with an empty configuration clears the map. Use the existing test setup and public behavior rather than changing production logic.
🤖 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 `@fact/src/bpf/mod.rs`:
- Around line 153-180: Add test coverage for the reload behavior implemented by
cleanup_lpm_map and load_paths: insert multiple prefixes, reload with a subset,
and assert removed prefixes no longer exist in the LPM trie; also verify that
reloading with an empty configuration clears the map. Use the existing test
setup and public behavior rather than changing production logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: f0abe901-71c7-49c8-aa4f-e37adb780e5a
📒 Files selected for processing (2)
fact-ebpf/src/lib.rsfact/src/bpf/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- fact-ebpf/src/lib.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
fact/src/bpf/mod.rs (1)
177-211: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd regression coverage for incremental and empty-path cleanup.
Codecov reports no coverage for the reload logic. Add tests that preload prefixes, reload with a changed set, then reload with an empty set; assert stale trie keys are removed while retained/new keys remain.
🤖 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 `@fact/src/bpf/mod.rs` around lines 177 - 211, The load_paths reload behavior in load_paths lacks regression coverage for incremental updates and empty-path cleanup. Add tests that preload prefixes, reload with a changed configuration, and verify stale LPM trie keys are removed while retained and new keys remain; then reload with an empty configuration and verify all keys are cleaned up and programs are detached. Use the existing paths_lpm_map and paths_config test setup and target load_paths directly.
🤖 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 `@fact/src/bpf/mod.rs`:
- Around line 177-211: The load_paths reload behavior in load_paths lacks
regression coverage for incremental updates and empty-path cleanup. Add tests
that preload prefixes, reload with a changed configuration, and verify stale LPM
trie keys are removed while retained and new keys remain; then reload with an
empty configuration and verify all keys are cleaned up and programs are
detached. Use the existing paths_lpm_map and paths_config test setup and target
load_paths directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 108ee8ab-384d-4ae3-8e4a-794a3a9cbefe
📒 Files selected for processing (2)
fact-ebpf/src/lib.rsfact/src/bpf/mod.rs
2 participants
Description
This was causing a bug where the map was not being cleaned up, potentially leading to it being completely full and crashing fact.
Fixes #1322
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
Working on a separate PR that runs all tests on a single fact instance, fact was crashing with a full LPM map and this patch fixes that.
Summary by CodeRabbit