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