diff --git a/AGENTS.md b/AGENTS.md index 8132485b..3e2ef1d3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -204,8 +204,8 @@ confirm after that warning. canonical case: `kopia snapshot pin` rewrites the snapshot manifest to a *new* ID and deletes the old one, so without a refresh the server keeps serving the now-deleted ID for a backup that still exists, and a later client - `klio backup delete` deletes the wrong ID — leaving the real backup (and its - WALs) pinned forever. + `klio backup delete` asks Kopia to delete an ID that no longer matches + anything: the command fails and the real backup (and its WALs) stay pinned. - A direct write that only **deletes** snapshots (the tier1/tier2 retention apply) does **not** need a refresh: it removes IDs the server may still list, but it never rewrites a live backup's ID, and WAL retention is recomputed from @@ -218,6 +218,34 @@ Do not introduce direct-write paths anywhere else. If, after warning the user, a new direct write is genuinely unavoidable, it must be paired with a server refresh of the affected tier. +### Snapshot identity: manifest ID vs root object ID + +A snapshot's **manifest ID is not a stable identity**. `kopia snapshot pin` +(the tier1 unpin above) rewrites a snapshot's manifest under a new ID and +deletes the old one, so any code that lists snapshots and then acts on them a +moment later can be holding an ID that no longer exists. Pick the identity by +what the operation does: + +- **Reads that must survive a concurrent rewrite** use the root object ID + (`Manifest.RootEntry.ObjID`), which the rewrite leaves untouched. Backup + verification (`core/internal/client/klioclient/kopia/verify.go`) does this, + passing every root to Kopia as `--file-id` regardless of whether it is a + directory root (pgdata, metadata) or a file root (the control data file, + snapshotted on its own). +- **Deletions must NOT use the root object ID.** Unchanged content dedupes to + the same root across backups (two backups of an idle tablespace share one), and + `kopia snapshot delete` removes *every* snapshot matching the ID it is given, + so deleting one backup by root ID can take another backup's snapshot with it. + Delete by manifest ID, and on failure re-list and retry so a concurrent + rewrite is picked up (`DeleteBackup` in the same package). +- **The tier1 unpin is a write, not a read, and knowingly accepts the same + collision as delete.** The consumer's `getPinnedSnapshots`/`maintainTier2` + (`core/internal/consumer/backup.go`) also targets the root object ID, so a + root shared with another backup gets unpinned too. This is tolerated only + because the step is best-effort and the affected snapshot would be unpinned + anyway on the next tier2 migration — it is not a safe pattern to copy for + anything that isn't equally tolerant of that collision. + ### Dagger caching issues When running e2e tests, Dagger may cache Helm repo indexes. If a new version of diff --git a/core/internal/client/klioclient/kopia/delete.go b/core/internal/client/klioclient/kopia/delete.go index c1c47092..775f3bbf 100644 --- a/core/internal/client/klioclient/kopia/delete.go +++ b/core/internal/client/klioclient/kopia/delete.go @@ -23,46 +23,96 @@ import ( "context" "errors" "fmt" + "slices" + "time" "github.com/cloudnative-pg/machinery/pkg/log" + "k8s.io/apimachinery/pkg/util/wait" "github.com/cloudnative-pg/klio/core/internal/client/klioclient" + "github.com/cloudnative-pg/klio/core/internal/kopia" ) // ErrBackupNotFound is returned when attempting to delete a backup that does not exist. var ErrBackupNotFound = errors.New("backup not found") +// deleteBackupBackoff defines the backoff between failed attempts to delete snapshots of a backup. +// +//nolint:gochecknoglobals +var deleteBackupBackoff = wait.Backoff{ + Duration: time.Second, + Factor: 2, + Cap: 2 * time.Second, + Steps: 3, +} + +// snapshotStore is the subset of the Kopia client that DeleteBackup needs. +type snapshotStore interface { + ListSnapshots(ctx context.Context, tags map[string]string, logFn kopia.LogFunc) ([]kopia.Manifest, error) + DeleteSnapshot(ctx context.Context, id string) error +} + // DeleteBackup removes all snapshots associated with the backup with the provided name. func (s *Connection) DeleteBackup(ctx context.Context, hostname string, name string) error { + return deleteBackupSnapshots(ctx, s.kopia, hostname, name) +} + +// deleteBackupSnapshots removes every snapshot of a backup on the given host. +func deleteBackupSnapshots(ctx context.Context, store snapshotStore, hostname, name string) error { contextLogger := log.FromContext(ctx) - // List all snapshots for this backup (all content types) - entries, err := s.kopia.ListSnapshots(ctx, map[string]string{ - klioclient.BackupNameTagName: name, - }, contextLogger.Debug) - if err != nil { - return fmt.Errorf("while listing snapshots: %w", err) - } + // Concurrent operations on the same snapshots (e.g. post-backup + // maintenance) can invalidate the IDs resolved below before they are + // used, so this race condition is tolerated with retries. + retryErr := wait.ExponentialBackoffWithContext(ctx, deleteBackupBackoff, + func(ctx context.Context) (bool, error) { + entries, err := store.ListSnapshots(ctx, map[string]string{ + klioclient.BackupNameTagName: name, + }, contextLogger.Debug) + if err != nil { + return false, fmt.Errorf("while listing snapshots: %w", err) + } - var deleted int - for _, entry := range entries { - if entry.Source.Host == hostname { - contextLogger.Info("DeleteBackup: deleting snapshot", "snapshotID", entry.ID) - if deleteErr := s.kopia.DeleteSnapshot(ctx, entry.ID); deleteErr != nil { - err = errors.Join(err, deleteErr) - } else { - deleted++ + entries = slices.DeleteFunc(entries, func(e kopia.Manifest) bool { + return e.Source.Host != hostname + }) + + if len(entries) == 0 { + return false, fmt.Errorf("%w: %s", ErrBackupNotFound, name) } - } - } - if err != nil { - return err - } + err = deleteSnapshots(ctx, store, entries) + if err == nil { + return true, nil + } + + contextLogger.Info("DeleteBackup: an error occurred while trying to delete backup's snapshots, retrying...", + "backupName", name, "error", err) + + return false, nil + }, + ) + + return retryErr +} - if deleted == 0 { - return fmt.Errorf("%w: %s", ErrBackupNotFound, name) +// deleteSnapshots deletes every given entry, joining and returning any +// deletion errors. +func deleteSnapshots( + ctx context.Context, + store snapshotStore, + entries []kopia.Manifest, +) error { + contextLogger := log.FromContext(ctx) + + var err error + + for _, entry := range entries { + contextLogger.Info("DeleteBackup: deleting snapshot", "snapshotID", entry.ID) + if deleteErr := store.DeleteSnapshot(ctx, entry.ID); deleteErr != nil { + err = errors.Join(err, deleteErr) + } } - return nil + return err } diff --git a/core/internal/client/klioclient/kopia/delete_test.go b/core/internal/client/klioclient/kopia/delete_test.go new file mode 100644 index 00000000..640d93d2 --- /dev/null +++ b/core/internal/client/klioclient/kopia/delete_test.go @@ -0,0 +1,158 @@ +/* +Copyright © contributors to CloudNativePG, established as +CloudNativePG a Series of LF Projects, LLC. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. + +SPDX-License-Identifier: Apache-2.0 +*/ + +package kopia + +import ( + "context" + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/util/wait" + + "github.com/cloudnative-pg/klio/core/internal/kopia" +) + +var errDeleteFailed = errors.New("no snapshots matched") + +// fakeSnapshotStore serves a scripted sequence of snapshot listings and records +// the IDs it was asked to delete. Listings are consumed one per call so a test +// can model the manifest IDs changing between attempts. +type fakeSnapshotStore struct { + listings [][]kopia.Manifest + listCalls int + + // failDeleteOf holds the IDs whose deletion fails. + failDeleteOf map[string]bool + + deleted []string + listErr error +} + +func (f *fakeSnapshotStore) ListSnapshots( + _ context.Context, + _ map[string]string, + _ kopia.LogFunc, +) ([]kopia.Manifest, error) { + if f.listErr != nil { + return nil, f.listErr + } + + f.listCalls++ + + // The last scripted listing is reused for any further attempt. + idx := min(f.listCalls-1, len(f.listings)-1) + + return f.listings[idx], nil +} + +func (f *fakeSnapshotStore) DeleteSnapshot(_ context.Context, id string) error { + if f.failDeleteOf[id] { + return errDeleteFailed + } + + f.deleted = append(f.deleted, id) + + return nil +} + +func manifest(id, host string) kopia.Manifest { + return kopia.Manifest{ID: id, Source: kopia.SourceInfo{Host: host}} +} + +func TestDeleteBackupSnapshots(t *testing.T) { + ctx := context.Background() + + oldBackoff := deleteBackupBackoff + deleteBackupBackoff.Duration = 0 + deleteBackupBackoff.Cap = 0 + t.Cleanup(func() { deleteBackupBackoff = oldBackoff }) + + t.Run("deletes every snapshot of the backup on the host", func(t *testing.T) { + store := &fakeSnapshotStore{ + listings: [][]kopia.Manifest{ + {manifest("a", "cluster"), manifest("b", "cluster")}, + {}, + }, + } + + require.NoError(t, deleteBackupSnapshots(ctx, store, "cluster", "backup-1")) + assert.Equal(t, []string{"a", "b"}, store.deleted) + assert.Equal(t, 1, store.listCalls) + }) + + t.Run("ignores snapshots belonging to another host", func(t *testing.T) { + store := &fakeSnapshotStore{ + listings: [][]kopia.Manifest{{manifest("a", "other-cluster")}}, + } + + err := deleteBackupSnapshots(ctx, store, "cluster", "backup-1") + + require.ErrorIs(t, err, ErrBackupNotFound) + assert.Empty(t, store.deleted) + }) + + t.Run("returns not found when the backup has no snapshots", func(t *testing.T) { + store := &fakeSnapshotStore{listings: [][]kopia.Manifest{{}}} + + require.ErrorIs(t, deleteBackupSnapshots(ctx, store, "cluster", "backup-1"), ErrBackupNotFound) + }) + + // A concurrent "kopia snapshot pin" rewrites a snapshot's manifest under a + // new ID, so deleting the ID we listed matches nothing. Re-resolving must + // pick up the new ID and finish the deletion. + t.Run("retries with the rewritten manifest ID", func(t *testing.T) { + store := &fakeSnapshotStore{ + listings: [][]kopia.Manifest{ + {manifest("stale", "cluster")}, + {manifest("stale", "cluster")}, + {manifest("rewritten", "cluster")}, + {}, + }, + failDeleteOf: map[string]bool{"stale": true}, + } + + require.NoError(t, deleteBackupSnapshots(ctx, store, "cluster", "backup-1")) + assert.Equal(t, []string{"rewritten"}, store.deleted) + assert.Equal(t, 3, store.listCalls) + }) + + t.Run("gives up after the attempt budget and reports the failure", func(t *testing.T) { + store := &fakeSnapshotStore{ + listings: [][]kopia.Manifest{{manifest("stuck", "cluster")}}, + failDeleteOf: map[string]bool{"stuck": true}, + } + + err := deleteBackupSnapshots(ctx, store, "cluster", "backup-1") + + require.Error(t, err) + assert.True(t, wait.Interrupted(err), "expected the attempt budget to be exhausted, got: %v", err) + assert.Equal(t, deleteBackupBackoff.Steps, store.listCalls) + assert.Empty(t, store.deleted) + }) + + t.Run("a listing failure is returned as is", func(t *testing.T) { + listErr := errors.New("connection refused") + store := &fakeSnapshotStore{listErr: listErr} + + require.ErrorIs(t, deleteBackupSnapshots(ctx, store, "cluster", "backup-1"), listErr) + }) +} diff --git a/core/internal/client/klioclient/kopia/verify.go b/core/internal/client/klioclient/kopia/verify.go index 040de624..1ea33055 100644 --- a/core/internal/client/klioclient/kopia/verify.go +++ b/core/internal/client/klioclient/kopia/verify.go @@ -21,6 +21,7 @@ package kopia import ( "context" + "errors" "fmt" "github.com/cloudnative-pg/machinery/pkg/log" @@ -29,6 +30,10 @@ import ( kopiaClient "github.com/cloudnative-pg/klio/core/internal/kopia" ) +// ErrNoSnapshotsForBackup is returned when no verifiable snapshot can be +// found for a backup name. +var ErrNoSnapshotsForBackup = errors.New("no snapshots found for backup") + // BackupVerificationError is returned when backup verification detects corruption. type BackupVerificationError struct { // Result contains the verification details from Kopia. @@ -90,22 +95,29 @@ func (s *Connection) verifyAllBackups(ctx context.Context, hostname string) erro return nil } -// verifySpecificBackups verifies the specified backups by resolving their snapshot IDs. +// verifySpecificBackups verifies the specified backups by resolving them to the +// root object IDs of their snapshots. func (s *Connection) verifySpecificBackups(ctx context.Context, hostname string, backupNames []string) error { contextLogger := log.FromContext(ctx) - var allSnapshotIDs []string + if len(backupNames) == 0 { + return nil + } + + var rootObjectIDs []string + for _, name := range backupNames { - ids, err := s.getSnapshotIDsForBackup(ctx, hostname, name) + ids, err := s.getBackupRootObjIDs(ctx, hostname, name) if err != nil { return fmt.Errorf("backup %q: %w", name, err) } - allSnapshotIDs = append(allSnapshotIDs, ids...) + + rootObjectIDs = append(rootObjectIDs, ids...) } - contextLogger.Info("Verifying backups", "backupNames", backupNames, "snapshotCount", len(allSnapshotIDs)) + contextLogger.Info("Verifying backups", "backupNames", backupNames, "objectsCount", len(rootObjectIDs)) - result, err := s.kopia.VerifySnapshots(ctx, allSnapshotIDs...) + result, err := s.kopia.VerifySnapshots(ctx, rootObjectIDs...) if err != nil { return classifyVerifyError(ctx, result, err) } @@ -115,8 +127,12 @@ func (s *Connection) verifySpecificBackups(ctx context.Context, hostname string, return nil } -// getSnapshotIDsForBackup resolves a backup name to its constituent Kopia snapshot IDs. -func (s *Connection) getSnapshotIDsForBackup(ctx context.Context, hostname, backupName string) ([]string, error) { +// getBackupRootObjIDs resolves a backup name to the root object IDs of its +// constituent Kopia snapshots. +func (s *Connection) getBackupRootObjIDs( + ctx context.Context, + hostname, backupName string, +) ([]string, error) { contextLogger := log.FromContext(ctx) entries, err := s.kopia.ListSnapshots(ctx, map[string]string{ @@ -126,18 +142,29 @@ func (s *Connection) getSnapshotIDsForBackup(ctx context.Context, hostname, back return nil, err } - var ids []string + rootObjIDs := make([]string, 0, len(entries)) + for _, e := range entries { - if e.Source.Host == hostname { - ids = append(ids, e.ID) + if e.Source.Host != hostname { + continue } + + // An incomplete snapshot has no root entry to verify. + if e.RootEntry == nil || e.RootEntry.ObjID == "" { + contextLogger.Info("Skipping snapshot without a root object ID", + "backupName", backupName, "snapshotID", e.ID) + + continue + } + + rootObjIDs = append(rootObjIDs, e.RootEntry.ObjID) } - if len(ids) == 0 { - return nil, fmt.Errorf("no snapshots found for backup %q", backupName) + if len(rootObjIDs) == 0 { + return nil, fmt.Errorf("%w: %q", ErrNoSnapshotsForBackup, backupName) } - return ids, nil + return rootObjIDs, nil } // classifyVerifyError inspects the verify result to distinguish corruption diff --git a/core/internal/client/klioclient/kopia/verify_test.go b/core/internal/client/klioclient/kopia/verify_test.go index ab6d8064..492fcd32 100644 --- a/core/internal/client/klioclient/kopia/verify_test.go +++ b/core/internal/client/klioclient/kopia/verify_test.go @@ -99,4 +99,38 @@ func TestClassifyVerifyError(t *testing.T) { require.ErrorIs(t, err, infraErr) require.NotErrorAs(t, err, &backupErr) }) + + // A missing object or blob is real data loss and must stay fatal, even + // though Kopia reports it with wording that also contains "not found". + t.Run("missing blob is still corruption", func(t *testing.T) { + verifyErr := errors.New("while verifying Kopia snapshots: command failed: exit status 1") + result := kopiaClient.VerifyResult{ + ErrorCount: 1, + ErrorStrings: []string{ + "object 8f848427a18ebe0fbc2f063b4616e362 is backed by missing blob " + + "p79f56bafb33cd7823558352ea947c830-s59fd9cbc252f04f7143", + }, + } + + err := classifyVerifyError(ctx, result, verifyErr) + + var backupErr *BackupVerificationError + require.ErrorAs(t, err, &backupErr) + }) + + t.Run("missing object referenced by a directory id is still corruption", func(t *testing.T) { + verifyErr := errors.New("while verifying Kopia snapshots: command failed: exit status 1") + result := kopiaClient.VerifyResult{ + ErrorCount: 1, + ErrorStrings: []string{ + "error reading directory: unable to open object: kdeadbeef: " + + "content kdeadbeef not found: object not found", + }, + } + + err := classifyVerifyError(ctx, result, verifyErr) + + var backupErr *BackupVerificationError + require.ErrorAs(t, err, &backupErr) + }) } diff --git a/core/internal/kopia/write.go b/core/internal/kopia/write.go index bf1a798f..2e83fab8 100644 --- a/core/internal/kopia/write.go +++ b/core/internal/kopia/write.go @@ -239,22 +239,13 @@ func (s *Client) SnapshotFileContent( return nil } -// VerifySnapshots verifies snapshot integrity. When called with no -// snapshotIDs, all snapshots in the repository are verified. +// VerifySnapshots verifies snapshots integrity by targeting directly their root object IDs. // It uses --json output to distinguish corruption (errorCount > 0) from // infrastructure errors (command failed but no corruption detected). -func (s *Client) VerifySnapshots(ctx context.Context, snapshotIDs ...string) (VerifyResult, error) { +func (s *Client) VerifySnapshots(ctx context.Context, objectIDs ...string) (VerifyResult, error) { contextLogger := log.FromContext(ctx) - args := make([]string, 0, 5+len(snapshotIDs)) - args = append(args, - "snapshot", - "verify", - "--json", - "--disable-file-logging", - "--config-file="+s.ConfigFile, - ) - args = append(args, snapshotIDs...) + args := buildVerifyArgs(s.ConfigFile, objectIDs...) contextLogger.Info("Verifying Kopia snapshots", "args", args) @@ -269,3 +260,21 @@ func (s *Client) VerifySnapshots(ctx context.Context, snapshotIDs ...string) (Ve return parseVerifyOutput(stdout.Bytes()), nil } + +// buildVerifyArgs builds the "kopia snapshot verify" arguments. +func buildVerifyArgs(configFile string, objectIDs ...string) []string { + args := make([]string, 0, 5+len(objectIDs)) + args = append(args, + "snapshot", + "verify", + "--json", + "--disable-file-logging", + "--config-file="+configFile, + ) + + for _, id := range objectIDs { + args = append(args, "--file-id="+id) + } + + return args +} diff --git a/core/internal/kopia/write_test.go b/core/internal/kopia/write_test.go index 18c87862..87ba8955 100644 --- a/core/internal/kopia/write_test.go +++ b/core/internal/kopia/write_test.go @@ -50,3 +50,35 @@ func TestParseVerifyOutput(t *testing.T) { assert.Equal(t, VerifyResult{}, result) }) } + +func TestBuildVerifyArgs(t *testing.T) { + t.Run("no selection verifies everything visible to the client", func(t *testing.T) { + args := buildVerifyArgs("/etc/kopia/config") + + assert.Equal(t, []string{ + "snapshot", + "verify", + "--json", + "--disable-file-logging", + "--config-file=/etc/kopia/config", + }, args) + }) + + // Root object IDs are passed as --file-id rather than as positional + // snapshot manifest IDs, which "kopia snapshot pin" can rewrite while a + // verification is in flight. Kopia auto-detects a directory root passed + // this way, so both directory and file roots go through --file-id. + t.Run("object IDs are passed as --file-id flags", func(t *testing.T) { + args := buildVerifyArgs("/etc/kopia/config", "kaaa", "kbbb") + + assert.Equal(t, []string{ + "snapshot", + "verify", + "--json", + "--disable-file-logging", + "--config-file=/etc/kopia/config", + "--file-id=kaaa", + "--file-id=kbbb", + }, args) + }) +} diff --git a/operator/test/e2e/wal_retention_test.go b/operator/test/e2e/wal_retention_test.go index 7a41de88..aa616244 100644 --- a/operator/test/e2e/wal_retention_test.go +++ b/operator/test/e2e/wal_retention_test.go @@ -251,6 +251,47 @@ func (s *walRetentionScenario) deleteBackup( return nil } +// verifyBackups runs "klio backup verify" on tier1 for the given backup names +// using the klio CLI. +// +// The backups it is given have already been relayed to tier2 and unpinned, so +// their Kopia snapshot manifests were rewritten under new IDs after they were +// taken. Verification resolves each snapshot to its root object ID, which the +// rewrite leaves untouched, and routes directory and file roots to different +// Kopia flags: a backup has both, since pgdata and metadata are directory +// snapshots while the control data file is snapshotted on its own. Verifying +// here covers that resolution against real backups. +func (s *walRetentionScenario) verifyBackups( + ctx context.Context, + r *resources.Resources, + backupNames []string, +) error { + const klioConfigPath = "/var/lib/postgresql/klio/klio-archive" + + var stdout, stderr bytes.Buffer + + verifyCmd := make([]string, 0, 6+len(backupNames)) + verifyCmd = append(verifyCmd, + "klio", + "backup", + "verify", + "--config", + klioConfigPath, + "--tiers=tier1", + ) + verifyCmd = append(verifyCmd, backupNames...) + + err := r.ExecInPod( + ctx, s.namespace.Name, s.sourcePrimaryPod.Name, cnpgi.KlioPluginContainerName, verifyCmd, &stdout, &stderr) + if err != nil { + return fmt.Errorf( + "failed to verify backups %v: %w; stdout: %s; stderr: %s", + backupNames, err, stdout.String(), stderr.String()) + } + + return nil +} + // kopiaBackupInfo mirrors the subset of klioclient.BackupMetadata fields // needed to identify and order the backups printed by "klio backup list". type kopiaBackupInfo struct { @@ -450,6 +491,27 @@ func (f *WALRetentionFeature) Run() types.StepFunc { t.Logf("Server-side WAL retention verified: %d WAL files remain, all >= begin WAL %q", len(walFiles), boundary) + // Step 7: the newest backup has been through a full maintenance pass, so + // the tier2 unpin has already rewritten its snapshot manifests by the + // time we get here. Verifying it now exercises real resolution by root + // object ID against a backup that mixes directory and file roots. It + // does not reproduce the manifest-rewrite race itself, since maintenance + // has settled long before this step runs. + // + // Only the newest backup is verified: "klio backup list" spans both + // tiers, so it also reports the backup deleted in step 4, which no + // longer has tier1 snapshots to verify. + t.Log("Verifying the newest backup on tier1...") + remainingBackups, err := f.scenario.listBackups(ctx, r) + require.NoError(t, err, "failed to list backups before verification") + require.NotEmpty(t, remainingBackups, "no backups left to verify") + + newestBackup := remainingBackups[len(remainingBackups)-1] + require.NoError(t, f.scenario.verifyBackups(ctx, r, []string{newestBackup}), + "verification failed for backup %q", newestBackup) + + t.Logf("Verified newest backup %q on tier1", newestBackup) + return ctx } }