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: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

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>

reneradoi

Hi Mykola! In addition to the detailed comments below, I want to add a couple of general feedback points:

I like the design of the restore workflow, but I think the separation of concerns can be improved. This might seem a little nitpicking now, but for the long term, this will be very important to maintain clear responsibilities per manager, and clear decoupling between event handling and business logic.
The integration test has a lot of points that we already brought as feedback for the backup integration test PR. I skipped detailled review and would like to ask you to first update the integration test with the feedback from the previous PR.
The unit tests should go through ops.testing and be based on events, as far as possible. I see a lot of very low-level unit testing, which is contrary to what we typically do. I would like to keep the style of our project, for a variety of reasons. One of them being that the integration of components through ops has to work at any time (for example data-interfaces for the peer relation). Another one is that any kind of refactoring will end up in a total rework of unit tests, which is not what we typically want.

In general, I agree with the concept of the restore workflow. Testing from my side will follow when the design and logic is settled.

reneradoi · 2026-07-07T08:06:14Z

+                reraise=True,
+            ):
+                with attempt:
+                    if not self.state.charm.cluster_manager.is_replica_synced():  # pyright: ignore[reportAttributeAccessIssue]


This line highlights a conflict with the general design patterns that I noticed: By adding methods like wait_until_resynced or is_local_primary to the backup manager, it effectively takes over responsibilites from the cluster and sentinel managers. This results in calling one manager from another, which we should avoid, because it creates dependencies between the managers.

As a resolution, I recommend to call the cluster and/or sentinel manager from the backup events to check whether a replica is synced, or whether a Valkey instance is the primary. If the existing methods in the managers don't provide what is required (e.g. retry mechanism), it should be added there.

Fixed. Moved is_local_primary → ClusterManager.is_primary, and the bounded wait_until_resynced / wait_until_loaded into ClusterManager (raising a generic ValkeyClusterNotReadyError on timeout); the backup events call them directly. The backup manager no longer reaches into cluster_manager via state.charm, and holds no Valkey client for restore. bd32226, e1e9900

reneradoi · 2026-07-07T08:09:21Z

+        unit = self.charm.state.unit_server
+        bm = self.charm.backup_manager
+


nit: Can we avoid these? It makes it harder to understand the code if it is always required to check what exactly unit.update is, whereas self.charm.state.unit_server.update is precise and understandable at every single point in the code.

Done — dropped the unit/cluster/bm locals in the restore handlers; each access spells out self.charm.state.unit_server / .cluster / the manager in full. 1326b81

reneradoi · 2026-07-07T08:55:52Z

+                # Not our turn: tuple doesn't match a valid transition.
+                return
+
+    def _do_primary_restore(self) -> None:


This is backup/restore business logic and should be moved to the backup manager.

The low-level work is now all in the manager — restore_on_primary (stop/move-aside/download/start), roll_back, and verify_backup_is_rdb are BackupManager methods. What remains in _do_primary_restore is orchestration + error handling: verify → restore → cluster_manager.wait_until_loaded → roll_back on failure. Two constraints keep that in the event rather than the manager:

Per your fix: unblock CI integration tests #42, the post-check (wait_until_loaded) is called from the event handler "where errors can be handled accordingly," and the rollback wrapper has to cover both the restore and that post-check — so it belongs where both are called.

verify_backup_is_rdb deliberately runs outside the rollback try (before the primary is stopped), which is the regression fix in 52031de — a bad backup-id must fail while the primary still serves, with nothing to roll back.

Folding this into the manager would reintroduce the manager→manager coupling from fix: unblock CI by switching to self-hosted runners #35 (the backup manager calling cluster_manager.wait_until_loaded).

So I've kept _do_primary_restore as thin event-layer orchestration.

reneradoi · 2026-07-07T08:56:13Z

+            bm.roll_back()
+            raise
+
+    def _advance_if_leader(self) -> None:


This is backup/restore business logic and should be moved to the backup manager.

#79 (comment)

reneradoi · 2026-07-07T09:02:53Z

+        if s3_parameters is None:
+            raise ValkeyRestoreError("S3 credentials unavailable")


No need to check here, we do gatekeeping when the restore action is started and once in progress, the credentials will not be removed anymore.

#79 (comment)

reneradoi · 2026-07-07T09:24:27Z

+        # Data partition is now free; download the restore RDB directly onto it.
+        self.download_backup(self.state.cluster.restore_id)
+        self.workload.start_service(self.workload.valkey_service)
+        self._wait_until_loaded()


We can decouple here and also have clearer responsibilites by deviding the restore and the post-check: wait_until_loaded should be in the cluster manager and called from the event handler, where errors can be handled accordingly. This would also allow for the valkey client to be removed from the backup manager, which currently is a syndrom of "cluttered" design, as the backup manager does not actually need to connect to Valkey itself to perform the tasks it is responsible for (other than self.workload.exec_stream for creating the backup, which is encapsulated in the workload already).

Done — wait_until_loaded is now a ClusterManager method, called from the event after restore_on_primary; restore_on_primary no longer health-waits internally. All restore Valkey-client usage is out of the backup manager. One caveat: the ValkeyClient import stays because create_backup's _build_rdb_command (pre-existing, from #59) still uses it to build the valkey-cli --rdb - argv — removing that is a change to the merged backup-create path, out of scope for this PR. bd32226, e1e9900.

reneradoi · 2026-07-07T09:33:15Z

+
+import jubilant
+import pytest
+from tests.integration.backup.test_s3_backup import (


Let's move them to a helpers file in the backup directory if we need them in multiple tests.

Done — moved the shared setup into tests/integration/backup/helpers.py (deploy_and_relate_s3, S3_INTEGRATOR_APP, BACKUP_ID_RE); both the backup and restore tests now use it. 1c0d1ef

reneradoi · 2026-07-07T09:34:13Z

+    status = juju.status()
+
+    if APP_NAME not in status.apps:
+        juju.deploy(APP_NAME, channel="9/edge", num_units=3, trust=True, base="ubuntu@24.04")


Same comment as in the PR for backup integration test: We ALWAYS want to test against our local charm version, otherwise the test is pointless.

Fixed — the restore test now deploys the local charm via the charm fixture (deploy_and_relate_s3), same as the backup test, instead of a Charmhub revision. 1c0d1ef

reneradoi · 2026-07-07T09:35:03Z

+    )
+    juju.run(
+        f"{S3_INTEGRATOR_APP}/0",
+        "sync-s3-credentials",


Please update this test with the feedback from the backup PR; this action has been replaced by a user secret.

Fixed — switched to the Juju user-secret flow (add_secret → grant_secret → credentials config) against s3-integrator 2/edge; the sync-s3-credentials action is gone. Shared with the backup test via the helper. 1c0d1ef

…h waits Give the cluster manager the restore-facing checks it already had the machinery for (it owns all role()/replica-sync logic): - is_primary(): local server reports the master role. - wait_until_loaded(timeout_s): bounded poll until the server responds and finished loading; raises ValkeyClusterNotReadyError on timeout. - wait_until_resynced(timeout_s): bounded poll until this replica's link is in sync; raises ValkeyClusterNotReadyError on timeout. Additive only -- the backup manager still owns the restore copies for now; the next commit moves the call sites over and drops them there (PR #79 review: keep primary/replica-sync checks in the cluster manager, not the backup manager). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Move the restore's primary-check and post-restore health waits off the backup manager and onto the cluster manager, so the backup manager no longer connects to Valkey at all for a restore (PR #79 review: separation of concerns / decouple the managers). - events/backup.py orchestrates: cluster_manager.is_primary() picks the restore role, wait_until_loaded() confirms the primary came up healthy (rolling back on failure), and wait_until_resynced() waits out each replica. The timeouts stay in the restore domain and are passed in. - backup.py loses _valkey_client, is_local_primary, _wait_until_loaded and wait_until_resynced (which had reached back into cluster_manager via state.charm); restore_on_primary no longer health-waits internally. - Teardown maps ValkeyClusterNotReadyError -> RESTORE_UNHEALTHY; the now unraised ValkeyRestoreUnhealthyError is removed. The ValkeyClient import stays only for create_backup's _build_rdb_command (pre-existing #59 code), which is out of scope here. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…ndlers Drop the local `unit`/`cluster`/`bm` aliases in the restore event handlers and spell out self.charm.state.unit_server / .cluster and the managers in full at each use, so every access says precisely which part of state (or which manager) it touches (PR #79 review #36). Comment/style only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…rtition download_backup buffered the whole S3 object into a charm-local NamedTemporaryFile before pushing it to the workload, so a restore needed RDB-sized free space in the (small, ephemeral) charm container and could fail for a large dataset (PR #79 review #41). Pass the S3 object's StreamingBody directly to push_data_file instead: it copies in bounded chunks (VM: copyfileobj; K8s: Pebble push), so the full object is never buffered whole in the charm regardless of RDB size. It still lands under dump.rdb.part and is atomically renamed onto dump.rdb, so the final file never appears partial. The RDB magic is already validated up front by verify_backup_is_rdb (a tiny ranged GET -- not a full download), so dropping the post-download head check loses no coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…h waits Give the cluster manager the restore-facing checks it already had the machinery for (it owns all role()/replica-sync logic): - is_primary(): local server reports the master role. - wait_until_loaded(timeout_s): bounded poll until the server responds and finished loading; raises ValkeyClusterNotReadyError on timeout. - wait_until_resynced(timeout_s): bounded poll until this replica's link is in sync; raises ValkeyClusterNotReadyError on timeout. Additive only -- the backup manager still owns the restore copies for now; the next commit moves the call sites over and drops them there (PR #79 review: keep primary/replica-sync checks in the cluster manager, not the backup manager). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Move the restore's primary-check and post-restore health waits off the backup manager and onto the cluster manager, so the backup manager no longer connects to Valkey at all for a restore (PR #79 review: separation of concerns / decouple the managers). - events/backup.py orchestrates: cluster_manager.is_primary() picks the restore role, wait_until_loaded() confirms the primary came up healthy (rolling back on failure), and wait_until_resynced() waits out each replica. The timeouts stay in the restore domain and are passed in. - backup.py loses _valkey_client, is_local_primary, _wait_until_loaded and wait_until_resynced (which had reached back into cluster_manager via state.charm); restore_on_primary no longer health-waits internally. - Teardown maps ValkeyClusterNotReadyError -> RESTORE_UNHEALTHY; the now unraised ValkeyRestoreUnhealthyError is removed. The ValkeyClient import stays only for create_backup's _build_rdb_command (pre-existing #59 code), which is out of scope here. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…ndlers Drop the local `unit`/`cluster`/`bm` aliases in the restore event handlers and spell out self.charm.state.unit_server / .cluster and the managers in full at each use, so every access says precisely which part of state (or which manager) it touches (PR #79 review #36). Comment/style only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…rtition download_backup buffered the whole S3 object into a charm-local NamedTemporaryFile before pushing it to the workload, so a restore needed RDB-sized free space in the (small, ephemeral) charm container and could fail for a large dataset (PR #79 review #41). Pass the S3 object's StreamingBody directly to push_data_file instead: it copies in bounded chunks (VM: copyfileobj; K8s: Pebble push), so the full object is never buffered whole in the charm regardless of RDB size. It still lands under dump.rdb.part and is atomically renamed onto dump.rdb, so the final file never appears partial. The RDB magic is already validated up front by verify_backup_is_rdb (a tiny ranged GET -- not a full download), so dropping the post-download head check loses no coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…r-secret creds align the restore test with it and stop deploying a Charmhub revision with the removed sync-s3-credentials action (PR #79 review #44/#45/#46). - Add tests/integration/backup/helpers.py with S3_INTEGRATOR_APP / BACKUP_ID_RE and an idempotent deploy_and_relate_s3 (local charm + user-secret credentials, s3-integrator 2/edge), shared by both backup and restore tests. - test_s3_restore.py: deploy the local charm, drop the broken `from test_s3_backup import (...)` / _wait_active / _deploy_cluster_and_s3, and make every restore scenario independently runnable via the idempotent helper. - test_s3_backup.py: call the shared helper instead of its inline deploy block. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Align the action name with charmed-etcd-operator's `restore` action (same single `backup-id` string param). The action is unreleased (this PR), so the rename carries no backward-compatibility concern. Renamed the actions.yaml key, the observed event (restore_backup_action -> restore_action), the audit log label, and the `juju run ... restore` calls in the integration tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

skourta

Thank you @delgod. I left a couple of comments until I do some manual testing.

skourta · 2026-07-08T07:47:36Z

+                "restore_participants": participants,
+            }
+        )
+        event.set_results({"restore": f"initiated for {backup_id}"})


update_status is unreliable. But even if it is disabled when a unit writes to its databag the leader will continue correct?

skourta · 2026-07-08T07:55:26Z

+            return
+        instruction = cluster.restore_instruction
+        if instruction == RestoreStep.COMPLETED:
+            self.charm.state.statuses.delete(


@Mehdi-Bendriss

skourta · 2026-07-08T08:00:53Z

            self.workload.move_file(self._dump_path, self._pre_restore_path)
-        self.workload.move_file(self._download_path, self._dump_path)
+        # Data partition is now free; download the restore RDB directly onto it.
+        self.download_backup(self.state.cluster.restore_id)


We should download the backup file before we stop the service. If the file is very large we can still serve users until we are ready. Just to minimize the downtime.

Mehdi-Bendriss

Thank you Mykola! I really like the concept! I have some comments, one that is major on the juju leader vs valkey primary

Mehdi-Bendriss · 2026-07-08T16:55:37Z

-    error code -- a fixed, non-sensitive token such as "AccessDenied" --
-    is surfaced. Everything else collapses to a generic message; the full
-    detail stays in the unit log.
+    Action results are world-readable, so surface only the structured S3 error


Suggested change

Action results are world-readable, so surface only the structured S3 error

Action results are world-readable, so surface only the structured object-storage error

Mehdi-Bendriss · 2026-07-08T16:58:48Z

-        # S3Parameters trims whitespace, strips the separators that would
-        # corrupt S3 key paths, and rejects an envelope missing a required
-        # field or whose path/bucket strips to empty.
+        # S3Parameters trims/validates the envelope and rejects missing/empty fields.


can we replace envelope by object storage integrator payload or something more natural ?

Mehdi-Bendriss · 2026-07-08T17:19:06Z

-        # Audit the invocation itself, not just the manager-level transfer
-        # (P1-24): ties a specific Juju action run to the resulting backup,
-        # for forensics if an RDB later turns up somewhere unexpected.
+        # Audit the action invocation (P1-24): ties an action run to its backup.


2 questions here:

What does P1-24 mean?

what do we audit here? or do we mean by that logging?

Mehdi-Bendriss · 2026-07-08T18:31:02Z

+        backup_id = event.params.get("backup-id", "")
+        if not backup_id:


nit, walrus 😬

Mehdi-Bendriss · 2026-07-08T18:33:05Z

+            if backup_id not in self.charm.backup_manager.list_backups():
+                event.fail(f"backup-id {backup_id} not found.")
+                return
+        except ValkeyBackupError as e:


can we log the exception here?

Mehdi-Bendriss · 2026-07-08T22:45:33Z

+        self.workload.stop_service(self.workload.valkey_service)
+        if self.workload.path_exists(self._pre_restore_path):
+            self.workload.move_file(self._pre_restore_path, self._dump_path)
+        self.workload.start_service(self.workload.valkey_service)


Similar comment on post-restart operations.

this method is funny, it seems like it's the exact same yet opposite of restore_on_primary

Mehdi-Bendriss · 2026-07-08T22:46:44Z

+            self.workload.move_file(self._dump_path, self._pre_restore_path)
+        # Data partition is now free; download the restore RDB directly onto it.
+        self.download_backup(self.state.cluster.restore_id)
+        self.workload.start_service(self.workload.valkey_service)


Given we are doing a stop / start manually directly, it doesn't do all the graceful pre-stop / post-start operations we do in _on_restart_workload (e.g: reconcile_min_replicas_to_write, checing that sentinel is healthy etc.).

So they should either be all put here, or we should have it orchestrated through a similar graceful rolling mechanism like _on_restart_workload

Mehdi-Bendriss · 2026-07-08T22:49:58Z

+            self.charm.sentinel_manager.get_primary_ip()
+        except ValkeyCannotGetPrimaryIPError:
+            return "No primary available; cannot restore."
+        if self.charm.sentinel_manager.is_failover_in_progress():


Missing exception handling of ValkeyWorkloadCommandError

Mehdi-Bendriss · 2026-07-08T22:51:27Z

+            return "A restore is in progress; backups are paused."
        return None
+
+    def _restore_blocking_reason(self) -> str | None:


I feel like we could extract a lot of common code between _blocking_reason and this method

I think we should also sefeguard against TLS transition states

I think in _blocking_reason we could check it, only if want to check_running_operations=True (as the list backups will use the s3 client not valkey)

Mehdi-Bendriss · 2026-07-08T22:55:10Z

@@ -212,4 +215,195 @@ def _blocking_reason(self, check_running_operations: bool = True) -> str | None:
            return "Valkey is not running on this unit."
        if check_running_operations and self.charm.state.unit_server.is_backup_in_progress:


Why do we check only on the current unit doing a backup while we check for restore ALL the cluster?

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

reneradoi reviewed Jul 7, 2026

View reviewed changes

delgod and others added 4 commits July 7, 2026 18:53

delgod force-pushed the s3-restore branch from 9b4cfba to e0d16fa Compare July 7, 2026 16:53

delgod and others added 3 commits July 8, 2026 08:21

Merge remote-tracking branch 'origin/9/edge' into s3-restore

bb0f911

delgod force-pushed the s3-restore branch from 8bc493a to febc53f Compare July 8, 2026 07:52

skourta reviewed Jul 8, 2026

View reviewed changes

Mehdi-Bendriss reviewed Jul 8, 2026

View reviewed changes

		unit = self.charm.state.unit_server
		bm = self.charm.backup_manager

		if s3_parameters is None:
		raise ValkeyRestoreError("S3 credentials unavailable")

	Action results are world-readable, so surface only the structured S3 error
	Action results are world-readable, so surface only the structured object-storage error

		backup_id = event.params.get("backup-id", "")
		if not backup_id:

		@@ -212,4 +215,195 @@ def _blocking_reason(self, check_running_operations: bool = True) -> str \| None:
		return "Valkey is not running on this unit."
		if check_running_operations and self.charm.state.unit_server.is_backup_in_progress:

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

reneradoi left a comment

Choose a reason for hiding this comment

Uh oh!

reneradoi Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

delgod Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

reneradoi Jul 7, 2026 •

edited

Loading

delgod Jul 7, 2026 •

edited

Loading

delgod Jul 7, 2026 •

edited

Loading

delgod Jul 7, 2026 •

edited

Loading