| Age | Commit message (Collapse) | Author |
|---|
|
The `intercepted_method` option was used to mark a method as being handled
by Praefect. Since the operation type and scope annotations were mostly for
Praefect's proxy, marking a method as intecepted allowed for dropping the
annotations as they were not needed. As we've now annotated again the methods
that had the option set, let's remove the option as it is no longer needed.
|
|
Some RPCs in Gitaly are missing operation type and scope annotations.
These are needed to handle starting transactions for RPCs in a general
manner. The RPCs were missing the annotations as they are intercepted
by Praefect and the annotations were previously only used by Praefect
for deciding how to proxy the requests. As Gitaly will soon need the
annotations itself, this commit adds back the missing annotations.
The ServerService was previously using a Server scope that doesn't
exist these days. We're annotating the RPCs as storage scoped even
though they're not really storage scoped. The current use cases do
not differentiate between server and storage scoped RPCs:
1. Gitaly's transaction code only needs to know whether or not the
RPC is repository scoped or not.
2. Praefect implements the ServerService directly so it wouldn't
differentiate either based on storage vs server scope.
We'll do the smaller change for now to annotate the RPCs even if the
scope is slightly wrong. We can later move from a multivalued scope
field to a boolean that says whether the RPC is repository scoped or
not.
TransactionService and PraefectInfoService service remain without
annotations. These are only implemented by Praefect and thus Gitaly
doesn't need annotations for them.
|
|
go: Update module github.com/prometheus/client_golang to v1.16.0
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5962
Merged-by: Toon Claes <toon@gitlab.com>
Approved-by: Toon Claes <toon@gitlab.com>
Co-authored-by: GitLab Renovate Bot <gitlab-bot@gitlab.com>
|
|
tools/protoc-gen-go: Update module google.golang.org/protobuf to v1.31.0
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6011
Merged-by: Toon Claes <toon@gitlab.com>
Approved-by: Toon Claes <toon@gitlab.com>
Co-authored-by: GitLab Renovate Bot <gitlab-bot@gitlab.com>
|
|
tools/dlv: Update module github.com/go-delve/delve to v1.21.0
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6015
Merged-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Co-authored-by: GitLab Renovate Bot <gitlab-bot@gitlab.com>
|
|
git: Increase locking timeout for loose references
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6013
Merged-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
|
Makefile: Allow user to override bench targets
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5998
Merged-by: Toon Claes <toon@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Approved-by: Toon Claes <toon@gitlab.com>
Co-authored-by: Will Chandler <wchandler@gitlab.com>
|
|
go: Update module github.com/miekg/dns to v1.1.55
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5971
Merged-by: Toon Claes <toon@gitlab.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>
Approved-by: Toon Claes <toon@gitlab.com>
Co-authored-by: GitLab Renovate Bot <gitlab-bot@gitlab.com>
|
|
|
|
git: Update to Git v2.41.0.gl1
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6014
Merged-by: karthik nayak <knayak@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
|
We have recently upstreamed a new `-Z` option for git-cat-objects(1)
that will cause it to NUL-terminate its output. This is required so that
it becomes possible to request revisions from it that contain embedded
newlines while using one of the `--batch` modes.
We have backported this patch series into Git v2.41.0.gl1, so let's
upgrade to that version so that we can start using this option.
|
|
praefect: Final unification of repository verification
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5991
Merged-by: karthik nayak <knayak@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Reviewed-by: Justin Tobler <jtobler@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
|
Makefile: Fix test repository state depending on host Git version
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6010
Merged-by: karthik nayak <knayak@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
|
Reserve Unavailable response code for service-level unavailability
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5951
Merged-by: Toon Claes <toon@gitlab.com>
Approved-by: Will Chandler <wchandler@gitlab.com>
Approved-by: Toon Claes <toon@gitlab.com>
Reviewed-by: Toon Claes <toon@gitlab.com>
Reviewed-by: Will Chandler <wchandler@gitlab.com>
Reviewed-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Co-authored-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
|
|
When trying to update references, Git will first try to acquire a lock
and write the new value to this lockfile. As those locks may also be
acquired by concurrently running processes it uses a timeout during
which it will spin around the lock until it finally gets released. If
Git fails to acquire the lock in time it aborts the transaction and
returns an error.
We have recently bumped the timeout for acquiring the packed-refs lock
to 1 second given that it is a shared resource that needs to be locked
whenever deleting any reference. This new timeout value does not apply
to loose references though, and that is on purpose: loose references
only require fine-grained locking and thus don't have the same amount of
lock contention as the packed-refs file.
But we have still observed locking issues here in production systems.
During a storm of incoming RPC calls the system was heavily loaded.
While there was some amount of contention around loose references, it
still was comparatively limited. But given that:
- The system was heavily loaded and thus slowed down.
- The default loose reference locking timeout is only a 100
milliseconds.
We saw thousands of `WriteRef()` RPC calls failing because of
concurrently held locks.
Let's thus increase the timeout for loose references, as well. Given
that the contention here should be much lower we don't go all the way to
10 seconds though, but instead only bump it to 1 second, which is the
same 10-fold factor we have applied to the packed-refs locking timeout.
Changelog: fixed
|
|
The coordinator uses its own hand-crafted error for when a repository
cannot be found when rewriting repository-scoped messages. Furthermore,
it uses an error prefix that makes the errors mismatch those returned by
Gitaly.
Drop the prefix and use `storage.NewRepositoryNotFoundError()` to unify
the errors that both Gitaly and Praefect would return.
|
|
Gitaly returns `ErrRepositoryNotSet()` when neither the storage name nor
the relative path are set. Let's do the same in the coordinator so that
both behaviours align.
|
|
The coordinator prefixes most of its errors with "repo scoped", which
makes the errors mismatch those returned by Gitaly. Consequentially,
Praefect does not act as an actually-transparent proxy. This can be
painfully felt in our tests, where we need to distinguish between both
errors.
Drop the prefix and return the error directly to unify the errors that
both Gitaly and Praefect would return.
|
|
The `StreamDirector()` function explicitly checks whether the virtual
storage cannot be found when rewriting the repository-scoped request.
This isn't necessary though given that we call `validateTargetRepo()`
immediately before rewriting the message, which already checks for us
whether the storage is known or not.
Remove the unneeded error check.
|
|
Some of the tests for FindRefsByOid assert whether Gitaly correctly
validates invalid requests. One of these tests checks whether the
request fails as expected when the repository is missing by first
creating the repository, but then deleting it on disk again. This causes
weird test behaviour on Praefect as the repository will exist in the
database, but doesn't on disk. So the actual observed error will be
returned by Gitaly and not by Praefect.
Refactor the test to just not create the repository in the first place,
which is equivalent to a missing repository. While we could instead also
call the `RemoveRepository()` RPC, this would only cause us to evaluate
whether that RPC correctly removes the repository as expected.
|
|
Some of the tests for PruneUnreachableObjects assert whether Gitaly
correctly validates invalid requests. One of these tests checks whether
the request fails as expected when the repository is missing by first
creating the repository, but then deleting it on disk again. This causes
weird test behaviour on Praefect as the repository will exist in the
database, but doesn't on disk. So the actual observed error will be
returned by Gitaly and not by Praefect.
Refactor the test to just not create the repository in the first place,
which is equivalent to a missing repository. While we could instead also
call the `RemoveRepository()` RPC, this would only cause us to evaluate
whether that RPC correctly removes the repository as expected.
|
|
Some of the tests for CreateFork assert whether Gitaly correctly
validates invalid requests. One of these tests is intended to check for
the case where the source repository wasn't provided. But because the
repository has already been created at that point, Praefect has
diverging behaviour from Gitaly as it will refuse to re-create the
repository.
Fix this by instead asking for a new repository to be created.
|
|
One of the tests for RenameRepository expects the same error for both
Gitaly and Praefect, but still uses `testhelper.GitalyOrPraefect()`.
Unify these conditions.
|
|
Some of the tests for PostReceivePack assert whether Gitaly correctly
validates invalid requests. A subset of those tests has diverging
behaviour for Praefect though because it is being called with a repo
that does not exist in the database. This is not the behaviour that we
intend to test.
Fix these by creating the repository via `gittest.CreateRepository()` so
that Praefect knows about its existence.
|
|
|
|
ci: Update FIPS test job to Go 1.20
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6009
Merged-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
|
While deprecated, many tests still depend on the `gitlab-test.git` test
repository. This repository is getting cloned in our Makefile with the
host Git version and is thus subject to whatever Git version the system
got installed.
With Git v2.41, reverse indices have been enabled by default, which
causes git-clone(1) to write an additional `.rev` file into the repo's
object database. And given that we use the local transport in our test
helpers to set up the seeded test repository, the consequence is that
this file also ends up in the cloned test repo, which ultimately ends up
causing a test failure:
=== FAIL: internal/gitaly/service/smarthttp TestPostReceivePack_packfiles (1.08s)
receive_pack_test.go:261:
Error Trace: .../internal/gitaly/service/smarthttp/receive_pack_test.go:261
Error: Should be empty, but was [/tmp/gitaly-446142327/309375621/storages.d/default/@hashed/31/48/31482fd7627141e518a13db4a1e7815044a2e7bfad7b141bd71a2276d9375aa4.git/objects/pack/pack-96123679492d2e0a3bf6a4477532c7e969fdeda4.rev]
Test: TestPostReceivePack_packfiles
DONE 74 tests, 1 skipped, 1 failure in 4.290s
Fix this dependence on the host Git version by explicitly asking Git to
not write reverse indices when we create the test repository.
|
|
Back when we migrated to Go 1.20 we didn't yet have a FIPS-enabled image
for that Go version available. Now we do, so let's update our CI job to
use the new Go version.
|
|
Bump grpc v1.56 and add LookupTimeout configuration to DNS resolver
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5978
Merged-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Reviewed-by: Justin Tobler <jtobler@gitlab.com>
Reviewed-by: karthik nayak <knayak@gitlab.com>
|
|
repository: Avoid NLP to detect license from README
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5970
Merged-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Reviewed-by: karthik nayak <knayak@gitlab.com>
Co-authored-by: Toon Claes <toon@gitlab.com>
|
|
Add expected_old_oid to UserMergeToRefRequest
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5953
Merged-by: karthik nayak <knayak@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Reviewed-by: Toon Claes <toon@gitlab.com>
Reviewed-by: Hordur Freyr Yngvason <hfyngvason@gitlab.com>
Reviewed-by: karthik nayak <knayak@gitlab.com>
Co-authored-by: Hordur Freyr Yngvason <hfyngvason@gitlab.com>
|
|
repository: Add ObjectsSize RPC to calculate fine-grained objects size
Closes #5010
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5980
Merged-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Reviewed-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Reviewed-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
|
This grpc version introduces some new features for client-side
load-balancers and some internal implementation changes. Generally, new
changes don't affect the functionalities of Gitaly's DNS load-balancer.
New features are hidden behind configurations. The only impact to us is
that the connection doesn't wait for the resolver to finish. That leads
to leaked goroutines in the test suite. This problem was fixed in a
prior commit.
Release notes: https://github.com/grpc/grpc-go/releases/tag/v1.56.0
Changelog: added
|
|
Use HeadReference instead of GetDefaultBranch
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5988
Merged-by: Justin Tobler <jtobler@gitlab.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>
Reviewed-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Reviewed-by: Justin Tobler <jtobler@gitlab.com>
Co-authored-by: James Fargher <jfargher@gitlab.com>
|
|
operations: Replace git2go merge in squash with git-merge-tree
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5985
Merged-by: Justin Tobler <jtobler@gitlab.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>
Reviewed-by: Justin Tobler <jtobler@gitlab.com>
Reviewed-by: karthik nayak <knayak@gitlab.com>
Co-authored-by: John Cai <jcai@gitlab.com>
|
|
go: Update module github.com/urfave/cli/v2 to v2.25.7
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5942
Merged-by: Sami Hiltunen <shiltunen@gitlab.com>
Approved-by: Sami Hiltunen <shiltunen@gitlab.com>
Co-authored-by: GitLab Renovate Bot <gitlab-bot@gitlab.com>
|
|
UserSquash only contains one git2go call, a merge. We already have
a helper function merge() that uses Git plumbing commands. Use a feature
flag to gate switching the merge over to the git plumbing commands to
remove all Git2Go usage in this RPC.
|
|
This can be used to guard against races in composite git operations that
act as a single user-facing operation. The motivation here is the
construction of merge request refs using "semi-linear" merges with
automatic rebase prior to the creation of the merge commit.
See https://gitlab.com/gitlab-org/gitlab/-/issues/26996.
Changelog: changed
|
|
merge-tree: Fix length checking for conflict parsing
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5992
Merged-by: karthik nayak <knayak@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Reviewed-by: karthik nayak <knayak@gitlab.com>
Co-authored-by: Xing Xin <xingxin.xx@bytedance.com>
|
|
In order to calculate a repository's size we provide multiple different
functions. All of these have in common that they return the on-disk of
various data structures in varying degrees of detail. But none of them
provide the caller with the means to calculate the size of objects which
are reachable from a starting set of revisions. This results in multiple
problems:
- It is hard to calculate the size for subsets of the object graph,
e.g. for only newly pushed objects or to exclude references that
are internal, only.
- While `RepositorySize()` discerns normal objects from those which
are currently waiting to be pruned via cruft packs, this metric is
lagging behind significantly as cruft packs are only updated every
few days.
- Objects that exist in multiple packfiles or both as a packed and
loose object will be accounted for multiple times.
- It is impossible to figure out whether a subset of objects is
deduplicated via object pools.
This information can be quite important in certain contexts though, e.g.
when trying to calculate storage size quotas.
Implement a new `ObjectsSize()` RPC that calculates the size of objects
reachable from a given set of (pseudo-)revisions via git-rev-list(1).
This is as accurate as we can get and allows for determining the size of
objects for various usecases:
- The size of a single branch (`refs/heads/master`).
- The size of all references (`--all`) or branches (`--branches`).
- The size of new objects in a push (`$new_tips --not --all`).
- The size of objects which are not deduplicated in an object
deduplication network (`--all --not --alternate-refs`).
- The size of objects which are deduplicated in an object
deduplication network (`--alternate-refs`).
This RPC is thus both as accurate as possible while also being quite
flexible. It comes with the downside though that doing the graph walk to
figure out reachable objects is quite expensive depending on both the
number of references and objects. This cannot really be helped though:
the caller needs to choose between either getting fast but coarse or
slow but accurate results.
Changelog: added
|
|
Add `--alternate-refs` to the list of allowed pseudo-options so that it
becomes possible to include or exclude all references present in any
alternate Git object directories.
|
|
The git-rev-list(1) command is somewhat special in that it accepts
pseudo-options as regular arguments. That is, next to revisions like
`refs/heads/main`, it also accepts pseudo-options like `--all` or
`--branch`. These need to be intermixed as the order of arguments is
indeed important when pseudo-options like `--not` are passed in, as
well.
We have thus special-cased git-rev-list(1) to accept a subset of such
pseudo-options via arguments even though we normally reject positional
arguments which start with a dash. The list of accepted pseudo-options
is hardcoded so that we indeed only accept actual pseudo-options and no
actual options that would change behaviour of the command.
We have unified validation of these options recently in c1b8d6a09 (git:
Optionally allow pseudo-revisions in `ValidateRevision()`, 2023-06-05),
but forgot to also update this location. Convert it so that the list of
accepted pseudo-options needs to be maintained in a single location,
only.
|
|
Implement RestoreRepository RPC
Closes #4939
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5968
Merged-by: Sami Hiltunen <shiltunen@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Approved-by: Sami Hiltunen <shiltunen@gitlab.com>
Reviewed-by: Sami Hiltunen <shiltunen@gitlab.com>
Reviewed-by: karthik nayak <knayak@gitlab.com>
Co-authored-by: James Fargher <jfargher@gitlab.com>
|
|
praefect: Introduce `GetObjectPoolHandler()`
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5928
Merged-by: Sami Hiltunen <shiltunen@gitlab.com>
Approved-by: Sami Hiltunen <shiltunen@gitlab.com>
Reviewed-by: Sami Hiltunen <shiltunen@gitlab.com>
Co-authored-by: Justin Tobler <jtobler@gitlab.com>
|
|
featureflag: Remove Git v2.41 flag
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5976
Merged-by: karthik nayak <knayak@gitlab.com>
Approved-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Reviewed-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Reviewed-by: Justin Tobler <jtobler@gitlab.com>
Co-authored-by: Justin Tobler <jtobler@gitlab.com>
|
|
The length checking for conflicting file parsing can not protect us from
a out-of-range panic if we received malformed output from git, though it
can hardly happen.
Signed-off-by: Xing Xin <xingxin.xx@bytedance.com>
|
|
Bump Go to 1.20.5
Closes #5416
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5987
Merged-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: Will Chandler <wchandler@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Co-authored-by: Robert Marshall <rmarshall@gitlab.com>
|
|
|
|
The restore logic itself is already tested with backup.Manager. The
tests here are only smoke tests and to ensure that the vanity repository
works through praefect.
|
|
|
