feat(backups): backup api rework #1873
Conversation
…oduction Signed-off-by: Andrey Kolkov <[email protected]>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThe changes introduce a new BackupClass custom resource that centralizes backup strategy and storage configuration. Backup jobs and plans now reference a BackupClass by name instead of individual storage and strategy references. A new resolver component matches application types to strategies within a BackupClass and derives concrete strategy references at runtime. Changes
Sequence DiagramsequenceDiagram
participant JobCtrl as BackupJob<br/>Controller
participant Resolver as BackupClass<br/>Resolver
participant BackupClass as BackupClass<br/>Resource
participant StrategyCtrl as Strategy<br/>Controller
JobCtrl->>Resolver: ResolveBackupClass(backupClassName, appRef)
Resolver->>Resolver: NormalizeApplicationRef(appRef)
Resolver->>BackupClass: Get(backupClassName)
BackupClass-->>Resolver: BackupClass{strategies[...]}
Resolver->>Resolver: Match strategy by<br/>normalized APIGroup + Kind
Resolver-->>JobCtrl: ResolvedBackupConfig{<br/>StrategyRef, Parameters}
JobCtrl->>StrategyCtrl: reconcile(job, resolved)
StrategyCtrl->>StrategyCtrl: Use resolved.StrategyRef<br/>for strategy ops
StrategyCtrl-->>JobCtrl: Result/Error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @androndo, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the backup API by introducing a Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a significant and well-executed refactoring of the backup API. The introduction of the BackupClass CRD is a great design choice that decouples backup strategies and storage configuration from the BackupJob and Plan resources, making the system more flexible and maintainable. The changes are consistent across the API types, controllers, and new tests have been added to cover the new logic. I've identified a few areas for improvement, mainly concerning code duplication and clarity, which should be straightforward to address.
| // Verify strategy ref | ||
| if resolved.StrategyRef.Kind != "Velero" { | ||
| t.Errorf("ResolveBackupClass() StrategyRef.Kind = %v, want Velero", resolved.StrategyRef.Kind) | ||
| } |
There was a problem hiding this comment.
The assertion for the resolved strategy is a bit weak as it only checks the Kind. To make the test more robust, I suggest you also verify the Name and APIGroup of the StrategyRef. This will ensure that the correct strategy is being selected from the BackupClass for each test case.
For example, you could add checks like:
if resolved.StrategyRef.Name != "velero-strategy-vm" {
t.Errorf("...")
}for the "successful resolution - matches VirtualMachine strategy" test. A good way to implement this would be to add an expectedStrategyRef field to your test case struct.
Signed-off-by: Andrey Kolkov <[email protected]>
Signed-off-by: Andrey Kolkov <[email protected]>
Signed-off-by: Andrey Kolkov <[email protected]>
Signed-off-by: Andrey Kolkov <[email protected]>
androndo
force-pushed
the
feat/backups-rework
branch
from
754ddfb to
6c11dff
Compare
dosubot
bot
added
size:XXL
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
api/backups/v1alpha1/plan_types.go (1)
31-34: Typo in comment: "Condtions" should be "Conditions".Proposed fix
-// Condtions +// Conditions const ( PlanConditionError = "Error" )internal/backupcontroller/plan_controller.go (1)
53-62: ConditionReasonshould be CamelCase without spaces.Per Kubernetes API conventions, the
Reasonfield should be a single CamelCase word. "Failed to parse cron spec" contains spaces and should be reformatted.Proposed fix
meta.SetStatusCondition(&p.Status.Conditions, metav1.Condition{ Type: backupsv1alpha1.PlanConditionError, Status: metav1.ConditionTrue, - Reason: "Failed to parse cron spec", + Reason: "InvalidCronSpec", Message: errWrapped.Error(), })internal/backupcontroller/velerostrategy_controller.go (1)
299-327: Artifact URI should use the Velero backup's namespace, not the BackupJob's namespace.Velero backups are created in the
cozy-veleronamespace (seeveleroNamespaceconstant at line 47 and thecreateVeleroBackupfunction), but the artifact URI at line 303 usesbackupJob.Namespace. This causes the URI to point to a namespace that will never contain the Velero Backup. ThedriverMetadataalready correctly capturesveleroBackup.Namespace, so the URI should do the same.Proposed fix
- artifact := &backupsv1alpha1.BackupArtifact{ - URI: fmt.Sprintf("velero://%s/%s", backupJob.Namespace, veleroBackup.Name), - } + artifact := &backupsv1alpha1.BackupArtifact{ + URI: fmt.Sprintf("velero://%s/%s", veleroBackup.Namespace, veleroBackup.Name), + }
♻️ Duplicate comments (2)
internal/backupcontroller/factory/backupjob.go (1)
12-26: Consolidate ApplicationRef normalization to avoid drift.This duplicates the default API group and normalization logic from the backupcontroller package. Consider moving the shared constant/function into a small common package (or API package) so both callers use a single source of truth without introducing cycles.
internal/backupcontroller/backupclass_resolver_test.go (1)
334-337: Strengthen StrategyRef assertions (Kind/Name/APIGroup).Right now the test only checks
Kind. Adding expectedNameandAPIGroupassertions would make the resolution tests more robust and prevent false positives.
🧹 Nitpick comments (4)
api/backups/v1alpha1/backupclass_types.go (1)
69-78: Consider adding validation for theKindfield.The
Kindfield is required but has no validation to prevent empty strings. Consider adding a minimum length validation.Proposed enhancement
// ApplicationSelector specifies which application types a strategy applies to. type ApplicationSelector struct { // APIGroup is the API group of the application. // If not specified, defaults to "apps.cozystack.io". // +optional APIGroup *string `json:"apiGroup,omitempty"` // Kind is the kind of the application (e.g., VirtualMachine, MySQL). + // +kubebuilder:validation:MinLength=1 Kind string `json:"kind"` }internal/backupcontroller/backupclass_resolver.go (1)
54-58: Redundant APIGroup extraction after normalization.Since
applicationRefis already normalized at line 46 (which guaranteesAPIGroupis non-nil and non-empty), this block will always take the branch at line 57. The initial assignment and nil check are unnecessary.Proposed simplification
- // Determine application API group (already normalized, but extract for matching) - appAPIGroup := DefaultApplicationAPIGroup - if applicationRef.APIGroup != nil { - appAPIGroup = *applicationRef.APIGroup - } + // Extract normalized API group for matching + appAPIGroup := *applicationRef.APIGroup // Find matching strategyinternal/backupcontroller/plan_controller.go (1)
66-76: Good practice: clearing error condition on success.However, the
Reasonfield here should also follow CamelCase convention.Proposed fix
meta.SetStatusCondition(&p.Status.Conditions, metav1.Condition{ Type: backupsv1alpha1.PlanConditionError, Status: metav1.ConditionFalse, - Reason: "Cron spec is valid", + Reason: "CronSpecValid", Message: "The cron schedule has been successfully parsed", })internal/backupcontroller/factory/backupjob_test.go (1)
123-150: Strengthen the job-name assertion to prevent false positives.Right now the test only checks length. It would be more robust to assert the exact expected name derived from the plan and scheduled timestamp.
Proposed test hardening
import ( + "fmt" "testing" "time" @@ validate: func(t *testing.T, job *backupsv1alpha1.BackupJob) { if job.Name == "" { t.Error("BackupJob name should be generated") } - // Name should start with plan name - if len(job.Name) < len("test-plan") { - t.Errorf("BackupJob name = %v, should start with test-plan", job.Name) - } + expected := fmt.Sprintf("test-plan-%d", tt.scheduled.Unix()/60) + if job.Name != expected { + t.Errorf("BackupJob name = %v, want %v", job.Name, expected) + } },
| return err | ||
| } | ||
|
|
||
| veleroBackupSpec, err := template.Template(&strategy.Spec.Template.Spec, app.Object) |
There was a problem hiding this comment.
| templateContext := map[string]interface{}{ | |
| "Application": app.Object, | |
| "Parameters": resolved.Parameters, | |
| } | |
| veleroBackupSpec, err := template.Template(&strategy.Spec.Template.Spec, templateContext) |
Also update teh docs
lllamnyp
left a comment
lllamnyp
left a comment
There was a problem hiding this comment.
Add parameters to templating
What this PR does
Release note
Closes #1880
The following functions related StorageRef were removed as they are no longer used after migrating to BackupClass API:
// - resolveBucketStorageRef: Previously resolved S3 credentials from Bucket storageRef
// - createS3CredsForVelero: Previously created Velero S3 credentials secrets
// - createBackupStorageLocation: Previously created Velero BackupStorageLocation resources
// - createVolumeSnapshotLocation: Previously created Velero VolumeSnapshotLocation resources
Summary by CodeRabbit
Release Notes
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.