[Shared Mount] E2E Same Bucket Different Volume - #1598
Conversation
|
@chrisThePattyEater: The label(s) 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 kubernetes/test-infra repository. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces E2E tests for mounting the same GCS bucket via different PVs using unique volume handle suffixes in a shared-mount setup. It updates CreateVolumeResource to accept a custom volume handle suffix and adds a new test case to verify distinct mounter pods and access mode enforcement. The review feedback highlights a compilation error due to an undefined constant (specs.SameBucketDiffVolPrefix instead of specs.SingleBucketDiffVolWithSuffixPrefix), suggests clarifying the test description regarding unique volume handles, and recommends using the more idiomatic gomega.HaveLen assertion.
There was a problem hiding this comment.
Code Review
This pull request introduces a new end-to-end test to verify mounting the same bucket via different Persistent Volumes (PVs) using unique volume handle suffixes in a shared-mount configuration. It updates CreateVolumeResource to support appending a custom suffix to the volume handle and adds a new test case in multivolume.go. However, the review feedback points out critical typos where undefined constants (specs.SameBucketDiffVolPrefix and specs.SameBucketDiffVolWithSuffixPrefix) are used instead of the newly introduced specs.SingleBucketDiffVolWithSuffixPrefix, which will lead to compilation and logical errors.
1405720 to
d2e57a8
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces end-to-end tests for mounting the same bucket via different PVs with unique volume handles using shared mounts. It updates CreateVolumeResource to support custom volume handle suffixes and adds a new multi-volume test case. The review feedback suggests adding a safety check to fail the test if pvSource.CSI is nil when a suffix is requested, and adding nil assertions before accessing nested fields in volumeResourceList to prevent potential panics.
a458683 to
14b32ca
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support and end-to-end tests for mounting the same GCS bucket via different PVs with unique volume handle suffixes using shared mounts. It updates CreateVolumeResource to accept a custom volume handle suffix and adds a new E2E test verifying that two pods (one read-write, one read-only) can access the same bucket on the same node with distinct mounter pods. Feedback on the new test suggests moving the defer cleanup() call immediately after init to prevent resource leaks if initial assertions fail.
8730a32 to
1f613c6
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support and E2E tests for mounting the same bucket via different Persistent Volumes (PVs) using unique volume handle suffixes. It updates CreateVolumeResource to accept an optional custom volume handle suffix and adds a new E2E test to verify that multiple pods on the same node can access the same bucket via different PVs with unique volume handles. The review feedback suggests adding defensive assertions to check the length of l.volumeResourceList and verify that its elements are not nil before accessing them, preventing potential panics during test execution.
1f613c6 to
182a60c
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces end-to-end tests for mounting the same bucket via different Persistent Volumes (PVs) with unique volume handle suffixes using shared mounts. It updates CreateVolumeResource to support custom volume handle suffixes and adds a new test case in multivolume.go to verify read-write and read-only operations on the same bucket from different pods on the same node. However, a critical issue was identified where setting the ReadOnly property on the volume resource has no effect because tPod.setupVolume ignores VolSource when Pvc is non-nil, meaning the CSI-level read-only enforcement is not actually being tested. A fix is suggested to propagate the readOnly parameter correctly in specs.go.
|
@chrisThePattyEater: The following test failed, say
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/test-infra repository. I understand the commands that are listed here. |
c0abff5 to
f6985b6
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds an E2E test to verify mounting the same bucket via different PVs with unique volume handles using shared mount. To support this, CreateVolumeResource was updated to accept a custom volume handle suffix, and the volume setup was updated to respect the readOnly flag. Feedback suggests dynamically generating the volume handle suffix in the test suite loop instead of hardcoding values based on the index.
f6985b6 to
822c257
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces an E2E test to verify mounting the same bucket via different PVs with unique volume handles using shared mounts. It updates CreateVolumeResource to support custom volume handle suffixes and configures setupVolume to respect the read-only flag for PVCs. The review feedback suggests extending the non-nil assertions in the new test to cover the second volume resource and its nested fields, preventing potential nil pointer dereferences.
822c257 to
6deff46
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds an E2E test to verify mounting the same bucket via different PVs with unique volumeHandles using shared mounts. It updates CreateVolumeResource to support custom volume handle suffixes and configures ReadOnly on the PVC volume source. There are no review comments, so I have no feedback to provide.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces end-to-end tests and supporting infrastructure to verify mounting the same bucket via different Persistent Volumes (PVs) with unique volume handle suffixes in a shared-mount configuration. Specifically, it updates CreateVolumeResource to support custom volume handle suffixes, ensures the ReadOnly property is set on the PVC volume source, and adds a new multi-volume E2E test that deploys two pods (one ReadWriteMany and one ReadOnlyMany) on the same node to verify distinct mounter pods and access mode enforcement. There are no review comments to address.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces an E2E test to verify mounting the same GCS bucket via different PVs with unique volume handle suffixes using shared mounts. It updates CreateVolumeResource to support appending a custom volume handle suffix and ensures the ReadOnly property is correctly set on the persistent volume claim. Additionally, a new multi-volume test case is added to deploy two pods (one RWX and one ROX) on the same node, verifying that distinct mounter pods are created and access modes are properly enforced. There are no review comments, so I have no feedback to provide.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: chrisThePattyEater, uriel-guzman 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 |
5fcdc04
into
GoogleCloudPlatform:main
2 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
Adds test for verifying that the CSI driver correctly handles de-duplicating multiple PVs pointing to the same bucket with different access modes (ReadWriteMany vs ReadOnlyMany) or mount options using de-duplicated volumeHandles, creating distinct Mounter Pods for each volume
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
This change was tested by running e2e on the multivolume test package with both Shared Mount enabled and disabled. The Shared mount tests is successfully skipped when its not enabled. During the test run i manually check pantheon logs to verify both mounter pods were created and CPV and NSV succeed as expected.
Does this PR introduce a user-facing change?: