feat(backup): add cleanup snapshot after backup test case #2288
Conversation
|
Warning Rate limit exceeded@mantissahz has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 53 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
WalkthroughThis pull request introduces enhancements to the backup functionality in Longhorn's testing framework. The changes include adding a new parameter Changes
Sequence DiagramsequenceDiagram
participant User
participant BackupKeywords
participant Backup
participant Volume
User->>BackupKeywords: create_backup(volume_name, backup_id, cleanup_snapshot)
BackupKeywords->>Backup: create(volume_name, backup_id, cleanup_snapshot)
Backup->>Volume: snapshotBackup(name, cleanupBackupSnapshot)
alt cleanup_snapshot is True
Volume-->>Backup: Delete snapshot after backup
else cleanup_snapshot is False
Volume-->>Backup: Keep snapshot
end
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 (
|
![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
🧹 Nitpick comments (4)
e2e/tests/regression/test_backup.robot (1)
110-122: Consider adding more test cases for edge scenarios.The test case effectively verifies the basic functionality. Consider adding tests for:
- Cleanup with empty volume (no data written)
- Cleanup with multiple existing snapshots
- Cleanup when snapshot is already removed
Example test structure:
Test Cleanup Snapshot After Backup With Empty Volume Given Create volume 0 with dataEngine=${DATA_ENGINE} And Attach volume 0 And Wait for volume 0 healthy When Create backup 0 for volume 0 with cleanup snapshot True Then Check snapshot for backup 0 of volume 0 exists Falsee2e/libs/backup/rest.py (3)
27-28: Consider aligning parameter names for consistency.The parameter name
cleanup_snapshotin the method signature differs fromcleanupBackupSnapshotin the API call. While functionally correct, consistent naming would improve code readability.Also, the indentation appears to be using both spaces and tabs. Consider using consistent indentation (preferably spaces) throughout the file.
- volume.snapshotBackup(name=snapshot.name, - cleanupBackupSnapshot=cleanup_snapshot) + volume.snapshotBackup( + name=snapshot.name, + cleanupBackupSnapshot=cleanup_snapshot + )
48-49: Fix indentation for consistency.The indentation in the
set_data_checksumcall is inconsistent with the rest of the file.- self.set_data_checksum(backup.name, - self.volume.get_last_data_checksum(volume_name)) + self.set_data_checksum( + backup.name, + self.volume.get_last_data_checksum(volume_name) + )
22-28: Consider adding error handling for cleanup failures.The current implementation doesn't handle potential failures during snapshot cleanup. Consider:
- Adding error handling for cleanup failures
- Verifying successful cleanup
- Adding logging for cleanup operations
This would improve the robustness of the test automation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/content/manual/pre-release/cluster-restore/restore-to-an-old-cluster.md(1 hunks)e2e/keywords/backup.resource(2 hunks)e2e/libs/backup/backup.py(2 hunks)e2e/libs/backup/rest.py(2 hunks)e2e/libs/keywords/backup_keywords.py(2 hunks)e2e/tests/regression/test_backup.robot(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/content/manual/pre-release/cluster-restore/restore-to-an-old-cluster.md
🧰 Additional context used
🪛 Ruff (0.8.2)
e2e/libs/backup/backup.py
76-76: Use not ... instead of False if ... else True
Replace with not ...
(SIM211)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (7)
e2e/libs/backup/backup.py (1)
18-19: LGTM!The addition of the
cleanup_snapshotparameter with a default value ofFalsemaintains backward compatibility while adding new functionality.e2e/libs/keywords/backup_keywords.py (2)
12-13: LGTM!Clean implementation that properly delegates to the backup implementation layer.
61-65: LGTM!Good separation of concerns with proper delegation to the backup implementation.
e2e/keywords/backup.resource (2)
18-21: LGTM!Good reuse of existing helper keyword
generate_name_with_suffixand maintains consistency with existing keyword patterns.
61-63: LGTM!Clean implementation that follows the established pattern of other similar keywords.
e2e/libs/backup/rest.py (2)
22-22: LGTM! Method signature change looks good.The new parameter
cleanup_snapshotwith default valueFalsemaintains backward compatibility while adding the new cleanup functionality.
22-28: Verify test coverage for cleanup functionality.Please ensure there are test cases that cover:
- Successful snapshot cleanup
- Failed snapshot cleanup
- Cleanup with various volume states
✅ Verification successful
Test coverage for cleanup functionality is verified ✓
The cleanup functionality is well-covered by the "Test Cleanup Snapshot After Backup Completed" test case in
e2e/tests/regression/test_backup.robot, which verifies:
- Successful snapshot cleanup
- Normal backup without cleanup
- Volume state handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test cases covering the new cleanup functionality rg -l "cleanup.*snapshot" --type py --type robotLength of output: 295
Script:
#!/bin/bash # Check content of the robot test file and integration test echo "=== Robot Framework Test ===" cat e2e/tests/regression/test_backup.robot echo -e "\n=== Integration Test ===" cat manager/integration/tests/test_snapshot.pyLength of output: 33742
| def check_snapshot_exists_for_backup(self, volume_name, backup_id, | ||
| exists=True): | ||
| backup = self.backup.get(backup_id, volume_name) | ||
| snap_name = backup.snapshotName | ||
| snapshot_id = self.backup.snapshot.get_snapshot_id(snap_name) | ||
| snap = self.backup.snapshot.get(volume_name, snapshot_id) | ||
| snap_exists = False if snap.removed else True | ||
| assert snap_exists == exists, \ | ||
| f"Snapshot {snap_name} exists: {snap_exists}, expected: {exists}" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null check and simplify boolean expression.
The implementation should handle cases where backup.snapshotName might be None, and the boolean expression can be simplified.
Apply this diff to improve the implementation:
def check_snapshot_exists_for_backup(self, volume_name, backup_id,
exists=True):
backup = self.backup.get(backup_id, volume_name)
+ if not backup or not backup.snapshotName:
+ raise ValueError(f"Backup {backup_id} not found or missing snapshot name")
snap_name = backup.snapshotName
snapshot_id = self.backup.snapshot.get_snapshot_id(snap_name)
snap = self.backup.snapshot.get(volume_name, snapshot_id)
- snap_exists = False if snap.removed else True
+ snap_exists = not snap.removed
assert snap_exists == exists, \
f"Snapshot {snap_name} exists: {snap_exists}, expected: {exists}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def check_snapshot_exists_for_backup(self, volume_name, backup_id, | |
| exists=True): | |
| backup = self.backup.get(backup_id, volume_name) | |
| snap_name = backup.snapshotName | |
| snapshot_id = self.backup.snapshot.get_snapshot_id(snap_name) | |
| snap = self.backup.snapshot.get(volume_name, snapshot_id) | |
| snap_exists = False if snap.removed else True | |
| assert snap_exists == exists, \ | |
| f"Snapshot {snap_name} exists: {snap_exists}, expected: {exists}" | |
| def check_snapshot_exists_for_backup(self, volume_name, backup_id, | |
| exists=True): | |
| backup = self.backup.get(backup_id, volume_name) | |
| if not backup or not backup.snapshotName: | |
| raise ValueError(f"Backup {backup_id} not found or missing snapshot name") | |
| snap_name = backup.snapshotName | |
| snapshot_id = self.backup.snapshot.get_snapshot_id(snap_name) | |
| snap = self.backup.snapshot.get(volume_name, snapshot_id) | |
| snap_exists = not snap.removed | |
| assert snap_exists == exists, \ | |
| f"Snapshot {snap_name} exists: {snap_exists}, expected: {exists}" |
🧰 Tools
🪛 Ruff (0.8.2)
76-76: Use not ... instead of False if ... else True
Replace with not ...
(SIM211)
ref: longhorn/longhorn 9213 Signed-off-by: James Lu <[email protected]>
Which issue(s) this PR fixes:
Issue # longhorn/longhorn#9213
What this PR does / why we need it:
Special notes for your reviewer:
Additional documentation or context
Summary by CodeRabbit
Release Notes
Documentation
New Features
Tests