internal/backupcontroller/factory/backupjob.go (1)

12-28: Consider adding TriggeredBy and ownerReferences.

The DESIGN.md contract (lines 151-152) specifies that BackupJobs created by Plans should have spec.triggeredBy = "Plan" and ownerReferences set. Currently, this factory doesn't set either field.

Consider applying this diff:

 func BackupJob(p *backupsv1alpha1.Plan, scheduledFor time.Time) *backupsv1alpha1.BackupJob {
 	job := &backupsv1alpha1.BackupJob{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      fmt.Sprintf("%s-%d", p.Name, scheduledFor.Unix()/60),
 			Namespace: p.Namespace,
+			OwnerReferences: []metav1.OwnerReference{
+				*metav1.NewControllerRef(p, backupsv1alpha1.GroupVersion.WithKind("Plan")),
+			},
 		},
 		Spec: backupsv1alpha1.BackupJobSpec{
 			PlanRef: &corev1.LocalObjectReference{
 				Name: p.Name,
 			},
 			ApplicationRef: *p.Spec.ApplicationRef.DeepCopy(),
 			StorageRef:     *p.Spec.StorageRef.DeepCopy(),
 			StrategyRef:    *p.Spec.StrategyRef.DeepCopy(),
+			TriggeredBy:    "Plan",
 		},
 	}
 	return job
 }

packages/system/backup-controller/definitions/backups.cozystack.io_plans.yaml (1)

68-82: Consider making schedule.cron a required field.

The description at line 75 states "omitting this field is not allowed" since only cron type is currently supported. However, the cron field is not in the required list for the schedule object. This creates a mismatch between documentation and validation.

Consider applying this diff to make the validation explicit:

               schedule:
                 description: Schedule specifies when backup copies are created.
                 properties:
                   cron:
                     description: |-
                       Cron contains the cron spec for scheduling backups. Must be
                       specified if the schedule type is `cron`. Since only `cron` is
                       supported, omitting this field is not allowed.
                     type: string
                   type:
                     description: |-
                       Type is the type of schedule specification. Supported values are
                       [`cron`]. If omitted, defaults to `cron`.
                     type: string
+                required:
+                - cron
                 type: object

Alternatively, implement validation via a CEL rule or validating webhook if you want to support future schedule types where cron is optional.

packages/system/backup-controller/definitions/backups.cozystack.io_backups.yaml (1)

1-234: Consider adding status subresource configuration.

The CRD definition is missing the subresources field to enable the status subresource. This is a recommended practice for Kubernetes CRDs with a status field, as it allows separate RBAC control and optimistic concurrency for status updates.

Add the following after line 16 (under versions:):

   - name: v1alpha1
+    subresources:
+      status: {}
     schema:

internal/backupcontroller/plan_controller.go (1)

29-61: Add deletion handling and optimize requeue strategy.

The reconciler has two issues:

1. Missing deletion handling: The reconciler doesn't check if the Plan is being deleted (has a deletionTimestamp), which could lead to creating BackupJobs for Plans that are being removed.

2. Suboptimal requeue strategy: The reconciler always requeues after startingDeadlineSeconds (300s) on success, even when the next scheduled run might be much further in the future. This causes unnecessary reconciliations.

Apply these improvements:

 func (r *PlanReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	log := log.FromContext(ctx)
 
 	log.V(2).Info("reconciling")
 
 	p := &backupsv1alpha1.Plan{}
 
 	if err := r.Get(ctx, client.ObjectKey{Namespace: req.Namespace, Name: req.Name}, p); err != nil {
 		if apierrors.IsNotFound(err) {
 			log.V(3).Info("Plan not found")
 			return ctrl.Result{}, nil
 		}
 		return ctrl.Result{}, err
 	}
+
+	// Skip reconciliation if Plan is being deleted
+	if !p.DeletionTimestamp.IsZero() {
+		return ctrl.Result{}, nil
+	}
 
 	tCheck := time.Now().Add(-startingDeadlineSeconds)
 	tNext := nextCronAfter(p.Spec.Schedule.Cron, tCheck)
 
 	if time.Now().Before(tNext) {
 		return ctrl.Result{RequeueAfter: tNext.Sub(time.Now())}, nil
 	}
 
 	run := factory.BackupJob(p, tNext)
 
 	if err := r.Create(ctx, run); err != nil {
 		if apierrors.IsAlreadyExists(err) {
-			return ctrl.Result{RequeueAfter: startingDeadlineSeconds}, nil
+			// Job already exists, compute next run
+			tNextRun := nextCronAfter(p.Spec.Schedule.Cron, time.Now())
+			delay := tNextRun.Sub(time.Now())
+			if delay < minRequeueDelay {
+				delay = minRequeueDelay
+			}
+			return ctrl.Result{RequeueAfter: delay}, nil
 		}
 		return ctrl.Result{}, err
 	}
 
-	return ctrl.Result{RequeueAfter: startingDeadlineSeconds}, nil
+	// Compute next run after successful creation
+	tNextRun := nextCronAfter(p.Spec.Schedule.Cron, time.Now())
+	delay := tNextRun.Sub(time.Now())
+	if delay < minRequeueDelay {
+		delay = minRequeueDelay
+	}
+	return ctrl.Result{RequeueAfter: delay}, nil
 }

packages/system/backup-controller/definitions/backups.cozystack.io_backupjobs.yaml (1)

1-231: Consider adding status subresource configuration.

Similar to the Backup CRD, this CRD is missing the subresources field to enable the status subresource. This is a recommended practice for Kubernetes CRDs with a status field.

Add the following after line 16 (under versions:):

   - name: v1alpha1
+    subresources:
+      status: {}
     schema:

packages/system/backup-controller/definitions/backups.cozystack.io_restorejobs.yaml (1)

1-168: Consider adding status subresource configuration.

Similar to the other CRDs in this PR, this CRD is missing the subresources field to enable the status subresource.

Add the following after line 16 (under versions:):

   - name: v1alpha1
+    subresources:
+      status: {}
     schema:

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T11:21:45.808Z
Learning: Applies to **/zz_generated.*.go : Do not edit auto-generated Go files matching pattern `zz_generated.*.go`

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

Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T11:21:45.808Z
Learning: Applies to {go.mod,go.sum} : Do not manually modify `go.mod` and `go.sum` files; use `go get` command instead

api/backups/v1alpha1/restorejob_types.go (1)

13-80: RestoreJob API shape looks good; consider status subresource and phase validation

The RestoreJob spec/status types follow standard Kubernetes patterns and match how similar resources are modeled elsewhere in the repo. Two non-blocking improvements you might consider:

• Add a status subresource so controllers can safely update .status without racing on .spec:

  // +kubebuilder:object:root=true
  // +kubebuilder:subresource:status
  type RestoreJob struct {
      ...
  }

• Tighten the CRD schema for Phase by adding an enum, to align with the defined constants:

  // +kubebuilder:validation:Enum=Pending;Running;Succeeded;Failed
  Phase RestoreJobPhase `json:"phase,omitempty"`

Both can be added incrementally without changing the Go API surface.

internal/backupcontroller/factory/backupjob.go (1)

12-27: BackupJob factory logic is sound; consider clarifying the name format

The factory correctly:

• Derives namespace/name from the Plan and scheduled time.
• Sets PlanRef and copies ApplicationRef/StorageRef/StrategyRef from the Plan spec, avoiding accidental aliasing via DeepCopy().

One small optional improvement: the scheduledFor.Unix()/60 naming scheme is a bit opaque to readers. A brief comment explaining that the suffix is “minutes since epoch” (and why minute-level granularity is chosen) or switching to a more self-describing format (e.g. YYYYMMDD-HHMM in UTC) would make debugging and manual inspection of BackupJobs easier.

hack/update-codegen.sh (1)

26-30: Codegen CRD moves are fragile with set -e and unguarded globs

The new TMPDIR workflow and split of CRDs into dedicated directories is a nice improvement, but with set -o errexit the unguarded mv calls on globs can cause the script to fail if a pattern has no matches (e.g., if backups CRDs are temporarily removed or renamed, or if no “other” CRDs are emitted):

• mv ${TMPDIR}/backups.cozystack.io*.yaml ...
• mv ${TMPDIR}/*.yaml ...

In such cases, Bash leaves the glob unexpanded and mv exits non‑zero.

To make this more robust, you can enable nullglob and guard the moves, and quote variables:

 CONTROLLER_GEN="go run sigs.k8s.io/controller-tools/cmd/controller-gen@v0.16.4"
-TMPDIR=$(mktemp -d)
-COZY_CONTROLLER_CRDDIR=packages/system/cozystack-controller/crds
-COZY_RD_CRDDIR=packages/system/cozystack-resource-definition-crd/definition
-BACKUPS_CORE_CRDDIR=packages/system/backup-controller/definitions
-trap 'rm -rf ${TMPDIR}' EXIT
+TMPDIR="$(mktemp -d)"
+COZY_CONTROLLER_CRDDIR="packages/system/cozystack-controller/crds"
+COZY_RD_CRDDIR="packages/system/cozystack-resource-definition-crd/definition"
+BACKUPS_CORE_CRDDIR="packages/system/backup-controller/definitions"
+trap 'rm -rf "${TMPDIR}"' EXIT
+shopt -s nullglob

@@
-$CONTROLLER_GEN rbac:roleName=manager-role crd paths="./api/..." output:crd:artifacts:config=${TMPDIR} 
-
-mv ${TMPDIR}/cozystack.io_cozystackresourcedefinitions.yaml \
-        ${COZY_RD_CRDDIR}/cozystack.io_cozystackresourcedefinitions.yaml
-
-mv ${TMPDIR}/backups.cozystack.io*.yaml ${BACKUPS_CORE_CRDDIR}/
-
-mv ${TMPDIR}/*.yaml ${COZY_CONTROLLER_CRDDIR}/
+$CONTROLLER_GEN rbac:roleName=manager-role crd paths="./api/..." output:crd:artifacts:config="${TMPDIR}"
+
+mv "${TMPDIR}/cozystack.io_cozystackresourcedefinitions.yaml" \
+   "${COZY_RD_CRDDIR}/cozystack.io_cozystackresourcedefinitions.yaml"
+
+backups_crds=("${TMPDIR}"/backups.cozystack.io*.yaml)
+if ((${#backups_crds[@]})); then
+  mv "${backups_crds[@]}" "${BACKUPS_CORE_CRDDIR}/"
+fi
+
+other_crds=("${TMPDIR}"/*.yaml)
+if ((${#other_crds[@]})); then
+  mv "${other_crds[@]}" "${COZY_CONTROLLER_CRDDIR}/"
+fi

This keeps the intent while avoiding spurious failures if CRD outputs change over time.

Also applies to: 61-68

api/backups/v1alpha1/plan_types.go (1)

13-71: Plan/PlanSchedule types are well-shaped; consider tightening validation to match docs

The Plan, PlanSpec, and PlanSchedule types look good and fit kubebuilder/controller-runtime conventions. The comments, however, promise semantics that aren’t currently enforced at the schema level:

• PlanScheduleType:
  • Comment says supported values are cron and that omitting Type defaults it to cron, but there is no default or Enum marker.
• PlanSchedule.Cron:
  • Comment says “omitting this field is not allowed”, but the field is marked +optional and omitempty.

To align CRD behavior with the documented contract, you could do something like:

type PlanScheduleType string

const (
    PlanScheduleTypeEmpty PlanScheduleType = ""
    PlanScheduleTypeCron  PlanScheduleType = "cron"
)

// PlanSchedule specifies when backup copies are created.
type PlanSchedule struct {
    // Type is the type of schedule specification. Supported values are [`cron`].
    // If omitted, defaults to `cron`.
    // +kubebuilder:validation:Enum=cron
    // +kubebuilder:default=cron
    Type PlanScheduleType `json:"type,omitempty"`

    // Cron contains the cron spec for scheduling backups.
    // Must be specified if the schedule type is `cron`.
    // +kubebuilder:validation:MinLength=1
    Cron string `json:"cron"`
}

This keeps the Go API the same while ensuring invalid Plans are rejected early by the API server.

As per coding guidelines, tightening kubebuilder validation here will make the CRDs more robust and self-documenting.

internal/backupcontroller/plan_controller.go (1)

18-21: Remove or use minRequeueDelay

minRequeueDelay is declared but never used; only startingDeadlineSeconds participates in requeue decisions. Either wire minRequeueDelay into the scheduling logic or drop the constant to avoid dead code.

api/backups/v1alpha1/backup_types.go (1)

68-83: Consider adding +kubebuilder:subresource:status to Backup

BackupStatus is clearly controller-owned (phase, artifact, conditions), but Backup currently only has +kubebuilder:object:root=true. Adding a status subresource:

// +kubebuilder:object:root=true
// +kubebuilder:subresource:status
type Backup struct {
    ...
}

would follow standard controller-runtime/kubebuilder patterns and allow controllers to update status via /status without risking concurrent spec updates.

Also applies to: 84-93

api/backups/v1alpha1/backupjob_types.go (1)

24-42: Align BackupJob API with DESIGN.md (TriggeredBy) and consider status subresource

Two small inconsistencies worth addressing:

1. DESIGN.md documents a BackupJobSpec field:

// Informational: what triggered this run ("Plan", "Manual", etc.).
TriggeredBy string `json:"triggeredBy,omitempty"`

but the actual BackupJobSpec (and the CRD) do not include TriggeredBy. Either:

• add TriggeredBy to BackupJobSpec and regenerate the CRD, or
• update DESIGN.md to drop that field if you intentionally decided against it.

2. As with Backup, BackupJob has a controller-updated Status but no +kubebuilder:subresource:status. Adding it:

// +kubebuilder:object:root=true
// +kubebuilder:subresource:status
type BackupJob struct {
    ...
}

would better match standard controller-runtime/kubebuilder practice and avoid spec/status write races.

Also applies to: 44-84

api/backups/v1alpha1/DESIGN.md (1)

88-176: Address markdownlint MD036 (“emphasis used instead of a heading”) if CI enforces it

Sections like **Purpose**, **Key fields (spec)**, and **Backup contract with drivers** are styled as bold text rather than proper headings, which triggers markdownlint’s MD036 rule.

If markdownlint is part of your checks, consider:

• Converting these to real headings, e.g. #### Purpose, #### Key fields (spec), etc., or
• Disabling MD036 for this file (or specific lines) if you prefer the current style.

Also applies to: 253-308, 378-407

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T11:21:45.808Z
Learning: Applies to **/zz_generated.*.go : Do not edit auto-generated Go files matching pattern `zz_generated.*.go`

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

Learnt from: lllamnyp
Repo: cozystack/cozystack PR: 1130
File: hack/e2e-apps/kubernetes.bats:101-101
Timestamp: 2025-07-11T06:11:25.438Z
Learning: In cozystack, the plural form for the Kubernetes custom resource is `kuberneteses.apps.cozystack.io`, not `kubernetes.apps.cozystack.io`. This is defined in the API schema even though it's not grammatically perfect.

api/backups/v1alpha1/plan_types.go (1)

66-77: Align PlanSchedule comments, validation, and controller behavior for type/cron.

Cron is tagged +optional and the schema doesn’t require it, but the comment says omitting it “is not allowed”, and the controller unconditionally parses p.Spec.Schedule.Cron. Similarly, Type is a free-form string while comments say only cron is supported and it “defaults to cron”, but there’s no defaulting logic here.

To make this consistent and fail fast:

• Either:

  • Make cron required in the schema and remove +optional, and
  • Add an enum validation on Type (e.g. "" and "cron") or drop the field until more kinds exist,

• Or soften the comments to reflect that:

  • Both fields are optional at the API level,
  • Invalid/missing cron and unsupported Type values are rejected by runtime validation/webhooks.

Given you already parse Schedule.Cron in PlanReconciler, tightening the schema (via kubebuilder markers here then re-running controller-gen) would keep users from submitting unusable Plans.

api/backups/v1alpha1/groupversion_info.go (1)

39-53: Scheme registration for backup API types looks correct now.

addKnownTypes now explicitly registers all CRD types (Plan, Backup, BackupJob, RestoreJob and their list variants) against GroupVersion, so AddToScheme will work with controller-runtime clients as expected. This addresses the earlier concern about AddKnownTypes being a no-op.

packages/system/backup-controller/definitions/backups.cozystack.io_plans.yaml (1)

68-82: Tighten spec.schedule schema to match comments and controller behavior.

The CRD currently allows schedule objects without cron, and type is an unconstrained string, but the descriptions say cron is mandatory and only cron is supported. Given the controller always reads spec.schedule.cron, this mismatch can let users submit Plans that will never schedule successfully.

Recommend:

• Under spec.properties.schedule, add a required: ["cron"] (and optionally "type" if you want to reject unknown schedule kinds), and
• Restrict type with an enum (e.g. ["", "cron"]) or remove it until more schedule types exist.

Best to express this via kubebuilder markers on PlanSchedule in plan_types.go and then regenerate this CRD with controller-gen, rather than editing the YAML directly.

api/backups/v1alpha1/plan_types.go (1)

20-23: Fix typo and consider more specific condition type name.

The // Condtions comment has a typo, and PlanConditionError = "Error" is quite generic as a condition type. Consider renaming the constant to something like "ScheduleError" (and updating call sites) to better convey what the condition represents.

api/backups/v1alpha1/backup_types.go (1)

84-102: Consider enabling the status subresource for Backup.

BackupStatus is modeled for controller updates, but Backup lacks a +kubebuilder:subresource:status marker. If drivers or controllers will update .status via client.Status().Update, it’s more idiomatic (and safer) to expose a status subresource.

You can add:

// +kubebuilder:object:root=true
// +kubebuilder:subresource:status
type Backup struct {
    ...
}

and re-run controller-gen to update the CRD.

api/backups/v1alpha1/restorejob_types.go (1)

62-80: Optionally expose a status subresource for RestoreJob.

RestoreJobStatus is intended to be driven by a controller, but RestoreJob doesn’t declare +kubebuilder:subresource:status. If a restore controller will update status via the status client, consider adding the marker so it doesn’t need full-object updates:

// +kubebuilder:object:root=true
// +kubebuilder:subresource:status
type RestoreJob struct {
    ...
}

Then regenerate the CRD.

api/backups/v1alpha1/backupjob_types.go (1)

74-93: Expose BackupJob status as a subresource for controller updates.

BackupJobStatus is designed for lifecycle reporting, but BackupJob doesn’t declare +kubebuilder:subresource:status. For controller-runtime style reconcilers that call client.Status().Update, it’s conventional to expose a status subresource:

// +kubebuilder:object:root=true
// +kubebuilder:subresource:status
type BackupJob struct {
    ...
}

Then re-run controller-gen to refresh the CRD.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

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

Learnt from: lllamnyp
Repo: cozystack/cozystack PR: 1130
File: hack/e2e-apps/kubernetes.bats:101-101
Timestamp: 2025-07-11T06:11:25.438Z
Learning: In cozystack, the plural form for the Kubernetes custom resource is `kuberneteses.apps.cozystack.io`, not `kubernetes.apps.cozystack.io`. This is defined in the API schema even though it's not grammatically perfect.