Restore backup from specific location #5491
Conversation
📝 WalkthroughWalkthroughThe pull request introduces enhancements to the backup and restore functionalities across multiple files in the supervisor's backup system. The changes primarily focus on improving location handling during backup and restore operations, adding more flexibility for specifying backup locations. The modifications include updates to the API, backup management, and context handling, with new methods for creating and opening backups asynchronously. The changes also include corresponding test updates to validate the new functionality. Changes
Sequence DiagramsequenceDiagram
participant API as Backup API
participant Manager as BackupManager
participant Backup as Backup Class
API->>Manager: Request backup/restore
Manager->>Backup: Create or Open backup
alt Backup Creation
Backup-->>Manager: Prepare backup file
else Backup Restoration
Backup->>Manager: Validate location
alt Location Specified
Manager->>Backup: Restore from specified location
else Default Location
Manager->>Backup: Restore from default location
end
end
Manager-->>API: Confirm backup/restore operation
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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)
supervisor/backups/backup.py (1)
6-7: Imports Update: Ensure Unused Imports are NecessaryThe addition of
Awaitablein the import statement may not be necessary if it is not used elsewhere in the code. Please verify and remove any unused imports to keep the code clean.tests/api/test_backups.py (3)
823-823: Consider making test file creation more robustThe test file creation could be more robust by adding content to verify after restoration and ensuring the parent directory exists.
-(test_file := coresys.config.path_share / "test.txt").touch() +(test_file := coresys.config.path_share / "test.txt").parent.mkdir(parents=True, exist_ok=True) +test_file.write_text("test content")
849-852: Improve error message assertion precisionThe error message assertion could be more precise by using a constant or helper method to avoid hardcoding the error message string.
-assert ( - body["message"] - == f"Cannot open backup at {backup.all_locations[None].as_posix()}, file does not exist!" -) +# Define error message constant at module level +BACKUP_NOT_FOUND_ERROR = "Cannot open backup at {}, file does not exist!" +assert body["message"] == BACKUP_NOT_FOUND_ERROR.format( + backup.all_locations[None].as_posix() +)
858-859: Add more comprehensive restoration verificationThe test could verify more aspects of the restored file to ensure the restoration was completely successful.
assert resp.status == 200 assert test_file.is_file() +# Verify file content and permissions +assert test_file.read_text() == "test content" +assert test_file.stat().st_mode & 0o777 == 0o644 # Verify default permissions
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
supervisor/api/backups.py(3 hunks)supervisor/backups/backup.py(5 hunks)supervisor/backups/manager.py(7 hunks)tests/api/test_backups.py(1 hunks)tests/backups/test_backup.py(1 hunks)
🔇 Additional comments (13)
supervisor/backups/backup.py (4)
380-401: Enhancement: Introduce Asynchronous Context Managers
The introduction of create and open methods with @asynccontextmanager improves the clarity and management of asynchronous operations in the Backup class. This change enhances the code's readability and follows best practices for asynchronous context management.
402-417: Consistent Error Handling in open Method
The open method properly raises a BackupError if the specified backup location does not exist or the file is missing. This ensures that errors are caught early and handled appropriately, maintaining the robustness of the backup restoration process.
Line range hint 436-459: Validation and Backup Metadata Addition
The _create_cleanup method effectively validates the backup data and adds the backup.json to the tar file. This ensures that backups are correctly formatted and contain the necessary metadata.
Line range hint 578-590: Proper Usage of the location Parameter in _do_restore
The addition of the location parameter in the _do_restore method allows for restoring backups from specified locations. The parameter is correctly passed to backup.open(location), ensuring that the backup restoration process utilizes the correct backup file.
supervisor/backups/manager.py (4)
408-408: Update Context Manager Usage with New create Method
The change from async with backup: to async with backup.create(): reflects the updated context manager implementation in the Backup class. This ensures that backups are created using the new asynchronous context manager, improving code clarity and consistency.
Line range hint 578-590: Enhanced Restore Functionality with location Parameter
The addition of the location parameter in the _do_restore method and its usage in async with backup.open(location): correctly integrates the ability to restore backups from specific locations. This change aligns with the updated backup handling and enhances the flexibility of the restore process.
Line range hint 675-714: Update do_restore_full Method with location Parameter
Including the location parameter in do_restore_full allows users to specify the backup location when performing a full restore. The method correctly passes the parameter to _do_restore, ensuring seamless integration with the updated restore logic.
Line range hint 743-784: Update do_restore_partial Method with location Parameter
The do_restore_partial method now includes the location parameter, providing consistency with the full restore method. This addition enhances the partial restore functionality by allowing location specification, and correctly passes the parameter to _do_restore.
tests/backups/test_backup.py (1)
17-20: Adjust Test to Use Updated create Context Manager
The test test_new_backup_stays_in_folder is updated to use async with backup.create():, aligning with the changes made to the Backup class. This ensures that the test accurately reflects the new backup creation process and remains valid.
supervisor/api/backups.py (3)
86-88: Addition of ATTR_LOCATION to SCHEMA_RESTORE_FULL
Including ATTR_LOCATION in SCHEMA_RESTORE_FULL allows users to specify a backup location when performing a full restore. This enhancement increases the flexibility of the API and aligns with the extended restore capabilities.
384-386: Validate Backup Location in restore_full Method
The _validate_cloud_backup_location method is appropriately called with the ATTR_LOCATION from the request body or the backup's default location. This ensures that the restore operation is authorized and the location is accessible.
404-406: Validate Backup Location in restore_partial Method
Similarly, the restore_partial method now validates the backup location, enhancing security and reliability when restoring backups. This consistent validation prevents unauthorized access to backups in restricted locations.
tests/api/test_backups.py (1)
815-859: LGTM! Well-structured test for backup restoration from specific locations
The test comprehensively covers:
- Backup creation with multiple locations
- Error handling for missing backups
- Successful restoration from alternate location
Proposed change
Allow users to restore a backup from a specific location in case they prefer a particular mount over another (perhaps one is flaky). Request comes from the cloud backup design.
Type of change
Additional information
Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed:
Summary by CodeRabbit
New Features
Bug Fixes
Tests