From ca28469706d4eeb34868dafc2ef65bab869c7534 Mon Sep 17 00:00:00 2001 From: Jarek Kowalski Date: Wed, 13 May 2020 23:43:45 -0700 Subject: [PATCH] cli: improved 'snapshot delete' usage (#436) New usage: ``` kopia snapshot delete manifestID... [--delete] kopia snapshot delete rootObjectID... [--delete] ``` Fixes #435 cli: added --unsafe-ignore-source as alias for `--delete` This is a hidden flag for backwards compatibility. It will be removed. --- cli/command_snapshot_delete.go | 92 ++++--- snapshot/manager.go | 8 +- tests/end_to_end_test/snapshot_delete_test.go | 229 +++++------------- tests/tools/kopiarunner/kopia_snapshotter.go | 2 +- 4 files changed, 126 insertions(+), 205 deletions(-) diff --git a/cli/command_snapshot_delete.go b/cli/command_snapshot_delete.go index af5c33fc9..39996e806 100644 --- a/cli/command_snapshot_delete.go +++ b/cli/command_snapshot_delete.go @@ -2,66 +2,90 @@ import ( "context" + "fmt" + "strings" "github.com/pkg/errors" "github.com/kopia/kopia/repo" "github.com/kopia/kopia/repo/manifest" + "github.com/kopia/kopia/repo/object" + "github.com/kopia/kopia/snapshot" ) var ( - snapshotDeleteCommand = snapshotCommands.Command("delete", "Explicitly delete a snapshot by providing a snapshot ID.") - snapshotDeleteID = snapshotDeleteCommand.Arg("id", "Snapshot ID to be deleted").Required().String() - snapshotDeletePath = snapshotDeleteCommand.Flag("path", "Specify the path of the snapshot to be deleted").String() - snapshotDeleteHostname = snapshotDeleteCommand.Flag("hostname", "Specify the hostname of the snapshot to be deleted").String() - snapshotDeleteUsername = snapshotDeleteCommand.Flag("username", "Specify the username of the snapshot to be deleted").String() - snapshotDeleteIgnoreSource = snapshotDeleteCommand.Flag("unsafe-ignore-source", "Override the requirement to specify source info for the delete to succeed").Bool() + snapshotDeleteCommand = snapshotCommands.Command("delete", "Explicitly delete a snapshot by providing a snapshot ID.") + snapshotDeleteIDs = snapshotDeleteCommand.Arg("id", "Snapshot ID or root object ID to be deleted").Required().Strings() + snapshotDeleteConfirm = snapshotDeleteCommand.Flag("delete", "Confirm deletion").Bool() ) func runDeleteCommand(ctx context.Context, rep repo.Repository) error { - if !*snapshotDeleteIgnoreSource && *snapshotDeletePath == "" { - return errors.New("path is required") + for _, id := range *snapshotDeleteIDs { + if strings.HasPrefix(id, "k") { + if err := deleteSnapshotsByRootObjectID(ctx, rep, object.ID(id)); err != nil { + return errors.Wrapf(err, "error deleting snapshots by root ID %v", id) + } + } else { + m, err := snapshot.LoadSnapshot(ctx, rep, manifest.ID(id)) + if err != nil { + return errors.Wrapf(err, "error loading snapshot %v", id) + } + + if err := deleteSnapshot(ctx, rep, m); err != nil { + return errors.Wrapf(err, "error deleting %v", id) + } + } } - manifestID := manifest.ID(*snapshotDeleteID) + return nil +} - manifestMeta, err := rep.GetManifest(ctx, manifestID, nil) +func deleteSnapshot(ctx context.Context, rep repo.Repository, m *snapshot.Manifest) error { + desc := fmt.Sprintf("snapshot %v of %v at %v", m.ID, m.Source, formatTimestamp(m.StartTime)) + + if !*snapshotDeleteConfirm { + printStderr("Would delete %v (pass --delete to confirm)\n", desc) + return nil + } + + printStderr("Deleting %v...\n", desc) + + return rep.DeleteManifest(ctx, m.ID) +} + +func deleteSnapshotsByRootObjectID(ctx context.Context, rep repo.Repository, rootID object.ID) error { + ids, err := snapshot.ListSnapshotManifests(ctx, rep, nil) if err != nil { - return err + return errors.Wrap(err, "error listing snapshot manifests") } - labels := manifestMeta.Labels - if labels["type"] != "snapshot" { - return errors.Errorf("snapshot ID provided (%v) did not reference a snapshot", manifestID) + manifests, err := snapshot.LoadSnapshots(ctx, rep, ids) + if err != nil { + return errors.Wrap(err, "error loading snapshot manifests") } - if !*snapshotDeleteIgnoreSource { - h := *snapshotDeleteHostname - if h == "" { - h = rep.Hostname() - } + cnt := 0 - if labels["hostname"] != h { - return errors.Errorf("host name does not match for deleting requested snapshot ID (got %q, expected %q)", h, labels["hostname"]) - } + for _, m := range manifests { + if m.RootObjectID() == rootID { + cnt++ - u := *snapshotDeleteUsername - if u == "" { - u = rep.Username() - } - - if labels["username"] != u { - return errors.Errorf("user name does not match for deleting requested snapshot ID (got %q, expected %q)", u, labels["username"]) - } - - if labels["path"] != *snapshotDeletePath { - return errors.New("path does not match for deleting requested snapshot ID") + if err := deleteSnapshot(ctx, rep, m); err != nil { + return errors.Wrap(err, "error deleting") + } } } - return rep.DeleteManifest(ctx, manifestID) + if cnt == 0 { + return errors.Errorf("no snapshots matched %v", rootID) + } + + return nil } func init() { snapshotDeleteCommand.Action(repositoryAction(runDeleteCommand)) + + // hidden flag for backwards compatibility + snapshotDeleteCommand.Flag("unsafe-ignore-source", "Alias for --delete").Hidden().BoolVar(snapshotDeleteConfirm) } diff --git a/snapshot/manager.go b/snapshot/manager.go index 51e980eb3..e44ce1a18 100644 --- a/snapshot/manager.go +++ b/snapshot/manager.go @@ -70,10 +70,16 @@ func ListSnapshots(ctx context.Context, rep repo.Repository, si SourceInfo) ([]* // LoadSnapshot loads and parses a snapshot with a given ID. func LoadSnapshot(ctx context.Context, rep repo.Repository, manifestID manifest.ID) (*Manifest, error) { sm := &Manifest{} - if _, err := rep.GetManifest(ctx, manifestID, sm); err != nil { + + em, err := rep.GetManifest(ctx, manifestID, sm) + if err != nil { return nil, errors.Wrap(err, "unable to find manifest entries") } + if em.Labels[manifest.TypeLabelKey] != ManifestType { + return nil, errors.Errorf("manifest is not a snapshot") + } + sm.ID = manifestID return sm, nil diff --git a/tests/end_to_end_test/snapshot_delete_test.go b/tests/end_to_end_test/snapshot_delete_test.go index f7ee339b3..128b58772 100644 --- a/tests/end_to_end_test/snapshot_delete_test.go +++ b/tests/end_to_end_test/snapshot_delete_test.go @@ -10,7 +10,7 @@ "github.com/kopia/kopia/tests/testenv" ) -type deleteArgMaker func(manifestID string, source testenv.SourceInfo) []string +type deleteArgMaker func(manifestID, objectID string, source testenv.SourceInfo) []string //nolint:funlen func TestSnapshotDelete(t *testing.T) { @@ -28,181 +28,73 @@ func TestSnapshotDelete(t *testing.T) { }{ { "Test manifest rm function", - func(manifestID string, source testenv.SourceInfo) []string { + func(manifestID, objectID string, source testenv.SourceInfo) []string { return []string{"manifest", "rm", manifestID} }, expectSuccess, }, { - "Specify all source values correctly", - func(manifestID string, source testenv.SourceInfo) []string { - return []string{"snapshot", "delete", manifestID, - "--hostname", source.Host, - "--username", source.User, - "--path", source.Path, - } - }, - expectSuccess, - }, - { - "Specify path and username, using default hostname", - func(manifestID string, source testenv.SourceInfo) []string { - return []string{"snapshot", "delete", manifestID, - "--username", source.User, - "--path", source.Path, - } - }, - expectSuccess, - }, - { - "Specify path and hostname, using default username", - func(manifestID string, source testenv.SourceInfo) []string { - return []string{"snapshot", "delete", manifestID, - "--hostname", source.Host, - "--path", source.Path, - } - }, - expectSuccess, - }, - { - "No source flags, with unsafe ignore source flag", - func(manifestID string, source testenv.SourceInfo) []string { - return []string{"snapshot", "delete", manifestID, - "--unsafe-ignore-source", - } - }, - expectSuccess, - }, - { - "Specify path only, using default username and hostname", - func(manifestID string, source testenv.SourceInfo) []string { - return []string{"snapshot", "delete", manifestID, - "--path", source.Path, - } - }, - expectSuccess, - }, - { - "Specify all source flags, incorrect host name", - func(manifestID string, source testenv.SourceInfo) []string { - return []string{"snapshot", "delete", manifestID, - "--hostname", "some-other-host", - "--username", source.User, - "--path", source.Path, - } - }, - expectFail, - }, - { - "Specify all source flags, incorrect user name", - func(manifestID string, source testenv.SourceInfo) []string { - return []string{"snapshot", "delete", manifestID, - "--hostname", source.Host, - "--username", "some-other-user", - "--path", source.Path, - } - }, - expectFail, - }, - { - "Specify all source flags, incorrect path", - func(manifestID string, source testenv.SourceInfo) []string { - return []string{"snapshot", "delete", manifestID, - "--hostname", source.Host, - "--username", source.User, - "--path", "some-wrong-path", - } - }, - expectFail, - }, - { - "Specify all source flags, incorrect hostname, ignore flag set", - func(manifestID string, source testenv.SourceInfo) []string { - return []string{"snapshot", "delete", manifestID, - "--unsafe-ignore-source", - "--hostname", "some-other-host", - "--username", source.User, - "--path", source.Path, - } - }, - expectSuccess, - }, - { - "Specify all source flags, incorrect username, ignore flag set", - func(manifestID string, source testenv.SourceInfo) []string { - return []string{"snapshot", "delete", manifestID, - "--hostname", source.Host, - "--username", "some-other-user", - "--unsafe-ignore-source", - "--path", source.Path, - } - }, - expectSuccess, - }, - { - "Specify all source flags, incorrect path, ignore flag set", - func(manifestID string, source testenv.SourceInfo) []string { - return []string{"snapshot", "delete", manifestID, - "--hostname", source.Host, - "--username", source.User, - "--path", "some-wrong-path", - "--unsafe-ignore-source", - } - }, - expectSuccess, - }, - { - "No manifest ID provided", - func(manifestID string, source testenv.SourceInfo) []string { - return []string{"snapshot", "delete"} - }, - expectFail, - }, - { - "No manifest ID provided, ignore source flag set", - func(manifestID string, source testenv.SourceInfo) []string { - return []string{"snapshot", "delete", - "--unsafe-ignore-source", - } - }, - expectFail, - }, - { - "Garbage manifest ID provided", - func(manifestID string, source testenv.SourceInfo) []string { - return []string{"snapshot", "delete", "some-garbage-manifestID"} - }, - expectFail, - }, - { - "Hostname flag provided but no value input", - func(manifestID string, source testenv.SourceInfo) []string { - return []string{"snapshot", "delete", manifestID, - "--hostname", - "--username", source.User, - "--path", source.Path, - } - }, - expectFail, - }, - { - "No path provided and no unsafe ignore source flag provided", - func(manifestID string, source testenv.SourceInfo) []string { + "Dry run - by manifest ID", + func(manifestID, objectID string, source testenv.SourceInfo) []string { return []string{"snapshot", "delete", manifestID} }, - expectFail, + expectSuccess, }, { - "Specify hostname and username with no path provided", - func(manifestID string, source testenv.SourceInfo) []string { - return []string{"snapshot", "delete", manifestID, - "--hostname", source.Host, - "--username", source.User, - } + "Delete - by manifest ID", + func(manifestID, objectID string, source testenv.SourceInfo) []string { + return []string{"snapshot", "delete", manifestID, "--delete"} + }, + expectSuccess, + }, + { + "Delete - by manifest ID - legacy flag", + func(manifestID, objectID string, source testenv.SourceInfo) []string { + return []string{"snapshot", "delete", manifestID, "--unsafe-ignore-source"} + }, + expectSuccess, + }, + { + "Dry run - by objectID ID", + func(manifestID, objectID string, source testenv.SourceInfo) []string { + return []string{"snapshot", "delete", objectID} + }, + expectSuccess, + }, + { + "Delete - by object ID", + func(manifestID, objectID string, source testenv.SourceInfo) []string { + return []string{"snapshot", "delete", objectID, "--delete"} + }, + expectSuccess, + }, { + "Dry run - invalid object ID", + func(manifestID, objectID string, source testenv.SourceInfo) []string { + return []string{"snapshot", "delete", "no-such-manifest"} }, expectFail, }, - } { + { + "Delete - invalid manifest ID", + func(manifestID, objectID string, source testenv.SourceInfo) []string { + return []string{"snapshot", "delete", "no-such-manifest", "--delete"} + }, + expectFail, + }, + { + "Dry run - invalid object ID", + func(manifestID, objectID string, source testenv.SourceInfo) []string { + return []string{"snapshot", "delete", "k001122"} + }, + expectFail, + }, + { + "Delete - invalid object ID", + func(manifestID, objectID string, source testenv.SourceInfo) []string { + return []string{"snapshot", "delete", "k001122", "--delete"} + }, + expectFail, + }} { t.Log(tc.desc) testSnapshotDelete(t, tc.mf, tc.expectSuccess) } @@ -230,8 +122,7 @@ func testSnapshotDelete(t *testing.T, argMaker deleteArgMaker, expectDeleteSucce for _, source := range si { for _, ss := range source.Snapshots { manifestID := ss.SnapshotID - args := argMaker(manifestID, source) - t.Logf("manifestID: %v", manifestID) + args := argMaker(manifestID, ss.ObjectID, source) if expectDeleteSucceeds { e.RunAndExpectSuccess(t, args...) @@ -270,7 +161,7 @@ func TestSnapshotDeleteTypeCheck(t *testing.T) { t.Fatalf("Expected global policy manifest on a fresh repo") } - e.RunAndExpectFailure(t, "snapshot", "delete", manifestID, "--unsafe-ignore-source") + e.RunAndExpectFailure(t, "snapshot", "delete", manifestID) } } @@ -317,10 +208,10 @@ func TestSnapshotDeleteRestore(t *testing.T) { compareDirs(t, source, restoreDir) // snapshot delete should succeed - e.RunAndExpectSuccess(t, "snapshot", "delete", snapID, "--unsafe-ignore-source") + e.RunAndExpectSuccess(t, "snapshot", "delete", snapID, "--delete") // Subsequent snapshot delete to the same ID should fail - e.RunAndExpectFailure(t, "snapshot", "delete", snapID, "--unsafe-ignore-source") + e.RunAndExpectFailure(t, "snapshot", "delete", snapID, "--delete") // garbage-collect to clean up the root object. Otherwise // a restore will succeed diff --git a/tests/tools/kopiarunner/kopia_snapshotter.go b/tests/tools/kopiarunner/kopia_snapshotter.go index dcda5e123..64eb3a2fa 100644 --- a/tests/tools/kopiarunner/kopia_snapshotter.go +++ b/tests/tools/kopiarunner/kopia_snapshotter.go @@ -100,7 +100,7 @@ func (ks *KopiaSnapshotter) RestoreSnapshot(snapID, restoreDir string) (err erro // DeleteSnapshot implements the Snapshotter interface, issues a kopia snapshot // delete of the provided snapshot ID func (ks *KopiaSnapshotter) DeleteSnapshot(snapID string) (err error) { - _, _, err = ks.Runner.Run("snapshot", "delete", snapID, "--unsafe-ignore-source") + _, _, err = ks.Runner.Run("snapshot", "delete", snapID, "--delete") return err }