diff --git a/README.md b/README.md index f83d065..d283310 100644 --- a/README.md +++ b/README.md @@ -446,6 +446,9 @@ once the store is reachable to remove the snapshot's metadata from it (`vaultik prune` does not). For a snapshot that is only on the destination store, that failure is an error. To wipe everything on the destination in one go, use `vaultik remote nuke --force`. +* Accepts a snapshot ID, or — for a snapshot only on the destination + store — its remote key (or an unambiguous leading part of it) as shown + by `snapshot list`. * `--local-only`: Skip remote cleanup; only touch the local index * `--dry-run`: Show what would be deleted without deleting * `--force`: Skip confirmation prompt diff --git a/internal/cli/snapshot.go b/internal/cli/snapshot.go index 8996d35..7a3b402 100644 --- a/internal/cli/snapshot.go +++ b/internal/cli/snapshot.go @@ -288,6 +288,10 @@ know about), which is what 'vaultik prune' does. The command prints the 'vaultik prune' invocation to run as a follow-up only when it removed the snapshot's metadata from the destination store, and never under --json. +The snapshot may be named by its ID or, on a host with no local index, by +the remote key that 'snapshot list' prints for a remote-only snapshot (an +unambiguous leading part is enough). + An ID that names no snapshot in the local index or on the destination store is an error. diff --git a/internal/vaultik/remove_snapshot_test.go b/internal/vaultik/remove_snapshot_test.go index 9d43aec..46d196a 100644 --- a/internal/vaultik/remove_snapshot_test.go +++ b/internal/vaultik/remove_snapshot_test.go @@ -451,6 +451,100 @@ func TestRemoveSnapshot_NotInLocalIndex(t *testing.T) { assert.Contains(t, out, "Removed snapshot metadata from remote storage") } +// TestRemoveSnapshot_RemoteOnlyByRemoteKey checks that a snapshot only +// on the destination store, on a host with no local index, is removed by +// the remote key that 'snapshot list' prints for such a snapshot, and by +// an unambiguous leading part of it — the same identifiers verify and +// restore accept. Snapshot remove used to hash the argument again, so +// the hashed key never matched and only the plain snapshot ID worked. +func TestRemoveSnapshot_RemoteOnlyByRemoteKey(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + remoteKey := snapshot.RemoteSnapshotKey("snapshot-001") + + identifiers := map[string]string{ + "full key": remoteKey, + "leading part": remoteKey[:8], + } + + for name, identifier := range identifiers { + t.Run(name, func(t *testing.T) { + t.Parallel() + + store := newTestStorer() + addManifest(t, store, "snapshot-001", nil) + + // No local index: a host that sees the snapshot only remotely. + tv := vaultik.NewForTesting(store) + tv.Repositories = newLocalIndex(t) + + result, err := tv.RemoveSnapshot(identifier, + &vaultik.RemoveOptions{Force: true}) + + require.NoError(t, err) + assert.True(t, result.RemoteRemoved) + assert.False(t, store.hasKey(remoteKeyPath("snapshot-001"))) + + out := tv.Stdout.String() + assert.NotContains(t, out, "from local database") + assert.Contains(t, out, "Removed snapshot metadata from remote storage") + }) + } +} + +// TestRemoveSnapshot_LocalIndexByRemoteKey checks that a remote key (or +// its unambiguous leading part) that names a snapshot the local index +// holds removes that snapshot's local row and its remote metadata +// together. `snapshot list --json` prints a remote key for every +// snapshot, including local ones. +func TestRemoveSnapshot_LocalIndexByRemoteKey(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + remoteKey := snapshot.RemoteSnapshotKey("snapshot-001") + + identifiers := map[string]string{ + "full key": remoteKey, + "leading part": remoteKey[:8], + } + + for name, identifier := range identifiers { + t.Run(name, func(t *testing.T) { + t.Parallel() + + store := newTestStorer() + addManifest(t, store, "snapshot-001", nil) + + // A backup wrote to this destination, so it carries the + // marker, and the local index holds the snapshot. + require.NoError(t, store.Put(context.Background(), markerFile, + bytes.NewReader(nil))) + + tv := vaultik.NewForTesting(store) + repos := newLocalIndex(t, "snapshot-001") + tv.Repositories = repos + + result, err := tv.RemoveSnapshot(identifier, + &vaultik.RemoveOptions{Force: true}) + + require.NoError(t, err) + assert.True(t, result.RemoteRemoved) + assert.False(t, store.hasKey(remoteKeyPath("snapshot-001"))) + + row, err := repos.Snapshots.GetByID( + context.Background(), "snapshot-001") + require.NoError(t, err) + assert.Nil(t, row) + + out := tv.Stdout.String() + assert.Contains(t, out, + "Removed snapshot 'snapshot-001' from local database") + assert.Contains(t, out, "Removed snapshot metadata from remote storage") + }) + } +} + // TestRemoveSnapshot_NotInLocalIndexDeleteFails checks that removing a // snapshot only the destination store holds fails when its metadata // cannot be deleted there, under --force and under --json: nothing was diff --git a/internal/vaultik/snapshot.go b/internal/vaultik/snapshot.go index 68b7d00..52b9835 100644 --- a/internal/vaultik/snapshot.go +++ b/internal/vaultik/snapshot.go @@ -1480,7 +1480,7 @@ func (v *Vaultik) RemoveSnapshot( return result, err } - inLocalIndex, err := v.findSnapshotToRemove(snapshotID, opts) + inLocalIndex, localID, remoteKey, err := v.findSnapshotToRemove(snapshotID, opts) if err != nil { return result, err } @@ -1502,16 +1502,16 @@ func (v *Vaultik) RemoveSnapshot( } if inLocalIndex { - log.Info("Removing snapshot from local database", "snapshot_id", snapshotID) + log.Info("Removing snapshot from local database", "snapshot_id", localID) - err = v.deleteSnapshotFromLocalDB(snapshotID) + err = v.deleteSnapshotFromLocalDB(localID) if err != nil { return result, fmt.Errorf("removing from local database: %w", err) } } result.RemoteRemoved, err = v.removeSnapshotRemote( - snapshotID, inLocalIndex, markerErr, opts) + snapshotID, remoteKey, inLocalIndex, markerErr, opts) if err != nil { return result, err } @@ -1530,7 +1530,7 @@ func (v *Vaultik) RemoveSnapshot( if inLocalIndex { v.UI.Infof("Removed snapshot '%s' from local database.", - v.UI.Snapshot(snapshotID)) + v.UI.Snapshot(localID)) } if result.RemoteRemoved { @@ -1543,50 +1543,68 @@ func (v *Vaultik) RemoveSnapshot( return result, nil } -// findSnapshotToRemove reports whether the local index has a row for -// snapshotID, and returns errSnapshotNotFound when there is nothing to -// remove: no row and, unless LocalOnly is set, no metadata on the -// destination store either. The destination store is consulted only -// when the local index has no row, so a snapshot in the local index can -// still be removed from it while the destination store is unreachable, -// and only after checkDestinationMarker passes, so an unplugged volume's -// empty mount point is not taken for a store without the snapshot. +// findSnapshotToRemove resolves snapshotID, the identifier remove was +// given, and returns whether the local index holds that snapshot, the +// human ID of its row when it does, and the remote key to strip from +// the destination store. A human ID is looked up in the local index +// literally and hashed without consulting the store; a remote key, or +// an unambiguous leading part of one, is resolved against the store's +// metadata listing the way verify and restore do, and then matched back +// to the local row whose hashed ID equals it, so removing by such a key +// removes the local row too. The destination store is consulted only +// after checkDestinationMarker, so an unplugged volume's empty mount +// point is not taken for a store without the snapshot. The identifier +// naming nothing, on the local index or, unless LocalOnly is set, on +// the store, is errSnapshotNotFound; a remote-key form that matches +// nothing or more than one directory surfaces +// resolveSnapshotRemoteKey's errSnapshotKeyNotFound or +// errSnapshotKeyAmbiguous. func (v *Vaultik) findSnapshotToRemove( snapshotID string, opts *RemoveOptions, -) (bool, error) { +) (bool, string, string, error) { if v.Repositories != nil { snap, err := v.Repositories.Snapshots.GetByID(v.ctx, snapshotID) if err != nil { - return false, fmt.Errorf("looking up snapshot in local index: %w", err) + return false, "", "", fmt.Errorf("looking up snapshot in local index: %w", err) } if snap != nil { - return true, nil + return true, snapshotID, snapshot.RemoteSnapshotKey(snapshotID), nil } } if opts.LocalOnly { - return false, fmt.Errorf("%w in the local index: %s", + return false, "", "", fmt.Errorf("%w in the local index: %s", errSnapshotNotFound, snapshotID) } err := v.checkDestinationMarker() if err != nil { - return false, err + return false, "", "", err } - objects, err := v.remoteSnapshotObjects(snapshot.RemoteSnapshotKey(snapshotID)) + remoteKey, err := v.resolveSnapshotRemoteKey(snapshotID) if err != nil { - return false, fmt.Errorf("looking up snapshot on destination store: %w", err) + return false, "", "", err + } + + objects, err := v.remoteSnapshotObjects(remoteKey) + if err != nil { + return false, "", "", fmt.Errorf("looking up snapshot on destination store: %w", err) } if len(objects) == 0 { - return false, fmt.Errorf( + return false, "", "", fmt.Errorf( "%w in the local index or on the destination store: %s", errSnapshotNotFound, snapshotID) } - return false, nil + localID, err := v.localSnapshotIDForRemoteKey(remoteKey) + if err != nil { + return false, "", "", err + } + + return localID != "", localID, remoteKey, nil } // removeSnapshotDryRun reports what RemoveSnapshot would do without @@ -1649,19 +1667,20 @@ func (v *Vaultik) confirmRemoveSnapshot(snapshotID string, opts *RemoveOptions) } // removeSnapshotRemote strips the snapshot's metadata from the -// destination store. It returns true only when the delete finished -// without an error and removed at least one object; under LocalOnly it -// does nothing. markerErr is the result of checkDestinationMarker; when -// it is set, nothing is deleted and it is the failure. When the snapshot -// is only on the destination store, a failure is returned as an error, -// because that removal is all the command does. When the snapshot was in -// the local index, its removal there has already happened, so a failure -// is only a warning telling the user to run `vaultik snapshot remove` -// for the snapshot again once the destination store is reachable -// (`vaultik prune` never removes snapshot metadata that has its -// manifest). +// destination store. remoteKey is the resolved metadata directory name +// to delete, as returned by findSnapshotToRemove. It returns true only +// when the delete finished without an error and removed at least one +// object; under LocalOnly it does nothing. markerErr is the result of +// checkDestinationMarker; when it is set, nothing is deleted and it is +// the failure. When the snapshot is only on the destination store, a +// failure is returned as an error, because that removal is all the +// command does. When the snapshot was in the local index, its removal +// there has already happened, so a failure is only a warning telling +// the user to run `vaultik snapshot remove` for the snapshot again once +// the destination store is reachable (`vaultik prune` never removes +// snapshot metadata that has its manifest). func (v *Vaultik) removeSnapshotRemote( - snapshotID string, inLocalIndex bool, markerErr error, + snapshotID, remoteKey string, inLocalIndex bool, markerErr error, opts *RemoveOptions, ) (bool, error) { if opts.LocalOnly { @@ -1675,8 +1694,7 @@ func (v *Vaultik) removeSnapshotRemote( err := markerErr if err == nil { - deleted, err = v.deleteRemoteSnapshotByKey( - snapshot.RemoteSnapshotKey(snapshotID)) + deleted, err = v.deleteRemoteSnapshotByKey(remoteKey) } if err != nil && !inLocalIndex { @@ -1790,6 +1808,23 @@ func (v *Vaultik) localSnapshotIDs() ([]string, error) { return ids, nil } +// localSnapshotIDForRemoteKey returns the human snapshot ID of the local +// index row whose RemoteSnapshotKey is key, or "" when no row has it. +func (v *Vaultik) localSnapshotIDForRemoteKey(key string) (string, error) { + ids, err := v.localSnapshotIDs() + if err != nil { + return "", fmt.Errorf("listing local snapshots: %w", err) + } + + for _, id := range ids { + if snapshot.RemoteSnapshotKey(id) == key { + return id, nil + } + } + + return "", nil +} + // listAllRemoteSnapshotKeys collects the hashed remote keys // (subdirectories under metadata/) currently present in the // destination store. Returns (nil, err) when the store cannot be