test(model): exercise RebuildDelta via a model-test restore cycle#1593
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release/v3.0 #1593 +/- ##
================================================
- Coverage 74.83% 74.47% -0.37%
================================================
Files 430 430
Lines 45722 45722
================================================
- Hits 34215 34050 -165
- Misses 8455 8638 +183
+ Partials 3052 3034 -18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
✅ Approve — automated reviewAll previously identified issues (the unsupported No findings. |
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 2 new inline findings.
Summary: #1593 (comment)
Review feedback on #1593 (NumaryBot): 1. With --restore and no explicit duration, the single-node default (30s) ended the run before the first jittered cycle (fires within [interval, 1.5*interval], 45-67s by default) — a vacuous PASS. The restore default is now 2*interval+30, which covers the first fire plus the cycle for any interval, and the report FAILS outright when --restore completed zero cycles (fail-fast preempting the first cycle with a real finding stays a legitimate early stop). The cycle count is printed like the cluster restart count. 2. wait_leader grepped the whole server log, which is appended across boots, so the initial boot's leadership line satisfied the post-restore wait instantly — the driver could resume against a node that never came up. Relaunching callers now pass the log's byte offset and only lines after it count. Also documents --restore and RESTORE_INTERVAL in the script header. Validated: default-duration run armed at 120s, completed a cycle, and reported it; a 20s run fails with NO RESTORE CYCLES.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 3 new inline findings.
Summary: #1593 (comment)
Azorlogh
changed the title
Review feedback on #1593 (NumaryBot, third finding): a restore cycle failing after a prior success (backup RPC, no exports, bootstrap, or the post-restore leader wait) was logged and the run continued — the report only failed on zero completed cycles, so a mid-run bootstrap regression could still report PASS. Failed cycles are now counted, shown next to the completed count, and any failure fails the report. Validated by killing MinIO mid-run: cycle 3's incremental backup failed with connection refused, the report showed "2 (1 failed)" and RESTORE CYCLE FAILURES, and the driver resumed after the err response.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot review complete: no remaining inline findings.
Resolved 3 stale NumaryBot review threads (3 fixed, 0 outdated).
Summary: #1593 (comment)
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
Reviewed at 32b47dd. The restore cycle now exercises a non-empty incremental backup through store bootstrap/RebuildDelta, resumes the existing model checks against the rebuilt store, and fails the run when no cycle completes or any cycle fails. The shutdown, relaunch, and quiescence paths are coherent, all prior review threads are resolved, and the full CI suite is green. I found no remaining actionable issue.
The model driver periodically drains its workers, hands the quiesced ledger to an external orchestrator that backs it up and restores the backup into a fresh store (running the RebuildDelta replay), then relaunches the node on it. The driver keeps running against the restored node, so its ordinary read/commit checks validate the rebuilt state with no separate comparison. The trigger is environment-specific behind RestoreTrigger: locally a request/response file rendezvous consumed by run_model_test.sh (bash polls files far more robustly than sockets); the operator-driven k8s path is left for the Antithesis environment. run_model_test.sh --restore (single-node) stands up MinIO, takes a startup full backup, and per cycle runs an incremental backup + store bootstrap into a fresh dir + relaunch on the restored store with a fresh WAL (the RESTORED marker self-bootstraps it). Each cycle asserts the manifest carried exports, since RebuildDelta only runs on the delta beyond the checkpoint. pauseAndDrain flushes the re-order buffer once nothing is outstanding: handleObservation's failure/transient paths drop the inflight ticket without calling tryDrain, so with no further op to re-trigger it a committed success could otherwise sit un-drained forever.
The --restore cycle bootstraps a full store per iteration, so on a host whose filesystem is already near-full the server's WAL/data disk guard blocks writes and the run fails with ResourceExhausted noise unrelated to the test. - WORKDIR is placed under $TMPDIR (default /tmp), so disk-heavy runs can target a larger volume. - HEALTH_THRESHOLD (default 0.8, matching the server) is passed to --health-wal-threshold / --health-data-threshold, so a run on a >80%-full host can raise the guard. Both default to the prior behaviour.
Review feedback on #1593 (NumaryBot): 1. With --restore and no explicit duration, the single-node default (30s) ended the run before the first jittered cycle (fires within [interval, 1.5*interval], 45-67s by default) — a vacuous PASS. The restore default is now 2*interval+30, which covers the first fire plus the cycle for any interval, and the report FAILS outright when --restore completed zero cycles (fail-fast preempting the first cycle with a real finding stays a legitimate early stop). The cycle count is printed like the cluster restart count. 2. wait_leader grepped the whole server log, which is appended across boots, so the initial boot's leadership line satisfied the post-restore wait instantly — the driver could resume against a node that never came up. Relaunching callers now pass the log's byte offset and only lines after it count. Also documents --restore and RESTORE_INTERVAL in the script header. Validated: default-duration run armed at 120s, completed a cycle, and reported it; a 20s run fails with NO RESTORE CYCLES.
Review feedback on #1593 (NumaryBot, third finding): a restore cycle failing after a prior success (backup RPC, no exports, bootstrap, or the post-restore leader wait) was logged and the run continued — the report only failed on zero completed cycles, so a mid-run bootstrap regression could still report PASS. Failed cycles are now counted, shown next to the completed count, and any failure fails the report. Validated by killing MinIO mid-run: cycle 3's incremental backup failed with connection refused, the report showed "2 (1 failed)" and RESTORE CYCLE FAILURES, and the driver resumed after the err response.
The workload module imports the oracle library from the main module since the tests/oracle extraction, but the image build copies only go.*, internal/, and pkg/ into the partial repo the replace directive resolves to — so the instrumentor's go mod tidy fails to find tests/oracle (and oracletest). Copy tests/oracle into both replacement roots (/src for the pre-instrumentation module, / for the instrumented copy), like internal/ and pkg/.
The model driver's restore rendezvous now works in the Antithesis k8s template: a restore-orchestrator sidecar in the workload pod services each request with a full disaster-recovery pass through the operator — quiesce-point incremental backup (ledgerctl exec'd in a ledger pod, retried across pods), Cluster CR + PVC teardown, restore-mode re-create at one replica, restore download/finalize, then the flip back to the full cluster (pod 0 self-bootstraps from the restored store, the others join fresh and snapshot-install it). The sidecar snapshots the live Cluster spec at startup and derives both mode variants from it; on any failure it re-applies the normal spec before answering err, so the driver never resumes against a cluster parked in restore mode. An initial full backup at startup makes every cycle's delta force RebuildDelta. Driver: MODEL_RESTORE_TIMEOUT makes the per-cycle lease tunable (the k8s cycle takes minutes under fault injection; the local default is unchanged), and a Sometimes assertion — "restore cycle completed" — is the report-visible proof the path ran, mirroring the local runner's zero-cycles guard. The workload image gains jq, a pinned kubectl, and the orchestrator script (outside the test-command tree so the Composer never runs it); the workload RBAC gains cluster create/delete, pods/exec, and PVC deletion.
Azorlogh
force-pushed
the
test/model-test-backups
branch
from
32b47dd to
9cb4410
Compare
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #1593 (comment)
The RUN-time curl to dl.k8s.io resolves through the build stage's DNS, which fails intermittently under qemu emulation. COPY --from an OCI image goes through the docker host's pulls instead, like the grpc-health-probe layer above it.
The bitnami/kubectl public tags no longer exist on Docker Hub; the official registry.k8s.io/kubectl image carries the binary at /bin/kubectl.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 2 new inline findings.
Summary: #1593 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #1593 (comment)
The environment builder mirrors images found in pod templates; the ledger image only appears in the Cluster CR's spec.image fields, so the hermetic environment lacked it and every ledger pod failed to pull (no such host). A no-op preload Job carries the reference in a pod template. The legacy Justfile solved this with the antithesis.images webhook param, which snouty rejects by design.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 2 new inline findings.
Summary: #1593 (comment)
Review feedback on #1593 (NumaryBot): 1. ledgerctl restore download does not define the --timeout flag the shared exec wrapper appended, so every download invocation failed on flag parsing. The wrapper now bounds every exec with coreutils timeout instead — uniform across subcommands and also covering a hung exec stream. 2. A failed initial full backup was never retaken (cycles only run incrementals, which need the full base), leaving every cycle broken with a log line claiming otherwise. The orchestrator now tracks whether the full backup succeeded and retakes it at the start of the next cycle — the driver is quiesced there, so it is as valid a base as one taken at startup. 3. The failed-cycle path applied the normal-mode spec and released the driver immediately; it now also waits for the cluster to reach full readiness first, so the driver never resumes against a cluster still forming.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 3 new inline findings.
Summary: #1593 (comment)
Review feedback on #1593 (NumaryBot): after a cycle-time full-backup retake, the incremental taken in the same quiesce window can carry an empty delta — the restore then replays no exports and the cycle records ok without exercising RebuildDelta. backup_exec now keeps the command's --json output and the cycle fails when it exported nothing, mirroring the local runner's no-exports guard.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 3 new inline findings.
Summary: #1593 (comment)
Review feedback on #1593 (NumaryBot): a cycle failing after teardown_cluster left the recovery path applying the normal-mode spec — bringing up a FRESH, EMPTY cluster (the PVCs are gone; the data exists only in the backup) and releasing the driver into it, where every model check would report false divergence. The cycle is now split at the point of no return. Pre-teardown failures (backup, exports guard) leave the running cluster untouched and report err as before. From the teardown onward the restore choreography is retried until it lands, clearing any half-state between attempts; the driver's lease expiry merely parks it on transient retries until the restored cluster serves. The verified quiesce-point backup is what makes the teardown safe to cross in the first place.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 5 new inline findings.
Summary: #1593 (comment)
Observed in the Antithesis run: a network partition ate the exec response of a restore download that had completed server-side, and every retry then failed on FailedPrecondition "backup already downloaded" until the cycle gave up. The download and finalize retries now recognise the FailedPrecondition a repeat hits as prior success — "already downloaded" for download, and "no backup downloaded" for finalize, which consumed the staging the download step already proved existed.
…uard Review feedback on #1593 (NumaryBot): ledgerctl's --json output can be pretty-printed across lines, which the single-line grep never matched — the exports guard would then read empty and fail every cycle. The backup capture now separates stdout (pure JSON, fed to jq whole, the same way the local runner parses it) from stderr (kept for failure logs).
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 7 new inline findings.
Summary: #1593 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 10 new inline findings.
Summary: #1593 (comment)
…sidecar The sidecar reused the workload image, and any container whose image carries /opt/antithesis/test/v1 hosts those templates too: the Composer scheduled main/first_default_ledger into the sidecar, where the workload env (LEDGER_GRPC_ADDR, ...) is absent, so it dialed the localhost fallback and died — failing the "Commands finish with zero exit code" Always property on every k8s run of this branch (present in takes 3-4, absent in pre-sidecar baselines). The sidecar now uses a --target sidecar image variant that strips the test tree.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 4 new inline findings.
Summary: #1593 (comment)
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
Current-head re-review on e01cdee3: this PR has materially changed since my approval at 32b47dd, so that approval is no longer valid. Two state-safety blockers remain in the existing threads: the driver can resume after its restore lease expires while the sidecar is already past teardown and still retrying recovery, and an incomplete teardown is allowed to continue into restore against potentially stale PVCs. In addition, the Antithesis success property is only registered after a completed cycle, so a zero-cycle run provides no failing coverage signal. I replied on the existing threads rather than duplicating findings. Repository CI is green; NumaryBot remains red.
Review feedback on #1593 (NumaryBot): a request the driver had given up on (lease expiry) stayed on disk, so a late orchestrator could pick it up and cut the backup while the driver was committing again — the restore would then erase committed state and poison every model check. Both sides now take the request file by rename, so exactly one wins: the orchestrators claim it to start a cycle, the driver withdraws it on lease expiry or shutdown. If the claim already happened, the driver stays parked until the orchestrator answers — resuming mid-cycle has the same poisoning effect (the cycle may already be past the teardown).
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 7 new inline findings.
Summary: #1593 (comment)
Azorlogh
changed the title
Review feedback on #1593 (flemzord): the point-of-no-return crossed into the restore even when the teardown had failed partway, so a lingering PVC could hand the restore-mode pod its old store back — stale data served as "restored", hollow RebuildDelta coverage. teardown_cluster is now idempotent (--ignore-not-found on the CR), verifies every claim is actually gone before reporting success, and is retried until it does — both at the point of no return and between restore attempts, where the inline half-state cleanup duplicated it.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot review complete: no remaining inline findings.
Resolved 7 stale NumaryBot review threads (7 fixed, 0 outdated).
Summary: #1593 (comment)
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@flemzord on the third point from your review (zero-cycle runs giving no failing signal): the instrumentor registers every assertion call site statically, so the Sometimes property exists in the report even when never executed. Empirically, run The two thread findings are addressed at head (1a5ff58, bf2d0bd) — re-requesting your review. |
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
Approved current head bf2d0bd. The restore-cycle blockers from my previous review are fixed: the file rendezvous now uses an atomic claim/withdraw so an expired unclaimed request cannot later start a restore after the driver resumes, and the orchestrator now enters restore only after teardown has verifiably removed the old cluster storage. Required checks are green.
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
Approved current head bf2d0bd. The two restore-state blockers from my previous review look addressed: request ownership is now atomic claim/withdraw, so the driver does not resume while an orchestrator cycle is already in flight, and teardown is retried/verified through CR, pod, and PVC removal before restore mode starts. CI is green.
3 participants
Follow-up to #1534. Adds backup→restore coverage to the model test: the driver periodically quiesces itself, triggers a full restore, and resumes against the rebuilt store — so every continuous model check (committed bulks, failures, account/ledger/transaction/schema reads) validates the state
RebuildDeltareconstructed, with no dedicated post-restore assertions needed. Wired both locally and on Antithesis k8s.How it works
restore.go): a restore cycle fires every ~45s (180s on k8s) —pauseAndDraingates the workers and waits for every in-flight bulk and read to resolve, then signals an external orchestrator through a file rendezvous and blocks until the restore completes, then resumes. The request file is taken by rename on both sides, so exactly one wins: the orchestrator claims it to start a cycle, the driver withdraws it on lease expiry — and if the claim already happened, the driver stays parked until the cycle answers rather than resuming mid-restore.run_model_test.sh --restore, single-node): starts MinIO, takes a full backup of the empty store at boot so all activity lands in incremental deltas, then per cycle: incremental backup → SIGKILL the node →ledgerctl store bootstrapfrom the backup (RebuildDelta) → relaunch on the rebuilt store.restore-orchestrator.sh, sidecar in the workload pod): services the same rendezvous with a full disaster-recovery pass through the operator — incremental backup at the quiesce point (execledgerctlin a ledger pod) → delete the Cluster CR and every PVC → re-create the Cluster in restore mode →restore download+finalize(RebuildDelta) → flip back to normal mode, where pod 0 self-bootstraps from the restored store and the other replicas join fresh via snapshot install. Fault-hardened: teardown is verified complete before the point of no return, the post-teardown restore retries until it lands (the driver's state exists only in the backup from there), download/finalize recognise lost-response retries as prior success, and an exports guard fails cycles whose delta would make RebuildDelta a no-op. The sidecar uses an image variant without/opt/antithesis/testso the Composer never schedules test commands into it.Sometimesproperty (singleton_driver_model: restore cycle completed) is registered statically, so a run with zero completed cycles reports it failing; the local runner additionally fails on zero cycles or any failed cycle.What it has already caught
Restore bugs fixed this week were found by exactly this cycle:
b:false, restored server returneds:false).Locally, a 300s run with 5 restore cycles passes clean.
On Antithesis (runs
87dbab…-57-8,5e600bbb…-57-8, 1h each), the restore property passes — and the cycle surfaced a further, still-open SUT finding: right after a restore, the freshly-joined follower's post-commit sentinel firesaggregated volume imbalanceon its first applies and the node crash-loops (all hits on the snapshot-installed follower, never on the pod that booted from the restored store; a run whose cycles never completed shows no hits). Those runs' redUnreachableproperties trace back to this, not to the harness.