Skip to content

fix: a single failed drive no longer blanks a controller's device paths - #91

Open
g-carre wants to merge 6 commits into
mainfrom
fix/ARTESCA-17960-degraded-volume-paths
Open

fix: a single failed drive no longer blanks a controller's device paths#91
g-carre wants to merge 6 commits into
mainfrom
fix/ARTESCA-17960-degraded-volume-paths

Conversation

@g-carre

@g-carre g-carre commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Context

Backend root cause behind ARTESCA-17960 (and its UI/QA parents ARTESCA-17957 / RD-2154). On a bare-metal node with one failed data drive, disk-management-agent produced DiscoveredPhysicalDisk records with an empty status.devicePath for the whole controller, not just the failed drive — so consumers keying on device path lost the entire node's disk inventory.

For RAID-member drives the device path is not intrinsic to the drive; it is resolved from the containing logical volume. Several getters aborted a whole controller's enumeration when a single drive/volume could not be fully resolved, blanking every (including healthy) disk on that controller.

Fixes (one commit per controller)

  • megaraid (v1)getPaths returned an error when a volume's /dev/disk/by-id/wwn-* link was absent and the volume had more than one backing drive. A degraded-but-online array still exposes a valid OS device path, so it is now returned (empty permanent path) instead of failing the whole controller.
  • ssacli — the "Disk Name" parser called lsblk only to refine status to Used; a failed/pulled drive whose /dev node vanished made that lookup fail and aborted the whole inventory. The lookup failure is now non-fatal.
  • storcli2parseDrive resolved JBOD Used device paths via ComputePaths (real filesystem); a resolution failure now keeps the drive with empty paths instead of dropping the whole controller.

Tests & docs

  • Unit + end-to-end regression tests per fix, plus a degraded-volume guard for the storcli2 RAID (LV) path and the megaraid v1 read path.
  • DESIGN.md: documents the cross-adapter contract that getters degrade gracefully rather than dropping a controller's inventory.

rhel8 shares the structural pattern but is out of scope here (not consumed by the affected agent).

Issue: ARTESCA-17960

🤖 Generated with Claude Code

@g-carre
g-carre requested a review from a team as a code owner August 7, 2026 21:43
Comment thread pkg/implementation/physicaldrivegetter/storcli2_test.go
@claude

claude Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor
  • pkg/implementation/physicaldrivegetter/storcli2_test.go:196 — TestStorCLI2PhysicalDrivesComputePathsFailureIsNotFatal was inserted between TestStorCLI2PhysicalDrivesJBOD's doc comment and its function declaration, merging both comments into one godoc block and leaving the JBOD test without its comment. Split them apart.

    Otherwise this is a solid set of fixes. The graceful-degradation approach is consistent across all three adapters (megaraid, ssacli, storcli2), the new getPaths logic in megaraid correctly guards against an empty SCSI NAA ID, and each fix has matching test coverage with appropriate testdata fixtures. The clearing of DevicePath/PermanentPath after a ComputePaths failure in the storcli2 adapter is correct since ComputePaths can partially set PermanentPath before failing on symlink resolution.

    Review by Claude Code

@g-carre
g-carre force-pushed the fix/ARTESCA-17960-degraded-volume-paths branch from 8f88eea to cd16d11 Compare August 10, 2026 12:43
Comment thread pkg/implementation/physicaldrivegetter/storcli2_test.go
@claude

claude Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor
  • pkg/implementation/physicaldrivegetter/storcli2_test.go:196 — New test function inserted between the existing doc comment for TestStorCLI2PhysicalDrivesJBOD and the function itself, merging both doc comments into one block attached to the wrong function. Move the original comment back above TestStorCLI2PhysicalDrivesJBOD.

    The core fix logic across all three adapters (megaraid getPaths, ssacli parsePDLine, storcli2 parseDrive) is sound: each correctly degrades to empty paths instead of aborting the whole controller's inventory. Test coverage is thorough with unit tests per adapter plus an end-to-end regression test for the megaraid read path. The DESIGN.md contract addition is a good guardrail.

    Review by Claude Code

Comment thread pkg/implementation/physicaldrivegetter/storcli2_test.go
CustomFileExists = tc.fileExists
if tc.evalSymlinks != nil {
CustomEvalSymlinks = tc.evalSymlinks
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CustomEvalSymlinks is only set when tc.evalSymlinks != nil, so after test case 2 ("wwn link present, os drive name empty") runs and installs its custom function, test cases 3 and 4 inherit that stale stub. Not a bug today because those cases never hit the EvalSymlinks code path, but fragile if cases are added or reordered. Resetting at the top of each iteration is safer:

Suggested change
}
CustomFileExists = tc.fileExists
CustomEvalSymlinks = origEvalSymlinks
if tc.evalSymlinks != nil {
CustomEvalSymlinks = tc.evalSymlinks
}

— Claude Code

@claude

claude Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor
  • pkg/implementation/physicaldrivegetter/storcli2_test.go:196 — Doc comment for TestStorCLI2PhysicalDrivesJBOD merged into the new test's comment block (no blank-line separator); TestStorCLI2PhysicalDrivesJBOD loses its doc comment and the new test gets a misleading preamble.
    - pkg/implementation/raidcontroller/megaraid/logicalvolume_internal_test.go:80 — CustomEvalSymlinks not reset between subtests; stale stub leaks from test case 2 into later cases (not a bug today but fragile).

    The core fixes across all three adapters (megaraid, ssacli, storcli2) look correct: error handling follows the project's patterns, the graceful-degradation contract is well documented in DESIGN.md, tests cover the exact failure scenarios, and test fixtures are realistic.

    Review by Claude Code

g-carre and others added 6 commits August 10, 2026 18:52
…olumes

getPaths() returned an error when a volume's /dev/disk/by-id/wwn-* link was
absent and the volume had more than one backing drive. A degraded-but-online
RAID volume (one whose data drive has failed) still exposes a valid OS device
path and still serves I/O, but its udev by-id link can be missing while the
array is not optimal. The error propagated up through LogicalVolumes(), so a
single failed drive aborted logical-volume discovery for the entire
controller, leaving every disk on it with an empty device path.

Resolve the by-id/wwn permanent path only when a SCSI NAA Id is reported and
its link exists; otherwise fall back to the backing drive for single-drive
volumes, or return the (still valid) OS device path with an empty permanent
path for multi-drive volumes, instead of failing. Discovery now returns every
volume with its status and a usable device path.

Issue: ARTESCA-17960

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ails

The "Disk Name" parser called lsblk purely to refine a drive's status to Used
when the device is mounted or formatted; the device path was already set. A
failed or pulled drive whose /dev node has disappeared makes that lsblk lookup
fail, and the error aborted parsing for the whole controller, leaving every
drive on it absent from discovery.

Treat an lsblk lookup failure as "cannot refine status" and keep the status
ssacli already reported, so a single unhealthy drive no longer takes down the
controller's entire inventory. Mirrors the SmartArray equivalent of the
MegaRAID logical-volume fix.

Issue: ARTESCA-17960

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
parseDrive resolves host device paths for JBOD "Used" drives via ComputePaths,
which reads the real filesystem. When that failed (e.g. a drive whose udev
by-id link is missing or has not settled yet), the error aborted parsing for
the whole controller, dropping every drive on it from discovery.

Treat a path-resolution failure as best-effort: keep the drive with empty
paths instead of failing the entire inventory, mirroring the megaraid and
ssacli fixes for the same class of bug.

Issue: ARTESCA-17960

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The storcli2 logical-volume getter resolves the device path of RAID disks, but
end-to-end coverage only exercised the Optimal case; degraded/failed states
were only checked at the lvStatus() mapping level. Add a test proving a
degraded volume (one member drive failed) is still returned with its device
and permanent path intact and does not abort the controller's discovery — the
ARTESCA-17960 property for storcli2 RAID disks.

Issue: ARTESCA-17960

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a suite test that drives the full legacy v1 read path
(logicalVolume -> fillPhysicalDrives -> getPaths) for a degraded multi-drive
volume whose /dev/disk/by-id/wwn link is absent — the exact original-bug
trigger. It asserts the volume is returned with its OS device path intact and
degraded status, rather than erroring and blanking the whole controller.
Complements the getPaths unit test with coverage of the real read chain.

Issue: ARTESCA-17960

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
State the cross-adapter contract that a getter returns a drive or volume it
cannot fully resolve (missing by-id link, failed JBOD device node, degraded
array) with empty paths and its status, rather than failing and dropping the
whole controller's inventory. Documents the behavior the megaraid, ssacli and
storcli2 fixes now guarantee.

Issue: ARTESCA-17960

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@g-carre
g-carre force-pushed the fix/ARTESCA-17960-degraded-volume-paths branch from cd16d11 to d7e2206 Compare August 10, 2026 16:57
@claude

claude Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

LGTM — all three adapter fixes (megaraid v1, ssacli, storcli2) correctly demote a path-resolution failure to graceful degradation with empty paths rather than aborting the whole controller's inventory. The refactored getPaths/resolveWWNPath split is clean, the DESIGN.md contract is well-documented (including the deliberate exception for the megaraid create/settle path), and each fix has a corresponding unit test plus an end-to-end guard where applicable.

Review by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant