feat(kafka): add topic-data backup/restore example and e2e roundtrip - #3580

Actionable comments posted: 7

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@examples/backups/kafka/01-create-strategy.sh`:
- Around line 193-202: Update the Fidelity bullet in the Kafka backup README to
explicitly state that null-key records are replayed with the literal
four-character key “null” rather than a null key, in addition to the existing
partition-change caveat.
- Around line 120-124: Update the s3() wrapper to stop passing AWS credentials
via curl’s --user command-line argument; provide them through a curl
configuration file on stdin while preserving the existing SigV4, TLS, and
forwarded-argument behavior.

In `@examples/backups/kafka/05-create-backupjob.sh`:
- Around line 36-38: Persist the resolved backupRef name after retrieving it in
the backup job script, using the established variable or state mechanism
consumed by later steps. Update downstream steps 06 and 07 to reference the
cached backupRef instead of BACKUPJOB_NAME, while preserving the existing
empty-value validation and success logging.

In `@examples/backups/kafka/06-restore-in-place.sh`:
- Around line 37-38: Update the backupRef.name value in the restore
configuration to use the resolved Backup name from
BackupJob.status.backupRef.name rather than BACKUPJOB_NAME, preserving the
reference to the created Backup resource.

In `@examples/backups/kafka/07-restore-to-copy.sh`:
- Around line 50-51: Update the backupRef.name value in the restore manifest to
reference the resolved Backup resource name rather than BACKUPJOB_NAME, matching
the correction made in step 06.

In `@examples/backups/kafka/cleanup.sh`:
- Line 14: Update the cleanup command to delete the Backup using its resolved
Backup resource name rather than assuming it matches BACKUPJOB_NAME; reuse the
variable or lookup established by the script’s resolution flow, while preserving
the namespace and ignore-not-found options.

In `@hack/e2e-chainsaw/kafka/chainsaw-test.yaml`:
- Around line 200-205: Update the KAFKA_IMAGE extraction in the backup setup to
parse the multi-line STRIMZI_KAFKA_IMAGES version-to-image map by selecting one
explicit mapping and extracting its image value. Validate that the result is
exactly one non-empty image value, and retain the existing failure path when
parsing does not produce a valid image.

---

Nitpick comments:
In `@examples/backups/kafka/00-helpers.sh`:
- Around line 55-82: Update wait_for_field to accept an optional
terminal-failure value in addition to desired, and return failure immediately
when the current field matches it. Preserve existing success and timeout
behavior, and keep the argument optional so current callers remain compatible
while Kafka BackupJob/RestoreJob steps can pass Failed.

In `@examples/backups/kafka/03-create-bucket.sh`:
- Around line 62-68: Update the .bucket-info.env cache generation in the heredoc
to emit shell-safe quoted literals for every S3 value, using printf %q before
writing S3_ACCESS_KEY, S3_SECRET_KEY, S3_ENDPOINT, S3_REGION, and S3_BUCKET.
Keep create_s3_secret’s sourcing behavior unchanged while ensuring values
containing spaces, quotes, dollar signs, or semicolons remain data.

Verdict

LGTM with non-blocking notes.

This is example scripts plus one appended Chainsaw Test. Nothing here touches a chart, package, controller, CRD, or values contract, so the upgrade / SSA / CRD-transition / vendored-rename / config-matrix regression classes don't apply, and I checked that against the envelope: 0 charts, 0 Go packages, 0 packages/ files, and the "make generate produces no diff" claim holds up. The scripts fail closed on the paths that matter, cleanup.sh deletes the right Backup object (Backup.Name == BackupJob.Name, internal/backupcontroller/jobstrategy_controller.go:348), and the e2e actually asserts on record count rather than passing vacuously.

The notes below are accuracy and coverage refinements. None of them block merge. Two are worth calling out first, though: the ^_ over-match and the unvalidated explicit-topic list both let a backup example silently produce an incomplete backup that still reports success, which is the one failure a backup example must not teach. Both are one-line fixes.

Findings

[MINOR] 01-create-strategy.sh:167, grep -v '^_' silently drops legal user topics in the default "all topics" mode

Kafka's internal topics are __consumer_offsets and __transaction_state, both double-underscore. This filter also drops every single-underscore name, so a user topic like _schemas (Confluent Schema Registry) or _events holds user data yet is silently excluded from the documented "every non-internal topic" default. The comment at line 155 ("bookkeeping topics start with _") carries the same imprecision. Use grep -v '^__'.

[MINOR] 01-create-strategy.sh:156-157, an explicit topics: value naming a missing topic yields a successful empty backup

The explicit-TOPICS branch takes the list verbatim with no existence check, unlike the empty branch right below it, which is deliberately fail-closed. kafka-get-offsets.sh on a nonexistent topic prints nothing and exits 0, so the loop writes no manifest entry, uploads a near-empty tarball, and the BackupJob reports Succeeded. A typo in the BackupClass topics parameter produces a "backup" that restores nothing, and you find out at restore time. (I reasoned the exit-0-on-missing-topic behaviour from the CLI, I did not run it here.) A pre-flight check that each requested topic exists, failing loudly otherwise, closes this.

[MINOR] 01-create-strategy.sh:139-140, one S3 key per app: a later backup overwrites the earlier one, but old Backup CRs stay Ready

The object key is ${SRC}/kafka-topics.tar, scoped only by app name. Take backup A, produce more data, take backup B, then run a RestoreJob referencing A: the restore succeeds and hands back B's data, because the key is the same. Nothing marks the older Backup CR as superseded. This matches the nats sibling's single-key pattern and may be forced by the engine (the template gets no per-backup identity in backup mode, .Backup is nil there), so I read it as a documentation gap rather than a code defect. The README should say that only the latest backup of an app is actually restorable through this strategy.

[MINOR] 06-restore-in-place.sh:22, the topic-deletion wait can false-positive on a transient broker error and hollow out what the e2e proves

Inside the Pod snippet (set -eu, no pipefail) the check is ! kafka-topics.sh ... --list | grep -qx "$TOPIC". If --list itself fails, its output is empty, grep exits 1, and the negation declares the topic deleted while it's still there with all its records. Restore then replays into the live topic, the count reaches 60, and because the verification is >= (next finding) it still passes. So the round-trip test can go green without ever exercising topic recreation. Add set -o pipefail to that snippet, or check --list's own exit status before trusting its output.

[MINOR] 06-restore-in-place.sh:49, 07-restore-to-copy.sh:65, the round-trip check is >= and total-only

Verification passes when count >= MESSAGE_COUNT over a partition-summed offset delta. That can't catch over-restore (duplicate records: a restore Pod retried mid-replay re-runs the whole producer, and --create --if-not-exists skips the topic, so records double) and it doesn't assert per-partition distribution. The PR's headline claim is partition-preserving replay ("8/11/11 distribution preserved"), but nothing in the scripts or the test asserts per-partition counts, so the murmur2 repartitioning this PR is about is never exercised by CI. Consider exact equality where the flow uses a fresh/empty topic, plus a per-partition kafka-get-offsets assertion.

[MINOR] 01-create-strategy.sh:189-194, the drain assumes end - begin equals the consumable-record count

--max-messages $((end - begin)) holds only for a non-compacted, non-transactional topic. On a compacted topic (sparse offsets) or one written by transactional producers (control markers occupy offsets but are never delivered), fewer records than end-begin are consumable, so kafka-console-consumer idles until --timeout-ms 120000 and exits non-zero, killing the Pod under set -e and failing the BackupJob. That's fail-closed, no silent partial tarball, which is the right direction. But the topic_message_count comment at 00-helpers.sh:190 ("exact ... even for a compacted or truncated topic") is about the offset-math helper, not the drain, and could mislead. A one-line README limitation that compacted/transactional topics are out of scope for this logical path settles it.

[MINOR] README.md:21 / 01-create-strategy.sh:227-232, null-key fidelity is understated

The README says null-key records "are not guaranteed to land in their original partition." It's worse than that. print.key=true renders a null key as the literal token null, and replay with parse.key=true turns it into the 4-byte string key "null". So a null-key record doesn't just move partition, it gains a non-null key, and every originally-null-key record collapses onto one partition; null values (tombstones) similarly become ordinary payloads. The demo uses keyed records only, so this isn't exercised, but the limitation text should say null-key/null-value records are not round-tripped faithfully. The pinned 3.8 image supports the null.marker property (KIP-810) if faithful round-tripping is wanted later.

[NIT] 01-create-strategy.sh:128-129, hand-rolled endpoint parse mishandles IPv6 / path-carrying endpoints

HOST="${HOSTPORT%%:*}" mis-splits an IPv6 literal like [::1]:8333, and any endpoint carrying a path prefix loses it, since OBJ_URL is rebuilt from the bare host. The BucketInfo host:port shape works; other legal endpoint forms don't. Demo-scope, worth a comment.

Claim mismatches

[PARTIAL] "every partition is captured as of the same instant" / "a coherent point-in-time view" (README.md:12, PR body). The --time -1/--time -2 reads sit inside the for t in $TLIST loop (01-create-strategy.sh:172-178), so each topic is frozen when its iteration starts, after the previous topics have drained. The cut is atomic within a topic, not across topics. Kafka gives no cross-topic guarantee anyway, but the prose generalizes to "every non-internal topic." Narrow it to per-topic consistency.

[PARTIAL] "recreates each topic with only its original partition count and replication factor" (README.md:18). The partition count is original (from the manifest). The replication factor is not: restore uses the static REPLICATION_FACTOR parameter, default 1 (01-create-strategy.sh:53-54, applied at :222), and the source RF is never captured. Restoring into a multi-broker cluster without setting the parameter recreates topics at RF=1, a silent durability downgrade. Say "the configured replication factor (default 1)", not "its original replication factor."

[PARTIAL] "partition distribution preserved" (PR body). Correct in the code path (topic recreated with original partition count, deterministic murmur2), but not asserted by the shipped e2e, which checks total record count only.

Caveats

• Mechanical fail-open sweep ran on all 10 shell files. Every || true / 2>/dev/null hit was verified non-gating: 01-create-strategy.sh:167 handles grep's no-match exit only (the deciding --list is a separate command under set -e); 00-helpers.sh:77,110,116,126 are poll-loop reads; 00-helpers.sh:195-196 || exit 0 returns empty and every caller rejects a non-numeric count (06:48, 07:64); 06:20 --delete ... || true is backstopped by the disappearance wait-loop. Clean, with the one exception that the wait-loop's own --list pipe is unguarded (finding above).
• shellcheck SC2016 (00-helpers.sh:175,194; 06:19) and SC1091 (dynamic source) are false positives: the single-quoted bodies are Pod snippets whose $VAR must stay unexpanded until they reach the in-Pod bash, and the sourced path resolves at runtime. No action.
• job-service-label-overlap: no overlap. The throwaway CLI pods use kubectl run (run=kafka-cli-<rand>), which doesn't match the Strimzi bootstrap Service selector; the strategy Job pod labels are stamped by the controller, out of this PR.
• Sound on independent check: .bucket-info.env is gitignored (examples/backups/*/.bucket-info.env); empty partitions are handled (no data file, manifest still records the partition count); a tab inside a value round-trips because parse.key splits at the first separator only; the to-copy source-keyed S3 read via .Backup.ApplicationRef.Name is correct; the curl 7.76.1 --aws-sigv4 port-stripping workaround is internally consistent.

Recommended follow-ups

• grep -v '^__' and a topic-existence pre-check are the two cheapest, highest-value fixes: they stop the example from silently teaching a lossy backup.
• Add a per-partition offset assertion (and exact equality where the flow uses a fresh topic) to the e2e, so the partition-preserving-replay claim is guarded and duplicate-on-retry is caught.
• Note the latest-only-restorable and compacted/transactional-out-of-scope limitations in the README.

Thanks for this — the value here is bigger than the title suggests. The generic Job strategy has had unit tests and exactly one worked example (examples/backups/nats/) but no end-to-end coverage at all, and no Chainsaw suite drives it. kafka-2-backup-roundtrip is therefore the first e2e that exercises the generic driver for real: template rendering including .Mode/.Parameters/.Backup.ApplicationRef.Name, the parameter round-trip through the Backup's driverMetadata, and the to-copy retarget. The run bears it out — 452s, and the log shows 30 records seeded, backed up, the topic deleted, restored in place, then restored into a second cluster.

The design call also looks right to me. A logical frozen-end-offset drain is not the highest-fidelity Kafka backup available, but the README says so plainly and up front rather than burying it: no topic configs, no ACLs, no SCRAM users, no consumer-group offsets, offsets reassigned on restore, no headers or timestamps, and an explicit "for a faithful, offset-preserving Kafka backup, prefer a volume-snapshot strategy over this logical one". An example gets copied, so being told where it bites is most of what makes it safe to ship. The comment density throughout is well above average and the reasoning held up everywhere I cross-checked it against the controller.

Two things I'd like changed before merge, both small. The rest below are comments rather than blockers.

Blocking

1. Restore is not idempotent, and the assertion cannot see the failure mode. 01-create-strategy.sh:221-233 recreates topics with --create --if-not-exists (existing topics are left alone) and then replays every partition file unconditionally, so restoring into a topic that still holds data appends rather than replaces. That matters more than it looks, because buildJobStrategyBatchJob in internal/backupcontroller/jobstrategy_controller.go hard-codes backoffLimit: 2 — a restore Pod that dies partway through replay is retried, and the retry replays from the beginning. Both verifications then assert if (( count < MESSAGE_COUNT )), strictly less-than, so a restore that produced 45 or 60 records instead of 30 prints "In-place restore verified" and passes (06-restore-in-place.sh:49, 07-restore-to-copy.sh:65). Could we make both comparisons -eq, and add a line to the README's limitations saying restore appends into a non-empty topic and is safe only against an absent or empty one?

2. The roundtrip proves record counts, never record content. topic_message_count (00-helpers.sh:192) sums end - begin offsets per partition, and nothing ever consumes the records back. Meanwhile the README makes three fidelity claims the test does not check: keys survive, keyed records land in their original partition via the murmur2 partitioner, and per-partition order is preserved. A restore that replayed 30 records of garbage, or scrambled every key, passes today. The seeded sentinels are already perfect for this (k-<n> / order-<n>) — consuming the topic once and diffing a sorted list would close the gap in a few lines, and given that data fidelity is the entire subject of the example it seems worth having.

Non-blocking, roughly by weight

Helper Pods will time out on a first run outside CI. kafka_run (00-helpers.sh:157) uses kubectl run --restart=Never --rm -i, and kubectl run with attach waits for Running under --pod-running-timeout, whose default is 1m, with no override and no retry. CI never hits it because the Chainsaw test resolves KAFKA_IMAGE from the running strimzi-cluster-operator, so the image is already cached on the nodes. A user following the README gets the default quay.io/strimzi/kafka:0.45.0-kafka-3.8.0 pulled cold, which routinely takes longer than 60s, and step 04 dies with timed out waiting for the container to run before seeding anything. --pod-running-timeout=5m on the four call sites would cover it.

Helper Pods carry no securityContext at all. The passing CI log contains the evidence three times: Warning: would violate PodSecurity "restricted:latest": allowPrivilegeEscalation != false ..., unrestricted capabilities ..., runAsNonRoot != true ..., seccompProfile ... for each kafka-cli-* Pod. Warn-only in the e2e cluster, so the run stays green, but in a tenant that enforces restricted the demo cannot run at all. The strategy Pod is in better shape and its comment already acknowledges the runAsNonRoot gap; the helper Pods get no mention. Either --overrides with a securityContext, or a README line naming the requirement.

Null-key records come back with the literal key null. The dump uses --property print.key=true and Kafka's default formatter renders a null key as the string null; the replay reads that back through parse.key=true as a four-byte key (01-create-strategy.sh:190-192, 231-232). So such a record does not merely land in an unpredictable partition, which is what the README currently says — its key changes from null to "null", which also changes compaction semantics downstream. Worth restating that bullet in those terms.

No bound on the drain's local disk usage. WORK=/tmp/kafka-dump plus /tmp/kafka-topics.tar materialises the whole topic twice on the Pod's writable layer, and resources (01-create-strategy.sh:236-242) declares cpu and memory but no ephemeral-storage request or limit. A tenant copying this for a large topic fills the node's ephemeral storage and evicts unrelated Pods before the backup fails. A sizing caveat in the limitations section would earn its place.

Every backup of an app overwrites the previous one. The S3 key is ${SRC}/kafka-topics.tar with no run identifier (01-create-strategy.sh:139), so N Backup objects all resolve to the same tarball and restoring an older Backup silently restores current data. This is largely forced by the driver — at backup-render time the context carries only Application/Release/Mode/Parameters, and the Backup artifact is created after the Job completes, so the Pod has no unique handle — and the NATS example has the same shape. Still belongs in the limitations rather than being discovered later.

set -e fragility in the partition-count loop. [ "${mp}" -ge "${parts}" ] && parts=$((mp + 1)) as the last statement of a while read body (01-create-strategy.sh:217-220): if the final iteration's test is false, the loop returns non-zero and set -eu kills the restore Pod with no message. It cannot fire today because kafka-get-offsets emits partitions in ascending order, but if ... then ... fi is the same length and closes it.

The e2e never exercises the all-topics path. The BackupClass pins topics: "${TOPIC}" (02-create-backupclass.sh:33), so the --list-and-filter branch — exactly what "fix(kafka): fail closed when listing topics for an all-topics backup" addresses — has no coverage from the test that ships with it.

Smaller things: the hack/e2e-chainsaw/.chainsaw.yaml header enumerates the Tests that override the namespace to tenant-root as "postgres-2-, mariadb-2-, clickhouse-2-" and could pick up kafka-2-; every other script-based backup example ships the 90/91/92 scenario docs (clickhouse, nats, etcd, foundationdb) and this one has only the README, which may well be deliberate; and the default KAFKA_IMAGE pins 0.45.0 while the vendored operator chart is strimzi-kafka-operator 0.45.1-rc1, where the Chainsaw test already shows the drift-free way to resolve it.

On CI cost, for the record, since a backup roundtrip is the kind of thing that quietly grows the suite: hack/select-e2e.sh maps both examples/backups/kafka/* and hack/e2e-chainsaw/kafka/* to the single kafka suite, Chainsaw runs parallel: 1, and the measured 452s sits well inside the declared 40m budget, which is justified in-line as convention 6 asks. That reads as appropriately gated to me. Test hygiene follows the mariadb precedent closely and correctly — pre-clean before the run so a leftover Succeeded BackupJob in the shared namespace cannot instantly satisfy the wait, cleanup in an always-reached finally, wait_hr_ready failing fast on Stalled=True, wait_for_field failing fast on the terminal Failed phase, and a catch block selecting the strategy Pods by the real job.strategy.backups.cozystack.io/mode constant. The note in 05-create-backupjob.sh explaining why there is no intermediate "wait Running" gate is exactly the right instinct. I did not find anything I would expect to go red intermittently in CI.

Verdict

NOT LGTM

Reviewed at a43d90dc5692b01bf9a05a2e7cb357dd07d5abb9, against merge-base cd91627c. The round-trip Test really did run and pass on this head (--- PASS: chainsaw/kafka-2-backup-roundtrip (558.81s), job 106676143135), and I read the run's own output rather than the badge: the freeze cut captured 8/11/11, both restores came back byte-identical to the source dump, and the decoy topic survived with its 5 records. That settles the happy path. What it does not settle is the shape of a cluster nobody tested on, and that is where both blockers sit.

Two things block. The example cannot restore at all onto a Kafka with more than one broker, which is the only shape anyone runs in production. And the guard added last round to stop a restore appending into a live topic reads a listing that Kafka documents as lossy, without the check the backup side of the same file already performs on the same listing.

Findings

[MAJOR] examples/backups/kafka/02-create-backupclass.sh:38: a restore lands zero records on any Kafka with two or more brokers

replicationFactor is fixed at "1" here, and the strategy's own fallback at 01-create-strategy.sh:56 is also "1". The Cozystack Kafka chart sets the broker-level min.insync.replicas to 2 as soon as kafka.replicas reaches 2:

$ grep -n 'min.insync.replicas' packages/apps/kafka/templates/kafka.yaml
49:      min.insync.replicas: 1
55:      min.insync.replicas: 2
61:      min.insync.replicas: 2

Line 49 is the replicas == 1 branch, 55 the replicas == 2 branch, 61 the else branch for three or more.

That broker value becomes each topic's default (KafkaConfig.scala:1133, logProps.put(TopicConfig.MIN_IN_SYNC_REPLICAS_CONFIG, minInSyncReplicas)). kafka-console-producer defaults --request-required-acks to -1, i.e. acks=all (core/src/main/scala/kafka/tools/ConsoleProducer.scala:222-226 in Kafka 3.9.1), and the replay at 01-create-strategy.sh:366 does not override it. Upstream is explicit about what happens next:

When a producer sets acks to "all" (or "-1"), this configuration specifies the minimum number of replicas that must acknowledge a write for the write to be considered successful. If this minimum cannot be met, then the producer will raise an exception (either NotEnoughReplicas or NotEnoughReplicasAfterAppend).
Source: clients/src/main/java/org/apache/kafka/common/config/TopicConfig.java:174-182

So the restore recreates the topic with RF=1 against a broker default of min.insync.replicas=2, and every record it replays is rejected. The rejection is not fatal to the process: the async path hands the send an ErrorLoggingCallback (ConsoleProducer.scala:118), which logs and returns, and main still exits 0. The run therefore reaches the post-replay check and dies there instead:

restore verification failed for orders.v1: topic holds 0 record(s), backup recorded 30;
refusing to report a lossy or duplicated restore as complete

which names the wrong cause. The operator is told the replay was lossy; the actual NOT_ENOUGH_REPLICAS lines are only in the strategy Pod's log.

The in-place variant is worse, because 06-restore-in-place.sh:24-37 deletes the topic first, as the documented procedure. On a 3-broker cluster that turns an RF=3 topic into an unwritable RF=1 one and then fails, with the live topic already gone. The tarball still holds the data, so this is recoverable, but only by someone who works out that replicationFactor was the problem.

README.md:18 points the other way. It lists min.insync.replicas among the topic configs that are not captured, then says restoring into a multi-broker cluster without raising replicationFactor is "a silent durability downgrade". On a stock Cozystack Kafka it is not a downgrade, it is a restore that cannot complete. Nothing catches this because the demo pins kafka.replicas: 1 (04-create-kafka.sh:24, 07-restore-to-copy.sh:24), so the single tested shape is the single shape where RF=1 works.

The cheap fix is to stop guessing the replication factor. kafka-topics --describe already prints ReplicationFactor: on the very line the backup side parses PartitionCount: out of (01-create-strategy.sh:210-211), so capturing the source RF into the manifest and using it on restore is a small change, with the replicationFactor parameter kept as an override. Failing that, refuse up front when the requested RF is below the target's min.insync.replicas rather than discovering it through a misattributed count mismatch.

[MAJOR] examples/backups/kafka/01-create-strategy.sh:328: the "refuse a non-empty target" guard reads a listing that can come back short, and nothing checks that it did

ea2124076 moved the emptiness check ahead of the replay so a retried Pod leaves a live topic untouched. The check counts records from kafka-get-offsets, and that tool skips partitions it could not read while still exiting 0:

257                try {
258                    partitionInfo = listOffsetsResult.partitionResult(partition).get();
259                } catch (ExecutionException e) {
260                    if (e.getCause() instanceof KafkaException) {
261                        System.err.println("Skip getting offsets for topic-partition " + partition.toString() + " due to error: " + e.getMessage());
262                    } else {
263                        throw e;
264                    }
265
266                    continue;
267                }

Source: tools/src/main/java/org/apache/kafka/tools/GetOffsetShell.java, Kafka 3.9.1

The backup side of this same file knows that and guards it. The restore side reads the same listing and does not:

$ sed -n '215,220p' examples/backups/kafka/01-create-strategy.sh
                  seen=0
                  for e in \${ends}; do seen=\$((seen + 1)); done
                  if [ "\${seen}" -ne "\${pc}" ]; then
                    echo "offset listing returned \${seen} of \${pc} partition(s) for \${t}; refusing to upload an incomplete backup" >&2
                    exit 1
                  fi

$ sed -n '328,343p' examples/backups/kafka/01-create-strategy.sh
                  cends=\$("\${BIN}"/kafka-get-offsets.sh --bootstrap-server "\${BOOT}" --topic "\Q\${t}\E" --time -1)
                  cbegins=\$("\${BIN}"/kafka-get-offsets.sh --bootstrap-server "\${BOOT}" --topic "\Q\${t}\E" --time -2)
                  held=0
                  for e in \${cends}; do
                    ekey=\${e%:*}
                    eo=\${e##*:}
                    bo=0
                    for b in \${cbegins}; do
                      if [ "\${b%:*}" = "\${ekey}" ]; then bo=\${b##*:}; break; fi
                    done
                    held=\$((held + eo - bo))
                  done
                  if [ "\${held}" -ne 0 ]; then
                    echo "topic \${t} already holds \${held} record(s); this restore appends and is supported only against an absent or empty topic. Empty or delete it and re-run." >&2
                    exit 1
                  fi

A leader election in flight during the check, which is the same fault the comment at :194-201 was written for, drops the partitions holding the records from cends. held then sums the surviving empty partitions to 0, the guard passes, and the replay appends a full copy into a live topic. The post-replay check at :397 catches the result and fails the RestoreJob, but the duplication has already landed, which is precisely the outcome moving the check earlier was supposed to prevent. (If every partition fails the tool errors out and set -e stops the Pod, so only the partial case is exposed.)

The fix costs nothing: have_parts is already in scope twelve lines above (:312, validated numeric at :315-317 and equal to parts at :318). Count the lines in cends and refuse when they are fewer, exactly as the backup side does.

[MINOR] examples/backups/kafka/01-create-strategy.sh:172: a topic named twice in topics yields a Backup that reports Succeeded and can never be restored

TLIST is the parameter split on commas and spaces, with no de-duplication, and for t in ${TLIST} at :187 runs the whole per-topic block once per occurrence. The manifest gains a second set of lines; the data files are written to the same names and overwrite. On restore, want at :380-384 sums every manifest line while the replay writes each data file once, so the counts can never agree:

$ bash -c '
W=$(mktemp -d); M="$W/manifest.txt"; : > "$M"
TLIST=$(printf "%s" "orders.v1, orders.v1" | tr ", " "  ")
for t in $TLIST; do for p in 0 1 2; do
  printf "%s %s %s %s\n" "$t" "$p" 0 10 >> "$M"; : > "$W/data-$t-$p.tsv"
done; done
SEEN=""; while read -r mt mp mb me; do case " $SEEN " in *" $mt "*) ;; *) SEEN="$SEEN $mt";; esac; done < "$M"
for t in $SEEN; do want=0
  while read -r mt mp mb me; do [ "$mt" = "$t" ] || continue; want=$((want + me - mb)); done < "$M"
  echo "manifest lines=$(wc -l < "$M" | tr -d " ")  data files=$(ls "$W"/data-*.tsv | wc -l | tr -d " ")  want=$want  replayed=30"
done
rm -rf "$W"'
manifest lines=6  data files=3  want=60  replayed=30

have != want at :397 then fails every restore of that backup, permanently. The records do reach the target, so nothing is lost, but the operator is told the restore failed and the backup that reported Succeeded is not usable. A sort -u on TLIST, or a refusal on a repeated name, belongs with the other fail-closed guards in this script. While you are there: for t in ${TLIST} is unquoted and path-expanded, so a topics value containing * iterates the Pod's working directory instead. It fails closed at the PartitionCount guard, but it is the one place operator input reaches an unquoted expansion.

[MINOR] examples/backups/kafka/01-create-strategy.sh:160: curl -k, where the neighbouring example mounts the CA instead

The S3 helper disables certificate verification outright. The rabbitmq example in the same tree solves it against the same in-cluster seaweedfs: run-all.sh copies the seaweedfs CA into a Secret and the strategy Pod mounts it and sets CURL_CA_BUNDLE (examples/backups/rabbitmq/00-helpers.sh:44-51, 03-rabbitmq-strategy.yaml:86-94), with its own comment noting the curl script then "stays byte-for-byte the shipped one, with no -k". Both rabbitmq-2-backup-roundtrip and redis-2-backup-roundtrip pass in the same CI run under that arrangement, so it is not a theoretical alternative.

README.md:27 documents the weakness honestly and tells the reader to mount the tenant CA before pointing this anywhere external. That is worth something, but an example is copied more often than it is read, and the pattern it should be copying already exists two directories over.

[MINOR] hack/backup-access-preflight.bats:260: the renamed test claims "every" over a hand-written list of five

The rename from "all four Chainsaw backup roundtrips" to "every Chainsaw backup roundtrip" broadened a name that was accurate about a closed list into one that asserts an enumeration the test does not perform. Eight round-trip Tests exist; five are listed:

$ for f in hack/e2e-chainsaw/*/chainsaw-test.yaml; do
    grep -q 'name: .*-2-backup-roundtrip' "$f" &&
      printf '%s\t%s\n' "$(grep -c 'COZY_E2E_BACKUP_PREFLIGHT=1' "$f")" "$f"; done
1	hack/e2e-chainsaw/clickhouse/chainsaw-test.yaml
0	hack/e2e-chainsaw/etcd/chainsaw-test.yaml
1	hack/e2e-chainsaw/kafka/chainsaw-test.yaml
1	hack/e2e-chainsaw/mariadb/chainsaw-test.yaml
1	hack/e2e-chainsaw/mongodb/chainsaw-test.yaml
1	hack/e2e-chainsaw/postgres/chainsaw-test.yaml
0	hack/e2e-chainsaw/rabbitmq/chainsaw-test.yaml
0	hack/e2e-chainsaw/redis/chainsaw-test.yaml

rabbitmq and redis both provision a Bucket and drive the same in-cluster seaweedfs, and both passed in this run, so they are exactly the population the preflight exists for. Under the new name a reader concludes they are gated on it. The other two comments this PR touches (hack/select-e2e.sh:401-406, hack/e2e-chainsaw/.chainsaw.yaml:10-13) go the opposite way and delete their stale enumerations, which is the right instinct: either derive the list from hack/e2e-chainsaw/*/chainsaw-test.yaml or put the count back in the name.

[MINOR] examples/backups/kafka/00-helpers.sh:16: the documented default namespace is the one nothing runs in

NAMESPACE defaults to tenant-test, and README.md:39 repeats it. Every other S3-backed backup example defaults to tenant-root (clickhouse, postgres, mariadb, mongodb, redis, rabbitmq, etcd, foundationdb, vmi, kafka-metadata); only this one and nats do not. The round-trip Test overrides it (chainsaw-test.yaml:186, namespace: tenant-root) for the reason its own header gives: the e2e tenant-test is created isolated, so its Cilium egress allowlist blocks it from reaching tenant-root's seaweedfs. A fresh install has no tenant-test at all, so ./run-all.sh as documented fails at step 03 on a namespace that does not exist, and the documented default is never exercised by anything.

[MINOR] examples/backups/kafka/01-create-strategy.sh:348: a restore Pod that dies mid-replay can never be retried into success

The create/shape/emptiness loop at :299-344 runs over every topic first, then the replay loop at :348-368 writes them. If the Pod dies after topic A's file is replayed and before B's, the Job's backoffLimit: 2 (internal/backupcontroller/jobstrategy_controller.go:313) starts a new Pod, which reaches the emptiness check on A, finds records, and exits. Both retries burn that way and the RestoreJob settles Failed. That is the safe outcome, but the operator is handed a message naming one topic, with nothing recording which topics completed, so the remedy is "empty every topic this backup covers and start again" rather than what the message says. README.md:22 describes the retry as merely unable to stack a second copy, which is true and leaves out the part that matters. A progress line per topic before its replay would make the log a resume map; a sentence in that bullet would set the expectation.

[MINOR] examples/backups/kafka/00-helpers.sh:59: the image lookup fails on a digest-pinned operator, which is the air-gapped case the fallback is for

resolve_kafka_image filters with grep -E '=.*/kafka:', which requires a tag reference. The vendored Strimzi chart emits @<digest> instead of :<tag> whenever a digest value is set (packages/system/kafka-operator/charts/strimzi-kafka-operator/templates/_helpers.tpl:49), so a digest-pinned deployment produces 3.9.1=quay.io/strimzi/kafka@sha256:... and the filter drops every line:

$ printf '3.9.1=quay.io/strimzi/kafka@sha256:deadbeef\n' | grep -E '=.*/kafka:' | sed 's/.*=//' | wc -l
       0
$ printf '3.9.1=quay.io/strimzi/kafka@sha256:deadbeef\n' | grep '=' | sed 's/^[^=]*=//'
quay.io/strimzi/kafka@sha256:deadbeef

The fallback then assumes quay.io/strimzi/kafka:0.45.1-rc1-kafka-3.9.1, a public image, on a cluster that pins digests precisely because it cannot reach public registries. The warning does fire, so this is not silent, but the demo lands in ImagePullBackOff and wait_for_field spends its 600s before saying so. The jsonpath already selects only STRIMZI_KAFKA_IMAGES, so grep '=' is enough and the /kafka: shape assumption can go. The copy in chainsaw-test.yaml:262-264 carries the same filter.

[MINOR] hack/e2e-chainsaw/kafka/chainsaw-test.yaml:228: still open from earlier rounds: the guards keep arriving without anything that can redden them

Carried forward and re-reached independently by a second pass this round. The suite drives run-all.sh once, on one plain topic, with no fault injection, no second backup, no pre-seeded restore target and no tampered tarball. Of the guards in the strategy script, one is reachable: removing a \Q...\E pin from the offset listing lets the decoy's partitions into the count and the partition-set check fires, which the CI run confirms end to end (Decoy topic 'orders-v1' untouched: 5 record(s)). The rest are not: the partial-drain check (:252), the manifest-missing and manifest-field checks (:267, :284), the empty-manifest refusal (:289), both PartitionCount reads (:213, :315), the shape mismatch (:317), the emptiness refusal (:340), the data-file-versus-manifest check (:361) and the post-replay failure branch (:398).

The sharpest instance is the parameter default. topics defaults to empty, which selects every non-internal topic, and README.md:21 calls that "the riskiest, not the safest". 02-create-backupclass.sh:37 always pins a single topic, so the --list fail-closed path and the internal-topic filter at :174-182 never execute in CI, and neither does any multi-topic restore. Two of those would be cheap to cover without a fault injector: a second BackupClass with topics: "" exercising the all-topics path and the decoy's exclusion, and a second RestoreJob against the already-restored topic to drive the emptiness refusal.

[NIT] examples/backups/kafka/05-create-backupjob.sh:36: resolves the Backup's name, then throws it away

backup_ref is read from status.backupRef.name, checked non-empty, logged, and never used; 06 and 07 reference ${BACKUPJOB_NAME} instead. That is correct today, because createJobBackupArtifact names the Backup after the BackupJob (internal/backupcontroller/jobstrategy_controller.go:348), but the resolved value is the one that cannot go stale. Cache it to a file beside .backup-name.env, which .gitignore:125 already covers for the other examples, and use it.

[NIT] examples/backups/kafka/00-helpers.sh:289: the decoy count sums end offsets without subtracting begin

decoy_message_count adds ${e##*:} straight up, where topic_message_count eleven lines below subtracts the begin offset per partition. Equivalent for a freshly seeded topic and wrong for anything with retention behind it. Since this one is a regression fixture, that difference will read as a broken pin the first time someone reuses the helper.

[NIT] examples/backups/kafka/: no 90/91/92 scenario docs

clickhouse and nats, the two siblings this example copies its 00..07 layout from, both ship 90-scenario-admin-prepare.md, 91-scenario-user-backup.md and 92-scenario-user-restore.md alongside it. The README here covers the same ground in prose, so this is only a consistency note.

Checked and correct

Every finding from the previous round came back closed except the fixture one, and the two that were still open are both closed at this head: the delete-wait now matches with grep -qxF (06-restore-in-place.sh:31), and the host-side count defaults a missing begin offset to 0 instead of dropping the partition (00-helpers.sh:319). The preflight wiring is genuinely pinned now: deleting the call from 03-create-bucket.sh and stripping COZY_E2E_BACKUP_PREFLIGHT=1 from the Test were both run as mutations and both reddened hack/backup-access-preflight.bats, with the tree restored clean afterwards.

The \Q...\E design holds up against upstream rather than against its own comments: --topic is a regex in both GetOffsetShell and TopicCommand, and both route it through IncludeList, which calls String.matches, so the match is full-string anchored and Kafka's own [a-zA-Z0-9._-] name charset means the quoting cannot be escaped. The calls left unpinned are the ones that take a literal name. The template context is real (.Release.Name, .Release.Namespace, .Mode, .Parameters, .Backup.ApplicationRef.Name all come out of renderJobTemplate), the SRC_NAMESPACE reasoning holds because targetApplicationRef is a TypedLocalObjectReference and the restore Job always runs in restoreJob.Namespace, the job.strategy.backups.cozystack.io/mode selector in catch matches a label that really lands on the Pod template, and the fallback image literal really is what the vendored chart ships. hack/select-e2e.sh and hack/e2e-chainsaw/.chainsaw.yaml changed only comments, verified by diffing both files with comment lines stripped, and the Chainsaw file is append-only, so nothing existing moved.

What would change my mind

On the first blocker: a run of this example against a Kafka with kafka.replicas: 3 that restores successfully. If min.insync.replicas turns out not to reach the recreated topic on a Strimzi-managed cluster the way KafkaConfig.extractLogConfigMap says it does, the finding is mine to withdraw.

On the second: an argument that kafka-get-offsets cannot return a partial listing here, or a decision that the pre-replay check is advisory and the post-replay one is the real guard, in which case say so in the comment rather than leaving the two sides of the file asymmetric.

Verdict

NOT LGTM. The replication-factor fix is real and is now pinned by a fixture; the partition-listing fix is real and is pinned by nothing. The branch also no longer merges.

What this round closed

Both blockers from the last round are fixed, and the fix for the first one brought the demo up to a shape that can actually catch it.

• 02-create-backupclass.sh no longer pins replicationFactor: "1". The backup reads ReplicationFactor off the same kafka-topics --describe line the partition count comes from, writes it as the manifest's fifth field, and restore recreates the topic with it, with the parameter surviving only as an override.

• A backup taken by the previous revision carries a four-field manifest line. That case fails closed rather than falling back to 1, and the guard that does it is load-bearing:

  $ printf 'legacyTopic 0 0 30\n' > manifest.txt   # 4 fields, pre-fix format
  $ bash mcheck.sh                                  # the :356-366 validation loop, verbatim
  manifest line 1 (topic legacyTopic): non-numeric field ''; refusing to restore from a malformed backup
  exit=1
  

• The demo runs two brokers with two-replica topics, which is the shape the chart puts min.insync.replicas: 2 on, and steps 06 and 07 assert the restored replication factor. The e2e is the evidence that this is not decorative: chainsaw/kafka-2-backup-roundtrip passed on the previous head with replicationFactor: "1" and one broker (558.81s), and on this head it prints the assertions firing:

  OK Topic 'orders.v1' holds 30 record(s) at replication factor 2.
  OK Verified after in-place restore: 30 record(s) ... at replication factor 2, content matches source.
  OK Restore into the populated topic was refused (3 attempt(s)) and left it untouched.
  OK To-copy restore verified: ... at replication factor 2, content matches source.
  

Fifteen of the seventeen items I had open are closed. Credit also for the resolved Backup name cached in .backup-name.env, the CA verification replacing curl -k, the tenant-root default, the digest-tolerant image lookup, the topics charset and duplicate guards, and the three scenario walk-throughs.

Findings

[MAJOR] examples/backups/kafka/01-create-strategy.sh:192-195, the partition-listing guard reddens nothing when removed

topic_records' seen -ne want_parts check is the whole of the second fix: without it a listing shortened in flight sums the surviving partitions to zero and the replay appends a second copy into a live topic. Delete those four lines and every fixture in the tree stays green. The guard only fires when kafka-get-offsets skips a partition, nothing in examples/backups/kafka or hack/e2e-chainsaw/kafka induces that, and the second RestoreJob the round adds exercises the emptiness refusal on a healthy listing, which is a different branch. A mutation run against the extracted script with stubbed kafka-*.sh reproduces it: head refuses, the mutant appends 30 records into a populated topic, and the happy path run-all.sh drives is rc=0 either way.

The test is feasible without a cluster: lift args[0] out of the rendered strategy, stub /opt/kafka/bin, drive it from bats. The same harness would cover the eight earlier rounds' guards, none of which has one either. What would change my mind: a line in the README ruling that an example's embedded script is out of scope for in-tree tests. Then this closes as a decision rather than a gap.

[MINOR] examples/backups/kafka/01-create-strategy.sh:427-435, the shape check reads PartitionCount and not ReplicationFactor

Both come off the same --describe. Restoring into a pre-existing topic whose replication factor differs from the backup's is accepted, and the log line at :448 then reports the manifest's value rather than the topic's. On a min.insync.replicas: 2 broker a pre-existing rf=1 topic takes the replay, lands nothing, and fails on a record count that names neither cause. Parse rf beside have_parts and compare it.

[MINOR] examples/backups/kafka/01-create-strategy.sh:358, the topic name is the one manifest field restore does not validate

mp/mb/me/mr are checked numeric, mt only non-empty, and mt is what reaches for t in ${SEEN} at :407 unquoted with globbing on, because set -f is in the backup branch only (:244):

$ ls
data-a-0.tsv  g.sh  manifest.txt
$ mt='*'; SEEN=" ${mt}"; for t in ${SEEN}; do printf 'topic=[%s]\n' "$t"; done
topic=[data-a-0.tsv]
topic=[g.sh]
topic=[manifest.txt]

It ends fail-closed at Kafka's --create, so this is legibility rather than damage, but the backup side already applies [A-Za-z0-9._-] to exactly this at :247-249.

[MINOR] hack/e2e-chainsaw/kafka/chainsaw-test.yaml:179-183, the stated reason for the .svc endpoint is not true

The comment says the serving cert's SANs are *.tenant-root.svc and *.tenant-root.svc.cluster.local, and that seaweedfs-s3.tenant-root matches neither. With s3.tlsSecret unset, which it is, the S3 listener serves seaweedfs-client-cert, and the two-label wildcard is its first SAN:

$ helm template swfs packages/system/seaweedfs --namespace tenant-root \
    -s charts/seaweedfs/templates/s3/s3-deployment.yaml | grep cert.file
  -cert.file=/usr/local/share/ca-certificates/client/tls.crt

$ helm template swfs packages/system/seaweedfs --namespace tenant-root \
    -s charts/seaweedfs/templates/cert/client-cert.yaml | sed -n '/dnsNames/,+3p'
  dnsNames:
    - '*.tenant-root'
    - '*.tenant-root.svc'
    - '*.tenant-root.svc.cluster.local'

Four sibling suites (postgres, clickhouse, mongodb, mariadb) pass in CI on the two-label form every run. The .svc name is fine and I would keep it; the sentence explaining why will send the next person debugging a TLS failure down the wrong path. The same parenthetical is already wrong in examples/backups/rabbitmq/run-all.sh:23.

[MINOR] examples/backups/kafka/03-create-bucket.sh:77,81, a denial reads as an absence, and the persona column disagrees with the sibling

Both lookups discard stderr, so Forbidden on the cross-namespace read is indistinguishable from "not there" and surfaces as No seaweedfs CA Certificate found in tenant-root; set S3_CA_SECRET explicitly. Setting it explicitly cannot fix a 403. S3_CA_NAMESPACE is pinned to tenant-root independently of NAMESPACE, and the README invites pointing NAMESPACE at another tenant. Meanwhile README.md:39 still labels step 03 tenant, where the clickhouse example doing the same cross-namespace CA read labels it tenant, plus admin for the CA copy. One of the two is wrong.

[NIT] examples/backups/kafka/06-restore-in-place.sh:127 against :130

The comment says every attempt must have stopped at the emptiness check; the check accepts one. verify_restored afterwards covers the real risk, so this is the sentence rather than the logic.

[NIT] examples/backups/kafka/01-create-strategy.sh:186, called as $(topic_records ...) || exit 1, errexit is off inside the function

Confirmed on bash 3.2 and 5.1: a failing command anywhere in a function invoked as the operand of || does not abort it, execution continues, and the || never fires. The helper is safe today only because the seen -ne want_parts recount turns any such failure into return 1. Since this is now the calling convention for the helper, a future command added to it will be swallowed silently.

[NIT] examples/backups/kafka/01-create-strategy.sh:409-413, the manifest's replication factor has no agreement check

rf=${mr} takes the last matching line and never compares the lines with each other, where the partition count gets an explicit shape check. Only reachable through a tampered tarball, which is the trust boundary the surrounding code otherwise treats with real suspicion.

Still open from my earlier rounds

One item, at two anchors, open for a second round: hack/e2e-chainsaw/kafka/chainsaw-test.yaml (rounds_open=1). Guards keep arriving and nothing can fail when one is removed. This round improved it, the emptiness refusal and the replication factor are now genuinely driven, but the partial-drain check, the manifest validations, the data-file cross-check, both PartitionCount parses and the post-replay check still have no fixture, live or offline, and the all-topics path is never exercised because the BackupClass pins one topic. The MAJOR above is the same finding reaching this round's own fix, found independently by two passes.

Claim mismatches in the PR body

• "Verified end to end on a live single-broker cluster" is the evidence offered for a defect that only appears at two or more brokers. That run is what the CI at the previous head also did, green, with the hardcoded replicationFactor: "1" in place.
• "a seeded 3-partition topic" is now 3 partitions at 2 replicas, plus a decoy.
• "examples/backups/kafka/ — 11 files" is 14, and the file list omits hack/select-e2e.sh, hack/e2e-chainsaw/.chainsaw.yaml, hack/backup-access-preflight.bats and .gitignore.

Caveats

• The branch conflicts with main. hack/e2e-chainsaw/kafka/chainsaw-test.yaml is the only file changed on both sides since the merge base, and the API reports mergeable: CONFLICTING.
• timeout: 40m on the run step is unchanged while the flow grew a second broker per cluster and a third RestoreJob. The head's run took 806s, so there is headroom, but I did not measure the worst case.
• Everything I ran was offline: shellcheck (info only), bats hack/backup-access-preflight.bats (7/7), hack/select-e2e.sh on the changed file list (selects the full suite), helm template of the seaweedfs certificate and S3 deployment, and shell-level reproductions of the manifest validation and the glob. Broker-side behaviour is read from the e2e log on this head, not executed here.
• No chart, Go package or values.yaml is touched, and nothing under examples/ is packaged into an artifact, so there is no upgrade or fresh-install surface to judge.

Follow-ups, outside this diff

• examples/backups/nats/01-create-strategy.sh:112-115 still carries curl -fsS -k with the exact sentence this PR retracted from the kafka example.
• examples/backups/kafka-metadata/README.md:40 claims the source replication factor is applied as-is while its demo runs single-broker clusters, which is the blind spot this PR just removed from its own demo.
• The preflight roster at hack/backup-access-preflight.bats:260 is hand-maintained and correct today. redis and rabbitmq drive the same Bucket and endpoint without it. Deriving the list would keep it correct without a rename each time.

Verdict

LGTM

Round 2's blocker is genuinely closed: the partition-listing guard is now pinned, and I confirmed it by deleting the four lines and watching the suite go red under the runner CI actually uses, not only under bats. Eighteen of the twenty guards I mutated redden exactly one case each, the branch conflict from round 2 is gone, and the E2E run on this head is green. Nothing below blocks a merge.

Two of the findings are worth picking up, either before this lands or straight after. One new guard reads the lowest-precedence synonym and so can never fire on a real broker, which means it does not yet do the job its own README describes. And one comment this round rewrote is now false: that second one is my fault, not yours, and I say so below.

Findings

[MINOR] examples/backups/kafka/01-create-strategy.sh:476-484: the min.insync.replicas guard parses the lowest-precedence synonym, so it reads 1 on every Cozystack Kafka

kafka-configs --describe --all prints one line per config and that line carries the synonym list after sensitive=, ordered by descending precedence. The sed has a greedy leading .*, so it captures the last occurrence on the line, normally DEFAULT_CONFIG:min.insync.replicas=1, instead of the effective value. packages/apps/kafka/templates/kafka.yaml:53-59 sets broker-level min.insync.replicas: 2 whenever kafka.replicas is 2, which is exactly the shape this guard exists for, so misr comes out 1 and [ "${have_rf}" -lt 1 ] is false for every legal replication factor. Line 481 is dead.

The repo already has the right idiom for this same line, in the driver this example is modelled on:

$ sed -n '118p;133p' packages/system/backupstrategy-controller/files/kafka-backup.sh
    # ("  key=value sensitive=... synonyms=..."), so a value that contains a
        kv=${cl%% sensitive=*}

$ echo '  min.insync.replicas=2 sensitive=false synonyms={STATIC_BROKER_CONFIG:min.insync.replicas=2, DEFAULT_CONFIG:min.insync.replicas=1}' \
    | sed -n 's/.*min\.insync\.replicas=\([0-9][0-9]*\).*/\1/p' | head -1
1

Consequence: replicationFactor: 1 restored into a two-broker target passes the guard, the topic is created with rf=1, every acks=all write is rejected, and the run dies at the post-replay count with "topic holds 0 record(s), backup recorded 30": the misleading shortfall the guard was added to replace, and the one README.md:18 promises it prevents.

The test cannot see this. hack/kafka-topic-backup-strategy.bats:153 writes ${MISR} into both the value and the synonym, so the one line shape a real broker always produces is the one shape the stub never emits. Case 11 detects the guard being deleted (my mutation reddens it) and not the guard being wrong. Cutting at sensitive= like kafka-backup.sh does, plus a stub case where the two differ, closes both halves.

[MINOR] hack/e2e-chainsaw/kafka/chainsaw-test.yaml:146-151: the reworded SAN comment is false, and the previous wording was right

This round replaced the comment with "whose SANs cover *.tenant-root, *.tenant-root.svc and *.tenant-root.svc.cluster.local, so the 2-label name would verify too". A wildcard whose remainder is a single label does not match in any mainstream TLS stack. On a synthetic certificate carrying exactly those three SANs:

$ openssl verify -CAfile c.pem -verify_hostname seaweedfs-s3.tenant-root c.pem
error 62 at 0 depth lookup: hostname mismatch
$ openssl verify -CAfile c.pem -verify_hostname seaweedfs-s3.tenant-root.svc c.pem
c.pem: OK

$ curl -sS --cacert c.pem --resolve seaweedfs-s3.tenant-root:18443:127.0.0.1 https://seaweedfs-s3.tenant-root:18443/ -o /dev/null
curl: (60) SSL: no alternative certificate subject name matches target host name 'seaweedfs-s3.tenant-root'
$ curl -sS --cacert c.pem --resolve seaweedfs-s3.tenant-root.svc:18443:127.0.0.1 https://seaweedfs-s3.tenant-root.svc:18443/ -o /dev/null ; echo rc=$?
rc=0

This one is on me. My round-2 note said the comment's stated reason was untrue because *.tenant-root is the cert's first SAN, and you rewrote it to match. The SAN is there; it just does not match a 2-label name. Three neighbours in the tree still carry the correct statement and are worth copying rather than re-deriving: hack/e2e-chainsaw/rabbitmq/chainsaw-test.yaml:84-88, hack/e2e-chainsaw/redis/chainsaw-test.yaml:663-665, examples/backups/rabbitmq/run-all.sh:20-23. The same sentence also went into the commit message of 7ce85a2f7. Nothing catches it because the harness uses the 3-label name, so the comment is unexercised prose.

[MINOR] examples/backups/kafka/01-create-strategy.sh:366-385: a duplicated (topic, partition) manifest line is the one malformed shape the untrusted-tarball validation misses

The validation loop checks each line's topic charset and that four fields are numeric; it never checks that a pair appears once. The replay at :508-513 runs the producer once per matching line and the expected total at :529-533 accumulates me - mb once per matching line, so both double and have == want balances. Run against a tarball this script itself produced, with one line duplicated:

orders.v1 0 0 10 2
orders.v1 0 0 10 2
orders.v1 1 0 10 2
orders.v1 2 0 10 2
  orders.v1: restored 40 record(s)
restore complete
RECORDS=40

The backup side refuses a duplicate topic for exactly this reason (:249-253), and :536 says the check exists "refusing to report a lossy or duplicated restore as complete": a duplicated restore is what it passes. The precondition is a corrupt or tampered tarball, which is narrow, but it is inside the threat model the code states for itself at :356-357.

[MINOR] examples/backups/kafka/01-create-strategy.sh:232-233: a topics value of separators only backs up nothing and reports Succeeded

The branch is chosen by [ -n "${TOPICS}" ] on the raw string. tr ', ' ' ' turns ",", ", ," or " " into whitespace, the loop iterates zero times, and tar plus PUT still run: there is no [ -n "${TLIST}" ] gate before :269, though the restore side has exactly that gate at :385:

backing up topics [] from kafka-x-kafka-bootstrap.tenant-root.svc:9092
uploaded s3://bkt/tenant-root/kafka-test/kafka-topics.tar
MANIFEST_BYTES=0

Because the S3 key carries no run identifier (README.md:25), this overwrites the previous good tarball, and the loss surfaces only at restore time as "manifest is empty".

[MINOR] hack/backup-access-preflight.bats:242-258: still open from round 2: the roster is five hand-written paths, and the title claims "all"

The word "every" is gone, but :244-248 is still a literal list, not a glob over examples/backups/*/. A new example that waits on accessGranted without calling the preflight is not noticed: adding one and re-running left the case green. The title is also wrong on the tree as it stands: ten example directories gate on accessGranted and five run the preflight, and Kafka is not a database flow. The twin case twelve lines below was renamed this round for the same reason ("all four" to "the five preflight-wired"); this one was left.

[MINOR] ZooKeeper survives in text this PR added, in a KRaft-only tree

hack/e2e-chainsaw/kafka/chainsaw-test.yaml:220 says each cluster "boots ZooKeeper then the brokers", 217 lines below the same file's own header ("KRaft mode ... no ZooKeeper") and 111 lines below an assertion that the ZooKeeper client Service must never exist. examples/backups/kafka/cleanup.sh:35 waits on "two StatefulSets, ZooKeeper and their PVCs". Both lines are additions in this PR, and the same push removed the zookeeper: block from 04-create-kafka.sh and 07-restore-to-copy.sh because the chart no longer has the field.

[NIT] examples/backups/kafka/01-create-strategy.sh:243: || true reinstates the swallow the comment above it just removed

:235-241 explains at length that --list runs as its own command so a broker error aborts under set -e. The next line's grep -v '^__' || true cannot tell "no non-internal topics" (rc 1) from an I/O error (rc 2), and rc 2 yields an empty TLIST and the empty-backup path above. packages/system/backupstrategy-controller/files/kafka-backup.sh:127-130 checks rc -eq 1 explicitly for the same reason.

[NIT] examples/backups/kafka/01-create-strategy.sh:437,465: replicationFactor reaches Kafka unvalidated and comes back as a string comparison

Every other operator-supplied value gets a case guard (topics at :258, each manifest field at :377-383); rf=${REPLICATION_FACTOR} does not. :465 then compares it as a string against what the broker reports, where its sibling at :461 compares a number the script computed. Any value Kafka normalises rather than rejects makes the run abort with "refusing to restore into a pre-existing topic with a different replication factor" against a topic it created seconds earlier. I have not run this against a real broker, so treat the normalisation as reasoned, not measured.

[NIT] the retracted "every attempt" wording survives in two places

06-restore-in-place.sh:126-129 was weakened this round from "Every attempt must have stopped at the emptiness check" to "At least one attempt must have logged the refusal". hack/e2e-chainsaw/kafka/chainsaw-test.yaml:222 and examples/backups/kafka/92-scenario-user-restore.md:35 still carry the stronger claim.

Status of findings from my earlier rounds

Closed, each with the command behind it:

• The partition-listing guard reddens nothing (MAJOR, round 2). Closed. Deleting 01-create-strategy.sh:201-204 reddens case 9 and nothing else, under bats and under the runner CI uses: hack/cozytest.sh hack/kafka-topic-backup-strategy.bats exits 1 with "restore refuses a pre-replay listing short of the partition count". The suite runs in CI on this head: job 107215023221 shows all 14 cases, auto-discovered by BATS_UNIT_FILES in the Makefile, so nothing needed wiring.
• Shape check read PartitionCount and not ReplicationFactor (MINOR, round 2). Closed: :454, :458-460, :465-468. Deleting :465-468 reddens case 10.
• The manifest topic name was unvalidated (MINOR, round 2). Closed: :372-376. Deleting them reddens case 13.
• A denial read as an absence in 03-create-bucket.sh (MINOR, round 2). Closed at :81-89: only NotFound earns the discovery pass, anything else names the namespace and exits 1.
• The persona column disagreed with the ClickHouse example (MINOR, round 2). Closed: README.md:39 now matches examples/backups/clickhouse/README.md:30.
• Nothing in the chainsaw Test can redden its own guards (MINOR, open two rounds). Closed. Both script ops carry check: ($error == null): true, run-all.sh runs each step bare under set -euo pipefail, and each of the five assertions fails on a wrong value: exact equality on the count (06-restore-in-place.sh:71), diff against a source dump pinned non-empty at 04-create-kafka.sh:62, an RF compared against a variable the strategy Pod never sees, wait_for_field given Succeeded as a terminal fail-value, and the decoy count. The || true and 2>/dev/null shapes present sit on log fetches, not on verdicts.
• errexit is off inside topic_records (NIT, round 2). Closed in effect. Not every command carries || return 1, but the two that can fail do, and fault injection shows the rest either cannot fail silently or die as a fatal expansion error that kills the substitution subshell, so || exit 1 still fires.
• The manifest's replication factor had no agreement check (NIT, round 2). Closed: :431-434, reddens case 13 when deleted.
• 06-restore-in-place.sh comment against its own check (NIT, round 2). Closed at :126-132; the residue is the NIT above, in two other files.

Still open: the preflight roster, filed above.

The branch conflict from round 2 is gone: GitHub reports mergeable: MERGEABLE, and the E2E run on this exact head is green.

Mutation coverage, measured

I deleted twenty guards one at a time and ran the full suite against each. Eighteen redden exactly one case and no other. The two that redden nothing are :355 (manifest file missing) and :368 (manifest line with no topic); I checked both, and with the guard gone the script still fails closed: set -e on the redirect, and the numeric-field check at :377-383 respectively. They improve the message, not the outcome, so the suite not seeing them is fair. "Fourteen cases, one per guard" in the PR body is the one claim I would reword: the mapping is many guards to one case in several places, which is fine, but it means a reader cannot tell from the count which guards are held.

Both blockers are addressed, and the fixes go further than the review asked for.

The exact-count comparison plus the byte-equality diff against the source snapshot now prove that keys, values, partition placement and per-partition ordering survived the round trip, rather than just the record count. And instead of only making a double-replay fail an assertion, the driver refuses a non-empty target before replaying, with the partition-count shape check and the post-replay verification alongside it — so a retried Pod leaves the topic untouched rather than stacking a second copy onto a partial one. Driving that refusal path in the in-place step, requiring it to settle Failed, and then re-running the full verification to prove the topic came back unchanged is a better test than the one suggested.

The non-blocking items are all either fixed or documented. The two left as documentation — the unbounded ephemeral-storage usage and the per-run S3 key — are the right call for an example, and the second genuinely needs a driver change, since the Backup artifact does not exist at render time.

Worth calling out separately, because none of it came from the review: the Java-regex topic matching, the ^_ to ^__ internal-topic filter, the short-drain detection for compacted and transactional topics, and the min.insync.replicas interaction with the captured replication factor. The decoy topic sitting beside orders.v1 pins the first of those as a permanent regression guard, and the hermetic bats suite pins the rest — including the all-topics path the e2e cannot reach.

LGTM.