feat(system-backup): check outdated backup in if-not-present volume backup policy #3246
Conversation
WalkthroughThe changes in the pull request focus on the 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
Documentation and Community
|
c3y1huang
force-pushed
the
6027-system-backup-skip-when-backup-up-to-date
branch
from
514365a
to
c664be8
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
controller/system_backup_controller.go (2)
49-63: Ensure Consistent Naming for Error Constants
The error constant names could be standardized for better consistency. Some constants use the action last (e.g., SystemBackupErrArchive), while others include additional context (e.g., SystemBackupErrTimeoutSnapshot). Consider adopting a uniform naming convention, such as <Entity><Action><Outcome>, to enhance readability and maintainability.
959-961: Add Logging for Snapshot Readiness
You might consider adding a log statement when the snapshot becomes ready to improve observability.
if snapshot.Status.ReadyToUse {
+ log.Infof("Snapshot %v is ready to use", snapshot.Name)
return snapshot, nil
}📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- controller/system_backup_controller.go (4 hunks)
🔇 Additional comments (2)
controller/system_backup_controller.go (2)
774-786: Verify Backup Freshness Check Logic
The backupVolumesIfNotPresent method checks if the volume backup is up-to-date using the isVolumeBackupUpToDate function. Ensure that this function accurately determines the need for a new backup by correctly comparing snapshot and backup timestamps.
928-964: Validate Timeout Handling in Snapshot Creation
The createVolumeSnapshot function correctly implements a timeout mechanism when waiting for the snapshot to become ready. Ensure that datastore.SystemBackupTimeout is set appropriately to prevent premature timeouts or excessively long waits.
c3y1huang
force-pushed
the
6027-system-backup-skip-when-backup-up-to-date
branch
from
c664be8
to
3865408
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
c3y1huang
force-pushed
the
6027-system-backup-skip-when-backup-up-to-date
branch
from
3865408
to
70ad3d0
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🛑 Comments failed to post (1)
controller/system_backup_controller.go (1)
867-916: 🛠️ Refactor suggestion
Consider adding metrics for backup freshness tracking.
The
isVolumeBackupUpToDatemethod effectively checks backup freshness. Consider adding metrics to track:
- Time since last backup
- Number of outdated backups
- Average backup freshness
This would help in monitoring backup health and setting up alerts.
c3y1huang
force-pushed
the
6027-system-backup-skip-when-backup-up-to-date
branch
from
70ad3d0
to
d85549b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
controller/system_backup_controller_test.go (1)
Line range hint
89-114: Add test cases for outdated backup scenarios.The current test cases verify if a backup exists but don't cover the new functionality of checking if the backup is outdated. Consider adding test cases for:
- Backup exists but is outdated (should create new backup)
- Backup exists and is recent (should not create new backup)
- Edge cases like backup timestamp exactly at the threshold
Would you like me to help draft these additional test cases?
controller/system_backup_controller.go (1)
867-916: LGTM: Well-implemented backup freshness check.The implementation correctly checks if a backup is up-to-date by comparing snapshot timestamps, with proper error handling and logging.
Consider extracting time format constant.
The RFC3339 time format is used multiple times. Consider extracting it to a constant for better maintainability.
+const ( + snapshotTimeFormat = time.RFC3339 +) func (c *SystemBackupController) isVolumeBackupUpToDate(volume *longhorn.Volume, systemBackup *longhorn.SystemBackup) (bool, error) { // ... existing code ... - lastBackupTime, err := time.Parse(time.RFC3339, lastBackupSnapshot.Status.CreationTime) + lastBackupTime, err := time.Parse(snapshotTimeFormat, lastBackupSnapshot.Status.CreationTime) if err != nil { return false, err } // ... more code ... - snapshotTime, err := time.Parse(time.RFC3339, snapshot.Status.CreationTime) + snapshotTime, err := time.Parse(snapshotTimeFormat, snapshot.Status.CreationTime) if err != nil { return false, err } // ... rest of the code ... }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
controller/system_backup_controller.go(6 hunks)controller/system_backup_controller_test.go(1 hunks)
🔇 Additional comments (5)
controller/system_backup_controller_test.go (2)
Line range hint 147-167: LGTM! Comprehensive error handling test coverage.
The test cases effectively cover various error scenarios including:
- Upload timeouts
- Upload failures
- Configuration retrieval failures
- State transitions during errors
58-58: Document the purpose of SkipListerCheck and consider test isolation.
Setting global state in tests can lead to test pollution. Consider:
- Documenting why
SkipListerCheckis necessary - Moving this setting to individual test cases where needed
- Resetting the value after each test to prevent side effects
Let's check if this setting is used elsewhere:
controller/system_backup_controller.go (3)
50-64: LGTM: Error constants are well-defined.
The new error constants are clear, descriptive, and follow the existing naming pattern.
740-742: LGTM: Proper context management.
Good addition of context with cancel for managing operation lifecycle in backup functions.
Also applies to: 776-778
934-976: LGTM: Well-structured snapshot creation with proper timeout handling.
The implementation includes proper context cancellation, timeout handling, and status checks.
…ackup policy longhorn/longhorn-6027 Signed-off-by: Chin-Ya Huang <chin-ya.huang@suse.com>
derekbit
force-pushed
the
6027-system-backup-skip-when-backup-up-to-date
branch
from
d85549b
to
87f2398
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
controller/system_backup_controller.go (1)
934-976: Consider increasing the polling interval.The snapshot readiness polling occurs every second, which might be too frequent. Consider increasing the interval to reduce system load, especially when dealing with multiple snapshots.
Apply this diff to optimize the polling interval:
- ticker := time.NewTicker(time.Second) + ticker := time.NewTicker(5 * time.Second)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
controller/system_backup_controller.go(6 hunks)controller/system_backup_controller_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- controller/system_backup_controller_test.go
🔇 Additional comments (3)
controller/system_backup_controller.go (3)
50-64: LGTM! Error constants are well-defined.
The new error constants follow consistent naming conventions and provide clear, descriptive messages for various error scenarios.
867-916: LGTM! Well-implemented backup freshness check.
The isVolumeBackupUpToDate method effectively implements the requirement to check if the latest backup is up-to-date by:
- Checking for existence of last backup
- Comparing snapshot timestamps
- Properly handling error cases
- Providing informative logging
918-931: Refer to previous comment about unique backup names.
Which issue(s) this PR fixes:
Issue longhorn/longhorn#6027
What this PR does / why we need it:
When the system backup volume backup policy is set to
if-not-present, check the last backup and create a backup when it's outdated.longhorn/longhorn#6027 (comment)
Special notes for your reviewer:
NoneAdditional documentation or context
None