fix(rest): stamp reports[] on PoolMissing so CLI renders Faulty (Bug 83)#3
fix(rest): stamp reports[] on PoolMissing so CLI renders Faulty (Bug 83)#3kvaps wants to merge 1 commit into
Conversation
… (Bug 83)
Python linstor-client's `storpool_cmds.py:472` derives the State column
from `get_replies_state(storpool.reports)` — the structured ApiCallRc
array — not from the bare `state` string field. With an empty reports[],
`get_replies_state` returns ("Ok", green) regardless of `state`, so a
satellite-reported "Faulty" pool (PoolMissing=true after `zpool destroy`)
kept rendering as Ok in `linstor sp l` even though REST emitted
state="Faulty" correctly.
Synthesise an ERROR-severity reports[] entry whenever PoolMissing=true,
in both the k8s store's crdToWireStoragePool and the in-memory store's
withDerivedState. The ret_code mirrors upstream LINSTOR's
FAIL_STOR_POOL_CONFIGURATION_ERROR (990) | MASK_STOR_POOL | MASK_ERROR
so audit-log greppers and the Python CLI's reply classifier both route
the entry correctly. The mask + sub-code constants live in pkg/api/v1
to avoid a cyclic dependency from the store layer on pkg/rest.
Tests pin the wire contract at both layers: store-level
TestCrdToWireStoragePoolFaultyStampsReports /
TestCrdToWireStoragePoolHealthyHasNoReports, and REST-level
TestViewStoragePoolsFaultyStampsReports.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses Bug 83 by ensuring that storage pools with PoolMissing set to true correctly report a 'Faulty' state to the Python LINSTOR client. This is implemented by synthesizing an error-severity report entry in both the in-memory and Kubernetes store layers using new constants added to the API package. Feedback suggests refactoring the duplicated report generation logic into a shared helper in pkg/api/v1 to ensure consistency and reduce code duplication.
| func poolMissingReport(node, pool string) apiv1.APICallRc { | ||
| return apiv1.APICallRc{ | ||
| RetCode: apiv1.APICallRcMaskError | | ||
| apiv1.APICallRcMaskStorPool | | ||
| apiv1.APICallRcFailStorPoolConfigurationError, | ||
| Message: "pool backing storage missing on node " + node + | ||
| ": storage pool " + pool + " is not present", | ||
| Cause: "The satellite probed the backing volume group / zpool / " + | ||
| "directory for this storage pool and found it absent — " + | ||
| "typically the result of an operator `zpool destroy`, " + | ||
| "`vgremove`, or an unmounted filesystem.", | ||
| Correc: "Re-discover the pool via `linstor ps cdp <provider> " + | ||
| "<node> <pool>` once the backing storage is restored, or " + | ||
| "recreate the pool with `linstor sp c`.", | ||
| ObjRefs: map[string]string{ | ||
| "Node": node, | ||
| "StorPool": pool, | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
The poolMissingReport function is identical to the one added in pkg/store/k8s/storage_pools.go. Since both packages already depend on pkg/api/v1, moving this logic to a shared helper in that package (e.g., NewPoolMissingReport) would eliminate duplication and ensure consistency across store implementations. This is particularly important for maintaining the specific error messages and codes used for this state.
1 participant
Summary
Python linstor-client's
storpool_cmds.py:472derives the State column fromget_replies_state(storpool.reports)— the structured ApiCallRc array — not from the barestatestring field. With an empty reports[],get_replies_statereturns("Ok", green)regardless ofstate, so a satellite-reported Faulty pool (PoolMissing=trueafterzpool destroy) kept rendering as Ok inlinstor sp leven though REST/v1/view/storage-poolscorrectly emittedstate="Faulty".This fix synthesises an ERROR-severity reports[] entry whenever
PoolMissing=true, in both the k8s store'scrdToWireStoragePooland the in-memory store'swithDerivedState, keeping the two backends in lockstep.Wire shape
The new ApiCallRc entry carries:
ret_code = MASK_ERROR | MASK_STOR_POOL | FAIL_STOR_POOL_CONFIGURATION_ERROR (990)— byte-identical to upstream LINSTOR's value, so audit-log greppers and the Python CLI's reply classifier both route the entry to the storage-pool error bucket.messagementioning bothmissingand the pool name so operators can correlate fromlinstor sp loutput alone.cause/correctionstrings explaining the typical trigger (zpool destroy/vgremove) and the recovery path (linstor ps cdporlinstor sp c).obj_refs[Node]/obj_refs[StorPool]so log queries can filter by either axis.The mask + sub-code constants live in
pkg/api/v1(next to theAPICallRctype) rather thanpkg/rest, so the store layer can stamp the entry without pulling a cyclic dependency on pkg/rest.Tests
pkg/store/k8s/storage_pools_internal_test.go::TestCrdToWireStoragePoolFaultyStampsReports— pins the k8s store's CRD→wire converter:Status.PoolMissing=trueproducesState=="Faulty"AND a non-empty Reports slice with MASK_ERROR + FAIL_STOR_POOL_CONFIGURATION_ERROR +missing+ pool name + ObjRefs.pkg/store/k8s/storage_pools_internal_test.go::TestCrdToWireStoragePoolHealthyHasNoReports— pins the inverse: a healthy pool MUST emit an empty Reports slice (stamping a no-op entry would tripget_replies_stateinto rendering the State column from a misleading first entry).pkg/rest/storage_pools_test.go::TestViewStoragePoolsFaultyStampsReports— end-to-end through the REST view path with the in-memory store.Test plan
go test ./pkg/store/... ./pkg/rest/... -count=1go build ./...zpool destroy blockstor-zfson a worker →linstor sp lMUST render the affected pool's State column asFaulty/Error/Warn(whateverget_replies_statereturns for an ERROR-severity reports entry) rather thanOk.