From 3a2b1a7e078a191cecd164e95b9928e85e5cf098 Mon Sep 17 00:00:00 2001 From: James Fargher Date: Tue, 19 Sep 2023 09:55:34 +1200 Subject: [PATCH 1/5] backup: Refactor locators to deal with entire backups Previously locators only needed the specific step being backed up, but soon we will be saving manifest files and this will require all steps such that they can be persisted. --- internal/backup/backup.go | 25 ++++---- internal/backup/locator.go | 57 +++++++++++-------- internal/backup/locator_test.go | 51 ++++++++++------- internal/backup/server_side_test.go | 5 +- .../repository/restore_repository_test.go | 10 ++-- 5 files changed, 90 insertions(+), 58 deletions(-) diff --git a/internal/backup/backup.go b/internal/backup/backup.go index a11aa4953d..9100891ca9 100644 --- a/internal/backup/backup.go +++ b/internal/backup/backup.go @@ -60,14 +60,17 @@ type Step struct { // Locator finds sink backup paths for repositories type Locator interface { - // BeginFull returns a tentative first step needed to create a new full backup. - BeginFull(ctx context.Context, repo *gitalypb.Repository, backupID string) *Step + // BeginFull returns the tentative backup paths needed to create a full backup. + BeginFull(ctx context.Context, repo *gitalypb.Repository, backupID string) *Backup - // BeginIncremental returns a tentative step needed to create a new incremental backup. - BeginIncremental(ctx context.Context, repo *gitalypb.Repository, backupID string) (*Step, error) + // BeginIncremental returns the backup with the last element of Steps being + // the tentative step needed to create an incremental backup. + BeginIncremental(ctx context.Context, repo *gitalypb.Repository, backupID string) (*Backup, error) - // Commit persists the step so that it can be looked up by FindLatest - Commit(ctx context.Context, step *Step) error + // Commit persists the backup so that it can be looked up by FindLatest. It + // is expected that the last element of Steps will be the newly created + // backup. + Commit(ctx context.Context, backup *Backup) error // FindLatest returns the latest backup that was written by Commit FindLatest(ctx context.Context, repo *gitalypb.Repository) (*Backup, error) @@ -197,15 +200,15 @@ func (mgr *Manager) Create(ctx context.Context, req *CreateRequest) error { return fmt.Errorf("manager: %w", err) } - var step *Step + var backup *Backup if req.Incremental { var err error - step, err = mgr.locator.BeginIncremental(ctx, req.VanityRepository, req.BackupID) + backup, err = mgr.locator.BeginIncremental(ctx, req.VanityRepository, req.BackupID) if err != nil { return fmt.Errorf("manager: %w", err) } } else { - step = mgr.locator.BeginFull(ctx, req.VanityRepository, req.BackupID) + backup = mgr.locator.BeginFull(ctx, req.VanityRepository, req.BackupID) } refs, err := repo.ListRefs(ctx) @@ -216,6 +219,8 @@ func (mgr *Manager) Create(ctx context.Context, req *CreateRequest) error { return fmt.Errorf("manager: %w", err) } + step := &backup.Steps[len(backup.Steps)-1] + if err := mgr.writeRefs(ctx, step.RefPath, refs); err != nil { return fmt.Errorf("manager: %w", err) } @@ -226,7 +231,7 @@ func (mgr *Manager) Create(ctx context.Context, req *CreateRequest) error { return fmt.Errorf("manager: %w", err) } - if err := mgr.locator.Commit(ctx, step); err != nil { + if err := mgr.locator.Commit(ctx, backup); err != nil { return fmt.Errorf("manager: %w", err) } diff --git a/internal/backup/locator.go b/internal/backup/locator.go index f2897f2e09..338db8a18a 100644 --- a/internal/backup/locator.go +++ b/internal/backup/locator.go @@ -28,28 +28,23 @@ import ( type LegacyLocator struct{} // BeginFull returns the static paths for a legacy repository backup -func (l LegacyLocator) BeginFull(ctx context.Context, repo *gitalypb.Repository, backupID string) *Step { +func (l LegacyLocator) BeginFull(ctx context.Context, repo *gitalypb.Repository, backupID string) *Backup { return l.newFull(repo) } // BeginIncremental is not supported for legacy backups -func (l LegacyLocator) BeginIncremental(ctx context.Context, repo *gitalypb.Repository, backupID string) (*Step, error) { +func (l LegacyLocator) BeginIncremental(ctx context.Context, repo *gitalypb.Repository, backupID string) (*Backup, error) { return nil, errors.New("legacy layout: begin incremental: not supported") } // Commit is unused as the locations are static -func (l LegacyLocator) Commit(ctx context.Context, full *Step) error { +func (l LegacyLocator) Commit(ctx context.Context, full *Backup) error { return nil } // FindLatest returns the static paths for a legacy repository backup func (l LegacyLocator) FindLatest(ctx context.Context, repo *gitalypb.Repository) (*Backup, error) { - return &Backup{ - Steps: []Step{ - *l.newFull(repo), - }, - ObjectFormat: git.ObjectHashSHA1.Format, - }, nil + return l.newFull(repo), nil } // Find is not supported for legacy backups. @@ -57,13 +52,18 @@ func (l LegacyLocator) Find(ctx context.Context, repo *gitalypb.Repository, back return nil, errors.New("legacy layout: find: not supported") } -func (l LegacyLocator) newFull(repo *gitalypb.Repository) *Step { +func (l LegacyLocator) newFull(repo *gitalypb.Repository) *Backup { backupPath := strings.TrimSuffix(repo.RelativePath, ".git") - return &Step{ - BundlePath: backupPath + ".bundle", - RefPath: backupPath + ".refs", - CustomHooksPath: filepath.Join(backupPath, "custom_hooks.tar"), + return &Backup{ + ObjectFormat: git.ObjectHashSHA1.Format, + Steps: []Step{ + { + BundlePath: backupPath + ".bundle", + RefPath: backupPath + ".refs", + CustomHooksPath: filepath.Join(backupPath, "custom_hooks.tar"), + }, + }, } } @@ -84,13 +84,18 @@ type PointerLocator struct { } // BeginFull returns a tentative first step needed to create a new full backup. -func (l PointerLocator) BeginFull(ctx context.Context, repo *gitalypb.Repository, backupID string) *Step { +func (l PointerLocator) BeginFull(ctx context.Context, repo *gitalypb.Repository, backupID string) *Backup { repoPath := strings.TrimSuffix(repo.RelativePath, ".git") - return &Step{ - BundlePath: filepath.Join(repoPath, backupID, "001.bundle"), - RefPath: filepath.Join(repoPath, backupID, "001.refs"), - CustomHooksPath: filepath.Join(repoPath, backupID, "001.custom_hooks.tar"), + return &Backup{ + ObjectFormat: git.ObjectHashSHA1.Format, + Steps: []Step{ + { + BundlePath: filepath.Join(repoPath, backupID, "001.bundle"), + RefPath: filepath.Join(repoPath, backupID, "001.refs"), + CustomHooksPath: filepath.Join(repoPath, backupID, "001.custom_hooks.tar"), + }, + }, } } @@ -98,7 +103,7 @@ func (l PointerLocator) BeginFull(ctx context.Context, repo *gitalypb.Repository // backup. The incremental backup is always based off of the latest full // backup. If there is no latest backup, a new full backup step is returned // using fallbackBackupID -func (l PointerLocator) BeginIncremental(ctx context.Context, repo *gitalypb.Repository, fallbackBackupID string) (*Step, error) { +func (l PointerLocator) BeginIncremental(ctx context.Context, repo *gitalypb.Repository, fallbackBackupID string) (*Backup, error) { repoPath := strings.TrimSuffix(repo.RelativePath, ".git") backupID, err := l.findLatestID(ctx, repoPath) if err != nil { @@ -125,16 +130,22 @@ func (l PointerLocator) BeginIncremental(ctx context.Context, repo *gitalypb.Rep } id++ - return &Step{ + backup.Steps = append(backup.Steps, Step{ BundlePath: filepath.Join(backupPath, fmt.Sprintf("%03d.bundle", id)), RefPath: filepath.Join(backupPath, fmt.Sprintf("%03d.refs", id)), PreviousRefPath: previous.RefPath, CustomHooksPath: filepath.Join(backupPath, fmt.Sprintf("%03d.custom_hooks.tar", id)), - }, nil + }) + + return backup, nil } // Commit persists the step so that it can be looked up by FindLatest -func (l PointerLocator) Commit(ctx context.Context, step *Step) error { +func (l PointerLocator) Commit(ctx context.Context, backup *Backup) error { + if len(backup.Steps) < 1 { + return fmt.Errorf("pointer locator: commit: no steps") + } + step := backup.Steps[len(backup.Steps)-1] backupPath := filepath.Dir(step.BundlePath) bundleName := filepath.Base(step.BundlePath) repoPath := filepath.Dir(backupPath) diff --git a/internal/backup/locator_test.go b/internal/backup/locator_test.go index c9f48fdf1e..ef40a393c6 100644 --- a/internal/backup/locator_test.go +++ b/internal/backup/locator_test.go @@ -33,10 +33,15 @@ func TestLegacyLocator(t *testing.T) { t.Run("Begin/Commit Full", func(t *testing.T) { t.Parallel() - expected := &Step{ - BundlePath: repo.RelativePath + ".bundle", - RefPath: repo.RelativePath + ".refs", - CustomHooksPath: filepath.Join(repo.RelativePath, "custom_hooks.tar"), + expected := &Backup{ + ObjectFormat: git.ObjectHashSHA1.Format, + Steps: []Step{ + { + BundlePath: repo.RelativePath + ".bundle", + RefPath: repo.RelativePath + ".refs", + CustomHooksPath: filepath.Join(repo.RelativePath, "custom_hooks.tar"), + }, + }, } full := l.BeginFull(ctx, repo, "abc123") @@ -89,10 +94,15 @@ func TestPointerLocator(t *testing.T) { } const expectedIncrement = "001" - expected := &Step{ - BundlePath: filepath.Join(repo.RelativePath, backupID, expectedIncrement+".bundle"), - RefPath: filepath.Join(repo.RelativePath, backupID, expectedIncrement+".refs"), - CustomHooksPath: filepath.Join(repo.RelativePath, backupID, expectedIncrement+".custom_hooks.tar"), + expected := &Backup{ + ObjectFormat: git.ObjectHashSHA1.Format, + Steps: []Step{ + { + BundlePath: filepath.Join(repo.RelativePath, backupID, expectedIncrement+".bundle"), + RefPath: filepath.Join(repo.RelativePath, backupID, expectedIncrement+".refs"), + CustomHooksPath: filepath.Join(repo.RelativePath, backupID, expectedIncrement+".custom_hooks.tar"), + }, + }, } full := l.BeginFull(ctx, repo, backupID) @@ -146,19 +156,22 @@ func TestPointerLocator(t *testing.T) { tc.setup(t, ctx, backupPath) } - var expected *Step + var expected *Backup for i := 1; i <= 3; i++ { - incrementID := i + tc.expectedOffset - var previousRefPath string - if incrementID > 1 { - previousRefPath = filepath.Join(repo.RelativePath, tc.expectedBackupID, fmt.Sprintf("%03d.refs", incrementID-1)) + var previousRefPath, expectedIncrement string + expected = &Backup{ + ObjectFormat: git.ObjectHashSHA1.Format, } - expectedIncrement := fmt.Sprintf("%03d", incrementID) - expected = &Step{ - BundlePath: filepath.Join(repo.RelativePath, tc.expectedBackupID, expectedIncrement+".bundle"), - RefPath: filepath.Join(repo.RelativePath, tc.expectedBackupID, expectedIncrement+".refs"), - PreviousRefPath: previousRefPath, - CustomHooksPath: filepath.Join(repo.RelativePath, tc.expectedBackupID, expectedIncrement+".custom_hooks.tar"), + for incrementID := 1; incrementID <= i+tc.expectedOffset; incrementID++ { + expectedIncrement = fmt.Sprintf("%03d", incrementID) + step := Step{ + BundlePath: filepath.Join(repo.RelativePath, tc.expectedBackupID, expectedIncrement+".bundle"), + RefPath: filepath.Join(repo.RelativePath, tc.expectedBackupID, expectedIncrement+".refs"), + PreviousRefPath: previousRefPath, + CustomHooksPath: filepath.Join(repo.RelativePath, tc.expectedBackupID, expectedIncrement+".custom_hooks.tar"), + } + expected.Steps = append(expected.Steps, step) + previousRefPath = step.RefPath } step, err := l.BeginIncremental(ctx, repo, fallbackBackupID) diff --git a/internal/backup/server_side_test.go b/internal/backup/server_side_test.go index 13d375d69c..b609184394 100644 --- a/internal/backup/server_side_test.go +++ b/internal/backup/server_side_test.go @@ -157,7 +157,8 @@ func TestServerSideAdapter_Restore(t *testing.T) { checksum := gittest.ChecksumRepo(t, cfg, templateRepoPath) repo, repoPath := gittest.CreateRepository(t, ctx, cfg) - step := backupLocator.BeginFull(ctx, repo, "abc123") + backup := backupLocator.BeginFull(ctx, repo, "abc123") + step := backup.Steps[len(backup.Steps)-1] w, err := backupSink.GetWriter(ctx, step.BundlePath) require.NoError(t, err) @@ -173,7 +174,7 @@ func TestServerSideAdapter_Restore(t *testing.T) { require.NoError(t, err) require.NoError(t, w.Close()) - require.NoError(t, backupLocator.Commit(ctx, step)) + require.NoError(t, backupLocator.Commit(ctx, backup)) return setupData{ repo: repo, diff --git a/internal/gitaly/service/repository/restore_repository_test.go b/internal/gitaly/service/repository/restore_repository_test.go index 5d5fbfbb08..a8b50c92dc 100644 --- a/internal/gitaly/service/repository/restore_repository_test.go +++ b/internal/gitaly/service/repository/restore_repository_test.go @@ -49,7 +49,8 @@ func TestRestoreRepository(t *testing.T) { checksum := gittest.ChecksumRepo(t, cfg, templateRepoPath) repo, repoPath := gittest.CreateRepository(t, ctx, cfg) - step := backupLocator.BeginFull(ctx, repo, "abc123") + backup := backupLocator.BeginFull(ctx, repo, "abc123") + step := backup.Steps[len(backup.Steps)-1] w, err := backupSink.GetWriter(ctx, step.BundlePath) require.NoError(t, err) @@ -65,7 +66,7 @@ func TestRestoreRepository(t *testing.T) { require.NoError(t, err) require.NoError(t, w.Close()) - require.NoError(t, backupLocator.Commit(ctx, step)) + require.NoError(t, backupLocator.Commit(ctx, backup)) return setupData{ cfg: cfg, @@ -91,7 +92,8 @@ func TestRestoreRepository(t *testing.T) { checksum := gittest.ChecksumRepo(t, cfg, templateRepoPath) repo, repoPath := gittest.CreateRepository(t, ctx, cfg) - step := backupLocator.BeginFull(ctx, repo, "abc123") + backup := backupLocator.BeginFull(ctx, repo, "abc123") + step := backup.Steps[len(backup.Steps)-1] w, err := backupSink.GetWriter(ctx, step.BundlePath) require.NoError(t, err) @@ -107,7 +109,7 @@ func TestRestoreRepository(t *testing.T) { require.NoError(t, err) require.NoError(t, w.Close()) - require.NoError(t, backupLocator.Commit(ctx, step)) + require.NoError(t, backupLocator.Commit(ctx, backup)) return setupData{ cfg: cfg, -- GitLab From 81b893f3dbfa315bd61fc336c879fa164c11c80f Mon Sep 17 00:00:00 2001 From: James Fargher Date: Wed, 20 Sep 2023 08:28:53 +1200 Subject: [PATCH 2/5] backup: Convert locator repos to Repository interface Locators only use repositories to determine paths and so do not need the concrete repository types. --- internal/backup/backup.go | 8 ++++---- internal/backup/locator.go | 32 ++++++++++++++++---------------- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/internal/backup/backup.go b/internal/backup/backup.go index 9100891ca9..e924e30810 100644 --- a/internal/backup/backup.go +++ b/internal/backup/backup.go @@ -61,11 +61,11 @@ type Step struct { // Locator finds sink backup paths for repositories type Locator interface { // BeginFull returns the tentative backup paths needed to create a full backup. - BeginFull(ctx context.Context, repo *gitalypb.Repository, backupID string) *Backup + BeginFull(ctx context.Context, repo storage.Repository, backupID string) *Backup // BeginIncremental returns the backup with the last element of Steps being // the tentative step needed to create an incremental backup. - BeginIncremental(ctx context.Context, repo *gitalypb.Repository, backupID string) (*Backup, error) + BeginIncremental(ctx context.Context, repo storage.Repository, backupID string) (*Backup, error) // Commit persists the backup so that it can be looked up by FindLatest. It // is expected that the last element of Steps will be the newly created @@ -73,11 +73,11 @@ type Locator interface { Commit(ctx context.Context, backup *Backup) error // FindLatest returns the latest backup that was written by Commit - FindLatest(ctx context.Context, repo *gitalypb.Repository) (*Backup, error) + FindLatest(ctx context.Context, repo storage.Repository) (*Backup, error) // Find returns the repository backup at the given backupID. If the backup does // not exist then the error ErrDoesntExist is returned. - Find(ctx context.Context, repo *gitalypb.Repository, backupID string) (*Backup, error) + Find(ctx context.Context, repo storage.Repository, backupID string) (*Backup, error) } // Repository abstracts git access required to make a repository backup diff --git a/internal/backup/locator.go b/internal/backup/locator.go index 338db8a18a..480e031a01 100644 --- a/internal/backup/locator.go +++ b/internal/backup/locator.go @@ -10,8 +10,8 @@ import ( "strings" "gitlab.com/gitlab-org/gitaly/v16/internal/git" + "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" "gitlab.com/gitlab-org/gitaly/v16/internal/helper/text" - "gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb" ) // LegacyLocator locates backup paths for historic backups. This is the @@ -28,12 +28,12 @@ import ( type LegacyLocator struct{} // BeginFull returns the static paths for a legacy repository backup -func (l LegacyLocator) BeginFull(ctx context.Context, repo *gitalypb.Repository, backupID string) *Backup { +func (l LegacyLocator) BeginFull(ctx context.Context, repo storage.Repository, backupID string) *Backup { return l.newFull(repo) } // BeginIncremental is not supported for legacy backups -func (l LegacyLocator) BeginIncremental(ctx context.Context, repo *gitalypb.Repository, backupID string) (*Backup, error) { +func (l LegacyLocator) BeginIncremental(ctx context.Context, repo storage.Repository, backupID string) (*Backup, error) { return nil, errors.New("legacy layout: begin incremental: not supported") } @@ -43,17 +43,17 @@ func (l LegacyLocator) Commit(ctx context.Context, full *Backup) error { } // FindLatest returns the static paths for a legacy repository backup -func (l LegacyLocator) FindLatest(ctx context.Context, repo *gitalypb.Repository) (*Backup, error) { +func (l LegacyLocator) FindLatest(ctx context.Context, repo storage.Repository) (*Backup, error) { return l.newFull(repo), nil } // Find is not supported for legacy backups. -func (l LegacyLocator) Find(ctx context.Context, repo *gitalypb.Repository, backupID string) (*Backup, error) { +func (l LegacyLocator) Find(ctx context.Context, repo storage.Repository, backupID string) (*Backup, error) { return nil, errors.New("legacy layout: find: not supported") } -func (l LegacyLocator) newFull(repo *gitalypb.Repository) *Backup { - backupPath := strings.TrimSuffix(repo.RelativePath, ".git") +func (l LegacyLocator) newFull(repo storage.Repository) *Backup { + backupPath := strings.TrimSuffix(repo.GetRelativePath(), ".git") return &Backup{ ObjectFormat: git.ObjectHashSHA1.Format, @@ -84,8 +84,8 @@ type PointerLocator struct { } // BeginFull returns a tentative first step needed to create a new full backup. -func (l PointerLocator) BeginFull(ctx context.Context, repo *gitalypb.Repository, backupID string) *Backup { - repoPath := strings.TrimSuffix(repo.RelativePath, ".git") +func (l PointerLocator) BeginFull(ctx context.Context, repo storage.Repository, backupID string) *Backup { + repoPath := strings.TrimSuffix(repo.GetRelativePath(), ".git") return &Backup{ ObjectFormat: git.ObjectHashSHA1.Format, @@ -103,8 +103,8 @@ func (l PointerLocator) BeginFull(ctx context.Context, repo *gitalypb.Repository // backup. The incremental backup is always based off of the latest full // backup. If there is no latest backup, a new full backup step is returned // using fallbackBackupID -func (l PointerLocator) BeginIncremental(ctx context.Context, repo *gitalypb.Repository, fallbackBackupID string) (*Backup, error) { - repoPath := strings.TrimSuffix(repo.RelativePath, ".git") +func (l PointerLocator) BeginIncremental(ctx context.Context, repo storage.Repository, fallbackBackupID string) (*Backup, error) { + repoPath := strings.TrimSuffix(repo.GetRelativePath(), ".git") backupID, err := l.findLatestID(ctx, repoPath) if err != nil { if errors.Is(err, ErrDoesntExist) { @@ -164,8 +164,8 @@ func (l PointerLocator) Commit(ctx context.Context, backup *Backup) error { // FindLatest returns the paths committed by the latest call to CommitFull. // // If there is no `LATEST` file, the result of the `Fallback` is used. -func (l PointerLocator) FindLatest(ctx context.Context, repo *gitalypb.Repository) (*Backup, error) { - repoPath := strings.TrimSuffix(repo.RelativePath, ".git") +func (l PointerLocator) FindLatest(ctx context.Context, repo storage.Repository) (*Backup, error) { + repoPath := strings.TrimSuffix(repo.GetRelativePath(), ".git") backupID, err := l.findLatestID(ctx, repoPath) if err != nil { @@ -184,7 +184,7 @@ func (l PointerLocator) FindLatest(ctx context.Context, repo *gitalypb.Repositor // Find returns the repository backup at the given backupID. If the backup does // not exist then the error ErrDoesntExist is returned. -func (l PointerLocator) Find(ctx context.Context, repo *gitalypb.Repository, backupID string) (*Backup, error) { +func (l PointerLocator) Find(ctx context.Context, repo storage.Repository, backupID string) (*Backup, error) { backup, err := l.find(ctx, repo, backupID) if err != nil { return nil, fmt.Errorf("pointer locator: %w", err) @@ -192,8 +192,8 @@ func (l PointerLocator) Find(ctx context.Context, repo *gitalypb.Repository, bac return backup, nil } -func (l PointerLocator) find(ctx context.Context, repo *gitalypb.Repository, backupID string) (*Backup, error) { - repoPath := strings.TrimSuffix(repo.RelativePath, ".git") +func (l PointerLocator) find(ctx context.Context, repo storage.Repository, backupID string) (*Backup, error) { + repoPath := strings.TrimSuffix(repo.GetRelativePath(), ".git") backupPath := filepath.Join(repoPath, backupID) latestIncrementID, err := l.findLatestID(ctx, backupPath) -- GitLab From 5ae014ada8dfa9679ef68e2226a7c40a97a0ec51 Mon Sep 17 00:00:00 2001 From: James Fargher Date: Wed, 20 Sep 2023 08:40:38 +1200 Subject: [PATCH 3/5] backup: Add more metadata to Backup struct The plan is to introduce a new locator wrapper that will write backup manifest files. In order to properly locate the manifest file, we need to add the backup ID and repository. --- internal/backup/backup.go | 4 ++++ internal/backup/locator.go | 6 ++++++ internal/backup/locator_test.go | 16 ++++++++++++++++ 3 files changed, 26 insertions(+) diff --git a/internal/backup/backup.go b/internal/backup/backup.go index e924e30810..cf07c6f61d 100644 --- a/internal/backup/backup.go +++ b/internal/backup/backup.go @@ -40,6 +40,10 @@ type Sink interface { // Backup represents all the information needed to restore a backup for a repository type Backup struct { + // ID is the identifier that uniquely identifies the backup for this repository. + ID string + // Repository is the repository being backed up. + Repository storage.Repository // Steps are the ordered list of steps required to restore this backup Steps []Step // ObjectFormat is the name of the object hash used by the repository. diff --git a/internal/backup/locator.go b/internal/backup/locator.go index 480e031a01..906f0b4405 100644 --- a/internal/backup/locator.go +++ b/internal/backup/locator.go @@ -56,6 +56,7 @@ func (l LegacyLocator) newFull(repo storage.Repository) *Backup { backupPath := strings.TrimSuffix(repo.GetRelativePath(), ".git") return &Backup{ + Repository: repo, ObjectFormat: git.ObjectHashSHA1.Format, Steps: []Step{ { @@ -88,6 +89,8 @@ func (l PointerLocator) BeginFull(ctx context.Context, repo storage.Repository, repoPath := strings.TrimSuffix(repo.GetRelativePath(), ".git") return &Backup{ + ID: backupID, + Repository: repo, ObjectFormat: git.ObjectHashSHA1.Format, Steps: []Step{ { @@ -130,6 +133,7 @@ func (l PointerLocator) BeginIncremental(ctx context.Context, repo storage.Repos } id++ + backup.ID = fallbackBackupID backup.Steps = append(backup.Steps, Step{ BundlePath: filepath.Join(backupPath, fmt.Sprintf("%03d.bundle", id)), RefPath: filepath.Join(backupPath, fmt.Sprintf("%03d.refs", id)), @@ -207,6 +211,8 @@ func (l PointerLocator) find(ctx context.Context, repo storage.Repository, backu } backup := Backup{ + ID: backupID, + Repository: repo, ObjectFormat: git.ObjectHashSHA1.Format, } diff --git a/internal/backup/locator_test.go b/internal/backup/locator_test.go index ef40a393c6..3f99da2409 100644 --- a/internal/backup/locator_test.go +++ b/internal/backup/locator_test.go @@ -34,6 +34,8 @@ func TestLegacyLocator(t *testing.T) { t.Parallel() expected := &Backup{ + ID: "", // legacy storage can only store a single backup. + Repository: repo, ObjectFormat: git.ObjectHashSHA1.Format, Steps: []Step{ { @@ -54,6 +56,8 @@ func TestLegacyLocator(t *testing.T) { t.Parallel() expected := &Backup{ + ID: "", // legacy storage can only store a single backup. + Repository: repo, ObjectFormat: git.ObjectHashSHA1.Format, Steps: []Step{ { @@ -95,6 +99,8 @@ func TestPointerLocator(t *testing.T) { const expectedIncrement = "001" expected := &Backup{ + ID: backupID, + Repository: repo, ObjectFormat: git.ObjectHashSHA1.Format, Steps: []Step{ { @@ -160,6 +166,8 @@ func TestPointerLocator(t *testing.T) { for i := 1; i <= 3; i++ { var previousRefPath, expectedIncrement string expected = &Backup{ + ID: fallbackBackupID, + Repository: repo, ObjectFormat: git.ObjectHashSHA1.Format, } for incrementID := 1; incrementID <= i+tc.expectedOffset; incrementID++ { @@ -208,6 +216,8 @@ func TestPointerLocator(t *testing.T) { require.NoError(t, os.WriteFile(filepath.Join(backupPath, repo.RelativePath, "LATEST"), []byte(backupID), perm.SharedFile)) require.NoError(t, os.WriteFile(filepath.Join(backupPath, repo.RelativePath, backupID, "LATEST"), []byte("003"), perm.SharedFile)) expected := &Backup{ + ID: backupID, + Repository: repo, ObjectFormat: git.ObjectHashSHA1.Format, Steps: []Step{ { @@ -245,6 +255,8 @@ func TestPointerLocator(t *testing.T) { } expectedFallback := &Backup{ + ID: "", + Repository: repo, ObjectFormat: git.ObjectHashSHA1.Format, Steps: []Step{ { @@ -263,6 +275,8 @@ func TestPointerLocator(t *testing.T) { require.NoError(t, os.WriteFile(filepath.Join(backupPath, repo.RelativePath, "LATEST"), []byte(backupID), perm.SharedFile)) require.NoError(t, os.WriteFile(filepath.Join(backupPath, repo.RelativePath, backupID, "LATEST"), []byte("001"), perm.SharedFile)) expected := &Backup{ + ID: backupID, + Repository: repo, ObjectFormat: git.ObjectHashSHA1.Format, Steps: []Step{ { @@ -341,6 +355,8 @@ func TestPointerLocator(t *testing.T) { require.NoError(t, os.MkdirAll(filepath.Join(backupPath, repo.RelativePath, backupID), perm.SharedDir)) require.NoError(t, os.WriteFile(filepath.Join(backupPath, repo.RelativePath, backupID, "LATEST"), []byte("003"), perm.SharedFile)) expected := &Backup{ + ID: backupID, + Repository: repo, ObjectFormat: git.ObjectHashSHA1.Format, Steps: []Step{ { -- GitLab From 3011dd4b79373766c480b1b92204e74fc4217b8f Mon Sep 17 00:00:00 2001 From: James Fargher Date: Wed, 20 Sep 2023 09:27:08 +1200 Subject: [PATCH 4/5] backup: Add manifest writing locator --- internal/backup/backup.go | 16 +++---- internal/backup/locator.go | 65 +++++++++++++++++++++++++++ internal/backup/locator_test.go | 80 +++++++++++++++++++++++++++++++++ 3 files changed, 153 insertions(+), 8 deletions(-) diff --git a/internal/backup/backup.go b/internal/backup/backup.go index cf07c6f61d..05e0b14ee4 100644 --- a/internal/backup/backup.go +++ b/internal/backup/backup.go @@ -41,25 +41,25 @@ type Sink interface { // Backup represents all the information needed to restore a backup for a repository type Backup struct { // ID is the identifier that uniquely identifies the backup for this repository. - ID string + ID string `toml:"-"` // Repository is the repository being backed up. - Repository storage.Repository + Repository storage.Repository `toml:"-"` // Steps are the ordered list of steps required to restore this backup - Steps []Step + Steps []Step `toml:"steps"` // ObjectFormat is the name of the object hash used by the repository. - ObjectFormat string + ObjectFormat string `toml:"object_format"` } // Step represents an incremental step that makes up a complete backup for a repository type Step struct { // BundlePath is the path of the bundle - BundlePath string + BundlePath string `toml:"bundle_path,omitempty"` // RefPath is the path of the ref file - RefPath string + RefPath string `toml:"ref_path,omitempty"` // PreviousRefPath is the path of the previous ref file - PreviousRefPath string + PreviousRefPath string `toml:"previous_ref_path,omitempty"` // CustomHooksPath is the path of the custom hooks archive - CustomHooksPath string + CustomHooksPath string `toml:"custom_hooks_path,omitempty"` } // Locator finds sink backup paths for repositories diff --git a/internal/backup/locator.go b/internal/backup/locator.go index 906f0b4405..da789011d1 100644 --- a/internal/backup/locator.go +++ b/internal/backup/locator.go @@ -5,10 +5,12 @@ import ( "errors" "fmt" "io" + "path" "path/filepath" "strconv" "strings" + "github.com/pelletier/go-toml/v2" "gitlab.com/gitlab-org/gitaly/v16/internal/git" "gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage" "gitlab.com/gitlab-org/gitaly/v16/internal/helper/text" @@ -264,3 +266,66 @@ func (l PointerLocator) writeLatest(ctx context.Context, path, target string) (r return nil } + +// ManifestLocator locates backup paths based on manifest files that are +// written to a predetermined path: +// +// manifests///.toml +// +// It relies on Fallback to determine paths of new backups. +type ManifestLocator struct { + Sink Sink + Fallback Locator +} + +// BeginFull passes through to Fallback +func (l ManifestLocator) BeginFull(ctx context.Context, repo storage.Repository, backupID string) *Backup { + return l.Fallback.BeginFull(ctx, repo, backupID) +} + +// BeginIncremental passes through to Fallback +func (l ManifestLocator) BeginIncremental(ctx context.Context, repo storage.Repository, backupID string) (*Backup, error) { + return l.Fallback.BeginIncremental(ctx, repo, backupID) +} + +// Commit passes through to Fallback, then writes a manifest file for the backup. +func (l ManifestLocator) Commit(ctx context.Context, backup *Backup) (returnErr error) { + if err := l.Fallback.Commit(ctx, backup); err != nil { + return err + } + + f, err := l.Sink.GetWriter(ctx, manifestPath(backup)) + if err != nil { + return fmt.Errorf("manifest: commit: %w", err) + } + defer func() { + if err := f.Close(); err != nil && returnErr == nil { + returnErr = fmt.Errorf("manifest: commit: %w", err) + } + }() + + if err := toml.NewEncoder(f).Encode(backup); err != nil { + return fmt.Errorf("manifest: commit: %w", err) + } + + return nil +} + +// FindLatest passes through to Fallback +func (l ManifestLocator) FindLatest(ctx context.Context, repo storage.Repository) (*Backup, error) { + return l.Fallback.FindLatest(ctx, repo) +} + +// Find passes through to Fallback +func (l ManifestLocator) Find(ctx context.Context, repo storage.Repository, backupID string) (*Backup, error) { + return l.Fallback.Find(ctx, repo, backupID) +} + +func manifestPath(backup *Backup) string { + storageName := backup.Repository.GetStorageName() + // Other locators strip the .git suffix off of relative paths. This suffix + // is determined by gitlab-rails not gitaly. So here we leave the relative + // path as-is so that new backups can be more independent. + relativePath := backup.Repository.GetRelativePath() + return path.Join("manifests", storageName, relativePath, backup.ID+".toml") +} diff --git a/internal/backup/locator_test.go b/internal/backup/locator_test.go index 3f99da2409..7aed83608a 100644 --- a/internal/backup/locator_test.go +++ b/internal/backup/locator_test.go @@ -403,3 +403,83 @@ func TestPointerLocator(t *testing.T) { }) }) } + +func TestManifestLocator(t *testing.T) { + t.Parallel() + + const backupID = "abc123" + + ctx := testhelper.Context(t) + cfg := testcfg.Build(t) + + repo, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{ + SkipCreationViaService: true, + RelativePath: t.Name(), + }) + gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch(git.DefaultBranch)) + + t.Run("BeginFull/Commit", func(t *testing.T) { + t.Parallel() + + backupPath := testhelper.TempDir(t) + sink := NewFilesystemSink(backupPath) + var l Locator = PointerLocator{ + Sink: sink, + } + l = ManifestLocator{ + Sink: sink, + Fallback: l, + } + + full := l.BeginFull(ctx, repo, backupID) + require.NoError(t, l.Commit(ctx, full)) + + manifest := testhelper.MustReadFile(t, filepath.Join(backupPath, "manifests", repo.StorageName, repo.RelativePath, backupID+".toml")) + require.Equal(t, fmt.Sprintf(`object_format = 'sha1' + +[[steps]] +bundle_path = '%[1]s/%[2]s/001.bundle' +ref_path = '%[1]s/%[2]s/001.refs' +custom_hooks_path = '%[1]s/%[2]s/001.custom_hooks.tar' +`, repo.RelativePath, backupID), string(manifest)) + }) + + t.Run("BeginIncremental/Commit", func(t *testing.T) { + t.Parallel() + + backupPath := testhelper.TempDir(t) + + testhelper.WriteFiles(t, backupPath, map[string]any{ + filepath.Join(repo.RelativePath, "LATEST"): "abc123", + filepath.Join(repo.RelativePath, "abc123", "LATEST"): "001", + }) + + sink := NewFilesystemSink(backupPath) + var l Locator = PointerLocator{ + Sink: sink, + } + l = ManifestLocator{ + Sink: sink, + Fallback: l, + } + + incremental, err := l.BeginIncremental(ctx, repo, backupID) + require.NoError(t, err) + require.NoError(t, l.Commit(ctx, incremental)) + + manifest := testhelper.MustReadFile(t, filepath.Join(backupPath, "manifests", repo.StorageName, repo.RelativePath, backupID+".toml")) + require.Equal(t, fmt.Sprintf(`object_format = 'sha1' + +[[steps]] +bundle_path = '%[1]s/%[2]s/001.bundle' +ref_path = '%[1]s/%[2]s/001.refs' +custom_hooks_path = '%[1]s/%[2]s/001.custom_hooks.tar' + +[[steps]] +bundle_path = '%[1]s/%[2]s/002.bundle' +ref_path = '%[1]s/%[2]s/002.refs' +previous_ref_path = '%[1]s/%[2]s/001.refs' +custom_hooks_path = '%[1]s/%[2]s/002.custom_hooks.tar' +`, repo.RelativePath, backupID), string(manifest)) + }) +} -- GitLab From b584d322dd8a379b92ef7eb33bc22a20700e73a3 Mon Sep 17 00:00:00 2001 From: James Fargher Date: Wed, 20 Sep 2023 12:06:24 +1200 Subject: [PATCH 5/5] backup: Write backup manifest files Changelog: changed --- internal/backup/backup.go | 17 ++++++++++++----- internal/backup/backup_test.go | 7 +++++++ 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/internal/backup/backup.go b/internal/backup/backup.go index 05e0b14ee4..a2b38fc582 100644 --- a/internal/backup/backup.go +++ b/internal/backup/backup.go @@ -107,18 +107,25 @@ type Repository interface { // ResolveLocator returns a locator implementation based on a locator identifier. func ResolveLocator(layout string, sink Sink) (Locator, error) { - legacy := LegacyLocator{} + var locator Locator = LegacyLocator{} + switch layout { case "legacy": - return legacy, nil case "pointer": - return PointerLocator{ + locator = PointerLocator{ Sink: sink, - Fallback: legacy, - }, nil + Fallback: locator, + } default: return nil, fmt.Errorf("unknown layout: %q", layout) } + + locator = ManifestLocator{ + Sink: sink, + Fallback: locator, + } + + return locator, nil } // Manager manages process of the creating/restoring backups. diff --git a/internal/backup/backup_test.go b/internal/backup/backup_test.go index ac7b155c03..9b84cec55d 100644 --- a/internal/backup/backup_test.go +++ b/internal/backup/backup_test.go @@ -173,6 +173,7 @@ func TestManager_Create(t *testing.T) { StorageName: "some_storage", } + manifestPath := filepath.Join(backupRoot, "manifests", vanityRepo.StorageName, vanityRepo.RelativePath, backupID+".toml") refsPath := joinBackupPath(t, backupRoot, vanityRepo, backupID, "001.refs") bundlePath := joinBackupPath(t, backupRoot, vanityRepo, backupID, "001.bundle") customHooksPath := joinBackupPath(t, backupRoot, vanityRepo, backupID, "001.custom_hooks.tar") @@ -224,6 +225,12 @@ func TestManager_Create(t *testing.T) { require.NoFileExists(t, refsPath) } + if tc.createsBundle || tc.createsRefList { + require.FileExists(t, manifestPath) + } else { + require.NoFileExists(t, manifestPath) + } + if tc.createsCustomHooks { require.FileExists(t, customHooksPath) } else { -- GitLab