feat(storage): add csi driver health tests#523
Conversation
📝 WalkthroughWalkthroughAdds a new ChangesCSI Driver Health Check
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant k8s_storage_suite as k8s_storage suite
participant K8sCsiDriverHealthCheck
participant kubectl
participant CSIDriver
participant Deployment
participant DaemonSet
k8s_storage_suite->>K8sCsiDriverHealthCheck: run() with configured drivers
K8sCsiDriverHealthCheck->>kubectl: get storageclass -o json
kubectl-->>K8sCsiDriverHealthCheck: StorageClass list
K8sCsiDriverHealthCheck->>CSIDriver: verify registered provisioner
CSIDriver-->>K8sCsiDriverHealthCheck: registration result
alt workloads configured
K8sCsiDriverHealthCheck->>Deployment: fetch controller status
Deployment-->>K8sCsiDriverHealthCheck: replicas and readyReplicas
K8sCsiDriverHealthCheck->>DaemonSet: fetch node status
DaemonSet-->>K8sCsiDriverHealthCheck: desired, available, ready counts
else workloads missing
K8sCsiDriverHealthCheck->>K8sCsiDriverHealthCheck: skip controller/node subtests
end
K8sCsiDriverHealthCheck-->>k8s_storage_suite: subtest results
Related PRs: None identified 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
isvtest/tests/test_k8s_storage.py (1)
2214-2215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd docstrings to the non-test helpers.
_make()and_workloads()are helper methods, not test entrypoints, so they still need PEP 257 docstrings under the repo's Python rules. As per coding guidelines, "Every function and class must have docstrings following PEP 257".Also applies to: 2254-2264
🤖 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 `@isvtest/tests/test_k8s_storage.py` around lines 2214 - 2215, The non-test helper methods _make() and _workloads() in the K8s CSI driver test helpers are missing required PEP 257 docstrings. Add concise docstrings directly on those helper definitions (and any related helper in the referenced _workloads block) so they comply with the repo rule that every function and class must have docstrings.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 `@isvtest/src/isvtest/validations/k8s_storage.py`:
- Around line 2144-2149: The readiness check in the deployment validation still
enforces full rollout by comparing ready replicas to spec.replicas, which
conflicts with the intended min_controller_replicas threshold. Update the logic
in the Deployment validation path to use min_controller_replicas as the
readiness comparison after the existing desired < min_controller_replicas guard,
and adjust the failure message in that same block to reflect the threshold-based
check using the deployment validation symbols like desired, ready, and
min_replicas.
- Around line 2050-2053: The storage class aggregation in k8s_storage validation
is collapsing duplicate entries too early, causing later workloads for the same
class to be ignored before conflict detection. Update the logic around the
storage_classes loop in the validation path so duplicate StorageClass specs are
preserved until after provisioner grouping, and let the per-provisioner conflict
checks see all entries. Use the sc_to_workloads aggregation and the downstream
conflict-detection code to keep behavior consistent, and add a regression test
covering the same-StorageClass but different workloads/controller-node names
case.
---
Nitpick comments:
In `@isvtest/tests/test_k8s_storage.py`:
- Around line 2214-2215: The non-test helper methods _make() and _workloads() in
the K8s CSI driver test helpers are missing required PEP 257 docstrings. Add
concise docstrings directly on those helper definitions (and any related helper
in the referenced _workloads block) so they comply with the repo rule that every
function and class must have docstrings.
🪄 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: 8630636e-4f84-4cee-aa80-1ead975010e6
📒 Files selected for processing (3)
isvctl/configs/suites/k8s.yamlisvtest/src/isvtest/validations/k8s_storage.pyisvtest/tests/test_k8s_storage.py
|
/ok to test de577c1 |
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-07-02 17:23:32 UTC | Commit: de577c1 |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
isvtest/src/isvtest/validations/k8s_storage.py (2)
2049-2052: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDuplicate StorageClass entries are still collapsed before conflict detection.
The
sc and sc not in sc_to_workloadsguard keeps only the firstworkloadsblock when the same StorageClass appears across multipledriversspecs. Two specs for the same class with different controller/node names silently validate whichever entry appears first instead of tripping the per-provisioner conflict path. Preserve duplicate specs until after provisioner grouping.🤖 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 `@isvtest/src/isvtest/validations/k8s_storage.py` around lines 2049 - 2052, The StorageClass aggregation in k8s_storage validation is collapsing duplicate entries too early, so only the first workloads block is kept for the same class. Update the logic around the storage_classes loop in the validation path so duplicate StorageClass specs are preserved until after provisioner grouping, letting the existing per-provisioner conflict checks compare all driver specs. Adjust the sc_to_workloads population in the relevant k8s_storage validation function to avoid the sc and sc not in sc_to_workloads dedupe behavior.
2143-2148: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReadiness check still enforces full rollout instead of
min_controller_replicas.After the
desired < min_replicasguard,ready != desiredfails any controller above the configured minimum but not fully rolled out (e.g.min_controller_replicas: 1, 3-replica Deployment with 2 Ready). Comparereadyagainstmin_replicas, or rename the setting/docs if full readiness is the intended requirement.🤖 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 `@isvtest/src/isvtest/validations/k8s_storage.py` around lines 2143 - 2148, The readiness check in the deployment validation is still comparing `ready` to `desired` after the `desired < min_replicas` guard, which makes `min_controller_replicas` ineffective. Update the logic in the deployment validation path to compare `ready` against `min_replicas` instead of `spec.replicas`, or, if full rollout is intended, rename `min_controller_replicas` and adjust the related message/docs to match the actual requirement. Use the existing deployment check block and the `desired`, `ready`, and `min_replicas` variables to keep the behavior consistent.
🤖 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.
Duplicate comments:
In `@isvtest/src/isvtest/validations/k8s_storage.py`:
- Around line 2049-2052: The StorageClass aggregation in k8s_storage validation
is collapsing duplicate entries too early, so only the first workloads block is
kept for the same class. Update the logic around the storage_classes loop in the
validation path so duplicate StorageClass specs are preserved until after
provisioner grouping, letting the existing per-provisioner conflict checks
compare all driver specs. Adjust the sc_to_workloads population in the relevant
k8s_storage validation function to avoid the sc and sc not in sc_to_workloads
dedupe behavior.
- Around line 2143-2148: The readiness check in the deployment validation is
still comparing `ready` to `desired` after the `desired < min_replicas` guard,
which makes `min_controller_replicas` ineffective. Update the logic in the
deployment validation path to compare `ready` against `min_replicas` instead of
`spec.replicas`, or, if full rollout is intended, rename
`min_controller_replicas` and adjust the related message/docs to match the
actual requirement. Use the existing deployment check block and the `desired`,
`ready`, and `min_replicas` variables to keep the behavior consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0d147f15-a9fb-48a1-865c-613e6b813d3a
📒 Files selected for processing (2)
isvtest/src/isvtest/validations/k8s_storage.pyisvtest/tests/test_k8s_storage.py
🚧 Files skipped from review as they are similar to previous changes (1)
- isvtest/tests/test_k8s_storage.py
Signed-off-by: Alexandra Bueno <abueno@nvidia.com>
Signed-off-by: Alexandra Bueno <abueno@nvidia.com>
abueno-nvidia
force-pushed
the
storage-acceptance-tests/csi-driver-health
branch
from
6a7d185 to
e4bcdfe
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
isvtest/tests/test_k8s_storage.py (1)
2266-2477: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComprehensive coverage; consider extracting shared SC/CSIDriver fixture data.
Tests are thorough and assertions match the implementation's semantics (skip vs. fail, conflict detection, min-replica threshold, daemonset health). The
sc_items=[{"metadata": {"name": "ebs"}, ...}]/csidrivers={"ebs.csi.aws.com": {...}}pair is repeated verbatim across several tests (e.g. Lines 2298-2304, 2346-2352, 2367-2373, 2383-2389, 2399-2405). A small helper/fixture returning this pair could reduce duplication, but this is a nice-to-have given the tests are already clear and independent.🤖 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 `@isvtest/tests/test_k8s_storage.py` around lines 2266 - 2477, Extract the repeated StorageClass and CSIDriver test data used across the k8s storage tests into a shared helper or fixture, and update the affected cases to call it instead of inlining the same sc_items/csidrivers dictionaries. Keep the helper close to the existing test class so it’s easy to find alongside _make and _run, and preserve each test’s unique overrides for workloads, deployments, or daemonsets.
🤖 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.
Nitpick comments:
In `@isvtest/tests/test_k8s_storage.py`:
- Around line 2266-2477: Extract the repeated StorageClass and CSIDriver test
data used across the k8s storage tests into a shared helper or fixture, and
update the affected cases to call it instead of inlining the same
sc_items/csidrivers dictionaries. Keep the helper close to the existing test
class so it’s easy to find alongside _make and _run, and preserve each test’s
unique overrides for workloads, deployments, or daemonsets.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b26daf6b-af7b-4a1c-a816-a0f53b63c3ec
📒 Files selected for processing (3)
isvctl/configs/suites/k8s.yamlisvtest/src/isvtest/validations/k8s_storage.pyisvtest/tests/test_k8s_storage.py
🚧 Files skipped from review as they are similar to previous changes (2)
- isvctl/configs/suites/k8s.yaml
- isvtest/src/isvtest/validations/k8s_storage.py
|
/ok to test e4bcdfe |
2 participants
K8sCsiDriverHealthCheck
What it tests: Verifies that the CSI drivers backing the configured StorageClasses are installed and running in the cluster.
For each unique provisioner derived from the configured StorageClasses, it runs:
storageclass-found[<sc>]spec.provisioner)csidriver-registered[<provisioner>]CSIDriver/<provisioner>object exists in the clustercontroller-deployment-healthy[<provisioner>]spec.replicas >= min_controller_replicas(default 1) and all replicas Readynode-daemonset-healthy[<provisioner>]Success criteria: Every configured StorageClass resolves to a registered
CSIDriver; the controller and node workload subtests pass when aworkloadsblock is configured (otherwise they are reported as skipped, not failed).Configuration notes:
driversis a list of specs, each anchored on one or morestorage_classes. The whole check skips (passes) when no StorageClasses resolve.workloadsblock (namespace,controller.deployment,node.daemonset) enables the controller/node health subtests. If omitted, it checks sc registration only.min_controller_replicas(default1) sets the minimum Ready controller replicas.Summary by CodeRabbit
New Features
Bug Fixes
Tests