test(robot): v2 volume should block trim when volume is degraded #2114
Conversation
c3y1huang
force-pushed
the
8430-feat-v2-auto-salvage
branch
2 times, most recently
from
e36315b to
9374c8b
Compare
c3y1huang
force-pushed
the
8430-feat-v2-auto-salvage
branch
from
9374c8b to
fa81b99
Compare
WalkthroughThe changes introduce a new keyword for workload resource management, enabling conditional volume trimming based on the state of the workload. This includes a new method for trimming filesystem volumes across multiple classes, enhancing the functionality and error handling of volume operations. Additionally, a new test case has been added to validate the behavior of the trimming operation under specific conditions, particularly when the volume is degraded. Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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
8430-feat-v2-auto-salvage
branch
2 times, most recently
from
c6a4959 to
3b959ca
Compare
longhorn/longhorn-8430 Signed-off-by: Chin-Ya Huang <[email protected]>
c3y1huang
force-pushed
the
8430-feat-v2-auto-salvage
branch
from
3b959ca to
5b626a9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
e2e/tests/regression/test_v2.robot (2)
56-57: Add documentation for the test case.Consider adding a [Documentation] tag to describe the test's purpose, prerequisites, and expected behavior.
V2 Volume Should Block Trim When Volume Is Degraded + [Documentation] Verifies that volume trim operations are blocked when the volume + ... is in a degraded state, and succeed when the volume returns to healthy state. + ... Prerequisites: Cluster must support v2 volumes and trim operations.
70-73: Consider adding retry mechanism for workload stability check.The workload stability check might be sensitive to timing issues. Consider adding retry logic with appropriate timeout.
- When Wait for workloads pods stable - ... deployment 0 - And Check deployment 0 works + When Wait for workloads pods stable with timeout deployment 0 timeout=300s + And Run Keyword And Retry On Failure Check deployment 0 works retry=3e2e/libs/keywords/workload_keywords.py (1)
196-198: LGTM! Consider adding docstring.The implementation is clean and well-integrated with the existing codebase. It properly handles both success and failure scenarios needed for testing trim blocking behavior.
Consider adding a docstring to document the method's purpose and parameters:
def trim_workload_volume_filesystem(self, workload_name, is_expect_fail=False): + """Trim the filesystem of the volume associated with the given workload. + + Args: + workload_name (str): Name of the workload whose volume needs to be trimmed + is_expect_fail (bool, optional): If True, expects the trim operation to fail. + Defaults to False. + """ volume_name = get_workload_volume_name(workload_name) self.volume.trim_filesystem(volume_name, is_expect_fail=is_expect_fail)e2e/keywords/workload.resource (2)
193-202: Add documentation for the new keyword.The keyword implementation looks good but lacks documentation. Consider adding documentation to describe:
- Purpose of the keyword
- Expected values for
condition- Example usage
Add documentation above the keyword:
+[Documentation] Tests volume trim operation behavior. +... condition: "pass" - expects trim to succeed +... condition: "fail" - expects trim to fail +... Example: Trim pod my-pod volume should fail Trim ${workload_kind} ${workload_id} volume should ${condition}
196-202: Consider improving error handling and condition validation.While the implementation is functionally correct, consider these improvements:
- Define conditions as variables for better maintainability
- Provide more descriptive error message
- Add validation for workload existence before attempting trim
Example implementation:
+ ${TRIM_CONDITION_FAIL} = Set Variable fail + ${TRIM_CONDITION_PASS} = Set Variable pass + + # Validate workload exists + ${exists} = workload_exists ${workload_name} + IF not ${exists} + Fail "Workload ${workload_name} does not exist" + END + - IF $condition == "fail" + IF $condition == "${TRIM_CONDITION_FAIL}" trim_workload_volume_filesystem ${workload_name} is_expect_fail=True - ELSE IF $condition == "pass" + ELSE IF $condition == "${TRIM_CONDITION_PASS}" trim_workload_volume_filesystem ${workload_name} is_expect_fail=False ELSE - Fail "Invalid condition value: ${condition}" + Fail "Invalid condition value: '${condition}'. Expected values: '${TRIM_CONDITION_PASS}' or '${TRIM_CONDITION_FAIL}'" ENDe2e/libs/volume/rest.py (2)
374-389: Add docstring and type hints to improve code clarity.The method implementation looks good, but could benefit from better documentation.
- def trim_filesystem(self, volume_name, is_expect_fail=False): + def trim_filesystem(self, volume_name: str, is_expect_fail: bool = False) -> None: + """Trim the filesystem of a specified volume. + + Args: + volume_name: Name of the volume to trim + is_expect_fail: If True, the operation is expected to fail + + Raises: + Exception: If the operation fails unexpectedly or succeeds when expected to fail + """
374-389: Add success logging for better debugging.The method logs failures but not successes, which could make debugging more difficult.
try: self.get(volume_name).trimFilesystem(name=volume_name) + logging(f"Successfully trimmed filesystem for volume {volume_name}") if is_expect_fail: is_unexpected_pass = True
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
e2e/keywords/workload.resource(1 hunks)e2e/libs/keywords/workload_keywords.py(1 hunks)e2e/libs/volume/crd.py(1 hunks)e2e/libs/volume/rest.py(1 hunks)e2e/libs/volume/volume.py(1 hunks)e2e/tests/regression/test_v2.robot(2 hunks)
🔇 Additional comments (7)
e2e/tests/regression/test_v2.robot (2)
14-15: LGTM: Resource imports are properly organized.
The new resource imports follow the established pattern and are appropriately placed with related imports.
62-64: Consider adding validation for data consistency.
The test writes data continuously but doesn't verify data integrity after the cluster restart. Consider adding data validation steps.
FOR ${i} IN RANGE ${LOOP_COUNT}
- And Keep writing data to pod of deployment 0
+ And Keep writing data to pod of deployment 0
+ ${written_data}= Get Written Data From Pod deployment 0e2e/libs/volume/volume.py (1)
157-159: LGTM! The implementation follows the established patterns.
The new trim_filesystem method:
- Maintains consistency with the facade pattern used throughout the class
- Properly delegates to the underlying implementation
- Includes support for expected failures which is crucial for testing negative scenarios (e.g., trim being blocked when volume is degraded)
Let's verify the corresponding implementations in the underlying classes:
✅ Verification successful
The implementation is correctly verified across all layers
The verification confirms that:
- The
trim_filesystemmethod is properly implemented in both CRD and REST classes - CRD implementation correctly delegates to REST implementation
- REST implementation contains the actual trimming logic with proper error handling for expected failures
- The method signatures are consistent across all three layers (Volume, CRD, and REST)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify trim_filesystem implementations in CRD and REST classes
# Expected: Both classes should have corresponding implementations
# Check CRD implementation
echo "Checking CRD implementation..."
rg -A 5 "def trim_filesystem" "e2e/libs/volume/crd.py"
# Check REST implementation
echo "Checking REST implementation..."
rg -A 5 "def trim_filesystem" "e2e/libs/volume/rest.py"
Length of output: 619
e2e/libs/keywords/workload_keywords.py (1)
196-198: Verify trim_filesystem implementation.
Let's verify the implementation of the underlying trim_filesystem method to ensure it properly handles the is_expect_fail parameter.
✅ Verification successful
trim_filesystem implementation properly handles is_expect_fail parameter
The implementation in e2e/libs/volume/rest.py correctly handles the is_expect_fail parameter:
- Attempts to trim the filesystem via REST API
- Properly handles expected failures through try-catch block
- Logs failures when
is_expect_fail=True - Propagates exceptions for unexpected failures
The method is also being used correctly in test cases as shown in e2e/keywords/workload.resource with both positive and negative test scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for trim_filesystem implementation in volume-related files
echo "Searching for trim_filesystem implementation..."
rg -A 10 "def trim_filesystem.*is_expect_fail" --type python
# Search for test cases using this new method
echo "Searching for test cases using trim_workload_volume_filesystem..."
rg "trim_workload_volume_filesystem" --type robot
Length of output: 389
Script:
#!/bin/bash
# Search for trim_filesystem implementation without file type restriction
echo "Searching for trim_filesystem implementation..."
rg -A 10 "def trim_filesystem"
# Search for test cases using trim_workload_volume_filesystem without file type restriction
echo "Searching for test cases using trim_workload_volume_filesystem..."
rg "trim_workload_volume_filesystem"
# Search for volume-related files that might contain the implementation
echo "Searching for volume-related files..."
fd "volume.*\.py$"
Length of output: 2087
e2e/keywords/workload.resource (1)
197-197: Verify trim operation behavior in degraded state.
The implementation correctly handles expected pass/fail conditions, which aligns with the PR objective to test trim blocking in degraded state.
Let's verify the trim operation implementation:
Also applies to: 199-199
✅ Verification successful
Let me gather more context about the trim operation implementation, particularly focusing on the actual implementation in the REST layer and any test cases.
Based on the code inspection, I can now generate the final response:
Trim operation's error handling for degraded state is properly implemented
The implementation in rest.py correctly handles the expected failure scenario through:
- Proper error handling with try-catch block
is_expect_failparameter propagation through all layers- Appropriate logging of expected failures
- Correct detection of unexpected passes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify trim operation implementation in workload keywords
# Test 1: Check if trim_workload_volume_filesystem method exists and handles is_expect_fail
rg -A 5 "def trim_workload_volume_filesystem" "e2e/libs/keywords/workload_keywords.py"
# Test 2: Check volume trim implementation in volume libraries
rg -A 5 "trim_filesystem.*is_expect_fail" "e2e/libs/volume/"
Length of output: 1139
Script:
#!/bin/bash
# Check the complete implementation of trim operation in REST layer
rg -A 10 "def trim_filesystem" "e2e/libs/volume/rest.py"
# Check for related test cases
rg -A 5 "trim.*filesystem.*degraded" .
rg -A 5 "test.*trim.*degraded" .
Length of output: 509
e2e/libs/volume/rest.py (1)
374-389: Implementation looks good and aligns with PR objectives.
The method correctly implements filesystem trimming with proper error handling for both expected and unexpected failures. This implementation supports the PR objective of testing v2 volume trim behavior in degraded states.
Key strengths:
- Proper error handling
- Support for testing negative scenarios via
is_expect_fail - Consistent with the class's error handling patterns
e2e/libs/volume/crd.py (1)
514-516: LGTM! The implementation aligns with the PR objectives.
The new trim_filesystem method is well-structured and follows the established patterns in the codebase. It appropriately delegates to the Rest class while providing the ability to test both successful and failing scenarios through the is_expect_fail parameter.
Let's verify the Rest class implementation:
✅ Verification successful
The implementation is correctly integrated with the Rest class
The verification confirms that:
- The
trim_filesystemmethod exists ine2e/libs/volume/rest.pywith matching parameters - The implementation follows the proper delegation chain: CRD -> Rest -> Volume
- The method handles both success and failure scenarios as expected through
is_expect_fail
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the trim_filesystem implementation in Rest class
# Expected: The Rest class should have a corresponding trim_filesystem method
ast-grep --pattern 'class Rest {
$$$
trim_filesystem($_, $_) {
$$$
}
$$$
}'
Length of output: 89
Script:
#!/bin/bash
# Let's try a different approach to find the Rest class and its trim_filesystem method
# First, let's locate the Rest class file
fd -t f "rest.py" -x rg -l "class Rest"
# Then search for trim_filesystem method
rg "def trim_filesystem" -A 5
Length of output: 1041
Which issue(s) this PR fixes:
Issue longhorn/longhorn#8430
What this PR does / why we need it:
Propose a new robot case to test v2 volume should block trim when the volume is degraded.
Special notes for your reviewer:
NoneAdditional documentation or context
NoneSummary by CodeRabbit
New Features
Bug Fixes
Tests