[backups] Scaffold a backup strategy API group #1687
Conversation
|
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. WalkthroughA new v1alpha1 group version registration file is introduced for the backups strategy API. The file defines the GroupVersion identifier, initializes SchemeBuilder, and registers the group version with Kubernetes runtime through standard kubebuilder scaffolding patterns. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 introduces a new, extensible backup strategy within the Cozystack ecosystem. It defines a 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new backup strategy based on one-shot Kubernetes Jobs, by adding the Job.strategy.backups.cozystack.io/v1alpha1 CRD and a corresponding controller BackupJobStrategyReconciler. The changes also make BackupJob resources selectable by strategy reference fields and add a status subresource to the Plan CRD.
The overall structure is good and follows standard practices for extending Kubernetes with custom controllers. However, I've found a critical bug in the new controller that would prevent it from working correctly. I've also noted that the controller logic is currently a stub and provided a suggestion to improve a comment. It's also worth noting that for the new controller to function, it needs to be registered with the manager in main.go and its scheme must be added, which seems to be missing from this pull request.
| if j.Spec.StrategyRef.APIGroup != &strategyv1alpha1.GroupVersion.Group { | ||
| return ctrl.Result{}, nil | ||
| } |
There was a problem hiding this comment.
This check for the APIGroup compares pointers instead of string values. j.Spec.StrategyRef.APIGroup is a pointer to a string from the deserialized Kubernetes object, while &strategyv1alpha1.GroupVersion.Group is a pointer to a package-level variable. These pointers will almost never be equal, even if the strings they point to have the same value. This will cause the controller to ignore all BackupJobs it's supposed to handle. You should dereference the pointer to compare the string values, and also handle the case where j.Spec.StrategyRef.APIGroup is nil.
| if j.Spec.StrategyRef.APIGroup != &strategyv1alpha1.GroupVersion.Group { | |
| return ctrl.Result{}, nil | |
| } | |
| if j.Spec.StrategyRef.APIGroup == nil || *j.Spec.StrategyRef.APIGroup != strategyv1alpha1.GroupVersion.Group { | |
| return ctrl.Result{}, nil | |
| } |
| backupsv1alpha1 "github.com/cozystack/cozystack/api/backups/v1alpha1" | ||
| ) | ||
|
|
||
| // JobStrategyReconciler reconiles |
There was a problem hiding this comment.
There's a typo in this comment (reconiles instead of reconciles). The comment also refers to JobStrategyReconciler, but the struct is named BackupJobStrategyReconciler. It's good practice to keep comments accurate and consistent with the code.
| // JobStrategyReconciler reconiles | |
| // BackupJobStrategyReconciler reconciles BackupJob objects that use the Job strategy. |
| return ctrl.Result{}, nil | ||
| } | ||
|
|
||
| return ctrl.Result{}, nil |
There was a problem hiding this comment.
The reconcile logic for this strategy is currently a stub. While this PR sets up the controller structure, the Reconcile function doesn't perform any actions after identifying a relevant BackupJob. For this to be a useful reference implementation as mentioned in the PR description, you should consider adding the logic to actually create and manage the underlying backup Job.
fc9f355 to
8928552
Compare
666d8f0 to
2320543
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
internal/unstructured/client.go (1)
17-31: Consider returning the exportedClientinterface instead of the unexported*clienttype.The function returns
*client(unexported), but callers in other packages (likewebhook.go) access the embeddedInterfaceandRESTMapperfields directly. While this works due to Go's embedded field promotion, returning theClientinterface would be more idiomatic and make the API contract clearer.-func NewForConfig(cfg *rest.Config) (*client, error) { +func NewForConfig(cfg *rest.Config) (Client, error) { c := &client{} var err error if c.Interface, err = dynamic.NewForConfig(cfg); err != nil { return nil, err } var h *http.Client if h, err = rest.HTTPClientFor(cfg); err != nil { return nil, err } if c.RESTMapper, err = apiutil.NewDynamicRESTMapper(cfg, h); err != nil { return nil, err } return c, nil }Note: If you change the return type, callers accessing
.Interfaceand.RESTMapperdirectly will need to be updated to use type assertion or the interface methods.internal/template/template_test.go (2)
55-64: Minor: Log statements before error checks.The
t.Logfcalls on lines 56 and 61 execute before verifying the marshal succeeded. Whilet.Fatalfstops execution immediately after, reordering would be slightly cleaner:b1, err := json.Marshal(reference) - t.Logf("reference:\n%s", string(b1)) if err != nil { t.Fatalf("failed to marshal reference value: %v", err) } + t.Logf("reference:\n%s", string(b1)) b2, err := json.Marshal(got) - t.Logf("got:\n%s", string(b2)) if err != nil { t.Fatalf("failed to marshal transformed value: %v", err) } + t.Logf("got:\n%s", string(b2))
11-68: Consider adding edge case tests.The test covers the happy path well. For robustness, consider adding tests for:
- Missing template variables (e.g.,
{{ .Missing.Key }})- Malformed template syntax
- Strings without template markers (passthrough)
internal/template/template.go (1)
19-25: Silent error handling may hide template failures.When template parsing or execution fails, the original string is returned without any indication of failure. This could lead to subtle bugs where misconfigured templates silently pass through unchanged.
Consider either:
- Logging template errors for debugging
- Accumulating errors and returning them from the outer
Templatefunction- At minimum, documenting this behavior
+import "log/slog" + templateFunc := func(in string) string { out, err := template(in, templateContext) if err != nil { + slog.Debug("template substitution failed", "input", in, "error", err) return in } return out }internal/backupcontroller/jobstrategy_controller.go (1)
74-104: Consider adding watches for referenced resources.The TODO comments (lines 76-78, 85-87, 102) correctly identify that not-found errors shouldn't use exponential backoff. A better approach is to add watches for the referenced Application, Strategy, and Storage resources in SetupWithManager, triggering reconciliation when they become available.
Example pattern in SetupWithManager:
return ctrl.NewControllerManagedBy(mgr). For(&backupsv1alpha1.BackupJob{}). Owns(&batchv1.Job{}). Watches( &strategyv1alpha1.Job{}, handler.EnqueueRequestsFromMapFunc(r.findBackupJobsForStrategy), ). Complete(r)With a corresponding mapper function to find BackupJobs referencing a given Strategy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
api/backups/strategy/v1alpha1/groupversion_info.go(1 hunks)api/backups/strategy/v1alpha1/job_types.go(1 hunks)api/backups/strategy/v1alpha1/zz_generated.deepcopy.go(1 hunks)api/backups/v1alpha1/backupjob_types.go(1 hunks)api/backups/v1alpha1/zz_generated.deepcopy.go(2 hunks)internal/backupcontroller/jobstrategy_controller.go(1 hunks)internal/lineagecontrollerwebhook/webhook.go(2 hunks)internal/template/template.go(1 hunks)internal/template/template_test.go(1 hunks)internal/unstructured/client.go(1 hunks)packages/system/backup-controller/definitions/backups.cozystack.io_backupjobs.yaml(1 hunks)packages/system/backup-controller/definitions/backups.cozystack.io_plans.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use Controller-runtime patterns and kubebuilder style for Go code
Files:
api/backups/v1alpha1/backupjob_types.goapi/backups/strategy/v1alpha1/zz_generated.deepcopy.gointernal/template/template_test.gointernal/lineagecontrollerwebhook/webhook.goapi/backups/v1alpha1/zz_generated.deepcopy.gointernal/template/template.goapi/backups/strategy/v1alpha1/groupversion_info.goapi/backups/strategy/v1alpha1/job_types.gointernal/unstructured/client.gointernal/backupcontroller/jobstrategy_controller.go
**/zz_generated.*.go
📄 CodeRabbit inference engine (AGENTS.md)
Do not edit auto-generated Go files matching pattern
zz_generated.*.go
Files:
api/backups/strategy/v1alpha1/zz_generated.deepcopy.goapi/backups/v1alpha1/zz_generated.deepcopy.go
🧠 Learnings (1)
📚 Learning: 2025-11-27T11:21:45.808Z
Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T11:21:45.808Z
Learning: Applies to **/*.go : Use Controller-runtime patterns and kubebuilder style for Go code
Applied to files:
internal/lineagecontrollerwebhook/webhook.go
🧬 Code graph analysis (6)
api/backups/strategy/v1alpha1/zz_generated.deepcopy.go (1)
api/backups/strategy/v1alpha1/job_types.go (4)
Job(22-28)JobList(33-37)JobSpec(40-48)JobStatus(50-52)
internal/template/template_test.go (1)
internal/template/template.go (1)
Template(9-37)
internal/lineagecontrollerwebhook/webhook.go (1)
internal/unstructured/client.go (1)
NewForConfig(17-31)
api/backups/v1alpha1/zz_generated.deepcopy.go (1)
api/backups/v1alpha1/plan_types.go (1)
PlanStatus(80-82)
api/backups/strategy/v1alpha1/groupversion_info.go (2)
pkg/apiserver/apiserver.go (1)
Scheme(53-53)api/backups/strategy/v1alpha1/job_types.go (2)
Job(22-28)JobList(33-37)
api/backups/strategy/v1alpha1/job_types.go (1)
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 (12)
api/backups/v1alpha1/zz_generated.deepcopy.go (1)
1-501: Auto-generated file: no manual edits required.This file is generated by controller-gen and matches the
zz_generated.*.gopattern specified in the coding guidelines. The added deepcopy methods forPlanStatus(lines 371-391) and the updatedPlan.DeepCopyInto(line 284) correctly follow controller-runtime patterns for types containing slices ofmetav1.Condition.internal/lineagecontrollerwebhook/webhook.go (1)
52-57: Clean refactor to unified client initialization.The change properly uses the new
cozyunstructured.NewForConfighelper, which encapsulates dynamic client and REST mapper creation. Usingrest.CopyConfigis correct to avoid mutating the manager's shared configuration.packages/system/backup-controller/definitions/backups.cozystack.io_plans.yaml (1)
133-196: Well-structured status conditions schema.The
status.conditionsarray follows standard Kubernetes API conventions with proper validation patterns and required fields. The status subresource enablement at the end is correct for allowing separate status updates.packages/system/backup-controller/definitions/backups.cozystack.io_backupjobs.yaml (1)
230-233: Generated selectableFields match the type annotations.The CRD correctly reflects the
+kubebuilder:selectablefieldmarkers frombackupjob_types.go, enabling efficient field-based selection of BackupJobs by strategy reference.api/backups/strategy/v1alpha1/zz_generated.deepcopy.go (1)
1-123: Auto-generated file - no review needed.This file is generated by
controller-genand should not be manually edited. The generated deepcopy methods correctly handle all the new backup strategy types.internal/template/template.go (2)
39-56: Clean recursive string walker implementation.The
mapAtStringsfunction correctly handles the JSON-unmarshaled types (map[string]any,[]any,string) and passes through other types unchanged. This is appropriate for walking the JSON structure.
58-68: Template parsing on every call.Each string is parsed as a new template. For strings without template markers, this is overhead. However, since this is used for backup job creation (not a hot path), the simplicity is reasonable.
api/backups/strategy/v1alpha1/groupversion_info.go (1)
28-47: LGTM! Standard API group registration.The GroupVersion registration, SchemeBuilder setup, and addKnownTypes implementation follow controller-runtime and kubebuilder conventions correctly. The registration of both Job and JobList types with metav1.AddToGroupVersion is appropriate for the new v1alpha1 API surface.
api/backups/strategy/v1alpha1/job_types.go (1)
13-52: LGTM! Well-structured CRD definitions.The Job and JobList types follow standard Kubernetes API conventions:
- Appropriate kubebuilder markers for cluster-scoped resources with status subresource
- JobSpec.Template using PodTemplateSpec provides the right abstraction for defining backup job pods
- Clear documentation of template support with available context variables
- JobStatus with Conditions follows the standard Kubernetes status pattern
internal/backupcontroller/jobstrategy_controller.go (3)
54-72: LGTM! Pointer dereferencing is now correct.The APIGroup extraction (lines 54-65) safely dereferences the pointers, and the subsequent comparisons (lines 67, 70) correctly compare string values rather than pointers. This resolves the critical bug mentioned in previous reviews.
157-173: LGTM! Proper handling of namespace-scoped vs cluster-scoped resources.The getUnstructured helper correctly uses RESTMapping to determine resource scope (line 164) and conditionally clears the namespace for cluster-scoped resources (line 165), ensuring proper resource lookup regardless of scope.
110-121: No action needed. Kubernetes Kind values are always alphanumeric PascalCase strings (e.g., "Bucket", "Application", "Pod"). Simple lowercasing of these values produces valid DNS-compliant names (e.g., "bucket", "application", "pod"). The concern about Kind containing non-alphanumeric characters does not apply in practice, as Kind is a standard Kubernetes schema type that only accepts valid resource kind names.Likely an incorrect or invalid review comment.
| // +kubebuilder:selectablefield:JSONPath=`.spec.strategyRef.apiGroup` | ||
| // +kubebuilder:selectablefield:JSONPath=`.spec.strategyRef.kind` | ||
| // +kubebuilder:selectablefield:JSONPath=`.spec.strategyRef.name` |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Kubernetes selectableFields feature introduction version requirement
💡 Result:
- selectableFields for CRDs was introduced as an alpha feature (behind the CustomResourceFieldSelectors feature gate) in Kubernetes v1.30. [1][2]
- The CustomResourceFieldSelectors gate became enabled by default in v1.31. [2]
- The feature reached GA (stable) in v1.32. [3]
Sources: [1] Kubernetes v1.30 announcement; [2] Kubernetes docs for CRD selectableFields (v1.30/v1.31 notes); [3] Kubernetes v1.32 release/docs.
🌐 Web query:
kubebuilder selectablefield annotation marker documentation
💡 Result:
The +kubebuilder:selectablefield marker adds a field (via a JSONPath) to the CRD so it can be used with Kubernetes field selectors. Syntax:
// +kubebuilder:selectablefield=
Example:
// +kubebuilder:selectablefield=".status.phase"
Reference: Kubebuilder markers (selectablefield). (book.kubebuilder.io) [1]; controller-tools markers pkg. (pkg.go.dev) [2]
Citations:
- 1: https://book.kubebuilder.io/reference/markers/crd.html?utm_source=openai
- 2: https://pkg.go.dev/sigs.k8s.io/controller-tools/pkg/crd/markers?utm_source=openai
Good use of selectable fields for efficient filtering.
The selectablefield markers enable server-side filtering by strategy reference, which is useful for controllers watching specific BackupJobs. Note that selectableFields was introduced as an alpha feature in Kubernetes 1.30, enabled by default in 1.31, and reached GA in 1.32.
🤖 Prompt for AI Agents
In api/backups/v1alpha1/backupjob_types.go around lines 75 to 77, add a brief
note documenting the Kubernetes version requirement for selectable fields and,
if necessary for your project, include/update a marker or README entry
indicating that selectableFields is alpha in 1.30, enabled by default in 1.31
and GA in 1.32 so consumers know the minimum supported Kubernetes version;
update any CRD generation or CI tests to target a compatible Kubernetes version
to avoid runtime incompatibilities.
| func (r *BackupJobStrategyReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { | ||
| log := log.FromContext(ctx) | ||
|
|
||
| log.V(2).Info("reconciling") | ||
|
|
||
| j := &backupsv1alpha1.BackupJob{} | ||
|
|
||
| if err := r.Get(ctx, client.ObjectKey{Namespace: req.Namespace, Name: req.Name}, j); err != nil { | ||
| if apierrors.IsNotFound(err) { | ||
| log.V(3).Info("BackupJob not found") | ||
| return ctrl.Result{}, nil | ||
| } | ||
| return ctrl.Result{}, err | ||
| } | ||
|
|
||
| var applicationRefAPIGroup string | ||
| var strategyRefAPIGroup string | ||
| var storageRefAPIGroup string | ||
| 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 | ||
| } | ||
|
|
||
| if strategyRefAPIGroup != strategyv1alpha1.GroupVersion.Group { | ||
| return ctrl.Result{}, nil | ||
| } | ||
| if j.Spec.StrategyRef.Kind != strategyv1alpha1.JobStrategyKind { | ||
| return ctrl.Result{}, nil | ||
| } | ||
|
|
||
| app, err := r.getUnstructured(ctx, applicationRefAPIGroup, j.Spec.ApplicationRef.Kind, j.Namespace, j.Spec.ApplicationRef.Name) | ||
| if err != nil { | ||
| // TODO: we should handle not-found errors separately, but it's not | ||
| // clear, how to trigger a reconcile if the application is created | ||
| // later, so we just rely on the default exponential backoff. | ||
| return ctrl.Result{}, err | ||
| } | ||
|
|
||
| strategy := &strategyv1alpha1.Job{} | ||
| err = r.Get(ctx, types.NamespacedName{Name: j.Spec.StrategyRef.Name}, strategy) | ||
| if err != nil { | ||
| // TODO: as with the app, not-found errors for strategies are pointless | ||
| // to retry, but a reconcile should be triggered if a strategy is later | ||
| // created. | ||
| return ctrl.Result{}, err | ||
| } | ||
|
|
||
| // TODO: we should use the storage in a more generic way, but since the | ||
| // storage part of the backups API is not implemented at all, we skip this | ||
| // for now and revert to a default implementation: only Bucket is supported | ||
| if storageRefAPIGroup != appscozystackio.GroupName { | ||
| return ctrl.Result{}, nil | ||
| } | ||
| if j.Spec.StorageRef.Kind != "Bucket" { | ||
| return ctrl.Result{}, nil | ||
| } | ||
| _, err = r.getUnstructured(ctx, storageRefAPIGroup, j.Spec.StorageRef.Kind, j.Namespace, j.Spec.StorageRef.Name) | ||
| if err != nil { | ||
| // TODO: same not-found caveat as before | ||
| return ctrl.Result{}, err | ||
| } | ||
|
|
||
| values, ok := app.Object["spec"].(map[string]any) | ||
| if !ok { | ||
| values = map[string]any{} | ||
| } | ||
| release := map[string]any{ | ||
| "Name": fmt.Sprintf("%s-%s", strings.ToLower(j.Spec.ApplicationRef.Kind), j.Spec.ApplicationRef.Name), | ||
| "Namespace": j.Namespace, | ||
| } | ||
| templateContext := map[string]any{ | ||
| "Release": release, | ||
| "Values": values, | ||
| "Storage": map[string]any{ | ||
| "APIGroup": storageRefAPIGroup, | ||
| "Kind": j.Spec.StorageRef.Kind, | ||
| "Name": fmt.Sprintf("%s-%s", strings.ToLower(j.Spec.StorageRef.Kind), j.Spec.StorageRef.Name), | ||
| }, | ||
| } | ||
| podTemplate, err := template.Template(&strategy.Spec.Template, templateContext) | ||
| if err != nil { | ||
| return ctrl.Result{}, err | ||
| } | ||
|
|
||
| job := &batchv1.Job{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: j.Name, | ||
| Namespace: j.Namespace, | ||
| }, | ||
| Spec: batchv1.JobSpec{ | ||
| Template: *podTemplate, | ||
| }, | ||
| } | ||
|
|
||
| if err := r.Create(ctx, job); err != nil { | ||
| return ctrl.Result{}, err | ||
| } | ||
|
|
||
| return ctrl.Result{}, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Missing status updates for BackupJob resource.
The reconciler creates a Kubernetes Job but never updates the BackupJob's status to reflect the operation's outcome. Users have no visibility into whether the job was created, is running, succeeded, or failed. Consider updating the BackupJob.Status.Conditions to track the Job's lifecycle.
Example pattern to add after Job creation/retrieval:
// Update BackupJob status based on Job state
condition := metav1.Condition{
Type: "JobCreated",
Status: metav1.ConditionTrue,
ObservedGeneration: j.Generation,
LastTransitionTime: metav1.Now(),
Reason: "JobCreated",
Message: fmt.Sprintf("Backup job %s created", job.Name),
}
// Check job status and update accordingly
if job.Status.Succeeded > 0 {
condition.Type = "Completed"
condition.Reason = "JobSucceeded"
condition.Message = "Backup job completed successfully"
} else if job.Status.Failed > 0 {
condition.Type = "Failed"
condition.Status = metav1.ConditionFalse
condition.Reason = "JobFailed"
condition.Message = "Backup job failed"
}
meta.SetStatusCondition(&j.Status.Conditions, condition)
if err := r.Status().Update(ctx, j); err != nil {
return ctrl.Result{}, err
}🤖 Prompt for AI Agents
internal/backupcontroller/jobstrategy_controller.go lines 39-143: the reconciler
creates a Kubernetes Job but never updates the BackupJob.Status to reflect
creation/running/succeeded/failed; after creating or retrieving the Job,
construct a metav1.Condition (set Type to JobCreated/Completed/Failed, Status,
ObservedGeneration = j.Generation, LastTransitionTime = metav1.Now(), Reason and
Message) inspect job.Status.Succeeded/Failed to pick Completed or Failed (or
keep JobCreated/Unknown), call meta.SetStatusCondition(&j.Status.Conditions,
condition) and then r.Status().Update(ctx, j), and handle update errors (retry
on conflict) so users can see the Job lifecycle.
| job := &batchv1.Job{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: j.Name, | ||
| Namespace: j.Namespace, | ||
| }, | ||
| Spec: batchv1.JobSpec{ | ||
| Template: *podTemplate, | ||
| }, | ||
| } | ||
|
|
||
| if err := r.Create(ctx, job); err != nil { | ||
| return ctrl.Result{}, err | ||
| } |
There was a problem hiding this comment.
Critical: Job creation lacks idempotency and owner reference.
The current implementation creates a Job without checking if it already exists. On subsequent reconciliations, this will fail with an "AlreadyExists" error, breaking the controller's reconcile loop. Additionally, the Job lacks an owner reference to the BackupJob, preventing proper garbage collection.
Apply this approach to fix both issues:
job := &batchv1.Job{
ObjectMeta: metav1.ObjectMeta{
Name: j.Name,
Namespace: j.Namespace,
+ OwnerReferences: []metav1.OwnerReference{
+ *metav1.NewControllerRef(j, backupsv1alpha1.GroupVersion.WithKind("BackupJob")),
+ },
},
Spec: batchv1.JobSpec{
Template: *podTemplate,
},
}
- if err := r.Create(ctx, job); err != nil {
+ if err := r.Create(ctx, job); err != nil {
+ if !apierrors.IsAlreadyExists(err) {
- return ctrl.Result{}, err
+ return ctrl.Result{}, err
+ }
+ // Job already exists; fetch and check its status
+ if err := r.Get(ctx, client.ObjectKey{Namespace: j.Namespace, Name: j.Name}, job); err != nil {
+ return ctrl.Result{}, 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.
| job := &batchv1.Job{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: j.Name, | |
| Namespace: j.Namespace, | |
| }, | |
| Spec: batchv1.JobSpec{ | |
| Template: *podTemplate, | |
| }, | |
| } | |
| if err := r.Create(ctx, job); err != nil { | |
| return ctrl.Result{}, err | |
| } | |
| job := &batchv1.Job{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: j.Name, | |
| Namespace: j.Namespace, | |
| OwnerReferences: []metav1.OwnerReference{ | |
| *metav1.NewControllerRef(j, backupsv1alpha1.GroupVersion.WithKind("BackupJob")), | |
| }, | |
| }, | |
| Spec: batchv1.JobSpec{ | |
| Template: *podTemplate, | |
| }, | |
| } | |
| if err := r.Create(ctx, job); err != nil { | |
| if !apierrors.IsAlreadyExists(err) { | |
| return ctrl.Result{}, err | |
| } | |
| // Job already exists; fetch and check its status | |
| if err := r.Get(ctx, client.ObjectKey{Namespace: j.Namespace, Name: j.Name}, job); err != nil { | |
| return ctrl.Result{}, err | |
| } | |
| } |
🤖 Prompt for AI Agents
In internal/backupcontroller/jobstrategy_controller.go around lines 128 to 140,
the code blindly creates a Job which causes AlreadyExists errors on requeues and
the Job has no owner reference to the BackupJob; change the logic to first try
to fetch the Job (r.Get) using the same name/namespace, if found reuse it (or
update its spec if you need to reconcile changes) and only call r.Create when
the GET returns NotFound; before creating set the owner reference on the Job to
the BackupJob using controllerutil.SetControllerReference (or construct a proper
OwnerReference) so Kubernetes can garbage-collect the Job; ensure you handle and
return other errors from r.Get/r.Create appropriately and import controllerutil
if not already present.
## What this PR does This patch adds the boilerplate for the `strategy.backups.cozystack.io` API group that will contain reference backup/restore strategy implementations that will live in Cozystack core. ### Release note ```release-note [backups] Scaffold the `strategy.backups.cozystack.io` API group to provide a well-defined point for adding reference strategy implementations. ``` Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
2320543 to
0d71525
Compare
## What this PR does
This patch adds the boilerplate for the `strategy.backups.cozystack.io`
API group that will contain reference backup/restore strategy
implementations that will live in Cozystack core.
### Release note
```release-note
[backups] Scaffold the `strategy.backups.cozystack.io` API group to
provide a well-defined point for adding reference strategy
implementations.
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **New Features**
* Introduced v1alpha1 backup strategy API group registration, enabling
support for backup strategy resources within the system.
<sub>✏️ Tip: You can customize this high-level summary in your review
settings.</sub>
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
(cherry picked from commit 03a71eb)
Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
…d backup system (#1867) ## What this PR does Update changelog for v1.0.0-alpha.1 to include missing features: - **Cozystack Operator**: New operator for Package and PackageSource management (#1740, #1741, #1755, #1756, #1760, #1761) - **Backup System**: Comprehensive backup functionality with Velero integration (#1640, #1685, #1687, #1708, #1719, #1720, #1737, #1762) - Add @androndo to contributors - Update Full Changelog link to v0.38.0...v1.0.0-alpha.1 ### Release note ```release-note [docs] Update changelog for v1.0.0-alpha.1: add cozystack-operator and backup system ```
What this PR does
This patch adds the boilerplate for the
strategy.backups.cozystack.ioAPI group that will contain reference backup/restore strategy implementations that will live in Cozystack core.Release note
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.