ROX-33036: add mount-related operations - #1059
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughMount, unmount, and move-mount hooks now emit events with dedicated metrics. Rust event handling, host scanning, integration-test container setup, and mount tracking tests support these operations. ChangesMount event tracking
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant KernelHooks
participant RingBuffer
participant FileData
participant HostScanner
KernelHooks->>RingBuffer: submit mount-related event
RingBuffer->>FileData: construct Mount, Umount, or MoveMount
FileData->>HostScanner: classify mount-related event
HostScanner->>HostScanner: perform full host scan
Possibly related PRs
Suggested reviewers: 🚥 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 @@
## mauro/refactor/cleanup-file-data-accesors #1059 +/- ##
=============================================================================
- Coverage 35.48% 35.04% -0.45%
=============================================================================
Files 22 22
Lines 3235 3276 +41
Branches 3235 3276 +41
=============================================================================
Hits 1148 1148
- Misses 2082 2123 +41
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
3cc1980 to
b15f103
Compare
b15f103 to
0eb199b
Compare
0eb199b to
5e41f99
Compare
|
/retest |
a0d0d78 to
edc7efa
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fact/src/event/mod.rs (1)
657-687: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
PartialEq for FileDatamissing arms forMount,Umount,MoveMount.Every other variant (including all pre-existing ones) has an explicit equality arm, but the three new mount variants fall through to
_ => false. This means two identicalMount/Umount/MoveMountevents will never compare equal.🐛 Proposed fix
(FileData::AclSet(this), FileData::AclSet(other)) => { this.inner == other.inner && this.acl_type == other.acl_type && this.entries == other.entries } + (FileData::Mount(this), FileData::Mount(other)) => this == other, + (FileData::Umount(this), FileData::Umount(other)) => this == other, + ( + FileData::MoveMount { to: l_to, from: l_from }, + FileData::MoveMount { to: r_to, from: r_from }, + ) => l_to == r_to && l_from == r_from, _ => false,🤖 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/event/mod.rs` around lines 657 - 687, Add explicit equality arms to PartialEq::eq for FileData covering Mount, Umount, and MoveMount, comparing each variant’s corresponding fields consistently with the other variant arms. Keep the fallback _ => false for differing variants.
🧹 Nitpick comments (3)
fact/src/host_scanner.rs (1)
429-439: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftMount events trigger a synchronous full scan per event, with no coalescing for bursts.
handle_mount_eventcallsself.scan()directly and inline in theselect!branch, blocking the task for the scan's duration and running once per mount-related event with no debouncing. The existingscan_trigger/Notifymechanism used for periodic andpaths.changed()scans already coalesces repeated triggers (multiplenotify_one()calls before consumption collapse to a single pending scan), but that path isn't reused here. Under a burst of mount/unmount activity this could serialize many redundant full scans and delay processing of other events in the loop (e.g. introspection queries).Also applies to: 493-497
🤖 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 429 - 439, Update handle_mount_event to signal the existing scan_trigger/Notify mechanism instead of calling self.scan() synchronously. Route mount events through the same coalesced scan path used by periodic and paths.changed() triggers, preserving a single pending scan during event bursts and keeping the select loop responsive.fact/src/event/mod.rs (1)
437-535: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider adding Rust-side test coverage for the new
Mount/Umount/MoveMountvariants.Codecov flags 130 uncovered lines in this file for this PR, and the
PartialEqgap above went unnoticed likely due to this.EventTestData(used by the#[cfg(all(test, feature = "bpf-test"))]constructor) doesn't have variants for the new mount events either.🤖 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/event/mod.rs` around lines 437 - 535, Add Rust-side tests covering FileData::Mount, FileData::Umount, and FileData::MoveMount, including their constructed fields and equality behavior. Extend EventTestData and its bpf-test constructor with corresponding mount-event variants so these cases can be exercised through the existing test path, and ensure the tests cover both source and destination data for MoveMount.tests/test_mount.py (1)
44-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSubprocess calls don't check exit status.
mount/umountfailures (e.g. wrong tmpfs support, misconfigured environment) will pass silently, and the test would then just fail (or worse, pass) based on stale inode state rather than surfacing the real cause.♻️ Proposed fix
- subprocess.run( - ['mount', '-t', 'tmpfs', '-o', 'size=10M', 'tmpfs', monitored_dir] - ) + subprocess.run( + ['mount', '-t', 'tmpfs', '-o', 'size=10M', 'tmpfs', monitored_dir], + check=True, + ) assert_tracked_path(monitored_dir) - subprocess.run(['umount', monitored_dir]) + subprocess.run(['umount', monitored_dir], check=True)🤖 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 `@tests/test_mount.py` around lines 44 - 50, Update the subprocess.run calls in the mount/umount test to enforce successful exit status, so mount or unmount failures raise immediately instead of allowing assertions to use stale inode state. Preserve the existing command arguments and tracking assertions.
🤖 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 `@CHANGELOG.md`:
- Line 9: Update the ROX-33036 changelog entry to hyphenate “mount-related” in
the description of the operations.
In `@fact-ebpf/src/bpf/main.c`:
- Around line 557-603: Update trace_move_mount so events are emitted for every
monitored classification except MONITORED_NOT, matching the sb_mount/sb_umount
handling and allowing parent/path-monitored moves to reach rescan. In the same
function, derive args.parent_inode from to’s actual parent inode rather than
reusing to->dentry->d_inode, and pass that value to is_monitored.
---
Outside diff comments:
In `@fact/src/event/mod.rs`:
- Around line 657-687: Add explicit equality arms to PartialEq::eq for FileData
covering Mount, Umount, and MoveMount, comparing each variant’s corresponding
fields consistently with the other variant arms. Keep the fallback _ => false
for differing variants.
---
Nitpick comments:
In `@fact/src/event/mod.rs`:
- Around line 437-535: Add Rust-side tests covering FileData::Mount,
FileData::Umount, and FileData::MoveMount, including their constructed fields
and equality behavior. Extend EventTestData and its bpf-test constructor with
corresponding mount-event variants so these cases can be exercised through the
existing test path, and ensure the tests cover both source and destination data
for MoveMount.
In `@fact/src/host_scanner.rs`:
- Around line 429-439: Update handle_mount_event to signal the existing
scan_trigger/Notify mechanism instead of calling self.scan() synchronously.
Route mount events through the same coalesced scan path used by periodic and
paths.changed() triggers, preserving a single pending scan during event bursts
and keeping the select loop responsive.
In `@tests/test_mount.py`:
- Around line 44-50: Update the subprocess.run calls in the mount/umount test to
enforce successful exit status, so mount or unmount failures raise immediately
instead of allowing assertions to use stale inode state. Preserve the existing
command arguments and tracking assertions.
🪄 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: 05fc42f0-315c-46df-9f9b-5b00f13b4fd4
📒 Files selected for processing (12)
CHANGELOG.mdfact-ebpf/src/bpf/bound_path.hfact-ebpf/src/bpf/events.hfact-ebpf/src/bpf/main.cfact-ebpf/src/bpf/types.hfact-ebpf/src/lib.rsfact/src/event/mod.rsfact/src/host_scanner.rsfact/src/metrics/kernel_metrics.rstests/conftest.pytests/test_config_hotreload.pytests/test_mount.py
Molter73
changed the title
4a5ee00 to
72aa4a2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/test_mount.py`:
- Line 17: Update the requests.get call for FACT_INTROSPECTION_INODES in the
test to include a finite timeout, using the project’s established timeout
convention if available, so the test fails promptly when the local service is
unavailable or unresponsive.
- Around line 25-35: Update the retry loop around get_inodes so it refreshes res
by calling get_inodes() at the start of each attempt, rather than reusing the
initial snapshot. Preserve the existing assertion, KeyError logging, and retry
delay behavior.
🪄 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: 37bf14d9-5342-4cbd-b40a-6d0974b27ddf
📒 Files selected for processing (12)
CHANGELOG.mdfact-ebpf/src/bpf/bound_path.hfact-ebpf/src/bpf/events.hfact-ebpf/src/bpf/main.cfact-ebpf/src/bpf/types.hfact-ebpf/src/lib.rsfact/src/event/mod.rsfact/src/host_scanner.rsfact/src/metrics/kernel_metrics.rstests/conftest.pytests/test_config_hotreload.pytests/test_mount.py
🚧 Files skipped from review as they are similar to previous changes (9)
- CHANGELOG.md
- fact/src/metrics/kernel_metrics.rs
- fact-ebpf/src/bpf/bound_path.h
- fact-ebpf/src/bpf/events.h
- fact/src/host_scanner.rs
- fact-ebpf/src/bpf/types.h
- fact-ebpf/src/bpf/main.c
- tests/test_config_hotreload.py
- fact/src/event/mod.rs
|
|
||
| def get_inodes() -> dict[str, str]: | ||
| FACT_INTROSPECTION_INODES = 'http://127.0.0.1:9000/inodes' | ||
| res = requests.get(FACT_INTROSPECTION_INODES) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Set a bounded introspection timeout.
If the local service is unavailable or wedged, this test can block indefinitely rather than fail.
Proposed fix
- res = requests.get(FACT_INTROSPECTION_INODES)
+ res = requests.get(FACT_INTROSPECTION_INODES, timeout=5)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| res = requests.get(FACT_INTROSPECTION_INODES) | |
| res = requests.get(FACT_INTROSPECTION_INODES, timeout=5) |
🤖 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 `@tests/test_mount.py` at line 17, Update the requests.get call for
FACT_INTROSPECTION_INODES in the test to include a finite timeout, using the
project’s established timeout convention if available, so the test fails
promptly when the local service is unavailable or unresponsive.
Source: Linters/SAST tools
| res = get_inodes() | ||
|
|
||
| for _ in range(3): | ||
| try: | ||
| assert res[inode] == monitored_dir | ||
| return | ||
| except KeyError as e: | ||
| print(f'Failed to find inode: {e}') | ||
| print(f'{json.dumps(res)}') | ||
| sleep(1) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Refresh inode data on every retry.
res is read once before the loop, so retries repeatedly inspect the same snapshot. A delayed mount scan can never make this assertion pass.
Proposed fix
- res = get_inodes()
-
for _ in range(3):
try:
+ res = get_inodes()
assert res[inode] == monitored_dir
return📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| res = get_inodes() | |
| for _ in range(3): | |
| try: | |
| assert res[inode] == monitored_dir | |
| return | |
| except KeyError as e: | |
| print(f'Failed to find inode: {e}') | |
| print(f'{json.dumps(res)}') | |
| sleep(1) | |
| for _ in range(3): | |
| try: | |
| res = get_inodes() | |
| assert res[inode] == monitored_dir | |
| return | |
| except KeyError as e: | |
| print(f'Failed to find inode: {e}') | |
| print(f'{json.dumps(res)}') | |
| sleep(1) |
🧰 Tools
🪛 ast-grep (0.45.0)
[info] 32-32: use jsonify instead of json.dumps for JSON output
Context: json.dumps(res)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 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 `@tests/test_mount.py` around lines 25 - 35, Update the retry loop around
get_inodes so it refreshes res by calling get_inodes() at the start of each
attempt, rather than reusing the initial snapshot. Preserve the existing
assertion, KeyError logging, and retry delay behavior.
This is a small refactor done ahead of #1059, cleaning up getters in the `FileData` type by using better pattern matching. The `Rename` variant is also streamlined by using a struct variant instead of a tuple holding `RenameFileData`.
This is a small refactor done ahead of #1059, cleaning up getters in the `FileData` type by using better pattern matching. The `Rename` variant is also streamlined by using a struct variant instead of a tuple holding `RenameFileData`.
This was originally going to be about adding just `lsm/sb_mount`, however while adding this hook it became pretty clear we needed `lsm/sb_umount` and `lsm/move_mount` for a comprehensive implementation and it really didn't add too much code, so they are all added in. These operations are not currently intended to be forwarded via gRPC, they only trigger inode tracking related behavior (scans on new/moved mounts, inode map cleanups on moved/unmounted directories). The move mount operation shares quite a bit of similarities with rename, so there is a bit of refactoring mixed in so they can be reused while keeping the code clean. TODO: add integration tests.
Add a basic test for checking mount operations are properly tracked. This test works by checking the monitored directory is tracked using the inodes introspection endpoint, then a tmpfs is mounted on top of this directory and we validate we see the new inode, then we unmount and check the introspection endpoint one last time.
72aa4a2 to
3d17ab3
Compare
2 participants
Description
This was originally going to be about adding just
lsm/sb_mount, however while adding this hook it became pretty clear we neededlsm/sb_umountandlsm/move_mountfor a comprehensive implementation and it really didn't add too much code, so they are all added in.These operations are not currently intended to be forwarded via gRPC, they only trigger inode tracking related behavior (scans on new/moved mounts, inode map cleanups on moved/unmounted directories).
The move mount operation shares quite a bit of similarities with rename, so there is a bit of refactoring mixed in so they can be reused while keeping the code clean.
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
Added integration tests.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation