feat(restore): S3 backup restoration (restore-backup action) by delgod · Pull Request #79 · canonical/valkey-operator

delgod · 2026-07-01T04:24:56Z

What

Adds a restore-backup action that restores the cluster's dataset from an S3
backup produced by create-backup. Serves both in-place rollback and
disaster recovery onto a freshly-deployed cluster.

juju run valkey/leader restore-backup backup-id=2026-06-29T14:32:45Z

Leader-only and asynchronous: the action validates and initiates, then the
operator watches juju status (a RESTORE_IN_PROGRESS maintenance status)
until the cluster returns to active.

Approach

valkey is primary/replica via Sentinel with automatic full-resync, so — unlike
etcd's restore-every-node model — restore lands the RDB on only the current
primary and lets replicas resync from it. Coordination is an etcd-style
databag state machine (extends the existing BackupManager/BackupEvents):

DOWNLOAD → RESTORE → RESYNC → COMPLETED, with an app-level target instruction
vs. a per-unit completed step; the leader advances only when every participant
reaches the current step. The genuinely hard part is coordinating Sentinel so it
doesn't fail over while the primary briefly restarts.

Restore replaces only the dataset — ACLs, CharmUsers passwords, and TLS are
charm-managed config and are untouched.

Design highlights

Failover suppression: raises down-after-milliseconds on every sentinel
for the restore window (DOWNLOAD → RESYNC); symmetric teardown resumes it
on every abort path, so a failed restore can't leave Sentinel failover
disabled cluster-wide.
Tuple-match dispatch (instruction, prior_step): a unit acts only from its
exact prior step — a unit that missed DOWNLOAD can never run the destructive
RESTORE.
Fail-closed barrier: iterates a snapshotted participant set; a departed
participant stalls (never silently dropped), so a mid-restore scale change
can't wedge or bypass the gate.
Safe RDB handling: magic-byte validation in-stream during download, written
to a temp name and atomically renamed onto the final name only on full success
(a partial download never carries the final name); dump.rdb preserved as
dump.rdb.pre-restore for rollback.
Ordering/idempotency: stop only valkey-server (not Sentinel) to bracket the
swap; stop_service-first rollback to defeat supervisor auto-restart;
idempotent move-aside so a redelivered hook can't clobber the good pre-restore
copy; generous bounded dataset-aware waits (never hang, never false-pass).
Restore-awareness: peer-relation-changed handlers in base_events,
external_clients, and tls, plus restart_workload, skip work during a
restore; storage-detaching refuses a scale-down mid-restore (it would
otherwise issue a manual Sentinel failover and stop a participant).

Testing
Unit: 202/202; lint + static clean. Covers each state transition, the
fail-closed barrier, the tuple-match guard, rollback + suppression-resume on
any step failure, bounded-wait timeouts, and the guard matrix.
Integration (tests/integration/backup/, MicroCeph + s3-integrator):
rollback, disaster recovery, and a corrupt-restore that asserts the cluster
keeps its old data and Sentinel failover still works afterward.

Backward compatibility

New peer-databag fields (restore_id, restore_instruction,
restore_participants, restore_step, restore_role) default falsy and
tolerate an old-revision databag (9/edge rolls unit-by-unit); the guards are
pure no-ops until a restore is initiated — no impact on normal operation or the
existing backup feature.

…check

…ad primitives

…ore statuses

Register _on_restore_workflow on peer_relation_changed + update_status; dispatch via (instruction, prior-step) tuple match through DOWNLOAD → RESTORE → RESYNC → COMPLETED; primary suppresses/resumes failover; leader advances instruction once all participants reach each barrier (_advance_if_leader + can_restore_workflow_proceed); _restore_teardown resumes suppression and marks RESTORE_FAILED on any abort path. Add restore_id property to ValkeyCluster (needed by _run_restore_step).

…taching refusal)

…tent move-aside FIX 1 (critical): Broaden _on_restore_workflow except clause from a narrow tuple to except Exception as e: so ValkeyServicesCouldNotBeStoppedError / ValkeyServicesFailedToStartError (standalone Exception subclasses outside the restore-error hierarchy) no longer escape _restore_teardown -> resume_failover(). Also broadens _do_primary_restore to except Exception: so any service-control error triggers roll_back() before propagating. FIX 2: Guard the move-aside in restore_on_primary() with path_exists(pre_restore). On a redelivered hook the .pre-restore file already holds ORIGINAL data; an unconditional second move-aside would clobber it. FIX 3: _restore_teardown(exc) sets RESTORE_UNHEALTHY on ValkeyRestoreUnhealthyError, RESTORE_FAILED for all other failures. FIX 4: Correct download_backup() docstring - buffers via BytesIO (MVP tradeoff). FIX 5: Add event.set_results() before event.fail() in empty-backup-id branch. Tests: 5 new tests in test_restore.py covering service-error teardown path, RESTORE_UNHEALTHY status selection, idempotent move-aside, _wait_until_loaded timeout, and unknown-backup-id rejection.

skourta

I left several comments/questions I will do manual testing next.

skourta · 2026-07-02T10:36:45Z

+
+        Fail-closed: iterate the snapshotted participant *names* and look each
+        up in the live servers. A participant absent from the live set (it
+        departed mid-step) counts as NOT reached, so the gate stalls rather


This can be better explained. I only understood the comment when I read the code.

Fixed in b411469

skourta · 2026-07-02T10:48:47Z

+        except ValkeyCannotGetPrimaryIPError:
+            return "No primary available; cannot restore."
+        if "failover_in_progress" in (
+            self.charm.sentinel_manager._get_sentinel_client()


If we have to use this then we should make it "public". Do we not have a function that checks this?

Fixed in ec94e9f

skourta · 2026-07-02T10:57:27Z

+        try:
+            self._run_restore_step(instruction, step, role)
+        except Exception as e:
+            # Broad catch is deliberate: _restore_teardown -> resume_failover() is


Unclear comment

Fixed in b411469

skourta · 2026-07-02T11:03:48Z

+    def _do_primary_restore(self) -> None:
+        """Re-download the RDB if missing, then restore in-place; roll back on unhealthy state."""
+        bm = self.charm.backup_manager
+        if not self.charm.workload.path_exists(bm._download_path):


Same if the path is used from outside make it "public"

Fixed in c2ae788

skourta · 2026-07-02T11:04:32Z

+        """Re-download the RDB if missing, then restore in-place; roll back on unhealthy state."""
+        bm = self.charm.backup_manager
+        if not self.charm.workload.path_exists(bm._download_path):
+            bm.download_backup(self.charm.state.cluster.restore_id)


I thought the download step and restore are separate

This way we download twice the backup file

Fixed in c2ae788

skourta · 2026-07-02T11:19:43Z

+
+        # boto3 writes the whole object into this BytesIO buffer so we can
+        # inspect the magic header before committing the file to disk.
+        buffer = io.BytesIO()


This is very memory consuming. To ensure a proper restore we would need to have at least the size of the rdb file free in memory.

Fixed in 2883142

skourta · 2026-07-02T11:21:38Z

+            )
+
+        buffer.seek(0)
+        self.workload.push_data_file(


I don't understand if we're downloading into memory why do we write the part file then rename it?

Is it to protect against possible io writing issues?

Fixed in 2883142

skourta · 2026-07-02T11:28:11Z

+    def wait_until_resynced(self) -> None:
+        """Bounded poll until this replica reports a connected, in-sync link.
+
+        Purpose-built: the stock ``wait_for_replica_fully_synced`` has no ceiling


Comment also hard to understand

Fixed in b411469

skourta · 2026-07-02T11:43:49Z

+    substrate: Substrate,
+) -> None:
+    """Remove the app entirely, redeploy a fresh cluster, restore from S3 -- data comes back."""
+    _write_key(juju, "dr_key", "dr-value")


I would very much like our tests to be independently executable. If the cluster is not deployed we can deploy one.

Fixed in eeaf72c

skourta · 2026-07-02T11:45:12Z

+
+    # Old data must still be present (restore rolled back or never committed).
+    got = _read_key(juju, _leader_unit_name(juju), "safe_key")
+    assert got == "safe-value", f"Old data lost after corrupt restore; got {got!r}"


Why aren't we checking on status?

Fixed in eeaf72c

Collapse the DOWNLOAD step into RESTORE: the primary now suppresses failover, downloads, and swaps in the RDB in a single sweep, while replicas only record the step. suppress_failover already configures every sentinel in one call, so a separate DOWNLOAD barrier only added a hook round-trip with no coordination benefit (PR #79 review). Drop RestoreStep.DOWNLOAD, rework the (instruction, prior-step) tuple dispatch, and remove the re-download fallback in _do_primary_restore (download now always precedes restore in the same step), which also stops the events layer reaching into BackupManager._download_path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…mory download_backup buffered the entire object in an io.BytesIO in the charm process, needing RDB-sized free memory for a restore (PR #79 review). Stream the S3 object into a tempfile.NamedTemporaryFile instead -- O(1) memory -- validate the magic header from the temp file, then push to the workload and atomically rename onto the final name as before. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…ore guard _restore_blocking_reason reached into sentinel_manager._get_sentinel_client() and re-implemented the "failover_in_progress" flag test inline (PR #79 review). Add SentinelManager.is_failover_in_progress() -- a deliberately non-retrying snapshot, unlike the client's @retry-decorated version which would block the action guard for minutes -- and call it instead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…uards Rewrite the comments flagged as hard to follow in PR #79 review: can_restore_workflow_proceed (fail-closed rationale), the broad-except in _on_restore_workflow, and wait_until_resynced. Also document why the peer-relation guards in base_events / external_clients / tls return during a restore -- the skipped reconcile self-heals on the post-restore relation-changed, and deferring a departed event is unsafe. Comment-only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…on status Per PR #79 review: call the idempotent _deploy_cluster_and_s3 at the top of the disaster-recovery test so it runs standalone, and wait on the RESTORE_FAILED app status (a real convergence signal) instead of a bare sleep in the corrupt-restore test. Also drop the stale DOWNLOAD step from a comment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Condense the multi-paragraph comments and docstrings added across the restore feature to concise, effective one/two-liners. Comment-only: no code logic changes. Also fix two stale references to the removed DOWNLOAD step (restore_role docstring and the role= inline comment). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Resolve conflict in src/events/base_events._on_storage_detaching: keep the storage feature's is_being_removed early-return and _scale_down_unit split (#78) and broaden the pre-scale-down guard to also refuse scale-down while a restore is in progress. Update the restore unit test for the new guard order. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…x both partitions) Both the data and archive partitions now need only ~1x the dataset. The download and the dump.rdb.pre-restore rollback copy previously shared the data partition (2x data). Since a restore discards the currently served data anyway, downtime is not a concern, so optimise for space and I/O instead: keep only the pre-restore rollback copy on the archive partition (1x), and download the restore RDB directly onto the data partition as dump.rdb (after moving the old dump aside). The new dump is written once -- no staging copy -- and the install is a same-partition dump.rdb.part -> dump.rdb rename, not a cross-device copy. The one remaining cross-partition move (dump -> archive/pre-restore, and the reverse on rollback) uses shutil.move on VM (os.replace raises EXDEV across partitions); K8s mv already handles it. Crash-safety is unchanged: pre-restore copy + idempotent move-aside + rollback. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Magic-byte validation previously happened only during the download, which now runs after valkey is stopped -- so a missing or non-RDB backup-id would bounce the primary (stop -> download fails -> rollback -> restart). Add verify_backup_is_rdb: a cheap ranged GET of the first bytes, run in _do_primary_restore before the stop and outside the rollback wrapper, so a bad object fails while the primary is still serving, with nothing to roll back. The full-stream magic check during download stays as defence in depth. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

delgod added 13 commits July 1, 2026 06:24

feat(restore): add RestoreStep enum and restore constants

30b65d7

feat(restore): add restore databag fields and server/cluster properties

5223741

feat(restore): add fail-closed restore barrier and cross-unit backup …

dbcd050

…check

feat(restore): add RestoreStatuses

9efab1b

feat(restore): add per-service stop/start and binary push/move worklo…

2653409

…ad primitives

feat(restore): add Sentinel failover suppress/resume

bb0dade

feat(restore): download+verify RDB in-stream with atomic placement

51aebe6

feat(restore): restore_on_primary, roll_back, bounded load wait, rest…

ad59412

…ore statuses

feat(restore): add restore-backup action, guards, credentials-gone guard

c2fc8b7

feat(restore): make event handlers restore-aware (guards + storage-de…

ebcc9a4

…taching refusal)

test(integration): S3 restore rollback, DR, and corrupt-restore+failover

8a13d6b

delgod requested review from Mehdi-Bendriss, reneradoi and skourta July 1, 2026 04:24

skourta reviewed Jul 2, 2026

View reviewed changes

delgod and others added 8 commits July 6, 2026 08:27

delgod force-pushed the s3-restore branch from c5a1700 to b300db8 Compare July 6, 2026 07:50

delgod force-pushed the s3-restore branch from aedbcd7 to 52031de Compare July 6, 2026 08:23

Uh oh!

Conversation

delgod commented Jul 1, 2026

What

Approach

Design highlights

Testing

Backward compatibility

Uh oh!

skourta left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants