Skip to content

Commit 9ded5c5

Browse files
myasnikovdaniilmyasnikovdaniil
committed
fix(backups): fail a ClickHouse BackupJob whose Backup name is taken
A Backup outlives the BackupJob that made it, so a BackupJob created again under a name already used meets the earlier run's Backup when its own Job completes. That happens by hand, and from a Plan reconciled again inside its starting deadline, since a Plan names each BackupJob after its schedule slot. The Altinity driver took the earlier Backup as its result: the BackupJob turned Succeeded with a BackupRef recording the earlier run's archive, the archive this run wrote was recorded on nothing, and restoring what the BackupJob reported brought back the older data. An existing Backup is now taken as this run's only when it records this run's archive, which is what an earlier reconcile of the same run leaves when its status write is lost. Any other fails the BackupJob, naming both archives, and leaves the earlier Backup untouched. Assisted-by: LLM Signed-off-by: Myasnikov Daniil <[email protected]> (cherry picked from commit d7ae34d) Signed-off-by: cozystack-ci[bot] <274107086+cozystack-ci[bot]@users.noreply.github.com>
1 parent 1f9e078 commit 9ded5c5

2 files changed

Lines changed: 121 additions & 0 deletions

File tree

‎internal/backupcontroller/altinitystrategy_controller.go‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -509,6 +509,13 @@ func (r *BackupJobReconciler) createAltinityBackupArtifact(
509509
if getErr := r.Get(ctx, types.NamespacedName{Namespace: backup.Namespace, Name: backup.Name}, existing); getErr != nil {
510510
return nil, getErr
511511
}
512+
// A Backup outlives its BackupJob, so a BackupJob re-created under a
513+
// used name meets the earlier run's Backup here. Only one recording
514+
// this run's archive was created by an earlier reconcile of this run.
515+
if got := existing.Spec.DriverMetadata[altinityArchiveMetaKey]; got != archive {
516+
return nil, fmt.Errorf("Backup %s/%s already exists and records archive %q, not %q that this run wrote",
517+
backup.Namespace, backup.Name, got, archive)
518+
}
512519
return existing, nil
513520
}
514521
return backup, nil

‎internal/backupcontroller/altinitystrategy_controller_test.go‎

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package backupcontroller
33

44
import (
55
"context"
6+
"strings"
67
"testing"
78
"time"
89

@@ -861,6 +862,119 @@ func TestReconcileAltinity_RecordsNoArchiveTheJobWasNotGiven(t *testing.T) {
861862
}
862863
}
863864

865+
// TestReconcileAltinity_BackupNameAlreadyTaken covers a completed Job whose
866+
// Backup name is already in use. A Backup outlives its BackupJob, so a
867+
// BackupJob re-created under an earlier run's name finds that run's Backup,
868+
// and claiming it would point the new run at an archive it did not write.
869+
// A Backup recording this run's own archive is the one an earlier reconcile
870+
// created before its status write was lost, and is adopted.
871+
func TestReconcileAltinity_BackupNameAlreadyTaken(t *testing.T) {
872+
const thisRun = "clickhouse-ch-test-20260305T050607Z"
873+
cases := []struct {
874+
name string
875+
existing string // archive the existing Backup records; empty for none
876+
wantSucceeded bool
877+
}{
878+
{name: "earlier run's Backup records another archive", existing: "clickhouse-ch-test-20260304T050607Z"},
879+
{name: "earlier run's Backup records no archive", existing: ""},
880+
{name: "this run's Backup from a reconcile whose status write was lost", existing: thisRun, wantSucceeded: true},
881+
}
882+
for _, tc := range cases {
883+
t.Run(tc.name, func(t *testing.T) {
884+
app := newAltinityClickHouseApp("ch-test", "tenant-test")
885+
strategy := newAltinityArchiveStrategy("clickhouse-strategy")
886+
startedAt := metav1.NewTime(time.Date(2026, 3, 5, 5, 6, 7, 0, time.UTC))
887+
backupJob := &backupsv1alpha1.BackupJob{
888+
ObjectMeta: metav1.ObjectMeta{Name: "test-bj", Namespace: "tenant-test"},
889+
Spec: backupsv1alpha1.BackupJobSpec{
890+
ApplicationRef: newAltinityClickHouseRef("ch-test"),
891+
BackupClassName: "clickhouse-backup",
892+
},
893+
Status: backupsv1alpha1.BackupJobStatus{StartedAt: &startedAt, Phase: backupsv1alpha1.BackupJobPhaseRunning},
894+
}
895+
driverMD := map[string]string{altinityParamPrefix + "bucketName": "ch-bucket"}
896+
if tc.existing != "" {
897+
driverMD[altinityArchiveMetaKey] = tc.existing
898+
}
899+
existing := &backupsv1alpha1.Backup{
900+
ObjectMeta: metav1.ObjectMeta{Name: backupJob.Name, Namespace: backupJob.Namespace},
901+
Spec: backupsv1alpha1.BackupSpec{
902+
ApplicationRef: newAltinityClickHouseRef("ch-test"),
903+
StrategyRef: corev1.TypedLocalObjectReference{
904+
APIGroup: new(strategyv1alpha1.GroupVersion.Group),
905+
Kind: strategyv1alpha1.AltinityStrategyKind,
906+
Name: "clickhouse-strategy",
907+
},
908+
TakenAt: metav1.NewTime(time.Date(2026, 3, 4, 5, 6, 22, 0, time.UTC)),
909+
DriverMetadata: driverMD,
910+
},
911+
}
912+
resolved := &ResolvedBackupConfig{
913+
StrategyRef: corev1.TypedLocalObjectReference{
914+
APIGroup: new(strategyv1alpha1.GroupVersion.Group),
915+
Kind: strategyv1alpha1.AltinityStrategyKind,
916+
Name: "clickhouse-strategy",
917+
},
918+
Parameters: map[string]string{"bucketName": "ch-bucket"},
919+
}
920+
r, _, _ := newAltinityTestEnv(t, app,
921+
clientfake.NewClientBuilder().WithObjects(backupJob, strategy, existing))
922+
ctx := context.Background()
923+
924+
if _, err := r.reconcileAltinity(ctx, backupJob, resolved); err != nil {
925+
t.Fatalf("reconcileAltinity() creating the Job: %v", err)
926+
}
927+
job := &batchv1.Job{}
928+
if err := r.Get(ctx, client.ObjectKey{Namespace: "tenant-test", Name: jobNameForBackupJob(backupJob)}, job); err != nil {
929+
t.Fatalf("get backup Job: %v", err)
930+
}
931+
if got := job.Spec.Template.Spec.Containers[0].Args[1]; got != "--archive="+thisRun {
932+
t.Fatalf("backup Pod was rendered with %q, want --archive=%s", got, thisRun)
933+
}
934+
job.Status.Conditions = []batchv1.JobCondition{{Type: batchv1.JobComplete, Status: corev1.ConditionTrue}}
935+
if err := r.Status().Update(ctx, job); err != nil {
936+
t.Fatalf("mark backup Job complete: %v", err)
937+
}
938+
if err := r.Get(ctx, client.ObjectKeyFromObject(backupJob), backupJob); err != nil {
939+
t.Fatalf("refresh BackupJob: %v", err)
940+
}
941+
if _, err := r.reconcileAltinity(ctx, backupJob, resolved); err != nil {
942+
t.Fatalf("reconcileAltinity() on the completed Job: %v", err)
943+
}
944+
945+
updated := &backupsv1alpha1.BackupJob{}
946+
if err := r.Get(ctx, client.ObjectKeyFromObject(backupJob), updated); err != nil {
947+
t.Fatalf("get BackupJob: %v", err)
948+
}
949+
if tc.wantSucceeded {
950+
if updated.Status.Phase != backupsv1alpha1.BackupJobPhaseSucceeded {
951+
t.Errorf("phase = %q (%s), want Succeeded", updated.Status.Phase, updated.Status.Message)
952+
}
953+
if updated.Status.BackupRef == nil || updated.Status.BackupRef.Name != existing.Name {
954+
t.Errorf("BackupRef = %v, want %s", updated.Status.BackupRef, existing.Name)
955+
}
956+
} else {
957+
if updated.Status.Phase != backupsv1alpha1.BackupJobPhaseFailed {
958+
t.Errorf("phase = %q, want Failed: the run would claim a Backup recording %q instead of %q", updated.Status.Phase, tc.existing, thisRun)
959+
}
960+
if updated.Status.BackupRef != nil {
961+
t.Errorf("BackupRef = %v, want none", updated.Status.BackupRef)
962+
}
963+
if !strings.Contains(updated.Status.Message, thisRun) {
964+
t.Errorf("failure message %q does not name the archive this run wrote", updated.Status.Message)
965+
}
966+
}
967+
kept := &backupsv1alpha1.Backup{}
968+
if err := r.Get(ctx, client.ObjectKeyFromObject(existing), kept); err != nil {
969+
t.Fatalf("get existing Backup: %v", err)
970+
}
971+
if got := kept.Spec.DriverMetadata[altinityArchiveMetaKey]; got != tc.existing {
972+
t.Errorf("existing Backup now records archive %q, want %q left in place", got, tc.existing)
973+
}
974+
})
975+
}
976+
}
977+
864978
func TestReconcileAltinityRestore_RendersTheBackupsArchive(t *testing.T) {
865979
cases := []struct {
866980
name string

0 commit comments

Comments
 (0)