Skip to content

Commit b9e43fb

Browse files
mattia-eleuterimattia-eleuteri
committed
fix(backups): create no driver object for a job that already finished
The Velero, CNPG and MariaDB drivers decide a BackupJob or RestoreJob is still running from the informer cache, at the top of the pass, and create their driver object further down without looking again. A job another actor marked Failed in between, or whose Failed status the cache has not caught up with yet, still received a Velero Backup or Restore, a cnpg.io or k8s.mariadb.com Backup, or a k8s.mariadb.com Restore. Nothing tracks that object afterwards, because the reconcilers skip terminal jobs: a backup runs that no Backup artifact will ever reference, and a restore rewrites an application whose RestoreJob already says Failed. Restores are worse than that, because they rewrite the target before creating anything. The Velero driver suspends the HelmReleases, halts the VM and renames its PVCs over several passes; the CNPG driver suspends the HelmRelease, patches the Postgres app and deletes the Cluster with its PVCs once the WAL gate clears, which can take as long as the archive takes to catch up. Each driver now re-reads the job through the manager's uncached reader right before it creates a driver object, and before the destructive restore steps, and stops without touching the job's status when the apiserver holds it as finished, deleted or being deleted: whoever finished it wrote the verdict. A failed re-read requeues instead of failing the job, since the job's state is what is unknown. The re-read narrows the window rather than closing it: the job can still turn terminal between the Get and the Create, as no apiserver precondition ties the creation of one object to the state of another. Signed-off-by: Mattia Eleuteri <[email protected]> Assisted-by: LLM
1 parent bc400df commit b9e43fb

7 files changed

Lines changed: 641 additions & 0 deletions

‎internal/backupcontroller/backupjob_controller.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,10 @@ type BackupJobReconciler struct {
3535
Scheme *runtime.Scheme
3636
Recorder record.EventRecorder
3737
CredentialsConfig BackupCredentialsConfig
38+
// APIReader is the manager's uncached reader, through which a driver
39+
// re-reads the BackupJob before creating its driver object (see
40+
// ensureJobNotFinished). Wired in SetupWithManager.
41+
APIReader client.Reader
3842
}
3943

4044
func (r *BackupJobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
@@ -230,6 +234,7 @@ func (r *BackupJobReconciler) SetupWithManager(mgr ctrl.Manager) error {
230234
return err
231235
}
232236

237+
r.APIReader = mgr.GetAPIReader()
233238
cfg := mgr.GetConfig()
234239
var err error
235240
if r.Interface, err = dynamic.NewForConfig(cfg); err != nil {

‎internal/backupcontroller/cnpgstrategy_controller.go‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -309,6 +309,9 @@ func (r *BackupJobReconciler) reconcileCNPG(ctx context.Context, j *backupsv1alp
309309
}
310310

311311
cnpgBackup, err := r.ensureCNPGBackup(ctx, j, clusterName)
312+
if stopped, res, gerr := stoppedByJobGuard(err); stopped {
313+
return res, gerr
314+
}
312315
if err != nil {
313316
return r.markBackupJobFailed(ctx, j, fmt.Sprintf("failed to ensure cnpg.io/Backup: %v", err))
314317
}
@@ -584,6 +587,9 @@ func (r *BackupJobReconciler) ensureCNPGBackup(ctx context.Context, j *backupsv1
584587
},
585588
}
586589

590+
if err := ensureJobNotFinished(ctx, r.apiReader(), j); err != nil {
591+
return nil, err
592+
}
587593
if err := r.Create(ctx, obj); err != nil {
588594
return nil, err
589595
}
@@ -916,6 +922,12 @@ func (r *RestoreJobReconciler) reconcileCNPGRestore(ctx context.Context, restore
916922
logger.Info("restore does not pin its base backup; the barman-cloud plugin picks one from the catalog",
917923
"backup", backup.Name, "recoveryTime", options.RecoveryTime)
918924
}
925+
// From here the target is rewritten: HelmRelease suspended, app spec
926+
// patched, Cluster and PVCs deleted.
927+
if err := ensureJobNotFinished(ctx, r.apiReader(), restoreJob); err != nil {
928+
_, res, gerr := stoppedByJobGuard(err)
929+
return res, gerr
930+
}
919931
hrName := postgresAppPrefix + target.AppName
920932
if err := r.setCNPGRestoreHRSuspended(ctx, target.Namespace, hrName, true); err != nil {
921933
return ctrl.Result{}, err

‎internal/backupcontroller/job_finished_guard.go‎

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
// SPDX-License-Identifier: Apache-2.0
2+
package backupcontroller
3+
4+
import (
5+
"context"
6+
"errors"
7+
"fmt"
8+
9+
apierrors "k8s.io/apimachinery/pkg/api/errors"
10+
ctrl "sigs.k8s.io/controller-runtime"
11+
"sigs.k8s.io/controller-runtime/pkg/client"
12+
13+
backupsv1alpha1 "github.com/cozystack/cozystack/api/backups/v1alpha1"
14+
)
15+
16+
var (
17+
// errJobFinished: the apiserver already holds the job as finished.
18+
// Whoever finished it wrote the verdict, so the pass ends without
19+
// touching the job's status.
20+
errJobFinished = errors.New("job already finished on the apiserver")
21+
// errJobRecheck: the re-read itself failed. The pass is retried rather
22+
// than the job failed, since the job's state is exactly what is unknown.
23+
errJobRecheck = errors.New("re-read job")
24+
)
25+
26+
// ensureJobNotFinished re-reads job through reader, past the informer cache,
27+
// and returns errJobFinished when it is Succeeded, Failed, being deleted or
28+
// gone.
29+
//
30+
// The phase every driver checks comes from the cache at the top of the pass,
31+
// so a job another actor marked Failed meanwhile would still receive its
32+
// Velero Backup, operator dump or restore — an object nothing tracks once
33+
// the job is terminal, because the reconcilers skip terminal jobs. Call this
34+
// right before creating a driver object, and before any destructive step a
35+
// restore takes on the application.
36+
//
37+
// It narrows the window, it does not close it: the job can still turn
38+
// terminal between this Get and the Create that follows, since no apiserver
39+
// precondition ties the creation of one object to the state of another.
40+
func ensureJobNotFinished(ctx context.Context, reader client.Reader, job client.Object) error {
41+
var fresh client.Object
42+
switch job.(type) {
43+
case *backupsv1alpha1.BackupJob:
44+
fresh = &backupsv1alpha1.BackupJob{}
45+
case *backupsv1alpha1.RestoreJob:
46+
fresh = &backupsv1alpha1.RestoreJob{}
47+
default:
48+
return fmt.Errorf("%w: unsupported job type %T", errJobRecheck, job)
49+
}
50+
if err := reader.Get(ctx, client.ObjectKeyFromObject(job), fresh); err != nil {
51+
if apierrors.IsNotFound(err) {
52+
return logJobFinished(ctx, job, "deleted")
53+
}
54+
return fmt.Errorf("%w %s/%s: %w", errJobRecheck, job.GetNamespace(), job.GetName(), err)
55+
}
56+
if !fresh.GetDeletionTimestamp().IsZero() {
57+
return logJobFinished(ctx, job, "being deleted")
58+
}
59+
switch f := fresh.(type) {
60+
case *backupsv1alpha1.BackupJob:
61+
if f.Status.Phase == backupsv1alpha1.BackupJobPhaseSucceeded || f.Status.Phase == backupsv1alpha1.BackupJobPhaseFailed {
62+
return logJobFinished(ctx, job, string(f.Status.Phase))
63+
}
64+
case *backupsv1alpha1.RestoreJob:
65+
if f.Status.Phase == backupsv1alpha1.RestoreJobPhaseSucceeded || f.Status.Phase == backupsv1alpha1.RestoreJobPhaseFailed {
66+
return logJobFinished(ctx, job, string(f.Status.Phase))
67+
}
68+
}
69+
return nil
70+
}
71+
72+
func logJobFinished(ctx context.Context, job client.Object, state string) error {
73+
getLogger(ctx).Info("job finished before its driver object was created, creating nothing",
74+
"namespace", job.GetNamespace(), "name", job.GetName(), "state", state)
75+
return errJobFinished
76+
}
77+
78+
// stoppedByJobGuard tells a caller whether err, possibly returned through a
79+
// driver's create helper, came from ensureJobNotFinished, and if so what the
80+
// pass returns. Any other error is left to the caller's own handling, which
81+
// usually fails the job.
82+
func stoppedByJobGuard(err error) (bool, ctrl.Result, error) {
83+
switch {
84+
case errors.Is(err, errJobFinished):
85+
return true, ctrl.Result{}, nil
86+
case errors.Is(err, errJobRecheck):
87+
return true, ctrl.Result{}, err
88+
}
89+
return false, ctrl.Result{}, nil
90+
}
91+
92+
// apiReader returns the uncached reader the guard needs. Reconcilers built
93+
// by hand in unit tests have none and fall back to their fake Client, which
94+
// has no cache to bypass.
95+
func (r *BackupJobReconciler) apiReader() client.Reader {
96+
if r.APIReader != nil {
97+
return r.APIReader
98+
}
99+
return r.Client
100+
}
101+
102+
func (r *RestoreJobReconciler) apiReader() client.Reader {
103+
if r.APIReader != nil {
104+
return r.APIReader
105+
}
106+
return r.Client
107+
}

0 commit comments

Comments
 (0)