Skip to content

Commit 47b4971

Browse files
myasnikovdaniilmyasnikovdaniil
committed
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 <[email protected]>
1 parent 4087218 commit 47b4971

4 files changed

Lines changed: 523 additions & 22 deletions

File tree

‎hack/altinity-backup-strategy.bats‎

Lines changed: 209 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,209 @@
1+
#!/usr/bin/env bats
2+
# Behavioural tests for the shipped Altinity (ClickHouse) backup/restore
3+
# strategy script, extracted from the rendered cozy-default-altinity Strategy
4+
# CR and EXECUTED against a stubbed clickhouse-backup HTTP API, with the real
5+
# jq doing the archive selection. Which archive a restore reads back is
6+
# decided by that selection over list/remote, so it is exercised against
7+
# several archives rather than string-matched.
8+
#
9+
# The script runs with `bash`, not `sh`, for the reason given in
10+
# hack/redis-backup-strategy.bats: it opens `set -euo pipefail`, which the
11+
# production image's busybox ash supports and the CI runner's dash rejects.
12+
13+
load test_helper
14+
15+
CHART=packages/system/backupstrategy-controller
16+
17+
# Render the shipped strategy's in-Pod script into $1 and substitute the
18+
# controller-render placeholders the way the driver fills them: $2 mode, $3
19+
# app (release and source application), $4 the archive name recorded on the
20+
# Backup (empty for a Backup that records none), $5 the Backup name, $6 its
21+
# takenAt in the archive-name layout. An empty render fails early, as does a
22+
# placeholder left unsubstituted.
23+
render_script() {
24+
out=$1; mode=$2; app=$3; archive=$4; backup=$5; takenat=$6
25+
helm template t "$CHART" -n cozy-backup-controller \
26+
--set backupStorage.bucketNameOverride=test-bucket 2>/dev/null \
27+
| yq eval 'select(.kind == "Altinity") | .spec.template.spec.containers[0].args[0]' - \
28+
| sed -e "s#{{ .Mode }}#${mode}#g" \
29+
-e "s#{{ .Release.Name }}#${app}#g" \
30+
-e "s#{{ .Release.Namespace }}#tenant-x#g" \
31+
-e "s#{{ .Backup.ApplicationRef.Name }}#${app}#g" \
32+
-e "s#{{ .Backup.Name }}#${backup}#g" \
33+
-e "s#{{ .ArchiveName | default \"\" }}#${archive}#g" \
34+
-e "s#{{ .Backup.TakenAt | default \"\" }}#${takenat}#g" > "$out"
35+
[ -s "$out" ] || return 1
36+
if grep -q '{{' "$out"; then
37+
echo "rendered script still holds a controller placeholder" >&2
38+
return 1
39+
fi
40+
}
41+
42+
# Stub the clickhouse-backup API behind curl, first on PATH. list/remote
43+
# serves $state/remote; actions serves $state/actions plus one success row per
44+
# operation the script has issued, and after its first read swaps in
45+
# $state/actions.next when a test seeds one (an operation that finishes while
46+
# the script waits). Every request is logged to $state/curl.log. sleep is
47+
# stubbed to fail after a few calls, so a script that never sees its
48+
# operation finish fails the test instead of hanging it.
49+
make_stubs() {
50+
bin=$1; state=$2
51+
mkdir -p "$bin"
52+
: > "$state/remote"
53+
: > "$state/actions"
54+
cat > "$bin/curl" <<STUB
55+
#!/usr/bin/env bash
56+
set -eu
57+
method=GET; url=""
58+
for a in "\$@"; do
59+
case "\$a" in POST) method=POST ;; http://*) url="\$a" ;; esac
60+
done
61+
echo "\$method \$url" >> "$state/curl.log"
62+
case "\$method \$url" in
63+
"GET "*/backup/status) echo '{}' ;;
64+
"GET "*/backup/list/remote) cat "$state/remote" ;;
65+
"GET "*/backup/actions)
66+
cat "$state/actions"
67+
if [ -f "$state/actions.done" ]; then cat "$state/actions.done"; fi
68+
if [ -f "$state/actions.next" ]; then mv "$state/actions.next" "$state/actions"; fi
69+
;;
70+
"POST "*/backup/create_remote*)
71+
name=\${url#*name=}; name=\${name%%&*}
72+
printf '{"command":"create_remote --resume %s","status":"success","start":"2099-01-01 00:00:00"}\n' "\$name" >> "$state/actions.done"
73+
;;
74+
"POST "*/backup/restore_remote/*)
75+
printf '{"command":"restore_remote %s","status":"success","start":"2099-01-01 00:00:00"}\n' "\${url##*/}" >> "$state/actions.done"
76+
;;
77+
*) echo "unexpected request: \$method \$url" >&2; exit 22 ;;
78+
esac
79+
STUB
80+
cat > "$bin/sleep" <<STUB
81+
#!/usr/bin/env bash
82+
n=\$(cat "$state/sleeps" 2>/dev/null || echo 0)
83+
echo \$((n + 1)) > "$state/sleeps"
84+
if [ "\$n" -ge 3 ]; then echo "sleep budget spent: the operation never finished" >&2; exit 1; fi
85+
STUB
86+
chmod 0755 "$bin/curl" "$bin/sleep"
87+
export API_USERNAME=u API_PASSWORD=p
88+
}
89+
90+
# One list/remote row per argument, "name" or "name|desc"; created mirrors
91+
# the name's timestamp, which is the order the pre-fix selection sorted by.
92+
remote_rows() {
93+
for row in "$@"; do
94+
name=${row%%|*}; desc="tar, regular"
95+
if [ "$row" != "$name" ]; then desc=${row#*|}; fi
96+
ts=${name##*-}
97+
printf '{"name":"%s","created":"%s","location":"remote","required":"","desc":"%s"}\n' "$name" "$ts" "$desc"
98+
done
99+
}
100+
101+
@test "an empty render fails the fixture instead of passing as a no-op" {
102+
tmp=$(mktemp -d)
103+
CHART=$tmp/no-such-chart
104+
if render_script "$tmp/s.sh" restore myapp "" bkp 20260304T050622Z 2>/dev/null; then
105+
echo "FAIL: render_script reported success with no chart"
106+
false
107+
fi
108+
}
109+
110+
@test "restore reads back the archive the Backup recorded, not the newest one" {
111+
tmp=$(mktemp -d)
112+
make_stubs "$tmp/bin" "$tmp"
113+
render_script "$tmp/s.sh" restore myapp clickhouse-myapp-20260304T050607Z bkp-a 20260304T050622Z
114+
remote_rows clickhouse-myapp-20260304T040000Z clickhouse-myapp-20260304T050607Z \
115+
clickhouse-myapp-20260304T060000Z clickhouse-myapp-20260304T070000Z > "$tmp/remote"
116+
PATH="$tmp/bin:$PATH" bash "$tmp/s.sh"
117+
grep -qxF 'POST http://chi-clickhouse-myapp-clickhouse-0-0.tenant-x.svc:7171/backup/restore_remote/clickhouse-myapp-20260304T050607Z' "$tmp/curl.log"
118+
[ "$(grep -c '/backup/restore_remote/' "$tmp/curl.log")" -eq 1 ]
119+
}
120+
121+
@test "restore fails naming the recorded archive when remote storage lacks it" {
122+
tmp=$(mktemp -d)
123+
make_stubs "$tmp/bin" "$tmp"
124+
render_script "$tmp/s.sh" restore myapp clickhouse-myapp-20260304T050607Z bkp-a 20260304T050622Z
125+
remote_rows clickhouse-myapp-20260304T040000Z clickhouse-myapp-20260304T060000Z > "$tmp/remote"
126+
out=""
127+
if out=$(PATH="$tmp/bin:$PATH" bash "$tmp/s.sh" 2>&1); then
128+
echo "FAIL: restore succeeded without the recorded archive: $out"
129+
false
130+
fi
131+
printf '%s\n' "$out" | grep -qF 'archive clickhouse-myapp-20260304T050607Z recorded on Backup bkp-a is not in remote storage'
132+
if grep -qF '/backup/restore_remote/' "$tmp/curl.log"; then
133+
echo "FAIL: restored another archive: $(cat "$tmp/curl.log")"
134+
false
135+
fi
136+
}
137+
138+
@test "restore of a Backup without a recorded archive takes the newest one stamped by its takenAt, and warns" {
139+
tmp=$(mktemp -d)
140+
make_stubs "$tmp/bin" "$tmp"
141+
render_script "$tmp/s.sh" restore myapp "" bkp-a 20260304T050800Z
142+
# Each decoy sorts after the expected archive and is ruled out by one
143+
# filter: broken, not a bare timestamp after the prefix, or stamped after
144+
# takenAt.
145+
remote_rows clickhouse-myapp-20260304T040000Z clickhouse-myapp-20260304T050607Z \
146+
"clickhouse-myapp-20260304T050700Z|broken (can't stat metadata.json)" \
147+
clickhouse-myapp-20260304T050710Z-copy \
148+
clickhouse-myapp-20260304T060000Z > "$tmp/remote"
149+
out=$(PATH="$tmp/bin:$PATH" bash "$tmp/s.sh" 2>&1)
150+
grep -qxF 'POST http://chi-clickhouse-myapp-clickhouse-0-0.tenant-x.svc:7171/backup/restore_remote/clickhouse-myapp-20260304T050607Z' "$tmp/curl.log"
151+
[ "$(grep -c '/backup/restore_remote/' "$tmp/curl.log")" -eq 1 ]
152+
printf '%s\n' "$out" | grep -qF 'WARNING: Backup bkp-a records no archive name'
153+
}
154+
155+
@test "restore of a Backup without a recorded archive fails when no archive is stamped by its takenAt" {
156+
tmp=$(mktemp -d)
157+
make_stubs "$tmp/bin" "$tmp"
158+
render_script "$tmp/s.sh" restore myapp "" bkp-a 20260304T050800Z
159+
remote_rows clickhouse-myapp-20260304T060000Z clickhouse-myapp-20260304T070000Z > "$tmp/remote"
160+
out=""
161+
if out=$(PATH="$tmp/bin:$PATH" bash "$tmp/s.sh" 2>&1); then
162+
echo "FAIL: restore succeeded with every archive newer than takenAt: $out"
163+
false
164+
fi
165+
printf '%s\n' "$out" | grep -qF 'no archive of myapp stamped no later than 20260304T050800Z'
166+
if grep -qF '/backup/restore_remote/' "$tmp/curl.log"; then
167+
echo "FAIL: restored an archive newer than takenAt: $(cat "$tmp/curl.log")"
168+
false
169+
fi
170+
}
171+
172+
@test "backup writes the archive under the name the controller chose" {
173+
tmp=$(mktemp -d)
174+
make_stubs "$tmp/bin" "$tmp"
175+
render_script "$tmp/s.sh" backup myapp clickhouse-myapp-20260304T050607Z "" ""
176+
PATH="$tmp/bin:$PATH" bash "$tmp/s.sh"
177+
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"
178+
}
179+
180+
@test "a retried backup waits on the attempt the sidecar is still running" {
181+
tmp=$(mktemp -d)
182+
make_stubs "$tmp/bin" "$tmp"
183+
render_script "$tmp/s.sh" backup myapp clickhouse-myapp-20260304T050607Z "" ""
184+
printf '{"command":"create_remote --resume clickhouse-myapp-20260304T050607Z","status":"in progress","start":"2026-03-04 05:06:08"}\n' > "$tmp/actions"
185+
printf '{"command":"create_remote --resume clickhouse-myapp-20260304T050607Z","status":"success","start":"2026-03-04 05:06:08"}\n' > "$tmp/actions.next"
186+
PATH="$tmp/bin:$PATH" bash "$tmp/s.sh"
187+
# The sidecar answers a second operation with 423 while one runs.
188+
if grep -qF 'POST ' "$tmp/curl.log"; then
189+
echo "FAIL: started another operation over the running one: $(cat "$tmp/curl.log")"
190+
false
191+
fi
192+
}
193+
194+
@test "a retried backup after a failed attempt resumes the same archive" {
195+
tmp=$(mktemp -d)
196+
make_stubs "$tmp/bin" "$tmp"
197+
render_script "$tmp/s.sh" backup myapp clickhouse-myapp-20260304T050607Z "" ""
198+
printf '{"command":"create_remote clickhouse-myapp-20260304T050607Z","status":"error","start":"2026-03-04 05:06:08","error":"S3 PutObject 403"}\n' > "$tmp/actions"
199+
PATH="$tmp/bin:$PATH" bash "$tmp/s.sh"
200+
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"
201+
}
202+
203+
@test "backup falls back to a timestamped name when the controller supplies none" {
204+
tmp=$(mktemp -d)
205+
make_stubs "$tmp/bin" "$tmp"
206+
render_script "$tmp/s.sh" backup myapp "" "" ""
207+
PATH="$tmp/bin:$PATH" bash "$tmp/s.sh"
208+
grep -qE '^POST http://[^ ]*/backup/create_remote\?name=clickhouse-myapp-[0-9]{8}T[0-9]{6}Z&resume=true$' "$tmp/curl.log"
209+
}

‎internal/backupcontroller/altinitystrategy_controller.go‎

Lines changed: 60 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,9 @@
22
package backupcontroller
33

44
import (
5+
"bytes"
56
"context"
7+
"encoding/json"
68
"fmt"
79
"maps"
810
"strings"
@@ -43,6 +45,18 @@ const (
4345
// were in effect at backup time.
4446
altinityParamPrefix = "altinity.strategy.backups.cozystack.io/parameter/"
4547

48+
// altinityArchiveMetaKey records, on the Backup, the clickhouse-backup
49+
// archive its Job wrote, as named by altinityArchiveName. A restore reads
50+
// back exactly that archive: the newest one under the release's prefix
51+
// may belong to a later Backup.
52+
altinityArchiveMetaKey = "altinity.strategy.backups.cozystack.io/archive"
53+
54+
// altinityArchiveTimeLayout is the UTC timestamp that ends an archive
55+
// name. It is fixed-width, so names sharing a prefix sort chronologically
56+
// as plain strings, which is what a restore of a Backup that records no
57+
// archive relies on.
58+
altinityArchiveTimeLayout = "20060102T150405Z"
59+
4660
// Polling cadence for the Altinity Job lifecycle. Mirrors the CNPG /
4761
// Velero strategy cadence so behaviour is uniform across drivers.
4862
altinityPollInterval = 5 * time.Second
@@ -102,14 +116,37 @@ func backupParameters(b *backupsv1alpha1.Backup) map[string]string {
102116
return out
103117
}
104118

119+
// altinityArchiveName mints the archive name for one BackupJob from its
120+
// persisted StartedAt, so every reconcile and every Pod retry of the Job
121+
// writes the same archive. It keeps the clickhouse-<release>-<timestamp>
122+
// shape, because a Backup that records no archive is restored by the
123+
// timestamp in that shape.
124+
func altinityArchiveName(j *backupsv1alpha1.BackupJob) string {
125+
return fmt.Sprintf("clickhouse-%s-%s", j.Spec.ApplicationRef.Name, j.Status.StartedAt.UTC().Format(altinityArchiveTimeLayout))
126+
}
127+
128+
// altinityJobCarriesArchive reports whether the Job's Pod was rendered with
129+
// the archive name. A Job created by a controller that predates archive
130+
// names, or rendered from a strategy that names its own archives, wrote an
131+
// archive under another name, and recording this one would point the Backup
132+
// at an archive that does not exist.
133+
func altinityJobCarriesArchive(job *batchv1.Job, archive string) bool {
134+
spec, err := json.Marshal(job.Spec.Template.Spec)
135+
if err != nil {
136+
return false
137+
}
138+
return bytes.Contains(spec, []byte(archive))
139+
}
140+
105141
// renderAltinityTemplate runs the strategy's PodTemplateSpec through the
106142
// repository's text/template engine with the same context shape as the CNPG
107143
// driver: every string field is templated against the application object,
108-
// the release shorthand, the run mode, and the resolved parameters.
144+
// the release shorthand, the run mode, the archive name, and the resolved
145+
// parameters.
109146
func renderAltinityTemplate(
110147
tmpl corev1.PodTemplateSpec,
111148
app map[string]any,
112-
releaseName, releaseNamespace, mode string,
149+
releaseName, releaseNamespace, mode, archiveName string,
113150
parameters map[string]string,
114151
backup *backupsv1alpha1.Backup,
115152
) (*corev1.PodTemplateSpec, error) {
@@ -119,8 +156,9 @@ func renderAltinityTemplate(
119156
"Name": releaseName,
120157
"Namespace": releaseNamespace,
121158
},
122-
"Mode": mode,
123-
"Parameters": parameters,
159+
"Mode": mode,
160+
"ArchiveName": archiveName,
161+
"Parameters": parameters,
124162
}
125163
if backup != nil {
126164
// Expose ApplicationRef so restore strategies can reference the
@@ -143,6 +181,9 @@ func renderAltinityTemplate(
143181
"Kind": backup.Spec.ApplicationRef.Kind,
144182
"Name": backup.Spec.ApplicationRef.Name,
145183
},
184+
// In the archive-name layout, so a strategy can compare it with
185+
// the timestamp an archive name ends in.
186+
"TakenAt": backup.Spec.TakenAt.UTC().Format(altinityArchiveTimeLayout),
146187
}
147188
}
148189
return template.Template(&tmpl, ctxMap)
@@ -209,12 +250,14 @@ func (r *BackupJobReconciler) reconcileAltinity(ctx context.Context, j *backupsv
209250
return ctrl.Result{}, err
210251
}
211252

253+
archive := altinityArchiveName(j)
212254
rendered, err := renderAltinityTemplate(
213255
strategy.Spec.Template,
214256
app,
215257
j.Spec.ApplicationRef.Name,
216258
j.Namespace,
217259
altinityModeBackup,
260+
archive,
218261
resolved.Parameters,
219262
nil,
220263
)
@@ -239,7 +282,10 @@ func (r *BackupJobReconciler) reconcileAltinity(ctx context.Context, j *backupsv
239282
if j.Status.BackupRef != nil {
240283
return ctrl.Result{}, nil
241284
}
242-
artifact, err := r.createAltinityBackupArtifact(ctx, j, resolved)
285+
if !altinityJobCarriesArchive(batchJob, archive) {
286+
archive = ""
287+
}
288+
artifact, err := r.createAltinityBackupArtifact(ctx, j, resolved, archive)
243289
if err != nil {
244290
return r.markBackupJobFailed(ctx, j, fmt.Sprintf("failed to create Backup artifact: %v", err))
245291
}
@@ -411,16 +457,20 @@ func jobFailureMessage(j *batchv1.Job) string {
411457
}
412458

413459
// createAltinityBackupArtifact materialises a Cozystack Backup carrying the
414-
// strategy reference and the BackupClassStrategy parameters in effect at
415-
// backup time. Parameters round-trip via DriverMetadata under
416-
// altinityParamPrefix so a later RestoreJob can re-render the strategy
417-
// template against the same values.
460+
// strategy reference, the archive the Job wrote (when known) and the
461+
// BackupClassStrategy parameters in effect at backup time. Parameters
462+
// round-trip via DriverMetadata under altinityParamPrefix so a later
463+
// RestoreJob can re-render the strategy template against the same values.
418464
func (r *BackupJobReconciler) createAltinityBackupArtifact(
419465
ctx context.Context,
420466
j *backupsv1alpha1.BackupJob,
421467
resolved *ResolvedBackupConfig,
468+
archive string,
422469
) (*backupsv1alpha1.Backup, error) {
423470
driverMD := map[string]string{}
471+
if archive != "" {
472+
driverMD[altinityArchiveMetaKey] = archive
473+
}
424474
for k, v := range resolved.Parameters {
425475
driverMD[altinityParamPrefix+k] = v
426476
}
@@ -616,6 +666,7 @@ func (r *RestoreJobReconciler) reconcileAltinityRestore(ctx context.Context, res
616666
targetAppName,
617667
targetNamespace,
618668
altinityModeRestore,
669+
backup.Spec.DriverMetadata[altinityArchiveMetaKey],
619670
backupParameters(backup),
620671
backup,
621672
)

0 commit comments

Comments
 (0)