[DPE-10650] S3 restore followup by delgod · Pull Request #96 · canonical/valkey-operator

delgod · 2026-08-18T16:22:03Z

Follow-up to #79 (S3 restore) addressing the last review round (skourta, 2026-08-17).
Only the four commits on top of #79 are new; base becomes 9/edge once #79 lands.

fix(restore): self-heal a sentinel left failover-suppressed — resume_failover is
best-effort on failure paths and Sentinel persists SENTINEL SET to its own conf, so a
sentinel unreachable at that moment came back with the 24 h down-after and no automatic
failover until the next config re-render. Each unit now re-checks its own sentinel on
update-status (outside a restore) and resets it.
refactor(restore): log the restore trail — restore.* log lines for every action
rejection, step, barrier advance, completion, download, rollback and failure
(juju debug-log | grep restore. shows the whole run); the failure helpers are renamed
to say what they do (_fail_restore / _finish_failed_restore); comment/nit cleanups.
refactor(cluster): fuse the bounded waits — _wait_until(condition, timeout, failure)
replaces the duplicated tenacity loops in wait_until_loaded / wait_until_resynced.
test(restore): single-unit happy path — test_restore_single_unit (1-unit app:
leader == primary, no resync, cascade driven only by the leader's own app-databag
relation-changed); also asserts the unit is writable after the restart.

No databag/schema changes; behaviour change is limited to the sentinel self-heal.

…helper wait_until_loaded and wait_until_resynced carried the same tenacity loop; _wait_until(condition, timeout_s, failure) owns it now and each wait passes its predicate (is_loaded / is_replica_synced). Behaviour unchanged. PR #79 review (skourta r3794505615). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

resume_failover is best-effort on every restore-failure path and Sentinel persists SENTINEL SET to its own conf, so a sentinel that was unreachable at that moment came back at the suppressed 24h down-after and stayed there -- no automatic failover from it -- until the next sentinel.conf re-render. Each unit now re-checks its own sentinel on update-status when no restore is in progress (SentinelManager.is_failover_suppressed / resume_local_failover, BackupEvents._reconcile_failover_suppression) and resets it. The TLS expiry test's exec mock now returns the (stdout, stderr) tuple the workload contract promises. PR #79 review (skourta r3621161292). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…ainly Manual testing found the restore nearly silent in juju debug-log. Every rejected restore action now logs restore.rejected with the reason; the workflow logs the unit's role, each step start / restore.step_done, the leader's restore.advance A -> B and restore.completed; the manager logs verify, download start (bytes) / finish, stop, keep-copy, start, rollback and cleanup; a failure logs restore.step_failed (traceback), restore.failed unit/kind and the leader's restore.ended status. 'Teardown' was confusing (nothing is torn down): _restore_teardown -> _fail_restore (the failing unit records its marker), _clear_failed_restore -> _finish_failed_restore (leader ends the restore, un-wedges, sets the terminal status), _leader_restore_teardown_needed -> _leader_must_fail_restore. failed_restore_kind filters the empty kinds inside the comprehension instead of a trailing discard. PR #79 review (skourta r3794362707, r3794484866, r3794249766, review body). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

A one-unit app takes a different path from the 3-unit scenarios: the only participant is both juju leader and valkey primary, there is no replica RESYNC, the barrier has one member, and the RESTORE -> RESYNC -> COMPLETED cascade rides purely on the leader's own app-databag relation-changed self-delivery. test_restore_single_unit redeploys 1 unit, does write -> backup -> mutate -> restore -> original back, then writes again to prove the lone primary is not write-frozen after the restart (rendered min-replicas-to-write 1 relaxed by the post-restore reconcile). deploy_and_relate_s3 grows a num_units parameter for it. Passed locally on lxd/VM (7m25s incl. the redeploy). PR #79 review (skourta r3794577942). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

skourta

Thank you @delgod only two comments so far.
The PR is still marked as draft so when it is ready for review I will recheck it.

skourta · 2026-08-19T12:47:23Z

-        if not (backup_id := event.params.get("backup-id", "")):
-            event.set_results({"error": "Must provide backup-id to restore."})
-            event.fail("Must provide backup-id to restore.")
+        if not backup_id:


Consider adding this in the _restore_blocking_reason. This and the other checks

Done. _restore_blocking_reason now takes backup_id and owns all the gates. 00e0b3d.

skourta · 2026-08-20T06:43:14Z

        if self.charm.unit.is_leader():
            self._advance_if_leader()

+    def _reconcile_failover_suppression(self, _: ops.UpdateStatusEvent) -> None:


This can be moved to the sentinel manager.

Moved: SentinelManager.reconcile_failover_suppression() now does the check 62676ab.

…manager The check-and-reset of a sentinel left at the restore-suppressed down-after is sentinel business logic, so it belongs in SentinelManager, not in the backup event layer: reconcile_failover_suppression() reads is_failover_suppressed, logs restore.suppression_leak and calls resume_local_failover, tolerating an unreachable sentinel the same best-effort way resume_failover already does. The update-status handler keeps only the "when" -- unit_server.is_active and not cluster.is_restore_in_progress are restore-domain state -- so it is now a guard plus one manager call. PR #96 review (skourta r3819217492). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

_restore_blocking_reason now takes the backup-id and owns every gate, so _on_restore_action is a single `if reason := self._restore_blocking_reason( backup_id)` plus the audit trail. The two backup-id gates live in a sibling _unusable_backup_reason helper: fusing them inline put the guard at mccabe 12 and ruff C901 rejects anything over 10, so it is split the way _unstable_primary_reason already is. Gate order is unchanged -- the S3 round-trip stays last, after the cheap state checks. A listing failure now fails the action with "Could not list backups: S3 request failed: <Code>" instead of splitting the safe error between results.error and a generic fail message; still the error code only, the detail stays in the unit log. PR #96 review (skourta r3813059183). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

reneradoi

No concerns in general, I only have a design remark for the added logging: All the log messages are bloating the event flow, in some cases we even add private methods only for avoiding duplicating log messages.

Example: BackupEvents._record_restore_step() consists of only two lines: the log line and a call to the manager function. It makes to code much harder to read and I would prefer if we could be more concise. Could you please try to add the log output to the manager methods where possible, and leave them out of the event handler? This can be supported by not logging the unit name as part of the log message, which is also duplicate information (already logged by Juju for each line).

If this could be applied all over the events, it would increase readability a lot.

delgod force-pushed the s3-restore-followup branch 2 times, most recently from aeb4969 to 8ce1fc1 Compare August 19, 2026 06:02

delgod changed the title ~~S3 restore followup~~ [DPE-10650] S3 restore followup Aug 19, 2026

delgod and others added 4 commits August 19, 2026 17:27

delgod force-pushed the s3-restore-followup branch from 8ce1fc1 to 83fa75b Compare August 19, 2026 15:27

skourta reviewed Aug 20, 2026

View reviewed changes

delgod and others added 2 commits August 20, 2026 09:11

delgod marked this pull request as ready for review August 20, 2026 09:41

reneradoi reviewed Aug 20, 2026

View reviewed changes

Navigation Menu

Uh oh!

[DPE-10650] S3 restore followup - #96

[DPE-10650] S3 restore followup#96
delgod wants to merge 6 commits into
9/edgefrom
s3-restore-followup

delgod commented Aug 18, 2026 •

edited

Loading

Uh oh!

skourta left a comment

Uh oh!

skourta Aug 19, 2026

Uh oh!

delgod Aug 20, 2026 •

edited

Loading

Uh oh!

skourta Aug 20, 2026

Uh oh!

delgod Aug 20, 2026 •

edited

Loading

Uh oh!

reneradoi left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

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

Uh oh!

Uh oh!

skourta left a comment

Choose a reason for hiding this comment

Uh oh!

skourta Aug 19, 2026

Choose a reason for hiding this comment

Uh oh!

delgod Aug 20, 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!

skourta Aug 20, 2026

Choose a reason for hiding this comment

Uh oh!

delgod Aug 20, 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!

reneradoi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

delgod commented Aug 18, 2026 •

edited

Loading

delgod Aug 20, 2026 •

edited

Loading

delgod Aug 20, 2026 •

edited

Loading