diff --git a/AGENTS.md b/AGENTS.md index a3494ba3..f35b55c0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -198,8 +198,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 @@ -212,6 +212,36 @@ 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. + Root objects come in two kinds and take different flags: `--directory-id` + for the pgdata and metadata snapshots, `--file-id` for the control data + file, which is snapshotted on its own. Passing a file root to + `--directory-id` makes Kopia parse file content as a directory listing and + report healthy data as corrupt. +- **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..bc1a238b 100644 --- a/core/internal/client/klioclient/kopia/delete.go +++ b/core/internal/client/klioclient/kopia/delete.go @@ -23,46 +23,140 @@ import ( "context" "errors" "fmt" + "time" "github.com/cloudnative-pg/machinery/pkg/log" "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") +// deleteBackupAttempts bounds how many times DeleteBackup re-resolves the +// snapshots of a backup before giving up. +const deleteBackupAttempts = 3 + +// deleteBackupRetryDelay is the wait between DeleteBackup attempts, giving a +// concurrent "kopia snapshot pin" rewrite time to settle before the next +// attempt re-resolves snapshot IDs. Tests override this to keep the suite +// fast. +// +//nolint:gochecknoglobals +var deleteBackupRetryDelay = 200 * time.Millisecond + +// 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 { - contextLogger := log.FromContext(ctx) + return deleteBackupSnapshots(ctx, s.kopia, hostname, name) +} - // 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) - } +// deleteBackupSnapshots removes every snapshot of a backup on the given host. +// +// Snapshots are deleted by manifest ID, which post-backup maintenance can +// rewrite underneath us: "kopia snapshot pin" replaces a snapshot's manifest +// with a new ID, and deleting a replaced ID matches nothing and fails, leaving +// the backup in place. Deleting by root object ID is not an option, because +// unchanged content dedupes to the same root across backups and Kopia deletes +// every snapshot matching the ID it is given. Instead, resolve the snapshots +// again and retry: each attempt lists the current IDs, and snapshots deleted by +// an earlier attempt are simply no longer listed. +func deleteBackupSnapshots(ctx context.Context, store snapshotStore, hostname, name string) error { + contextLogger := log.FromContext(ctx) 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++ + + var lastErr error + + for attempt := 1; attempt <= deleteBackupAttempts; attempt++ { + // List all snapshots for this backup (all content types) + entries, err := store.ListSnapshots(ctx, map[string]string{ + klioclient.BackupNameTagName: name, + }, contextLogger.Debug) + if err != nil { + return fmt.Errorf("while listing snapshots: %w", err) + } + + pending, deletedNow, attemptErr := deleteHostSnapshots(ctx, store, contextLogger, hostname, entries) + deleted += deletedNow + + // Nothing left to delete: either we removed everything, or the backup + // was not there to begin with. + if pending == 0 { + if deleted > 0 { + return nil } + + return fmt.Errorf("%w: %s", ErrBackupNotFound, name) + } + + if attemptErr == nil { + return nil } - } - if err != nil { - return err + lastErr = attemptErr + + contextLogger.Info("DeleteBackup: retrying with freshly resolved snapshot IDs", + "backupName", name, "attempt", attempt, "error", attemptErr) + + if attempt < deleteBackupAttempts { + if waitErr := waitBeforeDeleteRetry(ctx); waitErr != nil { + return waitErr + } + } } - if deleted == 0 { - return fmt.Errorf("%w: %s", ErrBackupNotFound, name) + return lastErr +} + +// deleteHostSnapshots deletes every entry belonging to hostname, returning how +// many of them matched the host (pending), how many were deleted, and the +// deletion errors joined together. +func deleteHostSnapshots( + ctx context.Context, + store snapshotStore, + contextLogger log.Logger, + hostname string, + entries []kopia.Manifest, +) (int, int, error) { + var pending, deleted int + + var err error + + for _, entry := range entries { + if entry.Source.Host != hostname { + continue + } + + pending++ + + contextLogger.Info("DeleteBackup: deleting snapshot", "snapshotID", entry.ID) + if deleteErr := store.DeleteSnapshot(ctx, entry.ID); deleteErr != nil { + err = errors.Join(err, deleteErr) + + continue + } + + deleted++ } - return nil + return pending, deleted, err +} + +// waitBeforeDeleteRetry pauses deleteBackupRetryDelay before the next +// DeleteBackup attempt, returning early with ctx's error if it is cancelled +// first. +func waitBeforeDeleteRetry(ctx context.Context) error { + select { + case <-ctx.Done(): + return ctx.Err() + case <-time.After(deleteBackupRetryDelay): + return nil + } } 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..d059b5ea --- /dev/null +++ b/core/internal/client/klioclient/kopia/delete_test.go @@ -0,0 +1,154 @@ +/* +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" + + "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() + + oldDelay := deleteBackupRetryDelay + deleteBackupRetryDelay = 0 + t.Cleanup(func() { deleteBackupRetryDelay = oldDelay }) + + 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("a", "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{"a", "rewritten"}, store.deleted) + assert.Equal(t, 2, 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.ErrorIs(t, err, errDeleteFailed) + assert.Equal(t, deleteBackupAttempts, 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..8b6830d0 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,16 @@ import ( kopiaClient "github.com/cloudnative-pg/klio/core/internal/kopia" ) +var ( + // ErrNoSnapshotsForBackup is returned when no verifiable snapshot can be + // found for a backup name. + ErrNoSnapshotsForBackup = errors.New("no snapshots found for backup") + + // ErrUnsupportedRootEntryType is returned when a snapshot's root entry is + // neither a directory nor a file, so it cannot be verified by object ID. + ErrUnsupportedRootEntryType = errors.New("unsupported snapshot root entry type") +) + // BackupVerificationError is returned when backup verification detects corruption. type BackupVerificationError struct { // Result contains the verification details from Kopia. @@ -80,7 +91,7 @@ func (s *Connection) verifyAllBackups(ctx context.Context, hostname string) erro // verify all snapshots for this hostname. contextLogger.Info("Verifying all backups for hostname", "hostname", hostname) - result, err := s.kopia.VerifySnapshots(ctx) + result, err := s.kopia.VerifySnapshots(ctx, kopiaClient.VerifySnapshotsOptions{}) if err != nil { return classifyVerifyError(ctx, result, err) } @@ -90,24 +101,44 @@ 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. +// +// A backup whose snapshots fail to resolve does not stop the others from being +// verified: their resolution errors are joined and returned alongside whatever +// verification result the successfully resolved backups produce. func (s *Connection) verifySpecificBackups(ctx context.Context, hostname string, backupNames []string) error { contextLogger := log.FromContext(ctx) - var allSnapshotIDs []string + var verifyOpts kopiaClient.VerifySnapshotsOptions + + var resolveErr error + for _, name := range backupNames { - ids, err := s.getSnapshotIDsForBackup(ctx, hostname, name) + opts, err := s.getRootObjectsForBackup(ctx, hostname, name) if err != nil { - return fmt.Errorf("backup %q: %w", name, err) + resolveErr = errors.Join(resolveErr, fmt.Errorf("backup %q: %w", name, err)) + + continue } - allSnapshotIDs = append(allSnapshotIDs, ids...) + + verifyOpts.DirectoryIDs = append(verifyOpts.DirectoryIDs, opts.DirectoryIDs...) + verifyOpts.FileIDs = append(verifyOpts.FileIDs, opts.FileIDs...) + } + + if verifyOpts.IsEmpty() { + return resolveErr } - contextLogger.Info("Verifying backups", "backupNames", backupNames, "snapshotCount", len(allSnapshotIDs)) + contextLogger.Info("Verifying backups", "backupNames", backupNames, "snapshotCount", verifyOpts.Len()) - result, err := s.kopia.VerifySnapshots(ctx, allSnapshotIDs...) + result, err := s.kopia.VerifySnapshots(ctx, verifyOpts) if err != nil { - return classifyVerifyError(ctx, result, err) + return errors.Join(resolveErr, classifyVerifyError(ctx, result, err)) + } + + if resolveErr != nil { + return resolveErr } contextLogger.Info("All backups verified successfully") @@ -115,29 +146,63 @@ 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) { +// getRootObjectsForBackup resolves a backup name to the root objects of its +// constituent Kopia snapshots, split by object kind. +// +// The root object ID is used rather than the snapshot manifest ID because it is +// a stable identity. Post-backup maintenance unpins the snapshots of a backup +// that reached tier2, and "kopia snapshot pin" rewrites the manifest under a +// new ID, so a manifest ID resolved here can already have been replaced by the +// time verification runs. The root object ID is untouched by that rewrite. +// +// A backup mixes both kinds: pgdata and metadata are directory snapshots, while +// the control data file is snapshotted on its own and so has a file root. +func (s *Connection) getRootObjectsForBackup( + ctx context.Context, + hostname, backupName string, +) (kopiaClient.VerifySnapshotsOptions, error) { contextLogger := log.FromContext(ctx) + var opts kopiaClient.VerifySnapshotsOptions + entries, err := s.kopia.ListSnapshots(ctx, map[string]string{ klioclient.BackupNameTagName: backupName, }, contextLogger.Debug) if err != nil { - return nil, err + return opts, err } - var ids []string 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 + } + + switch e.RootEntry.Type { + case kopiaClient.EntryTypeDirectory: + opts.DirectoryIDs = append(opts.DirectoryIDs, e.RootEntry.ObjID) + case kopiaClient.EntryTypeFile: + opts.FileIDs = append(opts.FileIDs, e.RootEntry.ObjID) + default: + // Verifying with the wrong flag reports a healthy object as + // corrupt, so refuse rather than guess. + return opts, fmt.Errorf("%w: snapshot %q has root entry type %q", + ErrUnsupportedRootEntryType, e.ID, e.RootEntry.Type) } } - if len(ids) == 0 { - return nil, fmt.Errorf("no snapshots found for backup %q", backupName) + if opts.IsEmpty() { + return opts, fmt.Errorf("%w: %q", ErrNoSnapshotsForBackup, backupName) } - return ids, nil + return opts, 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/data.go b/core/internal/kopia/data.go index 3915c69e..9e5fea6a 100644 --- a/core/internal/kopia/data.go +++ b/core/internal/kopia/data.go @@ -54,11 +54,26 @@ type Manifest struct { Pins []string `json:"pins,omitempty"` } +// Entry types reported by Kopia for a directory entry. A snapshot's root entry +// is a directory for a directory snapshot and a file when a single file was +// snapshotted, and the two need different flags when verifying by object ID. +const ( + // EntryTypeDirectory marks an entry backed by a directory object. + EntryTypeDirectory = "d" + + // EntryTypeFile marks an entry backed by a file object. + EntryTypeFile = "f" +) + // DirEntry represents a directory entry as stored in JSON stream. type DirEntry struct { // Name is the name of the file or directory. Name string `json:"name,omitempty"` + // Type is the kind of object the entry points at, one of + // EntryTypeDirectory or EntryTypeFile. + Type string `json:"type,omitempty"` + // FileSize is the size of the file in bytes. FileSize int64 `json:"size,omitempty"` diff --git a/core/internal/kopia/write.go b/core/internal/kopia/write.go index bf1a798f..5db2544f 100644 --- a/core/internal/kopia/write.go +++ b/core/internal/kopia/write.go @@ -239,22 +239,66 @@ func (s *Client) SnapshotFileContent( return nil } -// VerifySnapshots verifies snapshot integrity. When called with no -// snapshotIDs, all snapshots in the repository are verified. -// 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) { - contextLogger := log.FromContext(ctx) +// VerifySnapshotsOptions selects what a verification run covers. +// +// Root object IDs are used rather than snapshot manifest IDs because they are a +// stable identity: "kopia snapshot pin" rewrites a snapshot's manifest under a +// new ID and deletes the old one, so a manifest ID resolved a moment earlier can +// already be gone by the time verification runs. The root object ID is +// unaffected by that rewrite. +// +// Directory and file objects take different flags: passing a file object to +// --directory-id makes Kopia parse file content as a directory listing and +// report it as corruption. +type VerifySnapshotsOptions struct { + // DirectoryIDs restricts verification to the given directory object IDs. + DirectoryIDs []string + + // FileIDs restricts verification to the given file object IDs. + FileIDs []string +} + +// IsEmpty returns true when no object is selected, meaning every snapshot +// visible to the client is verified. +func (o VerifySnapshotsOptions) IsEmpty() bool { + return len(o.DirectoryIDs) == 0 && len(o.FileIDs) == 0 +} + +// Len returns the number of selected objects. +func (o VerifySnapshotsOptions) Len() int { + return len(o.DirectoryIDs) + len(o.FileIDs) +} - args := make([]string, 0, 5+len(snapshotIDs)) +// buildVerifyArgs builds the "kopia snapshot verify" arguments. +func buildVerifyArgs(configFile string, opts VerifySnapshotsOptions) []string { + args := make([]string, 0, 5+opts.Len()) args = append(args, "snapshot", "verify", "--json", "--disable-file-logging", - "--config-file="+s.ConfigFile, + "--config-file="+configFile, ) - args = append(args, snapshotIDs...) + + for _, id := range opts.DirectoryIDs { + args = append(args, "--directory-id="+id) + } + + for _, id := range opts.FileIDs { + args = append(args, "--file-id="+id) + } + + return args +} + +// VerifySnapshots verifies snapshot integrity. When opts selects nothing, all +// snapshots visible to the client are verified. +// 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, opts VerifySnapshotsOptions) (VerifyResult, error) { + contextLogger := log.FromContext(ctx) + + args := buildVerifyArgs(s.ConfigFile, opts) contextLogger.Info("Verifying Kopia snapshots", "args", args) diff --git a/core/internal/kopia/write_test.go b/core/internal/kopia/write_test.go index 18c87862..864feff2 100644 --- a/core/internal/kopia/write_test.go +++ b/core/internal/kopia/write_test.go @@ -50,3 +50,79 @@ 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", VerifySnapshotsOptions{}) + + assert.Equal(t, []string{ + "snapshot", + "verify", + "--json", + "--disable-file-logging", + "--config-file=/etc/kopia/config", + }, args) + }) + + // Root object IDs are passed as --directory-id/--file-id rather than as + // positional snapshot manifest IDs, which "kopia snapshot pin" can rewrite + // while a verification is in flight. + t.Run("directory IDs are passed as --directory-id flags", func(t *testing.T) { + args := buildVerifyArgs("/etc/kopia/config", VerifySnapshotsOptions{ + DirectoryIDs: []string{"kaaa", "kbbb"}, + }) + + assert.Equal(t, []string{ + "snapshot", + "verify", + "--json", + "--disable-file-logging", + "--config-file=/etc/kopia/config", + "--directory-id=kaaa", + "--directory-id=kbbb", + }, args) + assert.NotContains(t, args, "kaaa") + }) + + // A file root passed to --directory-id makes Kopia parse file content as a + // directory listing and report a healthy object as corrupt. + t.Run("file IDs are passed as --file-id flags", func(t *testing.T) { + args := buildVerifyArgs("/etc/kopia/config", VerifySnapshotsOptions{ + DirectoryIDs: []string{"kaaa"}, + FileIDs: []string{"d8fe6706"}, + }) + + assert.Equal(t, []string{ + "snapshot", + "verify", + "--json", + "--disable-file-logging", + "--config-file=/etc/kopia/config", + "--directory-id=kaaa", + "--file-id=d8fe6706", + }, args) + }) +} + +func TestVerifySnapshotsOptions(t *testing.T) { + t.Run("empty selection", func(t *testing.T) { + var opts VerifySnapshotsOptions + assert.True(t, opts.IsEmpty()) + assert.Equal(t, 0, opts.Len()) + }) + + t.Run("counts both kinds of object", func(t *testing.T) { + opts := VerifySnapshotsOptions{ + DirectoryIDs: []string{"kaaa", "kbbb"}, + FileIDs: []string{"ccc"}, + } + assert.False(t, opts.IsEmpty()) + assert.Equal(t, 3, opts.Len()) + }) + + t.Run("file IDs alone are a selection", func(t *testing.T) { + opts := VerifySnapshotsOptions{FileIDs: []string{"ccc"}} + assert.False(t, opts.IsEmpty()) + assert.Equal(t, 1, opts.Len()) + }) +} 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 } }