feat(storcli2): cache options setter and JBOD setters by g-carre · Pull Request #71 · scality/raidmgmt

g-carre · 2026-06-25T09:26:27Z

Summary

Write path of the storcli2/perccli2 adapter (ARTESCA-17649): the logical-volume cache setter and the per-drive JBOD setters.

SetLVCacheOptions — reads the current state through the logical-volume getter and emits only the policies that changed, as two independent set rdcache= / set wrcache= commands (storcli2 rejects the combined syntax and dropped the IO policy).
EnableJBOD / DisableJBOD — /cx/ex/sx set jbod and set uconf, surfacing the in-JSON failure payload.
Minor storcli2 envelope adjustment to support the setter error paths.

Testing

go build, go vet, gofmt, and the package test suite pass. Tests cover the only-changed-flag behavior (rdcache-only / wrcache-only / both / none), getter-error and command-error paths, and the JBOD enable/disable paths.

JBOD …/success.json fixtures still need a hardware capture via collect_storcli2_testdata.sh (noted in the ticket); the failure paths are covered.

Issue: ARTESCA-17649

🤖 Generated with Claude Code

claude · 2026-06-25T09:31:37Z

logicalvolumemanager/storcli2.go:63 — current.CacheOptions (pointer) is passed to storcli2CacheOptions without a nil guard; a nil-returning getter would cause a panic
- logicalvolumemanager/storcli2.go:31 — exported field StorCLI2 shadows the struct type name; physicaldrivegetter uses unexported runner for the same role

Overall: clean implementation with good test coverage and proper error wrapping. The envelope.go ErrCd change from int to any is correct — storcli2 returns "-" on success which would break int unmarshalling. The delta-only cache-set approach (only emitting changed flags) is well-designed and well-tested.

Review by Claude Code

claude · 2026-06-29T18:00:05Z

LGTM — the changes are correct and safe.

- The ErrCd widening from int to any is necessary: storcli2 returns an int on failure (e.g. 9) and the string "-" on success (confirmed by the fixtures). No code reads ErrCd directly — only json.Unmarshal touches it — so the type change is safe. Follows the same any pattern already used for VD, PID, and Controller in the same package.
- Fixture renames are clean: no Go code in the old packages references the moved files, and the new per-port package layout (jbodsetter, lvcachesetter) is consistent with the project's one-package-per-port decomposition.

Review by Claude Code

claude · 2026-06-29T18:09:04Z

        "Disable JBOD on e${FIRST_ENCLOSURE}/s${FIRST_SLOT} (expected failure - drive in VD)" \
-        "${OUTPUT_DIR}/physicaldrivegetter/testdata/storcli2/jbod/disable/fail.json" \
+        "${OUTPUT_DIR}/jbodsetter/testdata/storcli2/jbod/disable/fail.json" \
        "${C}/e${FIRST_ENCLOSURE}/s${FIRST_SLOT}" delete jbod


The adapter's DisableJBOD now runs set uconf (storcli2UConfOption), but this capture command still runs delete jbod (the old storcli v1 syntax). The comments on the preceding lines also reference the old API (adapter.setJBOD(metadata, "delete")). When someone regenerates fixtures with this script, the disable fixture will capture the wrong command's output.

Suggested change

"${C}/e${FIRST_ENCLOSURE}/s${FIRST_SLOT}" delete jbod

"${C}/e${FIRST_ENCLOSURE}/s${FIRST_SLOT}" set uconf

The comments above (lines 338-340) should also be updated to match, e.g.:

# Command: storcli2 /c0/e306/s0 set uconf J # Used by: adapter.DisableJBOD(metadata) → runner.Run(["/c0/e306/s0", "set", "uconf"])

— Claude Code

claude · 2026-06-29T18:09:07Z

 | `physicaldrivegetter` | `testdata/storcli2/show/e306s99_invalid.json` | drive not found | safe |
 | `physicaldrivegetter` | `testdata/storcli2/show/e320s11_UGood.json` | drive in unconfigured-good state | destructive |
-| `physicaldrivegetter` | `testdata/storcli2/jbod/{enable,disable}/fail.json` | `set jbod` / `delete jbod` | destructive |
+| `jbodsetter` | `testdata/storcli2/jbod/{enable,disable}/fail.json` | `set jbod` / `delete jbod` | destructive |


The command column still says delete jbod for the disable fixture, but the adapter now uses set uconf.

Suggested change

| `jbodsetter` | `testdata/storcli2/jbod/{enable,disable}/fail.json` | `set jbod` / `delete jbod` | destructive |

| `jbodsetter` | `testdata/storcli2/jbod/{enable,disable}/fail.json` | `set jbod` / `set uconf` | destructive |

— Claude Code

claude · 2026-06-29T18:09:19Z

tests/testdata-tools/collect_storcli2_testdata.sh:344 — JBOD disable fixture capture still runs delete jbod (storcli v1 syntax), but the adapter uses set uconf. Comments on lines 338-340 also reference the old API. Regenerating fixtures would capture the wrong command's output.
- tests/testdata-tools/README.md:49 — command column says delete jbod for the disable fixture, should be set uconf to match the adapter.

The adapter code, envelope change, cache setter, and tests all look correct. The hexagonal boundaries are clean, error handling uses errors.Wrap/Wrapf consistently, the ErrCd widening to any is safe (field is never read programmatically), and the read-before-write + only-changed-flags pattern is well-motivated and tested.

Review by Claude Code

Implement the storcli2/perccli2 write path for ARTESCA-17649: - SetLVCacheOptions on the logicalvolumemanager StorCLI2: diffs current vs desired and emits only the changed rdcache/wrcache flags as separate "set" commands (storcli2 rejects the combined syntax and dropped the IO policy). - EnableJBOD / DisableJBOD on the physicaldrivegetter StorCLI2 via "set jbod" / "set uconf" (storcli's "delete jbod" no longer parses). Both surface in-JSON failures through storcli2.Decode regardless of exit code. Widen the shared envelope's DetailedStatus.ErrCd to "any": storcli2 reports it as an int on failure and the string "-" on success, mirroring PID/VD. Issue: ARTESCA-17649 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Relocate the storcli2 JBOD setter and LV cache setter into their own packages named after their RAIDController port interfaces (jbodsetter, lvcachesetter), instead of folding them into physicaldrivegetter and logicalvolumemanager. This matches the decomposed one-package-per-port pattern already used by the getters and blinker, and makes each setter self-contained. Also document the conventions so future work follows suit: - CLAUDE.md: package layout mirrors the ports. - DESIGN.md: per-port packages; adapter token mapping expresses the per-controller settable subset (fails closed on new enum values); read-before-write emits only changed flags. - tests/testdata-tools: repoint fixture paths to the new packages. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

claude · 2026-06-30T07:36:14Z

+type StorCLI2 struct {
+	ports.LogicalVolumesGetter
+
+	StorCLI2 commandrunner.CommandRunner


The field StorCLI2 commandrunner.CommandRunner shadows the struct type name StorCLI2. This makes s.StorCLI2 on line 115 read as if the struct is referencing itself. The sibling adapters (jbodsetter, physicaldrivegetter) name this field runner, which is clearer.

Suggested change

StorCLI2 commandrunner.CommandRunner

runner commandrunner.CommandRunner

— Claude Code

claude · 2026-06-30T07:36:18Z

+		return errors.Wrapf(err, "failed to get logical volume %s", metadata.ID)
+	}
+
+	options, err := storcli2CacheOptions(current.CacheOptions, desired)


current.CacheOptions is a pointer (*CacheOptions) and will be nil if the getter returns a LogicalVolume without cache options populated — dereferencing it inside storcli2CacheOptions (lines 91, 100) would panic. The storcli2 getter currently always populates it, but since the getter is injected via the port interface, a nil guard here would be safer.

— Claude Code

claude · 2026-06-30T07:36:23Z

        "Disable JBOD on e${FIRST_ENCLOSURE}/s${FIRST_SLOT} (expected failure - drive in VD)" \
-        "${OUTPUT_DIR}/physicaldrivegetter/testdata/storcli2/jbod/disable/fail.json" \
+        "${OUTPUT_DIR}/jbodsetter/testdata/storcli2/jbod/disable/fail.json" \
        "${C}/e${FIRST_ENCLOSURE}/s${FIRST_SLOT}" delete jbod


The capture command still uses the v1 delete jbod syntax, and the comment references the old setJBOD(metadata, "delete") API. The adapter now uses set uconf (storcli2UConfOption). When someone regenerates fixtures with this script, they will capture the output of the wrong command.

Suggested change

"${C}/e${FIRST_ENCLOSURE}/s${FIRST_SLOT}" delete jbod

"${C}/e${FIRST_ENCLOSURE}/s${FIRST_SLOT}" set uconf

Lines 338-340 comments should also be updated to reference set uconf instead of delete jbod.

— Claude Code

claude · 2026-06-30T07:36:32Z

pkg/implementation/lvcachesetter/storcli2.go:33 — Field StorCLI2 shadows the struct type name; sibling adapters use runner instead
- pkg/implementation/lvcachesetter/storcli2.go:69 — Potential nil dereference if getter returns a LogicalVolume with nil CacheOptions
- tests/testdata-tools/collect_storcli2_testdata.sh:344 — JBOD disable capture still uses v1 delete jbod syntax; adapter now uses set uconf

Review by Claude Code

g-carre requested a review from a team as a code owner June 25, 2026 09:26

claude Bot reviewed Jun 25, 2026

View reviewed changes

Comment thread pkg/implementation/logicalvolumemanager/storcli2.go

claude Bot reviewed Jun 25, 2026

View reviewed changes

Comment thread pkg/implementation/logicalvolumemanager/storcli2.go

g-carre force-pushed the improvement/storcli2-cache-jbod-setter branch from d242702 to 185c909 Compare June 29, 2026 18:05

claude Bot reviewed Jun 29, 2026

View reviewed changes

g-carre and others added 2 commits June 30, 2026 09:31

g-carre force-pushed the improvement/storcli2-cache-jbod-setter branch from 185c909 to b218449 Compare June 30, 2026 07:32

claude Bot reviewed Jun 30, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(storcli2): cache options setter and JBOD setters#71

feat(storcli2): cache options setter and JBOD setters#71
g-carre wants to merge 2 commits into
mainfrom
improvement/storcli2-cache-jbod-setter

g-carre commented Jun 25, 2026

Uh oh!

Uh oh!

Uh oh!

claude Bot commented Jun 25, 2026

Uh oh!

claude Bot commented Jun 29, 2026

Uh oh!

claude Bot Jun 29, 2026

Uh oh!

claude Bot Jun 29, 2026

Uh oh!

claude Bot commented Jun 29, 2026

Uh oh!

claude Bot Jun 30, 2026

Uh oh!

claude Bot Jun 30, 2026

Uh oh!

claude Bot Jun 30, 2026

Uh oh!

claude Bot commented Jun 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

	"${C}/e${FIRST_ENCLOSURE}/s${FIRST_SLOT}" delete jbod
	"${C}/e${FIRST_ENCLOSURE}/s${FIRST_SLOT}" set uconf

	\| `jbodsetter` \| `testdata/storcli2/jbod/{enable,disable}/fail.json` \| `set jbod` / `delete jbod` \| destructive \|
	\| `jbodsetter` \| `testdata/storcli2/jbod/{enable,disable}/fail.json` \| `set jbod` / `set uconf` \| destructive \|

	StorCLI2 commandrunner.CommandRunner
	runner commandrunner.CommandRunner

Uh oh!

Conversation

g-carre commented Jun 25, 2026

Summary

Testing

Uh oh!

Uh oh!

Uh oh!

claude Bot commented Jun 25, 2026

Uh oh!

claude Bot commented Jun 29, 2026

Uh oh!

claude Bot Jun 29, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Jun 29, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot commented Jun 29, 2026

Uh oh!

claude Bot Jun 30, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Jun 30, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Jun 30, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot commented Jun 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant