feat(recurring-job): support system backup task #3476
Conversation
Signed-off-by: Chin-Ya Huang <[email protected]>
longhorn/longhorn-6534 Signed-off-by: Chin-Ya Huang <[email protected]>
longhorn/longhorn-6534 Signed-off-by: Chin-Ya Huang <[email protected]>
longhorn/longhorn-6534 Signed-off-by: Chin-Ya Huang <[email protected]>
longhorn/longhorn-6534 Signed-off-by: Chin-Ya Huang <[email protected]>
WalkthroughThis pull request introduces a comprehensive implementation of recurring system backups for Longhorn, a Kubernetes storage system. The changes involve creating a new package Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Finishing Touches
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
|
longhorn/longhorn-6534 Signed-off-by: Chin-Ya Huang <[email protected]>
c3y1huang
force-pushed
the
6534-recurring-system-backup
branch
from
cab0f1c to
33cc9a5
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: 6
🧹 Nitpick comments (11)
app/post_upgrade.go (1)
Line range hint
144-149: Consider improving error handling in pod list retrieval.The error handling in the pod list retrieval could be more informative. Consider including more context in the warning message.
- logrus.Warnf("failed to list pods: %v", err) + logrus.Warnf("failed to list manager pods in namespace %s: %v", u.namespace, err)app/recurring_job.go (3)
14-14: Ensure imported packages are necessary and used.The newly added import for
"github.com/longhorn/longhorn-manager/types"should be verified for usage within the file. If the types from this package are not used in the current code, consider removing the import to keep the code clean.
55-59: Handle potential error when getting Longhorn clientset.The function
recurringjob.GetLonghornClientset()may return an error that should be handled appropriately. Ensure that the error logging provides enough context for debugging.Consider enhancing the error message to include additional context:
lhClient, err := recurringjob.GetLonghornClientset() if err != nil { - return errors.Wrap(err, "failed to get clientset") + return errors.Wrap(err, "failed to get Longhorn clientset in recurringJob") }
76-80: Validate the handling of different recurring job types in the switch statement.The switch statement introduces support for
longhorn.RecurringJobTypeSystemBackup. Ensure that all desired recurring job types are properly handled and that the default case correctly starts volume jobs.Also, consider adding a default case to handle unexpected job types, possibly logging an error or warning.
Example:
default: return recurringjob.StartVolumeJobs(job, recurringJob) + default: + job.logger.Errorf("Unknown recurring job type: %v", recurringJob.Spec.Task) + return fmt.Errorf("unknown recurring job type: %v", recurringJob.Spec.Task)app/recurringjob/type.go (3)
17-29: Consider encapsulating client interfaces for better testability.The
Jobstruct contains concrete clients like*longhornclient.RancherClientand*lhclientset.Clientset. To improve testability and reduce coupling, consider injecting interfaces instead of concrete types.Define interfaces that encapsulate the required methods and update the
Jobstruct to use these interfaces. This approach facilitates mocking dependencies during testing.Example:
type RancherClientInterface interface { // Define required methods } type ClientsetInterface interface { // Define required methods } type Job struct { api RancherClientInterface lhClient ClientsetInterface // ... }
31-43: Ensure consistency in logging fields.The
VolumeJobstruct includes aloggerfield, which might duplicate theloggerin the embeddedJobstruct. Verify if this duplication is necessary or if theJob's logger can be reused to maintain consistency.If a separate logger is required, ensure that it includes context from both the
JobandVolumeJobstructs. Otherwise, consider removing the redundant logger field.Example:
type VolumeJob struct { *Job - logger logrus.FieldLogger // ... }
45-54: Clarify the purpose ofSystemBackupJobfields.Ensure that the fields
systemBackupNameandvolumeBackupPolicyin theSystemBackupJobstruct are correctly initialized and used. Provide comments to explain their roles in the system backup process.Consider adding comments to explain these fields:
type SystemBackupJob struct { *Job // systemBackupName is the name assigned to the system backup job instance. systemBackupName string // volumeBackupPolicy defines the backup policy for volumes during system backup. volumeBackupPolicy longhorn.SystemBackupCreateVolumeBackupPolicy }app/recurringjob/systembackup.go (2)
55-60: Avoid redundant logger fields inSystemBackupJob.Similar to
VolumeJob,SystemBackupJobincludes aloggerfield that may duplicate the logger from the embeddedJobstruct. Evaluate if it's necessary to have a separate logger or if the existing one can be utilized with added context.If additional context is needed, consider extending the existing logger rather than introducing a new field.
Example:
job.logger = job.logger.WithFields(logrus.Fields{ // additional fields })
104-122: Optimize cleanup of expired system backups.In the
cleanupmethod, the deletion of expired backups happens sequentially. For a large number of backups, this might be inefficient. Consider parallelizing deletions or implementing batching to improve performance.Example:
var wg sync.WaitGroup for _, systemBackupName := range expiredSystemBackups { wg.Add(1) go func(name string) { defer wg.Done() job.logger.Infof("Deleting system backup %v", name) err := job.DeleteSystemBackup(name) if err != nil { job.logger.WithError(err).Warnf("Failed to delete system backup %v", name) } }(systemBackupName) } wg.Wait()Ensure that concurrency is handled safely, especially if the underlying client is not thread-safe.
app/recurringjob/volume.go (1)
260-274: Fix typo in function namewaitForSnaphotReadyThe function name
waitForSnaphotReadycontains a typo. It should bewaitForSnapshotReady.Apply this diff to correct the typo and update references:
-func (job *VolumeJob) waitForSnaphotReady(volume *longhornclient.Volume, timeout int) error { +func (job *VolumeJob) waitForSnapshotReady(volume *longhornclient.Volume, timeout int) error {Also, update the function call:
- if err := job.waitForSnaphotReady(volume, SnapshotReadyTimeout); err != nil { + if err := job.waitForSnapshotReady(volume, SnapshotReadyTimeout); err != nil {app/recurringjob/constant.go (1)
6-6: Fix typo in constant nameHTTPClientTimoutThe constant
HTTPClientTimouthas a typo in its name. It should beHTTPClientTimeout.Apply this diff to correct the typo:
- HTTPClientTimout = 1 * time.Minute + HTTPClientTimeout = 1 * time.MinuteAlso, update any references to this constant in the codebase.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
app/post_upgrade.go(2 hunks)app/pre_upgrade.go(2 hunks)app/recurring_job.go(2 hunks)app/recurringjob/constant.go(1 hunks)app/recurringjob/job.go(1 hunks)app/recurringjob/systembackup.go(1 hunks)app/recurringjob/type.go(1 hunks)app/recurringjob/util.go(1 hunks)app/recurringjob/volume.go(1 hunks)app/util/util.go(2 hunks)datastore/longhorn.go(3 hunks)k8s/crds.yaml(73 hunks)k8s/pkg/apis/longhorn/v1beta2/recurringjob.go(5 hunks)types/types.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeFactor
app/recurringjob/volume.go
[notice] 427-542: app/recurringjob/volume.go#L427-L542
Complex Method
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build binaries
- GitHub Check: Summary
🔇 Additional comments (21)
app/util/util.go (2)
1-1: LGTM! Good package organization.Moving this utility function to a dedicated
utilpackage improves code organization and reusability.
Line range hint
21-22: Consider addressing the TODO comment about client wrapper.The comment indicates temporary code that should be removed when clients move to use the clientset. This technical debt should be tracked.
Would you like me to create a GitHub issue to track the removal of this wrapper once all clients have moved to use the clientset?
app/pre_upgrade.go (1)
19-19: LGTM! Clean integration with util package.The changes correctly integrate the moved
CreateEventBroadcasterfunction from the util package while maintaining existing error handling.Also applies to: 63-63
app/post_upgrade.go (1)
25-25: LGTM! Clean integration with util package.The changes correctly integrate the moved
CreateEventBroadcasterfunction from the util package while maintaining existing error handling.Also applies to: 70-70
app/recurringjob/type.go (1)
56-60: Ensure thread safety when handling time-related data.The
NameWithTimestampstruct might be used in concurrent operations. Verify that any usage of this struct in concurrent contexts handles synchronization appropriately to prevent race conditions.If the struct is accessed concurrently, consider using synchronization mechanisms or immutable data patterns.
app/recurringjob/systembackup.go (1)
41-42: Ensure uniqueness and validity of generated system backup names.The
systemBackupNamegeneration truncates and concatenates strings, which may lead to non-unique names if not carefully handled. Confirm that the generated names are unique and conform to Kubernetes naming conventions.Consider including a timestamp or a UUID to ensure uniqueness:
systemBackupName := fmt.Sprintf("%s-%s", sliceStringSafely(types.GetCronJobNameForRecurringJob(job.name), 0, 8), util.UUID())app/recurringjob/job.go (1)
1-131: The implementation injob.gois well-structured and clearThe code defines the
Jobstruct and associated methods effectively. Initialization, error handling, and client configurations are appropriately managed.k8s/pkg/apis/longhorn/v1beta2/recurringjob.go (4)
5-5: LGTM: Validation enum updated to include system backup taskThe validation enum is properly updated to include
system-backupas a valid recurring job type.Also applies to: 16-16
42-44: LGTM: Task field documentation updatedThe Task field documentation is properly updated to include the new
system-backupoption.
58-60: LGTM: Parameters field documentation updatedThe Parameters field documentation is properly updated to include the new
volume-backup-policyparameter.
79-79: LGTM: Print column description updatedThe print column description is properly updated to include the new
system-backupoption.app/recurringjob/util.go (1)
1-168: LGTM: Well-structured utility package for recurring jobsThe new utility package provides a clean implementation of recurring job management functions with proper error handling and logging.
types/types.go (1)
238-239: LGTM: New recurring job parameter constantsThe new constants for recurring job parameters follow the project's naming conventions.
datastore/longhorn.go (2)
5016-5030: LGTM: Validation logic for recurring job parametersThe validation logic for recurring job parameters is properly implemented with appropriate error handling.
5045-5046: LGTM: System backup added to valid recurring job tasksThe system backup task is properly added to the list of valid recurring job tasks.
k8s/crds.yaml (6)
2753-2756: LGTM: Description for groups field is clear and accurate.The description for the
groupsfield in the RecurringJob CRD accurately explains that when set to "default", the group will be added to the volume label when no other job label exists in the volume.
2757-2760: LGTM: Task field description is comprehensive and includes the new system-backup type.The description for the
taskfield in the RecurringJob CRD has been updated to include all valid task types, including the newly added "system-backup" task.
2829-2829: LGTM: Parameters field description is accurate.The description for the
parametersfield in the RecurringJob CRD spec clearly lists the supported parameters: "full-backup-interval" and "volume-backup-policy".
Line range hint
2837-2846: LGTM: Task field enumeration is complete and includes system-backup.The task field enumeration in the RecurringJob CRD spec has been updated to include all valid task types:
- Existing tasks: snapshot, snapshot-force-create, snapshot-cleanup, snapshot-delete, backup, backup-force-create, filesystem-trim
- New task: system-backup
2850-2856: LGTM: Status fields are well documented.The RecurringJobStatus fields are well documented:
- executionCount: Tracks the number of jobs triggered
- ownerID: Identifies the controller responsible for reconciliation
Line range hint
2694-2757: Verify system-backup task integration in printer columns.The printer columns configuration should be reviewed to ensure it properly displays system backup information.
Run this script to verify the printer columns configuration:
✅ Verification successful
System-backup task integration is properly configured
The system-backup task is correctly integrated in the CRD configuration with proper printer columns and documentation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if printer columns configuration needs updating for system backup task # Search for printer column definitions that might need updating for system backup rg -A 5 'additionalPrinterColumns:.*task' k8s/crds.yaml # Search for any potential missing system backup related columns rg -i 'system.?backup' k8s/crds.yamlLength of output: 1633
|
Regression test result: https://ci.longhorn.io/job/private/job/longhorn-tests-regression/8271/ |
| @@ -19,8 +19,7 @@ spec: | |||
| scope: Namespaced | |||
| versions: | |||
| - additionalPrinterColumns: | |||
| - description: The current state of the pod used to provision the backing image | |||
| file from source | |||
| - description: The current state of the pod used to provision the backing image file from source | |||
| jsonPath: .status.currentState | |||
There was a problem hiding this comment.
Is there a war between yaml formatters on this file? Two days ago, commit a6cb36e split this and other long descriptions from a single line into multiple lines. Now they are being reverted into a single line again. IIRC, I've seen it go back and forth more than once.
There was a problem hiding this comment.
I am not sure, but this is the output I got after running k8s/generate_code.sh. Could you try running the script on your end and let me know if it comes out differently?
There was a problem hiding this comment.
Interesting. I get this output:
jmunson@JMunson:~/repos/Longhorn/longhorn-manager$ ./k8s/generate_code.sh
controller-gen is missing
Prepare to install controller-gen
go: sigs.k8s.io/[email protected] requires go >= 1.22.0; switching to go1.22.10
kustomize is missing
Prepare to install kustomize
~/go/src/github.com/kubernetes-sigs ~/repos/Longhorn/longhorn-manager
~/repos/Longhorn/longhorn-manager
Generating deepcopy funcs
Generating clientset for longhorn:v1beta1,v1beta2 at github.com/longhorn/longhorn-manager/k8s/pkg/client/clientset
Generating listers for longhorn:v1beta1,v1beta2 at github.com/longhorn/longhorn-manager/k8s/pkg/client/listers
Generating informers for longhorn:v1beta1,v1beta2 at github.com/longhorn/longhorn-manager/k8s/pkg/client/informers
Generating CRD
./k8s/generate_code.sh: line 68: controller-gen: command not found
And it doesn't touch crds.yaml
There was a problem hiding this comment.
jmunson@JMunson:~/repos/Longhorn/longhorn-manager$ go version
go version go1.23.4 linux/amd64
There was a problem hiding this comment.
My gopath was not in my PATH.
When I run it, I get the multi-line descriptions
There was a problem hiding this comment.
Seems like this whole question will be resolved by longhorn/longhorn#10193.
| } | ||
|
|
||
| job.logger.Infof("Waiting for system backup to reach state %v, current state is %v", expectedStates, systemBackup.Status.State) | ||
| time.Sleep(5 * time.Second) |
There was a problem hiding this comment.
Would it be reasonable to use the named constant WaitInterval here?
Which issue(s) this PR fixes:
Issue longhorn/longhorn#6534
What this PR does / why we need it:
Support system backup task in the recurring job.
Special notes for your reviewer:
NoneAdditional documentation or context
None