test(robot): wait for pvc and volume deleted when cleaning up statefulset #2187
Conversation
…lset Signed-off-by: Yang Chiu <[email protected]>
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces several modifications to improve the handling of persistent volume claims and volumes within the codebase. Key changes include updating logging statements to accurately reflect the counts of cleaned-up resources, simplifying iteration over claims and volumes, and altering the return values of list methods to provide only the relevant items. Additionally, enhancements have been made to the deletion process of stateful sets, ensuring associated resources are properly managed and logged during deletion. Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
e2e/libs/workload/statefulset.py (2)
Line range hint
85-97: Ensure Consistent Logic in Deletion LoopsIn the loop waiting for the stateful set deletion, the
deletedvariable is initialized toTrueand set toFalseif the stateful set still exists. In the subsequent loops for PVC and volume deletion,deletedis initialized toFalseand set toTruewhen the items are deleted. For consistency and clarity, consider using the same approach to initializing and updating thedeletedvariable across all loops.
Line range hint
85-123: Refactor Repetitive Deletion Waiting Loops into a Helper FunctionThe loops for waiting on the deletion of the stateful set, PVCs, and volumes have similar structures and logic. Consider refactoring this repeated code into a reusable helper function to improve maintainability and reduce code duplication.
Suggested Change:
Create a helper function, for example,
wait_for_resource_deletion(resource_list_function, resource_name, label_selector), which encapsulates the deletion waiting logic.e2e/libs/persistentvolumeclaim/crd.py (1)
25-25: Update Method Documentation to Reflect Changed Return TypeThe
listmethod now returns only the.itemsfrom the API response, which is a list of PersistentVolumeClaim objects. To prevent confusion and potential misuse, consider updating the method's docstring or adding a comment to indicate that the method returns a list of items rather than the full API response.e2e/libs/volume/volume.py (1)
37-37: Ensure Consistency with Updated List MethodThe
list_namesmethod assumes thatself.list(label_selector)returns a list of volume objects directly. Verify that all other usages ofself.list()in the codebase are updated accordingly to handle the new return type, preventing potential attribute errors or misuses.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
e2e/libs/keywords/persistentvolumeclaim_keywords.py(1 hunks)e2e/libs/keywords/volume_keywords.py(1 hunks)e2e/libs/persistentvolumeclaim/crd.py(1 hunks)e2e/libs/volume/crd.py(1 hunks)e2e/libs/volume/rest.py(2 hunks)e2e/libs/volume/volume.py(1 hunks)e2e/libs/workload/statefulset.py(3 hunks)
🔇 Additional comments (4)
e2e/libs/keywords/persistentvolumeclaim_keywords.py (1)
19-20: Confirm Correct Logging of PersistentVolumeClaim Count
The logging statement now correctly reflects the number of persistent volume claims being cleaned up by using len(claims). Ensure that claims is always a list to prevent any unexpected issues.
e2e/libs/keywords/volume_keywords.py (1)
29-30: LGTM: Simplified volume iteration
The change improves code readability by directly iterating over volumes while maintaining the same functionality. This aligns well with the changes in the CRD class where list() now returns items directly.
e2e/libs/volume/rest.py (1)
Line range hint 241-258: LGTM: Improved replica count verification
The changes enhance reliability and debuggability by:
- Adding explicit condition tracking
- Providing more detailed logging
- Including descriptive assertion messages
🧰 Tools
🪛 Ruff (0.8.0)
251-256: Combine if branches using logical or operator
Combine if branches
(SIM114)
e2e/libs/volume/crd.py (1)
181-181: Verify all callers of the list method
The change to return only items is a breaking change that could affect callers expecting the full response object. While this simplifies the API, we should ensure all callers have been updated.
✅ Verification successful
Breaking change verified - callers already handle items-only response
Based on the search results, the main callers of the volume CRD list method are:
e2e/libs/volume/volume.py: Uses the list results directly to extract names or process itemse2e/libs/keywords/volume_keywords.py: Uses the results to iterate through volumese2e/libs/workload/statefulset.py: Only checks length and iterates through volumes
All identified callers are already treating the response as a list of items, either by iterating directly or checking the length. No callers are accessing other properties of the response object.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all callers of the CRD.list method to verify they handle the new return value
# Search for direct method calls
rg -A 3 "\.list\(" --type python
# Search for variable assignments from list calls
rg -A 3 "=.*\.list\(" --type python
Length of output: 134
Script:
#!/bin/bash
# Search for direct method calls and variable assignments using list method
# Note: Using ripgrep without file type filtering since python type is not recognized
rg -A 3 "\.list\("
# Also search for potential imports of the CRD class to identify files that might use it
rg "from.*volume.*crd.*import"
Length of output: 8315
| claim = PersistentVolumeClaim() | ||
| volume_name = None | ||
| for i in range(retry_count): | ||
| time.sleep(retry_interval) | ||
| logging(f"Waiting for pvc of statefulset {name} deleted ... ({i})") | ||
| claims = claim.list(label_selector=f"app={name}") | ||
| deleted = False | ||
| if len(claims) == 0: | ||
| deleted = True | ||
| break | ||
| else: | ||
| volume_name = claims[0].spec.volume_name |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle Multiple PersistentVolumeClaims and Associated Volumes
Currently, the code retrieves the volume_name from only the first PersistentVolumeClaim (claims[0].spec.volume_name). If the stateful set has multiple replicas, there may be multiple PVCs and associated volumes. Consider iterating over all claims to handle multiple PVCs and their volumes properly.
Suggested Change:
Modify the loop to handle all PVCs and collect their associated volume names:
volume_names = []
for i in range(retry_count):
time.sleep(retry_interval)
logging(f"Waiting for PVCs of statefulset {name} to be deleted... ({i})")
claims = claim.list(label_selector=f"app={name}")
if len(claims) == 0:
deleted = True
break
else:
- volume_name = claims[0].spec.volume_name
+ volume_names = [c.spec.volume_name for c in claims]
assert deletedCommittable suggestion skipped: line range outside the PR's diff.
| if volume_name: | ||
| volume = Volume() | ||
| for i in range(retry_count): | ||
| time.sleep(retry_interval) | ||
| logging(f"Waiting for volume {volume_name} deleted ... ({i})") | ||
| volumes = volume.list(label_selector=f"longhornvolume={volume_name}") | ||
| deleted = False | ||
| if len(volumes) == 0: | ||
| deleted = True | ||
| break | ||
| assert deleted | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle Multiple Volumes During Deletion
Similar to the PVC deletion logic, the code is currently checking for the deletion of a single volume. If there are multiple volumes associated with the stateful set, they should all be checked and waited upon during the deletion process. Consider modifying the code to handle deletion checks for all relevant volumes.
Suggested Change:
Iterate over all collected volume_names to ensure each is deleted:
-if volume_name:
+if volume_names:
volume = Volume()
for volume_name in volume_names:
for i in range(retry_count):
time.sleep(retry_interval)
logging(f"Waiting for volume {volume_name} to be deleted... ({i})")
volumes = volume.list(label_selector=f"longhornvolume={volume_name}")
if len(volumes) == 0:
deleted = True
break
else:
deleted = False
assert deletedCommittable suggestion skipped: line range outside the PR's diff.
Which issue(s) this PR fixes:
Issue N/A
What this PR does / why we need it:
wait for pvc and volume deleted when cleaning up statefulset
Special notes for your reviewer:
Additional documentation or context
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores