Inspect changes in Gitaly 5274274cf9ea696f8bdb95687a0f9591b73b6550
Upstream merge commit 75e2b7e17f5d changes Gitaly server version from e4f946ba6483d4e297e66b7790a4831b9bae9651 to 5274274cf9ea696f8bdb95687a0f9591b73b6550, with changes in protocol or one of the source subtrees currently under watch.
The topic upstream-merge
is at 115 non-merge commits.
To further inspect, go to an up to date Gitaly source clone and run:
git diff e4f946ba6483d4e297e66b7790a4831b9bae9651..5274274cf9ea696f8bdb95687a0f9591b73b6550 -- proto/*.proto internal/backup
Here is a preview, truncated at 500 lines:
diff --git a/internal/backup/backup.go b/internal/backup/backup.go
index bc54cbaf0..e11d67bf9 100644
--- a/internal/backup/backup.go
+++ b/internal/backup/backup.go
@@ -13,6 +13,7 @@ import (
"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/client_golang/prometheus/promauto"
+ "github.com/sirupsen/logrus"
"gitlab.com/gitlab-org/gitaly/v16/internal/git"
"gitlab.com/gitlab-org/gitaly/v16/internal/git/catfile"
"gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo"
@@ -131,6 +132,14 @@ type Repository interface {
ObjectHash(ctx context.Context) (git.ObjectHash, error)
// HeadReference fetches the reference pointed to by HEAD.
HeadReference(ctx context.Context) (git.ReferenceName, error)
+ // ResetRefs attempts to reset the list of refs in the repository to match the
+ // specified refs slice. This can fail if objects pointed to by a ref no longer
+ // exists in the repository. The list of refs should not include the symbolic
+ // HEAD reference.
+ ResetRefs(ctx context.Context, refs []git.Reference) error
+ // SetHeadReference sets the symbolic HEAD reference of the repository to the
+ // given target, for example a branch name.
+ SetHeadReference(ctx context.Context, target git.ReferenceName) error
}
// ResolveLocator returns a locator implementation based on a locator identifier.
@@ -144,6 +153,8 @@ func ResolveLocator(layout string, sink Sink) (Locator, error) {
Sink: sink,
Fallback: locator,
}
+ case "manifest":
+ locator = nil
default:
return nil, fmt.Errorf("unknown layout: %q", layout)
}
@@ -161,6 +172,7 @@ type Manager struct {
sink Sink
conns *client.Pool
locator Locator
+ logger log.Logger
// repositoryFactory returns an abstraction over git repositories in order
// to create and restore backups.
@@ -168,7 +180,7 @@ type Manager struct {
}
// NewManager creates and returns initialized *Manager instance.
-func NewManager(sink Sink, locator Locator, pool *client.Pool) *Manager {
+func NewManager(sink Sink, logger log.Logger, locator Locator, pool *client.Pool) *Manager {
return &Manager{
sink: sink,
conns: pool,
@@ -183,8 +195,9 @@ func NewManager(sink Sink, locator Locator, pool *client.Pool) *Manager {
return nil, err
}
- return newRemoteRepository(repo, conn), nil
+ return NewRemoteRepository(repo, conn), nil
},
+ logger: logger,
}
}
@@ -206,8 +219,9 @@ func NewManagerLocal(
repositoryFactory: func(ctx context.Context, repo *gitalypb.Repository, server storage.ServerInfo) (Repository, error) {
localRepo := localrepo.New(logger, storageLocator, gitCmdFactory, catfileCache, repo)
- return newLocalRepository(logger, storageLocator, gitCmdFactory, txManager, repoCounter, localRepo), nil
+ return NewLocalRepository(logger, storageLocator, gitCmdFactory, txManager, repoCounter, localRepo), nil
},
+ logger: logger,
}
}
@@ -338,68 +352,126 @@ func (mgr *Manager) Restore(ctx context.Context, req *RestoreRequest) error {
var backup *Backup
if req.BackupID == "" {
backup, err = mgr.locator.FindLatest(ctx, req.VanityRepository)
+ switch {
+ case errors.Is(err, ErrDoesntExist):
+ return repositorySkipped(ctx, repo, req.AlwaysCreate, err)
+ case err != nil:
+ return fmt.Errorf("manager: %w", err)
+ }
} else {
backup, err = mgr.locator.Find(ctx, req.VanityRepository, req.BackupID)
+ switch {
+ case errors.Is(err, ErrDoesntExist):
+ return fmt.Errorf("manager: %w: %w", ErrSkipped, err)
+ case err != nil:
+ return fmt.Errorf("manager: %w", err)
+ }
}
- switch {
- case errors.Is(err, ErrDoesntExist):
- return fmt.Errorf("manager: %w: %s", ErrSkipped, err.Error())
- case err != nil:
- return fmt.Errorf("manager: %w", err)
+
+ if len(backup.Steps) == 0 {
+ return fmt.Errorf("manager: no backup steps")
+ }
+
+ // Restore Git objects, potentially from increments.
+ if err := mgr.restoreFromRefs(ctx, repo, backup); err != nil {
+ mgr.logger.WithFields(log.Fields{
+ "storage": req.Repository.GetStorageName(),
+ "relative_path": req.Repository.GetRelativePath(),
+ "backup_id": backup.ID,
+ logrus.ErrorKey: err,
+ }).Warn("unable to reset refs. Proceeding with a normal restore")
+
+ // If we can't reset the refs, perform a full restore by recreating the repo and cloning from the bundle.
+ if err := mgr.restoreFromBundle(ctx, repo, backup, req.AlwaysCreate); err != nil {
+ return fmt.Errorf("manager: restore from bundle: %w", err)
+ }
+ }
+
+ // Restore custom hooks. Each custom hooks archive contains the entirety of the hooks, so
+ // we can just restore the most recent archive.
+ latestStep := backup.Steps[len(backup.Steps)-1]
+ return mgr.restoreCustomHooks(ctx, repo, latestStep.CustomHooksPath)
+}
+
+func (mgr *Manager) restoreFromRefs(ctx context.Context, repo Repository, backup *Backup) error {
+ latestStep := backup.Steps[len(backup.Steps)-1]
+ refs, err := mgr.readRefs(ctx, latestStep.RefPath)
+ if err != nil {
+ return fmt.Errorf("read refs from backup: %w", err)
+ }
+ if len(refs) == 0 {
+ return errors.New("no refs in backup")
+ }
+
+ // Reset all refs except for HEAD.
+ if err := repo.ResetRefs(ctx, refs); err != nil {
+ return fmt.Errorf("reset refs: %w", err)
}
+ // Explicitly reset HEAD to the default branch tracked by the manifest if available. In a
+ // bundle restore, this would've been done during repository creation.
+ headRef := git.ReferenceName(backup.HeadReference)
+ if headRef == "" {
+ return errors.New("expected HEAD to be a symbolic reference")
+ }
+
+ return repo.SetHeadReference(ctx, headRef)
+}
+
+func (mgr *Manager) restoreFromBundle(ctx context.Context, repo Repository, backup *Backup, alwaysCreate bool) error {
hash, err := git.ObjectHashByFormat(backup.ObjectFormat)
if err != nil {
- return fmt.Errorf("manager: %w", err)
+ return err
}
defaultBranch, defaultBranchKnown := git.ReferenceName(backup.HeadReference).Branch()
if err := repo.Remove(ctx); err != nil {
- return fmt.Errorf("manager: %w", err)
+ return err
}
-
if err := repo.Create(ctx, hash, defaultBranch); err != nil {
- return fmt.Errorf("manager: %w", err)
+ return err
}
for _, step := range backup.Steps {
refs, err := mgr.readRefs(ctx, step.RefPath)
switch {
case errors.Is(err, ErrDoesntExist):
- // For compatibility with existing backups we need to make sure the
- // repository exists 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. Since the
- // repository has already been created, we simply skip cleaning up.
- if req.AlwaysCreate {
- return nil
- }
-
- if err := repo.Remove(ctx); err != nil {
- return fmt.Errorf("manager: remove on skipped: %w", err)
- }
-
- return fmt.Errorf("manager: %w: %s", ErrSkipped, err.Error())
+ return repositorySkipped(ctx, repo, alwaysCreate, err)
case err != nil:
- return fmt.Errorf("manager: %w", err)
+ return fmt.Errorf("read refs: %w", err)
}
// Git bundles can not be created for empty repositories. Since empty
// repository backups do not contain a bundle, skip bundle restoration.
if len(refs) > 0 {
if err := mgr.restoreBundle(ctx, repo, step.BundlePath, !defaultBranchKnown); err != nil {
- return fmt.Errorf("manager: %w", err)
+ return fmt.Errorf("restore bundle: %w", err)
}
}
- if err := mgr.restoreCustomHooks(ctx, repo, step.CustomHooksPath); err != nil {
- return fmt.Errorf("manager: %w", err)
- }
}
+
return nil
}
+func repositorySkipped(ctx context.Context, repo Repository, alwaysCreate bool, cause error) error {
+ // For compatibility with existing backups we need to make sure the
+ // repository exists 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. Since the
+ // repository has already been created, we simply skip cleaning up.
+ if alwaysCreate {
+ return nil
+ }
+
+ if err := repo.Remove(ctx); err != nil {
+ return fmt.Errorf("manager: remove on skipped: %w", err)
+ }
+
+ return fmt.Errorf("%w: %w", ErrSkipped, cause)
+}
+
// setContextServerInfo overwrites server with gitaly connection info from ctx metadata when server is zero.
func setContextServerInfo(ctx context.Context, server *storage.ServerInfo, storageName string) error {
if !server.Zero() {
@@ -512,6 +584,11 @@ func (mgr *Manager) readRefs(ctx context.Context, path string) ([]git.Reference,
return refs, fmt.Errorf("read refs: %w", err)
}
+ // HEAD is tracked as a symbolic reference in the backup manifest and will be restored separately.
+ if ref.Name == "HEAD" {
+ continue
+ }
+
refs = append(refs, ref)
}
diff --git a/internal/backup/backup_test.go b/internal/backup/backup_test.go
index 42e2d3c39..979c32a66 100644
--- a/internal/backup/backup_test.go
+++ b/internal/backup/backup_test.go
@@ -21,6 +21,7 @@ import (
"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/transaction"
"gitlab.com/gitlab-org/gitaly/v16/internal/grpc/client"
"gitlab.com/gitlab-org/gitaly/v16/internal/helper/perm"
+ "gitlab.com/gitlab-org/gitaly/v16/internal/log"
"gitlab.com/gitlab-org/gitaly/v16/internal/testhelper"
"gitlab.com/gitlab-org/gitaly/v16/internal/testhelper/testcfg"
"gitlab.com/gitlab-org/gitaly/v16/internal/testhelper/testserver"
@@ -29,6 +30,8 @@ import (
)
func TestManager_RemoveRepository(t *testing.T) {
+ testhelper.SkipWithReftable(t, "localrepo.SetDefaultBranch modifies HEAD through the filesystem directly")
+
if testhelper.IsPraefectEnabled() {
t.Skip("local backup manager expects to operate on the local filesystem so cannot operate through praefect")
}
@@ -51,10 +54,10 @@ func TestManager_RemoveRepository(t *testing.T) {
sink := backup.NewFilesystemSink(backupRoot)
defer testhelper.MustClose(t, sink)
- locator, err := backup.ResolveLocator("pointer", sink)
+ locator, err := backup.ResolveLocator("manifest", sink)
require.NoError(t, err)
- fsBackup := backup.NewManager(sink, locator, pool)
+ fsBackup := backup.NewManager(sink, testhelper.SharedLogger(t), locator, pool)
err = fsBackup.RemoveRepository(ctx, &backup.RemoveRepositoryRequest{
Server: storage.ServerInfo{Address: cfg.SocketPath, Token: cfg.Auth.Token},
Repo: repo,
@@ -142,10 +145,10 @@ func TestManager_ListRepositories(t *testing.T) {
sink := backup.NewFilesystemSink(backupRoot)
defer testhelper.MustClose(t, sink)
- locator, err := backup.ResolveLocator("pointer", sink)
+ locator, err := backup.ResolveLocator("manifest", sink)
require.NoError(t, err)
- fsBackup := backup.NewManager(sink, locator, pool)
+ fsBackup := backup.NewManager(sink, testhelper.SharedLogger(t), locator, pool)
for storageName, repos := range tc.repos {
actualRepos, err := fsBackup.ListRepositories(ctx, &backup.ListRepositoriesRequest{
@@ -184,7 +187,7 @@ func TestManager_Create(t *testing.T) {
testhelper.MustClose(tb, pool)
})
- return backup.NewManager(sink, locator, pool)
+ return backup.NewManager(sink, testhelper.SharedLogger(t), locator, pool)
},
},
{
@@ -339,7 +342,7 @@ func TestManager_Create(t *testing.T) {
sink := backup.NewFilesystemSink(backupRoot)
defer testhelper.MustClose(t, sink)
- locator, err := backup.ResolveLocator("pointer", sink)
+ locator, err := backup.ResolveLocator("manifest", sink)
require.NoError(t, err)
fsBackup := managerTC.setup(t, sink, locator)
@@ -431,7 +434,7 @@ func TestManager_Create_incremental(t *testing.T) {
testhelper.MustClose(tb, pool)
})
- return backup.NewManager(sink, locator, pool)
+ return backup.NewManager(sink, testhelper.SharedLogger(t), locator, pool)
},
},
{
@@ -472,19 +475,18 @@ func TestManager_Create_incremental(t *testing.T) {
repo, repoPath := gittest.CreateRepository(tb, ctx, cfg)
gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch(git.DefaultBranch))
- backupRepoPath := joinBackupPath(tb, backupRoot, repo)
- backupPath := filepath.Join(backupRepoPath, backupID)
- bundlePath := filepath.Join(backupPath, "001.bundle")
- refsPath := filepath.Join(backupPath, "001.refs")
-
- require.NoError(tb, os.MkdirAll(backupPath, perm.PublicDir))
- gittest.Exec(tb, cfg, "-C", repoPath, "bundle", "create", bundlePath, "--all")
-
- refs := gittest.Exec(tb, cfg, "-C", repoPath, "show-ref", "--head")
- require.NoError(tb, os.WriteFile(refsPath, refs, perm.PublicFile))
+ testhelper.WriteFiles(tb, backupRoot, map[string]any{
+ filepath.Join("manifests", repo.GetStorageName(), repo.GetRelativePath(), "+latest.toml"): fmt.Sprintf(`
+object_format = '%[1]s'
- require.NoError(tb, os.WriteFile(filepath.Join(backupRepoPath, "LATEST"), []byte(backupID), perm.PublicFile))
- require.NoError(tb, os.WriteFile(filepath.Join(backupPath, "LATEST"), []byte("001"), perm.PublicFile))
+[[steps]]
+bundle_path = '%[2]s/001.bundle'
+ref_path = '%[2]s/001.refs'
+custom_hooks_path = '%[2]s/001.custom_hooks.tar'
+ `, gittest.DefaultObjectHash.Format, repo.GetRelativePath()),
+ filepath.Join(repo.GetRelativePath(), "001.bundle"): gittest.Exec(tb, cfg, "-C", repoPath, "bundle", "create", "-", "--all"),
+ filepath.Join(repo.GetRelativePath(), "001.refs"): gittest.Exec(tb, cfg, "-C", repoPath, "show-ref", "--head"),
+ })
return repo, repoPath
},
@@ -496,19 +498,18 @@ func TestManager_Create_incremental(t *testing.T) {
repo, repoPath := gittest.CreateRepository(tb, ctx, cfg)
commitID := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch(git.DefaultBranch))
- backupRepoPath := joinBackupPath(tb, backupRoot, repo)
- backupPath := filepath.Join(backupRepoPath, backupID)
- bundlePath := filepath.Join(backupPath, "001.bundle")
- refsPath := filepath.Join(backupPath, "001.refs")
-
- require.NoError(tb, os.MkdirAll(backupPath, perm.PublicDir))
- gittest.Exec(tb, cfg, "-C", repoPath, "bundle", "create", bundlePath, "--all")
-
- refs := gittest.Exec(tb, cfg, "-C", repoPath, "show-ref", "--head")
- require.NoError(tb, os.WriteFile(refsPath, refs, perm.PublicFile))
+ testhelper.WriteFiles(tb, backupRoot, map[string]any{
+ filepath.Join("manifests", repo.GetStorageName(), repo.GetRelativePath(), "+latest.toml"): fmt.Sprintf(`
+object_format = '%[1]s'
- require.NoError(tb, os.WriteFile(filepath.Join(backupRepoPath, "LATEST"), []byte(backupID), perm.PublicFile))
- require.NoError(tb, os.WriteFile(filepath.Join(backupPath, "LATEST"), []byte("001"), perm.PublicFile))
+[[steps]]
+bundle_path = '%[2]s/001.bundle'
+ref_path = '%[2]s/001.refs'
+custom_hooks_path = '%[2]s/001.custom_hooks.tar'
+ `, gittest.DefaultObjectHash.Format, repo.GetRelativePath()),
+ filepath.Join(repo.GetRelativePath(), "001.bundle"): gittest.Exec(tb, cfg, "-C", repoPath, "bundle", "create", "-", "--all"),
+ filepath.Join(repo.GetRelativePath(), "001.refs"): gittest.Exec(tb, cfg, "-C", repoPath, "show-ref", "--head"),
+ })
gittest.WriteCommit(tb, cfg, repoPath, gittest.WithBranch(git.DefaultBranch), gittest.WithParents(commitID))
@@ -527,7 +528,7 @@ func TestManager_Create_incremental(t *testing.T) {
sink := backup.NewFilesystemSink(backupRoot)
defer testhelper.MustClose(t, sink)
- locator, err := backup.ResolveLocator("pointer", sink)
+ locator, err := backup.ResolveLocator("manifest", sink)
require.NoError(t, err)
fsBackup := managerTC.setup(t, sink, locator)
@@ -556,6 +557,8 @@ func TestManager_Create_incremental(t *testing.T) {
}
func TestManager_Restore_latest(t *testing.T) {
+ testhelper.SkipWithReftable(t, "localrepo.SetDefaultBranch modifies HEAD through the filesystem directly")
+
t.Parallel()
cfg := testcfg.Build(t)
@@ -565,22 +568,22 @@ func TestManager_Restore_latest(t *testing.T) {
for _, managerTC := range []struct {
desc string
- setup func(t testing.TB, sink backup.Sink, locator backup.Locator) *backup.Manager
+ setup func(t testing.TB, sink backup.Sink, locator backup.Locator, logger log.LogrusLogger) *backup.Manager
}{
{
desc: "RPC manager",
- setup: func(tb testing.TB, sink backup.Sink, locator backup.Locator) *backup.Manager {
+ setup: func(tb testing.TB, sink backup.Sink, locator backup.Locator, logger log.LogrusLogger) *backup.Manager {
pool := client.NewPool()
tb.Cleanup(func() {
testhelper.MustClose(tb, pool)
})
- return backup.NewManager(sink, locator, pool)
+ return backup.NewManager(sink, logger, locator, pool)
},
},
{
desc: "Local manager",
- setup: func(tb testing.TB, sink backup.Sink, locator backup.Locator) *backup.Manager {
+ setup: func(tb testing.TB, sink backup.Sink, locator backup.Locator, logger log.LogrusLogger) *backup.Manager {
if testhelper.IsPraefectEnabled() {
tb.Skip("local backup manager expects to operate on the local filesystem so cannot operate through praefect")
}
@@ -591,7 +594,7 @@ func TestManager_Restore_latest(t *testing.T) {
tb.Cleanup(catfileCache.Stop)
txManager := transaction.NewTrackingManager()
- return backup.NewManagerLocal(sink, testhelper.SharedLogger(t), locator, storageLocator, gitCmdFactory, catfileCache, txManager, repoCounter)
+ return backup.NewManagerLocal(sink, logger, locator, storageLocator, gitCmdFactory, catfileCache, txManager, repoCounter)
},
},
} {
@@ -608,13 +611,6 @@ func TestManager_Restore_latest(t *testing.T) {
repoClient := gitalypb.NewRepositoryServiceClient(cc)
- _, repoPath := gittest.CreateRepository(t, ctx, cfg)
- commitID := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("main"))
- gittest.WriteTag(t, cfg, repoPath, "v1.0.0", commitID.Revision())
- repoChecksum := gittest.ChecksumRepo(t, cfg, repoPath)
- repoBundle := gittest.BundleRepo(t, cfg, repoPath, "-")
- repoRefs := gittest.Exec(t, cfg, "-C", repoPath, "show-ref", "--head")
-
backupRoot := testhelper.TempDir(t)
for _, tc := range []struct {
@@ -626,20 +622,53 @@ func TestManager_Restore_latest(t *testing.T) {
expectedErrAs error
}{
{
- desc: "existing repo, without hooks",
+ desc: "non-optimised",
setup: func(tb testing.TB) (*gitalypb.Repository, *git.Checksum) {
+ _, repoPath, _ := createAndSeedRepository(t, ctx, cfg)
+ repoChecksum, repoBundle, repoRefs := createBackupArtifacts(t, cfg, repoPath)
+
+ // Restoring into an empty repo, so ref reset won't work.
repo, _ := gittest.CreateRepository(t, ctx, cfg)
relativePath := stripRelativePath(tb, repo)
testhelper.WriteFiles(tb, backupRoot, map[string]any{
filepath.Join("manifests", repo.GetStorageName(), repo.GetRelativePath(), "+latest.toml"): fmt.Sprintf(`
object_format = '%[1]s'
+head_reference = '%[3]s'
[[steps]]
bundle_path = '%[2]s.bundle'
ref_path = '%[2]s.refs'
custom_hooks_path = '%[2]s/custom_hooks.tar'
- `, gittest.DefaultObjectHash.Format, relativePath),
+ `, gittest.DefaultObjectHash.Format, relativePath, git.DefaultRef.String()),
+ relativePath + ".bundle": repoBundle,
+ relativePath + ".refs": repoRefs,
+ })
+
+ require.NoError(tb, os.MkdirAll(filepath.Join(backupRoot, relativePath), perm.PublicDir))
+
+ return repo, repoChecksum
+ },
+ expectExists: true,
+ },
+ {
+ desc: "existing repo, without hooks",
+ setup: func(tb testing.TB) (*gitalypb.Repository, *git.Checksum) {
+ repo, repoPath, head := createAndSeedRepository(t, ctx, cfg)
+ repoChecksum, repoBundle, repoRefs := createBackupArtifacts(t, cfg, repoPath)
+ gittest.WriteCommit(t, cfg, repoPath, gittest.WithParents(head), gittest.WithBranch("main"))
+
+ relativePath := stripRelativePath(tb, repo)
+ testhelper.WriteFiles(tb, backupRoot, map[string]any{
+ filepath.Join("manifests", repo.GetStorageName(), repo.GetRelativePath(), "+latest.toml"): fmt.Sprintf(`
+object_format = '%[1]s'
+head_reference = '%[3]s'
+
+[[steps]]
+bundle_path = '%[2]s.bundle'
+ref_path = '%[2]s.refs'
+custom_hooks_path = '%[2]s/custom_hooks.tar'
+ `, gittest.DefaultObjectHash.Format, relativePath, git.DefaultRef.String()),
A merge request may have already been submitted, check that on the merge commit, as usually changes in Gitaly are not breaking, compliance becoming mandatory in the following development cycles.