| Age | Commit message (Collapse) | Author |
|---|
|
|
|
This reverts commit 4666f8afdd6a005034af3882eb24ecc23e86a670, reversing
changes made to 1a82d06e41054952e957b97e62325ea2853382ab.
|
|
Fix 64-bit alignment errors with request queue counters
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5223
Merged-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Reviewed-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Co-authored-by: Stan Hu <stanhu@gmail.com>
|
|
In 7c5bbf61, the addition of the `ProtoFormat` field to `ObjectHash`
increased the number of bytes in the structure from 64 to 72. As a
result, `requestQueue` now looks like:
```go
type requestQueue struct {
objectHash git.ObjectHash // 72 bytes
outstandingRequests int64 // 8 bytes
closed int32 // 4 bytes
isReadingObject int32 // 4 bytes
isObjectQueue bool // 1 byte
stdout *bufio.Reader // 8 bytes
stdin *bufio.Writer // 8 bytes
trace *trace // 8 bytes
}
```
Previously `requestQueue` totaled 112 bytes, which is 64-bit
aligned. However, the addition of this new field in `objectHash`
pushes the structure to 113 raw bytes. On a Raspberry Pi system, the
Go compiler only guarantees 32-bit alignment, so we end up with a
structure that is 116 bytes, which isn't 64-bit aligned.
Moving `outstandingRequests` to be first in the struct isn't
enough. As https://github.com/golang/go/issues/11891#issue-97544684
says, in an array of structs, only the first element of a structure
may be automatically 64-bit aligned. It depends if the size of each
element, plus padding, is a multiple of 8.
To fix this mess, we now split these atomic counters in its own
structure and add a test to ensure that it is 64-bit aligned.
Relates to https://gitlab.com/gitlab-org/gitaly/-/issues/4701
Changelog: fixed
|
|
structerr: Fix error metadata log field colliding with normal error
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5220
Merged-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Reviewed-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
|
When the `structerr.FieldsProducer` is installed as a gRPC interceptor
then we append error metadata part of the `structerr` error to the
logrus fields. This is done by setting the "error.metadata" field, but
this collides with the normal "error" field: if both are set they would
override each other as we serialize logs into JSON.
Fix this by changing the metadata field to use "error_metadata" as key.
Changelog: fixed
|
|
git/objectpool: Remove remote configuration after creation
Closes #4384
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5199
Merged-by: Sami Hiltunen <shiltunen@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
|
go: Update module golang.org/x/time to v0.3.0
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5214
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: Bump minimum required Git version to v2.38.0
Closes #4672
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5210
Merged-by: karthik nayak <knayak@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
|
Update Markdown-related Ruby gems
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5213
Merged-by: Toon Claes <toon@gitlab.com>
Approved-by: Toon Claes <toon@gitlab.com>
Co-authored-by: Stan Hu <stanhu@gmail.com>
|
|
repository: modernize the tests for `FetchSourceBranch`
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5203
Merged-by: karthik nayak <knayak@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Reviewed-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Reviewed-by: Will Chandler <wchandler@gitlab.com>
Reviewed-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Reviewed-by: karthik nayak <knayak@gitlab.com>
|
|
Currently `FetchSourceBranch` doesn't use a quarantined repository.
Because of this, if there is a failure mid-way and refs are already
pulled, we don't discard these refs. The fix for this is to move to
using a quarantined repository. But before we do that, let's add a test
to verify this behavior. When we do eventually fix this with quarantined
repositories, this test would fail (test-driven development).
|
|
[ci skip]
|
|
These are all sub-dependencies of `activesupport` and aren't really
used in Gitaly. However, let's update the versions to match the GitLab
Rails versions and prevent security scanners from flagging CVEs.
Changelog: changed
|
|
operations: Use `ExpectedOldOid` in `UserRevert`
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5182
Merged-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Reviewed-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Reviewed-by: karthik nayak <knayak@gitlab.com>
Co-authored-by: Karthik Nayak <knayak@gitlab.com>
|
|
version: Make builds deterministic by dropping build time
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5201
Merged-by: Pavlo Strokov <pstrokov@gitlab.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>
Approved-by: Pavlo Strokov <pstrokov@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
|
Git v2.38.0 brings some important changes with it that we want to build
on:
- It fixes a source of corruption when upgrading commit-graphs that
contain generation data in v1 to v2. We currently had to disable
use of generation data completely because of that bug so that we
don't corrupt repositories. This information is important though
to speed up some computations, like for example in the packfile
negotiation.
- git-merge-tree(1) learned two merge two trees. This is important
to allow us to do in-memory merges.
- git-cat-file(1) starts to support mailmaps, which is important for
us to support them in Gitaly.
- git-clone(1) has clearned about bundle URIs, which allows
offloading parts of a clone to a CDN.
Bump the minimum required Git version to v2.38.0.
Changelog: changed
|
|
We're about to bump the minimum required Git version to v2.38.0. Adapt
our CI to match this.
|
|
We have removed the infrastructure to use bundled Git v2.37 in v15.7,
but retained the binaries for backwards compatibility. Remove them.
|
|
helper: Make `GrpcCode()` helper part of the `structerr` package
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5196
Merged-by: Pavlo Strokov <pstrokov@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Approved-by: Pavlo Strokov <pstrokov@gitlab.com>
Reviewed-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Reviewed-by: karthik nayak <knayak@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
|
[ci skip]
|
|
Add new GetPatchID RPC
Closes #4612
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5174
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: Sincheol (David) Kim <dkim@gitlab.com>
Co-authored-by: David Kim <dkim@gitlab.com>
|
|
|
|
Currently the tests for `FetchSourceBranch` use seeded repositories and
spread across multiple smaller tests. Combine all these tests using
table-driven tests and remove seeded repositories. This makes it easier
to extend the tests.
|
|
go: Update module golang.org/x/sys to v0.3.0
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5204
Merged-by: Toon Claes <toon@gitlab.com>
Approved-by: Toon Claes <toon@gitlab.com>
Co-authored-by: GitLab Renovate Bot <gitlab-bot@gitlab.com>
|
|
operations: Use `ExpectedOldOid` in `UserApplyPatch`
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5132
Merged-by: Toon Claes <toon@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: Toon Claes <toon@gitlab.com>
Reviewed-by: Sami Hiltunen <shiltunen@gitlab.com>
Reviewed-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Reviewed-by: karthik nayak <knayak@gitlab.com>
Co-authored-by: Karthik Nayak <knayak@gitlab.com>
|
|
operations: Use `ExpectedOldOid` in `UserFFBranch`
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5173
Merged-by: Toon Claes <toon@gitlab.com>
Approved-by: Will Chandler <wchandler@gitlab.com>
Approved-by: Toon Claes <toon@gitlab.com>
Reviewed-by: Will Chandler <wchandler@gitlab.com>
Reviewed-by: karthik nayak <knayak@gitlab.com>
Co-authored-by: Karthik Nayak <knayak@gitlab.com>
|
|
housekeeping: Enable testing with SHA256 object format
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5191
Merged-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>
|
|
Patch IDs are a unique ID computed by hashing a patch with some parameters like line numbers ignored.
The patch ID can thus be used to compare whether diffs make the same change.
Please refer to git-patch-id(1) for further information.
|
|
objectpool: Fix error code when trying to recreate existing pool
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5202
Merged-by: Will Chandler <wchandler@gitlab.com>
Approved-by: Will Chandler <wchandler@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
|
tools/goimports: Update module golang.org/x/tools to v0.4.0
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5205
Merged-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: James Fargher <proglottis@gmail.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Co-authored-by: GitLab Renovate Bot <gitlab-bot@gitlab.com>
|
|
commit: Don't put tree entry name into error message
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5200
Merged-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Reviewed-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
|
With the recent refactorings to create object pools atomically the error
code has changed when the target exists from `FailedPrecondition` to
`AlreadyExists`. While the latter is arguably better, Rails' rspecs
still expect the former error code.
Fix this by overriding the error code so that for the time being we
abide and use whatever Rails wants to see. Eventually though we should
fix this to return `AlreadyExists` so that we're consistent with all the
other repository-creating functions.
Changelog: fixed
|
|
We don't have a service-level test that tries to recreate an already
existing object pool. Because of this we didn't catch that the error
code has changed when converting to atomically create object pools from
`FailedPrecondition` to `AlreadyExists`.
Add a testcase to catch this regression in the future.
|
|
The `structerr` package propagates error codes of wrapped errors so that
we by default retain the most specific one. While this works in general,
there may be cases where we explicitly want to override the error code
with a different one.
Add a new option `WithGRPCCode()` that allows us to do so.
|
|
Before converting to the new `WithGoBuildInformation()` monitoring
option in the preceding commit we always exposed the build time. We have
stopped doing that now and instead only expose information that Go puts
into the binary itself.
As a result, we can now drop the infrastructure to embed the build time
into the binary. This has the advantage that it significantly improves
incremental build times: previously we'd basically have to rebuild all
binaries every single time, as the embedded build time changes every
second. As we stop doing that we can in many cases skip this step. On my
machine this leads to a 3.5x build speedup.
Benchmark 1: make
Time (mean ± σ): 12.163 s ± 0.148 s [User: 16.677 s, System: 3.496 s]
Range (min … max): 12.044 s … 12.328 s 3 runs
Benchmark 2: make GO_LDFLAGS=
Time (mean ± σ): 3.469 s ± 0.060 s [User: 5.593 s, System: 1.880 s]
Range (min … max): 3.433 s … 3.539 s 3 runs
Summary
'make GO_LDFLAGS=' ran
3.51 ± 0.07 times faster than 'make'
This has been benchmarked with the preceding commit which still embedded
the build time into the binary. By stubbing out the `GO_LDFLAGS`, which
is how we put the build time into the resulting binary, we are able to
emulate the new behaviour with this commit.
But besides the quality-of-life improvements for developers another
significant advantage is that we now have reproducible builds.
Changelog: fixed
|
|
Starting with LabKit v1.17.0 there is a new `WithGoBuildInformation()`
option that uses the `debug.BuildInfo` structure provided by Go to
derive information about the executable. This new infrastructure
replaces `WithBuildInformation()` and exposes additional information
that was not previously exposed:
- The Go version we're running at.
- Whether sources have been modified.
- The module path.
- The exact commit ID that was built.
Convert Gitaly, Praefect and our blackbox to use this new build option
instead of the old infrastructure. Note that while we only conditionally
pass the option in case `ReadBuildInfo()` returns `true`, Go always
embeds the build information into binaries when building with modules
support enabled. So in practice it should always be present.
|
|
Praefect uses its own wrapper functions to derive version information,
but ultimately it really only shells out to `internal/version`. The only
difference is that Gitaly would return a "Gitaly"-prefixed string while
Praefect returns a "Praefect"-prefixed string.
Deduplicate this infrastructure by adding a new parameter to the
`GetVersionString()` function.
|
|
Bump the LabKit dependency to v1.17. It only includes a single change,
which is a new option for the monitoring infrastructure to obtain build
information from the new `debug.BuildInfo` structure introduced with Go
1.18.
|
|
If the clients provide the `ExpectedOldOid` in the request of
`UserFFBranch`, we need to use this as the ref's current OID for
updating the reference.
This is directly fed to git-update-ref(1) where it'll exit with an error
code if the current OID of the ref doesn't match the OID provided by us.
This allows us to avoid any race conditions wherein the branch was
updated concurrently by another process.
|
|
When creating object pools we do so by cloning the primary object pool
member. As git-clone(1) creates remote configuration the gitconfig of
the pool repository will afterwards contain the `remote.origin` remote.
We have dropped storing remote configuration in repositories a long time
ago though and clean up any such config entries during housekeeping.
Explicitly remove the remote after we have created the object pool.
Changelog: fixed
|
|
When checking our production logs for gitlab.com, the top-most error
message we see is that a specific tree entry was not found in the
`TreeEntry()` RPC. This error includes the path of the tree entry that
wasn't found, which makes it impossible to deduplicate this error in
Sentry.
Convert the error to use structured metadata instead to fix this. This
also serves as a test balloon to verify that we indeed propagate the
gRPC error metadata as expected in production systems.
Changelog: fixed
|
|
The `structerr` package already handles gRPC error code propagation in
the way the `helper.GrpcCode()` helper function does it: upon wrapping
an error with a preexisting error code we retain the code and propagate
it further up the error chain. As such, `helper.GrpcCode()` duplicates
error code handling logic of the `structerr` package.
Move the helper into the `structerr` package and simplify its logic so
that it just builds on top of it. Instead of manually iterating through
the error chain and figuring out what the error code is supposed to be,
we now just unwrap the error to an error that implements `GRPCStatus()`.
Like this we can either:
- See a `grpc/status.Status()` error. As these errors cannot wrap
any other errors anyway we have no other choice but to return its
error code.
- See a `structerr.Error`, which already knows to propagate error
codes up the callchain properly.
So in both cases we indeed retain the current semantics.
|
|
We only assert error codes in the ReferenceTransactionHook test.
Refactor it to instead assert the complete returned error as well as the
response.
|
|
We only assert error codes in some of the Namespace service tests.
Refactor them to instead assert the complete returned error as well as
the response.
|
|
Simplify the test setup for the `RenameNamespace()` RPC. While we set up
the logic to iterate over multiple testcases and execute some subset of
the verification conditionally on the testcase, we only have a single
testcase anyway.
I don't expect that we'll extend the namespace service at any point in
time anymore as we'd rather want to remove it altogether. So let's
refactor the test to not have the boilerplate anymore.
|
|
Fully verify errors returned by `Path()` in some of our testcases
instead of only verifying the gRPC error code.
|
|
Refactor tests for the `RefExists()` RPC to match our modern coding
style. Furthermore, verify returned errors more strictly by not only
matching the error code, but the complete error.
|
|
Remove tests that check functionality that is already tested in other
tests. For both removed tests we already have `Test$RPC_validate` tests
that verify behaviour with empty requests.
|
|
When creating repositories Git will install files present in a specific
templates directory into the newly created repository. This includes
some stubs like `info/exclude` and `description`, but also a set of
sample hooks. While these files are tiny on their own, they do add up
when installed across millions of repositories. We have thus overridden
`init.templateDir` in bd0f89954 (git: Do not install templates when
creating repos, 2021-10-28) so that we don't install these templates
anymore.
One left-over bit from before that time is that we still manually delete
the `hooks` directory when creating object pools. This is not necessary
anymore though, so let's drop this logic. Note that we have preexisting
tests that verify there is indeed no `hooks` directory after creating
the object pool in `TestCreate/successful`.
|
