fix: honor explicit disk includes#221
Merged
Merged
Conversation
Make disk_includes a hard override for small and normally filtered mounts. Deduplicate bind mounts by default while preserving explicitly included bind mounts, and document the behavior.
📝 WalkthroughWalkthroughRefactors agent disk monitoring: adds explicit-include tracking, moves partition filtering and usage collection into a builder that enforces min-size only for non-explicit mounts, and deduplicates bind mounts while preserving explicitly included mounts; updates README and CLI help text and adds unit tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/agent/disk_utils_test.go (1)
15-109: Consider table-driving these cases and adding include-vs-exclude precedence.These tests all follow the same arrange/act path, and the new hard-override behavior depends on include-before-exclude ordering, which is still untested. A table-driven test would cut the duplication and make that regression case cheap to add.
As per coding guidelines,
**/*_test.go: Prefer table-driven tests for new backend logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/agent/disk_utils_test.go` around lines 15 - 109, The three near-duplicate tests (TestBuildDiskReportEntries_ExplicitIncludeBypassesSizeFilter, TestBuildDiskReportEntries_DedupesNonExplicitBindMounts, TestBuildDiskReportEntries_KeepsExplicitlyIncludedBindMounts) should be converted into a single table-driven test that iterates cases calling agent.buildDiskReportEntries; for each case include fields: name, AgentConfig (DiskIncludes/DiskExcludes), partitions ([]disk.PartitionStat), mocked usage func, expected entry count, expected mountpoints order, and expected ExplicitInclude booleans; add an explicit case to assert include-vs-exclude precedence (e.g., include contains a bind mount while excludes would normally drop it) and assert the include wins; implement the table loop to call New(...) and buildDiskReportEntries and use the same assertions currently used (Len, Equal, True) driven by the table rows.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/agent/disk_utils.go`:
- Around line 92-96: The helper method shouldIncludeDisk on type Agent is dead
code (no callers) and triggers lint failures; either remove this unused wrapper
or reconnect callers to use it instead of directly calling diskFilterDecision.
Locate the method shouldIncludeDisk and either delete it (and run tests/lint) or
update the code paths that previously inspected disk inclusion to call
shouldIncludeDisk (which wraps diskFilterDecision), ensuring you preserve the
behavior of diskFilterDecision(mountpoint, device, fstype) and its returned
include value.
- Around line 183-185: The current diskReportSignature in
diskReportSignature(entry diskReportEntry) uses volatile fields Used and Free,
causing racey signatures; change it to a stable key by removing Used and Free
and only using stable identifiers such as entry.Partition.Device,
entry.Partition.Fstype and a stable size like entry.Usage.Total (e.g. format
"%s|%s|%d"), so the signature is stable across small writes and bind-mount
dedupe works reliably.
---
Nitpick comments:
In `@internal/agent/disk_utils_test.go`:
- Around line 15-109: The three near-duplicate tests
(TestBuildDiskReportEntries_ExplicitIncludeBypassesSizeFilter,
TestBuildDiskReportEntries_DedupesNonExplicitBindMounts,
TestBuildDiskReportEntries_KeepsExplicitlyIncludedBindMounts) should be
converted into a single table-driven test that iterates cases calling
agent.buildDiskReportEntries; for each case include fields: name, AgentConfig
(DiskIncludes/DiskExcludes), partitions ([]disk.PartitionStat), mocked usage
func, expected entry count, expected mountpoints order, and expected
ExplicitInclude booleans; add an explicit case to assert include-vs-exclude
precedence (e.g., include contains a bind mount while excludes would normally
drop it) and assert the include wins; implement the table loop to call New(...)
and buildDiskReportEntries and use the same assertions currently used (Len,
Equal, True) driven by the table rows.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 37518254-ef29-451d-ac3e-70225b4e3b2b
📒 Files selected for processing (5)
README.mdcmd/netronome/main.gointernal/agent/disk_utils.gointernal/agent/disk_utils_test.gointernal/agent/hardware.go
Remove the unused disk include wrapper and use a stable dedupe signature based on device, filesystem, and total size. Refactor the disk selection tests into a table-driven form and add coverage for include-over-exclude precedence.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/agent/disk_utils.go (1)
136-138: Minor: avoid recomputing signature twice per entry.
diskReportSignature(entry)is called twice in the loop body. Consider caching it in a local variable.♻️ Suggested optimization
for i, entry := range entries { - grouped[diskReportSignature(entry)] = append(grouped[diskReportSignature(entry)], i) + sig := diskReportSignature(entry) + grouped[sig] = append(grouped[sig], i) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/agent/disk_utils.go` around lines 136 - 138, In the loop that builds grouped from entries, avoid calling diskReportSignature(entry) twice by caching the result in a local variable (e.g., sig := diskReportSignature(entry)) and then use grouped[sig] = append(grouped[sig], i); update the for i, entry := range entries loop in the function where grouped and diskReportSignature are used to reference the cached sig instead of recomputing.internal/agent/disk_utils_test.go (1)
142-146: Consider guarding the index-based loop against out-of-bounds access.If
assert.Lenfails, the test continues and the loop at line 143 could panic when accessingtt.expectedMountpoints[i]if entries has more items than expected. Usingrequire.Lenwould stop the test immediately on mismatch.♻️ Suggested fix using require
+import ( + "testing" + + "github.com/shirou/gopsutil/v4/disk" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/autobrr/netronome/internal/config" +)- assert.Len(t, entries, len(tt.expectedMountpoints)) + require.Len(t, entries, len(tt.expectedMountpoints))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/agent/disk_utils_test.go` around lines 142 - 146, The test currently uses assert.Len(t, entries, len(tt.expectedMountpoints)) then iterates over entries comparing tt.expectedMountpoints[i] and tt.expectedExplicitIncludes[i], which can panic if the length assertion fails; replace assert.Len with require.Len to abort the test on mismatch and prevent the loop from running, and keep the index-based comparisons inside the existing for i, entry := range entries loop (references: variables entries, tt.expectedMountpoints, tt.expectedExplicitIncludes and the loop that does assert.Equal on entry.Partition.Mountpoint and entry.ExplicitInclude).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/agent/disk_utils_test.go`:
- Around line 142-146: The test currently uses assert.Len(t, entries,
len(tt.expectedMountpoints)) then iterates over entries comparing
tt.expectedMountpoints[i] and tt.expectedExplicitIncludes[i], which can panic if
the length assertion fails; replace assert.Len with require.Len to abort the
test on mismatch and prevent the loop from running, and keep the index-based
comparisons inside the existing for i, entry := range entries loop (references:
variables entries, tt.expectedMountpoints, tt.expectedExplicitIncludes and the
loop that does assert.Equal on entry.Partition.Mountpoint and
entry.ExplicitInclude).
In `@internal/agent/disk_utils.go`:
- Around line 136-138: In the loop that builds grouped from entries, avoid
calling diskReportSignature(entry) twice by caching the result in a local
variable (e.g., sig := diskReportSignature(entry)) and then use grouped[sig] =
append(grouped[sig], i); update the for i, entry := range entries loop in the
function where grouped and diskReportSignature are used to reference the cached
sig instead of recomputing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a22d4612-13bf-467b-9684-adbc2710c015
📒 Files selected for processing (2)
internal/agent/disk_utils.gointernal/agent/disk_utils_test.go
Cache the dedupe signature within the grouping loop and use require.Len in the table-driven disk tests so mismatched lengths abort before index-based assertions run.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/agent/disk_utils_test.go (1)
143-147: Add a length guard forexpectedExplicitIncludesbefore index-based asserts.You already guard mountpoint length; add the same for explicit flags so bad table rows fail with a clear assertion instead of panicking.
Proposed change
entries := agent.buildDiskReportEntries(tt.partitions, tt.usageFn) require.Len(t, entries, len(tt.expectedMountpoints)) + require.Len(t, tt.expectedExplicitIncludes, len(entries)) for i, entry := range entries { assert.Equal(t, tt.expectedMountpoints[i], entry.Partition.Mountpoint) assert.Equal(t, tt.expectedExplicitIncludes[i], entry.ExplicitInclude) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/agent/disk_utils_test.go` around lines 143 - 147, The test currently indexes tt.expectedExplicitIncludes inside the for loop without verifying its length, which can panic; add a length guard such as require.Len(t, tt.expectedExplicitIncludes, len(entries)) (or require.Len(t, tt.expectedExplicitIncludes, len(tt.expectedMountpoints))) immediately before the for i, entry := range entries loop so the subsequent assert.Equal(t, tt.expectedExplicitIncludes[i], entry.ExplicitInclude) is safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/agent/disk_utils_test.go`:
- Around line 143-147: The test currently indexes tt.expectedExplicitIncludes
inside the for loop without verifying its length, which can panic; add a length
guard such as require.Len(t, tt.expectedExplicitIncludes, len(entries)) (or
require.Len(t, tt.expectedExplicitIncludes, len(tt.expectedMountpoints)))
immediately before the for i, entry := range entries loop so the subsequent
assert.Equal(t, tt.expectedExplicitIncludes[i], entry.ExplicitInclude) is safe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb7551a4-0959-437a-8722-dd7d92676bfc
📒 Files selected for processing (2)
internal/agent/disk_utils.gointernal/agent/disk_utils_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/agent/disk_utils.go
Abort the table-driven disk test when expectedExplicitIncludes does not match the built entries length before the index-based assertions run.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/agent/disk_utils_test.go (1)
66-74: Optional: deduplicate repeatedusageFnfixtures.These three cases repeat the same
disk.UsageStatbuilder, which makes future edits noisier than needed. Consider a small helper to keep fixtures centralized.♻️ Suggested refactor
func TestBuildDiskReportEntries(t *testing.T) { + makeUsageFn := func(total, used, free uint64, usedPercent float64) func(string) (*disk.UsageStat, error) { + return func(mountpoint string) (*disk.UsageStat, error) { + return &disk.UsageStat{ + Path: mountpoint, + Total: total, + Used: used, + Free: free, + UsedPercent: usedPercent, + }, nil + } + } + tests := []struct { ... }{ { ... - usageFn: func(mountpoint string) (*disk.UsageStat, error) { - return &disk.UsageStat{ - Path: mountpoint, - Total: 32 * 1024 * 1024 * 1024, - Used: 8 * 1024 * 1024 * 1024, - Free: 24 * 1024 * 1024 * 1024, - UsedPercent: 25, - }, nil - }, + usageFn: makeUsageFn(32*1024*1024*1024, 8*1024*1024*1024, 24*1024*1024*1024, 25), ... },Also applies to: 97-105, 123-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/agent/disk_utils_test.go` around lines 66 - 74, Create a small helper to centralize the repeated disk.UsageStat fixture (e.g., func makeUsageStat(mountpoint string) *disk.UsageStat or makeUsageFixture) and use it from each usageFn instead of constructing the same &disk.UsageStat literal inline; update the three anonymous usageFn blocks (and the similar ones around the other occurrences) to return makeUsageStat(mountpoint) (or makeUsageFixture(mountpoint), nil) so the test cases reuse a single, named fixture and reduce duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/agent/disk_utils_test.go`:
- Around line 66-74: Create a small helper to centralize the repeated
disk.UsageStat fixture (e.g., func makeUsageStat(mountpoint string)
*disk.UsageStat or makeUsageFixture) and use it from each usageFn instead of
constructing the same &disk.UsageStat literal inline; update the three anonymous
usageFn blocks (and the similar ones around the other occurrences) to return
makeUsageStat(mountpoint) (or makeUsageFixture(mountpoint), nil) so the test
cases reuse a single, named fixture and reduce duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fe5905f2-b4d7-475a-881f-6cfb45ad34e1
📒 Files selected for processing (1)
internal/agent/disk_utils_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Make
disk_includesa hard override so explicitly included mounts still show up even when they would normally be filtered, including small tmpfs-backed mounts like log2ram.Deduplicate bind-mounted disk entries by default while still keeping bind mounts that were explicitly included.
Users trying to monitor log2ram were seeing explicit includes silently ignored for tmpfs mounts.
Bind mounts were reported as duplicate disks with identical usage, which made the hardware view confusing.
Summary by CodeRabbit
Documentation
disk_includes/NETRONOME__AGENT_DISK_INCLUDES are described as a “hard override”; bind-mount deduplication behavior is explained and help text formatting improved.Bug Fixes
Refactor
Tests