feat(backup): delete snapshot immediately after backup completed #3505
Conversation
WalkthroughThis pull request introduces a new Changes
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 YAMLlint (1.35.1)k8s/crds.yaml[Errno 2] No such file or directory: 'k8s/crds.yaml' 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.
mantissahz
force-pushed
the
issue9213
branch
from
7134b90 to
53f6655
Compare
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>
mantissahz
force-pushed
the
issue9213
branch
from
53f6655 to
70d76a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
k8s/crds.yaml (2)
908-910: Backup CRD – New FieldcleanupSnapshotAddedThe new boolean field
cleanupSnapshotin the Backup CRD specifies whether to clean up the snapshot after the backup is completed. The type and description are correctly defined.
Suggestion: Consider enhancing the description to explicitly mention that this cleanup applies to snapshots produced by backup operations (e.g., "Whether to automatically remove the snapshot after a backup is successfully completed.") for greater clarity in documentation.
2879-2882: RecurringJob CRD – New FieldcleanupBackupSnapshotAddedThe new field
cleanupBackupSnapshotin the RecurringJob spec provides a flag to control the immediate deletion of the corresponding snapshot once a recurring backup job completes. The boolean type is appropriate, and the description is clear.
Suggestion: You might consider clarifying in the description that this flag specifically affects recurring job backups. For example, "The flag for cleaning up the associated snapshot immediately after a recurring backup completes." This could help users differentiate it from similar snapshot cleanup options in other CRDs.
📜 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 skipped from review as they are similar to previous changes (12)
- client/generated_snapshot_input.go
- k8s/pkg/apis/longhorn/v1beta2/backup.go
- app/recurringjob/job.go
- app/recurringjob/type.go
- api/recurringjob.go
- manager/engine.go
- k8s/pkg/apis/longhorn/v1beta2/recurringjob.go
- manager/recurringjob.go
- app/recurringjob/volume.go
- controller/backup_controller.go
- api/snapshot.go
- api/model.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build binaries
- GitHub Check: Summary
🔇 Additional comments (1)
k8s/crds.yaml (1)
4259-4261: Volume CRD – New FieldStandbyAddedThe Volume CRD now includes the
Standbyfield to indicate the standby status of the volume. It is correctly defined as a boolean and straightforward in its purpose.
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