fix(robot): support multiple backup stores #2220
Conversation
fix test case `test_recurring_jobs_allow_detached_volume` ref: longhorn/longhorn 5411 Signed-off-by: James Lu <[email protected]>
WalkthroughThis pull request introduces comprehensive modifications to backup and volume management across multiple files. The changes primarily focus on enhancing backup verification and restoration processes by adding a new Changes
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 (3)
e2e/libs/backup/rest.py (2)
90-97: Consider raising an error when the backup volume is not found.
Currently, if the backup volume is not found, the function returns None. If the caller requires a valid backup volume for subsequent operations, raising an exception might make it easier to spot issues early and handle them more predictably.
207-208: Consider parallel or batched deletion for performance.
If there are many backup volumes, deleting them sequentially could become slow. You might consider parallel or batched deletions, but do ensure concurrency safety.e2e/tests/negative/test_dr_volume_node_reboot.robot (1)
52-52: Maintain consistent terminology.
“Wait for volume 1 restoration from backup 0 of volume 0 completed” keeps the naming scheme consistent, reducing risk of test confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
e2e/keywords/backup.resource(1 hunks)e2e/keywords/volume.resource(2 hunks)e2e/libs/backup/rest.py(3 hunks)e2e/tests/negative/test_dr_volume_node_reboot.robot(2 hunks)e2e/tests/regression/test_backup.robot(1 hunks)manager/integration/tests/backupstore.py(1 hunks)manager/integration/tests/test_recurring_job.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
e2e/libs/backup/rest.py
171-171: Comparison to None should be cond is not None
Replace with cond is not None
(E711)
🔇 Additional comments (18)
e2e/keywords/backup.resource (1)
43-46: Well-structured update to ensure uniqueness of backup references.
Adding “${source_volume_id}” clarifies the linkage between backup and its source volume. This prevents ambiguity when multiple volumes share similar naming patterns.
e2e/tests/negative/test_dr_volume_node_reboot.robot (6)
49-50: Improves clarity of DR volume creation flow.
Specifying “from backup 0 of volume 0” explicitly ties the DR volume to its source volume, eliminating confusion when multiple backups exist.
56-56: Verifying correct data source ensures robust DR testing.
The “Check volume 1 data is backup 0 of volume 0” step is an effective final data integrity check.
81-82: Retains clarity in incremental restoration references.
Adding “of volume 0” helps confirm that the DR volume references the correct source during restore.
85-85: Explicit reference for incremental restore consistency.
This step clarifies that the newly triggered incremental restore is from backup 1 of volume 0, aligning with test logic elsewhere.
87-87: Reinforce clarity of restoration process.
Referring to the specific source volume ensures that we track the correct restore event following node reboots.
91-91: Confirms final data correctness after incremental restore.
Verifying the DR volume truly matches “backup 1 of volume 0” strengthens confidence in incremental restoration logic.
e2e/tests/regression/test_backup.robot (6)
66-71: Good approach for initializing multiple DR volumes from the same backup.
Creating DR volumes 1, 2, and 3 all from “backup 0 of volume 0” ensures thorough coverage of parallel DR volume scenarios.
79-79: Efficient data validation step.
Ensuring the DR volume’s data matches the backup source after activation fosters robust incremental restore checks.
85-85: Incremental restore correctness.
Waiting for DR volume 2’s restoration from “backup 1 of volume 0” clarifies which volume/backup pair is being tested.
89-89: Validating data post-restore.
Executions of “Check volume 2 data is backup 1 of volume 0” confirm that the restore process remains consistent after new data is added.
94-94: Maintaining clarity for DR volume 3 restore.
Ensuring “Wait for volume 3 restoration from backup 2 of volume 0 completed” keeps consistent naming for incremental backup steps.
98-98: Ensures final incremental restore data integrity.
Verifying “Check volume 3 data is backup 2 of volume 0” completes the chain of incremental backups, guaranteeing correctness.
e2e/keywords/volume.resource (3)
19-24: LGTM: Well-structured enhancement for DR volume creation
The addition of source_volume_id parameter improves the traceability of backup sources, which is essential for supporting multiple backup stores. The implementation follows existing patterns for name generation and backup URL retrieval.
192-196: LGTM: Consistent enhancement for backup restoration tracking
The addition of source_volume_id parameter maintains consistency with the DR volume creation changes and properly tracks backup sources during restoration.
198-202: LGTM: Consistent enhancement for backup restoration initialization
The addition of source_volume_id parameter maintains consistency across the backup restoration workflow and properly tracks backup sources.
manager/integration/tests/backupstore.py (1)
83-83: LGTM: Enhanced test cleanup
Adding backup cleanup in the fixture teardown ensures proper cleanup of backup resources, preventing test interference and maintaining a clean test environment.
manager/integration/tests/test_recurring_job.py (1)
786-786: LGTM: Improved test reliability
Adding retry mechanism to find_backup_volume call improves test reliability by properly handling potential delays in backup volume availability.
Modify keywords of related resources to get backups with corresponding volumes. ref: longhorn/lonhorn 5411 Signed-off-by: James Lu <[email protected]>
mantissahz
force-pushed
the
issue5411v1219t1
branch
from
287de91 to
9fde0e2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
e2e/keywords/volume.resource (1)
19-23: LGTM! Consider adding documentation.The changes correctly implement source volume tracking for DR volume creation. Consider adding a docstring to document the parameters and their purpose.
Add documentation like this:
Create DR volume ${volume_id} from backup ${backup_id} of volume ${source_volume_id} + [Documentation] Creates a DR volume from a backup + ... ${volume_id} - The ID of the new DR volume + ... ${backup_id} - The ID of the backup to restore from + ... ${source_volume_id} - The ID of the volume that created the backup [Arguments] &{config}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
e2e/keywords/backup.resource(1 hunks)e2e/keywords/volume.resource(2 hunks)e2e/libs/backup/rest.py(3 hunks)e2e/tests/negative/test_dr_volume_node_reboot.robot(2 hunks)e2e/tests/regression/test_backup.robot(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- e2e/tests/regression/test_backup.robot
- e2e/keywords/backup.resource
- e2e/tests/negative/test_dr_volume_node_reboot.robot
- e2e/libs/backup/rest.py
🔇 Additional comments (2)
e2e/keywords/volume.resource (2)
192-195: LGTM! Changes maintain consistency with DR volume creation.
The addition of source_volume_id parameter correctly tracks the source volume during restoration completion checks.
198-201: LGTM! Changes maintain consistency across restoration keywords.
The addition of source_volume_id parameter correctly tracks the source volume during restoration start checks.
|
@mergify backport v1.8.x |
✅ Backports have been created
|
Which issue(s) this PR fixes:
Issue # longhorn/longhorn#5411
What this PR does / why we need it:
Special notes for your reviewer:
Additional documentation or context
e2e test cases running: https://ci.longhorn.io/job/private/job/longhorn-e2e-test/2282
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores