feat(csi): 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 a new access mode, 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
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.
|
@james-munson Could you create Feature ticket in longhorn/longhorn for this PR and help follow up it? Thank you. |
|
Associated tracking issue is longhorn/longhorn#9727 |
|
Thanks, @shikanime contribution. The commit message can start with For reviewers. besides reviewing, please help verify if this works as expected. The issue owner should be @james-munson . |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
csi/util.go (1)
121-132: Add documentation for exclusive access mode behavior.Consider adding a comment explaining that exclusive access mode (
ReadWriteOncePod) ensures the volume can only be used by a single pod at a time, which is more restrictive thanReadWriteOncethat allows multiple pods on the same node.+ // Set exclusive access mode (ReadWriteOncePod) which ensures the volume + // can only be used by a single pod across all nodes if exclusive, ok := volOptions["exclusive"]; ok {
📜 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)
🚧 Files skipped from review as they are similar to previous changes (3)
- k8s/pkg/apis/longhorn/v1beta1/volume.go
- k8s/pkg/apis/longhorn/v1beta2/volume.go
- types/types.go
🧰 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 (5)
csi/util.go (2)
471-483: Add function documentation and verify CSI capability constant.
The function implementation looks correct, but would benefit from documentation explaining its purpose and return value.
Add documentation:
+// requireExclusiveAccess determines if a volume requires exclusive access based on
+// its configured access mode (ReadWriteOncePod) or the requested CSI capability
+// (SINGLE_NODE_SINGLE_WRITER). Returns true if either condition is met.
func requireExclusiveAccess(vol *longhornclient.Volume, cap *csi.VolumeCapability) bool {Let's verify the CSI capability constant:
✅ Verification successful
The search results show that the CSI capability constant VolumeCapability_AccessMode_SINGLE_NODE_SINGLE_WRITER is properly defined in the CSI spec's generated protobuf code and is consistently used across the codebase. Let me verify one more aspect of the implementation by checking how this constant is used in the controller server.
Function implementation and constant usage verified
The implementation correctly uses the CSI capability constant SINGLE_NODE_SINGLE_WRITER. The constant is properly defined in the CSI spec's protobuf code and is consistently used across the codebase, including the controller server where it's listed as one of the supported access modes.
Add documentation as suggested:
+// requireExclusiveAccess determines if a volume requires exclusive access based on
+// its configured access mode (ReadWriteOncePod) or the requested CSI capability
+// (SINGLE_NODE_SINGLE_WRITER). Returns true if either condition is met.
func requireExclusiveAccess(vol *longhornclient.Volume, cap *csi.VolumeCapability) bool {🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify CSI capability constant is properly defined
# Test: Search for VolumeCapability_AccessMode constants. Expect: Definition of SINGLE_NODE_SINGLE_WRITER
rg "VolumeCapability_AccessMode_SINGLE_NODE_SINGLE_WRITER"
Length of output: 411
Script:
#!/bin/bash
# Check the context of SINGLE_NODE_SINGLE_WRITER usage in controller_server.go
rg -B 2 -A 2 "VolumeCapability_AccessMode_SINGLE_NODE_SINGLE_WRITER" csi/controller_server.go
Length of output: 378
🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 471-471: csi/util.go#L471
Redefinition of the built-in function cap. (redefines-builtin-id)
134-136: Verify access mode constant usage.
The implementation looks correct. Let's verify that the access mode constants are properly defined and imported.
✅ Verification successful
Let me gather more information about the access mode constants.
Let me verify the import path in the CSI util file to ensure it's using the correct version of the constants.
Let me check the complete imports in the file to find the longhorn package import.
Access mode constant usage is correctly implemented
The code is using the proper import longhorn "github.com/longhorn/longhorn-manager/k8s/pkg/apis/longhorn/v1beta2" which contains the AccessMode constants (AccessModeReadWriteOnce = "rwo"). The default assignment in the code matches the imported constant definition.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify access mode constants are properly defined
# Test: Search for AccessMode constants. Expect: Definitions of ReadWriteOnce, ReadWriteOncePod
rg -A 2 "type AccessMode string|const.*AccessMode"
Length of output: 359
Script:
#!/bin/bash
# Get the complete constant definitions for AccessMode
rg -A 10 "type AccessMode string" k8s/pkg/apis/longhorn/v1beta2/volume.go
Length of output: 361
Script:
#!/bin/bash
# Check imports in the CSI util file
rg -A 10 "^import" csi/util.go
Length of output: 143
Script:
#!/bin/bash
# Get all imports from the file
cat csi/util.go | awk '/^import/,/^)/'
Length of output: 535
datastore/kubernetes.go (1)
1018-1022: LGTM! Implementation looks correct.
The switch statement implementation for handling the new ReadWriteOncePod access mode is clean and follows the existing pattern. The mapping between Longhorn's access mode and Kubernetes core API's access mode is correct.
Let's verify the access mode constant is properly defined:
✅ Verification successful
AccessModeReadWriteOncePod constant is properly defined and validated
The verification confirms:
- The constant
AccessModeReadWriteOncePodis properly defined as"rwop"in both v1beta1 and v1beta2 API versions - The access mode is included in the validation logic in
types/types.go, which ensures only valid access modes are accepted
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify AccessModeReadWriteOncePod constant definition
# Test: Search for AccessModeReadWriteOncePod constant definition
rg -A 2 "AccessModeReadWriteOncePod.*=.*"
# Test: Search for related validation logic
rg -A 5 "func.*ValidateAccessMode.*"
Length of output: 879
csi/controller_server.go (2)
225-228: LGTM: Exclusive access handling is correctly implemented.
The code properly sets the exclusive parameter in volume parameters when exclusive access is required, which is essential for ReadWriteOncePod support.
463-469: LGTM: Volume publishing changes for exclusive access are well implemented.
The code properly updates the volume access mode to ReadWriteOncePod during volume publishing when exclusive access is required. The implementation includes proper error handling and aligns with CSI specifications.
Signed-off-by: William Phetsinorath <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
csi/util.go (1)
121-137: LGTM! Consider enhancing the error message.The implementation correctly handles exclusive access mode and prevents invalid combinations. The validation logic and default access mode setting are well-implemented.
Consider making the error message more descriptive:
- return nil, errors.New("cannot set both share and exclusive to true") + return nil, errors.New("conflicting access modes: volume cannot be both shared (RWX) and exclusive (RWOP)")csi/controller_server.go (1)
462-467: Consider adding a comment explaining the exclusive access mode update.While the implementation is correct, adding a comment explaining why we update to ReadWriteOncePod when exclusive access is required would improve code maintainability.
Add a comment before the if block:
+ // Update volume access mode to ReadWriteOncePod if the volume requires exclusive access + // This ensures proper access control at the storage level if requireExclusiveAccess(volume, volumeCapability) { volume, err = cs.updateVolumeAccessMode(volume, longhorn.AccessModeReadWriteOncePod) if err != nil { return nil, err } }
📜 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)
🚧 Files skipped from review as they are similar to previous changes (3)
- k8s/pkg/apis/longhorn/v1beta1/volume.go
- k8s/pkg/apis/longhorn/v1beta2/volume.go
- types/types.go
🧰 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 (5)
csi/util.go (1)
471-483: Implementation looks good, but parameter shadowing needs to be addressed.
The function correctly determines exclusive access requirements by checking both the volume's access mode and CSI capability. However, as noted in the previous review, the parameter name cap shadows Go's built-in function.
Please refer to the previous review comment about renaming the cap parameter to volCap to avoid shadowing the built-in cap function.
🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 471-471: csi/util.go#L471
Redefinition of the built-in function cap. (redefines-builtin-id)
datastore/kubernetes.go (1)
1018-1022: LGTM! Consider adding Kubernetes version compatibility check.
The switch statement is a clean implementation for mapping access modes. The code correctly maps Longhorn's AccessModeReadWriteOncePod to Kubernetes' ReadWriteOncePod.
Since ReadWriteOncePod requires Kubernetes v1.22+, let's verify the minimum supported Kubernetes version:
✅ Verification successful
Let me gather more information about Kubernetes version compatibility by searching for version constraints in the codebase.
Let me check if there are any version constraints defined in the codebase that ensure compatibility with ReadWriteOncePod access mode.
Based on the search results, I can now provide a final response about the ReadWriteOncePod access mode support:
ReadWriteOncePod access mode is properly supported and validated
- The access mode is defined in the API types and properly validated
- The CSI driver correctly maps between Longhorn and Kubernetes access modes
- The webhook validator enforces access mode validation
- No explicit minimum Kubernetes version requirement is defined, but ReadWriteOncePod requires Kubernetes v1.22+
The implementation appears complete and correct, with proper validation and mapping between Longhorn's internal representation (rwop) and Kubernetes' ReadWriteOncePod access mode.
Let me know if you would like me to check anything else about the ReadWriteOncePod implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any version constraints defined in the codebase
# that ensure compatibility with ReadWriteOncePod access mode.
# Test: Search for version-related constants or checks
rg -g '!vendor' -g '!*.sum' -g '!*.mod' -A 5 'MinimumKubernetesVersion|v1\.22|ReadWriteOncePod'
Length of output: 2337
Script:
#!/bin/bash
# Search for Kubernetes version related constants, checks, and documentation
rg -g '!vendor' -g '!*.sum' -g '!*.mod' -A 5 'MinimumKubernetesVersion|KubernetesMinVersion|KUBERNETES_MIN_VERSION|supported.*kubernetes|kubernetes.*version'
# Also check README and documentation
fd -g 'README*' -X cat {}
Length of output: 512197
Script:
#!/bin/bash
# Search for Kubernetes version constraints and ReadWriteOncePod related code
rg -g '!vendor' -g '!*.sum' -g '!*.mod' -A 5 'MinimumKubernetesVersion|ReadWriteOncePod|AccessMode|v1\.22'
Length of output: 43856
csi/controller_server.go (3)
72-72: LGTM: Appropriate addition of SINGLE_NODE_SINGLE_WRITER access mode.
The addition of SINGLE_NODE_SINGLE_WRITER access mode is correct for implementing ReadWriteOncePod support.
224-227: LGTM: Proper implementation of exclusive access check.
The implementation follows the same pattern as the shared access check and correctly sets the exclusive parameter for volumes requiring exclusive access.
224-227: Verify the requireExclusiveAccess implementation.
Let's verify the implementation of the requireExclusiveAccess function to ensure it correctly identifies volumes requiring exclusive access.
Also applies to: 462-467
✅ Verification successful
Implementation of requireExclusiveAccess is correct and properly handles access modes
The function correctly identifies volumes requiring exclusive access by checking:
- If the volume has
AccessModeReadWriteOncePodmode set - If the CSI capability requests
SINGLE_NODE_SINGLE_WRITERmode
The implementation aligns with CSI spec requirements and properly handles the transition from older access modes to newer ones, as evidenced by the CSI spec comments in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find and verify the requireExclusiveAccess implementation
# Expected: Function should check for SINGLE_NODE_SINGLE_WRITER access mode
# Search for the requireExclusiveAccess function implementation
ast-grep --pattern 'func requireExclusiveAccess($_, $_) bool {
$$$
}'
# Search for any tests covering this functionality
rg -l "TestRequireExclusiveAccess|requireExclusiveAccess.*test"
Length of output: 748
Script:
#!/bin/bash
# Let's verify the AccessMode constants and their usage
rg "AccessMode" csi/util.go -B 2 -A 2
# Check for any related test files
fd "test.*go" csi/
# Look for any other usage of AccessMode in the codebase
rg "AccessModeReadWriteOncePod|SINGLE_NODE_SINGLE_WRITER" -A 2 -B 2
Length of output: 8551
shikanime
changed the title
shikanime
changed the title
Related: longhorn/longhorn-manager#3236 Signed-off-by: William Phetsinorath <[email protected]>
Signed-off-by: William Phetsinorath [email protected]