feat(backups): implement Job backup strategy#1721
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 @lllamnyp, 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 lays the foundational groundwork for a robust backup system by implementing the Highlights
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 introduces a new JobStrategy implementation for BackupJob resources, allowing the creation of Kubernetes Jobs based on templated PodTemplateSpec definitions. It also includes a generic templating utility to process Go template expressions within Kubernetes objects. The changes are a good step towards a flexible backup strategy. However, there are a few areas that could be improved for robustness and error handling, particularly regarding the templating logic and handling of API group references.
| templateFunc := func(in string) string { | ||
| out, err := template(in, templateContext) | ||
| if err != nil { | ||
| return in | ||
| } | ||
| return out | ||
| } |
There was a problem hiding this comment.
The templateFunc silently swallows errors from template(in, templateContext), returning the original string in instead. This can lead to subtle bugs where template expressions are malformed or refer to non-existent fields, but no error is propagated, and the resulting object contains untemplated strings. It would be more robust to propagate these errors or at least log them with a higher severity to make debugging easier.
| templateFunc := func(in string) string { | |
| out, err := template(in, templateContext) | |
| if err != nil { | |
| return in | |
| } | |
| return out | |
| } | |
| templateFunc := func(in string) string { | |
| out, err := template(in, templateContext) | |
| if err != nil { | |
| // TODO: Log this error or return it from Template function for better error visibility | |
| return in | |
| } | |
| return out | |
| } |
| if j.Spec.ApplicationRef.APIGroup != nil { | ||
| applicationRefAPIGroup = *j.Spec.ApplicationRef.APIGroup | ||
| } | ||
| if j.Spec.StrategyRef.APIGroup != nil { | ||
| strategyRefAPIGroup = *j.Spec.StrategyRef.APIGroup | ||
| } | ||
| if j.Spec.StorageRef.APIGroup != nil { | ||
| storageRefAPIGroup = *j.Spec.StorageRef.APIGroup | ||
| } |
There was a problem hiding this comment.
The APIGroup fields in TypedLocalObjectReference are pointers to strings. If they are nil, the current logic initializes applicationRefAPIGroup, strategyRefAPIGroup, and storageRefAPIGroup to empty strings. While an empty string for APIGroup typically refers to the core Kubernetes API group, this behavior is implicit and not explicitly handled or documented. It might be clearer to explicitly check for nil and assign "" if that's the intended behavior for core API group resources, or handle it as an error if APIGroup is always expected to be set for non-core resources.
var applicationRefAPIGroup string
var strategyRefAPIGroup string
var storageRefAPIGroup string
if j.Spec.ApplicationRef.APIGroup != nil {
applicationRefAPIGroup = *j.Spec.ApplicationRef.APIGroup
} else {
// Default to core API group if not specified
applicationRefAPIGroup = ""
}
if j.Spec.StrategyRef.APIGroup != nil {
strategyRefAPIGroup = *j.Spec.StrategyRef.APIGroup
} else {
strategyRefAPIGroup = ""
}
if j.Spec.StorageRef.APIGroup != nil {
storageRefAPIGroup = *j.Spec.StorageRef.APIGroup
} else {
storageRefAPIGroup = ""
}| values, ok := app.Object["spec"].(map[string]any) | ||
| if !ok { | ||
| values = map[string]any{} | ||
| } |
There was a problem hiding this comment.
The type assertion app.Object["spec"].(map[string]any) and subsequent fallback to an empty map if the assertion fails could mask potential issues. If the application object's spec is expected to always be a map[string]any, a failure here might indicate a malformed resource or an unexpected API version. It would be safer to log a warning or return an error in such cases, rather than silently proceeding with an empty map, which could lead to incorrect template rendering.
values, ok := app.Object["spec"].(map[string]any)
if !ok {
log.Info("Application object 'spec' field is not a map[string]any, using empty values", "application", client.ObjectKeyFromObject(app))
values = map[string]any{}
}
github-actions
Bot
added
do-not-merge/work-in-progress
lllamnyp
force-pushed
the
feat/jobdriver-implementation
branch
from
15ce380 to
89c72b0
Compare
github-actions
Bot
added
area/uncategorized
lllamnyp
changed the title
github-actions
Bot
added
area/storage
Complete the strategy.backups.cozystack.io/Job driver as the generic, app-agnostic counterpart to the Altinity driver: render the strategy PodTemplateSpec, run it as a one-shot batch/v1.Job, poll its terminal condition, and emit a Backup. Wire it into the existing BackupClass dispatch via reconcileJob/reconcileJobRestore rather than a standalone reconciler, matching the post-BackupClass backups API (the old draft read BackupJob.Spec.StrategyRef/StorageRef, which no longer exist). Template context follows the in-tree convention (.Application, .Release, .Mode, .Parameters, .Backup); the JobSpec.Template doc and regenerated CRD are updated to match. Restore re-renders the same template with .Mode=restore; cross-namespace restore is unsupported, mirroring Altinity. Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
lllamnyp
force-pushed
the
feat/jobdriver-implementation
branch
from
89c72b0 to
0291149
Compare
1 participant
What
Implements the
strategy.backups.cozystack.io/Jobbackup strategy — a generic, application-agnostic driver that runs a user-supplied Kubernetes Job to create (and restore) a backup.Background
This PR began as a draft written against an earlier shape of the backups API. The API has since moved to a
BackupClass-based dispatch model, which broke the original draft on three counts:BackupJob.Spec.StrategyRefandBackupJob.Spec.StorageRefwere removed; the strategy and its parameters are now resolved from aBackupClassinto aResolvedBackupConfig.BackupJobReconciler.Reconcile/RestoreJobReconciler.Reconcileresolve the strategy and switch into per-strategy methods (reconcileVelero,reconcileCNPG,reconcileAltinity, …)..Values,.Storage) diverged from the convention the other strategies adopted, and its.Storagehalf targeted a storage API that does not exist.What changed
reconcileJob/reconcileJobRestore(internal/backupcontroller/jobstrategy_controller.go): replaces the standaloneBackupJobStrategyReconcilerwith the two dispatch methods theBackupJob/RestoreJobreconcilers call. The Job strategy is the generic counterpart of the Altinity driver — same lifecycle (renderPodTemplateSpec→ run a one-shotbatch/v1.Job→ poll its terminal condition → emit aBackup), minus the ClickHouse-specificapplicationRefgate, so it runs for any application. Shared package helpers (renderJobTemplate,jobConditionState,getApplicationUnstructured, status helpers, deterministic Job naming, controller refs) are reused..Application(live application object),.Release.Name/.Release.Namespace,.Mode(backup/restore),.Parameters(resolvedBackupClassparameters), and.Backup(restore runs only). Strategy parameters round-trip through theBackup'sdriverMetadataso a later restore re-renders against the same values..Mode == "restore". Cross-namespace restore is unsupported (the RestoreJob target reference is namespace-local), matching the Altinity driver.JobSpec.Templatedoc + regenerated CRD: the field doc previously advertised.Values; it now documents the real context, andstrategy.backups.cozystack.io_jobs.yamlis regenerated to match (the change is limited to that field's description).jobstrategy_controller_test.go): backup-Job creation / args / labels / status, completion →Backupartifact with parameter round-trip, Job-failure surfacing, missing-strategy terminal failure, restore into the target namespace, restore completion, and the parameter-prefix unit. A deliberately generic application kind pins that the driver never gates onapplicationRef.kind.No RBAC changes are required: the controller already holds
strategy.backups.cozystack.io/*,batch/jobs,apps.cozystack.io/*, andbackupspermissions.Notes for reviewers
PodTemplateSpecmust branch on.Modeto do anything restore-specific. This matches the existing convention; worth confirming it's the intended contract for the Job strategy's docs/examples.🤖 Generated with Claude Code