fix(host_scanner): reduce syscalls during scans - #1221
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughHost scanning now records scan durations, logs lifecycle events at info level, skips glob and metadata lookup failures, passes fetched metadata through updates, and avoids redundant inode tracking work. ChangesHost scanner observability and resilient inode tracking
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to This PR makes localized host-scanning performance improvements, and no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1221 +/- ##
==========================================
- Coverage 34.78% 34.52% -0.27%
==========================================
Files 22 22
Lines 3300 3325 +25
Branches 3300 3325 +25
==========================================
Hits 1148 1148
- Misses 2147 2172 +25
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fact/src/host_scanner.rs (1)
205-234: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not retain a userspace inode when kernel insertion fails.
Line 217 inserts into
inode_mapbefore the BPF insertion at Line 221. On a full or failed kernel map insertion, later scans hit Lines 207-213 and never retry BPF tracking, leaving the inode falsely marked as monitored. Insert intoinode_maponly after kernel insertion succeeds, or roll back the userspace entry on every error path.🤖 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/host_scanner.rs` around lines 205 - 234, The inode_map entry must not remain when kernel_inode_map insertion fails. Update the new-inode path around inode_map and kernel_inode_map so the userspace inode is inserted only after the kernel insertion succeeds, or remove it on every error path, while preserving the existing already-tracked path and error reporting.
🤖 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.
Outside diff comments:
In `@fact/src/host_scanner.rs`:
- Around line 205-234: The inode_map entry must not remain when kernel_inode_map
insertion fails. Update the new-inode path around inode_map and kernel_inode_map
so the userspace inode is inserted only after the kernel insertion succeeds, or
remove it on every error path, while preserving the existing already-tracked
path and error reporting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 5edde49b-9885-47e7-a91d-25e6873b308f
📒 Files selected for processing (2)
fact/src/host_scanner.rsfact/src/metrics/host_scanner.rs
|
/retest |
Add a histogram capturing the duration of a host_scanner scan.
Instead of using `PathBuf::is_file`, `PathBuf::is_dir` and `PathBuf::metadata` individually, each of which produce independent calls to `stat` under the hood, use a single call to `PathBuf::metadata` and propagate it down to the places it is needed.
Avoid additional syscalls to insert inodes into the kernel BPF inode map if the userspace already has said inode.
0a1d04e to
9836694
Compare
3 participants
Description
This PR makes some efforts to try and reduce the amount of syscalls that
HostScannerdoes when scanning through the filesystem. This is achieved essentially with two changes:statcalls.The first one is done by replacing individual calls to
PathBuf::is_file,PathBuf::is_dirandPathBuf::metadata, all of which callstatunder the hood, with a singlePathBuf::metadatacall and using it where needed.The second one is done by first checking if the userspace inode map has the found inode and if it is already there, we simply skip inserting it kernel side, since it should've been put there in a previous scan.
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
Reduced number of syscalls
Running fact with the following command:
FACT_LOGLEVEL=info RUST_BACKTRACE=1 cargo srun --bin fact --all-features -- -p '/etc/**/*:/etc/' --inodes-max=2097152 --expose-metrics --scan-interval 30, then usingperf stat -e 'syscalls:sys_enter_statx,syscalls:sys_enter_bpf' -p "$(pgrep fact)" -- sleep 30capturing a single scan. No events generated during the run, no modifications to the/etcdirectory, 25966 inodes tracked for all the scans.Before changes:
After all changes:
Average scan time
Run
factwith the following command in each commit in this PR:FACT_LOGLEVEL=info RUST_BACKTRACE=1 cargo srun --bin fact --all-features -- -p '/etc/**/*:/etc/' --inodes-max=2097152 --expose-metrics --scan-interval 1. No events generated during the run, no modifications to the/etcdirectory, 25966 inodes tracked for all the scans.Average scan time adding scan duration to log and metric: 162.48ms
Average scan time using a single metadata call: 136.44ms
Average scan time skipping re-insertion of inodes in BPF map: 114.19ms
Summary by CodeRabbit
Improvements
Monitoring