Refactor Filesystem into Manager and FilesystemSink

As we are required to support not only a filesystem storage
for the backups current implementation needs to be refactored.
The core logic of backup creation and restoring is untouched
except the operations working with data streams. The new
Sink interface is used to store and retrieve backup data.
Current functionality that allows to store/retrieve repositories
to/from the local filesystem now part of the FilesystemSink
type. And it is used as Sink implementation for the Manager.
The Manager doesn't know anything about underling storage
and just uses methods defined on the interface.

Part of: https://gitlab.com/gitlab-org/gitaly/-/issues/3633

6 files changed, 113 insertions, 99 deletions

diff --git a/cmd/gitaly-backup/create.go b/cmd/gitaly-backup/create.go
index 8e2d1e25d..e4d99a7b7 100644
--- a/cmd/gitaly-backup/create.go
+++ b/cmd/gitaly-backup/create.go
@@ -34,7 +34,7 @@ func (cmd *createSubcommand) Flags(fs *flag.FlagSet) {
 }
 
 func (cmd *createSubcommand) Run(ctx context.Context, stdin io.Reader, stdout io.Writer) error {
-	fsBackup := backup.NewFilesystem(cmd.backupPath)
+	fsBackup := backup.NewManager(backup.NewFilesystemSink(cmd.backupPath))
 
 	var pipeline backup.CreatePipeline
 	pipeline = backup.NewPipeline(log.StandardLogger(), fsBackup)
diff --git a/cmd/gitaly-backup/restore.go b/cmd/gitaly-backup/restore.go
index dffbc0a20..c24dc8e25 100644
--- a/cmd/gitaly-backup/restore.go
+++ b/cmd/gitaly-backup/restore.go
@@ -31,7 +31,7 @@ func (cmd *restoreSubcommand) Flags(fs *flag.FlagSet) {
 }
 
 func (cmd *restoreSubcommand) Run(ctx context.Context, stdin io.Reader, stdout io.Writer) error {
-	fsBackup := backup.NewFilesystem(cmd.backupPath)
+	fsBackup := backup.NewManager(backup.NewFilesystemSink(cmd.backupPath))
 	pipeline := backup.NewPipeline(log.StandardLogger(), fsBackup)
 
 	decoder := json.NewDecoder(stdin)
diff --git a/internal/backup/backup.go b/internal/backup/backup.go
index 2bcd2d57c..288ec7b8a 100644
--- a/internal/backup/backup.go
+++ b/internal/backup/backup.go
@@ -5,7 +5,6 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"os"
 	"path/filepath"
 	"strings"
 
@@ -19,20 +18,30 @@ import (
 
 var (
 	// ErrSkipped means the repository was skipped because there was nothing to backup
-	ErrSkipped     = errors.New("repository skipped")
+	ErrSkipped = errors.New("repository skipped")
+	// ErrDoesntExist means that the data was not found.
 	ErrDoesntExist = errors.New("doesn't exist")
 )
 
-// Filesystem strategy for creating and restoring backups
-type Filesystem struct {
-	path  string
+// Sink is an abstraction over the real storage used for storing/restoring backups.
+type Sink interface {
+	// Write saves all the data from the r by relativePath.
+	Write(ctx context.Context, relativePath string, r io.Reader) error
+	// GetReader returns a reader that servers the data stored by relativePath.
+	// If relativePath doesn't exists the ErrDoesntExist will be returned.
+	GetReader(ctx context.Context, relativePath string) (io.ReadCloser, error)
+}
+
+// Manager manages process of the creating/restoring backups.
+type Manager struct {
+	sink  Sink
 	conns *client.Pool
 }
 
-// NewFilesystem creates a new Filesystem strategy
-func NewFilesystem(path string) *Filesystem {
-	return &Filesystem{
-		path:  path,
+// NewManager creates and returns initialized *Manager instance.
+func NewManager(sink Sink) *Manager {
+	return &Manager{
+		sink:  sink,
 		conns: client.NewPool(),
 	}
 }
@@ -43,26 +52,23 @@ type CreateRequest struct {
 	Repository *gitalypb.Repository
 }
 
-// Create creates a repository backup on a local filesystem
-func (fs *Filesystem) Create(ctx context.Context, req *CreateRequest) error {
-	if isEmpty, err := fs.isEmpty(ctx, req.Server, req.Repository); err != nil {
-		return fmt.Errorf("filesystem: %w", err)
+// Create creates a repository backup.
+func (mgr *Manager) Create(ctx context.Context, req *CreateRequest) error {
+	if isEmpty, err := mgr.isEmpty(ctx, req.Server, req.Repository); err != nil {
+		return fmt.Errorf("manager: %w", err)
 	} else if isEmpty {
 		return ErrSkipped
 	}
 
-	backupPath := strings.TrimSuffix(filepath.Join(fs.path, req.Repository.RelativePath), ".git")
+	backupPath := strings.TrimSuffix(req.Repository.RelativePath, ".git")
 	bundlePath := backupPath + ".bundle"
 	customHooksPath := filepath.Join(backupPath, "custom_hooks.tar")
 
-	if err := os.MkdirAll(backupPath, 0700); err != nil {
-		return fmt.Errorf("filesystem: %w", err)
-	}
-	if err := fs.writeBundle(ctx, bundlePath, req.Server, req.Repository); err != nil {
-		return fmt.Errorf("filesystem: write bundle: %w", err)
+	if err := mgr.writeBundle(ctx, bundlePath, req.Server, req.Repository); err != nil {
+		return fmt.Errorf("manager: write bundle: %w", err)
 	}
-	if err := fs.writeCustomHooks(ctx, customHooksPath, req.Server, req.Repository); err != nil {
-		return fmt.Errorf("filesystem: write custom hooks: %w", err)
+	if err := mgr.writeCustomHooks(ctx, customHooksPath, req.Server, req.Repository); err != nil {
+		return fmt.Errorf("manager: write custom hooks: %w", err)
 	}
 
 	return nil
@@ -75,37 +81,37 @@ type RestoreRequest struct {
 	AlwaysCreate bool
 }
 
-// Restore restores a repository from a backup on a local filesystem
-func (fs *Filesystem) Restore(ctx context.Context, req *RestoreRequest) error {
-	backupPath := strings.TrimSuffix(filepath.Join(fs.path, req.Repository.RelativePath), ".git")
+// Restore restores a repository from a backup.
+func (mgr *Manager) Restore(ctx context.Context, req *RestoreRequest) error {
+	backupPath := strings.TrimSuffix(req.Repository.RelativePath, ".git")
 	bundlePath := backupPath + ".bundle"
 	customHooksPath := filepath.Join(backupPath, "custom_hooks.tar")
 
-	if err := fs.removeRepository(ctx, req.Server, req.Repository); err != nil {
-		return fmt.Errorf("filesystem: %w", err)
+	if err := mgr.removeRepository(ctx, req.Server, req.Repository); err != nil {
+		return fmt.Errorf("manager: %w", err)
 	}
-	if err := fs.restoreBundle(ctx, bundlePath, req.Server, req.Repository); err != nil {
+	if err := mgr.restoreBundle(ctx, bundlePath, req.Server, req.Repository); err != nil {
 		// For compatibility with existing backups we need to always create the
 		// repository even if there's no bundle for project repositories
 		// (not wiki or snippet repositories).  Gitaly does not know which
 		// repository is which type so here we accept a parameter to tell us
 		// to employ this behaviour.
 		if req.AlwaysCreate && errors.Is(err, ErrSkipped) {
-			if err := fs.createRepository(ctx, req.Server, req.Repository); err != nil {
-				return fmt.Errorf("filesystem: %w", err)
+			if err := mgr.createRepository(ctx, req.Server, req.Repository); err != nil {
+				return fmt.Errorf("manager: %w", err)
 			}
 		} else {
-			return fmt.Errorf("filesystem: %w", err)
+			return fmt.Errorf("manager: %w", err)
 		}
 	}
-	if err := fs.restoreCustomHooks(ctx, customHooksPath, req.Server, req.Repository); err != nil {
-		return fmt.Errorf("filesystem: %w", err)
+	if err := mgr.restoreCustomHooks(ctx, customHooksPath, req.Server, req.Repository); err != nil {
+		return fmt.Errorf("manager: %w", err)
 	}
 	return nil
 }
 
-func (fs *Filesystem) isEmpty(ctx context.Context, server storage.ServerInfo, repo *gitalypb.Repository) (bool, error) {
-	repoClient, err := fs.newRepoClient(ctx, server)
+func (mgr *Manager) isEmpty(ctx context.Context, server storage.ServerInfo, repo *gitalypb.Repository) (bool, error) {
+	repoClient, err := mgr.newRepoClient(ctx, server)
 	if err != nil {
 		return false, fmt.Errorf("isEmpty: %w", err)
 	}
@@ -119,8 +125,8 @@ func (fs *Filesystem) isEmpty(ctx context.Context, server storage.ServerInfo, re
 	return !hasLocalBranches.GetValue(), nil
 }
 
-func (fs *Filesystem) removeRepository(ctx context.Context, server storage.ServerInfo, repo *gitalypb.Repository) error {
-	repoClient, err := fs.newRepoClient(ctx, server)
+func (mgr *Manager) removeRepository(ctx context.Context, server storage.ServerInfo, repo *gitalypb.Repository) error {
+	repoClient, err := mgr.newRepoClient(ctx, server)
 	if err != nil {
 		return fmt.Errorf("remove repository: %w", err)
 	}
@@ -130,8 +136,8 @@ func (fs *Filesystem) removeRepository(ctx context.Context, server storage.Serve
 	return nil
 }
 
-func (fs *Filesystem) createRepository(ctx context.Context, server storage.ServerInfo, repo *gitalypb.Repository) error {
-	repoClient, err := fs.newRepoClient(ctx, server)
+func (mgr *Manager) createRepository(ctx context.Context, server storage.ServerInfo, repo *gitalypb.Repository) error {
+	repoClient, err := mgr.newRepoClient(ctx, server)
 	if err != nil {
 		return fmt.Errorf("create repository: %w", err)
 	}
@@ -141,8 +147,8 @@ func (fs *Filesystem) createRepository(ctx context.Context, server storage.Serve
 	return nil
 }
 
-func (fs *Filesystem) writeBundle(ctx context.Context, path string, server storage.ServerInfo, repo *gitalypb.Repository) error {
-	repoClient, err := fs.newRepoClient(ctx, server)
+func (mgr *Manager) writeBundle(ctx context.Context, path string, server storage.ServerInfo, repo *gitalypb.Repository) error {
+	repoClient, err := mgr.newRepoClient(ctx, server)
 	if err != nil {
 		return err
 	}
@@ -154,20 +160,24 @@ func (fs *Filesystem) writeBundle(ctx context.Context, path string, server stora
 		resp, err := stream.Recv()
 		return resp.GetData(), err
 	})
-	return writeFile(path, bundle)
+
+	if err := mgr.sink.Write(ctx, path, bundle); err != nil {
+		return fmt.Errorf("%T write: %w", mgr.sink, err)
+	}
+	return nil
 }
 
-func (fs *Filesystem) restoreBundle(ctx context.Context, path string, server storage.ServerInfo, repo *gitalypb.Repository) error {
-	f, err := os.Open(path)
+func (mgr *Manager) restoreBundle(ctx context.Context, path string, server storage.ServerInfo, repo *gitalypb.Repository) error {
+	reader, err := mgr.sink.GetReader(ctx, path)
 	if err != nil {
-		if os.IsNotExist(err) {
+		if errors.Is(err, ErrDoesntExist) {
 			return fmt.Errorf("%w: bundle does not exist: %q", ErrSkipped, path)
 		}
 		return fmt.Errorf("restore bundle: %w", err)
 	}
-	defer f.Close()
+	defer reader.Close()
 
-	repoClient, err := fs.newRepoClient(ctx, server)
+	repoClient, err := mgr.newRepoClient(ctx, server)
 	if err != nil {
 		return fmt.Errorf("restore bundle: %q: %w", path, err)
 	}
@@ -187,7 +197,7 @@ func (fs *Filesystem) restoreBundle(ctx context.Context, path string, server sto
 
 		return nil
 	})
-	if _, err := io.Copy(bundle, f); err != nil {
+	if _, err := io.Copy(bundle, reader); err != nil {
 		return fmt.Errorf("restore bundle: %q: %w", path, err)
 	}
 	if _, err = stream.CloseAndRecv(); err != nil {
@@ -196,8 +206,8 @@ func (fs *Filesystem) restoreBundle(ctx context.Context, path string, server sto
 	return nil
 }
 
-func (fs *Filesystem) writeCustomHooks(ctx context.Context, path string, server storage.ServerInfo, repo *gitalypb.Repository) error {
-	repoClient, err := fs.newRepoClient(ctx, server)
+func (mgr *Manager) writeCustomHooks(ctx context.Context, path string, server storage.ServerInfo, repo *gitalypb.Repository) error {
+	repoClient, err := mgr.newRepoClient(ctx, server)
 	if err != nil {
 		return err
 	}
@@ -209,23 +219,23 @@ func (fs *Filesystem) writeCustomHooks(ctx context.Context, path string, server
 		resp, err := stream.Recv()
 		return resp.GetData(), err
 	})
-	if err := writeFile(path, hooks); err != nil {
-		return err
+	if err := mgr.sink.Write(ctx, path, hooks); err != nil {
+		return fmt.Errorf("%T write: %w", mgr.sink, err)
 	}
 	return nil
 }
 
-func (fs *Filesystem) restoreCustomHooks(ctx context.Context, path string, server storage.ServerInfo, repo *gitalypb.Repository) error {
-	f, err := os.Open(path)
+func (mgr *Manager) restoreCustomHooks(ctx context.Context, path string, server storage.ServerInfo, repo *gitalypb.Repository) error {
+	reader, err := mgr.sink.GetReader(ctx, path)
 	if err != nil {
-		if os.IsNotExist(err) {
+		if errors.Is(err, ErrDoesntExist) {
 			return nil
 		}
 		return fmt.Errorf("restore custom hooks: %w", err)
 	}
-	defer f.Close()
+	defer reader.Close()
 
-	repoClient, err := fs.newRepoClient(ctx, server)
+	repoClient, err := mgr.newRepoClient(ctx, server)
 	if err != nil {
 		return fmt.Errorf("restore custom hooks, %q: %w", path, err)
 	}
@@ -246,7 +256,7 @@ func (fs *Filesystem) restoreCustomHooks(ctx context.Context, path string, serve
 
 		return nil
 	})
-	if _, err := io.Copy(bundle, f); err != nil {
+	if _, err := io.Copy(bundle, reader); err != nil {
 		return fmt.Errorf("restore custom hooks, %q: %w", path, err)
 	}
 	if _, err = stream.CloseAndRecv(); err != nil {
@@ -255,35 +265,11 @@ func (fs *Filesystem) restoreCustomHooks(ctx context.Context, path string, serve
 	return nil
 }
 
-func (fs *Filesystem) newRepoClient(ctx context.Context, server storage.ServerInfo) (gitalypb.RepositoryServiceClient, error) {
-	conn, err := fs.conns.Dial(ctx, server.Address, server.Token)
+func (mgr *Manager) newRepoClient(ctx context.Context, server storage.ServerInfo) (gitalypb.RepositoryServiceClient, error) {
+	conn, err := mgr.conns.Dial(ctx, server.Address, server.Token)
 	if err != nil {
 		return nil, err
 	}
 
 	return gitalypb.NewRepositoryServiceClient(conn), nil
 }
-
-func writeFile(path string, r io.Reader) (returnErr error) {
-	f, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0600)
-	if err != nil {
-		return fmt.Errorf("write file: %w", err)
-	}
-	defer func() {
-		if err := f.Close(); err != nil && returnErr == nil {
-			returnErr = fmt.Errorf("write file: %w", err)
-		}
-	}()
-	size, err := io.Copy(f, r)
-	if err != nil {
-		return fmt.Errorf("write file %q: %w", path, err)
-	}
-	if size == 0 {
-		// If the file is empty means that we received an empty stream, we delete the file
-		if err := os.Remove(path); err != nil {
-			return fmt.Errorf("write file: %w", err)
-		}
-		return nil
-	}
-	return nil
-}
diff --git a/internal/backup/backup_test.go b/internal/backup/backup_test.go
index dc01c94f1..0eb70eb47 100644
--- a/internal/backup/backup_test.go
+++ b/internal/backup/backup_test.go
@@ -18,7 +18,7 @@ import (
 	"google.golang.org/protobuf/proto"
 )
 
-func TestFilesystem_Create(t *testing.T) {
+func TestManager_Create(t *testing.T) {
 	cfg := testcfg.Build(t)
 
 	gitalyAddr := testserver.RunGitalyServer(t, cfg, nil, setup.RegisterAll)
@@ -76,7 +76,7 @@ func TestFilesystem_Create(t *testing.T) {
 			ctx, cancel := testhelper.Context()
 			defer cancel()
 
-			fsBackup := NewFilesystem(path)
+			fsBackup := NewManager(NewFilesystemSink(path))
 			err := fsBackup.Create(ctx, &CreateRequest{
 				Server:     storage.ServerInfo{Address: gitalyAddr, Token: cfg.Auth.Token},
 				Repository: tc.repo,
@@ -113,7 +113,7 @@ func TestFilesystem_Create(t *testing.T) {
 	}
 }
 
-func TestFilesystem_Restore(t *testing.T) {
+func TestManager_Restore(t *testing.T) {
 	cfg := testcfg.Build(t)
 	testhelper.ConfigureGitalyHooksBin(t, cfg)
 
@@ -178,7 +178,7 @@ func TestFilesystem_Restore(t *testing.T) {
 			ctx, cancel := testhelper.Context()
 			defer cancel()
 
-			fsBackup := NewFilesystem(path)
+			fsBackup := NewManager(NewFilesystemSink(path))
 			err := fsBackup.Restore(ctx, &RestoreRequest{
 				Server:       storage.ServerInfo{Address: gitalyAddr, Token: cfg.Auth.Token},
 				Repository:   tc.repo,
diff --git a/internal/backup/filesystem_sink.go b/internal/backup/filesystem_sink.go
index 03c188aff..cb97a6cef 100644
--- a/internal/backup/filesystem_sink.go
+++ b/internal/backup/filesystem_sink.go
@@ -1,6 +1,7 @@
 package backup
 
 import (
+	"context"
 	"errors"
 	"fmt"
 	"io"
@@ -8,14 +9,21 @@ import (
 	"path/filepath"
 )
 
+// FilesystemSink is a sink for creating and restoring backups from the local filesystem.
+type FilesystemSink struct {
+	path string
+}
+
 // NewFilesystemSink returns a sink that uses a local filesystem to work with data.
-func NewFilesystemSink(path string) *Filesystem {
-	return &Filesystem{
+func NewFilesystemSink(path string) *FilesystemSink {
+	return &FilesystemSink{
 		path: path,
 	}
 }
 
-func (fs *Filesystem) Write(relativePath string, r io.Reader) (returnErr error) {
+// Write creates required file structure and stored data from r into relativePath location.
+// If created file is empty it will be removed.
+func (fs *FilesystemSink) Write(ctx context.Context, relativePath string, r io.Reader) (returnErr error) {
 	path := filepath.Join(fs.path, relativePath)
 	dir := filepath.Dir(path)
 	if err := os.MkdirAll(dir, 0744); err != nil {
@@ -46,7 +54,8 @@ func (fs *Filesystem) Write(relativePath string, r io.Reader) (returnErr error)
 
 // GetReader returns a reader of the requested file path.
 // It's the caller's responsibility to Close returned reader once it is not needed anymore.
-func (fs *Filesystem) GetReader(relativePath string) (io.ReadCloser, error) {
+// If relativePath doesn't exist the ErrDoesntExist is returned.
+func (fs *FilesystemSink) GetReader(ctx context.Context, relativePath string) (io.ReadCloser, error) {
 	path := filepath.Join(fs.path, relativePath)
 	f, err := os.Open(path)
 	if err != nil {
diff --git a/internal/backup/filesystem_sink_test.go b/internal/backup/filesystem_sink_test.go
index 5a7da5557..c7a295da1 100644
--- a/internal/backup/filesystem_sink_test.go
+++ b/internal/backup/filesystem_sink_test.go
@@ -14,12 +14,15 @@ import (
 
 func TestFilesystemSink_GetReader(t *testing.T) {
 	t.Run("ok", func(t *testing.T) {
+		ctx, cancel := testhelper.Context()
+		defer cancel()
+
 		dir := testhelper.TempDir(t)
 		const relativePath = "test.dat"
 		require.NoError(t, ioutil.WriteFile(filepath.Join(dir, relativePath), []byte("test"), 0644))
 
 		fsSink := NewFilesystemSink(dir)
-		reader, err := fsSink.GetReader(relativePath)
+		reader, err := fsSink.GetReader(ctx, relativePath)
 		require.NoError(t, err)
 
 		defer func() { require.NoError(t, reader.Close()) }()
@@ -30,10 +33,14 @@ func TestFilesystemSink_GetReader(t *testing.T) {
 	})
 
 	t.Run("no file", func(t *testing.T) {
+		ctx, cancel := testhelper.Context()
+		defer cancel()
+
 		dir, err := os.Getwd()
 		require.NoError(t, err)
+
 		fsSink := NewFilesystemSink(dir)
-		reader, err := fsSink.GetReader("non-existing.dat")
+		reader, err := fsSink.GetReader(ctx, "non-existing.dat")
 		require.Equal(t, ErrDoesntExist, err)
 		require.Nil(t, reader)
 	})
@@ -41,11 +48,14 @@ func TestFilesystemSink_GetReader(t *testing.T) {
 
 func TestFilesystemSink_Write(t *testing.T) {
 	t.Run("ok", func(t *testing.T) {
+		ctx, cancel := testhelper.Context()
+		defer cancel()
+
 		dir := testhelper.TempDir(t)
 		const relativePath = "nested/dir/test.dat"
 
 		fsSink := NewFilesystemSink(dir)
-		require.NoError(t, fsSink.Write(relativePath, strings.NewReader("test")))
+		require.NoError(t, fsSink.Write(ctx, relativePath, strings.NewReader("test")))
 
 		require.FileExists(t, filepath.Join(dir, relativePath))
 		data, err := ioutil.ReadFile(filepath.Join(dir, relativePath))
@@ -54,16 +64,22 @@ func TestFilesystemSink_Write(t *testing.T) {
 	})
 
 	t.Run("no data doesn't create a file", func(t *testing.T) {
+		ctx, cancel := testhelper.Context()
+		defer cancel()
+
 		dir := testhelper.TempDir(t)
 		const relativePath = "nested/dir/test.dat"
 
 		fsSink := NewFilesystemSink(dir)
-		require.NoError(t, fsSink.Write(relativePath, strings.NewReader("")))
+		require.NoError(t, fsSink.Write(ctx, relativePath, strings.NewReader("")))
 
 		require.NoFileExists(t, filepath.Join(dir, relativePath))
 	})
 
 	t.Run("overrides existing data", func(t *testing.T) {
+		ctx, cancel := testhelper.Context()
+		defer cancel()
+
 		dir := testhelper.TempDir(t)
 		const relativePath = "nested/dir/test.dat"
 		fullPath := filepath.Join(dir, relativePath)
@@ -72,7 +88,7 @@ func TestFilesystemSink_Write(t *testing.T) {
 		require.NoError(t, ioutil.WriteFile(fullPath, []byte("initial"), 0655))
 
 		fsSink := NewFilesystemSink(dir)
-		require.NoError(t, fsSink.Write(relativePath, strings.NewReader("test")))
+		require.NoError(t, fsSink.Write(ctx, relativePath, strings.NewReader("test")))
 
 		require.FileExists(t, fullPath)
 		data, err := ioutil.ReadFile(fullPath)
@@ -81,12 +97,15 @@ func TestFilesystemSink_Write(t *testing.T) {
 	})
 
 	t.Run("dir creation error", func(t *testing.T) {
+		ctx, cancel := testhelper.Context()
+		defer cancel()
+
 		dir := testhelper.TempDir(t)
 		const relativePath = "nested/test.dat"
 		require.NoError(t, ioutil.WriteFile(filepath.Join(dir, "nested"), []byte("lock"), os.ModePerm))
 
 		fsSink := NewFilesystemSink(dir)
-		err := fsSink.Write(relativePath, strings.NewReader("test"))
+		err := fsSink.Write(ctx, relativePath, strings.NewReader("test"))
 		require.EqualError(t, err, fmt.Sprintf(`create directory structure %[1]q: mkdir %[1]s: not a directory`, filepath.Join(dir, "nested")))
 	})
 }