add LVMS toolset for storage diagnostics#370
Conversation
📝 WalkthroughWalkthroughAdds a new ChangesLVMS Toolset
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 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
Add a new toolset for diagnosing LVMS (Logical Volume Manager Storage) issues on OpenShift clusters. LVMS troubleshooting requires correlating information across LVM, Kubernetes, and LVMS-specific CRDs - expertise that rarely exists at edge sites. Tools (10): - list_lvmclusters, describe_lvmcluster: LVMCluster CRD inspection - list_lvmvolumegroups, get_volumegroup_node_status: VG status - lvms_list_pods, lvms_get_operator_logs, lvms_get_vgmanager_logs: Pod/log inspection - lvms_create_cluster, lvms_delete_cluster: Cluster management - lvms_check_thin_pool_health: Thin pool capacity monitoring Prompts (2): - lvms-troubleshoot: Step-by-step troubleshooting guide - lvms-capacity: Capacity analysis and planning
mmakwana30
force-pushed
the
add-lvms-toolset
branch
from
8c3f548 to
7a2e9b4
Compare
|
@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. |
1 participant
Jira: OCPEDGE-2726
Added a new toolset for diagnosing LVMS (Logical Volume Manager Storage) issues on OpenShift clusters.
Tools (10):
Prompts (2):
Summary by CodeRabbit