chore: add support for ReadWriteOncePod access mode #3235
Conversation
WalkthroughThe changes in this pull request enhance the handling of volume access modes within the Longhorn project. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PVC
participant VolumeSpec
participant Validator
participant Options
participant Controller
User->>PVC: Request PVC creation
PVC->>VolumeSpec: Determine access mode
VolumeSpec->>PVC: Return access mode
PVC->>Validator: Validate access mode
Validator->>PVC: Return validation status
PVC->>Options: Get volume options
Options->>PVC: Return volume options
PVC->>Controller: Create volume with options
Controller->>PVC: Return volume creation response
PVC->>User: PVC creation 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
|
shikanime
changed the title
![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 (1)
k8s/pkg/apis/longhorn/v1beta1/volume.go (1)
62-64: Add documentation for access modes.The implementation of the new
AccessModeReadWriteOncePodconstant looks good and follows the existing pattern. However, consider adding documentation comments to explain each access mode's behavior and use cases.Add documentation like this:
type AccessMode string const ( + // AccessModeReadWriteOnce makes the volume can be mounted as read-write by a single node AccessModeReadWriteOnce = AccessMode("rwo") + // AccessModeReadWriteOncePod makes the volume can be mounted as read-write by a single pod AccessModeReadWriteOncePod = AccessMode("rwop") + // AccessModeReadWriteMany makes the volume can be mounted as read-write by many nodes AccessModeReadWriteMany = AccessMode("rwx") )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- 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 comments (5)
k8s/pkg/apis/longhorn/v1beta1/volume.go (1)
62-64: Verify access mode validation.The new access mode needs proper validation to ensure it's handled correctly throughout the codebase.
✅ Verification successful
Let me gather more information about the validation logic by examining the content of the files where the access mode is used.
Based on the results, I can now provide the final response as we have sufficient evidence about the validation and handling of the new access mode.
Access mode validation is properly implemented
The new
AccessModeReadWriteOncePodis correctly validated and handled throughout the codebase:
- Validation is implemented in
types/types.gothrough theValidateAccessModefunction which checks for all valid access modes including the new one- Proper conversion to Kubernetes native types is implemented in
datastore/kubernetes.gowhere it's mapped tocorev1.ReadWriteOncePod- The constant is consistently defined across both v1beta1 and v1beta2 API versions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the new access mode is properly validated # Test 1: Look for validation logic for the new access mode echo "Checking for validation logic..." rg -l "AccessModeReadWriteOncePod" --type go # Test 2: Look for switch/if statements handling access modes echo "Checking access mode handling..." ast-grep --pattern 'switch $x.(AccessMode|accessMode) { $$$ }' # Test 3: Look for test coverage echo "Checking test coverage..." fd -e test.go | xargs rg "AccessModeReadWriteOncePod"Length of output: 894
Script:
#!/bin/bash # Check the validation logic implementation and usage # Show the validation logic in types/types.go echo "Checking validation in types/types.go:" rg -A 5 -B 5 "AccessModeReadWriteOncePod" types/types.go # Show the handling in datastore/kubernetes.go echo -e "\nChecking handling in datastore/kubernetes.go:" rg -A 5 -B 5 "AccessModeReadWriteOncePod" datastore/kubernetes.go # Look for any validation functions related to access modes echo -e "\nChecking for validation functions:" ast-grep --pattern 'func $_ ($x $_) $_ { if $x.$_ != $_ { $$$ } }'Length of output: 1206
k8s/pkg/apis/longhorn/v1beta2/volume.go (2)
Line range hint
315-315: LGTM: AccessMode field integration in VolumeSpecThe AccessMode field is properly integrated into the VolumeSpec struct with appropriate JSON tags and optional marking.
60-62: Consider Kubernetes version compatibility and documentationThe introduction of ReadWriteOncePod access mode has several implications:
- Ensure that the minimum supported Kubernetes version is compatible with this feature (introduced in K8s 1.22)
- Consider adding documentation about:
- Version compatibility requirements
- Migration guidelines for existing volumes
- Use cases and limitations of this access mode
Let's check if there's any version compatibility information:
✅ Verification successful
Let me try a different approach to gather information about version compatibility and documentation.
These scripts will help us:
- Search for version requirements in all markdown and text files
- Find context around ReadWriteOncePod usage
- Check dependencies in go.mod for K8s version
- Look for version checks in the code
Let's analyze the results to provide a more informed response.
Based on the search results, I can now provide a final response about the ReadWriteOncePod access mode implementation:
ReadWriteOncePod support is properly implemented with required validations
The codebase shows proper implementation of ReadWriteOncePod (RWOP) access mode:
- The feature is integrated with Kubernetes v1.31.1 (as seen in go.mod), which is well beyond the K8s 1.22 requirement
- Proper validations are in place:
- Core Kubernetes validation prevents mixing RWOP with other access modes
- The access mode is properly mapped in the datastore package
- The feature is supported by the SELinuxMountReadWriteOncePod feature gate (which is in Beta and enabled by default)
No additional documentation is required as:
- The implementation follows standard Kubernetes patterns
- The feature is now stable in both Kubernetes and this codebase
- The access mode is a standard Kubernetes concept documented in upstream Kubernetes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for Kubernetes version compatibility information rg -i "kubernetes.*version|version.*requirement" README.mdLength of output: 60
Script:
#!/bin/bash # Check for version-related information in docs and other common locations fd -e md -e txt | xargs rg -i "kubernetes.*version|version.*requirement|minimum.*version" # Look for any documentation about ReadWriteOncePod rg -i "readwriteoncepod|rwop" -A 3 -B 3 # Check if there's any version information in go.mod cat go.mod # Look for any version checks in the codebase ast-grep --pattern 'if $_ >= $_.$_.$$$ { $$$ }'Length of output: 46196
types/types.go (1)
847-847: LGTM! The ReadWriteOncePod access mode validation is correctly implemented.The implementation properly validates the new
AccessModeReadWriteOncePodaccess mode alongside the existing modes. This aligns with the Kubernetes storage specification where ReadWriteOncePod was introduced to provide stronger guarantees for pod access exclusivity compared to ReadWriteOnce.datastore/kubernetes.go (1)
1018-1022: LGTM! Clean implementation of ReadWriteOncePod access mode support.The switch statement implementation for access mode mapping is clean and follows the existing pattern. The new case correctly maps
longhorn.AccessModeReadWriteOncePodtocorev1.ReadWriteOncePod.Let's verify the access mode type is properly defined:
✅ Verification successful
The search results show that
AccessModeReadWriteOncePodis properly defined in both v1beta1 and v1beta2 API versions of Longhorn types with the value "rwop". Let me verify one more aspect to ensure complete validation:
Let me modify the search to look for import statements in a different way:
Let me try one more search with a larger context to find the longhorn imports:
Let me try one final search to look for the entire file content to find the imports:
Now I have all the necessary information to provide the final response:
AccessModeReadWriteOncePod implementation is correctly defined and imported
The verification confirms:
AccessModeReadWriteOncePodis properly defined in both v1beta1 and v1beta2 API versions with the value "rwop"- The correct Longhorn types are imported via
longhorn "github.com/longhorn/longhorn-manager/k8s/pkg/apis/longhorn/v1beta2"- The mapping between Longhorn's
AccessModeReadWriteOncePodand Kubernetes'corev1.ReadWriteOncePodis implemented correctly🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify AccessModeReadWriteOncePod is defined in the Longhorn types # Test: Search for AccessModeReadWriteOncePod definition. Expect: Definition in types package. rg "AccessModeReadWriteOncePod.*=.*" -A 2Length of output: 484
Script:
#!/bin/bash # Verify the import statement in kubernetes.go uses the correct package for longhorn types rg "import.*longhorn" datastore/kubernetes.goLength of output: 47
Script:
#!/bin/bash # Look for longhorn imports in the entire import block rg -A 10 "^import \(" datastore/kubernetes.goLength of output: 185
Script:
#!/bin/bash # Look for longhorn imports with larger context rg -A 20 "^import \(" datastore/kubernetes.go | grep -i "longhorn"Length of output: 66
Script:
#!/bin/bash # Get the entire file content to analyze imports head -n 50 datastore/kubernetes.goLength of output: 1408
shikanime
force-pushed
the
pr3235
branch
4 times, most recently
from
fa60d58
to
2415bc6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
csi/util.go (2)
131-133: Add documentation for default access mode behavior.While the default to ReadWriteOnce is correct, it would be helpful to add a comment explaining this default behavior and its relationship with the
exclusiveandshareparameters.Add a comment like:
+// Default to ReadWriteOnce if no access mode was set by exclusive or share parameters if vol.AccessMode == "" { vol.AccessMode = string(longhorn.AccessModeReadWriteOnce) }
Line range hint
110-133: Add unit tests for the new exclusive parameter.The implementation looks good, but needs test coverage for:
- Exclusive parameter parsing
- Interaction between exclusive and share parameters
- Default access mode behavior
Would you like me to help generate unit tests for these scenarios?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- 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 (4)
- datastore/kubernetes.go
- k8s/pkg/apis/longhorn/v1beta1/volume.go
- k8s/pkg/apis/longhorn/v1beta2/volume.go
- types/types.go
🔇 Additional comments (1)
csi/util.go (1)
110-118: Consider reordering parameter processing to avoid conflicts.The
exclusiveparameter is processed before theshareparameter, which could lead to unexpected behavior if both are set. The ReadWriteOncePod mode set byexclusive=truecould be immediately overwritten byshare=trueto ReadWriteMany.Let's verify the potential parameter conflict:
Consider:
- Processing share parameter first since it's more restrictive (ReadWriteMany)
- Adding validation to prevent conflicting parameters
- Documenting the parameter precedence in comments
shikanime
force-pushed
the
pr3235
branch
2 times, most recently
from
42db3a0
to
f2f33c7
Compare
Signed-off-by: William Phetsinorath <william.phetsinorath@shikanime.studio>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
csi/util.go (2)
110-118: Add documentation for exclusive parameter behavior.The exclusive parameter's behavior and its impact on access mode should be documented for better maintainability.
Add a comment above the if block explaining:
+// If exclusive=true and current access mode is ReadWriteOnce, +// upgrade it to ReadWriteOncePod for stronger guarantees if exclusive, ok := volOptions["exclusive"]; ok {
468-480: Improve function documentation and parameter naming.
- The function is exported and needs proper documentation.
- The parameter name
capshadows Go's built-incap()function.Apply these improvements:
+// requireExclusiveAccess determines if the volume requires exclusive access based on +// its access mode and volume capability. A volume requires exclusive access when: +// - Its access mode is ReadWriteOncePod, or +// - The requested capability mode is SINGLE_NODE_SINGLE_WRITER -func requireExclusiveAccess(vol *longhornclient.Volume, cap *csi.VolumeCapability) bool { +func requireExclusiveAccess(vol *longhornclient.Volume, capability *csi.VolumeCapability) bool { isExclusive := false if vol != nil { isExclusive = vol.AccessMode == string(longhorn.AccessModeReadWriteOncePod) } mode := csi.VolumeCapability_AccessMode_UNKNOWN - if cap != nil { - mode = cap.AccessMode.Mode + if capability != nil { + mode = capability.AccessMode.Mode } return isExclusive || mode == csi.VolumeCapability_AccessMode_SINGLE_NODE_SINGLE_WRITER }🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 468-468: csi/util.go#L468
Redefinition of the built-in function cap. (redefines-builtin-id)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
- csi/controller_server.go (3 hunks)
- csi/util.go (3 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 (4)
- datastore/kubernetes.go
- 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] 468-468: csi/util.go#L468
Redefinition of the built-in function cap. (redefines-builtin-id)
🔇 Additional comments (4)
csi/util.go (2)
131-133: LGTM!Good placement of the default access mode setting after handling explicit parameters.
468-480: LGTM on the implementation logic!The function correctly determines exclusive access requirements by checking both the volume's access mode and the requested capability mode.
🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 468-468: csi/util.go#L468
Redefinition of the built-in function cap. (redefines-builtin-id)csi/controller_server.go (2)
463-469: Ensure safe update of volume access mode toReadWriteOncePodIn
ControllerPublishVolume, whenrequireExclusiveAccess(volume, volumeCapability)returns true, the code updates the volume's access mode toAccessModeReadWriteOncePodby callingcs.updateVolumeAccessMode(volume, longhorn.AccessModeReadWriteOncePod).Verify that this access mode change is safely handled, considering any concurrent access or existing attachments.
Check that the
updateVolumeAccessModefunction safely updates the volume's access mode and that the volume manager handles access mode changes correctly during runtime. Ensure that appropriate synchronization mechanisms are in place to prevent data races or inconsistencies.
72-73: Verify comprehensive handling of new access modes across the codebaseThe addition of
csi.VolumeCapability_AccessMode_SINGLE_NODE_SINGLE_WRITER,csi.VolumeCapability_AccessMode_SINGLE_NODE_MULTI_WRITER, andcsi.VolumeCapability_AccessMode_MULTI_NODE_MULTI_WRITERin theaccessModesslice broadens the volume capabilities.Ensure that these newly added access modes are fully supported and correctly handled in all relevant methods, such as
validateVolumeCapabilities,CreateVolume,ControllerPublishVolume, and any custom logic that depends on access modes.You can run the following shell script to identify where these new access modes are being used and verify their handling:
✅ Verification successful
Let me gather more context about how these access modes are handled in the codebase, particularly in the validation and volume management logic.
Let me try a different approach to check the validation and handling of these access modes.
Based on the gathered information, I can now provide a final response about the handling of the new access modes.
Access modes are properly integrated and validated across the codebase
The codebase shows comprehensive handling of the new access modes:
The access modes are properly declared in the
ControllerServerinitialization and included in the supported capabilities.The
requiresSharedAccessandrequireExclusiveAccessutility functions incsi/util.goproperly handle these access modes:
SINGLE_NODE_SINGLE_WRITERis correctly handled inrequireExclusiveAccessSINGLE_NODE_MULTI_WRITERandMULTI_NODE_MULTI_WRITERare handled inrequiresSharedAccessThe validation flow is complete:
- Volume capabilities are validated in
validateVolumeCapabilities- Access modes are checked during volume operations (Create, Stage, Publish)
- The mounter selection logic in
getMounterconsiders these access modes for proper mounting strategy🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for handling of new access modes in the codebase. # Expected: All occurrences of the new access modes are properly processed. # Search for the new access modes in the codebase. rg 'SINGLE_NODE_SINGLE_WRITER|SINGLE_NODE_MULTI_WRITER|MULTI_NODE_MULTI_WRITER' --type goLength of output: 3420
Script:
#!/bin/bash # Check the implementation of validateVolumeCapabilities and related methods ast-grep --pattern 'func $_($_, $_, $_) error { $$$ validateVolumeCapabilities $$$ }' # Look for any validation logic around access modes rg -A 5 "validateVolumeCapability" --type go # Check the implementation of CreateVolume and ControllerPublishVolume ast-grep --pattern 'func (cs *controllerServer) CreateVolume($_, $_) ($_, error) { $$$ }' ast-grep --pattern 'func (cs *controllerServer) ControllerPublishVolume($_, $_) ($_, error) { $$$ }'Length of output: 337
Script:
#!/bin/bash # Search for validation logic and access mode handling rg -B 2 -A 5 "func.*validateVolumeCapability" --type go # Look for access mode validation in util.go since we saw references there cat csi/util.go # Check the implementation details around volume capabilities rg -B 2 -A 10 "VolumeCapability" --type go --glob '!vendor/**'Length of output: 27772
|
Duplicate pull request due to bug |
Signed-off-by: William Phetsinorath william.phetsinorath@shikanime.studio