feat(backup): delete snapshot immediately after backup completed #3505
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 17 minutes and 15 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 (13)
WalkthroughThis pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant User
participant RecurringJob
participant VolumeManager
participant BackupController
User->>RecurringJob: Create/Update with CleanupBackupSnapshot
RecurringJob->>VolumeManager: Trigger Backup
VolumeManager->>BackupController: Perform Backup
BackupController->>BackupController: Check CleanupBackupSnapshot
alt CleanupBackupSnapshot is true
BackupController->>BackupController: Delete Snapshot
end
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
client/generated_snapshot_input.go (1)
10-11: Add documentation for CleanupBackupSnapshot fieldConsider adding a comment to document the purpose and behavior of this field, especially since it's part of the client API.
+ // CleanupBackupSnapshot determines whether to delete the snapshot immediately after backup completion CleanupBackupSnapshot bool `json:"cleanupBackupSnapshot,omitempty" yaml:"cleanupBackupSnapshot,omitempty"`app/recurringjob/type.go (1)
23-29: Improve field organization and documentation consistencyThe fields should be logically grouped and documented consistently. Consider reorganizing the fields to keep related ones together.
type Job struct { api *longhornclient.RancherClient // Rancher client used to interact with the Longhorn API. lhClient *lhclientset.Clientset // Kubernetes clientset for Longhorn resources. eventRecorder record.EventRecorder // Used to record events related to the job. logger logrus.FieldLogger // Log messages related to the job. - name string // Name for the RecurringJob. - namespace string // Kubernetes namespace in which the RecurringJob is running. - retain int // Number of task CRs to retain.+ - cleanupBackupSnapshot bool - task longhorn.RecurringJobType // Type of task to be executed. - parameters map[string]string // Additional parameters for the task. - executionCount int // Number of times the job has been executed. + // Job configuration + name string // Name for the RecurringJob. + namespace string // Kubernetes namespace in which the RecurringJob is running. + task longhorn.RecurringJobType // Type of task to be executed. + + // Task configuration + retain int // Number of task CRs to retain. + cleanupBackupSnapshot bool // Whether to delete the snapshot after backup completion. + parameters map[string]string // Additional parameters for the task. + + // Runtime state + executionCount int // Number of times the job has been executed. }manager/engine.go (1)
257-257: Add documentation for the new parameter.Consider adding a comment to document the purpose of the
cleanupSnapparameter.+// BackupSnapshot creates a backup of the specified snapshot. +// Parameters: +// backupName: name of the backup +// backupTargetName: name of the backup target +// volumeName: name of the volume +// snapshotName: name of the snapshot to backup +// labels: labels to apply to the backup +// backupMode: mode of the backup (full or incremental) +// cleanupSnap: whether to delete the snapshot after successful backup func (m *VolumeManager) BackupSnapshot(backupName, backupTargetName, volumeName, snapshotName string, labels map[string]string, backupMode string, cleanupSnap bool) error {k8s/crds.yaml (1)
2879-2882: Consider simplifying the description, but the implementation is good.The field is well-implemented and serves its purpose. Consider making the description more concise, similar to other fields in the CRD, e.g., "Whether to cleanup the snapshot after the backup is completed."
cleanupBackupSnapshot: - description: The flag of cleaning up the corresponding snapshot after - the backup is completed. + description: Whether to cleanup the snapshot after the backup is completed. type: boolean
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
api/model.go(1 hunks)api/recurringjob.go(1 hunks)api/snapshot.go(1 hunks)app/recurringjob/job.go(1 hunks)app/recurringjob/type.go(1 hunks)app/recurringjob/volume.go(1 hunks)client/generated_snapshot_input.go(1 hunks)controller/backup_controller.go(1 hunks)k8s/crds.yaml(2 hunks)k8s/pkg/apis/longhorn/v1beta2/backup.go(1 hunks)k8s/pkg/apis/longhorn/v1beta2/recurringjob.go(1 hunks)manager/engine.go(2 hunks)manager/recurringjob.go(1 hunks)
👮 Files not reviewed due to content moderation or server errors (4)
- app/recurringjob/volume.go
- controller/backup_controller.go
- api/model.go
- api/recurringjob.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (7)
manager/recurringjob.go (1)
75-75: Consider adding validation for CleanupBackupSnapshot fieldWhile the field update is implemented correctly, consider adding validation to ensure the field value is handled appropriately when used with different job types (e.g., backup vs. snapshot).
app/recurringjob/job.go (1)
67-73: Add validation for cleanupBackupSnapshot based on task typeThe cleanupBackupSnapshot field should only be used with backup tasks. Consider adding validation to ensure this field is only set when appropriate.
k8s/pkg/apis/longhorn/v1beta2/recurringjob.go (1)
54-56: LGTM! Well-documented field addition.The new
CleanupBackupSnapshotfield is properly documented, correctly tagged, and follows the existing struct field pattern.k8s/pkg/apis/longhorn/v1beta2/backup.go (1)
54-56: LGTM! Well-documented field addition.The new
CleanupSnapshotfield is properly documented, correctly tagged, and follows the existing struct field pattern.api/snapshot.go (1)
198-198: LGTM! Proper propagation of cleanup flag.The
CleanupBackupSnapshotflag is correctly passed through to the manager layer while maintaining all existing validations and error handling.manager/engine.go (1)
274-277: LGTM! Proper initialization of BackupSpec.The
CleanupSnapshotfield is correctly initialized in the BackupSpec struct while maintaining the existing initialization pattern.k8s/crds.yaml (1)
908-910: LGTM! ThecleanupSnapshotfield is well-defined.The boolean field is appropriately added to the BackupSpec with a clear description that matches the PR objectives.
Introduce a new field `Spec.CleanupSnapshot` to specify the backup will delete the snapshot after the backup is completed. ref: longhorn/longhorn 9213 Signed-off-by: James Lu <james.lu@suse.com>
after backup completed for recurring jobs. Introduce a new boolean field `Spec.CleanupBackupSnasphot` to check if it needs to delete the snapshot after the backup is completed when doing a recurring job. ref: longhorn/longhorn 9213 Signed-off-by: James Lu <james.lu@suse.com>
ref: longhorn/longhorn 9213 Signed-off-by: James Lu <james.lu@suse.com>
Which issue(s) this PR fixes:
Issue # longhorn/longhorn#9213
What this PR does / why we need it:
Spec.CleanupBackupSnasphotto check if it needs to delete the snapshot after the backup is completed when doing a recurring job.Spec.CleanupSnapshotto specify the backup will delete the snapshot after the backup is completed.Special notes for your reviewer:
Additional documentation or context