add LVMS toolset for storage diagnostics - #370
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a prompts-only LVMS toolset with Kubernetes-backed troubleshooting and capacity reports, registers it with MCP and README generation, documents the prompts, and adds 11 LVMS evaluation scenarios. ChangesLVMS toolset
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mmakwana30 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (11)
pkg/toolsets/lvms/suite_test.go (1)
1-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffMock-based test scaffolding conflicts with "avoid mocks" guideline.
The suite constructs
mockKubernetesClient/mockToolCallRequestin lieu of real implementations. As per coding guidelines,*_test.gofiles should "Use real implementations and integration testing where possible - avoid mocks," and usesetup-envtestfor test setup. This scaffolding is foundational for the whole LVMS test cohort, so reworking it to use envtest would be a broad, high-effort change; flagging for awareness rather than blocking, since it may already be an established pattern elsewhere in the codebase.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/toolsets/lvms/suite_test.go` around lines 1 - 77, The LVMS test suite currently relies on local mock types (`mockKubernetesClient` and `mockToolCallRequest`) instead of real test setup, which conflicts with the testing guideline. Update `LVMSTestSuite` to use `setup-envtest`-backed Kubernetes client initialization and real request/argument handling where possible, and remove or minimize the mock scaffolding in `SetupTest`, `SetDynamicClient`, `SetClientset`, and `mockToolCallRequest` so the suite exercises actual implementations.Source: Coding guidelines
pkg/toolsets/lvms/manage.go (1)
115-122: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRaw
params.GetArguments()access instead of theWrapParamshelper used elsewhere in this function.
device_pathsextraction bypassespand readsparams.GetArguments()directly, unlikename/namespace/etc. above. IfWrapParamssupports slice accessors, prefer using them for consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/toolsets/lvms/manage.go` around lines 115 - 122, The device_paths parsing in manageLVMS is bypassing the existing WrapParams-based access pattern and reads params.GetArguments() directly. Update the device_paths extraction to use the same p helper used for name, namespace, and the other fields in this function, and use its slice/array accessor if available so the lookup is consistent with the rest of manage.go.pkg/toolsets/lvms/lvmcluster.go (1)
79-82: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent parameter extraction vs.
manage.go'sWrapParamspattern.Here
namespaceis pulled directly viaparams.GetArguments()["namespace"].(string), whilemanage.gousesapi.WrapParams(params).OptionalString(...)for the same purpose. Consider using the same helper here for consistency and to avoid re-implementing the same type-assertion logic.Also applies to: 154-156
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/toolsets/lvms/lvmcluster.go` around lines 79 - 82, The namespace extraction in lvms lvmcluster handling is re-implementing the same type-assertion logic used elsewhere instead of following the WrapParams pattern. Update the namespace reads in the affected logic to use api.WrapParams(params).OptionalString(...) consistently, matching manage.go and the surrounding parameter handling in lvmcluster.go so the same helper is used at both call sites.pkg/toolsets/lvms/lvmcluster_test.go (1)
10-175: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for the
Forbiddenerror branch.Both
listLVMClustersanddescribeLVMClusterhave explicitapierrors.IsForbiddenhandling (lvmcluster.go lines 98-100, 163-165), but no test exercises that branch. As per coding guidelines, tests should "Aim for high test coverage of the public API including edge case tests for error paths and boundary conditions."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/toolsets/lvms/lvmcluster_test.go` around lines 10 - 175, Add test coverage for the Forbidden error path in both `TestListLVMClusters` and `TestDescribeLVMCluster` by introducing a case that makes `listLVMClusters` and `describeLVMCluster` return `apierrors.IsForbidden`-matching errors from the dynamic client. Use the existing helpers and symbols (`listLVMClusters`, `describeLVMCluster`, `LVMSTestSuite`, `SetDynamicClient`) to assert the result contains the forbidden message and verifies the explicit forbidden handling branch instead of the generic error path.Source: Coding guidelines
pkg/toolsets/lvms/manage_test.go (1)
9-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing success-path test for
lvmsCreateClusterandForbidden-branch tests for create/delete.Only the missing-name failure is tested for
lvmsCreateCluster; there's no test validating a successful create (which would also exercise/validate the YAML templating inmanage.go). Neither create nor delete exercises theapierrors.IsForbiddenbranch. As per coding guidelines, tests should "Aim for high test coverage of the public API including edge case tests for error paths and boundary conditions."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/toolsets/lvms/manage_test.go` around lines 9 - 96, Add coverage for the missing create success path in lvmsCreateCluster by adding a test that supplies a valid name and namespace, uses the fake dynamic client, and asserts the returned content indicates successful creation so the templating path in manage.go is exercised. Also add Forbidden-branch tests for both lvmsCreateCluster and lvmsDeleteCluster by configuring the client/errors to trigger apierrors.IsForbidden and asserting the returned result/error message matches the forbidden case. Use the existing lvmsCreateCluster and lvmsDeleteCluster helpers and the LVMSTestSuite setup to keep the new tests consistent with the current suite.Source: Coding guidelines
pkg/toolsets/lvms/pods.go (1)
214-252: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate restart-count/pod-summary logic vs.
troubleshoot.go.The container-restart-count accumulation loop (Lines 233-242) is duplicated almost verbatim in
fetchLVMSPodsintroubleshoot.go(Lines 415-424). Consider extracting a shared helper, e.g.sumPodRestarts(pod unstructured.Unstructured) int64, to avoid drift between the two copies.♻️ Proposed helper extraction
+func sumPodRestarts(pod unstructured.Unstructured) int64 { + var restarts int64 + if containerStatuses, found, _ := unstructured.NestedSlice(pod.Object, "status", "containerStatuses"); found { + for _, cs := range containerStatuses { + if csMap, ok := cs.(map[string]interface{}); ok { + if rc, ok := csMap["restartCount"].(int64); ok { + restarts += rc + } + } + } + } + return restarts +}Then in
lvmsListPods:- // Get restart count - restarts := int64(0) - if containerStatuses, found, _ := unstructured.NestedSlice(pod.Object, "status", "containerStatuses"); found { - for _, cs := range containerStatuses { - if csMap, ok := cs.(map[string]interface{}); ok { - if rc, ok := csMap["restartCount"].(int64); ok { - restarts += rc - } - } - } - } - podInfo["Restarts"] = restarts + podInfo["Restarts"] = sumPodRestarts(pod)And reuse the same helper in
troubleshoot.go'sfetchLVMSPods.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/toolsets/lvms/pods.go` around lines 214 - 252, The pod restart-count logic in `lvmsListPods` is duplicated in `fetchLVMSPods` from `troubleshoot.go`, so extract the shared container-status accumulation into a helper such as `sumPodRestarts(pod unstructured.Unstructured) int64` and call it from both places. Keep the existing pod-summary assembly in `lvmsListPods`, but delegate the restart calculation to the helper so both code paths stay consistent and don’t drift.pkg/toolsets/lvms/volumegroups.go (1)
83-110: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNamespace default inconsistency vs. other LVMS tools.
Unlike
pods.gotools (lvmsGetOperatorLogs,lvmsListPods, etc.), which defaultnamespacetodefaults.DefaultNamespace()when unspecified,listLVMVolumeGroups/getVolumeGroupNodeStatusdefault to listing across all namespaces. Since these CRDs are effectively always created in the LVMS namespace, this asymmetry adds needless cluster-wide list calls/RBAC surface for little practical benefit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/toolsets/lvms/volumegroups.go` around lines 83 - 110, The namespace handling in listLVMVolumeGroups is inconsistent with the other LVMS tools: when namespace is omitted, it currently lists cluster-wide instead of defaulting to defaults.DefaultNamespace(). Update listLVMVolumeGroups (and any matching helper such as getVolumeGroupNodeStatus if it uses the same pattern) to use the default LVMS namespace when params.GetArguments()["namespace"] is empty, and keep the namespaced Resource(...).Namespace(namespace).List path as the normal case.pkg/toolsets/lvms/troubleshoot.go (1)
344-367: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMarshal errors from
yaml.Marshalare silently discarded.Unlike
volumegroups.go/pods.go/health.go, which check and surfaceyaml.Marshalerrors, the helper functions here (fetchLVMClusterStatusLine 356,fetchPVCDetailsLine 446) discard the error viadata, _ := yaml.Marshal(...). Low risk given the inputs are simple maps, but inconsistent with the rest of the package's error-handling convention.Also applies to: 432-455
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/toolsets/lvms/troubleshoot.go` around lines 344 - 367, The helper in fetchLVMClusterStatus is discarding yaml.Marshal errors, which is inconsistent with the package’s error-handling pattern. Update fetchLVMClusterStatus (and the similar fetchPVCDetails helper mentioned in the review) to check the marshal return value instead of using the blank identifier, and surface or log the error in the returned status text the same way other helpers like volumegroups.go, pods.go, and health.go do. Use the existing fetchLVMClusterStatus and fetchPVCDetails symbols to locate the affected paths.pkg/toolsets/lvms/health.go (1)
262-264: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
round2truncates rather than rounds — misleading name.
float64(int(v*100)) / 100truncates towards zero; it does not round. The test athealth_test.goLine 56-60 even documents this ("round up conceptually but truncate"). Displayed pool sizes will always be biased slightly low (e.g.,53.999→53.99instead of54.0). Considermath.Round(v*100)/100if actual rounding is intended, or rename totruncate2for clarity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/toolsets/lvms/health.go` around lines 262 - 264, The helper round2 in health.go is truncating values instead of rounding them, so either change its implementation to use true rounding for the displayed pool sizes or rename it to reflect truncation if that behavior is intended. Update the caller expectations and the related health_test.go case so the behavior matches the chosen name and logic, using round2 as the symbol to locate the change.pkg/toolsets/lvms/troubleshoot_test.go (1)
25-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnnecessary anonymous-struct copying adds boilerplate without benefit.
TestLvmsTroubleshootPromptMetadata,TestLvmsCapacityPromptMetadata, andTestTroubleshootPromptArgumentsMetadataeach copyapi.Prompt/api.PromptArgumentfields into ad-hoc anonymous structs before asserting on them. This adds indirection for no benefit — assertions could run directly againstp.Prompt.Title,p.Prompt.Arguments[i].Name, etc., inside the loop.♻️ Simplified version (for TestLvmsTroubleshootPromptMetadata)
func TestLvmsTroubleshootPromptMetadata(t *testing.T) { prompts := initLVMSTroubleshoot() - - var troubleshootPrompt *struct { - name string - title string - description string - arguments int - } - - for _, p := range prompts { - if p.Prompt.Name == "lvms-troubleshoot" { - troubleshootPrompt = &struct { - name string - title string - description string - arguments int - }{ - name: p.Prompt.Name, - title: p.Prompt.Title, - description: p.Prompt.Description, - arguments: len(p.Prompt.Arguments), - } - break - } - } - - require.NotNil(t, troubleshootPrompt, "lvms-troubleshoot prompt should be found") - - assert.Equal(t, "lvms-troubleshoot", troubleshootPrompt.name) - assert.Equal(t, "LVMS Troubleshoot", troubleshootPrompt.title) - assert.Contains(t, troubleshootPrompt.description, "troubleshooting") - assert.Equal(t, 3, troubleshootPrompt.arguments, "should have 3 arguments: namespace, pvc, pvc_namespace") + var found bool + for _, p := range prompts { + if p.Prompt.Name != "lvms-troubleshoot" { + continue + } + found = true + assert.Equal(t, "LVMS Troubleshoot", p.Prompt.Title) + assert.Contains(t, p.Prompt.Description, "troubleshooting") + assert.Len(t, p.Prompt.Arguments, 3, "should have 3 arguments: namespace, pvc, pvc_namespace") + } + require.True(t, found, "lvms-troubleshoot prompt should be found") }Also applies to: 103-159
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/toolsets/lvms/troubleshoot_test.go` around lines 25 - 93, The prompt metadata tests are over-copying fields into anonymous structs, adding unnecessary boilerplate in TestLvmsTroubleshootPromptMetadata, TestLvmsCapacityPromptMetadata, and TestTroubleshootPromptArgumentsMetadata. Update the assertions to read directly from the found prompt’s api.Prompt fields (for example, use p.Prompt.Name/Title/Description and p.Prompt.Arguments[i].Name/Description inside the loop) and remove the temporary anonymous-struct holders while keeping the same expectations and prompt lookup logic.pkg/toolsets/lvms/volumegroups_test.go (1)
10-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo error-path coverage (CRD-not-found / forbidden) for either tool.
Both
TestListLVMVolumeGroupsandTestGetVolumeGroupNodeStatusdefine anexpectedErrorfield in the table-driven test struct, but no test case ever sets it — the error branches inlistLVMVolumeGroups/getVolumeGroupNodeStatus(e.g.,apierrors.IsNotFound,apierrors.IsForbiddeninvolumegroups.goLines 98-106, 161-169) are never exercised.As per coding guidelines, "Aim for high test coverage of the public API including edge case tests for error paths and boundary conditions."
Also applies to: 85-171
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/toolsets/lvms/volumegroups_test.go` around lines 10 - 65, Add table-driven error-path cases to TestListLVMVolumeGroups (and the matching TestGetVolumeGroupNodeStatus) so the expectedError branch is actually exercised. Use the existing listLVMVolumeGroups and getVolumeGroupNodeStatus flows with fake dynamic client responses that trigger apierrors.IsNotFound and apierrors.IsForbidden, then assert the returned error text matches expectedError. Keep the current successful case, but extend the test table with namespace/object setups that cover the CRD-not-found and forbidden paths in volumegroups.go.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/toolsets/lvms/health.go`:
- Around line 222-223: Replace the `sb.WriteString(fmt.Sprintf(...))` calls in
the health report builder with `fmt.Fprintf` so the `strings.Builder` is written
to directly and the staticcheck QF1012 warning is removed. Update the same
pattern in the health status formatting logic around
`healthStatus(result.Healthy)` and the other matching call noted in the comment,
keeping the existing output unchanged while using `fmt.Fprintf(&sb, ...)`
instead of formatting into a temporary string first.
- Around line 121-151: The node scan in health.go is silently swallowing both
NodesDebugExec failures and JSON unmarshal errors inside the node loop, which
causes real execution/parse problems to look like a legitimate “no thin pools”
result. Update the health-check flow around the loop that calls NodesDebugExec
and parses lvsReport to track per-node failures (or at least the first
meaningful error), and return/report a distinct failure when all nodes fail for
exec or parsing rather than continuing to the generic empty-result path. Keep
the existing skip behavior only for expected “no LVM” cases, and surface the
actual error context for unexpected failures.
In `@pkg/toolsets/lvms/lvmcluster_test.go`:
- Around line 1-8: The tests are white-boxing private handlers instead of
exercising the public toolset API. Update the lvms tests to call the exported
surface exposed by Toolset.GetTools() and ServerTool.Handler, and remove direct
invocations of listLVMClusters and describeLVMCluster from the test flow. Keep
the assertions the same, but drive them through the public entry points so the
suite stays black-box and aligned with the testing guidelines.
In `@pkg/toolsets/lvms/manage.go`:
- Around line 114-151: The LVMCluster manifest assembly in manage.go is
vulnerable to YAML/field injection because `name`, `namespace`,
`deviceClassName`, `fstype`, and `devicePaths` are interpolated directly into
the template in the YAML-building block. Fix this by validating the identifiers
in the device selector/YAML construction path (the code around
`deviceSelectorYaml` and `yamlContent`) against Kubernetes-safe formats and safe
path patterns, and avoid manual string concatenation by constructing the
`LVMCluster` object as structured data and serializing it before
`ResourcesCreateOrUpdate`.
In `@pkg/toolsets/lvms/suite_test.go`:
- Around line 15-32: The mockKubernetesClient currently embeds a nil
api.KubernetesClient, so promoted calls like CoreV1() can still panic even
though Clientset() is set. Update mockKubernetesClient to forward the
api.KubernetesClient behavior to the underlying clientset used by
nodesdebug.NewNodeDebug, or replace the embedded field with a concrete fake that
fully implements api.KubernetesClient. Ensure the methods on
mockKubernetesClient and any setup in suite_test.go consistently use the fake
clientset for all delegated Kubernetes API calls.
In `@pkg/toolsets/lvms/troubleshoot.go`:
- Around line 77-102: The PVC lookup in lvmsTroubleshootHandler currently
becomes a silent no-op when only pvc or pvc_namespace is provided. Add input
validation or a user-facing hint before the fetchPVCDetails branch so incomplete
PVC arguments are surfaced instead of being ignored, and use the existing
lvmsTroubleshootHandler, pvcName, and pvcNamespace logic to detect and report
the mismatch.
---
Nitpick comments:
In `@pkg/toolsets/lvms/health.go`:
- Around line 262-264: The helper round2 in health.go is truncating values
instead of rounding them, so either change its implementation to use true
rounding for the displayed pool sizes or rename it to reflect truncation if that
behavior is intended. Update the caller expectations and the related
health_test.go case so the behavior matches the chosen name and logic, using
round2 as the symbol to locate the change.
In `@pkg/toolsets/lvms/lvmcluster_test.go`:
- Around line 10-175: Add test coverage for the Forbidden error path in both
`TestListLVMClusters` and `TestDescribeLVMCluster` by introducing a case that
makes `listLVMClusters` and `describeLVMCluster` return
`apierrors.IsForbidden`-matching errors from the dynamic client. Use the
existing helpers and symbols (`listLVMClusters`, `describeLVMCluster`,
`LVMSTestSuite`, `SetDynamicClient`) to assert the result contains the forbidden
message and verifies the explicit forbidden handling branch instead of the
generic error path.
In `@pkg/toolsets/lvms/lvmcluster.go`:
- Around line 79-82: The namespace extraction in lvms lvmcluster handling is
re-implementing the same type-assertion logic used elsewhere instead of
following the WrapParams pattern. Update the namespace reads in the affected
logic to use api.WrapParams(params).OptionalString(...) consistently, matching
manage.go and the surrounding parameter handling in lvmcluster.go so the same
helper is used at both call sites.
In `@pkg/toolsets/lvms/manage_test.go`:
- Around line 9-96: Add coverage for the missing create success path in
lvmsCreateCluster by adding a test that supplies a valid name and namespace,
uses the fake dynamic client, and asserts the returned content indicates
successful creation so the templating path in manage.go is exercised. Also add
Forbidden-branch tests for both lvmsCreateCluster and lvmsDeleteCluster by
configuring the client/errors to trigger apierrors.IsForbidden and asserting the
returned result/error message matches the forbidden case. Use the existing
lvmsCreateCluster and lvmsDeleteCluster helpers and the LVMSTestSuite setup to
keep the new tests consistent with the current suite.
In `@pkg/toolsets/lvms/manage.go`:
- Around line 115-122: The device_paths parsing in manageLVMS is bypassing the
existing WrapParams-based access pattern and reads params.GetArguments()
directly. Update the device_paths extraction to use the same p helper used for
name, namespace, and the other fields in this function, and use its slice/array
accessor if available so the lookup is consistent with the rest of manage.go.
In `@pkg/toolsets/lvms/pods.go`:
- Around line 214-252: The pod restart-count logic in `lvmsListPods` is
duplicated in `fetchLVMSPods` from `troubleshoot.go`, so extract the shared
container-status accumulation into a helper such as `sumPodRestarts(pod
unstructured.Unstructured) int64` and call it from both places. Keep the
existing pod-summary assembly in `lvmsListPods`, but delegate the restart
calculation to the helper so both code paths stay consistent and don’t drift.
In `@pkg/toolsets/lvms/suite_test.go`:
- Around line 1-77: The LVMS test suite currently relies on local mock types
(`mockKubernetesClient` and `mockToolCallRequest`) instead of real test setup,
which conflicts with the testing guideline. Update `LVMSTestSuite` to use
`setup-envtest`-backed Kubernetes client initialization and real
request/argument handling where possible, and remove or minimize the mock
scaffolding in `SetupTest`, `SetDynamicClient`, `SetClientset`, and
`mockToolCallRequest` so the suite exercises actual implementations.
In `@pkg/toolsets/lvms/troubleshoot_test.go`:
- Around line 25-93: The prompt metadata tests are over-copying fields into
anonymous structs, adding unnecessary boilerplate in
TestLvmsTroubleshootPromptMetadata, TestLvmsCapacityPromptMetadata, and
TestTroubleshootPromptArgumentsMetadata. Update the assertions to read directly
from the found prompt’s api.Prompt fields (for example, use
p.Prompt.Name/Title/Description and p.Prompt.Arguments[i].Name/Description
inside the loop) and remove the temporary anonymous-struct holders while keeping
the same expectations and prompt lookup logic.
In `@pkg/toolsets/lvms/troubleshoot.go`:
- Around line 344-367: The helper in fetchLVMClusterStatus is discarding
yaml.Marshal errors, which is inconsistent with the package’s error-handling
pattern. Update fetchLVMClusterStatus (and the similar fetchPVCDetails helper
mentioned in the review) to check the marshal return value instead of using the
blank identifier, and surface or log the error in the returned status text the
same way other helpers like volumegroups.go, pods.go, and health.go do. Use the
existing fetchLVMClusterStatus and fetchPVCDetails symbols to locate the
affected paths.
In `@pkg/toolsets/lvms/volumegroups_test.go`:
- Around line 10-65: Add table-driven error-path cases to
TestListLVMVolumeGroups (and the matching TestGetVolumeGroupNodeStatus) so the
expectedError branch is actually exercised. Use the existing listLVMVolumeGroups
and getVolumeGroupNodeStatus flows with fake dynamic client responses that
trigger apierrors.IsNotFound and apierrors.IsForbidden, then assert the returned
error text matches expectedError. Keep the current successful case, but extend
the test table with namespace/object setups that cover the CRD-not-found and
forbidden paths in volumegroups.go.
In `@pkg/toolsets/lvms/volumegroups.go`:
- Around line 83-110: The namespace handling in listLVMVolumeGroups is
inconsistent with the other LVMS tools: when namespace is omitted, it currently
lists cluster-wide instead of defaulting to defaults.DefaultNamespace(). Update
listLVMVolumeGroups (and any matching helper such as getVolumeGroupNodeStatus if
it uses the same pattern) to use the default LVMS namespace when
params.GetArguments()["namespace"] is empty, and keep the namespaced
Resource(...).Namespace(namespace).List path as the normal case.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0dde6e77-2a7a-4640-bdc4-4827da339333
📒 Files selected for processing (19)
README.mddocs/configuration.mdinternal/tools/update-readme/main.gopkg/mcp/modules.gopkg/toolsets/lvms/health.gopkg/toolsets/lvms/health_test.gopkg/toolsets/lvms/internal/defaults/defaults.gopkg/toolsets/lvms/lvmcluster.gopkg/toolsets/lvms/lvmcluster_test.gopkg/toolsets/lvms/manage.gopkg/toolsets/lvms/manage_test.gopkg/toolsets/lvms/pods.gopkg/toolsets/lvms/suite_test.gopkg/toolsets/lvms/toolset.gopkg/toolsets/lvms/toolset_test.gopkg/toolsets/lvms/troubleshoot.gopkg/toolsets/lvms/troubleshoot_test.gopkg/toolsets/lvms/volumegroups.gopkg/toolsets/lvms/volumegroups_test.go
8c3f548 to
7a2e9b4
Compare
7a2e9b4 to
5767bb7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
evals/tasks/lvms/diagnose-disk-not-used/task.yaml (1)
48-55: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGeneralize the node name in the prompt.
Hardcoding
worker-1as the node name could cause the evaluation task to spuriously fail if the test cluster does not have a node with that exact name, leading the agent to diagnose a missing node instead of evaluating LVMS filtering rules.🔧 Proposed fix for a generalized node reference
prompt: inline: |- - A user reports that /dev/sdb on node worker-1 is not being used by LVMS - even though they expected it to be available for storage. + A user reports that a specific disk (e.g., /dev/sdb on one of the worker nodes) + is not being used by LVMS even though they expected it to be available for storage. Investigate why this disk is not being used by LVMS and explain the reason to the user.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@evals/tasks/lvms/diagnose-disk-not-used/task.yaml` around lines 48 - 55, Generalize the node reference in the inline prompt instead of hardcoding “worker-1”; use a placeholder or variable consistent with the task’s existing input conventions while preserving the /dev/sdb investigation and LVMS filtering diagnosis.pkg/mcp/toolsets_test.go (1)
121-124: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse
Require().NoErrorfor setup operations to prevent panics.As per coding guidelines, use
s.Require().NoError(err)for setup operations that must succeed.Currently, if
ListToolsfails and returns aniltoolsobject,s.NoErrorinside this subtest will fail the subtest but will not abort the parent test. The subsequent subtest on line 125 will then panic with a nil pointer dereference when attempting to iterate overtools.Tools. Replace this subtest with required assertions in the parent scope.🛠️ Proposed fix
- s.Run("ListTools returns tools", func() { - s.NotNil(tools, "Expected tools from ListTools") - s.NoError(err, "Expected no error from ListTools") - }) + s.Require().NoError(err, "Expected no error from ListTools") + s.Require().NotNil(tools, "Expected tools from ListTools")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/mcp/toolsets_test.go` around lines 121 - 124, Replace the non-fatal assertions around the ListTools setup with s.Require().NoError(err) and a required non-nil check in the parent test scope, before any subtests use tools. Keep the existing ListTools success assertions and ensure failures abort before the later tools.Tools iteration.Source: Coding guidelines
🧹 Nitpick comments (2)
evals/tasks/lvms/check-thin-pool-utilization/task.yaml (1)
48-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign prompt with real user workflows.
The prompt dictates the specific properties (
data_percent,metadata_percent) and the exact resource exclusion ("not just the Kubernetes CRD status") to check. Based on learnings from the provided context (.agents/skills/toolset-design/SKILL.md), eval taskpromptfields should describe real user workflows and provide diagnostic data framing (not tool-shaped steps).♻️ Proposed fix to align with user workflows
prompt: inline: |- - What is the current thin pool utilization on each node? I need the actual - data_percent and metadata_percent values from the LVM thin pools, not just - the Kubernetes CRD status. + What is the true underlying thin pool utilization across the cluster nodes? + Please check the actual LVM thin pool metrics, as the Kubernetes CRD status + may not provide real-time or complete data.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@evals/tasks/lvms/check-thin-pool-utilization/task.yaml` around lines 48 - 53, Revise the task.yaml prompt to describe the user’s goal of assessing current thin-pool utilization across nodes and requesting actionable diagnostic context, without prescribing exact field names such as data_percent or metadata_percent or explicitly excluding Kubernetes CRD status. Keep the prompt focused on the real workflow and expected utilization assessment rather than tool-specific retrieval instructions.README.md (1)
506-508: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the generator to omit empty
<details>blocks.The
update-readme-toolsgenerator produces an empty<details>block for toolsets likelvmsbecause they do not export any MCP tools (GetToolsreturnsnil).Consider updating the
internal/tools/update-readme/main.goscript in a future PR to skip generating<details>blocks for toolsets that have no tools. As per coding guidelines, do not manually edit this generated file to fix the visual glitch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 506 - 508, Update the README generation logic in the update-readme generator, specifically the toolset rendering flow in internal/tools/update-readme/main.go, to skip emitting <details> blocks when GetTools returns nil or an otherwise empty tool list. Do not manually modify the generated README; regenerate it after fixing the generator so toolsets such as lvms produce no empty block.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@evals/tasks/lvms/check-lvmcluster-status/task.yaml`:
- Around line 25-27: Update the LVMCluster existence check around the kubectl
get pipeline so it tests whether the command produces a non-empty resource name,
rather than relying on the pipeline exit status. Preserve the existing behavior
of creating the test cluster when no LVMCluster exists.
---
Outside diff comments:
In `@evals/tasks/lvms/diagnose-disk-not-used/task.yaml`:
- Around line 48-55: Generalize the node reference in the inline prompt instead
of hardcoding “worker-1”; use a placeholder or variable consistent with the
task’s existing input conventions while preserving the /dev/sdb investigation
and LVMS filtering diagnosis.
In `@pkg/mcp/toolsets_test.go`:
- Around line 121-124: Replace the non-fatal assertions around the ListTools
setup with s.Require().NoError(err) and a required non-nil check in the parent
test scope, before any subtests use tools. Keep the existing ListTools success
assertions and ensure failures abort before the later tools.Tools iteration.
---
Nitpick comments:
In `@evals/tasks/lvms/check-thin-pool-utilization/task.yaml`:
- Around line 48-53: Revise the task.yaml prompt to describe the user’s goal of
assessing current thin-pool utilization across nodes and requesting actionable
diagnostic context, without prescribing exact field names such as data_percent
or metadata_percent or explicitly excluding Kubernetes CRD status. Keep the
prompt focused on the real workflow and expected utilization assessment rather
than tool-specific retrieval instructions.
In `@README.md`:
- Around line 506-508: Update the README generation logic in the update-readme
generator, specifically the toolset rendering flow in
internal/tools/update-readme/main.go, to skip emitting <details> blocks when
GetTools returns nil or an otherwise empty tool list. Do not manually modify the
generated README; regenerate it after fixing the generator so toolsets such as
lvms produce no empty block.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 257a9ef6-bd83-4e04-8ee4-fba85684f182
📒 Files selected for processing (20)
README.mddocs/configuration.mdevals/claude-code/eval.yamlevals/openai-agent/eval.yamlevals/tasks/lvms/README.mdevals/tasks/lvms/check-capacity/task.yamlevals/tasks/lvms/check-lvmcluster-status/task.yamlevals/tasks/lvms/check-thin-pool-utilization/task.yamlevals/tasks/lvms/diagnose-disk-not-used/task.yamlevals/tasks/lvms/diagnose-no-space-left/task.yamlevals/tasks/lvms/forced-cleanup/task.yamlevals/tasks/lvms/list-node-block-devices/task.yamlevals/tasks/lvms/show-lvm-physical-volumes/task.yamlevals/tasks/lvms/show-volume-groups/task.yamlevals/tasks/lvms/troubleshoot-prompt/task.yamlevals/tasks/lvms/troubleshoot-stuck-pvc/task.yamlinternal/tools/update-readme/main.gopkg/mcp/modules.gopkg/mcp/testdata/toolsets-lvms-tools.jsonpkg/mcp/toolsets_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/mcp/modules.go
- internal/tools/update-readme/main.go
0e6b0ed to
6331136
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
pkg/toolsets/lvms/lvms_capacity.go (1)
175-181: 🎯 Functional Correctness | 🔵 Trivial | ⚖️ Poor tradeoffIgnored errors from
unstructured.Nested*calls (same pattern aslvms_troubleshoot.go).Lines 175-176, 180-181, 209-210, 230, 235-236, 275, 281 all discard the
errorreturn fromunstructured.NestedString/NestedMap. As per coding guidelines,**/*.go: "Never ignore errors in production code; always check and handle returned errors."Also applies to: 209-236, 275-281
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/toolsets/lvms/lvms_capacity.go` around lines 175 - 181, The LVMS capacity parsing flow discards errors from unstructured.NestedString and NestedMap calls. Update the relevant extraction logic in the capacity-processing function to capture and handle each returned error, including the storageClassName, capacity, nodeTopology, and later Nested* calls around the referenced sections, using the existing error-handling convention rather than silently continuing.Source: Coding guidelines
pkg/toolsets/lvms/toolset_test.go (1)
1-81: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing unit coverage for pure helper functions.
This file only exercises the
Toolsetstruct's plumbing; none of the pure helpers inlvms_capacity.go/lvms_troubleshoot.go(parseCapacity,formatCapacity,getPodReadyAndRestarts,filterLogLines,valueOrNA) have tests. These require no mocks/fixtures and, per this review,parseCapacitycurrently has a real unit-conflation bug (Line 332-341 oflvms_capacity.go) that a table-driven test would have caught. As per coding guidelines,**/*_test.go: "Aim for high public-API coverage and include edge cases such as nil or empty inputs, invalid values, type mismatches, and malformed input."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/toolsets/lvms/toolset_test.go` around lines 1 - 81, Add table-driven unit tests covering the pure helpers parseCapacity, formatCapacity, getPodReadyAndRestarts, filterLogLines, and valueOrNA, including valid, empty or nil, invalid, type-mismatch, and malformed inputs where applicable. Assert the expected conversions and formatting, including distinct capacity units so the parseCapacity unit-conflation bug is detected, while keeping the tests independent of mocks and fixtures.Source: Coding guidelines
pkg/toolsets/lvms/lvms_troubleshoot.go (1)
340-385: 🎯 Functional Correctness | 🔵 Trivial | ⚖️ Poor tradeoffPervasive ignored errors from
unstructured.Nested*helpers across the LVMS package. Both files repeatedly discard the third (error) return value ofunstructured.NestedString/NestedSlice/NestedMap, violating the guideline**/*.go: "Never ignore errors in production code; always check and handle returned errors." One shared root cause: the fetch helpers rely onfoundalone and never inspecterr.
pkg/toolsets/lvms/lvms_troubleshoot.go#L340-L385: check/handle the discarded errors infetchLVMClusterStatus(Lines 344, 348, 375) and mirror the fix infetchVolumeGroupNodeStatus,fetchVGManagerPods,fetchOperatorHealth,fetchTopoLVMStorageClasses,fetchTopoLVMPVCs(Lines 412, 448-449, 606-607, 625, 649, 658, 694, 714, 719-720).pkg/toolsets/lvms/lvms_capacity.go#L175-L281: check/handle the discarded errors infetchCSICapacity(Lines 175-176, 180-181) andfetchPVCCapacitySummary(Lines 209-210, 230, 235-236).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/toolsets/lvms/lvms_troubleshoot.go` around lines 340 - 385, Handle the error returns from every unstructured.NestedString, NestedSlice, and NestedMap call instead of relying only on found. In pkg/toolsets/lvms/lvms_troubleshoot.go:340-385, update fetchLVMClusterStatus and mirror the same handling in fetchVolumeGroupNodeStatus, fetchVGManagerPods, fetchOperatorHealth, fetchTopoLVMStorageClasses, and fetchTopoLVMPVCs; in pkg/toolsets/lvms/lvms_capacity.go:175-281, update fetchCSICapacity and fetchPVCCapacitySummary. Propagate or report errors using each function’s existing error-handling behavior before consuming the fetched values.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@evals/claude-code/eval.yaml`:
- Around line 87-97: Update the LVMS task assertions in
evals/claude-code/eval.yaml lines 87-97 and evals/openai-agent/eval.yaml lines
85-95 to explicitly require the lvms-troubleshoot prompt/tool for the
troubleshoot-prompt task path, while preserving the existing generic Kubernetes
call limits for other LVMS tasks.
In `@pkg/toolsets/lvms/lvms_capacity.go`:
- Around line 285-304: The node-affinity lookup currently uses NestedMap through
nodeSelectorTerms, which is a slice, so the capacity update is never reached. In
the node-affinity handling block, read the required node selector terms with the
appropriate nested-slice accessor, then iterate the returned terms and preserve
the existing topology.topolvm.io/node match and nodeCapacity update logic.
- Around line 322-344: Replace the manual parsing in parseCapacity with
Kubernetes resource.ParseQuantity, returning the quantity’s byte value when
parsing succeeds. Handle the returned error by preserving the existing
zero-value fallback, and remove the fmt/string-based unit parsing so decimal,
binary, and exponent formats are interpreted correctly.
---
Nitpick comments:
In `@pkg/toolsets/lvms/lvms_capacity.go`:
- Around line 175-181: The LVMS capacity parsing flow discards errors from
unstructured.NestedString and NestedMap calls. Update the relevant extraction
logic in the capacity-processing function to capture and handle each returned
error, including the storageClassName, capacity, nodeTopology, and later Nested*
calls around the referenced sections, using the existing error-handling
convention rather than silently continuing.
In `@pkg/toolsets/lvms/lvms_troubleshoot.go`:
- Around line 340-385: Handle the error returns from every
unstructured.NestedString, NestedSlice, and NestedMap call instead of relying
only on found. In pkg/toolsets/lvms/lvms_troubleshoot.go:340-385, update
fetchLVMClusterStatus and mirror the same handling in
fetchVolumeGroupNodeStatus, fetchVGManagerPods, fetchOperatorHealth,
fetchTopoLVMStorageClasses, and fetchTopoLVMPVCs; in
pkg/toolsets/lvms/lvms_capacity.go:175-281, update fetchCSICapacity and
fetchPVCCapacitySummary. Propagate or report errors using each function’s
existing error-handling behavior before consuming the fetched values.
In `@pkg/toolsets/lvms/toolset_test.go`:
- Around line 1-81: Add table-driven unit tests covering the pure helpers
parseCapacity, formatCapacity, getPodReadyAndRestarts, filterLogLines, and
valueOrNA, including valid, empty or nil, invalid, type-mismatch, and malformed
inputs where applicable. Assert the expected conversions and formatting,
including distinct capacity units so the parseCapacity unit-conflation bug is
detected, while keeping the tests independent of mocks and fixtures.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0c30e54d-f71a-4a56-91c7-b8fa797be1ac
📒 Files selected for processing (24)
README.mddocs/configuration.mdevals/claude-code/eval.yamlevals/openai-agent/eval.yamlevals/tasks/lvms/README.mdevals/tasks/lvms/check-capacity/task.yamlevals/tasks/lvms/check-lvmcluster-status/task.yamlevals/tasks/lvms/check-thin-pool-utilization/task.yamlevals/tasks/lvms/diagnose-disk-not-used/task.yamlevals/tasks/lvms/diagnose-no-space-left/task.yamlevals/tasks/lvms/forced-cleanup/task.yamlevals/tasks/lvms/list-node-block-devices/task.yamlevals/tasks/lvms/show-lvm-physical-volumes/task.yamlevals/tasks/lvms/show-volume-groups/task.yamlevals/tasks/lvms/troubleshoot-prompt/task.yamlevals/tasks/lvms/troubleshoot-stuck-pvc/task.yamlinternal/tools/update-readme/main.gopkg/mcp/modules.gopkg/mcp/testdata/toolsets-lvms-tools.jsonpkg/mcp/toolsets_test.gopkg/toolsets/lvms/lvms_capacity.gopkg/toolsets/lvms/lvms_troubleshoot.gopkg/toolsets/lvms/toolset.gopkg/toolsets/lvms/toolset_test.go
🚧 Files skipped from review as they are similar to previous changes (16)
- evals/tasks/lvms/diagnose-disk-not-used/task.yaml
- evals/tasks/lvms/list-node-block-devices/task.yaml
- pkg/mcp/modules.go
- evals/tasks/lvms/show-lvm-physical-volumes/task.yaml
- evals/tasks/lvms/diagnose-no-space-left/task.yaml
- evals/tasks/lvms/show-volume-groups/task.yaml
- pkg/mcp/toolsets_test.go
- pkg/mcp/testdata/toolsets-lvms-tools.json
- internal/tools/update-readme/main.go
- evals/tasks/lvms/forced-cleanup/task.yaml
- evals/tasks/lvms/check-capacity/task.yaml
- evals/tasks/lvms/check-lvmcluster-status/task.yaml
- evals/tasks/lvms/troubleshoot-prompt/task.yaml
- evals/tasks/lvms/README.md
- evals/tasks/lvms/check-thin-pool-utilization/task.yaml
- evals/tasks/lvms/troubleshoot-stuck-pvc/task.yaml
8dd0ba7 to
66b17ee
Compare
dhensel-rh
left a comment
dhensel-rh
left a comment
There was a problem hiding this comment.
The heredoc YAML inside the indented bash setup scripts will break kubectl apply — the leading whitespace becomes part of the YAML content. For example in check-lvmcluster-status/task.yaml:
cat <<EOF | kubectl apply -f -
apiVersion: lvm.topolvm.io/v1alpha1
kind: LVMClusterThat indentation produces invalid YAML. Needs to use <<-EOF with tabs, or dedent the YAML block to column 0.
Same issue in forced-cleanup/task.yaml with the LogicalVolume heredoc.
66b17ee to
20fe635
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@evals/tasks/lvms/list-node-block-devices/task.yaml`:
- Around line 24-34: Update the verify step in the task configuration so it
evaluates the captured agent transcript instead of unconditionally exiting
successfully. Assert that the output demonstrates running lsblk and listing node
disk devices, partitions, and sizes via node access; alternatively, replace or
supplement the inline verifier with an llmJudge check covering those
requirements.
- Around line 1-2: Update the task’s verify step in the YAML so it evaluates the
produced answer against the expected block-device results and exits with a
nonzero status when they differ, rather than only printing expectations and
returning success. Preserve successful verification for correct answers and keep
the change scoped to the verify script.
In `@pkg/toolsets/lvms/lvms_capacity.go`:
- Around line 176-177: Handle the error return from every unstructured lookup in
pkg/toolsets/lvms/lvms_capacity.go: storageClassName and capacity at lines
176-177, nodeTopology at 181, provisioner at 210, storageClassName at 231, phase
and requestedStorage at 236-237, driver at 277, storage at 283, and
nodeSelectorTerms at 288. Replace discarded errors in the relevant
capacity-reporting flow with appropriate handling, such as logging through
klogutil.FromContext(ctx) or returning an error message, while preserving
successful value extraction.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9863f96f-dbcc-4628-af15-fd33d20f48c1
📒 Files selected for processing (24)
README.mddocs/configuration.mdevals/claude-code/eval.yamlevals/openai-agent/eval.yamlevals/tasks/lvms/README.mdevals/tasks/lvms/check-capacity/task.yamlevals/tasks/lvms/check-lvmcluster-status/task.yamlevals/tasks/lvms/check-thin-pool-utilization/task.yamlevals/tasks/lvms/diagnose-disk-not-used/task.yamlevals/tasks/lvms/diagnose-no-space-left/task.yamlevals/tasks/lvms/forced-cleanup/task.yamlevals/tasks/lvms/list-node-block-devices/task.yamlevals/tasks/lvms/show-lvm-physical-volumes/task.yamlevals/tasks/lvms/show-volume-groups/task.yamlevals/tasks/lvms/troubleshoot-prompt/task.yamlevals/tasks/lvms/troubleshoot-stuck-pvc/task.yamlinternal/tools/update-readme/main.gopkg/mcp/modules.gopkg/mcp/testdata/toolsets-lvms-tools.jsonpkg/mcp/toolsets_test.gopkg/toolsets/lvms/lvms_capacity.gopkg/toolsets/lvms/lvms_troubleshoot.gopkg/toolsets/lvms/toolset.gopkg/toolsets/lvms/toolset_test.go
🚧 Files skipped from review as they are similar to previous changes (20)
- pkg/mcp/modules.go
- evals/tasks/lvms/show-lvm-physical-volumes/task.yaml
- evals/openai-agent/eval.yaml
- pkg/mcp/testdata/toolsets-lvms-tools.json
- evals/claude-code/eval.yaml
- evals/tasks/lvms/diagnose-no-space-left/task.yaml
- evals/tasks/lvms/troubleshoot-prompt/task.yaml
- evals/tasks/lvms/check-thin-pool-utilization/task.yaml
- evals/tasks/lvms/diagnose-disk-not-used/task.yaml
- evals/tasks/lvms/README.md
- evals/tasks/lvms/show-volume-groups/task.yaml
- evals/tasks/lvms/forced-cleanup/task.yaml
- pkg/mcp/toolsets_test.go
- evals/tasks/lvms/check-capacity/task.yaml
- pkg/toolsets/lvms/toolset.go
- evals/tasks/lvms/check-lvmcluster-status/task.yaml
- pkg/toolsets/lvms/toolset_test.go
- evals/tasks/lvms/troubleshoot-stuck-pvc/task.yaml
- README.md
- pkg/toolsets/lvms/lvms_troubleshoot.go
| kind: Task | ||
| metadata: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n '^apiVersion: mcpchecker/v1alpha2$|^kind: Task$' evals/tasksRepository: openshift/openshift-mcp-server
Length of output: 20670
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== task file =="
cat -n evals/tasks/lvms/list-node-block-devices/task.yaml
echo
echo "== nearby similar task examples =="
for f in \
evals/tasks/lvms/troubleshoot-stuck-pvc/task.yaml \
evals/tasks/lvms/show-volume-groups/task.yaml \
evals/tasks/lvms/check-capacity/task.yaml \
evals/tasks/lvms/check-lvmcluster-status/task.yaml
do
echo "--- $f"
cat -n "$f" | sed -n '1,30p'
doneRepository: openshift/openshift-mcp-server
Length of output: 6307
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('evals/tasks/lvms/list-node-block-devices/task.yaml')
text = p.read_text()
print('apiVersion present:', 'apiVersion:' in text)
print('verify step mentions exit 0:', 'exit 0' in text)
print('verify step mentions result:', 'verify' in text.lower())
PYRepository: openshift/openshift-mcp-server
Length of output: 262
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== references to mcpchecker/v1alpha2 =="
rg -n --hidden --glob '!**/.git/**' 'mcpchecker/v1alpha2|apiVersion:' README* docs evals . 2>/dev/null | sed -n '1,200p'
echo
echo "== task schema/docs files =="
fd -a -t f 'schema|task|mcpchecker|README' . | sed -n '1,200p'Repository: openshift/openshift-mcp-server
Length of output: 30682
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== evals/tasks/README.md =="
cat -n evals/tasks/README.md | sed -n '1,220p'
echo
echo "== evals/core-eval-testing/README.md =="
cat -n evals/core-eval-testing/README.md | sed -n '1,220p'
echo
echo "== pkg/kubernetes/schema_validator.go =="
cat -n pkg/kubernetes/schema_validator.go | sed -n '1,260p'Repository: openshift/openshift-mcp-server
Length of output: 11000
Make the verify step fail on incorrect answers (evals/tasks/lvms/list-node-block-devices/task.yaml:24-34). The script only prints expectations and exits 0, so incorrect results still pass.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@evals/tasks/lvms/list-node-block-devices/task.yaml` around lines 1 - 2,
Update the task’s verify step in the YAML so it evaluates the produced answer
against the expected block-device results and exits with a nonzero status when
they differ, rather than only printing expectations and returning success.
Preserve successful verification for correct answers and keep the change scoped
to the verify script.
| verify: | ||
| inline: |- | ||
| #!/usr/bin/env bash | ||
| echo "=== Verification ===" | ||
| echo "The agent should have:" | ||
| echo " 1. Run 'lsblk' command on the node" | ||
| echo " 2. Shown disk devices, partitions, sizes" | ||
| echo " 3. This CANNOT be done with Kubernetes API - requires node access" | ||
| echo "" | ||
| echo "If the agent only queried Kubernetes resources, it FAILED." | ||
| exit 0 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf '\n== task file ==\n'
cat -n evals/tasks/lvms/list-node-block-devices/task.yaml | sed -n '1,120p'
printf '\n== similar verify patterns ==\n'
rg -n "inline: \|-|exit 0|llmJudge|Verification" evals/tasks -g 'task.yaml' | sed -n '1,200p'Repository: openshift/openshift-mcp-server
Length of output: 14021
Make the verifier check the agent output evals/tasks/lvms/list-node-block-devices/task.yaml:24-34 — this step always exits 0, so the task passes even when the agent does nothing. Replace it with an assertion on the captured transcript/output, or add an llmJudge step that checks the agent listed node block devices via node access.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@evals/tasks/lvms/list-node-block-devices/task.yaml` around lines 24 - 34,
Update the verify step in the task configuration so it evaluates the captured
agent transcript instead of unconditionally exiting successfully. Assert that
the output demonstrates running lsblk and listing node disk devices, partitions,
and sizes via node access; alternatively, replace or supplement the inline
verifier with an llmJudge check covering those requirements.
| storageClass, _, _ := unstructured.NestedString(cap.Object, "storageClassName") | ||
| capacity, _, _ := unstructured.NestedString(cap.Object, "capacity") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Handle errors from unstructured.Nested* functions.
As per coding guidelines, "Never ignore errors in production code; always check and handle returned errors." The unstructured.NestedString, NestedMap, and NestedSlice methods return a third error value when the field exists but is not of the expected type. Discarding this error with the blank identifier _ can silently hide type-mismatches and drop data from the report. Please check these errors and handle them appropriately (e.g., by logging with klogutil.FromContext(ctx) or returning an error message).
pkg/toolsets/lvms/lvms_capacity.go#L176-L177: handle the error forstorageClassNameandcapacity.pkg/toolsets/lvms/lvms_capacity.go#L181-L181: handle the error fornodeTopology.pkg/toolsets/lvms/lvms_capacity.go#L210-L210: handle the error forprovisioner.pkg/toolsets/lvms/lvms_capacity.go#L231-L231: handle the error forstorageClassName.pkg/toolsets/lvms/lvms_capacity.go#L236-L237: handle the error forphaseandrequestedStorage.pkg/toolsets/lvms/lvms_capacity.go#L277-L277: handle the error fordriver.pkg/toolsets/lvms/lvms_capacity.go#L283-L283: handle the error forstorage.pkg/toolsets/lvms/lvms_capacity.go#L288-L288: handle the error fornodeSelectorTerms.
📍 Affects 1 file
pkg/toolsets/lvms/lvms_capacity.go#L176-L177(this comment)pkg/toolsets/lvms/lvms_capacity.go#L181-L181pkg/toolsets/lvms/lvms_capacity.go#L210-L210pkg/toolsets/lvms/lvms_capacity.go#L231-L231pkg/toolsets/lvms/lvms_capacity.go#L236-L237pkg/toolsets/lvms/lvms_capacity.go#L277-L277pkg/toolsets/lvms/lvms_capacity.go#L283-L283pkg/toolsets/lvms/lvms_capacity.go#L288-L288
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/toolsets/lvms/lvms_capacity.go` around lines 176 - 177, Handle the error
return from every unstructured lookup in pkg/toolsets/lvms/lvms_capacity.go:
storageClassName and capacity at lines 176-177, nodeTopology at 181, provisioner
at 210, storageClassName at 231, phase and requestedStorage at 236-237, driver
at 277, storage at 283, and nodeSelectorTerms at 288. Replace discarded errors
in the relevant capacity-reporting flow with appropriate handling, such as
logging through klogutil.FromContext(ctx) or returning an error message, while
preserving successful value extraction.
Source: Coding guidelines
|
@mmakwana30: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Jira: OCPEDGE-2726
Added a new toolset for diagnosing LVMS (Logical Volume Manager Storage) issues on OpenShift clusters.
Prompts (2):
Summary by CodeRabbit
Summary by CodeRabbit
New Features
lvms) toolset with prompt-based troubleshooting and an LVMS capacity report.Documentation
lvmsand its available tools.Tests
lvmsprompts and registrations.