diff options
author | Patrick Steinhardt <psteinhardt@gitlab.com> | 2021-06-23 15:07:29 +0300 |
---|---|---|
committer | Patrick Steinhardt <psteinhardt@gitlab.com> | 2021-06-28 09:25:08 +0300 |
commit | 3d010219d28a5f5f49aeb60819e319d7101e99f1 (patch) | |
tree | 03061cd361b1ae0fcb0dcb269740a6a7ab862172 | |
parent | 3e5602b63b75966f184272a083d175b5f5f3f31f (diff) |
repository: Fix excessive voting in CreateRepositoryFromBundle
The `CreateRepositoryFromBundle()` RPC does a git-clone(1) followed by a
git-fetch(1) to obtain any references which aren't obtained via the
default refspec. While git-clone(1) creates a single reference
transaction to create all references, git-fetch(1) will by default
create one reference transaction per reference that is to be updated. In
the context of strong consistency, this can thus result in excessive
votes in case a repository has lots of references which are neither
branches nor tags.
To fix this issue, we can use the `--atomic` flag for git-fetch(1) which
we have upstreamed in Git v2.31.0. This will update all references in a
single atomic transaction and thus cause the reference-transaction hook
to only be executed twice (once for "prepare" state, and once for
"commit" state). This effectively puts a boundary of 7 on transactional
votes that CreateRepositoryFromBundle: two votes for the clone, two for
the fetch and a final one to iterate over all existing references via
git-for-each-ref(1).
Given that we cannot introduce new voting behaviour in a release and
also enable it right away to allow for zero-downtime upgrades, this
change is hidden behind a feature flag.
Changelog: performance
-rw-r--r-- | internal/gitaly/service/repository/create_from_bundle.go | 8 | ||||
-rw-r--r-- | internal/gitaly/service/repository/create_from_bundle_test.go | 35 | ||||
-rw-r--r-- | internal/metadata/featureflag/feature_flags.go | 4 |
3 files changed, 35 insertions, 12 deletions
diff --git a/internal/gitaly/service/repository/create_from_bundle.go b/internal/gitaly/service/repository/create_from_bundle.go index afaf9dc7d..d53c947c7 100644 --- a/internal/gitaly/service/repository/create_from_bundle.go +++ b/internal/gitaly/service/repository/create_from_bundle.go @@ -11,6 +11,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v14/internal/git" "gitlab.com/gitlab-org/gitaly/v14/internal/helper" + "gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/v14/internal/tempdir" "gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb" "gitlab.com/gitlab-org/gitaly/v14/streamio" @@ -92,12 +93,17 @@ func (s *server) CreateRepositoryFromBundle(stream gitalypb.RepositoryService_Cr return status.Error(codes.Internal, cleanError) } + fetchFlags := []git.Option{git.Flag{Name: "--quiet"}} + if featureflag.IsEnabled(ctx, featureflag.CreateRepositoryFromBundleAtomicFetch) { + fetchFlags = append(fetchFlags, git.Flag{Name: "--atomic"}) + } + // We do a fetch to get all refs including keep-around refs stderr.Reset() cmd, err = s.gitCmdFactory.NewWithDir(ctx, repoPath, git.SubCmd{ Name: "fetch", - Flags: []git.Option{git.Flag{Name: "--quiet"}}, + Flags: fetchFlags, Args: []string{bundlePath, "refs/*:refs/*"}, }, git.WithStderr(&stderr), diff --git a/internal/gitaly/service/repository/create_from_bundle_test.go b/internal/gitaly/service/repository/create_from_bundle_test.go index 98b544e6a..761b31e78 100644 --- a/internal/gitaly/service/repository/create_from_bundle_test.go +++ b/internal/gitaly/service/repository/create_from_bundle_test.go @@ -19,6 +19,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction" "gitlab.com/gitlab-org/gitaly/v14/internal/helper" "gitlab.com/gitlab-org/gitaly/v14/internal/helper/text" + "gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/v14/internal/tempdir" "gitlab.com/gitlab-org/gitaly/v14/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testassert" @@ -94,6 +95,12 @@ func TestServer_CreateRepositoryFromBundle_successful(t *testing.T) { } func TestServer_CreateRepositoryFromBundle_transactional(t *testing.T) { + testhelper.NewFeatureSets([]featureflag.FeatureFlag{ + featureflag.CreateRepositoryFromBundleAtomicFetch, + }).Run(t, testServerCreateRepositoryFromBundleTransactional) +} + +func testServerCreateRepositoryFromBundleTransactional(t *testing.T, ctx context.Context) { var votes []voting.Vote txManager := &transaction.MockManager{ VoteFn: func(ctx context.Context, tx txinfo.Transaction, vote voting.Vote) error { @@ -115,8 +122,6 @@ func TestServer_CreateRepositoryFromBundle_transactional(t *testing.T) { gittest.Exec(t, cfg, "-C", repoPath, "update-ref", keepAroundRef, masterOID) } - ctx, cancel := testhelper.Context() - defer cancel() ctx, err := txinfo.InjectTransaction(ctx, 1, "primary", true) require.NoError(t, err) ctx = helper.IncomingToOutgoing(ctx) @@ -157,15 +162,23 @@ func TestServer_CreateRepositoryFromBundle_transactional(t *testing.T) { ) // Keep-around references are not fetched via git-clone(1) because non-mirror clones only - // fetch branches and tags. These additional references are thus obtained via git-fetch(1), - // which by default creates one reference transaction per reference. We thus see two commits - // (again, for "prepare" and "commit" phases) per reference. - votingInput = append(votingInput, - fmt.Sprintf("%s %s refs/keep-around/2\n", git.ZeroOID, masterOID), - fmt.Sprintf("%s %s refs/keep-around/2\n", git.ZeroOID, masterOID), - fmt.Sprintf("%s %s refs/keep-around/1\n", git.ZeroOID, masterOID), - fmt.Sprintf("%s %s refs/keep-around/1\n", git.ZeroOID, masterOID), - ) + // fetch branches and tags. These additional references are thus obtained via git-fetch(1). + // If the following feature flag is enabled, then we'll use the `--atomic` flag for + // git-fetch(1) and thus bundle all reference updates into a single transaction. Otherwise, + // the old behaviour will create one transaction per reference. + if featureflag.IsEnabled(ctx, featureflag.CreateRepositoryFromBundleAtomicFetch) { + votingInput = append(votingInput, + fmt.Sprintf("%s %s refs/keep-around/2\n%s %s refs/keep-around/1\n", git.ZeroOID, masterOID, git.ZeroOID, masterOID), + fmt.Sprintf("%s %s refs/keep-around/2\n%s %s refs/keep-around/1\n", git.ZeroOID, masterOID, git.ZeroOID, masterOID), + ) + } else { + votingInput = append(votingInput, + fmt.Sprintf("%s %s refs/keep-around/2\n", git.ZeroOID, masterOID), + fmt.Sprintf("%s %s refs/keep-around/2\n", git.ZeroOID, masterOID), + fmt.Sprintf("%s %s refs/keep-around/1\n", git.ZeroOID, masterOID), + fmt.Sprintf("%s %s refs/keep-around/1\n", git.ZeroOID, masterOID), + ) + } // And this is the final vote in Create(), which does a git-for-each-ref(1) in the target // repository and then manually invokes the hook. The format is thus different from above diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go index 50bb19293..7565041a9 100644 --- a/internal/metadata/featureflag/feature_flags.go +++ b/internal/metadata/featureflag/feature_flags.go @@ -16,6 +16,9 @@ var ( // LFSPointersPipeline enables the alternative pipeline implementation of LFS-pointer // related RPCs. LFSPointersPipeline = FeatureFlag{Name: "lfs_pointers_pipeline", OnByDefault: false} + // CreateRepositoryFromBundleAtomicFetch will add the `--atomic` flag to git-fetch(1) in + // order to reduce the number of transactional votes. + CreateRepositoryFromBundleAtomicFetch = FeatureFlag{Name: "create_repository_from_bundle_atomic_fetch", OnByDefault: false} ) // All includes all feature flags. @@ -23,4 +26,5 @@ var All = []FeatureFlag{ GoUpdateRemoteMirror, FetchInternalRemoteErrors, LFSPointersPipeline, + CreateRepositoryFromBundleAtomicFetch, } |