Skip to content

Commit a9cd40f

Browse files
mattia-eleuterimattia-eleuteri
committed
fix(backups): hold a BackupJob while its application is being restored
Nothing on the backup path looks at restores. A Plan creates its BackupJobs on schedule, and the BackupJob reconciler dispatches them to their driver whatever else is happening to the application. For CNPG that is not merely a backup of a half-restored state: every BackupJob reconcile re-applies the Cluster's barman plugin, and on a cluster under an in-place restore it can land after the chart has re-rendered the Cluster with its new serverName and put it back on the source's. The restored cluster then archives onto the source's WAL prefix and its recovery fails with "WAL archive check failed ... Expected empty archive". This was reproduced with a backup triggered a few seconds after an in-place restore, and an hourly plan overlaps a restore of up to thirty minutes about half the time. A BackupJob that has not started yet now waits while a non-terminal RestoreJob in its namespace writes into the same application, resolved as the drivers resolve it: the targetApplicationRef when set, the Backup's applicationRef otherwise. A RestoreJob whose Backup is gone holds nothing, since the restore controller fails it on its next pass. options.targetNamespace is honoured only for a Backup taken by the Velero strategy, the one driver that reads it; the CNPG and Job-based drivers ignore the option and restore in place, so their restores hold whatever it says. The held job stays Pending with a Ready=False RestoreInProgress condition naming the restore and is requeued, rather than failed, so a scheduled run still happens once the restore ends. A failed write of that status is returned, so the manager retries it. When the restore ends the condition is removed before the job starts: the drivers write Ready again only when the backup completes or fails, and it would otherwise read "waiting" for the whole run. A run that has already started is left alone: its driver owns state the gate would strand. Assisted-by: LLM Signed-off-by: Mattia Eleuteri <[email protected]>
1 parent 9c94337 commit a9cd40f

3 files changed

Lines changed: 503 additions & 0 deletions

File tree

‎internal/backupcontroller/backupjob_controller.go‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,23 @@ func (r *BackupJobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
130130
return r.markBackupJobFailed(ctx, j, fmt.Sprintf("strategy Kind %q is not supported by this controller (supported: %s)", strategyRef.Kind, strings.Join(supportedBackupStrategyKinds(), ", ")))
131131
}
132132

133+
// Hold a run that has not started while a restore writes into its
134+
// application. Only before StartedAt: every driver stamps it on its
135+
// first pass, and a run already moving data is not interrupted here.
136+
if j.Status.StartedAt == nil {
137+
restore, err := activeRestoreTargeting(ctx, r.Client, j.Namespace, normalizedAppRef)
138+
if err != nil {
139+
logger.Error(err, "failed to check for a restore in progress")
140+
return ctrl.Result{}, err
141+
}
142+
if restore != "" {
143+
return r.holdForRestore(ctx, j, restore)
144+
}
145+
if err := r.releaseRestoreHold(ctx, j); err != nil {
146+
return ctrl.Result{}, err
147+
}
148+
}
149+
133150
// Now project the platform-managed S3 credentials into the tenant
134151
// namespace so default Strategy CRs can reference a deterministic
135152
// Secret name. The projection is idempotent and silently skipped on

‎internal/backupcontroller/backupjob_restore_lock.go‎

Lines changed: 163 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,163 @@
1+
// SPDX-License-Identifier: Apache-2.0
2+
package backupcontroller
3+
4+
import (
5+
"context"
6+
"encoding/json"
7+
"fmt"
8+
"sort"
9+
"time"
10+
11+
corev1 "k8s.io/api/core/v1"
12+
"k8s.io/apimachinery/pkg/api/meta"
13+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
14+
ctrl "sigs.k8s.io/controller-runtime"
15+
"sigs.k8s.io/controller-runtime/pkg/client"
16+
17+
strategyv1alpha1 "github.com/cozystack/cozystack/api/backups/strategy/v1alpha1"
18+
backupsv1alpha1 "github.com/cozystack/cozystack/api/backups/v1alpha1"
19+
)
20+
21+
// RestoreInProgressRequeue is how often a BackupJob held by a restore checks
22+
// again. Restores run for minutes, so there is nothing to gain from polling
23+
// faster.
24+
const RestoreInProgressRequeue = 30 * time.Second
25+
26+
// ConditionReasonRestoreInProgress is the Ready=False reason of a BackupJob
27+
// held back because its application is being restored.
28+
const ConditionReasonRestoreInProgress = "RestoreInProgress"
29+
30+
// activeRestoreTargeting returns the name of a non-terminal RestoreJob in
31+
// namespace that writes into app, or "" when there is none.
32+
//
33+
// A backup must not start against an application a restore is rewriting. It
34+
// would capture a half-restored state at best, and on the CNPG path it does
35+
// worse: every BackupJob reconcile re-applies the Cluster's barman plugin, and
36+
// one landing between the restore re-rendering the Cluster with its new
37+
// serverName and the plugin being attached can put the restored cluster back
38+
// on the source's WAL prefix, where its recovery fails with "Expected empty
39+
// archive". Plan-created runs are the ones that hit this, since nothing else
40+
// looks at restores before creating a BackupJob.
41+
func activeRestoreTargeting(ctx context.Context, c client.Client, namespace string, app corev1.TypedLocalObjectReference) (string, error) {
42+
app = NormalizeApplicationRef(app)
43+
list := &backupsv1alpha1.RestoreJobList{}
44+
if err := c.List(ctx, list, client.InNamespace(namespace)); err != nil {
45+
return "", fmt.Errorf("list RestoreJobs: %w", err)
46+
}
47+
var holding []string
48+
for i := range list.Items {
49+
rj := &list.Items[i]
50+
if rj.Status.Phase == backupsv1alpha1.RestoreJobPhaseSucceeded || rj.Status.Phase == backupsv1alpha1.RestoreJobPhaseFailed {
51+
continue
52+
}
53+
target, ok, err := restoreJobTarget(ctx, c, rj)
54+
if err != nil {
55+
return "", err
56+
}
57+
if ok && sameApplication(target, app) {
58+
holding = append(holding, rj.Name)
59+
}
60+
}
61+
if len(holding) == 0 {
62+
return "", nil
63+
}
64+
sort.Strings(holding)
65+
return holding[0], nil
66+
}
67+
68+
// restoreJobTarget resolves the application a RestoreJob writes into in its
69+
// own namespace, as the drivers resolve it: its targetApplicationRef when set,
70+
// the Backup's applicationRef otherwise. The CRD requires kind and name on a
71+
// target, so there is nothing to merge; a missing API group is defaulted, as
72+
// the drivers default it.
73+
//
74+
// It reports false for a restore that writes nothing here. One whose Backup is
75+
// gone is failed by the restore controller on its next pass. A Velero restore
76+
// whose options name another namespace writes there: only the Velero driver
77+
// reads options.targetNamespace, so the option is looked at only when the
78+
// Backup's strategy is Velero — the CNPG and Job-based drivers ignore it and
79+
// restore in place.
80+
func restoreJobTarget(ctx context.Context, c client.Client, rj *backupsv1alpha1.RestoreJob) (corev1.TypedLocalObjectReference, bool, error) {
81+
backup := &backupsv1alpha1.Backup{}
82+
if err := c.Get(ctx, client.ObjectKey{Namespace: rj.Namespace, Name: rj.Spec.BackupRef.Name}, backup); err != nil {
83+
if client.IgnoreNotFound(err) == nil {
84+
return corev1.TypedLocalObjectReference{}, false, nil
85+
}
86+
return corev1.TypedLocalObjectReference{}, false, fmt.Errorf("get Backup %s of RestoreJob %s: %w", rj.Spec.BackupRef.Name, rj.Name, err)
87+
}
88+
if backup.Spec.StrategyRef.Kind == strategyv1alpha1.VeleroStrategyKind && restoresIntoAnotherNamespace(rj, backup.Namespace) {
89+
return corev1.TypedLocalObjectReference{}, false, nil
90+
}
91+
if t := rj.Spec.TargetApplicationRef; t != nil {
92+
return NormalizeApplicationRef(*t), true, nil
93+
}
94+
return NormalizeApplicationRef(backup.Spec.ApplicationRef), true, nil
95+
}
96+
97+
// restoresIntoAnotherNamespace reports whether a Velero RestoreJob's
98+
// options.targetNamespace sends it away from namespace, compared as the Velero
99+
// driver compares it. Options that do not parse are read as none.
100+
func restoresIntoAnotherNamespace(rj *backupsv1alpha1.RestoreJob, namespace string) bool {
101+
if rj.Spec.Options == nil || len(rj.Spec.Options.Raw) == 0 {
102+
return false
103+
}
104+
var opts struct {
105+
TargetNamespace string `json:"targetNamespace"`
106+
}
107+
if err := json.Unmarshal(rj.Spec.Options.Raw, &opts); err != nil {
108+
return false
109+
}
110+
return opts.TargetNamespace != "" && opts.TargetNamespace != namespace
111+
}
112+
113+
// sameApplication expects both references normalized: an API group left
114+
// unset compares equal only once it has been defaulted on both sides.
115+
func sameApplication(a, b corev1.TypedLocalObjectReference) bool {
116+
return a.Kind == b.Kind && a.Name == b.Name && ptrString(a.APIGroup) == ptrString(b.APIGroup)
117+
}
118+
119+
func ptrString(s *string) string {
120+
if s == nil {
121+
return ""
122+
}
123+
return *s
124+
}
125+
126+
// holdForRestore keeps a BackupJob that has not started yet waiting while a
127+
// restore writes into its application. It stays non-terminal, so a scheduled
128+
// run still happens once the restore ends, only later than its slot.
129+
//
130+
// A failed status write is returned so the manager retries it with backoff.
131+
// The job is held either way, since nothing past the gate ran, but a hold
132+
// nobody can see reads as a run stuck for no reason.
133+
func (r *BackupJobReconciler) holdForRestore(ctx context.Context, j *backupsv1alpha1.BackupJob, restore string) (ctrl.Result, error) {
134+
if j.Status.Phase == backupsv1alpha1.BackupJobPhaseEmpty {
135+
j.Status.Phase = backupsv1alpha1.BackupJobPhasePending
136+
}
137+
meta.SetStatusCondition(&j.Status.Conditions, metav1.Condition{
138+
Type: "Ready",
139+
Status: metav1.ConditionFalse,
140+
Reason: ConditionReasonRestoreInProgress,
141+
Message: fmt.Sprintf("waiting for RestoreJob %q to finish: application %s/%s is being restored", restore, j.Spec.ApplicationRef.Kind, j.Spec.ApplicationRef.Name),
142+
})
143+
if err := r.Status().Update(ctx, j); err != nil {
144+
return ctrl.Result{}, fmt.Errorf("record RestoreInProgress on BackupJob %s: %w", j.Name, err)
145+
}
146+
return ctrl.Result{RequeueAfter: RestoreInProgressRequeue}, nil
147+
}
148+
149+
// releaseRestoreHold removes the condition holdForRestore left, once the
150+
// restore has ended and the job goes on to start. Left in place it would read
151+
// "waiting for RestoreJob" for the whole run: the drivers write Ready again
152+
// only when the backup completes or fails.
153+
func (r *BackupJobReconciler) releaseRestoreHold(ctx context.Context, j *backupsv1alpha1.BackupJob) error {
154+
cond := meta.FindStatusCondition(j.Status.Conditions, "Ready")
155+
if cond == nil || cond.Reason != ConditionReasonRestoreInProgress {
156+
return nil
157+
}
158+
meta.RemoveStatusCondition(&j.Status.Conditions, "Ready")
159+
if err := r.Status().Update(ctx, j); err != nil {
160+
return fmt.Errorf("clear RestoreInProgress on BackupJob %s: %w", j.Name, err)
161+
}
162+
return nil
163+
}

0 commit comments

Comments
 (0)