Repository navigation
[backups] add restore jobs controller - #1811
Timofei Larkin (lllamnyp) merged 2 commits into
Conversation
dosubot
Bot
added
size/L
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
📝 WalkthroughWalkthroughAdds a RestoreJobReconciler and Velero-based restore handling (new controller, reconcile flow, Velero restore creation/polling, status updates), augments BackupJob failure handling, adds CRD annotations for RestoreJob, and extends VeleroTemplate with a RestoreSpec field. Also removes BackupJobReconciler setup from cmd/backup-controller/main.go. Changes
Sequence DiagramsequenceDiagram
autonumber
participant Manager
participant RestoreJobReconciler
participant K8sAPI
participant VeleroAPI
Manager->>RestoreJobReconciler: Reconcile request (RestoreJob event)
RestoreJobReconciler->>K8sAPI: GET RestoreJob
alt not found or completed
RestoreJobReconciler-->>Manager: Exit (no-op)
else pending
RestoreJobReconciler->>K8sAPI: GET referenced Backup & StrategyRef
RestoreJobReconciler->>RestoreJobReconciler: Validate StrategyRef.APIGroup
alt StrategyKind == Job
RestoreJobReconciler->>K8sAPI: Call Job strategy handler (reconcileJobRestore)
K8sAPI-->>RestoreJobReconciler: Status updates
else StrategyKind == Velero
RestoreJobReconciler->>VeleroAPI: Ensure/Create Velero Restore
VeleroAPI-->>RestoreJobReconciler: Restore status (InProgress/Completed/Failed)
RestoreJobReconciler->>K8sAPI: Update RestoreJob status & emit events
else unsupported
RestoreJobReconciler->>K8sAPI: Mark RestoreJob Failed
end
RestoreJobReconciler-->>Manager: Return result (requeue/complete)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Failure to add the new IP will result in interrupted reviews. 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 Andrey Kolkov (@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 establishes the foundational framework for handling 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 (@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 the scaffolding for the RestoreJob controller and its associated strategy handlers. The changes correctly set up the new RestoreJobReconciler and define the basic reconciliation loop, dispatching to strategy-specific implementations. The initial implementations for the Velero and Job strategies are included as placeholders. My review includes suggestions to improve code organization by moving generic helper functions to a more appropriate location and to enhance the readability of the controller setup logic.
| var err error | ||
| if r.Interface, err = dynamic.NewForConfig(cfg); err != nil { | ||
| return err | ||
| } | ||
| var h *http.Client | ||
| if h, err = rest.HTTPClientFor(cfg); err != nil { | ||
| return err | ||
| } | ||
| if r.RESTMapper, err = apiutil.NewDynamicRESTMapper(cfg, h); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
The initialization logic in SetupWithManager is valid, but the use of multi-value assignments inside if statement initializers can be hard to read. For better clarity and to follow more common Go idioms, consider separating the assignment from the error check using short variable declarations (:=).
dynamicClient, err := dynamic.NewForConfig(cfg)
if err != nil {
return err
}
r.Interface = dynamicClient
h, err := rest.HTTPClientFor(cfg)
if err != nil {
return err
}
mapper, err := apiutil.NewDynamicRESTMapper(cfg, h)
if err != nil {
return err
}
r.RESTMapper = mapper![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @internal/backupcontroller/restorejob_controller.go:
- Around line 88-100: The default case for unsupported
backup.Spec.StrategyRef.Kind must mark the RestoreJob as failed instead of
silently returning: set restoreJob.Status.Phase (or appropriate status field) to
a failure state and populate a status message indicating the unsupported kind,
call r.Status().Update(ctx, restoreJob) (handle/update errors), log the failure,
and then return without requeueing; update the default branch where
reconcileJobRestore/reconcileVeleroRestore are chosen (the switch handling
backup.Spec.StrategyRef.Kind) to perform this status update and persist it for
the restoreJob and include the unsupported kind and supported kinds in the
message.
- Around line 74-85: When detecting an invalid Backup StrategyRef
(backup.Spec.StrategyRef.APIGroup == nil or *backup.Spec.StrategyRef.APIGroup !=
strategyv1alpha1.GroupVersion.Group) don't just skip: mark the related
RestoreJob as failed, set a clear status/condition and message, persist the
status update, emit an event, and log the error. Concretely, in the same places
where you currently log+return, set restoreJob.Status.Phase (or appropriate
condition) to "Failed" with a message describing the invalid StrategyRef
(include the actual value or "nil"), call r.Client.Status().Update(ctx,
restoreJob) (or the existing status update helper), record an event via
r.recorder.Event(restoreJob, "Warning", "InvalidBackup", <message>), log the
error, and then return ctrl.Result{}, nil.
🧹 Nitpick comments (1)
cmd/backup-controller/main.go (1)
171-178: Consider using a distinct EventRecorder name.The EventRecorder uses
"backup-controller", which is the same name as the BackupJobReconciler. While this works, using"restore-controller"would improve observability and make it easier to filter events by controller type.🔎 Proposed improvement
if err = (&backupcontroller.RestoreJobReconciler{ Client: mgr.GetClient(), Scheme: mgr.GetScheme(), - Recorder: mgr.GetEventRecorderFor("backup-controller"), + Recorder: mgr.GetEventRecorderFor("restore-controller"), }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "RestoreJob") os.Exit(1) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/backup-controller/main.gointernal/backupcontroller/jobstrategy_controller.gointernal/backupcontroller/restorejob_controller.gointernal/backupcontroller/velerostrategy_controller.go
🧰 Additional context used
📓 Path-based instructions (3)
cmd/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Follow controller-runtime patterns and kubebuilder style for Go code in cmd directory
Files:
cmd/backup-controller/main.go
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Do NOT manually edit vendor/ directory or auto-generated files (zz_generated.*.go)
Files:
cmd/backup-controller/main.gointernal/backupcontroller/restorejob_controller.gointernal/backupcontroller/velerostrategy_controller.gointernal/backupcontroller/jobstrategy_controller.go
internal/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Follow controller-runtime patterns and kubebuilder style for Go code in internal directory
Files:
internal/backupcontroller/restorejob_controller.gointernal/backupcontroller/velerostrategy_controller.gointernal/backupcontroller/jobstrategy_controller.go
🧬 Code graph analysis (2)
cmd/backup-controller/main.go (2)
internal/backupcontroller/restorejob_controller.go (1)
RestoreJobReconciler(32-38)pkg/apiserver/apiserver.go (1)
Scheme(53-53)
internal/backupcontroller/jobstrategy_controller.go (3)
internal/backupcontroller/restorejob_controller.go (1)
RestoreJobReconciler(32-38)api/backups/v1alpha1/restorejob_types.go (1)
RestoreJob(76-82)api/backups/v1alpha1/backup_types.go (1)
Backup(103-109)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (7)
internal/backupcontroller/jobstrategy_controller.go (1)
17-20: LGTM! Appropriate scaffolding for Job strategy restore.This no-op placeholder correctly mirrors the
reconcileJobpattern and aligns with the PR objective to define controller locations without full implementation.internal/backupcontroller/restorejob_controller.go (3)
40-60: LGTM! Proper RestoreJob fetch and completion check.The implementation correctly handles not-found scenarios and short-circuits for already-completed jobs, following controller-runtime best practices.
62-71: LGTM! Appropriate Backup fetch and error handling.The code correctly fails the RestoreJob when the referenced Backup is not found, while propagating other errors for retry.
104-120: LGTM! Standard controller-runtime setup.The SetupWithManager method correctly initializes the dynamic client and REST mapper, following controller-runtime patterns consistent with the existing BackupJobReconciler.
internal/backupcontroller/velerostrategy_controller.go (3)
629-655: LGTM! Appropriate Velero restore scaffolding.The method correctly initializes RestoreJob status on first reconcile and includes clear TODO markers for the pending implementation, aligning with the PR's scaffolding objectives.
657-665: LGTM! Correct target application reference resolution.The helper properly implements the fallback behavior documented in DESIGN.md, preferring the RestoreJob's target when specified and falling back to the Backup's application reference.
667-690: LGTM! Proper RestoreJob failure handling.The status update correctly sets the failure state with appropriate conditions and timestamps, mirroring the
markBackupJobFailedpattern established for BackupJob resources.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/backups/v1alpha1/restorejob_types.go (1)
74-75: LGTM! Essential CRD markers added correctly.The status subresource marker is essential for the controller to manage RestoreJob lifecycle phases independently from the spec. The Phase printcolumn provides useful visibility when listing RestoreJobs via kubectl. Both markers follow kubebuilder conventions correctly.
Optional: Consider additional printcolumns for enhanced UX
You could add printcolumns for
StartedAtandMessage(at priority=1 so they're shown with-o wide) to provide more context when debugging restore operations:// +kubebuilder:printcolumn:name="Phase",type="string",JSONPath=".status.phase",priority=0 // +kubebuilder:printcolumn:name="Started",type="date",JSONPath=".status.startedAt",priority=1 // +kubebuilder:printcolumn:name="Message",type="string",JSONPath=".status.message",priority=1This is completely optional and can be deferred.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/backups/v1alpha1/restorejob_types.go
🧰 Additional context used
📓 Path-based instructions (2)
api/**
📄 CodeRabbit inference engine (AGENTS.md)
Define Kubernetes CRDs in the api directory following kubebuilder conventions
Files:
api/backups/v1alpha1/restorejob_types.go
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Do NOT manually edit vendor/ directory or auto-generated files (zz_generated.*.go)
Files:
api/backups/v1alpha1/restorejob_types.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI Agents
In @internal/backupcontroller/restorejob_controller.go:
- Around line 71-82: The code currently returns early when
Backup.Spec.StrategyRef.APIGroup is nil or mismatched, which leaves the
corresponding RestoreJob pending; instead, update the RestoreJob status to a
failed state with a clear reason and message, persist that status (e.g., via
r.Status().Update(ctx, restoreJob)), and then return without retrying.
Specifically, in the Reconcile method (RestoreJobReconciler) where you check
backup.Spec.StrategyRef.APIGroup and compare to
strategyv1alpha1.GroupVersion.Group, replace the silent logger.Info + return
with logic that sets the restoreJob status/phase to Failed, populates a
FailureReason/FailureMessage or Condition explaining the nil/mismatched APIGroup
(include the expected and actual values), call Status().Update to persist, and
then return ctrl.Result{} , nil.
In @internal/backupcontroller/velerostrategy_controller.go:
- Around line 719-734: The code dereferences targetAppRef.APIGroup without
checking for nil in the block using r.RESTMapping; update the logic in the
getTargetApplicationRef/restore handling to safely handle a nil APIGroup by
computing a local group string (use empty string when targetAppRef.APIGroup ==
nil) and pass that into schema.GroupKind{Group: group, Kind: targetAppRef.Kind}
when calling r.RESTMapping; keep the rest of the flow (ns determination and
r.Resource(...).Get(...)) unchanged but ensure you never directly dereference
targetAppRef.APIGroup.
🧹 Nitpick comments (4)
api/backups/strategy/v1alpha1/velero_types.go (1)
57-60: Consider makingRestoreSpecoptional for backward compatibility.The
RestoreSpecfield lacksomitemptyin its json tag, making it a required field. If existingVelerostrategy resources don't includerestoreSpec, they will fail validation after this change. Consider addingomitemptyif backward compatibility is needed.🔎 Proposed fix
type VeleroTemplate struct { - Spec velerov1.BackupSpec `json:"spec"` - RestoreSpec velerov1.RestoreSpec `json:"restoreSpec"` + Spec velerov1.BackupSpec `json:"spec"` + RestoreSpec velerov1.RestoreSpec `json:"restoreSpec,omitempty"` }internal/backupcontroller/backupjob_controller.go (1)
104-110: Usemeta.SetStatusConditioninstead of appending conditions directly.Appending conditions directly can lead to duplicate entries if
markBackupJobFailedis called multiple times. Usemeta.SetStatusConditionfromk8s.io/apimachinery/pkg/api/metato properly manage condition transitions and avoid duplicates.🔎 Proposed fix
- // Add condition - backupJob.Status.Conditions = append(backupJob.Status.Conditions, metav1.Condition{ + // Set condition (handles duplicates and transitions properly) + meta.SetStatusCondition(&backupJob.Status.Conditions, metav1.Condition{ Type: "Ready", Status: metav1.ConditionFalse, Reason: "BackupFailed", Message: message, LastTransitionTime: now, })internal/backupcontroller/restorejob_controller.go (1)
138-144: Usemeta.SetStatusConditioninstead of appending conditions directly.Same issue as in
markBackupJobFailed: appending conditions directly can lead to duplicate entries. Usemeta.SetStatusConditionfor proper condition management.🔎 Proposed fix
- // Add condition - restoreJob.Status.Conditions = append(restoreJob.Status.Conditions, metav1.Condition{ + // Set condition (handles duplicates and transitions properly) + meta.SetStatusCondition(&restoreJob.Status.Conditions, metav1.Condition{ Type: "Ready", Status: metav1.ConditionFalse, Reason: "RestoreFailed", Message: message, LastTransitionTime: now, })internal/backupcontroller/velerostrategy_controller.go (1)
745-755: Verify owner reference handling for cross-namespace resources.The Velero Restore is created in
veleroNamespacewhile the RestoreJob exists in a different namespace. Kubernetes owner references only work within the same namespace. The current approach uses labels instead, which is correct, but consider documenting this design decision or adding a comment explaining why owner references aren't used.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/backups/strategy/v1alpha1/velero_types.gointernal/backupcontroller/backupjob_controller.gointernal/backupcontroller/restorejob_controller.gointernal/backupcontroller/velerostrategy_controller.go
🧰 Additional context used
📓 Path-based instructions (3)
internal/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Follow controller-runtime patterns and kubebuilder style for Go code in internal directory
Files:
internal/backupcontroller/backupjob_controller.gointernal/backupcontroller/restorejob_controller.gointernal/backupcontroller/velerostrategy_controller.go
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Do NOT manually edit vendor/ directory or auto-generated files (zz_generated.*.go)
Files:
internal/backupcontroller/backupjob_controller.goapi/backups/strategy/v1alpha1/velero_types.gointernal/backupcontroller/restorejob_controller.gointernal/backupcontroller/velerostrategy_controller.go
api/**
📄 CodeRabbit inference engine (AGENTS.md)
Define Kubernetes CRDs in the api directory following kubebuilder conventions
Files:
api/backups/strategy/v1alpha1/velero_types.go
🧬 Code graph analysis (3)
internal/backupcontroller/backupjob_controller.go (1)
api/backups/v1alpha1/backupjob_types.go (2)
BackupJob(101-107)BackupJobPhaseFailed(37-37)
internal/backupcontroller/restorejob_controller.go (5)
api/backups/v1alpha1/restorejob_types.go (3)
RestoreJob(78-84)RestoreJobPhaseSucceeded(31-31)RestoreJobPhaseFailed(32-32)api/backups/v1alpha1/backup_types.go (1)
Backup(103-109)api/backups/strategy/v1alpha1/job_types.go (1)
JobStrategyKind(25-25)api/backups/strategy/v1alpha1/velero_types.go (1)
VeleroStrategyKind(25-25)internal/controller/dashboard/manager.go (1)
Manager(45-48)
internal/backupcontroller/velerostrategy_controller.go (4)
internal/backupcontroller/restorejob_controller.go (1)
RestoreJobReconciler(29-35)api/backups/v1alpha1/restorejob_types.go (2)
RestoreJob(78-84)RestoreJobPhaseRunning(30-30)api/backups/v1alpha1/backupjob_types.go (1)
OwningJobNameLabel(25-25)internal/template/template.go (1)
Template(9-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (4)
internal/backupcontroller/restorejob_controller.go (1)
119-127: LGTM!The helper correctly implements the design spec: if
targetApplicationRefis omitted, it falls back to the backup'sapplicationRef. Clean and well-documented.internal/backupcontroller/velerostrategy_controller.go (3)
66-74: LGTM!Good use of constants for requeue intervals and metadata keys. This reduces duplication and potential typos.
607-623: LGTM!The restore initialization correctly sets
StartedAtandPhasetoRunningatomically on first reconcile, then requeues. This follows the same pattern asreconcileVelerofor backups.
673-676: Good error handling for multiple Velero Restores.Unlike the backup path (lines 166-173) which only sets the phase without a message, this correctly uses
markRestoreJobFailedwith a descriptive message. Consider applying the same improvement to the backup path for consistency.
| // Determine target application reference | ||
| targetAppRef := r.getTargetApplicationRef(restoreJob, backup) | ||
|
|
||
| // Get the target application object for templating | ||
| mapping, err := r.RESTMapping(schema.GroupKind{Group: *targetAppRef.APIGroup, Kind: targetAppRef.Kind}) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get REST mapping for target application: %w", err) | ||
| } | ||
| ns := restoreJob.Namespace | ||
| if mapping.Scope.Name() != meta.RESTScopeNameNamespace { | ||
| ns = "" | ||
| } | ||
| app, err := r.Resource(mapping.Resource).Namespace(ns).Get(ctx, targetAppRef.Name, metav1.GetOptions{}) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get target application: %w", err) | ||
| } |
There was a problem hiding this comment.
Potential nil pointer dereference on targetAppRef.APIGroup.
The getTargetApplicationRef returns a corev1.TypedLocalObjectReference, and APIGroup is a pointer that could be nil. Line 723 dereferences it without a nil check, which would cause a panic.
🔎 Proposed fix
// Determine target application reference
targetAppRef := r.getTargetApplicationRef(restoreJob, backup)
+ if targetAppRef.APIGroup == nil {
+ return fmt.Errorf("target application APIGroup is nil")
+ }
+
// Get the target application object for templating
mapping, err := r.RESTMapping(schema.GroupKind{Group: *targetAppRef.APIGroup, Kind: targetAppRef.Kind})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Determine target application reference | |
| targetAppRef := r.getTargetApplicationRef(restoreJob, backup) | |
| // Get the target application object for templating | |
| mapping, err := r.RESTMapping(schema.GroupKind{Group: *targetAppRef.APIGroup, Kind: targetAppRef.Kind}) | |
| if err != nil { | |
| return fmt.Errorf("failed to get REST mapping for target application: %w", err) | |
| } | |
| ns := restoreJob.Namespace | |
| if mapping.Scope.Name() != meta.RESTScopeNameNamespace { | |
| ns = "" | |
| } | |
| app, err := r.Resource(mapping.Resource).Namespace(ns).Get(ctx, targetAppRef.Name, metav1.GetOptions{}) | |
| if err != nil { | |
| return fmt.Errorf("failed to get target application: %w", err) | |
| } | |
| // Determine target application reference | |
| targetAppRef := r.getTargetApplicationRef(restoreJob, backup) | |
| if targetAppRef.APIGroup == nil { | |
| return fmt.Errorf("target application APIGroup is nil") | |
| } | |
| // Get the target application object for templating | |
| mapping, err := r.RESTMapping(schema.GroupKind{Group: *targetAppRef.APIGroup, Kind: targetAppRef.Kind}) | |
| if err != nil { | |
| return fmt.Errorf("failed to get REST mapping for target application: %w", err) | |
| } | |
| ns := restoreJob.Namespace | |
| if mapping.Scope.Name() != meta.RESTScopeNameNamespace { | |
| ns = "" | |
| } | |
| app, err := r.Resource(mapping.Resource).Namespace(ns).Get(ctx, targetAppRef.Name, metav1.GetOptions{}) | |
| if err != nil { | |
| return fmt.Errorf("failed to get target application: %w", err) | |
| } |
🤖 Prompt for AI Agents
In @internal/backupcontroller/velerostrategy_controller.go around lines 719 -
734, The code dereferences targetAppRef.APIGroup without checking for nil in the
block using r.RESTMapping; update the logic in the
getTargetApplicationRef/restore handling to safely handle a nil APIGroup by
computing a local group string (use empty string when targetAppRef.APIGroup ==
nil) and pass that into schema.GroupKind{Group: group, Kind: targetAppRef.Kind}
when calling r.RESTMapping; keep the rest of the flow (ns determination and
r.Resource(...).Get(...)) unchanged but ensure you never directly dereference
targetAppRef.APIGroup.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @internal/backupcontroller/backupjob_controller.go:
- Line 108: The call to logger.Debug("BackupJob failed", "message", message)
uses a nonexistent logr method; replace it with the standard logr debug pattern
used elsewhere by calling logger.V(1).Info("BackupJob failed", "message",
message) so the debug-level message is emitted correctly; update the BackupJob
controller logging site (the statement using logger.Debug) to use
logger.V(1).Info with the same message and key/value pairs.
- Around line 96-102: Replace the direct append to backupJob.Status.Conditions
with an idempotent update using meta.SetStatusCondition: construct the
metav1.Condition with Type "Ready", Status metav1.ConditionFalse, Reason
"BackupFailed", Message message and LastTransitionTime now, then call
meta.SetStatusCondition(&backupJob.Status.Conditions, yourCondition) so
duplicate "Ready" conditions are avoided and reconcilers remain idempotent.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/backupcontroller/backupjob_controller.gointernal/backupcontroller/restorejob_controller.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/backupcontroller/restorejob_controller.go
🧰 Additional context used
📓 Path-based instructions (2)
internal/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Follow controller-runtime patterns and kubebuilder style for Go code in internal directory
Files:
internal/backupcontroller/backupjob_controller.go
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Do NOT manually edit vendor/ directory or auto-generated files (zz_generated.*.go)
Files:
internal/backupcontroller/backupjob_controller.go
🧬 Code graph analysis (1)
internal/backupcontroller/backupjob_controller.go (2)
api/backups/v1alpha1/groupversion_info.go (1)
GroupVersion(34-34)api/backups/v1alpha1/backupjob_types.go (2)
BackupJob(101-107)BackupJobPhaseFailed(37-37)
🔇 Additional comments (3)
internal/backupcontroller/backupjob_controller.go (3)
5-10: LGTM!The new imports are correctly added and necessary for the failure handling logic.
50-66: LGTM!Good improvement to explicitly mark jobs as failed with descriptive messages instead of silently skipping. This provides better visibility into configuration errors for users.
89-89: Logger retrieval pattern is intentional.
getLogger(ctx)is defined in the same package (internal/backupcontroller/velerostrategy_controller.go) and returns aloggerWithDebugwrapper that augments the standardlogr.Loggerwith aDebug()convenience method. The different logger retrieval methods betweenReconcileandmarkBackupJobFailedare a deliberate pattern—Reconcileuses the standard logger directly while the helper uses the wrapper to access theDebug()method. This is not an issue.
| backupJob.Status.Conditions = append(backupJob.Status.Conditions, metav1.Condition{ | ||
| Type: "Ready", | ||
| Status: metav1.ConditionFalse, | ||
| Reason: "BackupFailed", | ||
| Message: message, | ||
| LastTransitionTime: now, | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use meta.SetStatusCondition to avoid duplicate conditions.
Directly appending to Conditions can result in duplicate "Ready" conditions if the reconcile loop retries after a transient status update failure. Use meta.SetStatusCondition for idempotent condition updates.
🔎 Proposed fix
- // Add condition
- backupJob.Status.Conditions = append(backupJob.Status.Conditions, metav1.Condition{
+ // Set condition (idempotent)
+ meta.SetStatusCondition(&backupJob.Status.Conditions, metav1.Condition{
Type: "Ready",
Status: metav1.ConditionFalse,
Reason: "BackupFailed",
Message: message,
LastTransitionTime: now,
})Note: The meta package from k8s.io/apimachinery/pkg/api/meta is already imported (line 9).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @internal/backupcontroller/backupjob_controller.go around lines 96 - 102,
Replace the direct append to backupJob.Status.Conditions with an idempotent
update using meta.SetStatusCondition: construct the metav1.Condition with Type
"Ready", Status metav1.ConditionFalse, Reason "BackupFailed", Message message
and LastTransitionTime now, then call
meta.SetStatusCondition(&backupJob.Status.Conditions, yourCondition) so
duplicate "Ready" conditions are avoided and reconcilers remain idempotent.
| logger.Error(err, "failed to update BackupJob status to Failed") | ||
| return ctrl.Result{}, err | ||
| } | ||
| logger.Debug("BackupJob failed", "message", message) |
There was a problem hiding this comment.
logger.Debug() is not a standard logr method.
The logr.Logger interface (used by controller-runtime) does not have a Debug() method. Use logger.V(1).Info() for debug-level logging, consistent with the pattern used elsewhere in this file (e.g., line 43).
🔎 Proposed fix
- logger.Debug("BackupJob failed", "message", message)
+ logger.V(1).Info("BackupJob marked as failed", "message", message)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logger.Debug("BackupJob failed", "message", message) | |
| logger.V(1).Info("BackupJob marked as failed", "message", message) |
🤖 Prompt for AI Agents
In @internal/backupcontroller/backupjob_controller.go at line 108, The call to
logger.Debug("BackupJob failed", "message", message) uses a nonexistent logr
method; replace it with the standard logr debug pattern used elsewhere by
calling logger.V(1).Info("BackupJob failed", "message", message) so the
debug-level message is emitted correctly; update the BackupJob controller
logging site (the statement using logger.Debug) to use logger.V(1).Info with the
same message and key/value pairs.
| } | ||
|
|
||
| // Template the restore spec from the strategy | ||
| veleroRestoreSpec, err := template.Template(&strategy.Spec.Template.RestoreSpec, app.Object) |
There was a problem hiding this comment.
This is problematic, because you presumably don't have an object to restore into (therefore you cannot read from that object). One option might be to expand the Backup object to be able to store the original app manifest, although this does feel like a stopgap solution. Maybe store this in an annotation on the Backup?
a1fa5fb to
63f8382
Compare
Signed-off-by: Andrey Kolkov <[email protected]>
63f8382 to
5e15a75
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @api/backups/strategy/v1alpha1/velero_types.go:
- Around line 58-59: The RestoreSpec field on the Velero strategy struct is
currently required; make it optional and tolerant of existing CRs by adding the
Kubernetes marker and JSON tag: add the `+optional` comment above the
RestoreSpec field and append `,omitempty` to its `json:"restoreSpec"` tag so the
field can be omitted/left out of existing CRs (target the RestoreSpec field of
type velerov1.RestoreSpec in the velero_types.go struct).
In @internal/backupcontroller/restorejob_controller.go:
- Around line 62-64: The current r.Get(ctx, backupKey, backup) error handling
always calls r.markRestoreJobFailed which treats transient API errors as
permanent failures; change it to check the error type using
apierrors.IsNotFound(err) (or equivalent) and only call r.markRestoreJobFailed
when the Backup is truly missing, while for other errors return a requeueable
error/result so the controller retries (e.g., return ctrl.Result{Requeue: true}
or return err) instead of permanently failing restoreJob; update the logic
around r.Get, backupKey, backup, and r.markRestoreJobFailed accordingly.
In @internal/backupcontroller/velerostrategy_controller.go:
- Around line 722-727: The code dereferences targetAppRef.APIGroup without a nil
check causing a potential panic; update the logic before calling r.RESTMapping
to explicitly check if targetAppRef == nil or targetAppRef.APIGroup == nil and
handle it (either return a clear error like "missing APIGroup in targetAppRef"
or set a safe default group string) so the call to
r.RESTMapping(schema.GroupKind{Group: *targetAppRef.APIGroup, Kind:
targetAppRef.Kind}) never dereferences a nil pointer; ensure the check is placed
just before the RESTMapping call and adjust the error message to reference
targetAppRef/APIGroup.
🧹 Nitpick comments (4)
internal/backupcontroller/jobstrategy_controller.go (1)
17-20: Stub implementation for Job strategy restore.This method is a no-op placeholder. If the Job strategy restore is not yet implemented, consider returning an error or marking the RestoreJob as failed with an "unsupported" message, rather than silently succeeding.
Would you like me to generate a proper placeholder that fails gracefully, or open an issue to track the Job strategy restore implementation?
internal/backupcontroller/backupjob_controller.go (1)
97-103: Conditions may accumulate on reconcile retries.Using
appendto add theReadycondition can result in duplicate conditions if the status update fails and the reconcile is retried. Consider usingmeta.SetStatusConditionfromk8s.io/apimachinery/pkg/api/metato ensure idempotent condition updates.Proposed fix using SetStatusCondition
+import ( + apimeta "k8s.io/apimachinery/pkg/api/meta" +) func (r *BackupJobReconciler) markBackupJobFailed(ctx context.Context, backupJob *backupsv1alpha1.BackupJob, message string) (ctrl.Result, error) { logger := getLogger(ctx) now := metav1.Now() backupJob.Status.CompletedAt = &now backupJob.Status.Phase = backupsv1alpha1.BackupJobPhaseFailed backupJob.Status.Message = message // Add condition - backupJob.Status.Conditions = append(backupJob.Status.Conditions, metav1.Condition{ + apimeta.SetStatusCondition(&backupJob.Status.Conditions, metav1.Condition{ Type: "Ready", Status: metav1.ConditionFalse, Reason: "BackupFailed", Message: message, LastTransitionTime: now, })internal/backupcontroller/restorejob_controller.go (1)
125-131: Conditions may accumulate on reconcile retries.Same issue as in
markBackupJobFailed- usingappendfor conditions can cause duplicates. Usemeta.SetStatusConditionfor idempotent updates.Proposed fix
+import ( + apimeta "k8s.io/apimachinery/pkg/api/meta" +) // Add condition - restoreJob.Status.Conditions = append(restoreJob.Status.Conditions, metav1.Condition{ + apimeta.SetStatusCondition(&restoreJob.Status.Conditions, metav1.Condition{ Type: "Ready", Status: metav1.ConditionFalse, Reason: "RestoreFailed", Message: message, LastTransitionTime: now, })internal/backupcontroller/velerostrategy_controller.go (1)
688-699: Consider adding a success condition.When the restore succeeds, only
PhaseandCompletedAtare updated. For consistency with the failure path (which adds aReadycondition), consider also setting aReady=Truecondition on success.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
api/backups/strategy/v1alpha1/velero_types.goapi/backups/v1alpha1/restorejob_types.gocmd/backup-controller/main.gocmd/backupstrategy-controller/main.gointernal/backupcontroller/backupjob_controller.gointernal/backupcontroller/jobstrategy_controller.gointernal/backupcontroller/restorejob_controller.gointernal/backupcontroller/velerostrategy_controller.go
💤 Files with no reviewable changes (1)
- cmd/backup-controller/main.go
🧰 Additional context used
📓 Path-based instructions (4)
internal/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Follow controller-runtime patterns and kubebuilder style for Go code in internal directory
Files:
internal/backupcontroller/restorejob_controller.gointernal/backupcontroller/backupjob_controller.gointernal/backupcontroller/velerostrategy_controller.gointernal/backupcontroller/jobstrategy_controller.go
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Do NOT manually edit vendor/ directory or auto-generated files (zz_generated.*.go)
Files:
internal/backupcontroller/restorejob_controller.goapi/backups/strategy/v1alpha1/velero_types.goapi/backups/v1alpha1/restorejob_types.gocmd/backupstrategy-controller/main.gointernal/backupcontroller/backupjob_controller.gointernal/backupcontroller/velerostrategy_controller.gointernal/backupcontroller/jobstrategy_controller.go
api/**
📄 CodeRabbit inference engine (AGENTS.md)
Define Kubernetes CRDs in the api directory following kubebuilder conventions
Files:
api/backups/strategy/v1alpha1/velero_types.goapi/backups/v1alpha1/restorejob_types.go
cmd/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Follow controller-runtime patterns and kubebuilder style for Go code in cmd directory
Files:
cmd/backupstrategy-controller/main.go
🧠 Learnings (2)
📚 Learning: 2025-12-25T09:45:26.524Z
Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T09:45:26.524Z
Learning: Applies to pkg/**/*.go : Follow controller-runtime patterns and kubebuilder style for Go code in pkg directory
Applied to files:
cmd/backupstrategy-controller/main.go
📚 Learning: 2025-12-25T09:45:26.524Z
Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T09:45:26.524Z
Learning: Applies to internal/**/*.go : Follow controller-runtime patterns and kubebuilder style for Go code in internal directory
Applied to files:
cmd/backupstrategy-controller/main.go
🧬 Code graph analysis (4)
internal/backupcontroller/restorejob_controller.go (4)
api/backups/v1alpha1/restorejob_types.go (2)
RestoreJob(78-84)RestoreJobPhaseSucceeded(31-31)api/backups/v1alpha1/backup_types.go (1)
Backup(103-109)api/backups/strategy/v1alpha1/job_types.go (1)
JobStrategyKind(25-25)api/backups/strategy/v1alpha1/velero_types.go (1)
VeleroStrategyKind(25-25)
cmd/backupstrategy-controller/main.go (2)
pkg/apiserver/apiserver.go (1)
Scheme(53-53)internal/backupcontroller/restorejob_controller.go (1)
RestoreJobReconciler(29-35)
internal/backupcontroller/velerostrategy_controller.go (2)
api/backups/v1alpha1/backupjob_types.go (2)
OwningJobNameLabel(25-25)OwningJobNamespaceLabel(26-26)internal/template/template.go (1)
Template(9-37)
internal/backupcontroller/jobstrategy_controller.go (3)
internal/backupcontroller/restorejob_controller.go (1)
RestoreJobReconciler(29-35)api/backups/v1alpha1/restorejob_types.go (1)
RestoreJob(78-84)api/backups/v1alpha1/backup_types.go (1)
Backup(103-109)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (5)
api/backups/v1alpha1/restorejob_types.go (1)
74-75: LGTM!The kubebuilder markers for status subresource and printcolumn are correctly added. The status subresource is essential for the new RestoreJobReconciler to update status independently, and the Phase printcolumn improves kubectl usability.
cmd/backupstrategy-controller/main.go (1)
154-170: LGTM!The controller setup follows controller-runtime patterns correctly. Both reconcilers are properly initialized with the event recorder, and scheme registration includes the necessary Velero types.
internal/backupcontroller/velerostrategy_controller.go (2)
66-74: LGTM on the constants.Good practice extracting the metadata keys (
veleroBackupNameMetadataKey,veleroBackupNamespaceMetadataKey) as constants. This ensures consistency between backup creation and restore lookup.
607-623: LGTM on first-reconcile initialization.The pattern of setting
StartedAtand phase toRunningon first reconcile, then requeuing, aligns well with the backup flow and ensures status is persisted before proceeding.internal/backupcontroller/restorejob_controller.go (1)
106-114: No action needed. ThegetTargetApplicationRefmethod is defined only once onRestoreJobReconcilerinrestorejob_controller.go(line 109). The method is called invelerostrategy_controller.go(line 720), but not defined there. There is no duplicate method declaration, and no compilation error would occur.Likely an incorrect or invalid review comment.
| Spec velerov1.BackupSpec `json:"spec"` | ||
| RestoreSpec velerov1.RestoreSpec `json:"restoreSpec"` |
There was a problem hiding this comment.
Consider making RestoreSpec optional for backward compatibility.
The RestoreSpec field lacks the omitempty JSON tag and +optional marker, making it a required field. Existing Velero strategy CRs that don't include restoreSpec will fail validation after this change.
If restore functionality should be opt-in or if you need to maintain backward compatibility during migration, consider:
Proposed fix to make RestoreSpec optional
type VeleroTemplate struct {
- Spec velerov1.BackupSpec `json:"spec"`
- RestoreSpec velerov1.RestoreSpec `json:"restoreSpec"`
+ Spec velerov1.BackupSpec `json:"spec"`
+ // +optional
+ RestoreSpec velerov1.RestoreSpec `json:"restoreSpec,omitempty"`
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Spec velerov1.BackupSpec `json:"spec"` | |
| RestoreSpec velerov1.RestoreSpec `json:"restoreSpec"` | |
| Spec velerov1.BackupSpec `json:"spec"` | |
| // +optional | |
| RestoreSpec velerov1.RestoreSpec `json:"restoreSpec,omitempty"` |
🤖 Prompt for AI Agents
In @api/backups/strategy/v1alpha1/velero_types.go around lines 58 - 59, The
RestoreSpec field on the Velero strategy struct is currently required; make it
optional and tolerant of existing CRs by adding the Kubernetes marker and JSON
tag: add the `+optional` comment above the RestoreSpec field and append
`,omitempty` to its `json:"restoreSpec"` tag so the field can be omitted/left
out of existing CRs (target the RestoreSpec field of type velerov1.RestoreSpec
in the velero_types.go struct).
| if err := r.Get(ctx, backupKey, backup); err != nil { | ||
| return r.markRestoreJobFailed(ctx, restoreJob, fmt.Sprintf("failed to get Backup: %v", err)) | ||
| } |
There was a problem hiding this comment.
Transient errors should not permanently fail the RestoreJob.
If fetching the Backup fails due to a transient error (e.g., network issue, API server unavailable), the RestoreJob is permanently marked as failed. Consider distinguishing between NotFound (permanent failure) and other errors (requeue for retry).
Proposed fix to handle transient errors
backup := &backupsv1alpha1.Backup{}
backupKey := types.NamespacedName{Namespace: req.Namespace, Name: restoreJob.Spec.BackupRef.Name}
if err := r.Get(ctx, backupKey, backup); err != nil {
+ if apierrors.IsNotFound(err) {
+ return r.markRestoreJobFailed(ctx, restoreJob, fmt.Sprintf("Backup not found: %s", restoreJob.Spec.BackupRef.Name))
+ }
+ // Transient error - requeue for retry
+ logger.Error(err, "failed to get Backup, will retry")
- return r.markRestoreJobFailed(ctx, restoreJob, fmt.Sprintf("failed to get Backup: %v", err))
+ return ctrl.Result{}, err
}🤖 Prompt for AI Agents
In @internal/backupcontroller/restorejob_controller.go around lines 62 - 64, The
current r.Get(ctx, backupKey, backup) error handling always calls
r.markRestoreJobFailed which treats transient API errors as permanent failures;
change it to check the error type using apierrors.IsNotFound(err) (or
equivalent) and only call r.markRestoreJobFailed when the Backup is truly
missing, while for other errors return a requeueable error/result so the
controller retries (e.g., return ctrl.Result{Requeue: true} or return err)
instead of permanently failing restoreJob; update the logic around r.Get,
backupKey, backup, and r.markRestoreJobFailed accordingly.
| // Get the target application object for templating | ||
| mapping, err := r.RESTMapping(schema.GroupKind{Group: *targetAppRef.APIGroup, Kind: targetAppRef.Kind}) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get REST mapping for target application: %w", err) | ||
| } | ||
| ns := restoreJob.Namespace |
There was a problem hiding this comment.
Potential nil pointer dereference on APIGroup.
targetAppRef.APIGroup is dereferenced without a nil check on line 723. If the TypedLocalObjectReference has a nil APIGroup, this will panic.
Proposed fix to add nil check
// Get the target application object for templating
+ if targetAppRef.APIGroup == nil {
+ return fmt.Errorf("target application reference has nil APIGroup")
+ }
mapping, err := r.RESTMapping(schema.GroupKind{Group: *targetAppRef.APIGroup, Kind: targetAppRef.Kind})
if err != nil {
return fmt.Errorf("failed to get REST mapping for target application: %w", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Get the target application object for templating | |
| mapping, err := r.RESTMapping(schema.GroupKind{Group: *targetAppRef.APIGroup, Kind: targetAppRef.Kind}) | |
| if err != nil { | |
| return fmt.Errorf("failed to get REST mapping for target application: %w", err) | |
| } | |
| ns := restoreJob.Namespace | |
| // Get the target application object for templating | |
| if targetAppRef.APIGroup == nil { | |
| return fmt.Errorf("target application reference has nil APIGroup") | |
| } | |
| mapping, err := r.RESTMapping(schema.GroupKind{Group: *targetAppRef.APIGroup, Kind: targetAppRef.Kind}) | |
| if err != nil { | |
| return fmt.Errorf("failed to get REST mapping for target application: %w", err) | |
| } | |
| ns := restoreJob.Namespace |
🤖 Prompt for AI Agents
In @internal/backupcontroller/velerostrategy_controller.go around lines 722 -
727, The code dereferences targetAppRef.APIGroup without a nil check causing a
potential panic; update the logic before calling r.RESTMapping to explicitly
check if targetAppRef == nil or targetAppRef.APIGroup == nil and handle it
(either return a clear error like "missing APIGroup in targetAppRef" or set a
safe default group string) so the call to r.RESTMapping(schema.GroupKind{Group:
*targetAppRef.APIGroup, Kind: targetAppRef.Kind}) never dereferences a nil
pointer; ensure the check is placed just before the RESTMapping call and adjust
the error message to reference targetAppRef/APIGroup.
Signed-off-by: Timofei Larkin <[email protected]>
c2a5572
into
feat/backup-work-consolidation
2 participants
What this PR does
Release note
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
Documentation / CRD
✏️ Tip: You can customize this high-level summary in your review settings.