Merge pull request #4817 from yuvalman/deleteEmptyBackup

fix: delete empty backups

changelogs/unreleased/4817-yuvalman

@@ -0,0 +1 @@
+fix: delete empty backups

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 {

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)
 			}

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

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)

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) {