chore: add support for ReadWriteOncePod access mode #3236
Conversation
|
@shikanime Awesome! Thanks for your contribution. |
derekbit
requested review from
PhanLe1010,
c3y1huang,
james-munson,
innobead and
derekbit
WalkthroughThe changes in this pull request enhance the volume management system by introducing new access modes for volumes and refining error handling across various methods. Key updates include the addition of three new access modes in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ControllerServer
participant VolumeManager
participant PVCManager
User->>ControllerServer: Create Volume Request
ControllerServer->>VolumeManager: Validate Access Modes
VolumeManager->>ControllerServer: Access Modes Validated
ControllerServer->>VolumeManager: Create Volume
VolumeManager->>ControllerServer: Volume Created
ControllerServer->>User: Volume Creation Response
User->>ControllerServer: Publish Volume Request
ControllerServer->>VolumeManager: Check Exclusive Access
VolumeManager->>ControllerServer: Exclusive Access Validated
ControllerServer->>PVCManager: Update PVC Manifest
PVCManager->>ControllerServer: PVC Manifest Updated
ControllerServer->>User: Volume Published Response
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🪛 GitHub Check: CodeFactor
🔇 Additional comments (6)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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
🧹 Outside diff range and nitpick comments (2)
k8s/pkg/apis/longhorn/v1beta1/volume.go (1)
62-64: LGTM! The new access mode aligns with Kubernetes standards.The addition of
AccessModeReadWriteOncePodfollows the Kubernetes storage specification correctly. This mode provides stronger guarantees thanReadWriteOnceby ensuring exclusive access at the pod level rather than the node level.Note that this access mode requires Kubernetes 1.22+ and provides better isolation for workloads requiring strict exclusive access, such as databases or stateful applications with strict consistency requirements.
types/types.go (1)
847-847: LGTM! Consider improving readability.The addition of
AccessModeReadWriteOncePodis correct and aligns with the PR objective.Consider improving readability by formatting the condition across multiple lines:
- if mode != longhorn.AccessModeReadWriteMany && mode != longhorn.AccessModeReadWriteOnce && mode != longhorn.AccessModeReadWriteOncePod { + if mode != longhorn.AccessModeReadWriteMany && + mode != longhorn.AccessModeReadWriteOnce && + mode != longhorn.AccessModeReadWriteOncePod {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
- csi/controller_server.go (3 hunks)
- csi/util.go (2 hunks)
- datastore/kubernetes.go (1 hunks)
- k8s/pkg/apis/longhorn/v1beta1/volume.go (1 hunks)
- k8s/pkg/apis/longhorn/v1beta2/volume.go (1 hunks)
- types/types.go (1 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeFactor
csi/util.go
[notice] 471-471: csi/util.go#L471
Redefinition of the built-in function cap. (redefines-builtin-id)
🔇 Additional comments (9)
k8s/pkg/apis/longhorn/v1beta2/volume.go (2)
60-62: LGTM! Constants follow Kubernetes conventionsThe new
AccessModeReadWriteOncePodconstant follows proper naming conventions and uses the standard "rwop" abbreviation that aligns with Kubernetes CSI specifications.
56-62: Verify integration with Kubernetes CSI and existing volumesWhile the implementation looks correct, please ensure:
- Existing volumes with other access modes continue to work
- CSI driver properly handles the new access mode
- Documentation is updated to reflect this new capability
Let's check for any existing volumes that might be affected:
datastore/kubernetes.go (1)
1018-1022: LGTM! Clean implementation of ReadWriteOncePod access mode support.The switch statement implementation correctly maps the Longhorn access mode to the corresponding Kubernetes PVC access mode.
Let's verify the access mode constant is properly defined:
✅ Verification successful
AccessModeReadWriteOncePod constant is properly defined and consistent across API versions
The constant is correctly defined in both v1beta1 and v1beta2 API versions with the same value "rwop", ensuring backward compatibility and proper type safety. The implementation in the switch statement correctly uses this constant.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify AccessModeReadWriteOncePod is defined in the types rg "AccessModeReadWriteOncePod.*=.*" -A 2Length of output: 484
csi/controller_server.go (4)
72-73: LGTM: Access mode additions follow CSI specThe new access modes SINGLE_NODE_SINGLE_WRITER and SINGLE_NODE_MULTI_WRITER enhance the granularity of volume access control and align with the CSI specification.
225-228: LGTM: Proper exclusive access handling in volume creationThe exclusive access check is properly implemented and follows the same pattern as the shared access check. The volumeParameters are correctly updated when exclusive access is required.
463-469: LGTM: Proper exclusive access handling in volume publishingThe exclusive access handling in ControllerPublishVolume is well-implemented:
- Follows the same pattern as shared access handling
- Properly updates volume access mode to ReadWriteOncePod when required
- Maintains consistent error handling
463-469: Verify the requireExclusiveAccess implementationLet's verify the implementation of the requireExclusiveAccess function to ensure proper handling of exclusive access modes.
✅ Verification successful
Based on the shell script results, I can now generate the final response as we have sufficient information about both the
requireExclusiveAccessimplementation and the access mode update mechanism.Implementation verified - Access mode handling is properly implemented
The
requireExclusiveAccessfunction incsi/util.gocorrectly handles exclusive access by:
- Checking if the volume's current access mode is ReadWriteOncePod
- Verifying if the requested capability is SINGLE_NODE_SINGLE_WRITER
- Properly combining both conditions with logical OR
The access mode update flow is also well-implemented:
- Changes are handled through a proper API client interface
- Updates are processed via the volume manager with appropriate error handling
- The operation is atomic at the API level
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the requireExclusiveAccess function implementation ast-grep --pattern 'func requireExclusiveAccess($_, $_) bool { $$$ }' # Search for potential race conditions in access mode changes rg -A 5 'UpdateAccessMode|ActionUpdateAccessMode'Length of output: 12110
csi/util.go (2)
121-132: The 'exclusive' parameter is correctly handled and mutually exclusive with 'share'.The logic implemented ensures that if 'exclusive' is set to true, it cannot be combined with 'share' set to true, preventing conflicting access modes. The assignment of
ReadWriteOncePodtovol.AccessModewhen 'exclusive' is true is appropriate.
134-136: Default access mode set to 'ReadWriteOnce' when unspecified.The code correctly assigns
ReadWriteOnceas the defaultvol.AccessModewhen no access mode is specified, ensuring consistent behavior.
Signed-off-by: William Phetsinorath <[email protected]>
Signed-off-by: William Phetsinorath [email protected]