diff --git a/changelogs/unreleased/4817-yuvalman b/changelogs/unreleased/4817-yuvalman new file mode 100644 index 0000000000..fcacccaa06 --- /dev/null +++ b/changelogs/unreleased/4817-yuvalman @@ -0,0 +1 @@ +fix: delete empty backups diff --git a/internal/delete/delete_item_action_handler.go b/internal/delete/delete_item_action_handler.go index 609f759141..b08c8e024e 100644 --- a/internal/delete/delete_item_action_handler.go +++ b/internal/delete/delete_item_action_handler.go @@ -62,16 +62,17 @@ func InvokeDeleteActions(ctx *Context) error { dir, err := archive.NewExtractor(ctx.Log, ctx.Filesystem).UnzipAndExtractBackup(ctx.BackupReader) if err != nil { return errors.Wrapf(err, "error extracting backup") - } defer ctx.Filesystem.RemoveAll(dir) ctx.Log.Debugf("Downloaded and extracted the backup file to: %s", dir) backupResources, err := archive.NewParser(ctx.Log, ctx.Filesystem).Parse(dir) - if err != nil { + if existErr := errors.Is(err, archive.ErrNotExist); existErr { + ctx.Log.Debug("ignore invoking delete item actions: ", err) + return nil + } else if err != nil { return errors.Wrapf(err, "error parsing backup %q", dir) } - processdResources := sets.NewString() for resource := range backupResources { diff --git a/internal/delete/delete_item_action_handler_test.go b/internal/delete/delete_item_action_handler_test.go index d7e5d153d7..8bd1206020 100644 --- a/internal/delete/delete_item_action_handler_test.go +++ b/internal/delete/delete_item_action_handler_test.go @@ -138,6 +138,17 @@ func TestInvokeDeleteItemActionsRunForCorrectItems(t *testing.T) { new(recordResourcesAction).ForLabelSelector("app=app1"): {"ns-1/pod-1", "ns-2/pvc-2"}, }, }, + { + name: "success if resources dir does not exist", + backup: builder.ForBackup("velero", "velero").Result(), + tarball: test.NewTarWriter(t). + Done(), + apiResources: []*test.APIResource{test.Pods(), test.PVCs()}, + actions: map[*recordResourcesAction][]string{ + new(recordResourcesAction).ForNamespace("ns-1").ForResource("persistentvolumeclaims"): nil, + new(recordResourcesAction).ForNamespace("ns-2").ForResource("pods"): nil, + }, + }, } for _, tc := range tests { @@ -149,7 +160,7 @@ func TestInvokeDeleteItemActionsRunForCorrectItems(t *testing.T) { } // Get the plugins out of the map in order to use them. - actions := []velero.DeleteItemAction{} + var actions []velero.DeleteItemAction for action := range tc.actions { actions = append(actions, action) } diff --git a/pkg/archive/parser.go b/pkg/archive/parser.go index beb0754f1f..166e03114d 100644 --- a/pkg/archive/parser.go +++ b/pkg/archive/parser.go @@ -29,6 +29,8 @@ import ( "github.com/vmware-tanzu/velero/pkg/util/filesystem" ) +var ErrNotExist = errors.New("does not exist") + // Parser traverses an extracted archive on disk to validate // it and provide a helpful representation of it to consumers. type Parser struct { @@ -66,17 +68,9 @@ func (p *Parser) Parse(dir string) (map[string]*ResourceItems, error) { // ensure top-level "resources" directory exists, and read subdirectories // of it, where each one is expected to correspond to a resource. resourcesDir := filepath.Join(dir, velerov1api.ResourcesDir) - exists, err := p.fs.DirExists(resourcesDir) + resourceDirs, err := p.checkAndReadDir(resourcesDir) if err != nil { - return nil, errors.Wrapf(err, "error checking for existence of directory %q", strings.TrimPrefix(resourcesDir, dir+"/")) - } - if !exists { - return nil, errors.Errorf("directory %q does not exist", strings.TrimPrefix(resourcesDir, dir+"/")) - } - - resourceDirs, err := p.fs.ReadDir(resourcesDir) - if err != nil { - return nil, errors.Wrapf(err, "error reading contents of directory %q", strings.TrimPrefix(resourcesDir, dir+"/")) + return nil, err } // loop through each subdirectory (one per resource) and assemble @@ -173,15 +167,15 @@ func (p *Parser) getResourceItemsForScope(dir, archiveRootDir string) ([]string, func (p *Parser) checkAndReadDir(dir string) ([]os.FileInfo, error) { exists, err := p.fs.DirExists(dir) if err != nil { - return []os.FileInfo{}, errors.Wrapf(err, "finding %q", dir) + return nil, errors.Wrapf(err, "error checking for existence of directory %q", filepath.ToSlash(dir)) } if !exists { - return []os.FileInfo{}, errors.Errorf("%q not found", dir) + return nil, errors.Wrapf(ErrNotExist, "directory %q", filepath.ToSlash(dir)) } contents, err := p.fs.ReadDir(dir) if err != nil { - return []os.FileInfo{}, errors.Wrapf(err, "reading contents of %q", dir) + return nil, errors.Wrapf(err, "reading contents of %q", filepath.ToSlash(dir)) } return contents, nil diff --git a/pkg/archive/parser_test.go b/pkg/archive/parser_test.go index 49a2c327df..4a4049804b 100644 --- a/pkg/archive/parser_test.go +++ b/pkg/archive/parser_test.go @@ -32,13 +32,13 @@ func TestParse(t *testing.T) { name string files []string dir string - wantErrMsg string + wantErrMsg error want map[string]*ResourceItems }{ { name: "when there is no top-level resources directory, an error is returned", dir: "root-dir", - wantErrMsg: "directory \"resources\" does not exist", + wantErrMsg: ErrNotExist, }, { name: "when there are no directories under the resources directory, an empty map is returned", @@ -109,8 +109,8 @@ func TestParse(t *testing.T) { } res, err := p.Parse(tc.dir) - if tc.wantErrMsg != "" { - assert.EqualError(t, err, tc.wantErrMsg) + if tc.wantErrMsg != nil { + assert.ErrorIs(t, err, tc.wantErrMsg, "Error should be: %v, got: %v", tc.wantErrMsg, err) } else { assert.Nil(t, err) assert.Equal(t, tc.want, res) @@ -124,13 +124,13 @@ func TestParseGroupVersions(t *testing.T) { name string files []string backupDir string - wantErrMsg string + wantErrMsg error want map[string]metav1.APIGroup }{ { name: "when there is no top-level resources directory, an error is returned", backupDir: "/var/folders", - wantErrMsg: "\"/var/folders/resources\" not found", + wantErrMsg: ErrNotExist, }, { name: "when there are no directories under the resources directory, an empty map is returned", @@ -223,8 +223,8 @@ func TestParseGroupVersions(t *testing.T) { } res, err := p.ParseGroupVersions(tc.backupDir) - if tc.wantErrMsg != "" { - assert.EqualError(t, err, tc.wantErrMsg) + if tc.wantErrMsg != nil { + assert.ErrorIs(t, err, tc.wantErrMsg, "Error should be: %v, got: %v", tc.wantErrMsg, err) } else { assert.Nil(t, err) assert.Equal(t, tc.want, res) diff --git a/pkg/restore/restore_test.go b/pkg/restore/restore_test.go index a81676085c..b31c87f7ca 100644 --- a/pkg/restore/restore_test.go +++ b/pkg/restore/restore_test.go @@ -740,7 +740,7 @@ func TestInvalidTarballContents(t *testing.T) { tarball: test.NewTarWriter(t). Done(), wantErrs: Result{ - Velero: []string{"error parsing backup contents: directory \"resources\" does not exist"}, + Velero: []string{archive.ErrNotExist.Error()}, }, }, { @@ -761,7 +761,7 @@ func TestInvalidTarballContents(t *testing.T) { }, wantErrs: Result{ Namespaces: map[string][]string{ - "ns-1": {"error decoding \"resources/pods/namespaces/ns-1/pod-1.json\": invalid character 'i' looking for beginning of value"}, + "ns-1": {"error decoding"}, }, }, }, @@ -792,12 +792,34 @@ func TestInvalidTarballContents(t *testing.T) { ) assertEmptyResults(t, warnings) - assert.Equal(t, tc.wantErrs, errs) + assertWantErrs(t, tc.wantErrs, errs) assertAPIContents(t, h, tc.want) }) } } +func assertWantErrs(t *testing.T, wantErrRes Result, errRes Result) { + t.Helper() + if wantErrRes.Velero != nil { + assert.Equal(t, len(wantErrRes.Velero), len(errRes.Velero)) + for i := range errRes.Velero { + assert.Contains(t, errRes.Velero[i], wantErrRes.Velero[i]) + } + } + if wantErrRes.Namespaces != nil { + assert.Equal(t, len(wantErrRes.Namespaces), len(errRes.Namespaces)) + for ns := range errRes.Namespaces { + assert.Equal(t, len(wantErrRes.Namespaces[ns]), len(errRes.Namespaces[ns])) + for i := range errRes.Namespaces[ns] { + assert.Contains(t, errRes.Namespaces[ns][i], wantErrRes.Namespaces[ns][i]) + } + } + } + if wantErrRes.Cluster != nil { + assert.Equal(t, wantErrRes.Cluster, errRes.Cluster) + } +} + // TestRestoreItems runs restores of specific items and validates that they are created // with the expected metadata/spec/status in the API. func TestRestoreItems(t *testing.T) {