test(robot): expand PVC more than storage maximum size #2178
Conversation
WalkthroughThe pull request introduces multiple enhancements to the handling of persistent volume claims (PVCs) and workload management in the codebase. Key modifications include the addition of new keywords for volume size assertions and expansion, as well as updates to existing methods to improve flexibility and error handling. Notably, the changes aim to prevent issues related to volume resizing, ensuring that operations do not exceed available storage capacity. A new test suite is also added to validate the behavior of PVCs under various conditions. Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
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
CodeRabbit Configuration File (
|
c3y1huang
force-pushed
the
6633-prevent-resize-stuck
branch
from
7aecb1c to
f8ae949
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (10)
e2e/libs/host/aws.py (2)
Line range hint
18-35: Add error handling for AWS waiter timeoutsThe code uses AWS waiters to check instance states but doesn't handle potential timeout exceptions. This could lead to test failures if AWS operations take longer than expected.
Consider wrapping waiter calls with try-except blocks:
- waiter = self.aws_client.get_waiter('instance_stopped') - waiter.wait(InstanceIds=instance_ids) + try: + waiter = self.aws_client.get_waiter('instance_stopped') + waiter.wait( + InstanceIds=instance_ids, + WaiterConfig={'Delay': 15, 'MaxAttempts': 40} + ) + except Exception as e: + logging(f"Error waiting for instances to stop: {e}") + raiseAlso applies to: 36-52, 53-69, 70-78, 79-87
Line range hint
18-35: Document implications of force shutdownThe
Force=Trueparameter instop_instancescalls might lead to data corruption or incomplete operations. This should be documented and potentially made configurable.Add a docstring explaining the force shutdown behavior:
def reboot_all_nodes(self, shut_down_time_in_sec=NODE_REBOOT_DOWN_TIME_SECOND): + """Reboot all nodes in the cluster. + + Args: + shut_down_time_in_sec (int): Time to wait between stop and start operations + + Note: + Uses force shutdown which might lead to abrupt termination of workloads. + Ensure your test cases can handle potential data loss or corruption. + """Also applies to: 36-52, 53-69
e2e/libs/backup/base.py (2)
Line range hint
8-10: Replacereturn NotImplementedwithraise NotImplementedError()in abstract methodsIn the abstract methods, using
return NotImplementedis inappropriate. Instead, you should raise aNotImplementedError()to indicate that the method must be implemented by subclasses. This ensures that if the method is accidentally called, an exception is raised.Apply this diff to correct the abstract methods:
@abstractmethod def create(self, volume_name, backup_id): - return NotImplemented + raise NotImplementedError() @abstractmethod def get(self, backup_id, volume_name): - return NotImplemented + raise NotImplementedError() @abstractmethod def get_backup_volume(self, volume_name): - return NotImplemented + raise NotImplementedError() @abstractmethod def list(self, volume_name): - return NotImplemented + raise NotImplementedError() @abstractmethod def list_all(self): - return NotImplemented + raise NotImplementedError() @abstractmethod def delete(self, volume_name, backup_id): - return NotImplemented + raise NotImplementedError() @abstractmethod def delete_backup_volume(self, volume_name): - return NotImplemented + raise NotImplementedError() @abstractmethod def restore(self, volume_name, backup_id): - return NotImplemented + raise NotImplementedError() @abstractmethod def check_restored_volume_checksum(self, volume_name, backup_name): - return NotImplemented + raise NotImplementedError() @abstractmethod def get_restored_checksum(self, backup_name): - return NotImplemented + raise NotImplementedError() @abstractmethod def cleanup_backup_volumes(self): - return NotImplemented + raise NotImplementedError() @abstractmethod def cleanup_system_backups(self): - return NotImplemented + raise NotImplementedError()Also applies to: 25-26, 36-37, 47-48, 54-55, 59-60, 65-66, 70-71
Line range hint
29-30: Add@abstractmethoddecorator to abstract methods without itThe methods
get_by_snapshotandwait_for_backup_completedare intended to be abstract but are missing the@abstractmethoddecorator. Additionally, they should raiseNotImplementedError()instead of returningNotImplemented.Apply this diff to make the methods abstract and properly signal their intent:
+ @abstractmethod def get_by_snapshot(self, volume_name, snapshot_name): - return NotImplemented + raise NotImplementedError() + @abstractmethod def wait_for_backup_completed(self, volume_name, snapshot_name): - return NotImplemented + raise NotImplementedError()Also applies to: 33-34
e2e/keywords/persistentvolumeclaim.resource (1)
24-26: Ensure consistent naming by using 'claim' instead of 'volume' in claim name generationIn the keyword
Delete persistentvolumeclaim for volume ${volume_id}, the claim name is generated usingvolumeas the kind. For consistency with other keywords and to avoid confusion, it should useclaim.Apply this diff to adjust the kind used in the claim name:
Delete persistentvolumeclaim for volume ${volume_id} - ${claim_name} = generate_name_with_suffix volume ${volume_id} + ${claim_name} = generate_name_with_suffix claim ${volume_id} delete_persistentvolumeclaim ${claim_name}e2e/libs/persistentvolumeclaim/crd.py (2)
Line range hint
108-109: Enhance error message for expansion failuresConsider making the error message more specific to help with debugging.
- assert False, f"Failed to expand {claim_name} size to {size}" + assert False, f"Failed to expand PVC {claim_name} from {current_size} to {size} after {retry_count} attempts"
Line range hint
87-109: Consider additional test scenariosTo comprehensively test PVC expansion behavior, consider adding test cases for:
- Expanding to exactly the maximum size
- Expanding with decimal values
- Expanding with different size units (Ki, Mi, Gi, Ti)
- Concurrent expansion requests
This will help ensure robust handling of all edge cases.
e2e/libs/keywords/volume_keywords.py (2)
2-2: Remove unused importThe
jsonmodule is imported but never used in this file.-import json🧰 Tools
🪛 Ruff (0.8.0)
2-2:
jsonimported but unusedRemove unused import:
json(F401)
343-360: Consider adding unit testsThe new methods are crucial for testing PVC expansion limits. Consider adding unit tests to verify their behavior, especially edge cases like:
- Non-existent volumes
- Volumes without replicas
- Replicas not scheduled to any disk
Would you like me to help generate unit tests for these methods?
🧰 Tools
🪛 Ruff (0.8.0)
360-360: Do not
assert False(python -Oremoves these calls), raiseAssertionError()Replace
assert False(B011)
e2e/libs/keywords/workload_keywords.py (1)
163-169: Consider adding size validation checksWhile the current changes provide the foundation for testing PVC expansion beyond maximum size, consider enhancing the method to optionally validate if the requested size exceeds available storage capacity. This would align with the linked issue's goal of preventing volume resize stuck situations.
Would you like me to propose an implementation that includes size validation checks?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (23)
e2e/keywords/persistentvolumeclaim.resource(2 hunks)e2e/keywords/statefulset.resource(0 hunks)e2e/keywords/workload.resource(2 hunks)e2e/libs/backup/backup.py(1 hunks)e2e/libs/backup/base.py(1 hunks)e2e/libs/backup/rest.py(1 hunks)e2e/libs/backupstore/base.py(1 hunks)e2e/libs/backupstore/nfs.py(1 hunks)e2e/libs/backupstore/s3.py(1 hunks)e2e/libs/engine_image/engine_image.py(1 hunks)e2e/libs/host/aws.py(1 hunks)e2e/libs/host/harvester.py(1 hunks)e2e/libs/k8s/k8s.py(1 hunks)e2e/libs/keywords/common_keywords.py(2 hunks)e2e/libs/keywords/persistentvolumeclaim_keywords.py(2 hunks)e2e/libs/keywords/volume_keywords.py(3 hunks)e2e/libs/keywords/workload_keywords.py(2 hunks)e2e/libs/node/node.py(2 hunks)e2e/libs/persistentvolumeclaim/crd.py(1 hunks)e2e/libs/persistentvolumeclaim/persistentvolumeclaim.py(4 hunks)e2e/libs/utility/utility.py(5 hunks)e2e/libs/volume/crd.py(0 hunks)e2e/tests/regression/test_persistentvolumeclaim.robot(1 hunks)
💤 Files with no reviewable changes (2)
- e2e/keywords/statefulset.resource
- e2e/libs/volume/crd.py
✅ Files skipped from review due to trivial changes (5)
- e2e/libs/backupstore/base.py
- e2e/libs/backupstore/nfs.py
- e2e/libs/host/harvester.py
- e2e/libs/engine_image/engine_image.py
- e2e/libs/k8s/k8s.py
🧰 Additional context used
🪛 Ruff (0.8.0)
e2e/libs/keywords/volume_keywords.py
2-2: json imported but unused
Remove unused import: json
(F401)
360-360: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
e2e/libs/utility/utility.py
355-355: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
e2e/libs/keywords/workload_keywords.py
33-33: Redefinition of unused convert_size_to_bytes from line 31
Remove definition: convert_size_to_bytes
(F811)
🔇 Additional comments (18)
e2e/libs/host/aws.py (1)
3-6: LGTM! Clean import organization
The import statements are well-organized and the addition of NODE_REBOOT_DOWN_TIME_SECOND from constants improves code maintainability.
e2e/libs/keywords/persistentvolumeclaim_keywords.py (2)
23-25: LGTM!
The create_persistentvolumeclaim method correctly includes the storage_size parameter and properly passes it to the creation function.
38-40: LGTM!
The get_claim_requested_size method accurately retrieves the requested storage size from the claim's specification.
e2e/tests/regression/test_persistentvolumeclaim.robot (1)
22-44: LGTM!
The test case effectively verifies that a PersistentVolumeClaim cannot be expanded beyond its maximum storage size and that data integrity is maintained throughout the process.
e2e/libs/backup/backup.py (1)
4-4: Clarify the necessity of changing the default strategy
The change from CRD to REST strategy seems unrelated to the PR's objective of testing PVC expansion. Could you explain why this change is necessary?
Also applies to: 9-9
e2e/libs/persistentvolumeclaim/persistentvolumeclaim.py (1)
27-29: LGTM: Storage size parameter addition
The addition of the storage_size parameter with size conversion is well implemented.
Also applies to: 44-46
e2e/libs/persistentvolumeclaim/crd.py (1)
87-89: LGTM: Skip retry implementation
The skip_retry parameter is well implemented, allowing tests to verify immediate failure scenarios.
e2e/libs/node/node.py (2)
8-8: LGTM!
The import of LONGHORN_NAMESPACE constant is appropriate for the namespace-specific node handling logic.
85-88: LGTM! The namespace-specific node retrieval logic is well-implemented.
The conditional logic appropriately handles different node types based on the namespace:
- Returns a Longhorn client node for the Longhorn namespace
- Falls back to Kubernetes core API node for other namespaces
e2e/libs/backupstore/s3.py (1)
6-11: LGTM! Import statements are well-organized.
The imports follow Python's recommended organization:
- Standard library imports
- Third-party imports (minio)
- Local application imports (backupstore.base)
This organization improves code readability and maintainability.
e2e/libs/backup/rest.py (3)
1-11: LGTM! Import statements are well-organized and complete.
The imports are properly organized and include all necessary modules for the enhanced error handling and logging functionality.
Line range hint 85-107: Excellent error handling and logging improvements.
The enhanced error handling in get_from_list now:
- Catches specific KeyError exceptions
- Provides detailed error messages
- Includes backup name in error logs
- Handles unexpected errors gracefully
This improvement aids in debugging and troubleshooting backup-related issues.
Line range hint 108-139: Well-implemented logging for backup operations.
The additional logging statements in get_by_snapshot and get_backup_volume provide better visibility into the backup process:
- Clear progress indicators
- Detailed error messages
- Helpful context information
This enhancement makes it easier to track and debug backup operations.
e2e/libs/keywords/volume_keywords.py (1)
343-345: LGTM!
The method correctly retrieves the volume size from the volume spec, which will be useful for PVC expansion testing.
e2e/keywords/workload.resource (3)
217-222: LGTM! Volume expansion keyword implementation.
The implementation correctly:
- Generates workload name
- Converts size to bytes
- Calls expand_workload_claim_size with proper parameters
223-230: LGTM! Test case for expansion beyond max size.
The implementation properly:
- Gets the maximum storage size from the node
- Attempts expansion to max_size + 1
- Expects and handles the failure case
- Uses skip_retry=True to avoid unnecessary retries
232-240: LGTM! Volume size assertion implementation.
The implementation correctly:
- Verifies size remains constant for the specified period
- Converts expected size to bytes for comparison
- Uses proper sleep interval for polling
e2e/libs/keywords/workload_keywords.py (1)
163-169: LGTM! Method changes align well with testing PVC expansion limits
The refactored method provides better support for testing PVC expansion scenarios, including:
- More precise size control using bytes
- Current size retrieval for validation
- Skip retry option for testing failure cases
These changes align perfectly with the PR objective of testing PVC expansion beyond maximum storage size.
Let's verify the method's usage in test cases:
✅ Verification successful
Method changes are properly integrated and used correctly in test cases
The verification shows that:
- The method is used correctly in test scenarios in
e2e/keywords/workload.resource - It's used for both normal expansion cases and negative test cases (testing failure scenarios)
- All usages have been updated to work with bytes instead of MiB
- The new
skip_retryparameter is properly utilized in failure test cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find test cases using this method to ensure proper migration
# from the old method name and parameter types
# Search for both old and new method names
echo "Searching for method usage patterns..."
rg -A 2 "expand_workload_claim_size(_by_mib)?\s*\("
# Look for any remaining MiB size calculations that might need updating
echo "Checking for MiB calculations that might need updating..."
rg "MEBIBYTE.*expand_workload_claim"
Length of output: 723
Script:
#!/bin/bash
# Let's search for any usage of this method in test files
echo "Searching for test files using expand_workload_claim_size..."
rg -l "expand_workload_claim_size" e2e/test_suites/
# Let's also check for any references to the old method name
echo "Checking for any remaining references to old method name..."
rg "expand_workload_claim_size_by_mib"
# Let's look for any test files modified in this PR that might use this method
echo "Checking modified test files in this PR..."
git diff --name-only HEAD~1 | grep "test_.*\.py$"
Length of output: 554
Script:
#!/bin/bash
# Let's first find the correct test directory structure
echo "Finding test directories..."
fd -t d "test"
# Then search for any usage of this method in the entire codebase
echo "Searching for all usages of expand_workload_claim_size..."
rg -B 2 -A 2 "expand_workload_claim_size"
# Look for any git changes related to this method
echo "Checking git changes related to this method..."
git log -p --all | rg "expand_workload_claim_size"
Length of output: 2042
c3y1huang
force-pushed
the
6633-prevent-resize-stuck
branch
from
f8ae949 to
07a8ed9
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
e2e/keywords/workload.resource (2)
232-240: Consider improvements to size assertion implementationThe size assertion implementation could be enhanced:
- Consider making the sleep interval configurable rather than hardcoded to 1 second
- Add validation for the input parameters (period should be positive integer)
- Consider using Robot Framework's built-in timing keywords
Here's a suggested improvement:
Assert volume size of ${workload_kind} ${workload_id} remains ${size} for at least ${period} seconds ${workload_name} = generate_name_with_suffix ${workload_kind} ${workload_id} ${volume_name} = get_workload_volume_name ${workload_name} + # Validate input parameters + ${period} = Convert To Integer ${period} + Should Be True ${period} > 0 Period must be a positive integer + + # Use configurable interval (default 1 second) + ${interval} = Set Variable ${INTERVAL} # Define INTERVAL in variables section FOR ${i} IN RANGE ${period} ${expected_size_byte} = convert_size_to_bytes ${size} to_str=True ${current_size_byte} = get_volume_size ${volume_name} Should Be Equal ${current_size_byte} ${expected_size_byte} - Sleep 1 + Sleep ${interval} END
217-240: Add documentation for new keywordsPlease add documentation strings for the new keywords to describe:
- Purpose of each keyword
- Expected parameters and their formats
- Expected behavior and failure conditions
Example:
+[Documentation] Expands the volume of specified workload by the given size. +... Fails if the expansion is not possible. +... +... Arguments: +... - workload_kind: Type of the workload (e.g., pod, deployment) +... - workload_id: Identifier of the workload +... - size: Size to expand to (e.g., "1Gi", "500Mi") Expand ${workload_kind} ${workload_id} volume by ${size} ${workload_name} = generate_name_with_suffix ${workload_kind} ${workload_id} ${new_size} = convert_size_to_bytes ${size}e2e/libs/keywords/workload_keywords.py (1)
161-167: Add parameter documentation and type hintsThe implementation looks good and aligns with the PR objectives for testing PVC expansion beyond limits. Consider adding:
- Type hints for parameters
- Documentation for the
skip_retryparameter- def expand_workload_claim_size(self, workload_name, size_in_byte, claim_index=0, skip_retry=False): + def expand_workload_claim_size( + self, + workload_name: str, + size_in_byte: int, + claim_index: int = 0, + skip_retry: bool = False + ) -> None: + """Expand the PVC size for a workload. + + Args: + workload_name: Name of the workload + size_in_byte: Target size in bytes + claim_index: Index of the PVC to expand + skip_retry: If True, doesn't retry on expansion failure + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
e2e/keywords/persistentvolumeclaim.resource(2 hunks)e2e/keywords/statefulset.resource(0 hunks)e2e/keywords/workload.resource(2 hunks)e2e/libs/keywords/common_keywords.py(2 hunks)e2e/libs/keywords/persistentvolumeclaim_keywords.py(2 hunks)e2e/libs/keywords/volume_keywords.py(3 hunks)e2e/libs/keywords/workload_keywords.py(2 hunks)e2e/libs/node/node.py(2 hunks)e2e/libs/persistentvolumeclaim/crd.py(1 hunks)e2e/libs/persistentvolumeclaim/persistentvolumeclaim.py(4 hunks)e2e/libs/utility/utility.py(5 hunks)e2e/tests/regression/test_persistentvolumeclaim.robot(1 hunks)
💤 Files with no reviewable changes (1)
- e2e/keywords/statefulset.resource
🚧 Files skipped from review as they are similar to previous changes (6)
- e2e/libs/node/node.py
- e2e/libs/persistentvolumeclaim/crd.py
- e2e/libs/keywords/common_keywords.py
- e2e/libs/keywords/persistentvolumeclaim_keywords.py
- e2e/tests/regression/test_persistentvolumeclaim.robot
- e2e/libs/persistentvolumeclaim/persistentvolumeclaim.py
👮 Files not reviewed due to content moderation or server errors (3)
- e2e/keywords/persistentvolumeclaim.resource
- e2e/libs/utility/utility.py
- e2e/libs/keywords/volume_keywords.py
🧰 Additional context used
🪛 Ruff (0.8.0)
e2e/libs/keywords/volume_keywords.py
2-2: json imported but unused
Remove unused import: json
(F401)
🔇 Additional comments (4)
e2e/keywords/workload.resource (2)
201-204: LGTM: Formatting improvements
The spacing adjustments in variable assignments improve code readability while maintaining the same functionality.
217-221: LGTM: Volume expansion keywords implement required test scenarios
The implementation correctly handles both normal expansion and testing of expansion beyond maximum size. The use of skip_retry=True ensures proper failure handling.
Let's verify the implementation of the called functions:
Also applies to: 223-230
✅ Verification successful
Volume expansion implementation is correctly integrated with required dependencies
The verification confirms that all required functions are properly implemented:
convert_size_to_byteshandles size conversion with GiB unitsexpand_workload_claim_sizeretrieves current size and handles expansionget_volume_node_disk_storage_maximumcorrectly fetches storage limits from node disks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of volume expansion related functions
# Check for the implementation of convert_size_to_bytes
rg -A 5 "def convert_size_to_bytes"
# Check for the implementation of expand_workload_claim_size
rg -A 5 "def expand_workload_claim_size"
# Check for the implementation of get_volume_node_disk_storage_maximum
rg -A 5 "def get_volume_node_disk_storage_maximum"
Length of output: 2062
e2e/libs/keywords/workload_keywords.py (2)
31-31: LGTM! Import statement is correctly placed
The import of convert_size_to_bytes is properly organized and not duplicated anymore.
166-167: Verify error handling in the PersistentVolumeClaim expand method
Since this method is specifically for testing PVC expansion beyond limits, we should verify that the underlying expand method properly handles expansion failures.
✅ Verification successful
Let me generate additional verification scripts to check the error handling in the PersistentVolumeClaim expand method.
Let me search for the PersistentVolumeClaim class implementation since it wasn't found in the previous search.
Let me search for the CRD class implementation since it appears to handle the actual expansion operation.
Based on the search results, I can now generate the final response regarding the error handling in the PersistentVolumeClaim expand method.
Error handling for PVC expansion is properly implemented
The PersistentVolumeClaim expansion implementation includes proper error handling through:
- A retry mechanism with configurable retry count and interval
- Proper error propagation from the underlying Kubernetes API calls
- Skip retry option for testing scenarios that need to verify immediate failures
The implementation in the CRD class handles expansion errors by:
def expand(self, claim_name, size, namespace="default", skip_retry=False):
retry_count = 1 if skip_retry else self.retry_count
for i in range(retry_count):
logging(f"Trying to expand pvc {claim_name} to size {size} ... ({i})")
try:
self.core_v1_api.patch_namespaced_persistent_volume_claim(...)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check error handling in PersistentVolumeClaim expand method
# Look for error handling patterns in the expand method
# Search for the expand method implementation
ast-grep --pattern 'def expand($_, $_, skip_retry=$_) {
$$$
}'
# Search for error handling patterns
rg -A 5 'raise|except|error|fail' --type py
Length of output: 74112
Script:
#!/bin/bash
# Search for the PersistentVolumeClaim class and expand method implementation
ast-grep --pattern 'class PersistentVolumeClaim {
$$$
expand($_, $_, $_) {
$$$
}
$$$
}'
# Search for error handling patterns specifically in workload_keywords.py and related files
rg -A 5 'try|except|raise|error|fail' --type py e2e/libs/keywords/workload_keywords.py e2e/libs/keywords/persistentvolumeclaim.py
Length of output: 2075
Script:
#!/bin/bash
# Find the PersistentVolumeClaim class implementation
fd -t f -e py persistentvolumeclaim --exec rg -A 10 'class PersistentVolumeClaim' {}
# Search for expand method implementation
fd -t f -e py persistentvolumeclaim --exec rg -A 10 'def expand.*skip_retry' {}
# Look for error handling in the found files
fd -t f -e py persistentvolumeclaim --exec rg -A 5 'try|except|raise|error|fail' {}
Length of output: 2334
Script:
#!/bin/bash
# Find the CRD class implementation and its expand method
fd -t f -e py -e yaml -e json . --exec rg -A 10 'class CRD.*:' {}
# Look for expand method in CRD class
fd -t f -e py . --exec rg -A 10 'def expand.*skip_retry' {}
# Search for error handling in CRD operations
fd -t f -e py . --exec rg -A 5 'try|except|raise|error|fail.*CRD' {}
# Search for PVC expansion error handling patterns
rg -A 5 'try|except|raise|error|fail.*expand' --type py
Length of output: 69963
| Test persistentvolumeclaim expand more than storage maximum size should fail | ||
| [Tags] volume expansion | ||
| [Documentation] Verify that a PersistentVolumeClaim cannot be expanded beyond | ||
| ... the storage maximum size. | ||
| ... | ||
| ... Issue: https://github.com/longhorn/longhorn/issues/6633 | ||
|
|
||
| Given Set setting storage-over-provisioning-percentage to 100 | ||
| And Create persistentvolumeclaim 0 using RWX volume storage_size=2GiB | ||
| And Create deployment 0 with persistentvolumeclaim 0 | ||
| And Write 10 MB data to file data.txt in deployment 0 | ||
|
|
||
| FOR ${i} IN RANGE ${LOOP_COUNT} | ||
| When Expand deployment 0 volume more than storage maximum size should fail | ||
| Then Assert volume size of deployment 0 remains 2GiB for at least 5 seconds | ||
| And Assert persistentvolumeclaim 0 requested size remains 2GiB for at least 5 seconds | ||
| And Check deployment 0 data in file data.txt is intact | ||
|
|
||
| When Expand deployment 0 volume by 1 GiB | ||
| And Assert persistentvolumeclaim 0 requested size remains 3GiB for at least 5 seconds | ||
| And Assert volume size of deployment 0 remains 3GiB for at least 5 seconds | ||
| And Check deployment 0 data in file data.txt is intact | ||
| END |
There was a problem hiding this comment.
cc @WebberHuang1118 for expected behavior after fix.
c3y1huang
force-pushed
the
6633-prevent-resize-stuck
branch
from
07a8ed9 to
f3021f0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
e2e/keywords/workload.resource (2)
217-221: Consider adding documentation for the new keywordThe implementation looks good and aligns with the PR objectives. Consider adding documentation to describe the expected format of the
sizeparameter (e.g., "10Gi", "1000Mi").
232-240: Consider adding custom error messagesThe size assertion implementation is solid. Consider enhancing the error message in
Should Be Equalto include both the current and expected sizes for better debugging:- Should Be Equal ${current_size_byte} ${expected_size_byte} + Should Be Equal ${current_size_byte} ${expected_size_byte} + ... msg=Volume size mismatch after ${i}s: expected=${expected_size_byte}, got=${current_size_byte}e2e/libs/keywords/volume_keywords.py (1)
2-2: Remove unused importThe
jsonmodule is imported but never used in this file.-import json🧰 Tools
🪛 Ruff (0.8.0)
2-2:
jsonimported but unusedRemove unused import:
json(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
e2e/keywords/persistentvolumeclaim.resource(2 hunks)e2e/keywords/workload.resource(2 hunks)e2e/libs/keywords/common_keywords.py(2 hunks)e2e/libs/keywords/persistentvolumeclaim_keywords.py(2 hunks)e2e/libs/keywords/volume_keywords.py(3 hunks)e2e/libs/node/node.py(2 hunks)e2e/tests/regression/test_persistentvolumeclaim.robot(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- e2e/tests/regression/test_persistentvolumeclaim.robot
- e2e/libs/node/node.py
- e2e/libs/keywords/common_keywords.py
- e2e/libs/keywords/persistentvolumeclaim_keywords.py
- e2e/keywords/persistentvolumeclaim.resource
🧰 Additional context used
🪛 Ruff (0.8.0)
e2e/libs/keywords/volume_keywords.py
2-2: json imported but unused
Remove unused import: json
(F401)
🔇 Additional comments (4)
e2e/keywords/workload.resource (2)
201-204: LGTM: Formatting improvements
The whitespace adjustments improve code readability while maintaining the same functionality.
223-230: Verify specific error message content
The implementation correctly tests PVC expansion beyond maximum size. Consider enhancing the error message validation to ensure it contains specific details about the maximum size limit.
✅ Verification successful
Error message validation is appropriate as implemented
The current implementation using Failed to expand* as the expected error pattern follows a good practice by:
- Using a wildcard (*) which allows for dynamic content after the base message
- Focusing on the essential error condition without over-constraining the exact message
Since we couldn't find other error message patterns in the codebase for comparison, the current implementation appears to be appropriate and sufficient for the test case.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify error message format in similar test cases
rg -l 'Failed to expand' --type robot
Length of output: 39
Script:
#!/bin/bash
# Let's try a broader search for error message patterns in robot files
rg -A 3 'Run Keyword And Expect Error.*expand' --type robot
Length of output: 61
Script:
#!/bin/bash
# Let's search for any error message patterns in robot files to understand the convention
rg -A 2 'Run Keyword And Expect Error' --type robot
Length of output: 53
e2e/libs/keywords/volume_keywords.py (2)
343-345: LGTM!
The implementation correctly retrieves the volume size from the volume spec.
347-360: 🛠️ Refactor suggestion
Several improvements needed for robustness
The method implementation could be enhanced for better error handling and logging:
- Add length check for
replica_listbefore accessing first element - Use a more specific exception type
- Add logging when starting the search
- Improve error message clarity
Consider applying these improvements:
def get_volume_node_disk_storage_maximum(self, volume_name, node_name):
+ logging(f"Searching for maximum storage capacity for volume {volume_name} on node {node_name}")
replica_list = self.replica.get(volume_name, node_name)
+ if not replica_list:
+ raise ValueError(f"No replicas found for volume {volume_name} on node {node_name}")
replica = replica_list[0]
replica_name = replica['metadata']['name']
node = self.node.get_node_by_name(node_name, namespace=LONGHORN_NAMESPACE)
for diskName in node.disks:
disk = node.disks[diskName]
for scheduledReplica in disk['scheduledReplica']:
if scheduledReplica == replica_name:
logging(f"Found replica {scheduledReplica} on node {node_name} scheduled to disk {diskName}")
return disk['storageMaximum']
- raise Exception(f"Failed to find storageMaximum for volume {volume_name} replica {replica_name} on node {node_name}")
+ raise ValueError(
+ f"Failed to find storageMaximum for volume {volume_name} replica {replica_name} on node {node_name}. "
+ f"This could indicate that the replica is not scheduled to any disk."
+ )Likely invalid or redundant comment.
|
This pull request is now in conflict. Could you fix it @c3y1huang? 🙏 |
c3y1huang
force-pushed
the
6633-prevent-resize-stuck
branch
from
f3021f0 to
ed9c48e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
e2e/tests/regression/test_persistentvolumeclaim.robot (1)
29-44: LGTM: Comprehensive test implementation with a suggestionThe test implementation thoroughly covers both failure and success scenarios for volume expansion, including data integrity verification. However, consider increasing the
${LOOP_COUNT}from 1 to a higher value (e.g., 3) to better stress test the functionality and ensure consistent behavior across multiple attempts.-${LOOP_COUNT} 1 +${LOOP_COUNT} 3e2e/keywords/workload.resource (1)
227-250: LGTM: Well-implemented keywords with a suggestionThe new keywords provide a robust implementation for testing volume expansion scenarios. The implementation includes proper size conversion, error handling, and continuous size verification.
Consider making the error message verification more specific to ensure we're catching the exact expected error:
- Run Keyword And Expect Error Failed to expand* expand_workload_claim_size ${workload_name} ${new_size} skip_retry=True + Run Keyword And Expect Error Failed to expand volume: requested size * exceeds available storage* expand_workload_claim_size ${workload_name} ${new_size} skip_retry=Truee2e/libs/utility/utility.py (2)
Line range hint
179-187: Add retry logic tofilter_crfunctionThe function should include retry logic similar to
get_all_crsto handle transient API errors and maintain consistency with other functions in the module.def filter_cr(group, version, namespace, plural, field_selector="", label_selector=""): api = client.CustomObjectsApi() + retry_count, retry_interval = get_retry_count_and_interval() + for _ in range(retry_count): + try: + resp = api.list_namespaced_custom_object( + group, version, namespace, plural, + field_selector=field_selector, + label_selector=label_selector) + return resp + except ApiException as e: + logging(f"Listing namespaced custom object error: {e}") + time.sleep(retry_interval) - try: - resp = api.list_namespaced_custom_object(group, version, namespace, plural, field_selector=field_selector, label_selector=label_selector) - return resp - except ApiException as e: - logging(f"Listing namespaced custom object: {e}") + raise ApiException("Failed to list namespaced custom objects after retries")
343-355: Consider adding support for more size unitsThe implementation is good and follows best practices. Consider enhancing it with:
- Support for additional units (TiB, KiB)
- Input validation for negative numbers
- Support for decimal values
def convert_size_to_bytes(size): size = size.replace(" ", "") + if not size: + raise ValueError("Size cannot be empty") + + # Extract numeric part and validate + numeric_part = size[:-3] if size.endswith(("GiB", "MiB", "TiB", "KiB")) else size + try: + value = float(numeric_part) + if value < 0: + raise ValueError("Size cannot be negative") + except ValueError: + raise ValueError(f"Invalid numeric value in size: {size}") + + if size.endswith("TiB"): + return int(value * 1024 * 1024 * 1024 * 1024) if size.endswith("GiB"): - return int(size[:-3]) * 1024 * 1024 * 1024 + return int(value * 1024 * 1024 * 1024) if size.endswith("MiB"): - return int(size[:-3]) * 1024 * 1024 + return int(value * 1024 * 1024) + if size.endswith("KiB"): + return int(value * 1024) if size.isdigit(): return int(size) raise ValueError(f"Invalid size format: {size}")e2e/libs/backupstore/s3.py (3)
Line range hint
31-52: Security and resource management improvements needed in certificate handling.Several issues need to be addressed in the certificate handling:
- Writing to current working directory may fail due to permissions
- The certificate file is not cleaned up after use
- Missing error handling for file operations
Consider applying these improvements:
def get_api_client(self, minio_secret_name): secret = self.core_api.read_namespaced_secret(name=minio_secret_name, namespace='longhorn-system') # ... existing code ... - minio_cert_file_path = os.path.join(os.getcwd(), "minio_cert.crt") - with open(minio_cert_file_path, 'w') as minio_cert_file: - base64_minio_cert = \ - base64.b64decode(base64_minio_cert).decode("utf-8") - minio_cert_file.write(base64_minio_cert) - - os.environ["SSL_CERT_FILE"] = minio_cert_file_path + try: + cert_dir = tempfile.mkdtemp() + minio_cert_file_path = os.path.join(cert_dir, "minio_cert.crt") + with open(minio_cert_file_path, 'w') as minio_cert_file: + base64_minio_cert = \ + base64.b64decode(base64_minio_cert).decode("utf-8") + minio_cert_file.write(base64_minio_cert) + + os.environ["SSL_CERT_FILE"] = minio_cert_file_path + # ... rest of the method ... + finally: + if os.path.exists(cert_dir): + os.remove(minio_cert_file_path) + os.rmdir(cert_dir)
Line range hint
54-77: Add error handling and input validation for URL parsing.The URL parsing methods could be more robust with proper error handling and input validation.
Consider these improvements:
def get_backupstore_bucket_name(self): backupstore = self.backup_target + if not backupstore: + raise ValueError("Backup target URL is empty") assert self.is_backupTarget_s3(backupstore) - bucket_name = urlparse(backupstore).netloc.split('@')[0] + try: + parsed_url = urlparse(backupstore) + if not parsed_url.netloc: + raise ValueError("Invalid backup target URL format") + bucket_name = parsed_url.netloc.split('@')[0] + return bucket_name + except Exception as e: + raise ValueError(f"Failed to parse backup target URL: {str(e)}") - return bucket_name
Line range hint
79-199: Improve process management and error handling in file operations.Several operational improvements are needed:
- Port forwarding process cleanup in error cases
- Consistent error handling
- Proper logging instead of print statements
Consider applying these improvements:
def create_file_in_backupstore(self, file_path, data={}): - process = self.port_forward() + process = None + try: + process = self.port_forward() + secret_name = self.secret + if not secret_name: + raise ValueError("Secret name is required") - secret_name = self.secret + minio_api = self.get_api_client(secret_name) + bucket_name = self.get_backupstore_bucket_name() - minio_api = self.get_api_client(secret_name) - bucket_name = self.get_backupstore_bucket_name() + if len(data) == 0: + data = {"testkey": "test data from create_file_in_backupstore()"} - if len(data) == 0: - data = {"testkey": "test data from create_file_in_backupstore()"} + with tempfile.NamedTemporaryFile('w') as fp: + json.dump(data, fp) + fp.flush() + with open(fp.name, mode='rb') as f: + temp_file_stat = os.stat(fp.name) + minio_api.put_object(bucket_name, + file_path, + f, + temp_file_stat.st_size) + read_back = minio_api.get_object(bucket_name, + file_path) + assert read_back.data.decode("utf-8") == json.dumps(data) + logging(f"Created file {file_path} in backupstore") + except ResponseError as err: + logging(f"Error in create_file_in_backupstore: {str(err)}") + raise + except Exception as e: + logging(f"Unexpected error in create_file_in_backupstore: {str(e)}") + raise + finally: + if process: + process.kill()Similar improvements should be applied to other file operation methods for consistency.
e2e/libs/keywords/volume_keywords.py (1)
2-2: Remove unused importThe
jsonmodule is imported but never used in the code.-import json🧰 Tools
🪛 Ruff (0.8.0)
2-2:
jsonimported but unusedRemove unused import:
json(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (23)
e2e/keywords/persistentvolumeclaim.resource(2 hunks)e2e/keywords/statefulset.resource(0 hunks)e2e/keywords/workload.resource(2 hunks)e2e/libs/backup/backup.py(1 hunks)e2e/libs/backup/base.py(1 hunks)e2e/libs/backup/rest.py(1 hunks)e2e/libs/backupstore/base.py(1 hunks)e2e/libs/backupstore/nfs.py(1 hunks)e2e/libs/backupstore/s3.py(1 hunks)e2e/libs/engine_image/engine_image.py(1 hunks)e2e/libs/host/aws.py(1 hunks)e2e/libs/host/harvester.py(1 hunks)e2e/libs/k8s/k8s.py(1 hunks)e2e/libs/keywords/common_keywords.py(2 hunks)e2e/libs/keywords/persistentvolumeclaim_keywords.py(2 hunks)e2e/libs/keywords/volume_keywords.py(3 hunks)e2e/libs/keywords/workload_keywords.py(2 hunks)e2e/libs/node/node.py(2 hunks)e2e/libs/persistentvolumeclaim/crd.py(1 hunks)e2e/libs/persistentvolumeclaim/persistentvolumeclaim.py(4 hunks)e2e/libs/utility/utility.py(5 hunks)e2e/libs/volume/crd.py(0 hunks)e2e/tests/regression/test_persistentvolumeclaim.robot(1 hunks)
💤 Files with no reviewable changes (2)
- e2e/libs/volume/crd.py
- e2e/keywords/statefulset.resource
🚧 Files skipped from review as they are similar to previous changes (16)
- e2e/libs/backupstore/base.py
- e2e/libs/backupstore/nfs.py
- e2e/libs/keywords/common_keywords.py
- e2e/libs/engine_image/engine_image.py
- e2e/libs/persistentvolumeclaim/crd.py
- e2e/libs/backup/backup.py
- e2e/keywords/persistentvolumeclaim.resource
- e2e/libs/keywords/persistentvolumeclaim_keywords.py
- e2e/libs/persistentvolumeclaim/persistentvolumeclaim.py
- e2e/libs/host/harvester.py
- e2e/libs/k8s/k8s.py
- e2e/libs/backup/rest.py
- e2e/libs/node/node.py
- e2e/libs/host/aws.py
- e2e/libs/keywords/workload_keywords.py
- e2e/libs/backup/base.py
🧰 Additional context used
🪛 Ruff (0.8.0)
e2e/libs/keywords/volume_keywords.py
2-2: json imported but unused
Remove unused import: json
(F401)
🔇 Additional comments (10)
e2e/tests/regression/test_persistentvolumeclaim.robot (3)
1-11: LGTM: Test setup and resources are well-structured
The test file has proper documentation, regression tagging, and includes all necessary resources for PVC, deployment, and workload management.
12-19: LGTM: Test configuration is appropriate
The setup/teardown procedures are in place, and the retry parameters (300 retries with 1-second intervals) are suitable for volume operations that may take time to complete.
22-28: LGTM: Test documentation is clear and well-linked
The test case documentation clearly states its purpose and links to the relevant issue (#6633), providing good traceability.
e2e/keywords/workload.resource (1)
202-205: Formatting changes only
These are purely formatting changes that don't affect functionality.
e2e/libs/utility/utility.py (2)
Line range hint 188-202: Replace assertion with explicit exception
Replace assert False with a proper exception to maintain consistency with error handling patterns and prevent issues when running Python with -O flag.
for i in range(retry_count):
try:
resp = api.list_namespaced_pod(
namespace=namespace,
label_selector=label_selector)
return resp.items
except Exception as e:
logging(f"Failed to list namespaced {namespace} pods with error: {e}")
time.sleep(retry_interval)
- assert False, f"Failed to list namespaced {namespace} pods"
+ raise ApiException(f"Failed to list namespaced {namespace} pods after {retry_count} retries")Line range hint 203-231: LGTM! Well-implemented conflict handling
The addition of conflict error handling with proper retry logic is a good improvement. The implementation correctly handles 409 conflicts while preserving other error cases.
🧰 Tools
🪛 Ruff (0.8.0)
201-201: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
e2e/libs/backupstore/s3.py (1)
1-14: LGTM! Import statements and class definition are well organized.
The imports are logically grouped and the inheritance from Base class is properly defined.
e2e/libs/keywords/volume_keywords.py (3)
350-352: LGTM!
The implementation is clean and correctly retrieves the volume size from its specification.
354-367: 🛠️ Refactor suggestion
Improve robustness and readability
The method could benefit from several improvements:
- Add validation for empty replica list
- Use specific exceptions instead of bare Exception
- Simplify nested loops with early returns
- Add safer dictionary access
Consider this improved implementation:
def get_volume_node_disk_storage_maximum(self, volume_name, node_name):
replica_list = self.replica.get(volume_name, node_name)
+ if not replica_list:
+ raise ValueError(f"No replicas found for volume {volume_name} on node {node_name}")
+
replica = replica_list[0]
replica_name = replica['metadata']['name']
node = self.node.get_node_by_name(node_name, namespace=LONGHORN_NAMESPACE)
+
for diskName in node.disks:
- disk = node.disks[diskName]
+ disk = node.disks.get(diskName, {})
+ scheduled_replicas = disk.get('scheduledReplica', [])
+ if replica_name in scheduled_replicas:
+ logging(f"Found replica {replica_name} on node {node_name} scheduled to disk {diskName}")
+ return disk.get('storageMaximum')
- for scheduledReplica in disk['scheduledReplica']:
- if scheduledReplica == replica_name:
- logging(f"Found replica {scheduledReplica} on node {node_name} scheduled to disk {diskName}")
- return disk['storageMaximum']
- raise Exception(f"Failed to find storageMaximum for volume {volume_name} replica {replica_name} on node {node_name}")
+ raise ValueError(
+ f"Failed to find storageMaximum for volume {volume_name} replica {replica_name} on node {node_name}. "
+ "Ensure the replica is properly scheduled to a disk on the specified node."
+ )Likely invalid or redundant comment.
350-367: Verify test coverage for new methods
The new methods appear to support testing PVC expansion beyond storage maximum size, but let's verify their usage in test cases.
c3y1huang
force-pushed
the
6633-prevent-resize-stuck
branch
from
ed9c48e to
7a9098b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
e2e/keywords/persistentvolumeclaim.resource (1)
27-34: Consider parameterizing the sleep durationThe sleep duration is hardcoded to 1 second. Consider making it configurable for different test scenarios.
Assert persistentvolumeclaim ${claim_id} requested size remains ${size} for at least ${period} seconds + [Arguments] ${sleep_duration}=1 ${claim_name} = generate_name_with_suffix claim ${claim_id} FOR ${i} IN RANGE ${period} ${expected_size_byte} = convert_size_to_bytes ${size} to_str=True ${current_size_byte} = get_claim_requested_size ${claim_name} Should Be Equal ${current_size_byte} ${expected_size_byte} - Sleep 1 + Sleep ${sleep_duration} ENDe2e/keywords/workload.resource (2)
242-249: Consider adding size validation in error message.The keyword correctly tests expansion failure, but could be more informative.
Consider capturing and validating the error message content:
- Run Keyword And Expect Error Failed to expand* expand_workload_claim_size ${workload_name} ${new_size} skip_retry=True + ${error_msg}= Run Keyword And Expect Error Failed to expand* expand_workload_claim_size ${workload_name} ${new_size} skip_retry=True + Should Contain ${error_msg} exceeds maximum size msg=Expected error message about exceeding maximum size
251-259: Consider adding tolerance for size comparison.The size verification is good but might be too strict.
Consider adding a small tolerance when comparing sizes to account for potential rounding differences:
- Should Be Equal ${current_size_byte} ${expected_size_byte} + ${diff}= Evaluate abs(${current_size_byte} - ${expected_size_byte}) + Should Be True ${diff} < 1024 msg=Size difference exceeds 1KB tolerancee2e/libs/keywords/volume_keywords.py (1)
2-2: Remove unused import.The
jsonimport is not used in this file.-import json🧰 Tools
🪛 Ruff (0.8.0)
2-2:
jsonimported but unusedRemove unused import:
json(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (23)
e2e/keywords/persistentvolumeclaim.resource(2 hunks)e2e/keywords/statefulset.resource(0 hunks)e2e/keywords/workload.resource(2 hunks)e2e/libs/backup/backup.py(1 hunks)e2e/libs/backup/base.py(1 hunks)e2e/libs/backup/rest.py(1 hunks)e2e/libs/backupstore/base.py(1 hunks)e2e/libs/backupstore/nfs.py(1 hunks)e2e/libs/backupstore/s3.py(1 hunks)e2e/libs/engine_image/engine_image.py(1 hunks)e2e/libs/host/aws.py(1 hunks)e2e/libs/host/harvester.py(1 hunks)e2e/libs/k8s/k8s.py(1 hunks)e2e/libs/keywords/common_keywords.py(2 hunks)e2e/libs/keywords/persistentvolumeclaim_keywords.py(2 hunks)e2e/libs/keywords/volume_keywords.py(3 hunks)e2e/libs/keywords/workload_keywords.py(2 hunks)e2e/libs/node/node.py(2 hunks)e2e/libs/persistentvolumeclaim/crd.py(1 hunks)e2e/libs/persistentvolumeclaim/persistentvolumeclaim.py(4 hunks)e2e/libs/utility/utility.py(5 hunks)e2e/libs/volume/crd.py(0 hunks)e2e/tests/regression/test_persistentvolumeclaim.robot(1 hunks)
💤 Files with no reviewable changes (2)
- e2e/keywords/statefulset.resource
- e2e/libs/volume/crd.py
🚧 Files skipped from review as they are similar to previous changes (15)
- e2e/libs/backupstore/base.py
- e2e/libs/backupstore/nfs.py
- e2e/libs/keywords/common_keywords.py
- e2e/libs/node/node.py
- e2e/libs/persistentvolumeclaim/crd.py
- e2e/libs/keywords/workload_keywords.py
- e2e/libs/backup/backup.py
- e2e/libs/k8s/k8s.py
- e2e/libs/host/harvester.py
- e2e/libs/engine_image/engine_image.py
- e2e/libs/keywords/persistentvolumeclaim_keywords.py
- e2e/libs/backupstore/s3.py
- e2e/libs/backup/rest.py
- e2e/libs/host/aws.py
- e2e/libs/backup/base.py
👮 Files not reviewed due to content moderation or server errors (1)
- e2e/libs/utility/utility.py
🧰 Additional context used
🪛 Ruff (0.8.0)
e2e/libs/keywords/volume_keywords.py
2-2: json imported but unused
Remove unused import: json
(F401)
🔇 Additional comments (7)
e2e/libs/persistentvolumeclaim/persistentvolumeclaim.py (2)
27-29: LGTM: Clean implementation of storage size parameter
The addition of the storage_size parameter with proper size conversion is well implemented.
Also applies to: 44-45
104-110:
Add size validation before expansion
To prevent volume resize stuck issues (longhorn/longhorn#6633), consider adding validation before attempting expansion.
def expand(self, claim_name, size_in_byte, skip_retry=False):
pvc = self.claim.get(claim_name)
current_size = int(pvc.spec.resources.requests['storage'])
target_size = current_size + size_in_byte
+ # Validate target size against system capacity
+ if target_size > self._get_max_storage_size():
+ raise ValueError(f"Target size {target_size} exceeds maximum storage capacity")
logging(f"Expanding PVC {claim_name} from {current_size} to {target_size}")
expanded_size = self.claim.expand(claim_name, target_size, skip_retry=skip_retry)e2e/keywords/persistentvolumeclaim.resource (1)
11-13: LGTM: Flexible PVC creation with config parameter
The addition of the config parameter allows for more flexible PVC creation scenarios.
e2e/tests/regression/test_persistentvolumeclaim.robot (1)
22-44: LGTM: Well-structured test case with comprehensive verification
The test case effectively validates:
- Failure when expanding beyond max size
- Success when expanding within limits
- Data integrity throughout the process
The BDD style makes the test flow clear and maintainable.
e2e/keywords/workload.resource (1)
236-240: LGTM! Clean implementation of volume expansion.
The keyword correctly converts the size and expands the volume.
e2e/libs/keywords/volume_keywords.py (2)
350-352: LGTM! Clean implementation of volume size retrieval.
The method correctly retrieves the volume size from the spec.
354-367: 🛠️ Refactor suggestion
Add input validation and use specific exceptions.
The method needs better error handling and input validation.
Consider these improvements:
def get_volume_node_disk_storage_maximum(self, volume_name, node_name):
replica_list = self.replica.get(volume_name, node_name)
+ if not replica_list:
+ raise ValueError(f"No replicas found for volume {volume_name} on node {node_name}")
replica = replica_list[0]
replica_name = replica['metadata']['name']
node = self.node.get_node_by_name(node_name, namespace=LONGHORN_NAMESPACE)
for diskName in node.disks:
disk = node.disks[diskName]
for scheduledReplica in disk['scheduledReplica']:
if scheduledReplica == replica_name:
logging(f"Found replica {scheduledReplica} on node {node_name} scheduled to disk {diskName}")
return disk['storageMaximum']
- raise Exception(f"Failed to find storageMaximum for volume {volume_name} replica {replica_name} on node {node_name}")
+ raise ValueError(
+ f"Failed to find storageMaximum for volume {volume_name} replica {replica_name} on node {node_name}. "
+ f"Ensure the replica is properly scheduled to a disk on the specified node."
+ )Likely invalid or redundant comment.
c3y1huang
force-pushed
the
6633-prevent-resize-stuck
branch
from
7a9098b to
20a04e4
Compare
c3y1huang
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (10)
e2e/tests/regression/test_persistentvolumeclaim.robot (2)
15-19: Consider documenting the variables.Add comments explaining the purpose and impact of these variables:
LOOP_COUNT: Number of times to repeat the expansion testRETRY_COUNT: Maximum number of retries for operationsRETRY_INTERVAL: Time to wait between retries
34-44: Consider adding validation for the initial data write.The test writes data and verifies it remains intact after volume operations, but doesn't validate the initial write was successful.
Add a verification step after the initial data write:
And Write 10 MB data to file data.txt in deployment 0 + And Check deployment 0 data in file data.txt is intact FOR ${i} IN RANGE ${LOOP_COUNT}e2e/libs/utility/utility.py (1)
Line range hint
167-202: Consider adding error handling for empty responses.The CR management functions should handle cases where the API returns empty responses.
Add null checks:
def get_all_crs(group, version, namespace, plural): api = client.CustomObjectsApi() retry_count, retry_interval = get_retry_count_and_interval() for _ in range(retry_count): try: resp = api.list_namespaced_custom_object(group, version, namespace, plural) + if not resp: + logging("Empty response from API") + return [] return respe2e/keywords/workload.resource (3)
236-240: Consider adding documentation for the new keywordThe implementation looks good, but adding documentation would help users understand:
- The expected format of the
sizeparameter- Any prerequisites or post-conditions
Add documentation like this:
+[Documentation] Expands the volume size of the specified workload. +... The size parameter should be in format like "1Gi", "500Mi", etc. Expand ${workload_kind} ${workload_id} volume by ${size}
242-249: Consider enhancing error message verificationThe implementation correctly tests expansion beyond maximum size. However, the error message verification could be more specific.
Consider using a more specific error pattern:
- Run Keyword And Expect Error Failed to expand* expand_workload_claim_size ${workload_name} ${new_size} skip_retry=True + Run Keyword And Expect Error Failed to expand*exceeds maximum storage size* expand_workload_claim_size ${workload_name} ${new_size} skip_retry=True
251-259: Consider enhancing failure message for better debuggingThe implementation provides good verification of size stability. However, the error message could be more informative.
Consider adding more context to the size comparison:
- Should Be Equal ${current_size_byte} ${expected_size_byte} + Should Be Equal ${current_size_byte} ${expected_size_byte} msg=Volume ${volume_name} size changed: expected=${expected_size_byte}, got=${current_size_byte}e2e/libs/keywords/volume_keywords.py (1)
2-2: Remove unused importThe
jsonmodule is imported but never used in this file.-import json🧰 Tools
🪛 Ruff (0.8.0)
2-2:
jsonimported but unusedRemove unused import:
json(F401)
e2e/libs/backup/rest.py (3)
Line range hint
20-43: Consider adding additional backup verification checks.While the current implementation verifies backup completion and timestamp, consider adding checks for:
- Backup size validation
- Backup status verification (e.g., health check)
- Storage capacity verification (aligning with PR objectives regarding volume resize issues)
Example addition:
assert volume.lastBackupAt != "", \ f"expect volume lastBackupAt is not empty, but it's {volume.lastBackupAt}" +# Verify backup size and storage capacity +assert backup.size > 0, f"expect backup size greater than 0, but got {backup.size}" +assert backup.status == "Ready", f"expect backup status Ready, but got {backup.status}"
Line range hint
45-89: Enhance error handling in list method.The list method's handling of None volume_name could be improved with proper error handling and logging.
Consider this improvement:
def list(self, volume_name): if not volume_name: backup_list = [] vol_list = self.volume.list() + logging(f"Listing backups for all volumes: {vol_list}") for volume_name in vol_list: + try: backup_volume = self.get_backup_volume(volume_name) if backup_volume: backup_list.extend(backup_volume.backupList().data) + except Exception as e: + logging(f"Error listing backups for volume {volume_name}: {e}") return backup_list else: + logging(f"Listing backups for volume: {volume_name}") backup_volume = self.get_backup_volume(volume_name) + if not backup_volume: + logging(f"No backup volume found for {volume_name}") + return [] return backup_volume.backupList().data
Line range hint
183-219: Add safety checks before cleanup operations.While the cleanup methods handle errors well, consider adding safety checks to prevent disrupting ongoing operations.
Consider these additions:
def cleanup_backup_volumes(self): + # Check for any ongoing backup operations + active_operations = self._check_active_operations() + if active_operations: + logging(f"Skipping cleanup due to active operations: {active_operations}") + return + backup_volumes = get_longhorn_client().list_backup_volume() # we delete the whole backup volume, which skips block gc for backup_volume in backup_volumes: self.delete_backup_volume(backup_volume.name)Also, consider adding a method to check for active operations:
def _check_active_operations(self): """Check for any ongoing backup or restore operations.""" volumes = self.volume.list() active_ops = [] for vol_name in volumes: vol = self.volume.get(vol_name) if any(status.state == "InProgress" for status in vol.backupStatus): active_ops.append(f"backup-{vol_name}") return active_ops
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (23)
e2e/keywords/persistentvolumeclaim.resource(2 hunks)e2e/keywords/statefulset.resource(0 hunks)e2e/keywords/workload.resource(2 hunks)e2e/libs/backup/backup.py(1 hunks)e2e/libs/backup/base.py(1 hunks)e2e/libs/backup/rest.py(1 hunks)e2e/libs/backupstore/base.py(1 hunks)e2e/libs/backupstore/nfs.py(1 hunks)e2e/libs/backupstore/s3.py(1 hunks)e2e/libs/engine_image/engine_image.py(1 hunks)e2e/libs/host/aws.py(1 hunks)e2e/libs/host/harvester.py(1 hunks)e2e/libs/k8s/k8s.py(1 hunks)e2e/libs/keywords/common_keywords.py(2 hunks)e2e/libs/keywords/persistentvolumeclaim_keywords.py(2 hunks)e2e/libs/keywords/volume_keywords.py(3 hunks)e2e/libs/keywords/workload_keywords.py(2 hunks)e2e/libs/node/node.py(2 hunks)e2e/libs/persistentvolumeclaim/crd.py(1 hunks)e2e/libs/persistentvolumeclaim/persistentvolumeclaim.py(4 hunks)e2e/libs/utility/utility.py(5 hunks)e2e/libs/volume/crd.py(0 hunks)e2e/tests/regression/test_persistentvolumeclaim.robot(1 hunks)
💤 Files with no reviewable changes (2)
- e2e/keywords/statefulset.resource
- e2e/libs/volume/crd.py
🚧 Files skipped from review as they are similar to previous changes (15)
- e2e/libs/backupstore/base.py
- e2e/libs/keywords/common_keywords.py
- e2e/libs/k8s/k8s.py
- e2e/libs/node/node.py
- e2e/libs/engine_image/engine_image.py
- e2e/libs/persistentvolumeclaim/crd.py
- e2e/keywords/persistentvolumeclaim.resource
- e2e/libs/keywords/persistentvolumeclaim_keywords.py
- e2e/libs/backupstore/nfs.py
- e2e/libs/backup/backup.py
- e2e/libs/persistentvolumeclaim/persistentvolumeclaim.py
- e2e/libs/host/harvester.py
- e2e/libs/backupstore/s3.py
- e2e/libs/host/aws.py
- e2e/libs/backup/base.py
🧰 Additional context used
🪛 Ruff (0.8.0)
e2e/libs/keywords/volume_keywords.py
2-2: json imported but unused
Remove unused import: json
(F401)
🔇 Additional comments (9)
e2e/tests/regression/test_persistentvolumeclaim.robot (2)
1-14: LGTM! Well-structured test suite setup.
The test suite is properly configured with:
- Clear documentation
- Appropriate test tags
- Required resources
- Test setup/teardown hooks
22-28: LGTM! Well-documented test case.
The test case includes:
- Clear tags for categorization
- Comprehensive documentation
- Reference to the related GitHub issue
e2e/libs/keywords/workload_keywords.py (1)
161-167: LGTM! Improved size handling in expansion method.
The method now:
- Properly retrieves current size
- Converts sizes to bytes for comparison
- Supports retry control through
skip_retryparameter
e2e/libs/utility/utility.py (1)
343-355: LGTM! Well-implemented size conversion function.
The function:
- Handles common size formats (GiB, MiB)
- Properly validates input
- Uses appropriate error handling
e2e/keywords/workload.resource (1)
211-214: LGTM: Formatting improvements
The spacing adjustments improve code consistency.
e2e/libs/keywords/volume_keywords.py (2)
350-352: LGTM!
The implementation correctly retrieves the volume size from the volume spec.
354-367: 🛠️ Refactor suggestion
Several improvements needed for robustness
The method implementation needs the following improvements:
- Add validation for
replica_listbefore accessing the first element - Enhance error message with more context
- Consider simplifying the nested loops using list comprehension or helper methods
Consider applying these improvements:
def get_volume_node_disk_storage_maximum(self, volume_name, node_name):
replica_list = self.replica.get(volume_name, node_name)
+ if not replica_list:
+ raise ValueError(f"No replicas found for volume {volume_name} on node {node_name}")
replica = replica_list[0]
replica_name = replica['metadata']['name']
node = self.node.get_node_by_name(node_name, namespace=LONGHORN_NAMESPACE)
- for diskName in node.disks:
- disk = node.disks[diskName]
-
- for scheduledReplica in disk['scheduledReplica']:
- if scheduledReplica == replica_name:
- logging(f"Found replica {scheduledReplica} on node {node_name} scheduled to disk {diskName}")
- return disk['storageMaximum']
+ # Find the disk containing our replica
+ matching_disks = [
+ (name, disk) for name, disk in node.disks.items()
+ if replica_name in disk['scheduledReplica']
+ ]
+ if matching_disks:
+ disk_name, disk = matching_disks[0]
+ logging(f"Found replica {replica_name} on node {node_name} scheduled to disk {disk_name}")
+ return disk['storageMaximum']
- raise Exception(f"Failed to find storageMaximum for volume {volume_name} replica {replica_name} on node {node_name}")
+ raise ValueError(
+ f"Failed to find storageMaximum for volume {volume_name} replica {replica_name} on node {node_name}. "
+ f"Ensure the replica is properly scheduled to a disk on the specified node."
+ )The improvements:
- Add validation for empty replica list
- Replace nested loops with a more readable list comprehension
- Use
ValueErrorwith a more descriptive error message - Improve variable naming (camelCase to snake_case for consistency)
Likely invalid or redundant comment.
e2e/libs/backup/rest.py (2)
1-14: LGTM! Clean import organization and class setup.
The imports are well-organized, and the renaming of Snapshot to RestSnapshot helps avoid naming conflicts.
Line range hint 1-219: Verify alignment with PR objectives regarding PVC expansion testing.
While the changes improve backup handling, I don't see direct implementation of tests for PVC expansion beyond maximum size (as mentioned in PR objectives - longhorn/longhorn#6633). Consider adding specific test cases for:
- Attempting to expand PVC beyond available capacity
- Verifying proper error handling when expansion fails
- Ensuring volumes don't get stuck in resizing state
Signed-off-by: Chin-Ya Huang <[email protected]>
longhorn/longhorn-6633 Signed-off-by: Chin-Ya Huang <[email protected]>
c3y1huang
force-pushed
the
6633-prevent-resize-stuck
branch
from
20a04e4 to
38c8f66
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
e2e/libs/keywords/workload_keywords.py (1)
161-167: Consider adding size validation before expansionThe implementation correctly handles size conversion and expansion. However, to address the core issue from longhorn/longhorn#6633 about preventing volumes from getting stuck, consider adding validation to check if the requested size exceeds the maximum available capacity before attempting expansion.
def expand_workload_claim_size(self, workload_name, size_in_byte, claim_index=0, skip_retry=False): claim_name = get_workload_persistent_volume_claim_name(workload_name, index=claim_index) current_size = self.persistentvolumeclaim.get(claim_name).spec.resources.requests['storage'] current_size_byte = convert_size_to_bytes(current_size) + # Get maximum available capacity + max_size = self.persistentvolumeclaim.get_maximum_available_size() + if size_in_byte > max_size: + raise ValueError(f"Requested size {size_in_byte} exceeds maximum available capacity {max_size}") logging(f'Expanding {workload_name} persistentvolumeclaim {claim_name} from {current_size_byte} to {size_in_byte}') self.persistentvolumeclaim.expand(claim_name, size_in_byte, skip_retry=skip_retry)e2e/libs/utility/utility.py (3)
Line range hint
179-187: Add retry logic for API call resilience.The function should implement retry logic similar to other API calls in the file to handle transient errors.
def filter_cr(group, version, namespace, plural, field_selector="", label_selector=""): api = client.CustomObjectsApi() + retry_count, retry_interval = get_retry_count_and_interval() + for _ in range(retry_count): + try: + resp = api.list_namespaced_custom_object(group, version, namespace, plural, field_selector=field_selector, label_selector=label_selector) + return resp + except ApiException as e: + logging(f"Listing namespaced custom object: {e}") + time.sleep(retry_interval) - try: - resp = api.list_namespaced_custom_object(group, version, namespace, plural, field_selector=field_selector, label_selector=label_selector) - return resp - except ApiException as e: - logging(f"Listing namespaced custom object: {e}")
Line range hint
188-202: Replace assert with proper exception handling.The assert statement can be disabled with Python -O flag. Use an explicit exception instead.
for i in range(retry_count): try: resp = api.list_namespaced_pod( namespace=namespace, label_selector=label_selector) return resp.items except Exception as e: logging(f"Failed to list namespaced {namespace} pods with error: {e}") time.sleep(retry_interval) - assert False, f"Failed to list namespaced {namespace} pods" + raise RuntimeError(f"Failed to list namespaced {namespace} pods after {retry_count} retries")
343-355: Consider supporting additional size formats.The function handles common size formats well, but could be enhanced to support more formats like KiB and TiB for better flexibility.
def convert_size_to_bytes(size): size = size.replace(" ", "") + if size.endswith("TiB"): + return int(size[:-3]) * 1024 * 1024 * 1024 * 1024 + if size.endswith("GiB"): return int(size[:-3]) * 1024 * 1024 * 1024 if size.endswith("MiB"): return int(size[:-3]) * 1024 * 1024 + if size.endswith("KiB"): + return int(size[:-3]) * 1024 + if size.isdigit(): return int(size) raise ValueError(f"Invalid size format: {size}")e2e/libs/backup/rest.py (1)
Line range hint
186-204: Consider improving retry logic for backup verificationWhile the retry mechanism is good, consider adding a maximum retry count and logging the current attempt number for better debugging.
Here's a suggested improvement:
def assert_all_backups_before_uninstall_exist(self, backups_before_uninstall): + success = False for i in range(self.retry_count): + logging(f"Attempt {i + 1}/{self.retry_count} to verify backups") time.sleep(self.retry_interval) try: current_backups = self.list_all() assert len(current_backups["items"]) == len(backups_before_uninstall["items"]) + success = True break except Exception as e: logging(f"{e}") continue + if not success: + raise AssertionError("Failed to verify backups after maximum retries")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (23)
e2e/keywords/persistentvolumeclaim.resource(2 hunks)e2e/keywords/statefulset.resource(0 hunks)e2e/keywords/workload.resource(2 hunks)e2e/libs/backup/backup.py(1 hunks)e2e/libs/backup/base.py(1 hunks)e2e/libs/backup/rest.py(1 hunks)e2e/libs/backupstore/base.py(1 hunks)e2e/libs/backupstore/nfs.py(1 hunks)e2e/libs/backupstore/s3.py(1 hunks)e2e/libs/engine_image/engine_image.py(1 hunks)e2e/libs/host/aws.py(1 hunks)e2e/libs/host/harvester.py(1 hunks)e2e/libs/k8s/k8s.py(1 hunks)e2e/libs/keywords/common_keywords.py(2 hunks)e2e/libs/keywords/persistentvolumeclaim_keywords.py(2 hunks)e2e/libs/keywords/volume_keywords.py(3 hunks)e2e/libs/keywords/workload_keywords.py(2 hunks)e2e/libs/node/node.py(2 hunks)e2e/libs/persistentvolumeclaim/crd.py(1 hunks)e2e/libs/persistentvolumeclaim/persistentvolumeclaim.py(4 hunks)e2e/libs/utility/utility.py(5 hunks)e2e/libs/volume/crd.py(0 hunks)e2e/tests/regression/test_persistentvolumeclaim.robot(1 hunks)
💤 Files with no reviewable changes (2)
- e2e/keywords/statefulset.resource
- e2e/libs/volume/crd.py
🚧 Files skipped from review as they are similar to previous changes (17)
- e2e/libs/backupstore/base.py
- e2e/libs/keywords/common_keywords.py
- e2e/libs/node/node.py
- e2e/tests/regression/test_persistentvolumeclaim.robot
- e2e/libs/backupstore/nfs.py
- e2e/libs/engine_image/engine_image.py
- e2e/libs/host/harvester.py
- e2e/libs/backup/backup.py
- e2e/libs/keywords/persistentvolumeclaim_keywords.py
- e2e/libs/persistentvolumeclaim/persistentvolumeclaim.py
- e2e/libs/k8s/k8s.py
- e2e/keywords/persistentvolumeclaim.resource
- e2e/libs/host/aws.py
- e2e/keywords/workload.resource
- e2e/libs/backupstore/s3.py
- e2e/libs/persistentvolumeclaim/crd.py
- e2e/libs/backup/base.py
🧰 Additional context used
🪛 Ruff (0.8.0)
e2e/libs/keywords/volume_keywords.py
2-2: json imported but unused
Remove unused import: json
(F401)
🔇 Additional comments (9)
e2e/libs/keywords/workload_keywords.py (1)
31-31: Remove duplicate import statement
The convert_size_to_bytes function is imported twice.
e2e/libs/utility/utility.py (1)
Line range hint 167-178: LGTM!
The implementation includes proper error handling and retry logic for retrieving custom resources.
e2e/libs/keywords/volume_keywords.py (2)
350-352: LGTM!
The function correctly retrieves the volume size from the volume specification.
354-367: 🛠️ Refactor suggestion
Add input validation for replica list.
The function should validate that replica_list is not empty before accessing its first element to prevent IndexError.
def get_volume_node_disk_storage_maximum(self, volume_name, node_name):
replica_list = self.replica.get(volume_name, node_name)
+ if not replica_list:
+ raise ValueError(f"No replicas found for volume {volume_name} on node {node_name}")
replica = replica_list[0]
replica_name = replica['metadata']['name']
node = self.node.get_node_by_name(node_name, namespace=LONGHORN_NAMESPACE)
for diskName in node.disks:
disk = node.disks[diskName]
for scheduledReplica in disk['scheduledReplica']:
if scheduledReplica == replica_name:
logging(f"Found replica {scheduledReplica} on node {node_name} scheduled to disk {diskName}")
return disk['storageMaximum']
- raise Exception(f"Failed to find storageMaximum for volume {volume_name} replica {replica_name} on node {node_name}")
+ raise ValueError(
+ f"Failed to find storageMaximum for volume {volume_name} replica {replica_name} on node {node_name}. "
+ f"Ensure the replica is properly scheduled to a disk on the specified node."
+ )Likely invalid or redundant comment.
e2e/libs/backup/rest.py (5)
1-11: LGTM: Import changes are well-organized
The imports are properly structured with clear aliases to avoid naming conflicts.
Line range hint 39-40: LGTM: Good addition of backup timestamp validation
The new assertion ensures that the backup timestamp is properly set after completion, which is a valuable validation step.
Line range hint 82-86: LGTM: Improved error handling with detailed logging
The enhanced error handling with specific logging for different error types will make debugging backup metadata issues much easier.
Line range hint 98-99: LGTM: Enhanced logging for backup operations
The added logging statements improve visibility into the backup retrieval process and help track potential issues.
Also applies to: 106-107
Line range hint 1-275: Verify coverage of PVC expansion testing
While the error handling and logging improvements are good, we should verify if this file adequately supports testing PVC expansion beyond maximum size as mentioned in the PR objectives (longhorn/longhorn#6633).
c3y1huang
force-pushed
the
6633-prevent-resize-stuck
branch
from
38c8f66 to
43a1a09
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
e2e/tests/negative/stress_filesystem.robot (1)
Line range hint
1-72: Add test documentation for storage requirementsThe test file should document the minimum storage requirements needed to run these tests successfully, especially given the repeated volume expansions.
Add to the Documentation section:
Documentation Negative Test Cases ... Prerequisites: ... - Minimum available storage: (LOOP_COUNT * 100 MiB) + 5Gi per test ... - Maximum storage size configuration should be definede2e/tests/regression/test_persistentvolumeclaim.robot (1)
2-2: Enhance test suite documentation.Consider adding more details to the documentation, such as test prerequisites, expected behaviors, and any specific setup requirements.
-Documentation PersistentVolumeClaim Test Cases +Documentation Test suite for validating PersistentVolumeClaim (PVC) behaviors. +... Prerequisites: +... - Kubernetes cluster with Longhorn installed +... - Storage over-provisioning enabled +... Expected behaviors: +... - PVC expansion should fail when requested size exceeds maximum storage +... - PVC expansion should succeed within storage limitse2e/keywords/workload.resource (2)
236-256: Add documentation for volume expansion keywords.The implementation is correct, but documentation would help users understand the purpose and usage of these keywords.
+[Documentation] Expands the volume of a workload to a specific size. +... Arguments: +... - workload_kind: Type of the workload (e.g., deployment) +... - workload_id: Identifier of the workload +... - size: Target size (e.g., "2GiB") Expand ${workload_kind} ${workload_id} volume to ${size} ${workload_name} = generate_name_with_suffix ${workload_kind} ${workload_id} ${new_size} = convert_size_to_bytes ${size} expand_workload_claim_size ${workload_name} ${new_size} +[Documentation] Expands the volume of a workload by adding additional size. +... Arguments: +... - workload_kind: Type of the workload (e.g., deployment) +... - workload_id: Identifier of the workload +... - size: Additional size to add (e.g., "1GiB") Expand ${workload_kind} ${workload_id} volume with additional ${size} ${workload_name} = generate_name_with_suffix ${workload_kind} ${workload_id} ${new_size} = convert_size_to_bytes ${size} expand_workload_claim_size_with_additional_bytes ${workload_name} ${new_size} +[Documentation] Verifies that volume expansion fails when exceeding maximum size. +... Arguments: +... - workload_kind: Type of the workload (e.g., deployment) +... - workload_id: Identifier of the workload Expand ${workload_kind} ${workload_id} volume more than storage maximum size should fail ${workload_name} = generate_name_with_suffix ${workload_kind} ${workload_id} ${volume_name} = get_workload_volume_name ${workload_name} ${node_name} = get_volume_node ${volume_name} ${max_size} = get_volume_node_disk_storage_maximum ${volume_name} ${node_name} ${new_size} = evaluate ${max_size} + 1 Run Keyword And Expect Error Failed to expand* expand_workload_claim_size ${workload_name} ${new_size} skip_retry=True
257-265: Add documentation for volume size assertion keyword.The implementation is correct, but documentation would help users understand the purpose and usage.
+[Documentation] Asserts that the volume size remains unchanged for a specified duration. +... Arguments: +... - workload_kind: Type of the workload (e.g., deployment) +... - workload_id: Identifier of the workload +... - size: Expected size (e.g., "2GiB") +... - period: Duration in seconds to maintain the size Assert volume size of ${workload_kind} ${workload_id} remains ${size} for at least ${period} seconds ${workload_name} = generate_name_with_suffix ${workload_kind} ${workload_id} ${volume_name} = get_workload_volume_name ${workload_name}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
e2e/keywords/persistentvolumeclaim.resource(2 hunks)e2e/keywords/workload.resource(2 hunks)e2e/libs/keywords/common_keywords.py(2 hunks)e2e/libs/keywords/persistentvolumeclaim_keywords.py(2 hunks)e2e/libs/keywords/volume_keywords.py(2 hunks)e2e/libs/keywords/workload_keywords.py(2 hunks)e2e/libs/node/node.py(2 hunks)e2e/libs/persistentvolumeclaim/crd.py(1 hunks)e2e/libs/persistentvolumeclaim/persistentvolumeclaim.py(4 hunks)e2e/tests/negative/stress_cpu.robot(2 hunks)e2e/tests/negative/stress_filesystem.robot(2 hunks)e2e/tests/negative/stress_memory.robot(2 hunks)e2e/tests/regression/test_persistentvolumeclaim.robot(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- e2e/tests/negative/stress_memory.robot
- e2e/tests/negative/stress_cpu.robot
🚧 Files skipped from review as they are similar to previous changes (7)
- e2e/libs/keywords/common_keywords.py
- e2e/libs/node/node.py
- e2e/libs/keywords/volume_keywords.py
- e2e/libs/persistentvolumeclaim/crd.py
- e2e/keywords/persistentvolumeclaim.resource
- e2e/libs/keywords/persistentvolumeclaim_keywords.py
- e2e/libs/persistentvolumeclaim/persistentvolumeclaim.py
🔇 Additional comments (3)
e2e/tests/negative/stress_filesystem.robot (1)
Line range hint 39-54: Consider adding storage capacity validation in expansion tests
Both online and offline expansion tests perform multiple expansions in a loop without checking available storage. This could inadvertently lead to the stuck volume scenario we're trying to prevent.
Add storage capacity checks before expansion:
FOR ${i} IN RANGE ${LOOP_COUNT}
${available_capacity}= Get Available Storage Capacity
${expansion_size}= Set Variable 100
Skip If ${available_capacity} < ${expansion_size} Insufficient storage space
When Expand statefulset 0 volume with additional ${expansion_size} MiB
...
ENDAlso applies to: 63-72
e2e/tests/regression/test_persistentvolumeclaim.robot (1)
22-44: LGTM! Well-structured test case with comprehensive validation.
The test case effectively:
- Sets up the environment with storage over-provisioning
- Creates and validates PVC with initial size
- Verifies expansion failure beyond maximum size
- Confirms successful expansion within limits
- Ensures data integrity throughout the process
e2e/keywords/workload.resource (1)
Line range hint 211-234: LGTM! Clean formatting changes.
The indentation and conditional logic are well-structured.
longhorn/longhorn-6633 Signed-off-by: Chin-Ya Huang <[email protected]>
longhorn/longhorn-6633 Signed-off-by: Chin-Ya Huang <[email protected]>
c3y1huang
force-pushed
the
6633-prevent-resize-stuck
branch
from
43a1a09 to
331b852
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
e2e/keywords/persistentvolumeclaim.resource (1)
27-34: Consider adding timeout handling for assertionThe new assertion keyword is crucial for verifying PVC size behavior, but it could benefit from additional error handling.
Consider adding a timeout message and handling test failures more gracefully:
Assert persistentvolumeclaim ${claim_id} requested size remains ${size} for at least ${period} seconds ${claim_name} = generate_name_with_suffix claim ${claim_id} + ${timeout_msg} = Set Variable PVC ${claim_name} size changed unexpectedly within ${period} seconds FOR ${i} IN RANGE ${period} ${expected_size_byte} = convert_size_to_bytes ${size} to_str=True ${current_size_byte} = get_claim_requested_size ${claim_name} - Should Be Equal ${current_size_byte} ${expected_size_byte} + Should Be Equal ${current_size_byte} ${expected_size_byte} ${timeout_msg} Sleep 1 END
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
e2e/keywords/persistentvolumeclaim.resource(2 hunks)e2e/keywords/workload.resource(2 hunks)e2e/libs/keywords/common_keywords.py(2 hunks)e2e/libs/keywords/persistentvolumeclaim_keywords.py(2 hunks)e2e/libs/keywords/volume_keywords.py(2 hunks)e2e/libs/keywords/workload_keywords.py(2 hunks)e2e/libs/node/node.py(2 hunks)e2e/libs/persistentvolumeclaim/crd.py(1 hunks)e2e/libs/persistentvolumeclaim/persistentvolumeclaim.py(4 hunks)e2e/tests/negative/stress_cpu.robot(2 hunks)e2e/tests/negative/stress_filesystem.robot(2 hunks)e2e/tests/negative/stress_memory.robot(2 hunks)e2e/tests/regression/test_persistentvolumeclaim.robot(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- e2e/tests/negative/stress_memory.robot
- e2e/tests/negative/stress_cpu.robot
- e2e/tests/negative/stress_filesystem.robot
- e2e/libs/keywords/volume_keywords.py
- e2e/libs/keywords/common_keywords.py
- e2e/libs/node/node.py
- e2e/tests/regression/test_persistentvolumeclaim.robot
- e2e/libs/keywords/persistentvolumeclaim_keywords.py
- e2e/libs/persistentvolumeclaim/crd.py
- e2e/libs/keywords/workload_keywords.py
🔇 Additional comments (8)
e2e/keywords/workload.resource (5)
211-214: LGTM! Improved code formatting
The indentation changes improve code readability while maintaining the same functionality.
236-241: LGTM! Well-structured volume expansion keyword
The implementation correctly handles size conversion and uses appropriate helper functions.
242-247: LGTM! Clear implementation for incremental volume expansion
The keyword provides a good complement to the fixed-size expansion, allowing for relative size increases.
257-265: LGTM! Robust implementation for size verification
The keyword effectively verifies volume size stability over time:
- Appropriate polling interval (1 second)
- Proper size conversion for comparison
- Clear error messages through Robot Framework's built-in assertions
248-256: LGTM! Robust implementation for testing expansion limits
The implementation aligns perfectly with the PR objective to test PVC expansion beyond maximum size. It:
- Correctly retrieves the maximum storage size
- Ensures the test exceeds this limit by exactly 1 byte
- Properly handles the expected failure case
Let's verify the helper functions being used:
✅ Verification successful
Let me verify the implementation of get_volume_node_disk_storage_maximum further since it's crucial for determining the maximum size limit.
Let me try a different approach to get the complete implementation.
Implementation verified - Test case correctly handles volume expansion limits
The test case and its helper functions are properly implemented:
get_volume_node_disk_storage_maximumcorrectly retrieves the maximum storage size by finding the replica's disk and itsstorageMaximumvalueexpand_workload_claim_sizehandles the expansion operation with proper size conversion and error handling- The test case properly validates the failure scenario by attempting to expand beyond the maximum size by 1 byte
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of helper functions used by the expansion keywords
# Check for the helper function implementations
rg -A 5 "def (convert_size_to_bytes|get_volume_node_disk_storage_maximum|expand_workload_claim_size)"
Length of output: 2809
Script:
#!/bin/bash
# Check the complete implementation of get_volume_node_disk_storage_maximum
ast-grep --pattern 'def get_volume_node_disk_storage_maximum($$$)'
Length of output: 68
Script:
#!/bin/bash
# Check the complete implementation using ripgrep with context
rg -A 15 "def get_volume_node_disk_storage_maximum" e2e/libs/keywords/volume_keywords.py
Length of output: 881
e2e/keywords/persistentvolumeclaim.resource (1)
11-13: LGTM: Enhanced PVC creation flexibility
The addition of the &{config} parameter allows for dynamic configuration of PVC properties, which is essential for testing storage size limitations.
e2e/libs/persistentvolumeclaim/persistentvolumeclaim.py (2)
27-29: LGTM: Flexible storage size configuration
The addition of the storage_size parameter with proper byte conversion enhances the PVC creation flexibility needed for testing size limitations.
Also applies to: 44-46
104-107:
Add size validation to prevent volume resize stuck issues
Both expand methods should validate the target size against system capacity before attempting expansion, as suggested in the linked issue longhorn/longhorn#6633.
Add validation to both methods:
def expand(self, claim_name, size_in_byte, skip_retry=False):
+ # Validate target size against system capacity
+ if size_in_byte > self._get_max_storage_size():
+ raise ValueError(f"Target size {size_in_byte} exceeds maximum storage capacity")
expanded_size = self.claim.expand(claim_name, size_in_byte, skip_retry=skip_retry)
self.set_annotation(claim_name, ANNOT_EXPANDED_SIZE, str(expanded_size))
def expand_with_additional_bytes(self, claim_name, size_in_byte, skip_retry=False):
pvc = self.claim.get(claim_name)
current_size = int(pvc.spec.resources.requests['storage'])
target_size = current_size + size_in_byte
+ # Validate target size against system capacity
+ if target_size > self._get_max_storage_size():
+ raise ValueError(f"Target size {target_size} exceeds maximum storage capacity")
expanded_size = self.claim.expand(claim_name, target_size, skip_retry=skip_retry)
self.set_annotation(claim_name, ANNOT_EXPANDED_SIZE, str(expanded_size))Additionally, implement the helper method:
def _get_max_storage_size(self):
"""Get maximum available storage size in bytes"""
# Implementation needed to fetch system capacity
raise NotImplementedError("Method needs to be implemented")Also applies to: 108-114
longhorn/longhorn-6633 Signed-off-by: Chin-Ya Huang <[email protected]>
longhorn/longhorn-6633 Signed-off-by: Chin-Ya Huang <[email protected]>
c3y1huang
force-pushed
the
6633-prevent-resize-stuck
branch
from
331b852 to
23fe339
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
e2e/keywords/workload.resource (1)
257-265: Add documentation for the temporal verification keywordThe implementation effectively verifies volume size stability over time. Consider adding a comment explaining why temporal verification is necessary (e.g., to ensure size remains stable after failed expansion attempts).
Assert volume size of ${workload_kind} ${workload_id} remains ${size} for at least ${period} seconds + [Documentation] Verifies that the volume size remains stable and unchanged + ... over the specified period. This is particularly useful for ensuring + ... that failed expansion attempts don't cause unexpected size changes. ${workload_name} = generate_name_with_suffix ${workload_kind} ${workload_id}e2e/keywords/persistentvolumeclaim.resource (1)
27-34: Consider optimizing the polling intervalThe keyword effectively verifies PVC size stability, but polling every second for the entire period might be resource-intensive for longer durations.
Consider using an exponential backoff or longer intervals:
- Sleep 1 + ${sleep_time}= Evaluate min(5, 1 * (${i} + 1)) # Gradually increase up to 5 seconds + Sleep ${sleep_time}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
e2e/keywords/persistentvolumeclaim.resource(2 hunks)e2e/keywords/workload.resource(2 hunks)e2e/libs/keywords/common_keywords.py(2 hunks)e2e/libs/keywords/persistentvolumeclaim_keywords.py(2 hunks)e2e/libs/keywords/volume_keywords.py(2 hunks)e2e/libs/keywords/workload_keywords.py(2 hunks)e2e/libs/node/node.py(2 hunks)e2e/libs/persistentvolumeclaim/crd.py(1 hunks)e2e/libs/persistentvolumeclaim/persistentvolumeclaim.py(4 hunks)e2e/tests/negative/stress_cpu.robot(2 hunks)e2e/tests/negative/stress_filesystem.robot(2 hunks)e2e/tests/negative/stress_memory.robot(2 hunks)e2e/tests/regression/test_persistentvolumeclaim.robot(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- e2e/tests/negative/stress_cpu.robot
- e2e/libs/keywords/common_keywords.py
- e2e/tests/negative/stress_memory.robot
- e2e/tests/regression/test_persistentvolumeclaim.robot
- e2e/libs/node/node.py
- e2e/tests/negative/stress_filesystem.robot
- e2e/libs/keywords/volume_keywords.py
- e2e/libs/persistentvolumeclaim/crd.py
- e2e/libs/keywords/persistentvolumeclaim_keywords.py
- e2e/libs/keywords/workload_keywords.py
🔇 Additional comments (8)
e2e/keywords/workload.resource (4)
211-214: LGTM: Formatting improvements
The whitespace adjustments improve code readability while maintaining the existing functionality.
236-241: LGTM: Clean implementation of volume expansion keyword
The keyword properly handles size conversion and follows the established pattern for workload management.
242-247: LGTM: Well-structured incremental expansion keyword
The keyword provides clear differentiation between absolute and incremental volume expansion scenarios.
248-256: LGTM: Effectively implements the core test case
This keyword directly addresses the PR objective by testing volume expansion beyond maximum size. The implementation:
- Correctly retrieves the maximum storage size from the node
- Attempts expansion with size + 1 to ensure exceeding the limit
- Properly expects and verifies the failure condition
- Uses skip_retry=True to prevent unnecessary retries
Let's verify the error message pattern used in the test:
✅ Verification successful
Error message pattern matches the implementation
The error message pattern "Failed to expand*" used in the test matches the actual error message format in e2e/libs/persistentvolumeclaim/crd.py: "Failed to expand {claim_name} size to {size}". The test's error handling is correctly implemented and consistent with the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the error message pattern consistency across the codebase
# Search for similar error handling patterns
rg -l 'Failed to expand' --type py
Length of output: 74
Script:
#!/bin/bash
# Let's check the actual error message pattern in the crd.py file
rg -A 5 'Failed to expand' e2e/libs/persistentvolumeclaim/crd.py
Length of output: 136
e2e/keywords/persistentvolumeclaim.resource (1)
11-13: LGTM! Verify the config parameter handling
The addition of the &{config} dictionary argument enhances the keyword's flexibility, allowing for dynamic configuration of PVC parameters including storage size.
✅ Verification successful
LGTM! The config parameter is correctly implemented and used
The verification shows that:
- The
Create persistentvolumeclaimkeyword is used extensively across test files with different configurations - The implementation in
e2e/libs/keywords/persistentvolumeclaim_keywords.pyproperly handles the config parameters including storage_size - Test files like
test_persistentvolumeclaim.robotdemonstrate correct usage with storage size parameter:Create persistentvolumeclaim 0 using RWX volume storage_size=2GiB
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the config parameter usage across test files
rg -l "Create persistentvolumeclaim.*&{" test/
Length of output: 176
Script:
#!/bin/bash
# Search for Create persistentvolumeclaim keyword usage with proper regex escaping
rg -l "Create persistentvolumeclaim.*\&\{" test/
# Also search in e2e directory as the keyword is defined there
rg -l "Create persistentvolumeclaim.*\&\{" e2e/
# Let's see the actual implementation of create_persistentvolumeclaim
ast-grep --pattern 'create_persistentvolumeclaim ${$_} ${$_} &{$_}'
Length of output: 225
Script:
#!/bin/bash
# Let's first find all Robot Framework files in the repository
fd -e robot -e resource
# Then search for Create persistentvolumeclaim usage in those files
rg "Create persistentvolumeclaim" -A 2
# Also look for the implementation of create_persistentvolumeclaim
rg "create_persistentvolumeclaim" -A 3
Length of output: 20598
e2e/libs/persistentvolumeclaim/persistentvolumeclaim.py (3)
Line range hint 27-46: LGTM! Good implementation of storage size configuration
The addition of the storage_size parameter with a default value maintains backward compatibility while enabling dynamic storage size testing.
104-107:
Add size validation to prevent volume resize stuck issues
While the skip_retry parameter is a good addition, the method should validate the target size before attempting expansion to prevent volumes from getting stuck.
As suggested in previous reviews, consider adding size validation:
def expand(self, claim_name, size_in_byte, skip_retry=False):
+ # Validate target size against system capacity
+ if not self._is_size_within_limits(size_in_byte):
+ raise ValueError(f"Target size {size_in_byte} exceeds storage capacity")
expanded_size = self.claim.expand(claim_name, size_in_byte, skip_retry=skip_retry)
self.set_annotation(claim_name, ANNOT_EXPANDED_SIZE, str(expanded_size))108-113:
Add size validation and error handling to expansion method
The incremental expansion logic is good, but it should include size validation and proper error handling to prevent stuck volumes.
Consider adding validation and error handling:
def expand_with_additional_bytes(self, claim_name, size_in_byte, skip_retry=False):
pvc = self.claim.get(claim_name)
current_size = int(pvc.spec.resources.requests['storage'])
target_size = current_size + size_in_byte
+ # Validate target size against system capacity
+ if not self._is_size_within_limits(target_size):
+ raise ValueError(f"Target size {target_size} exceeds storage capacity")
+ try:
expanded_size = self.claim.expand(claim_name, target_size, skip_retry=skip_retry)
self.set_annotation(claim_name, ANNOT_EXPANDED_SIZE, str(expanded_size))
+ return expanded_size
+ except Exception as e:
+ logging(f"Failed to expand PVC {claim_name} by {size_in_byte} bytes: {str(e)}")
+ raiseLikely invalid or redundant comment.
Which issue(s) this PR fixes:
Issue longhorn/longhorn#6633
What this PR does / why we need it:
Add a robot test case to verify that a PVC cannot be expanded beyond the storage maximum size.
Special notes for your reviewer:
NoneAdditional documentation or context
NoneSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style