Fix disk metrics for LVM: map /dev/mapper/* to dm-* when reading IO counters #133
Conversation
disk: support LVM/device-mapper by resolving /dev/mapper to dm-* and trying multiple candidates in collectIOStats
Enhanced comments for clarity and added functionality to resolve device mapper paths.
WalkthroughExpanded disk IO stats key resolution for LVM/device-mapper by scanning /sys/block to map /dev/mapper and dm-* names to their dm-* counterparts. Added symlink resolution, basename handling, deduplication, and a new helper to locate dm devices via dm/name in sysfs. Imports updated to include os. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Collector
participant K as buildDeviceKeyCandidates
participant S as Sysfs (/sys/block)
participant H as findDMForMapperBase
C->>K: Request key candidates for device path
K->>K: Add basename and resolved symlink basename
alt mapper or dm-* basename
K->>H: findDMForMapperBase(mapperBase)
H->>S: Read dm-*/dm/name entries
S-->>H: Return matching dm-* (exact/relaxed)
H-->>K: dm-* device name (if found)
K->>K: Include mapped dm-* candidate
end
K->>K: Deduplicate and trim keys
K-->>C: Candidate keys for IOCounters lookup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
|
We're using |
changes made |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/metric/disk.go (1)
189-195: Remove redundantdm-basename check.The
strings.HasPrefix(base, "dm-")branch never yields a new key—findDMForMapperBase("dm-0")returns empty since sysfsdm/nameentries hold logical names, notdm-X. Drop|| strings.HasPrefix(base, "dm-")to simplify the condition.- if strings.HasPrefix(d, "/dev/mapper/") || strings.HasPrefix(base, "dm-") { + if strings.HasPrefix(d, "/dev/mapper/") {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/metric/disk.go(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (windows-2025)
- GitHub Check: lint
🔇 Additional comments (4)
internal/metric/disk.go (4)
4-4: LGTM!The
osimport is necessary for the newfindDMForMapperBasefunction.
123-125: LGTM!The documentation accurately describes the LVM/device-mapper handling.
246-250: Verify the relaxed comparison doesn't cause false positives.The fallback comparison removes slashes and compares case-insensitively. While this adds robustness for edge cases, it could potentially match the wrong dm device if two devices have similar names (e.g.,
ubuntu-vg-ubuntu-lvvsubuntu/vg/ubuntu/lv).Additionally, the comment mentions LVM encoding
-as--, but the logic removes slashes (/), which are less common in LVM names. Could you clarify when this slash-removal logic would be beneficial? Consider either:
- Documenting a specific real-world scenario where this is needed, or
- Tightening the relaxed comparison to reduce false positive risk (e.g., only case-insensitive, no slash removal).
If you've observed specific udev rule edge cases that require this relaxed comparison, please document them in a code comment with an example.
219-254: Validate performance on systems with many dm devices
The function iterates over/sys/block/dm-*and reads eachdm/nameon every call, which may add latency if you have 20+ LVM volumes. Since this runs periodically, it’s probably fine, but please benchmark this path on a real system (e.g., measure average execution time over multiple runs) to ensure it remains within acceptable bounds. If you observe significant delays, consider caching the mapping or lowering the call frequency.
| "runtime" | ||
| "strings" | ||
|
|
||
| "github.com/shirou/gopsutil/v4/disk" |
There was a problem hiding this comment.
Could you please run gofmt -w . to fix import and documentation styles? It will add a new line between internal and external imports, and update the function documentation with correct indentation.
There was a problem hiding this comment.
If the formatting is OK, I would like to merge this PR. @VeNoMZiTo
2 participants
Summary
Disk metrics were missing for LVM/device-mapper volumes because the agent
looked up IO counters using
/dev/mapper/<lv>-style names, while gopsutilexposes IOCounters keyed by kernel device names (e.g.,
dm-0).Root cause
collectIOStatsstripped/dev/and attempted to querydisk.IOCounters()withnames like
mapper/ubuntu--vg-ubuntu--lvor its basename.dm-*keys (e.g.,dm-0), andfilepath.EvalSymlinksdoes not resolve device-mapper pseudo devices, so no matching entry was found.
Fix
disk.IOCounters()and search by key./dev/(e.g.,sda,mapper/vg-lv)basename(device)(e.g.,vg-lv)/dev/dm-0→dm-0)/dev/mapper/<name>to the correspondingdm-*by scanning/sys/block/dm-*/dm/nameand matching<name>to the dm logical name.stat.Name.Why this works
dm-*./sys/block/dm-*/dm/namereliably exposes the mapper logical name, whichallows us to derive the correct
dm-*key used by gopsutil.Testing
Environment: Ubuntu (LVM root)
/api/v1/metricsand confirmed disk metrics now appear for the LVM LV.sda,nvme0n1).Before (error): device stats not found: /dev/mapper/ubuntu--vg-ubuntu--lv (tried: mapper/ubuntu--vg-ubuntu--lv, ubuntu--vg-ubuntu--lv)
After (ok):
disk.read_bytes,disk.write_bytes,disk.read_time,disk.write_timepresent for the LVM volume.
Performance & compatibility
IOCounters()call per collection cycle (unchanged or cheaper thanrepeated lookups).
/sys/blockscan per device only when a mapper path is detected.Credits
Big thanks to @caleb for identifying the root cause and the right direction.
Checklist
/dev/mapper/*→dm-*stat.NamefallbackRelease note
Fix disk IO metrics not showing up on LVM/device-mapper volumes by mapping
/dev/mapper/*to the appropriatedm-*device when reading gopsutil counters.