feat: manage backup retention in Klio instead of Kopia - #238
Open
gabriele-wolfox wants to merge 36 commits into
Open
gabriele-wolfox wants to merge 36 commits into
gabriele-wolfox wants to merge 36 commits into
Conversation
gabriele-wolfox
force-pushed
the
dev/168
branch
4 times, most recently
from
September 8, 2026 07:30
5972f01 to
c1def0c
Compare
gabriele-wolfox
force-pushed
the
dev/168
branch
from
September 8, 2026 14:25
c1def0c to
e8ed782
Compare
armru
force-pushed
the
dev/168
branch
3 times, most recently
from
September 14, 2026 10:41
7d6e631 to
0bab821
Compare
gabriele-wolfox
force-pushed
the
dev/168
branch
2 times, most recently
from
September 14, 2026 12:13
423e596 to
86d9531
Compare
Replace Kopia's snapshot-expire retention with a Klio-managed policy manager that evaluates the backup catalog and deletes the expired backups itself. The only criterion for now is latest, which keeps the N most recent backups; it is configured per tier in the PluginConfiguration and must be at least 1. Because retention no longer runs through Kopia, the tier2 snapshot pins are gone. The invariant that a tier1 backup is never deleted before it reaches tier2 now lives in the retention selection, read from our own catalog, which keeps snapshot manifest IDs stable. The tier1 and tier2 policies travel to the server on the CloseBackup request, and the consumer applies them per tier after every backup. The klio retention CLI and the Kopia policy plumbing are removed. Assisted-by: Claude Signed-off-by: Gabriele Quaresima <gabriele.quaresima@enterprisedb.com>
Add a klio retention apply command and an ApplyRetention gRPC call that apply the configured retention immediately, without waiting for the next backup. This frees space on demand, for example after shrinking the policy on a nearly-full disk. The command reads the client configuration and sends the tier1 and tier2 policies to the server, which enqueues a maintenance-only task. The backup consumer then runs the same per-tier retention path used after a backup, so there is a single retention implementation. Assisted-by: Claude Signed-off-by: Gabriele Quaresima <gabriele.quaresima@enterprisedb.com>
The operator writes the client config with the `tier1_retention` key while the mapstructure tag said `retention`, so viper left the field nil and the tier1 retention policy never reached the server: tier1 retention was a no-op. Both synced copies of the struct are fixed and a decode test covers the retention blocks. Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
The `kopia repository create` command stores Kopia's default retention counters in the global policy and every `kopia snapshot create` applies the effective policy, so with the pins gone Kopia kept expiring tier1 snapshots behind Klio's back, per source, capped by its own buckets. The server now disables Kopia retention at start, before the Kopia servers come up: the global policy of each tier is set to keep everything, and the per-source policies left by earlier Klio versions are reset to inherit it. Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
The deletion glue (catalog listing, policy evaluation, keep guard, error joining) had no unit test. Narrow the client parameter to the two methods it uses so a fake can drive it. Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
…tion An empty cluster name is a wildcard for the backup listing, so a maintenance task carrying one evaluated the policy over every cluster's catalog and then failed on each delete, blocking the single-ack consumer through its retry budget. The RPC now rejects an empty or invalid name and the consumer refuses to apply retention without one. Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
…fails Best-effort tier1 maintenance is right after a backup, which must not fail on it. A maintenance-only task has no backup to protect: the retention run is the whole task, so a tier1 failure was logged once and acked as done while the client had been told it was scheduled. It now fails the task so the queue retries it. Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
…tasks The purge after a successful task exists because a completed backup makes the earlier failures moot. A maintenance-only task reuses the same handler, so an on-demand retention run erased dead-lettered backup tasks without any backup having succeeded. Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
The snapshots of a backup were deleted in listing order. A deletion interrupted after the metadata snapshot left the data snapshots orphaned and invisible to the catalog, so retention could never finish the job. Deleting the metadata last keeps a partially deleted backup listed until the next run completes it. Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
A PluginConfiguration stored before the `latest` field existed reads back with `latest: 0` once the old keys are pruned. The operator emitted a retention block for it, the client rejected the config and every backup of the cluster failed until the resource was edited. Such a block now converts to "no policy", keeping every backup; the CRD validation still rejects `latest` below 1 on write. Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
…ture The helper read every backup known to the Klio server, so the exact-count assertions would fail as soon as another cluster shared the server. Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
Remove the consumer options only read by the deleted tier1 server refresh, fix a comment naming a method that no longer exists, correct the upgrade note (only `klio retention get` and `set` went away, `apply` is new) and update AGENTS.md, which still described the pins and the tier1 unpin. Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
…backup The tier1 retention guard considered a backup safe to delete as soon as its metadata snapshot appeared on tier2. The relay is a single snapshot migration with no ordering between a backup's parts, so a relay that dies midway can leave the metadata on tier2 without the data, and the next maintenance run would then delete the only complete copy on tier1. The guard now lists the snapshots of both tiers and keeps a backup until every tier1 snapshot of it (pgdata, metadata, control data, tablespaces) has a counterpart on tier2. Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
…nreadable ListBackups skipped a backup whose metadata could not be read and returned the rest as if the catalog were complete. The catalog now drives base backup retention and WAL cleanup, so a transient read error made them delete one backup, or the WALs of one backup, too many. A metadata read error now fails the listing, and the callers fail closed. Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
The WAL server requires a client certificate but never bound its Common Name to the cluster named in the request, so any certificate signed by the CA could apply a retention policy to another cluster's backups. The host part of the userName@hostName Common Name must now match the cluster in the ApplyRetention request; the other calls are unchanged. Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
With tier2 enabled, the tier1 retention guard protected every backup that had not reached tier2, including backups taken with tier2 backup disabled, which never will. Those backups were never deleted from tier1. The client now records in the backup metadata whether it asked for the tier2 relay, and the guard lets tier1 retention delete a backup that was never meant to reach tier2. Backups taken by older clients carry no annotation and keep the protective behaviour. Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
Retention orders backups by the start time taken from the instance clock, so a clock skew after a switchover can misorder them. Spell that out, and describe when tier 1 retention waits for tier 2. Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
Add a tier1-only e2e that mirrors the tier2 retention test: configure a tier1 retention policy of latest:2, take three backups, and assert the retention manager keeps exactly two and deletes the oldest, then tighten the policy to one and verify `klio retention apply` leaves only the newest. Both retention features run the same tier-parameterized flow (verifyRetentionAndOnDemandApply): the tier1 and tier2 Run methods differ only in the tier annotation, the policy the on-demand step edits, and the tier2-only WAL and recovery-gate checks layered on top. Generalize the tier2 retention helpers accordingly, add a Tier1RetentionPolicy option to the Klio plugin-configuration template, register the new Tier1Retention feature, and update the e2e test-structure docs. Assisted-by: Claude Signed-off-by: Gabriele Quaresima <gabriele.quaresima@enterprisedb.com>
The count check already decoded the cluster name of each backup but counted every cluster's backups, while the name listing filtered by cluster. The exact-count assertions of the shared retention flow now look at the cluster under test only. Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
The tier1 retention feature checked that base backups are deleted but not that the WAL horizon moved with them, which is the part of the WAL cleanup this change touches: the first required WAL is now recomputed from Klio's catalog after retention. After the on-demand apply leaves the newest backup, the test waits until no tier1 WAL older than that backup's begin WAL survives. The tier1 WAL listing helpers move from the e2e package to the Klio features package so both tests can use them. Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
… removed The tier1 guard kept every backup whose snapshots were missing on tier2, which conflates "not relayed yet" with "relayed and deleted since". With a tier2 policy tighter than tier1's, tier2 retention runs first and removes the older backups; tier1 then kept those same backups, and their WALs, forever, with only an info log that reads as transient. The relay migrates every tier1 snapshot of the cluster at once, so a newer backup complete on tier2 proves the relay ran after the older one existed. A backup absent from tier2 is now deletable when such a newer backup exists; one that is only partially on tier2 is still kept, since a relay may be in flight or have failed midway. Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
A required `latest` makes PluginConfigurations stored by earlier versions, which carry the pruned Kopia-style keys and no `latest`, fail schema validation on their next write, including the controller's own status update, unless the cluster ratchets CRD validation. The field is optional with a plain minimum of 1; an empty block converts to "no policy" as before. Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
… retry In mergePolicyField a pointer field was required to detect "unset" via nil, but BackupTask.Tier2RetentionPolicy is now a value-typed retention.Policy whose own zero value means "no policy" (see core/pkg/retention/retention.go). Generalize the constraint from any to comparable and use each type's zero value as the sentinel instead of nil, so the same helper folds both the pointer-typed Tier2CompressionPolicy and the value-typed Tier2RetentionPolicy. mergeBackupTasks also dropped Tier1RetentionPolicy and MaintenanceOnly when folding failed backup tasks for the same cluster into one retry: Tier1RetentionPolicy was never copied at all, and MaintenanceOnly always reset to false. A retried maintenance-only task would silently run as a full backup task, and a retried backup would lose its tier1 retention policy for that cycle. Fold Tier1RetentionPolicy through the same agree-or-blank mergePolicyField used for Tier2RetentionPolicy, and AND MaintenanceOnly across entries: any genuine failed backup in the batch forces the merged task through full processing, since that already applies retention too. Also update retry_test.go, still written against the deleted kopia.RetentionPolicy pointer type removed together with Kopia's own retention policies; it no longer compiled. golangci-lint's ireturn flagged mergePolicyField returning T directly, introduced when its constraint changed from *T to comparable. T is a concrete type at every call site (retention.Policy, *kopia.CompressionPolicy), not a real interface return; ireturn can't tell the two apart for a generic type parameter. Assisted-by: Claude Signed-off-by: Francesco Canovai <francesco.canovai@enterprisedb.com>
CloseBackup drives tier1/tier2 retention deletion (via the retention policies on the request) and reads the cluster's WAL directory, keyed by the client-supplied cluster name, but unlike ApplyRetention never checked that the caller's certificate was issued for that cluster. A client authenticated for one cluster could request CloseBackup for another cluster's name and trigger retention deletes against it. Add the same checkPeerCluster check ApplyRetention already uses, at the top of CloseBackup before any of its cluster-scoped work runs. Assisted-by: Claude Signed-off-by: Francesco Canovai <francesco.canovai@enterprisedb.com>
RetentionPolicy.Validate returned an inline errors.New(...) instead of a package-level error variable. Assisted-by: Claude Signed-off-by: Francesco Canovai <francesco.canovai@enterprisedb.com>
The runTier1Retention had tier1RetentionGuard and applyRetention each call ListBackups on tier1 independently. Between the two calls, a concurrent change (relay, delete) could make them see different snapshots of the same cluster: the guard's "keep" decisions would then be evaluated against a catalog from a different point in time, the same race class fixed for tier1-vs-tier2 in aa7f5879. List tier1 backups once in runTier1Retention and pass that single listing into both the guard and applyRetention. Applied the same change to maintainTier2, whose applyRetention call listed tier2 backups internally; that listing now happens in the caller too, ahead of the call. Assisted-by: Claude Signed-off-by: Francesco Canovai <francesco.canovai@enterprisedb.com>
The policyTarget duplicated SourceInfo (same fields, same JSON tags, and a String() producing the same output) in the same package, plus an isGlobal() that reimplemented SourceInfo.String()'s own emptiness check. Removed policyTarget and its methods; listPolicyTargets and parsePolicyTargets now return []SourceInfo. Added SourceInfo.IsGlobal() (also used by its own String()) as the one place that check lives. Assisted-by: Claude Signed-off-by: Francesco Canovai <francesco.canovai@enterprisedb.com>
A backup that dies between uploading its parts and closing (Close uploads the metadata snapshot last) leaves snapshot parts with no metadata snapshot behind. ListBackups only sees metadata-tagged snapshots, so these orphans were invisible to the catalog and never retention-managed: they stayed in the repository forever, and this was also the failure mode of a since-reverted fix that let a failed delete strip a backup's metadata while leaving another part behind. Klio processes one backup at a time per cluster, so a newer backup reaching a real, catalogued completion proves an older metadata-less one is not still running: it is a permanently abandoned attempt, the same newer-backup-proves-it trick keepUntilOnTier2 already uses for tier2 relay. deleteAbandonedOrphans applies that rule during tier1 maintenance, judging each orphan snapshot part independently on its own StartTime against the newest catalogued backup's StartedAt: a part that isn't old enough yet simply stays an orphan candidate and gets swept on a later cycle, so there is no need to decide a backup's parts as a group. When no catalogued backup exists yet nothing is deletable, since the orphan could still be the cluster's first backup, genuinely in progress. The same blind spot exists on tier2 for a different reason: relayTier2's MigrateSnapshots call can migrate some of a backup's parts and not others before failing, and if that task's retries are exhausted, those parts never get their tier2 metadata snapshot either; deleteAbandonedOrphans runs during tier2 maintenance too, against tier2's own client, snapshot listing and backup catalog. Orphans are deleted directly by manifest ID via a new Connection.DeleteSnapshot: they have no metadata, so calling them a "backup" and deleting them through the name-based DeleteBackup, which re-lists from scratch to rediscover manifests already in hand, was the wrong fit. Reworked runTier1Retention to list tier1 snapshots once and share them with tier1RetentionGuard, alongside the existing shared tier1Backups listing, rather than adding a third redundant listing. Manifest.StartTime changes from a raw JSON string to UTCTimestamp, matching EndTime's existing, already-tested parsing on the same struct; it was otherwise unused. Root cause found live on a cluster showing a real orphan: Kopia's CLI stores every user-defined snapshot tag key with a literal "tag:" prefix (see the vendored cli/command_snapshot_create.go's getTags), but ListSnapshots never stripped it before returning manifests. Every direct Tags[klioclient.SomeConstant] lookup was a silent no-op, since the actual key was "tag:klio.io/tag" and so on. Filter-based --tags usage was unaffected because Kopia applies the same prefix when matching, which is why this went unnoticed. This broke isMetadataSnapshot (the delete-ordering safety net below), deleteAbandonedOrphans's own orphan detection (always saw zero candidates), and keepUntilOnTier2 (the tier1-must-wait-for-tier2 guard, unconditionally returning "deletable" for every backup). Confirmed against real "snapshot list --json" output pulled from the live cluster, which is why the fix is verified with a JSON fixture test rather than only synthetic Manifest structs. newRelayState (the tier2 relay guard) and the orphan detection above each rolled their own loop over Tags[BackupNameTagName] to partition raw snapshot manifests by backup name; extracted into one shared groupSnapshotsByBackup helper, so this grouping, and any future tag-related fix to it, has one call site instead of two. deleteSnapshots (backup deletion) used to keep a backup's metadata snapshot for last, so a partial delete failure left the backup listed for the next retention run to finish. But the loop never stops on a failure, so an isolated failure on one part still let metadata delete cleanly afterward, leaving the backup listed as valid while some of its data was already gone. Delete metadata first instead: a partially deleted backup is never listed, and its leftover parts become orphans for the sweep above to catch, once a newer backup proves them abandoned. On the retention path this is immediate, since a backup is only ever deleted for falling outside the "keep the newest N" policy, which means a newer, known-good backup already exists by construction. Assisted-by: Claude Signed-off-by: Francesco Canovai <francesco.canovai@enterprisedb.com>
Evaluate sorted by StartedAt to pick the N most recent backups for the "latest" policy. StartedAt only orders closed, catalogued backups here (no in-progress ones are ever in this catalog), so the false-positive risk StartedAt exists to avoid elsewhere (see deleteAbandonedOrphans) doesn't apply; StoppedAt is the more meaningful "how recent is this backup" signal, since it reflects when the backup actually finished and its data became a real, complete restore point, rather than merely when the attempt began. Assisted-by: Claude Signed-off-by: Francesco Canovai <francesco.canovai@enterprisedb.com>
Assisted-by: Claude Signed-off-by: Francesco Canovai <francesco.canovai@enterprisedb.com>
The only return path ever producing a result set scheduled: true; every failure path returns an error instead, never reaching it. The field was therefore always true whenever a result came back at all, making it dead API surface: the client's own scheduled check could never be false. Assisted-by: Claude Signed-off-by: Francesco Canovai <francesco.canovai@enterprisedb.com>
Added info-level logging across the retention lifecycle, to make the behavior traceable end to end without a debugger. Assisted-by: Claude Signed-off-by: Francesco Canovai <francesco.canovai@enterprisedb.com>
Sample PluginConfiguration manifests still used the old Kopia-style keepLatest/keepHourly/keepDaily/keepWeekly/keepMonthly/keepAnnual retention keys, which the CRD no longer has (RetentionPolicy is now a single latest field). The API server prunes unknown fields, so these samples silently configured no retention at all. Assisted-by: Claude Signed-off-by: Francesco Canovai <francesco.canovai@enterprisedb.com>
A feature is a standard way a test is set up and executed (Name/Setup/Run/Teardown). This PR had accumulated code in operator/test/klio/features/ that wasn't one. No functional change throughout: same types, same helpers, same behavior, just relocated and reordered. Assisted-by: Claude Signed-off-by: Francesco Canovai <francesco.canovai@enterprisedb.com>
Tier1 retention runs as a direct filesystem write, bypassing the persistent tier1 kopia server. Without a refresh, that server could keep listing an already-deleted backup for up to its staleness window, observed live as backups piling up past the configured latest:N policy before a delayed batch catch-up. Add refreshTier1KopiaServer, mirroring the existing refreshTier2KopiaServer, called right after tier1 retention succeeds. Collapse the per-tier server certificate fingerprint fields into one shared ServerCertificateFingerprint, since tier1 and tier2 share the same TLS certificate. Update the AGENTS.md note on direct-write refreshes: retention deletes don't corrupt the consumer's own decisions (those always read its own direct ListBackups), but server-routed callers (get-metadata, verify, restore, list/delete) can observe stale post-delete state without the refresh, which is the real justification for keeping it. Assisted-by: Claude Signed-off-by: Francesco Canovai <francesco.canovai@enterprisedb.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replace Kopia's snapshot-expire retention with a retention policy manager that Klio owns: Klio evaluates its own backup catalog and deletes the expired backups, rather than delegating to
kopia snapshot expire.Highlights
retentionPolicyin thePluginConfigurationwith a singlelatestcriterion (keep the N most recent backups), configurable per tier and validated to be at least 1. Omitting it keeps every backup.CloseBackuprequest and is applied per tier by the backup consumer after every backup.klio retention applycommand (andApplyRetentiongRPC call) to apply the configured retention on demand (eg. without waiting for the next backup, to reclaim space quickly after tightening the policy).klio retention set/getCLI and the Kopia retention-policy plumbing are removed.Testing
Evaluateselection and the consumer'sapplyRetention(against a fake client).klio retention applyleaves only the newest.klio retention apply) and adds tier2-only WAL retention and recovery-gate checks.Closes #168.