| Age | Commit message (Collapse) | Author |
|---|
|
We have statushandler middleware that changes status code of the RPC
depending on different conditions. One of such conditions is the
syscall.EMFILE error. This test is added to make sure this particular
error is returned when there is no more file descriptors available.
Otherwise, the statushandler middleware needs to be adjusted to catch
the proper error.
Part of: https://gitlab.com/gitlab-org/gitaly/-/issues/4688
|
|
When Gitaly has troubles with acquiring more file descriptors from the
system it returns Internal or Unknown status codes back to the caller.
That doesn't tell the caller anything meaningful, so the caller re-
attempts the operation. Re-attempts create additional load on the
Gitaly. What we want instead is a backoff that will allow Gitaly to
handle in process requests and lower file descriptors usage. That is
why now Gitaly returns ResourceExhausted status code in case it
encounters a problem with file descriptors.
Closes: https://gitlab.com/gitlab-org/gitaly/-/issues/4688
|
|
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>
|
|
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>
|
|
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 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.
|
|
objectpool: Enable atomic creation of object pools
Closes #4385
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5187
Merged-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Reviewed-by: Justin Tobler <jtobler@gitlab.com>
Reviewed-by: karthik nayak <knayak@gitlab.com>
|
|
If the clients provide the `ExpectedOldOid` in the request of
`UserRevert`, 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.
|
|
There are a few TestServer_UserRevert* tests which do similar things,
but are outdated and use seeded repos. In preparation of the upcoming
additions to UserRevert, let's merge these tests.
This makes it easy to add new tests and also no longer uses seeded
repos, making it easier to also migrate to SHA256.
|
|
localrepo: Use catfile cache in ReadObject
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5158
Merged-by: Toon Claes <toon@gitlab.com>
Approved-by: Will Chandler <wchandler@gitlab.com>
|
|
Enable testing with the SHA256 object format in remaining tests.
|
|
Enable testing the optimization strategy with SHA256. While we had the
logic in place to make object sizes conditional on the object hash, we
have since started to write empty trees in `gittest.WriteCommit()`. As a
result the object size has increased.
Fix these stale object sizes and enable testing with SHA256.
|
|
Refactor stale-data-related tests to not use the seed repository anymore
and enable testing with SHA256.
|
|
Refactor worktree-related tests to not use the seed repository anymore
and enable testing with SHA256.
|
|
When converting the localrepo package to support SHA256 as object format
we couldn't yet convert the remote extra tests. This was because they
relied on creating the repository via the `CreateRepository` RPC, but
the RPC didn't yet support creating SHA256 repositories.
This has since been implemented, so let's enable these tests.
|
|
We introduced the ability to use `gittest.CreateRepository()` back when
the `git.ObjectHash` didn't yet have a `Format` field. We thus have a
separate field that tracks the extra arguments required for git-init(1)
to set up the repository with the expected object format.
This is not required anymore as we can now just reuse the `Format`, so
let's simplify the code.
|
|
Creating repositories can be a complex task that involves multiple
steps. We must ensure though that if any of the steps fails that we
don't leave behind any cruft on disk in the form of a partially
initialized repository. This is why we have in the past refactored our
repository-creating RPCs to use a helper that knows to do this in
multiple steps:
1. The repository gets created in a temporary directory.
2. The temporary repository gets seeded with the data we want to put
into it.
3. We optionally vote on the resulting repository so that we can be
sure that any replicas have created the same repository.
4. When we see that everything is fine we move the final repository
into the target location.
This ensures that we only ever see either fully-initialized repositories
or no repository at all in the target location after a repo-creating RPC
finishes.
Object pools don't yet use that machinery though. But there is no other
reason than object pools frequently being an afterthought in Gitaly. So
let's use the same helper function to atomically create object pools.
Note that while the helper also supports voting onthe result we don't
yet wire up that functionality: transactions for `CreateObjectPool()`
are not enabled in Praefect. We will eventually do this in another
iteration.
Changelog: fixed
|
|
While we already try to resolve the storage path right now and thus fail
with an error in case the storage is not known, we don't do this for the
object pool path yet. While we check whether the object pool path is a
valid path at a later point anyway, we will require the path before
doing that check in a subsequent commit. As a consequence, the error
returned would fail in one of our tests.
Refactor the code to ask for the repository path early on so that we
don't need to change tests when introducing new logic. The new behaviour
could be claimed to be better than before as we now return a proper
error in case the path is misformed. But the previous behaviour was just
fine, too.
|
|
We don't thoroughly verify arguments passed to the `CreateObjectPool()`
RPC call. As a result, in some scenarios we fail with not-well-defined
semantics while already busy creating the repository. This creates
problems when we refactor the code to create repositories atomically as
the exact way how we fail would be different.
Add more preliminary checks to verify that arguments are what we expect
them to look like.
|
|
The `Create()` helper will make sure to call the repository-seeding
function with an already initialized Git repository so that we can skip
the step of having to call git-init(1) at all the callsites. While this
is generally useful, some of the users of `Create()` need to create the
target repository by calling git-clone(1), which isn't happy in case the
target directory exists and is not an empty directory. These callers
thus manually `os.RemoveAll()` the target directory right now before
doing the clone.
Provide a new option `WithSkipInit()` that allows callers to skip the
call to git-init(1). Convert callsites to use the new option.
|
|
All the options that can be passed to `Create()` can be represented as
simple `git.Option`s, which is why we a `CreateOption` is defined as
appending to an array of `git.Option`s. We're about to add another
option though that doesn't fit into this model.
Refactor the code to introduce a `createConfig` structure that for now
only has the array of Git options.
|
|
We have no tests that verify the behaviour of any of the existing
options for the `Create()` functions. Add some to plug this test gap.
|
|
We're about to add another caller for the `createRepository()` helper
functions so that we can also transactionally and atomically create
object pools. Let's thus move the function into its own package
`repoutil`.
Note that I'm not particularly happy with the resulting package
structure. But this is service-level functionality, which means that
moving it into `internal/git` is not an option. So let's not dwell too
long over it.
|
|
We're about to add another user for `createRepository()` that is outside
of the repository service. As a first step, convert the function to not
be a receiver function anymore so that we can move it into a different
package.
|
|
helper: Convert `ErrNotFoundf()` and `ErrInvalidArgumentf()`
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5194
Merged-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: James Fargher <proglottis@gmail.com>
|
