-
Notifications
You must be signed in to change notification settings - Fork 147
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: add support for ReadWriteOncePod access mode #3236
base: master
Are you sure you want to change the base?
Conversation
@shikanime Awesome! Thanks for your contribution. |
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
AccessModeReadWriteOncePod
follows the Kubernetes storage specification correctly. This mode provides stronger guarantees thanReadWriteOnce
by 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
AccessModeReadWriteOncePod
is 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
AccessModeReadWriteOncePod
constant 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
requireExclusiveAccess
implementation and the access mode update mechanism.Implementation verified - Access mode handling is properly implemented
The
requireExclusiveAccess
function incsi/util.go
correctly 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
ReadWriteOncePod
tovol.AccessMode
when 'exclusive' is true is appropriate.
134-136
: Default access mode set to 'ReadWriteOnce' when unspecified.The code correctly assigns
ReadWriteOnce
as the defaultvol.AccessMode
when no access mode is specified, ensuring consistent behavior.
Signed-off-by: William Phetsinorath <[email protected]>
Signed-off-by: William Phetsinorath [email protected]