From 47b497194eab5187e09ecbebc1ca862ae229808d Mon Sep 17 00:00:00 2001 From: Myasnikov Daniil Date: Wed, 4 Nov 2026 11:10:42 +0000 Subject: [PATCH] fix(backups): restore the ClickHouse archive a Backup was taken as The Altinity strategy named each archive inside its Pod with date -u and recorded nothing on the Backup, so a restore could only guess: it listed the remote archives under the application's prefix and took the newest. A RestoreJob of an older Backup therefore restored whichever backup had run last. The controller now names the archive before it creates the backup Job, from the BackupJob's persisted StartedAt, so every reconcile and every retried Pod of the Job writes the same one. The name is recorded in the Backup's driverMetadata next to the strategy parameters, and a restore renders it into the strategy Pod, which restores exactly that archive and fails, naming it, when remote storage no longer holds it. The name is recorded only when the Job's Pod was given it, so a Job created by an earlier controller, or a strategy that names its own archives, leaves the Backup without one instead of pointing it at an archive that does not exist. Because a retried Pod reuses the name, it waits on an attempt the sidecar is still running instead of being refused with 423, and asks for resume, so a local or partial remote backup an earlier attempt left is completed rather than refused as existing. Backups taken before names were recorded stay restorable: the Pod picks the newest archive whose name is stamped no later than the Backup's takenAt, which is set only after the backup Job finished, and logs a warning that the choice is a best effort. Assisted-by: LLM Signed-off-by: Myasnikov Daniil --- hack/altinity-backup-strategy.bats | 209 +++++++++++++++++ .../altinitystrategy_controller.go | 69 +++++- .../altinitystrategy_controller_test.go | 210 +++++++++++++++++- .../templates/strategy-altinity-default.yaml | 57 ++++- 4 files changed, 523 insertions(+), 22 deletions(-) create mode 100644 hack/altinity-backup-strategy.bats diff --git a/hack/altinity-backup-strategy.bats b/hack/altinity-backup-strategy.bats new file mode 100644 index 000000000..234ef0b8b --- /dev/null +++ b/hack/altinity-backup-strategy.bats @@ -0,0 +1,209 @@ +#!/usr/bin/env bats +# Behavioural tests for the shipped Altinity (ClickHouse) backup/restore +# strategy script, extracted from the rendered cozy-default-altinity Strategy +# CR and EXECUTED against a stubbed clickhouse-backup HTTP API, with the real +# jq doing the archive selection. Which archive a restore reads back is +# decided by that selection over list/remote, so it is exercised against +# several archives rather than string-matched. +# +# The script runs with `bash`, not `sh`, for the reason given in +# hack/redis-backup-strategy.bats: it opens `set -euo pipefail`, which the +# production image's busybox ash supports and the CI runner's dash rejects. + +load test_helper + +CHART=packages/system/backupstrategy-controller + +# Render the shipped strategy's in-Pod script into $1 and substitute the +# controller-render placeholders the way the driver fills them: $2 mode, $3 +# app (release and source application), $4 the archive name recorded on the +# Backup (empty for a Backup that records none), $5 the Backup name, $6 its +# takenAt in the archive-name layout. An empty render fails early, as does a +# placeholder left unsubstituted. +render_script() { + out=$1; mode=$2; app=$3; archive=$4; backup=$5; takenat=$6 + helm template t "$CHART" -n cozy-backup-controller \ + --set backupStorage.bucketNameOverride=test-bucket 2>/dev/null \ + | yq eval 'select(.kind == "Altinity") | .spec.template.spec.containers[0].args[0]' - \ + | sed -e "s#{{ .Mode }}#${mode}#g" \ + -e "s#{{ .Release.Name }}#${app}#g" \ + -e "s#{{ .Release.Namespace }}#tenant-x#g" \ + -e "s#{{ .Backup.ApplicationRef.Name }}#${app}#g" \ + -e "s#{{ .Backup.Name }}#${backup}#g" \ + -e "s#{{ .ArchiveName | default \"\" }}#${archive}#g" \ + -e "s#{{ .Backup.TakenAt | default \"\" }}#${takenat}#g" > "$out" + [ -s "$out" ] || return 1 + if grep -q '{{' "$out"; then + echo "rendered script still holds a controller placeholder" >&2 + return 1 + fi +} + +# Stub the clickhouse-backup API behind curl, first on PATH. list/remote +# serves $state/remote; actions serves $state/actions plus one success row per +# operation the script has issued, and after its first read swaps in +# $state/actions.next when a test seeds one (an operation that finishes while +# the script waits). Every request is logged to $state/curl.log. sleep is +# stubbed to fail after a few calls, so a script that never sees its +# operation finish fails the test instead of hanging it. +make_stubs() { + bin=$1; state=$2 + mkdir -p "$bin" + : > "$state/remote" + : > "$state/actions" + cat > "$bin/curl" <> "$state/curl.log" +case "\$method \$url" in + "GET "*/backup/status) echo '{}' ;; + "GET "*/backup/list/remote) cat "$state/remote" ;; + "GET "*/backup/actions) + cat "$state/actions" + if [ -f "$state/actions.done" ]; then cat "$state/actions.done"; fi + if [ -f "$state/actions.next" ]; then mv "$state/actions.next" "$state/actions"; fi + ;; + "POST "*/backup/create_remote*) + name=\${url#*name=}; name=\${name%%&*} + printf '{"command":"create_remote --resume %s","status":"success","start":"2099-01-01 00:00:00"}\n' "\$name" >> "$state/actions.done" + ;; + "POST "*/backup/restore_remote/*) + printf '{"command":"restore_remote %s","status":"success","start":"2099-01-01 00:00:00"}\n' "\${url##*/}" >> "$state/actions.done" + ;; + *) echo "unexpected request: \$method \$url" >&2; exit 22 ;; +esac +STUB + cat > "$bin/sleep" </dev/null || echo 0) +echo \$((n + 1)) > "$state/sleeps" +if [ "\$n" -ge 3 ]; then echo "sleep budget spent: the operation never finished" >&2; exit 1; fi +STUB + chmod 0755 "$bin/curl" "$bin/sleep" + export API_USERNAME=u API_PASSWORD=p +} + +# One list/remote row per argument, "name" or "name|desc"; created mirrors +# the name's timestamp, which is the order the pre-fix selection sorted by. +remote_rows() { + for row in "$@"; do + name=${row%%|*}; desc="tar, regular" + if [ "$row" != "$name" ]; then desc=${row#*|}; fi + ts=${name##*-} + printf '{"name":"%s","created":"%s","location":"remote","required":"","desc":"%s"}\n' "$name" "$ts" "$desc" + done +} + +@test "an empty render fails the fixture instead of passing as a no-op" { + tmp=$(mktemp -d) + CHART=$tmp/no-such-chart + if render_script "$tmp/s.sh" restore myapp "" bkp 20260304T050622Z 2>/dev/null; then + echo "FAIL: render_script reported success with no chart" + false + fi +} + +@test "restore reads back the archive the Backup recorded, not the newest one" { + tmp=$(mktemp -d) + make_stubs "$tmp/bin" "$tmp" + render_script "$tmp/s.sh" restore myapp clickhouse-myapp-20260304T050607Z bkp-a 20260304T050622Z + remote_rows clickhouse-myapp-20260304T040000Z clickhouse-myapp-20260304T050607Z \ + clickhouse-myapp-20260304T060000Z clickhouse-myapp-20260304T070000Z > "$tmp/remote" + PATH="$tmp/bin:$PATH" bash "$tmp/s.sh" + grep -qxF 'POST http://chi-clickhouse-myapp-clickhouse-0-0.tenant-x.svc:7171/backup/restore_remote/clickhouse-myapp-20260304T050607Z' "$tmp/curl.log" + [ "$(grep -c '/backup/restore_remote/' "$tmp/curl.log")" -eq 1 ] +} + +@test "restore fails naming the recorded archive when remote storage lacks it" { + tmp=$(mktemp -d) + make_stubs "$tmp/bin" "$tmp" + render_script "$tmp/s.sh" restore myapp clickhouse-myapp-20260304T050607Z bkp-a 20260304T050622Z + remote_rows clickhouse-myapp-20260304T040000Z clickhouse-myapp-20260304T060000Z > "$tmp/remote" + out="" + if out=$(PATH="$tmp/bin:$PATH" bash "$tmp/s.sh" 2>&1); then + echo "FAIL: restore succeeded without the recorded archive: $out" + false + fi + printf '%s\n' "$out" | grep -qF 'archive clickhouse-myapp-20260304T050607Z recorded on Backup bkp-a is not in remote storage' + if grep -qF '/backup/restore_remote/' "$tmp/curl.log"; then + echo "FAIL: restored another archive: $(cat "$tmp/curl.log")" + false + fi +} + +@test "restore of a Backup without a recorded archive takes the newest one stamped by its takenAt, and warns" { + tmp=$(mktemp -d) + make_stubs "$tmp/bin" "$tmp" + render_script "$tmp/s.sh" restore myapp "" bkp-a 20260304T050800Z + # Each decoy sorts after the expected archive and is ruled out by one + # filter: broken, not a bare timestamp after the prefix, or stamped after + # takenAt. + remote_rows clickhouse-myapp-20260304T040000Z clickhouse-myapp-20260304T050607Z \ + "clickhouse-myapp-20260304T050700Z|broken (can't stat metadata.json)" \ + clickhouse-myapp-20260304T050710Z-copy \ + clickhouse-myapp-20260304T060000Z > "$tmp/remote" + out=$(PATH="$tmp/bin:$PATH" bash "$tmp/s.sh" 2>&1) + grep -qxF 'POST http://chi-clickhouse-myapp-clickhouse-0-0.tenant-x.svc:7171/backup/restore_remote/clickhouse-myapp-20260304T050607Z' "$tmp/curl.log" + [ "$(grep -c '/backup/restore_remote/' "$tmp/curl.log")" -eq 1 ] + printf '%s\n' "$out" | grep -qF 'WARNING: Backup bkp-a records no archive name' +} + +@test "restore of a Backup without a recorded archive fails when no archive is stamped by its takenAt" { + tmp=$(mktemp -d) + make_stubs "$tmp/bin" "$tmp" + render_script "$tmp/s.sh" restore myapp "" bkp-a 20260304T050800Z + remote_rows clickhouse-myapp-20260304T060000Z clickhouse-myapp-20260304T070000Z > "$tmp/remote" + out="" + if out=$(PATH="$tmp/bin:$PATH" bash "$tmp/s.sh" 2>&1); then + echo "FAIL: restore succeeded with every archive newer than takenAt: $out" + false + fi + printf '%s\n' "$out" | grep -qF 'no archive of myapp stamped no later than 20260304T050800Z' + if grep -qF '/backup/restore_remote/' "$tmp/curl.log"; then + echo "FAIL: restored an archive newer than takenAt: $(cat "$tmp/curl.log")" + false + fi +} + +@test "backup writes the archive under the name the controller chose" { + tmp=$(mktemp -d) + make_stubs "$tmp/bin" "$tmp" + render_script "$tmp/s.sh" backup myapp clickhouse-myapp-20260304T050607Z "" "" + PATH="$tmp/bin:$PATH" bash "$tmp/s.sh" + grep -qxF 'POST http://chi-clickhouse-myapp-clickhouse-0-0.tenant-x.svc:7171/backup/create_remote?name=clickhouse-myapp-20260304T050607Z&resume=true' "$tmp/curl.log" +} + +@test "a retried backup waits on the attempt the sidecar is still running" { + tmp=$(mktemp -d) + make_stubs "$tmp/bin" "$tmp" + render_script "$tmp/s.sh" backup myapp clickhouse-myapp-20260304T050607Z "" "" + printf '{"command":"create_remote --resume clickhouse-myapp-20260304T050607Z","status":"in progress","start":"2026-03-04 05:06:08"}\n' > "$tmp/actions" + printf '{"command":"create_remote --resume clickhouse-myapp-20260304T050607Z","status":"success","start":"2026-03-04 05:06:08"}\n' > "$tmp/actions.next" + PATH="$tmp/bin:$PATH" bash "$tmp/s.sh" + # The sidecar answers a second operation with 423 while one runs. + if grep -qF 'POST ' "$tmp/curl.log"; then + echo "FAIL: started another operation over the running one: $(cat "$tmp/curl.log")" + false + fi +} + +@test "a retried backup after a failed attempt resumes the same archive" { + tmp=$(mktemp -d) + make_stubs "$tmp/bin" "$tmp" + render_script "$tmp/s.sh" backup myapp clickhouse-myapp-20260304T050607Z "" "" + printf '{"command":"create_remote clickhouse-myapp-20260304T050607Z","status":"error","start":"2026-03-04 05:06:08","error":"S3 PutObject 403"}\n' > "$tmp/actions" + PATH="$tmp/bin:$PATH" bash "$tmp/s.sh" + grep -qxF 'POST http://chi-clickhouse-myapp-clickhouse-0-0.tenant-x.svc:7171/backup/create_remote?name=clickhouse-myapp-20260304T050607Z&resume=true' "$tmp/curl.log" +} + +@test "backup falls back to a timestamped name when the controller supplies none" { + tmp=$(mktemp -d) + make_stubs "$tmp/bin" "$tmp" + render_script "$tmp/s.sh" backup myapp "" "" "" + PATH="$tmp/bin:$PATH" bash "$tmp/s.sh" + grep -qE '^POST http://[^ ]*/backup/create_remote\?name=clickhouse-myapp-[0-9]{8}T[0-9]{6}Z&resume=true$' "$tmp/curl.log" +} diff --git a/internal/backupcontroller/altinitystrategy_controller.go b/internal/backupcontroller/altinitystrategy_controller.go index b96d571f0..f7f60bde8 100644 --- a/internal/backupcontroller/altinitystrategy_controller.go +++ b/internal/backupcontroller/altinitystrategy_controller.go @@ -2,7 +2,9 @@ package backupcontroller import ( + "bytes" "context" + "encoding/json" "fmt" "maps" "strings" @@ -43,6 +45,18 @@ const ( // were in effect at backup time. altinityParamPrefix = "altinity.strategy.backups.cozystack.io/parameter/" + // altinityArchiveMetaKey records, on the Backup, the clickhouse-backup + // archive its Job wrote, as named by altinityArchiveName. A restore reads + // back exactly that archive: the newest one under the release's prefix + // may belong to a later Backup. + altinityArchiveMetaKey = "altinity.strategy.backups.cozystack.io/archive" + + // altinityArchiveTimeLayout is the UTC timestamp that ends an archive + // name. It is fixed-width, so names sharing a prefix sort chronologically + // as plain strings, which is what a restore of a Backup that records no + // archive relies on. + altinityArchiveTimeLayout = "20060102T150405Z" + // Polling cadence for the Altinity Job lifecycle. Mirrors the CNPG / // Velero strategy cadence so behaviour is uniform across drivers. altinityPollInterval = 5 * time.Second @@ -102,14 +116,37 @@ func backupParameters(b *backupsv1alpha1.Backup) map[string]string { return out } +// altinityArchiveName mints the archive name for one BackupJob from its +// persisted StartedAt, so every reconcile and every Pod retry of the Job +// writes the same archive. It keeps the clickhouse-- +// shape, because a Backup that records no archive is restored by the +// timestamp in that shape. +func altinityArchiveName(j *backupsv1alpha1.BackupJob) string { + return fmt.Sprintf("clickhouse-%s-%s", j.Spec.ApplicationRef.Name, j.Status.StartedAt.UTC().Format(altinityArchiveTimeLayout)) +} + +// altinityJobCarriesArchive reports whether the Job's Pod was rendered with +// the archive name. A Job created by a controller that predates archive +// names, or rendered from a strategy that names its own archives, wrote an +// archive under another name, and recording this one would point the Backup +// at an archive that does not exist. +func altinityJobCarriesArchive(job *batchv1.Job, archive string) bool { + spec, err := json.Marshal(job.Spec.Template.Spec) + if err != nil { + return false + } + return bytes.Contains(spec, []byte(archive)) +} + // renderAltinityTemplate runs the strategy's PodTemplateSpec through the // repository's text/template engine with the same context shape as the CNPG // driver: every string field is templated against the application object, -// the release shorthand, the run mode, and the resolved parameters. +// the release shorthand, the run mode, the archive name, and the resolved +// parameters. func renderAltinityTemplate( tmpl corev1.PodTemplateSpec, app map[string]any, - releaseName, releaseNamespace, mode string, + releaseName, releaseNamespace, mode, archiveName string, parameters map[string]string, backup *backupsv1alpha1.Backup, ) (*corev1.PodTemplateSpec, error) { @@ -119,8 +156,9 @@ func renderAltinityTemplate( "Name": releaseName, "Namespace": releaseNamespace, }, - "Mode": mode, - "Parameters": parameters, + "Mode": mode, + "ArchiveName": archiveName, + "Parameters": parameters, } if backup != nil { // Expose ApplicationRef so restore strategies can reference the @@ -143,6 +181,9 @@ func renderAltinityTemplate( "Kind": backup.Spec.ApplicationRef.Kind, "Name": backup.Spec.ApplicationRef.Name, }, + // In the archive-name layout, so a strategy can compare it with + // the timestamp an archive name ends in. + "TakenAt": backup.Spec.TakenAt.UTC().Format(altinityArchiveTimeLayout), } } return template.Template(&tmpl, ctxMap) @@ -209,12 +250,14 @@ func (r *BackupJobReconciler) reconcileAltinity(ctx context.Context, j *backupsv return ctrl.Result{}, err } + archive := altinityArchiveName(j) rendered, err := renderAltinityTemplate( strategy.Spec.Template, app, j.Spec.ApplicationRef.Name, j.Namespace, altinityModeBackup, + archive, resolved.Parameters, nil, ) @@ -239,7 +282,10 @@ func (r *BackupJobReconciler) reconcileAltinity(ctx context.Context, j *backupsv if j.Status.BackupRef != nil { return ctrl.Result{}, nil } - artifact, err := r.createAltinityBackupArtifact(ctx, j, resolved) + if !altinityJobCarriesArchive(batchJob, archive) { + archive = "" + } + artifact, err := r.createAltinityBackupArtifact(ctx, j, resolved, archive) if err != nil { return r.markBackupJobFailed(ctx, j, fmt.Sprintf("failed to create Backup artifact: %v", err)) } @@ -411,16 +457,20 @@ func jobFailureMessage(j *batchv1.Job) string { } // createAltinityBackupArtifact materialises a Cozystack Backup carrying the -// strategy reference and the BackupClassStrategy parameters in effect at -// backup time. Parameters round-trip via DriverMetadata under -// altinityParamPrefix so a later RestoreJob can re-render the strategy -// template against the same values. +// strategy reference, the archive the Job wrote (when known) and the +// BackupClassStrategy parameters in effect at backup time. Parameters +// round-trip via DriverMetadata under altinityParamPrefix so a later +// RestoreJob can re-render the strategy template against the same values. func (r *BackupJobReconciler) createAltinityBackupArtifact( ctx context.Context, j *backupsv1alpha1.BackupJob, resolved *ResolvedBackupConfig, + archive string, ) (*backupsv1alpha1.Backup, error) { driverMD := map[string]string{} + if archive != "" { + driverMD[altinityArchiveMetaKey] = archive + } for k, v := range resolved.Parameters { driverMD[altinityParamPrefix+k] = v } @@ -616,6 +666,7 @@ func (r *RestoreJobReconciler) reconcileAltinityRestore(ctx context.Context, res targetAppName, targetNamespace, altinityModeRestore, + backup.Spec.DriverMetadata[altinityArchiveMetaKey], backupParameters(backup), backup, ) diff --git a/internal/backupcontroller/altinitystrategy_controller_test.go b/internal/backupcontroller/altinitystrategy_controller_test.go index 68f18fa28..7797842c0 100644 --- a/internal/backupcontroller/altinitystrategy_controller_test.go +++ b/internal/backupcontroller/altinitystrategy_controller_test.go @@ -4,6 +4,7 @@ package backupcontroller import ( "context" "testing" + "time" batchv1 "k8s.io/api/batch/v1" corev1 "k8s.io/api/core/v1" @@ -697,6 +698,7 @@ func TestBackupParameters_RoundTrip(t *testing.T) { DriverMetadata: map[string]string{ altinityParamPrefix + "bucketName": "b1", altinityParamPrefix + "endpoint": "https://s3", + altinityArchiveMetaKey: "clickhouse-ch-test-20260304T050607Z", "job.batch/name": "ignored", "": "ignored", }, @@ -706,7 +708,213 @@ func TestBackupParameters_RoundTrip(t *testing.T) { if got["bucketName"] != "b1" || got["endpoint"] != "https://s3" { t.Errorf("backupParameters = %v", got) } - if _, ok := got["job.batch/name"]; ok { + if len(got) != 2 { t.Errorf("non-parameter key leaked through: %v", got) } } + +// newAltinityArchiveStrategy renders the archive name and the Backup's +// takenAt where a test can read them back off the Job. +func newAltinityArchiveStrategy(name string) *strategyv1alpha1.Altinity { + s := newAltinityStrategy(name) + s.Spec.Template.Spec.Containers[0].Args = []string{ + "--mode={{ .Mode }}", + "--archive={{ .ArchiveName }}", + "--taken-at={{ .Backup.TakenAt }}", + "--bucket={{ .Parameters.bucketName }}", + } + return s +} + +func TestAltinityArchiveName_FromStartedAtInUTC(t *testing.T) { + startedAt := metav1.NewTime(time.Date(2026, 3, 4, 8, 6, 7, 890000000, time.FixedZone("UTC+3", 3*3600))) + j := &backupsv1alpha1.BackupJob{ + Spec: backupsv1alpha1.BackupJobSpec{ApplicationRef: newAltinityClickHouseRef("ch-test")}, + Status: backupsv1alpha1.BackupJobStatus{StartedAt: &startedAt}, + } + if got, want := altinityArchiveName(j), "clickhouse-ch-test-20260304T050607Z"; got != want { + t.Errorf("altinityArchiveName = %q, want %q", got, want) + } +} + +// TestReconcileAltinity_RecordsTheArchiveTheJobWasGiven drives one BackupJob +// through Job creation and completion in separate reconciles. The archive +// name has to come out of the persisted StartedAt both times: the Pod is +// told it on the first, the Backup records it on the second, and a restore +// later reads back exactly that archive. +func TestReconcileAltinity_RecordsTheArchiveTheJobWasGiven(t *testing.T) { + app := newAltinityClickHouseApp("ch-test", "tenant-test") + strategy := newAltinityArchiveStrategy("clickhouse-strategy") + startedAt := metav1.NewTime(time.Date(2026, 3, 4, 5, 6, 7, 0, time.UTC)) + backupJob := &backupsv1alpha1.BackupJob{ + ObjectMeta: metav1.ObjectMeta{Name: "test-bj", Namespace: "tenant-test"}, + Spec: backupsv1alpha1.BackupJobSpec{ + ApplicationRef: newAltinityClickHouseRef("ch-test"), + BackupClassName: "clickhouse-backup", + }, + Status: backupsv1alpha1.BackupJobStatus{StartedAt: &startedAt, Phase: backupsv1alpha1.BackupJobPhaseRunning}, + } + resolved := &ResolvedBackupConfig{ + StrategyRef: corev1.TypedLocalObjectReference{ + APIGroup: new(strategyv1alpha1.GroupVersion.Group), + Kind: strategyv1alpha1.AltinityStrategyKind, + Name: "clickhouse-strategy", + }, + Parameters: map[string]string{"bucketName": "ch-bucket"}, + } + r, _, _ := newAltinityTestEnv(t, app, clientfake.NewClientBuilder().WithObjects(backupJob, strategy)) + ctx := context.Background() + const want = "clickhouse-ch-test-20260304T050607Z" + + if _, err := r.reconcileAltinity(ctx, backupJob, resolved); err != nil { + t.Fatalf("reconcileAltinity() creating the Job: %v", err) + } + job := &batchv1.Job{} + if err := r.Get(ctx, client.ObjectKey{Namespace: "tenant-test", Name: jobNameForBackupJob(backupJob)}, job); err != nil { + t.Fatalf("get backup Job: %v", err) + } + if got := job.Spec.Template.Spec.Containers[0].Args[1]; got != "--archive="+want { + t.Fatalf("backup Pod was rendered with %q, want --archive=%s", got, want) + } + + job.Status.Conditions = []batchv1.JobCondition{{Type: batchv1.JobComplete, Status: corev1.ConditionTrue}} + if err := r.Status().Update(ctx, job); err != nil { + t.Fatalf("mark backup Job complete: %v", err) + } + if err := r.Get(ctx, client.ObjectKeyFromObject(backupJob), backupJob); err != nil { + t.Fatalf("refresh BackupJob: %v", err) + } + if _, err := r.reconcileAltinity(ctx, backupJob, resolved); err != nil { + t.Fatalf("reconcileAltinity() on the completed Job: %v", err) + } + + created := &backupsv1alpha1.Backup{} + if err := r.Get(ctx, client.ObjectKey{Namespace: "tenant-test", Name: backupJob.Name}, created); err != nil { + t.Fatalf("get Backup: %v", err) + } + if got := created.Spec.DriverMetadata[altinityArchiveMetaKey]; got != want { + t.Errorf("Backup records archive %q, want %q", got, want) + } + if got := created.Spec.DriverMetadata[altinityParamPrefix+"bucketName"]; got != "ch-bucket" { + t.Errorf("expected driverMetadata[parameter/bucketName]=ch-bucket, got %q", got) + } +} + +// TestReconcileAltinity_RecordsNoArchiveTheJobWasNotGiven covers a Job whose +// Pod never carried the driver's archive name: one created before the +// upgrade that introduced the names, or rendered from a strategy that names +// its own archives. Recording the name would send the restore after an +// archive nobody wrote; recording none leaves it the fallback for Backups +// without a name. +func TestReconcileAltinity_RecordsNoArchiveTheJobWasNotGiven(t *testing.T) { + app := newAltinityClickHouseApp("ch-test", "tenant-test") + strategy := newAltinityArchiveStrategy("clickhouse-strategy") + startedAt := metav1.NewTime(time.Date(2026, 3, 4, 5, 6, 7, 0, time.UTC)) + backupJob := &backupsv1alpha1.BackupJob{ + ObjectMeta: metav1.ObjectMeta{Name: "test-bj", Namespace: "tenant-test"}, + Spec: backupsv1alpha1.BackupJobSpec{ + ApplicationRef: newAltinityClickHouseRef("ch-test"), + BackupClassName: "clickhouse-backup", + }, + Status: backupsv1alpha1.BackupJobStatus{StartedAt: &startedAt, Phase: backupsv1alpha1.BackupJobPhaseRunning}, + } + completedK8sJob := &batchv1.Job{ + ObjectMeta: metav1.ObjectMeta{Name: jobNameForBackupJob(backupJob), Namespace: backupJob.Namespace}, + Spec: batchv1.JobSpec{ + Template: corev1.PodTemplateSpec{Spec: corev1.PodSpec{ + RestartPolicy: corev1.RestartPolicyNever, + Containers: []corev1.Container{{ + Name: "clickhouse-backup", + Image: "altinity/clickhouse-backup:test", + Args: []string{`target="clickhouse-ch-test-$(date -u +%Y%m%dT%H%M%SZ)"`}, + }}, + }}, + }, + Status: batchv1.JobStatus{ + Conditions: []batchv1.JobCondition{{Type: batchv1.JobComplete, Status: corev1.ConditionTrue}}, + }, + } + resolved := &ResolvedBackupConfig{ + StrategyRef: corev1.TypedLocalObjectReference{ + APIGroup: new(strategyv1alpha1.GroupVersion.Group), + Kind: strategyv1alpha1.AltinityStrategyKind, + Name: "clickhouse-strategy", + }, + Parameters: map[string]string{"bucketName": "ch-bucket"}, + } + r, _, _ := newAltinityTestEnv(t, app, + clientfake.NewClientBuilder().WithObjects(backupJob, strategy, completedK8sJob)) + ctx := context.Background() + + if _, err := r.reconcileAltinity(ctx, backupJob, resolved); err != nil { + t.Fatalf("reconcileAltinity() error = %v", err) + } + created := &backupsv1alpha1.Backup{} + if err := r.Get(ctx, client.ObjectKey{Namespace: "tenant-test", Name: backupJob.Name}, created); err != nil { + t.Fatalf("get Backup: %v", err) + } + if got, ok := created.Spec.DriverMetadata[altinityArchiveMetaKey]; ok { + t.Errorf("Backup records archive %q its Job was never given", got) + } + if got := created.Spec.DriverMetadata[altinityParamPrefix+"bucketName"]; got != "ch-bucket" { + t.Errorf("expected driverMetadata[parameter/bucketName]=ch-bucket, got %q", got) + } +} + +func TestReconcileAltinityRestore_RendersTheBackupsArchive(t *testing.T) { + cases := []struct { + name string + archive string // recorded on the Backup; empty for a Backup taken before names were recorded + wantArchive string + }{ + {name: "recorded archive", archive: "clickhouse-ch-test-20260304T050607Z", wantArchive: "--archive=clickhouse-ch-test-20260304T050607Z"}, + {name: "no recorded archive", archive: "", wantArchive: "--archive="}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + targetApp := newAltinityClickHouseApp("ch-test", "tenant-test") + strategy := newAltinityArchiveStrategy("clickhouse-strategy") + driverMD := map[string]string{altinityParamPrefix + "bucketName": "ch-bucket"} + if tc.archive != "" { + driverMD[altinityArchiveMetaKey] = tc.archive + } + now := metav1.Now() + backup := &backupsv1alpha1.Backup{ + ObjectMeta: metav1.ObjectMeta{Name: "ch-backup", Namespace: "tenant-test"}, + Spec: backupsv1alpha1.BackupSpec{ + ApplicationRef: newAltinityClickHouseRef("ch-test"), + StrategyRef: corev1.TypedLocalObjectReference{ + APIGroup: new(strategyv1alpha1.GroupVersion.Group), + Kind: strategyv1alpha1.AltinityStrategyKind, + Name: "clickhouse-strategy", + }, + TakenAt: metav1.NewTime(time.Date(2026, 3, 4, 8, 6, 22, 0, time.FixedZone("UTC+3", 3*3600))), + DriverMetadata: driverMD, + }, + } + restoreJob := &backupsv1alpha1.RestoreJob{ + ObjectMeta: metav1.ObjectMeta{Name: "rj", Namespace: "tenant-test"}, + Spec: backupsv1alpha1.RestoreJobSpec{BackupRef: corev1.LocalObjectReference{Name: backup.Name}}, + Status: backupsv1alpha1.RestoreJobStatus{StartedAt: &now, Phase: backupsv1alpha1.RestoreJobPhaseRunning}, + } + _, rr, _ := newAltinityTestEnv(t, targetApp, + clientfake.NewClientBuilder().WithObjects(strategy, backup, restoreJob)) + ctx := context.Background() + + if _, err := rr.reconcileAltinityRestore(ctx, restoreJob, backup); err != nil { + t.Fatalf("reconcileAltinityRestore: %v", err) + } + job := &batchv1.Job{} + if err := rr.Get(ctx, client.ObjectKey{Namespace: "tenant-test", Name: jobNameForRestoreJob(restoreJob)}, job); err != nil { + t.Fatalf("get restore Job: %v", err) + } + args := job.Spec.Template.Spec.Containers[0].Args + want := []string{"--mode=restore", tc.wantArchive, "--taken-at=20260304T050622Z", "--bucket=ch-bucket"} + for i, w := range want { + if i >= len(args) || args[i] != w { + t.Errorf("rendered restore args[%d]: want %q, got %q", i, w, args) + } + } + }) + } +} diff --git a/packages/system/backupstrategy-controller/templates/strategy-altinity-default.yaml b/packages/system/backupstrategy-controller/templates/strategy-altinity-default.yaml index 3397d6d0d..bb9917094 100644 --- a/packages/system/backupstrategy-controller/templates/strategy-altinity-default.yaml +++ b/packages/system/backupstrategy-controller/templates/strategy-altinity-default.yaml @@ -3,8 +3,8 @@ 1. Helm renders this file at chart install time. Anything inside `{{ ... }}` is interpreted by Helm. 2. The strategy controller renders the resulting Strategy CR - per-BackupJob, expanding `{{ .Application | .Release | .Mode }}` - against the live application object. + per-BackupJob, expanding `{{ .Application | .Release | .Mode | + .ArchiveName | .Backup }}` against the live application object. To emit a literal "{{ .Foo }}" that survives Helm, we pass it as the string argument of `printf` — Helm does not interpret inner template delimiters inside printf's quoted strings. Do NOT simplify by inlining @@ -51,23 +51,56 @@ spec: host="chi-clickhouse-{{ printf "{{ .Release.Name }}" }}-clickhouse-0-0.{{ printf "{{ .Release.Namespace }}" }}.svc" api="http://${host}:7171" auth="-u ${API_USERNAME}:${API_PASSWORD}" + # The archive the controller named for this BackupJob and records + # on its Backup. The default renders it empty, rather than as + # text/template's missing-key marker, when a controller that + # predates archive names renders this template during an upgrade. + archive="{{ printf `{{ .ArchiveName | default "" }}` }}" for _ in $(seq 1 60); do if curl -fsS ${auth} --max-time 2 "${api}/backup/status" >/dev/null 2>&1; then break; fi sleep 5 done if [ "{{ printf "{{ .Mode }}" }}" = "backup" ]; then prefix="clickhouse-{{ printf "{{ .Release.Name }}" }}-" - target="${prefix}$(date -u +%Y%m%dT%H%M%SZ)" - echo "create_remote ${target}" - curl -fsS ${auth} -X POST "${api}/backup/create_remote?name=${target}" >/dev/null + # Only that older controller leaves the name empty, and it + # records no name on the Backup either. + target="${archive:-${prefix}$(date -u +%Y%m%dT%H%M%SZ)}" + # A retried Pod gets the same name. Wait on an attempt the + # sidecar is still running, which would answer a new one with + # 423, and ask for resume, so a local or partial remote backup + # an earlier attempt left is resumed rather than refused as + # existing, whatever use_resumable_state the sidecar runs with. + state=$(curl -fsS ${auth} "${api}/backup/actions" \ + | jq -rs --arg suffix "${target}" \ + '[.[] | select((.command // "") | endswith($suffix))] | sort_by(.start) | last | .status // ""') + if [ "${state}" != "in progress" ]; then + echo "create_remote ${target}" + curl -fsS ${auth} -X POST "${api}/backup/create_remote?name=${target}&resume=true" >/dev/null + fi else - prefix="clickhouse-{{ printf "{{ .Backup.ApplicationRef.Name }}" }}-" - target=$(curl -fsS ${auth} "${api}/backup/list/remote" \ - | jq -rs --arg prefix "${prefix}" \ - '[.[] | select((.name // "") | startswith($prefix))] | sort_by(.created // "") | last.name // ""') - if [ -z "${target}" ] || [ "${target}" = "null" ]; then - echo "no remote backup found" >&2 - exit 1 + app="{{ printf "{{ .Backup.ApplicationRef.Name }}" }}" + backup="{{ printf "{{ .Backup.Name }}" }}" + prefix="clickhouse-${app}-" + remote=$(curl -fsS ${auth} "${api}/backup/list/remote") + if [ -n "${archive}" ]; then + if ! printf '%s\n' "${remote}" | jq -es --arg name "${archive}" 'any(.[]; .name == $name)' >/dev/null; then + echo "archive ${archive} recorded on Backup ${backup} is not in remote storage; not restoring any other archive" >&2 + exit 1 + fi + target="${archive}" + else + # takenAt is set after the backup Job finished, so the + # archive that Job wrote is stamped no later than it. + notafter="{{ printf `{{ .Backup.TakenAt | default "" }}` }}" + echo "WARNING: Backup ${backup} records no archive name; restoring the newest archive of ${app} stamped no later than its takenAt (${notafter}), a best-effort choice that may not be the archive this Backup was taken as" >&2 + target=$(printf '%s\n' "${remote}" | jq -rs --arg prefix "${prefix}" --arg notafter "${notafter}" ' + [.[] | select((.desc // "") | startswith("broken") | not) | .name // "" + | select(startswith($prefix) and (ltrimstr($prefix) | test("^[0-9]{8}T[0-9]{6}Z$")) and ltrimstr($prefix) <= $notafter)] + | sort | last // ""') + if [ -z "${target}" ]; then + echo "no archive of ${app} stamped no later than ${notafter} in remote storage" >&2 + exit 1 + fi fi echo "restore_remote ${target}" curl -fsS ${auth} -X POST "${api}/backup/restore_remote/${target}" >/dev/null