STOR-3090: Add CSI storage test for pod delete after host umount of mounted volume - #31488
STOR-3090: Add CSI storage test for pod delete after host umount of mounted volume#31488radeore wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@radeore: This pull request references STOR-3090 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughAdds an optional OpenShift CSI test suite. The suite dynamically provisions a volume, mounts it in a pod, unmounts it on the host, removes the volume directory, and verifies successful pod deletion. ChangesCSI pod deletion validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant StorageFramework
participant KubernetesAPI
participant HostExecution
StorageFramework->>KubernetesAPI: Create volume and pod
KubernetesAPI-->>StorageFramework: Return bound PV and pod node
StorageFramework->>HostExecution: Verify CSI mount path
StorageFramework->>HostExecution: Unmount and remove volume directory
HostExecution-->>StorageFramework: Confirm path is not mounted
StorageFramework->>KubernetesAPI: Delete pod
KubernetesAPI-->>StorageFramework: Complete pod deletion
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors)
✅ Passed checks (13 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: radeore 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}
openshift-ci
Bot
added
the
ready-for-human-review
|
Scheduling required tests: |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: ccd7e34
New tests seen in this PR at sha: ccd7e34
|
|
/retest |
ccd7e34 to
31cba00
Compare
|
Scheduling required tests: |
|
/testwith openshift/csi-operator#596 |
|
/testwith openshift/origin/e2e-aws-csi openshift/csi-operator#596 |
|
@jsafrane, |
|
/testwith openshift/origin/master/e2e-aws-csi openshift/csi-operator#596 |
|
@jsafrane, |
|
/testwith openshift/origin/main/e2e-aws-csi openshift/csi-operator#596 |
31cba00 to
e8f1333
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/extended/storage/csi/pod_delete_after_umount.go`:
- Around line 97-99: Update the unmount command in the volume cleanup step of
the pod deletion test to use forced unmount (`umount -f`) before removing the
directory. Keep the existing command sequencing and error validation unchanged.
🪄 Autofix
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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 63789436-4e23-4887-a052-8479b8f6a7b2
📒 Files selected for processing (3)
test/extended/storage/csi/README.mdtest/extended/storage/csi/csi.gotest/extended/storage/csi/pod_delete_after_umount.go
| g.By("Unmounting and removing the volume directory on the node") | ||
| err = hostExec.IssueCommand(ctx, fmt.Sprintf("umount %q && rmdir %q", mountPath, mountPath), node) | ||
| e2e.ExpectNoError(err, "umount and rmdir of volume mount path %s", mountPath) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use a forced unmount for this test scenario.
Line 98 uses normal umount. The suite contract requires validation after a force-unmount. This command can test a different cleanup path and leave the intended regression untested. Add -f.
Proposed fix
- err = hostExec.IssueCommand(ctx, fmt.Sprintf("umount %q && rmdir %q", mountPath, mountPath), node)
+ err = hostExec.IssueCommand(ctx, fmt.Sprintf("umount -f %q && rmdir %q", mountPath, mountPath), node)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| g.By("Unmounting and removing the volume directory on the node") | |
| err = hostExec.IssueCommand(ctx, fmt.Sprintf("umount %q && rmdir %q", mountPath, mountPath), node) | |
| e2e.ExpectNoError(err, "umount and rmdir of volume mount path %s", mountPath) | |
| g.By("Unmounting and removing the volume directory on the node") | |
| err = hostExec.IssueCommand(ctx, fmt.Sprintf("umount -f %q && rmdir %q", mountPath, mountPath), node) | |
| e2e.ExpectNoError(err, "umount and rmdir of volume mount path %s", mountPath) |
🤖 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 `@test/extended/storage/csi/pod_delete_after_umount.go` around lines 97 - 99,
Update the unmount command in the volume cleanup step of the pod deletion test
to use forced unmount (`umount -f`) before removing the directory. Keep the
existing command sequencing and error validation unchanged.
|
/testwith openshift/origin/main/e2e-aws-csi openshift/csi-operator#596 |
|
/test images |
|
Scheduling required tests: |
|
@radeore: The following tests failed, say
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. |
3 participants
Summary
OpenShift CSI extended - Pod delete after umount(migrated from openshift tests private OCP-66187).podDeleteAfterUmountcapability to enable volume umount testpodDeleteAfterUmountcapability is set 'true' in CSI driver manifest.yaml.Test plan
make openshift-tests./openshift-tests run openshift/csi --dry-run | grep 'Pod delete after umount'Test run logs:
Summary by CodeRabbit
Summary by CodeRabbit