Skip to content

Commit 25ef6d4

Browse files
androndoAndrey
androndo
authored and
Andrey
committed
fix(backupstrategy-controller): fail on a stale same-named Backup and bound artifact-write retries
A Backup carries no ownerRef to its BackupJob and outlives it, so a BackupJob whose name is reused after the old one was deleted finds a stale same-named Backup pointing at the previous run's UID-scoped prefix. The mirror has already written a fresh copy under this run's prefix; returning the stale artifact reported Succeeded against the old snapshot and left the fresh copy unreferenced in the shared repo bucket, so a restore returned stale data. Compare the existing Backup's repo prefix to this run's on AlreadyExists and fail the BackupJob when they differ; a matching prefix is still a genuine idempotent retry. Bound the transient artifact-write retry by the same deadline as the bucket wait helpers and surface it through a Ready=False condition and a Warning Event, so a Create the apiserver keeps rejecting no longer sits Running indefinitely behind a log line the default level drops. Emit a Warning Event when a partial-backup reclaim Job cannot be started, matching the cleanup path's ArtifactNotDeleted signal instead of logging at debug only. Pin the merge half of the restore contract in s3mirror: a to-copy restore must keep a destination object the snapshot does not name. Add a reconcile test that a stale same-named Backup fails the run. Assisted-By: LLM Signed-off-by: Andrey Kolkov <[email protected]>
1 parent 223249d commit 25ef6d4

3 files changed

Lines changed: 174 additions & 5 deletions

File tree

‎internal/backupcontroller/bucketstrategy_controller.go‎

Lines changed: 56 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ package backupcontroller
44
import (
55
"context"
66
"encoding/json"
7+
"errors"
78
"fmt"
89
"strings"
910
"time"
@@ -390,11 +391,32 @@ func (r *BackupJobReconciler) reconcileBucket(ctx context.Context, j *backupsv1a
390391
}
391392
artifact, err := r.createBucketBackupArtifact(ctx, j, resolved, *rendered, claim.Status.BucketName, repoPrefix)
392393
if err != nil {
393-
// The mirror already wrote the full copy; failing terminally here
394-
// would strand it with no Backup for cleanup to key off. Requeue so a
395-
// transient artifact-write error retries (BackupRef gates re-create).
396-
logger := getLogger(ctx)
397-
logger.Debug("Backup artifact create failed, will retry", "backupjob", j.Name, "err", err)
394+
// A collision with a prior run's Backup is terminal - retrying cannot
395+
// resolve it, and this run must not report success against the stale
396+
// snapshot.
397+
if errors.Is(err, errBucketArtifactNameCollision) {
398+
return r.markBackupJobFailed(ctx, j, err.Error())
399+
}
400+
// Otherwise the mirror already wrote the full copy, so failing here
401+
// would strand it with no Backup for cleanup to key off: requeue the
402+
// transient artifact write (BackupRef gates re-create), bounded by the
403+
// same deadline as the wait helpers and surfaced through a condition
404+
// and Event so a persistently-rejected write is not silent.
405+
if strategyNotReadyDeadlineExceeded(j.Status.StartedAt) {
406+
return r.markBackupJobFailed(ctx, j, fmt.Sprintf("mirror completed but recording the Backup artifact did not succeed within %s: %v", StrategyNotReadyDeadline, err))
407+
}
408+
if r.Recorder != nil {
409+
r.Recorder.Eventf(j, corev1.EventTypeWarning, "ArtifactWritePending", "mirror completed but the Backup artifact is not yet recorded: %v", err)
410+
}
411+
apimeta.SetStatusCondition(&j.Status.Conditions, metav1.Condition{
412+
Type: "Ready",
413+
Status: metav1.ConditionFalse,
414+
Reason: "ArtifactWritePending",
415+
Message: fmt.Sprintf("mirror completed; recording Backup artifact: %v", err),
416+
})
417+
if updateErr := r.Status().Update(ctx, j); updateErr != nil {
418+
getLogger(ctx).Error(updateErr, "failed to update BackupJob status after artifact write error")
419+
}
398420
return ctrl.Result{RequeueAfter: bucketPollInterval}, nil
399421
}
400422
now := metav1.Now()
@@ -459,14 +481,25 @@ func (r *BackupJobReconciler) reclaimFailedBucketBackup(ctx context.Context, j *
459481
pod := buildBucketCleanupPod(snap, rendered.Image)
460482
desired := buildJobStrategyBatchJob(j.Namespace, j.Name+"-reclaim", map[string]string{bucketLabelMode: bucketModeCleanup}, pod)
461483
if err := controllerutil.SetControllerReference(j, desired, r.Scheme); err != nil {
484+
r.warnReclaimNotStarted(j, repoPrefix, err)
462485
logger.Debug("skipping partial-backup reclaim: cannot set controller reference", "backupjob", j.Name, "err", err)
463486
return
464487
}
465488
if err := r.Create(ctx, desired); err != nil && !apierrors.IsAlreadyExists(err) {
489+
r.warnReclaimNotStarted(j, repoPrefix, err)
466490
logger.Debug("partial-backup reclaim Job not created (best-effort)", "backupjob", j.Name, "err", err)
467491
}
468492
}
469493

494+
// warnReclaimNotStarted surfaces a failed partial-backup reclaim on the
495+
// BackupJob, matching releaseBucketCleanup's ArtifactNotDeleted signal so a
496+
// leaked partial copy is visible rather than only logged at debug level.
497+
func (r *BackupJobReconciler) warnReclaimNotStarted(j *backupsv1alpha1.BackupJob, repoPrefix string, err error) {
498+
if r.Recorder != nil {
499+
r.Recorder.Eventf(j, corev1.EventTypeWarning, "ReclaimNotStarted", "partial backup copy under %s left in the repo bucket: %v", repoPrefix, err)
500+
}
501+
}
502+
470503
// requeueBucketWaiting surfaces a transient Ready=False on the BackupJob while
471504
// a COSI-side precondition (source bucket, access grant) is not yet met, and
472505
// requeues. Bounded by StrategyNotReadyDeadline so a never-satisfied
@@ -567,6 +600,12 @@ func (r *BackupJobReconciler) ensureBucketAccess(ctx context.Context, namespace,
567600
// credentials Secret name) and the strategy parameters on driverMetadata.
568601
// Follows the MongoDB artifact pattern - Create persists status because the
569602
// core Backup type carries no status subresource.
603+
// errBucketArtifactNameCollision marks a Backup whose name already belongs to a
604+
// different run. A Backup carries no ownerRef to its BackupJob and outlives it,
605+
// so a reused BackupJob name finds a stale same-named Backup; the run must fail
606+
// rather than report success against that older snapshot.
607+
var errBucketArtifactNameCollision = errors.New("a Backup of this name already exists for a different run")
608+
570609
func (r *BackupJobReconciler) createBucketBackupArtifact(
571610
ctx context.Context,
572611
j *backupsv1alpha1.BackupJob,
@@ -638,6 +677,18 @@ func (r *BackupJobReconciler) createBucketBackupArtifact(
638677
if getErr := r.Get(ctx, types.NamespacedName{Namespace: backup.Namespace, Name: backup.Name}, existing); getErr != nil {
639678
return nil, getErr
640679
}
680+
// The repo prefix is scoped by BackupJob UID, so a Backup this same run
681+
// created points at repoPrefix (a genuine idempotent retry) while a stale
682+
// one from a prior run with the reused name points elsewhere. Returning
683+
// the latter would report Succeeded against the old snapshot and leave the
684+
// fresh copy this run wrote unreferenced in the shared repo bucket.
685+
existingPrefix := ""
686+
if snap, decErr := unmarshalBucketSnapshot(existing.Status.UnderlyingResources); decErr == nil && snap != nil {
687+
existingPrefix = snap.RepoPrefix
688+
}
689+
if existingPrefix != repoPrefix {
690+
return nil, fmt.Errorf("%w: Backup %q points at repo prefix %q, this run wrote %q", errBucketArtifactNameCollision, backup.Name, existingPrefix, repoPrefix)
691+
}
641692
return existing, nil
642693
}
643694
return backup, nil

‎internal/backupcontroller/bucketstrategy_controller_test.go‎

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -695,3 +695,90 @@ func TestReconcileBucketRestoreInPlaceVsToCopyJobArgs(t *testing.T) {
695695
}
696696
})
697697
}
698+
699+
func TestReconcileBucketFailsOnArtifactNameCollision(t *testing.T) {
700+
// A Backup carries no ownerRef to its BackupJob and outlives it, so a
701+
// BackupJob whose name is reused after the old one was deleted finds a stale
702+
// same-named Backup pointing at the previous run's UID-scoped prefix. The
703+
// mirror has already written a fresh copy under this run's prefix; returning
704+
// the stale Backup would report Succeeded against the old snapshot and strand
705+
// the fresh copy unreferenced. The run must fail instead.
706+
strategy := &strategyv1alpha1.Bucket{
707+
ObjectMeta: metav1.ObjectMeta{Name: "cozy-default-bucket"},
708+
Spec: strategyv1alpha1.BucketSpec{Template: strategyv1alpha1.BucketTemplate{
709+
Image: "controller:latest",
710+
Destination: strategyv1alpha1.BucketDestination{Bucket: "cozy-backups", Endpoint: "http://s3"},
711+
}},
712+
}
713+
resolved := &ResolvedBackupConfig{
714+
StrategyRef: corev1.TypedLocalObjectReference{
715+
APIGroup: stringPtr(strategyv1alpha1.GroupVersion.Group),
716+
Kind: strategyv1alpha1.BucketStrategyKind,
717+
Name: "cozy-default-bucket",
718+
},
719+
}
720+
now := metav1.Now()
721+
job := &backupsv1alpha1.BackupJob{
722+
ObjectMeta: metav1.ObjectMeta{Namespace: "tenant-x", Name: "manual-1", UID: "new-uid"},
723+
Spec: backupsv1alpha1.BackupJobSpec{
724+
ApplicationRef: corev1.TypedLocalObjectReference{Kind: bucketAppKind, Name: "web"},
725+
},
726+
Status: backupsv1alpha1.BackupJobStatus{StartedAt: &now, Phase: backupsv1alpha1.BackupJobPhaseRunning},
727+
}
728+
claim := &buckettypes.BucketClaim{
729+
ObjectMeta: metav1.ObjectMeta{Namespace: "tenant-x", Name: "bucket-web", UID: "claim-uid"},
730+
Spec: buckettypes.BucketClaimSpec{BucketClassName: "seaweedfs"},
731+
Status: buckettypes.BucketClaimStatus{BucketReady: true, BucketName: "app-bucket"},
732+
}
733+
access := bucketAccess("bucket-web-cozy-backup",
734+
map[string]string{managedByLabel: managedByValue},
735+
[]metav1.OwnerReference{{APIVersion: buckettypes.GroupVersion.String(), Kind: "BucketClaim", Name: "bucket-web", UID: "claim-uid"}},
736+
buckettypes.BucketAccessSpec{
737+
BucketClaimName: "bucket-web",
738+
BucketAccessClassName: "seaweedfs-readonly",
739+
Protocol: bucketProtocolS3,
740+
CredentialsSecretName: "bucket-web-cozy-backup",
741+
})
742+
access.Status.AccessGranted = true
743+
completedMirror := &batchv1.Job{
744+
ObjectMeta: metav1.ObjectMeta{
745+
Namespace: "tenant-x",
746+
Name: jobNameForBackupJob(job),
747+
Labels: map[string]string{
748+
backupsv1alpha1.OwningJobNameLabel: job.Name,
749+
backupsv1alpha1.OwningJobNamespaceLabel: job.Namespace,
750+
},
751+
},
752+
Status: batchv1.JobStatus{Conditions: []batchv1.JobCondition{
753+
{Type: batchv1.JobComplete, Status: corev1.ConditionTrue},
754+
}},
755+
}
756+
staleRaw, err := marshalBucketSnapshot(bucketBackupSnapshot{
757+
Kind: bucketSnapshotKind,
758+
RepoBucket: "cozy-backups",
759+
RepoPrefix: "manual-1-old-uid/", // a prior run's prefix, not this run's
760+
})
761+
if err != nil {
762+
t.Fatalf("marshalBucketSnapshot: %v", err)
763+
}
764+
staleBackup := &backupsv1alpha1.Backup{
765+
ObjectMeta: metav1.ObjectMeta{Namespace: "tenant-x", Name: "manual-1"},
766+
Status: backupsv1alpha1.BackupStatus{UnderlyingResources: staleRaw},
767+
}
768+
769+
r, _ := newBucketReconcileEnv(t, newBucketApp("web", "tenant-x"),
770+
clientfake.NewClientBuilder().WithObjects(job, strategy, claim, access, completedMirror, staleBackup))
771+
if _, err := r.reconcileBucket(context.Background(), job, resolved); err != nil {
772+
t.Fatalf("reconcileBucket: %v", err)
773+
}
774+
updated := &backupsv1alpha1.BackupJob{}
775+
if err := r.Get(context.Background(), client.ObjectKeyFromObject(job), updated); err != nil {
776+
t.Fatalf("get backupjob: %v", err)
777+
}
778+
if updated.Status.Phase != backupsv1alpha1.BackupJobPhaseFailed {
779+
t.Fatalf("phase = %q, want Failed (stale same-named Backup must not be reused)", updated.Status.Phase)
780+
}
781+
if updated.Status.BackupRef != nil {
782+
t.Fatalf("BackupRef = %+v, want nil (must not point at the stale snapshot)", updated.Status.BackupRef)
783+
}
784+
}

‎internal/s3mirror/mirror_test.go‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -168,6 +168,37 @@ func TestMirrorZeroObjectSourceDoesNotWipe(t *testing.T) {
168168
}
169169
}
170170

171+
func TestMirrorToCopyKeepsUnrelatedDestinationObjects(t *testing.T) {
172+
// A to-copy restore (deleteExtraneous=false) merges the snapshot into the
173+
// target: objects the snapshot names are (over)written, but objects the
174+
// target already holds and the snapshot does not name must survive. This is
175+
// the merge half of the contract the in-place path inverts, so it is pinned
176+
// separately from the delete-extraneous purge.
177+
s := newFakeStore()
178+
s.seed("repo", "p/a", fakeObject{data: []byte("A")})
179+
s.seed("target", "a", fakeObject{data: []byte("stale")}) // named by the snapshot
180+
s.seed("target", "keep", fakeObject{data: []byte("mine")}) // not named; must survive
181+
182+
from := copySide{store: s, bucket: "repo", prefix: "p/"}
183+
to := copySide{store: s, bucket: "target", prefix: ""}
184+
copied, err := mirror(context.Background(), from, to, false, false, true, false)
185+
if err != nil {
186+
t.Fatalf("mirror: %v", err)
187+
}
188+
if copied != 1 {
189+
t.Fatalf("copied = %d, want 1", copied)
190+
}
191+
if got := strings.Join(s.keys("target"), ","); got != "a,keep" {
192+
t.Fatalf("target keys = %q, want \"a,keep\" (unnamed object must not be purged)", got)
193+
}
194+
if string(s.buckets["target"]["a"].data) != "A" {
195+
t.Fatalf("target/a = %q, want the snapshot bytes \"A\" merged in", s.buckets["target"]["a"].data)
196+
}
197+
if string(s.buckets["target"]["keep"].data) != "mine" {
198+
t.Fatalf("target/keep = %q, want it left untouched", s.buckets["target"]["keep"].data)
199+
}
200+
}
201+
171202
func TestMirrorResumeSkipsAlreadyPresent(t *testing.T) {
172203
// A retried restore whose destination already holds every object with
173204
// identical content (matching ETag) copies nothing (resume), and the

0 commit comments

Comments
 (0)