[DPE-10650] 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.

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

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

fixed in 6a83ef1

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 ?

Done — went with payload, across events and models (6a83ef1).

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?

P1-24 was an internal requirement id — dropped it. It's just an audit log line tying the action invocation to the backup it produces (6a83ef1).

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

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


nit, walrus 😬

Fixed in 6a83ef1

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?

Done — logger.exception added (2bd3c9c).

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

Post-restart min-replicas is now reasserted for both the restore and the rollback restart (in a finally). The roll_back/restore_on_primary symmetry is intentional — rollback is the mirror image (2bd3c9c).

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

Went with "put them here": inline pre-stop save_database_blocking + post-start reconcile_min_replicas_to_write. No sentinel-health gate — restore restarts only valkey-server (not sentinel), failover is suppressed then resumed + SENTINEL RESET, and valkey readiness is gated by wait_until_loaded (2bd3c9c).

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

Done — now caught in _unstable_primary_reason (2bd3c9c).

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)

Extracted _unstable_primary_reason for the shared primary/failover checks; left the 2-line S3 checks inline (a helper there is more indirection than it's worth).

TLS-transition guard added (is_tls_transitioning).

Restore preflight uses the valkey path; list-backups stays on the S3 client (2bd3c9c).

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?

Intentional: a backup runs on a single unit, so it only blocks a second backup on that same unit; a restore is cluster-wide. (Separately, _on_s3_credentials_gone was widened to cluster-wide in da1136f.)

…eader The valkey primary that runs the RDB swap is Sentinel-elected, independent of the juju leader, and only the leader can clear the app-level restore_id. A failure on a non-leader primary (or a replica timing out in RESYNC) could not clear it, wedging the cluster in restore-in-progress forever -- and since _on_restore_workflow re-runs per event (update_status plus the relation_changed cascade re-emitted from tls), the destructive _do_primary_restore re-ran on every hook. The failing unit now records a RestoreFailure marker on its own databag; the leader observes any participant's marker (ClusterState.failed_restore_kind) and clears the shared restore state, while each unit stops re-running its step once it has failed. resume_failover in teardown is best-effort so a raise there can't re-wedge the restore or leave Sentinel failover suppressed. Hardening from the follow-up review: - Markers are scoped to a per-attempt token (restore_id is the backup-id and repeats on a same-backup re-run), so a stale marker can't false-abort a later restore. - The leader also resumes failover as a backstop, since a failing peer's own best-effort resume may raise and otherwise leave suppression leaked. - Both terminal statuses (RESTORE_FAILED and RESTORE_UNHEALTHY) are cleared on re-initiation and on completion, so a healthy restored cluster isn't left showing a stale blocked status. PR #79 review (Mehdi-Bendriss). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…primary Force the valkey primary onto a non-leader unit (SIGKILL failover), fail a restore of a corrupt object, and assert the leader tears it down (reaches RESTORE_FAILED), old data survives, and the cluster is usable again (create-backup is no longer blocked) -- the leader != primary case that used to wedge restore_id. PR #79 review (Mehdi-Bendriss). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Address PR #79 review (Mehdi-Bendriss): - Refuse a restore while a TLS transition (client enable/disable or CA rotation) is in flight: the restore restarts the primary and would collide with the cert swap (new ClusterState.is_tls_transitioning). - Treat a Sentinel query error during preflight as an unsettled cluster and fail the action cleanly, instead of letting ValkeyWorkloadCommandError from is_failover_in_progress crash the hook. Extracted the sentinel check into _unstable_primary_reason (also drops the method under the complexity limit). - Persist in-memory data to disk before the swap (cluster_manager .save_database_blocking, pre-stop) so the rollback copy that restore_on_primary moves aside is faithful, not as stale as the last save; a save failure aborts before the primary is touched. - Reassert min-replicas-to-write after the raw restore restart, which bypasses the rolling-restart path that normally reasserts it. - Log the exception when the restore action can't list backups. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…tore _on_s3_credentials_gone already defers while a backup/restore is in progress, but _on_s3_credentials_changed did not: a real credentials rotation mid-restore would swap the bucket/creds the primary is downloading from. The leader now defers the rotation until the operation finishes (creds stay in the databag meanwhile). The CA is still stored first, since the in-flight restore needs it. An unchanged re-fire (e.g. leader_elected) still returns early without deferring. PR #79 review (Mehdi-Bendriss). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Comment/docstring-only pass over the S3 backup/restore series, no behaviour change. Folds the wording nits and the two verbose-comment cleanups from the PR #79 review (Mehdi-Bendriss) into one place: - "S3 error" -> "object-storage error"; "envelope" -> "payload" for the s3-integrator data, across events and models. - Drop the opaque "P1-24"/"P1-2" requirement ids for plain descriptions; use a walrus for the restore action's backup-id check. - Trim the verbose restore comments/docstrings in the workflow, the cluster health waits, the VM cross-device move, and the restore-awareness guards in base_events/external_clients. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Resolve conflicts from the LDAP feature (#81) landing on 9/edge, all keep-both / additive: - statuses.py: keep RestoreStatuses and AuthStatuses. - core/models.py: keep restore_* and ldap_* fields on PeerUnitModel. - core/cluster_state.py: keep the restore properties and the LDAP properties. - managers/cluster.py: keep ValkeyClusterNotReadyError; follow 9/edge dropping the now-unused ValkeyConfigSetError (its use-sites were refactored away there). - tests/unit/test_cluster_manager.py: import both exception symbols. Verified on the merge result: tox lint + static clean, 262 unit tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Add WorkloadBase.service_running(service) with VM (snap active flag) and K8s (Pebble get_service().is_running()) implementations. Callers that gate a stop/restore on "valkey is up" need the single-service state: alive() reports False when any sibling service (e.g. the metrics exporter) is down, which would misread a still-running valkey as stopped. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

suppress_failover now raises SentinelFailoverError if a SENTINEL SET returns non-OK, before the primary is stopped for the restore. Otherwise a sentinel left at the normal down-after could mark the stopped primary down and promote a replica mid-restore (split-brain). resume_failover stays best-effort (logs a non-OK reply, does not raise) so a teardown path can never be re-wedged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

ConfigManager now renders down-after-milliseconds from SENTINEL_DOWN_AFTER_MS instead of a duplicated literal, so the value the charm writes and the value resume_failover reasserts share one source of truth. Also tidy the timeout comments (values unchanged at 600/900). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Two membership-change robustness fixes in the restore state machine: - A unit that joined after a restore was initiated (absent from the restore_participants snapshot) now runs no step. Previously it matched (RESTORE, NOT_STARTED), queried its own not-yet-started valkey, and the resulting teardown fanned resume_failover across every peer while the real primary was stopped mid-swap. - When a participant leaves the peer relation mid-restore (force-removal, lost machine, or a graceful remove-unit), the leader now fails the restore instead of stalling forever. The fail-closed barrier can never be satisfied by a departed unit, and a stuck restore freezes restarts/scaling/TLS/S3 cluster-wide with no escape. Detection is ClusterState.restore_participant_ departed, observed on relation_departed for prompt teardown (update-status as backstop); a present-but-lagging unit still holds the barrier and is not failed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…anding it If the agent dies mid-swap and Juju redelivers the hook, the (RESTORE, NOT_STARTED) arm would re-probe is_primary() against the now-stopped valkey, raise, and tear down without rolling back -- leaving the primary down with no dump.rdb. Instead, detect the interrupted primary from on-disk state (a pre-restore copy present while valkey is not running -- the only signal that survives a mid-hook crash, since Juju never committed the databag), roll back to the original data, and fail so the operator re-runs from a known-good baseline. _ensure_stopped/roll_back tolerate an already-stopped valkey (fixing a latent K8s stop-on-stopped error), gate on the specific service rather than alive(), drop a partial .part download on rollback, and drop a stale pre-restore copy before capturing this restore's rollback. restore_on_primary only ever runs for a fresh swap now, so its redelivery-preservation guard is gone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

If leadership drifts mid-restore to a unit that joined after initiation (a non-participant) without any original participant departing, that leader would otherwise return at the non-participant guard and never advance the shared instruction, wedging the restore. Let a non-participant leader still run _advance_if_leader so the barrier progresses. This covers an astronomically narrow corner (lease-drift to a late-joiner with no departure) that has no other backstop; it is kept off s3-restore as an optional hardening. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

reneradoi

I've found a couple of things that we need to fix/clarify. In addition: The LDAP feature has been merged, please also consider LDAP event handlers for restore safeguards.

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

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


Let's please add a short comment that single-unit deployments are triggered through the peer-relation-changed on app data.

Addresses PR #79 review feedback: - Use cluster_manager.save_dataset_before_shutdown() in the primary restore path. The public save_database_blocking() was made private on 9/edge and replaced by save_dataset_before_shutdown() (which also disables save-on-shutdown so Pebble can't kill the pre-stop save); the old call raised AttributeError after the merge. - Narrow the _clear_failed_restore resume-failover catch to ValkeyWorkloadCommandError, the only exception resume_failover raises. - Gate BACKUP_S3_PARAMETERS_MISSING on unit_server.is_started so a relation present from deploy time (e.g. Terraform) without applied credentials does not surface the status before startup. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Address PR #79 review: instead of separate stop_service/start_service/ service_running methods, give start/stop/alive an optional `service` parameter. With `service` set they act on that one service; without it they act on all (the existing behaviour). Removes the three extra abstractmethods and their per-substrate implementations. - alive(service): single-service check reuses the same code path as the all-services check, so the restore stop-gate uses alive(valkey_service) rather than a dedicated service_running (the intent is unchanged: gate on the specific service, not the all-services alive()). - start(service) / stop(service): single-service start skips the health wait; single-service stop verifies that one service went down. Behaviour-preserving; restore call sites updated accordingly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Addresses PR #79 review feedback: - Use cluster_manager.save_dataset_before_shutdown() in the primary restore path. The public save_database_blocking() was made private on 9/edge and replaced by save_dataset_before_shutdown() (which also disables save-on-shutdown so Pebble can't kill the pre-stop save); the old call raised AttributeError after the merge. - Narrow the _clear_failed_restore resume-failover catch to ValkeyWorkloadCommandError, the only exception resume_failover raises. - Gate BACKUP_S3_PARAMETERS_MISSING on unit_server.is_started so a relation present from deploy time (e.g. Terraform) without applied credentials does not surface the status before startup. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

The LDAP feature landed after the restore workflow, so its handlers had no restore safeguard (unlike base_events / external_clients / tls). During a restore the primary is stopped and restarted with failover suppressed; reconfiguring LDAP (CONFIG SET / ACL LOAD on the primary) mid-restart would collide with the RDB swap. - _on_peer_relation_changed: return during a restore. The restore workflow drives peer relation-changed itself, and completion re-fires it, so ACLs reconcile from current state then. - Externally-triggered handlers (ldap_ready, ldap_unavailable, config_changed, secret_changed, ldap_ca_available, ldap_ca_removed): defer, since they do not re-fire once the restore clears — deferring keeps the change instead of dropping it. - sync-ldap-users action: fail with a clear message. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Address PR #79 review: instead of separate stop_service/start_service/ service_running methods, give start/stop/alive an optional `service` parameter. With `service` set they act on that one service; without it they act on all (the existing behaviour). Removes the three extra abstractmethods and their per-substrate implementations. - alive(service): single-service check reuses the same code path as the all-services check, so the restore stop-gate uses alive(valkey_service) rather than a dedicated service_running (the intent is unchanged: gate on the specific service, not the all-services alive()). - start(service) / stop(service): single-service start skips the health wait; single-service stop verifies that one service went down. Behaviour-preserving; restore call sites updated accordingly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Follow the project convention of not wrapping status add/delete in error handling (PR #79 review). The un-wedge comes from clearing restore state before the status write, not from swallowing a status-write error — so a failing add now surfaces as a hook error and self-heals on retry instead of being silently swallowed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

reneradoi

Some final comments from my side for the sake of maintainability, other than that I'm good. Testing was fine, also a few error cases (invalid backup file, pebble services stopped during restore, ...) where the restore was stopped and the database recovered.

…eck_alive Replace the implicit "a service arg implies the liveness check" coupling with an explicit check_alive parameter (start defaults True, stop False), applied across WorkloadBase and both workloads. The restore path opts in per need: stop(check_alive=True) to confirm the primary is down before the RDB swap, and start(check_alive=False) where wait_until_loaded gates readiness separately. Addresses PR #79 review (reneradoi). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

_on_restore_workflow drove the state machine to a fixed point in one hook via a while loop. Remove it: the workflow now advances one step per hook and relies on Juju re-delivering peer relation_changed for each app-databag write -- including to the leader for its own writes to the peer app databag (the documented self-delivery guarantee) -- so single- and multi-unit restores both cascade, with update_status as a backstop. Unit tests drive the cascade via a helper instead of asserting one-hook completion. Addresses PR #79 review (reneradoi). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

jubilant's UnitStatus exposes `leader`, not `is_leader`; _leader_unit_name raised AttributeError under jubilant 1.8.0, failing the disaster-recovery test on its final read-back. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

The corrupt-restore test proved "failover suppression was resumed" by killing the primary and expecting a real failover. On K8s that races Pebble's auto-restart of valkey-server -- if the process returns within the 30 s down-after window Sentinel never promotes, so the test failed even though suppression was correctly reset. Assert down-after-milliseconds == the normal value on every sentinel instead: deterministic, and exactly the invariant the suppress/resume pair maintains. A leak would leave it at the suppressed value. 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

reneradoi reviewed Jul 7, 2026

View reviewed changes

Mehdi-Bendriss reviewed Jul 8, 2026

View reviewed changes

reneradoi changed the title ~~feat(restore): S3 backup restoration (restore-backup action)~~ [DPE-10650] S3 backup restoration (restore-backup action) Jul 13, 2026

delgod and others added 13 commits July 19, 2026 21:02

Merge branch '9/edge' into s3-restore

16d98c1

reneradoi reviewed Jul 21, 2026

View reviewed changes

delgod and others added 3 commits July 21, 2026 16:36

delgod force-pushed the s3-restore branch from 3e7c1ee to 9f1c325 Compare July 21, 2026 14:36

reneradoi reviewed Jul 22, 2026

View reviewed changes

Comment thread src/workload_k8s.py Outdated

Comment thread src/workload_k8s.py Outdated

Comment thread src/events/backup.py Outdated

delgod and others added 6 commits July 22, 2026 18:06

add spread configuration for integration tests

47988d3

add comment (to trigger CI after GH outtage)

dd85791

	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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Mehdi-Bendriss 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