| Age | Commit message (Collapse) | Author |
|---|
|
Add MinOccurrences cache middleware
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5501
Merged-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Reviewed-by: Jacob Vosmaer <jacob@gitlab.com>
Reviewed-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Reviewed-by: Justin Tobler <jtobler@gitlab.com>
Co-authored-by: Jacob Vosmaer <jacob@gitlab.com>
|
|
Inject the MinOccurrences middleware into the streamcache call stack
and make it configurable via the
GITALY_PACK_OBJECTS_CACHE_MIN_OCCURRENCES environment variable.
This is a temporary configuration mechanism that will allow us to
validate the MinOccurrences middleware on GitLab.com quicker. If it
works as expected we will replace the env var with normal config.toml
configuration logic.
|
|
This adds a new streamcache middleware called MinOccurrences. Its
purpose is to only forward requests to the inner cache for keys that
have been seen a minimum number of times.
|
|
repository: Test refactorings and SHA256 object format support
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5508
Merged-by: Pavlo Strokov <pstrokov@gitlab.com>
Approved-by: Pavlo Strokov <pstrokov@gitlab.com>
Reviewed-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Reviewed-by: Pavlo Strokov <pstrokov@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
|
repository: Drop size calculations via git-cat-file(1)
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5511
Merged-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Approved-by: John Cai <jcai@gitlab.com>
|
|
We have three different ways to calculate the repository size:
- The plain old way of using du(1) on the repository. This is fast,
but also counts in repository metadata, unreachable objects and
duplicate objects.
- The implementation based on git-rev-list(1). It allows to only
count in objects that are reachable via specific references and is
overall the most flexible approach in how we calculate the size.
The downside of this is that it is really slow compared to du(h).
- The implementation based on git-cat-file(1). This has been added
as a compromise between the other two options so that we can at
least account for duplicate objects. It's faster than using
git-rev-list(1), but still slower than du(1).
While the latter two options have been implemented quite a while ago
now, we still haven't managed to roll them out due to performance
reasons. By now it is clear though that we can either choose between
the correct and flexible approach, or the fast and dirty approach. Which
means that in the long run, our target architecture is going to be the
approach based on git-rev-list(1).
This kind of leaves the third implementation based on git-cat-file(1) on
the chopping block. It was implemented after we've seen how slow the
git-rev-list(1) based approach is as a middle ground where we can at
least count out duplicate objects while being reasonably fast. But it
did not deliver on that promise as it still wasn't fast enough without
also changing the overall architecture of how repository sizes are
calculated. So this approach is disfavoured nowadays.
Remove the git-cat-file(1) based implementation. We are not going to use
it anyway.
Changelog: removed
|
|
go: Update module gocloud.dev to v0.29.0
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5486
Merged-by: John Cai <jcai@gitlab.com>
Approved-by: Pavlo Strokov <pstrokov@gitlab.com>
Approved-by: John Cai <jcai@gitlab.com>
Co-authored-by: GitLab Renovate Bot <gitlab-bot@gitlab.com>
|
|
|
|
go: Update module gitlab.com/gitlab-org/labkit to v1.18.0
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5492
Merged-by: John Cai <jcai@gitlab.com>
Approved-by: James Fargher <proglottis@gmail.com>
Approved-by: John Cai <jcai@gitlab.com>
Co-authored-by: GitLab Renovate Bot <gitlab-bot@gitlab.com>
|
|
go: Update golang.org/x/exp digest to 9ff063c
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5458
Merged-by: John Cai <jcai@gitlab.com>
Approved-by: Will Chandler <wchandler@gitlab.com>
Approved-by: John Cai <jcai@gitlab.com>
Co-authored-by: GitLab Renovate Bot <gitlab-bot@gitlab.com>
|
|
|
|
housekeeping: Keep timestamp of last full repack
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5527
Merged-by: Justin Tobler <jtobler@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
|
statushandler: Don't wrap context errors
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5530
Merged-by: Justin Tobler <jtobler@gitlab.com>
Approved-by: John Cai <jcai@gitlab.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>
Co-authored-by: Will Chandler <wchandler@gitlab.com>
|
|
When returning a `structerr`, we will check for wrapped errors and
return the inner gRPC code when present. However, with context errors we
want ensure that the `Canceled`/`DeadlineExceeded` statuses override any
previous generated codes.
Wrapping the errors caused elevated error rates on `InfoRefsUploadPack`
as the inner error of canceled processes was `Internal`.
Use '%v' in our format strings for context-related errors so that we
return the expected status.
Changelog: fixed
|
|
repository: Refactor RepositorySize to not rely on du(1)
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5517
Merged-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Reviewed-by: James Fargher <proglottis@gmail.com>
Reviewed-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
|
Add a new metric to the housekeeping manager that tracks the time since
the last full repack. This metric will gives us a better picture of how
often we're currently doing full repacks. Equipped with this information
we can eventually come up with a good estimate of how often we should be
doing full repacks in a geometric-repacking world.
|
|
We have started writing the timestamp of the last full repack into
repositories. Let's start tracking it via the git/stats package so that
this information becomes available via logs.
|
|
At the moment, we can keep track of the last time a full repack has
happened in a repository by simply taking the timestamp of the oldest
packfile in a repository. This only really works because we have a split
between full and incremental repacks.
This is about to change soonish though once we implement support for
geometric repacking of repositories. In geometric repacking, we don't
control anymore whether old packfiles will get rewritten or not, but
instead we shift that burden to Git. So the oldest packfile in the
repository may have been written either by a full repack, or by a
geometric repack that decided to rewrite the oldest packfile.
Now why do we care for this? The problem is that once we move towards
geometric repacking, we still need to make sure that we perform a full
repack every once in a while. This full repack will then be responsible
for moving unreachable objects into a separate cruft pack so that we can
still properly prune objects from repositories. But as we will have no
direct control over the number of packfiles in a repository anymore, the
current heuristic that uses the number of existing packfiles won't work
anymore to decide whether we need to perform a full repack or not.
Instead, we'll be moving to a time-based heuristic where we decide to do
a full repack every once in a while, e.g. daily. This will ensure that
we perform geometric repacks most of the time but still move unreachable
objects out into cruft packs on a schedule that is easy to understand
and explain.
As mentioned though, we are not in a position to derive the last time
such a full repack has happened. To fix this, introduce a new timestamp
file ".gitaly-full-repack-timestamp" that we write into the repository
every time we are about to perform a full repack.
Changelog: added
|
|
gitaly: Fix transaction manager test flake
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5469
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: Justin Tobler <jtobler@gitlab.com>
|
|
One of the ways to calculate the repository size is to simply invoke the
du(1) executability to calculate the size for us. This does not make a
ton of sense though:
- It adds a dependency on an external program.
- This external program may behave differently depending on the
platform.
- We need to spawn an executable, which can be expensive at times.
- Go already provides us with an easy way to implement the
functionality ourselves via `filepath.WalkDir()`.
Refactor the code to implement the functionality ourselves. It is more
efficient as we don't need to spawn an external process, easier to
understand, has less platform-specific behaviour and has less external
dependencies. Furthermore, it is also faster especially in cases where
the repository is small, where the overhead induced by having to spawn
an external process can be felt.
The following benchmarks demonstrate this:
```
BenchmarkRepositorySize/disk-usage_with_du
BenchmarkRepositorySize/disk-usage_with_du/empty_repository
BenchmarkRepositorySize/disk-usage_with_du/empty_repository-12 552 3110944 ns/op
BenchmarkRepositorySize/disk-usage_with_du/benchmark_repository
BenchmarkRepositorySize/disk-usage_with_du/benchmark_repository-12 1 1389975933 ns/op
BenchmarkRepositorySize/disk-usage_with_walk
BenchmarkRepositorySize/disk-usage_with_walk/empty_repository
BenchmarkRepositorySize/disk-usage_with_walk/empty_repository-12 4200 302115 ns/op
BenchmarkRepositorySize/disk-usage_with_walk/benchmark_repository
BenchmarkRepositorySize/disk-usage_with_walk/benchmark_repository-12 1 1393811464 ns/op
```
As said, especially the case with empty repositories is a lot faster
with the new implementation, but the returns start to diminish once
repositories get bigger. I also very much expect that at some point, the
du(1)-based implementation would again take the lead. But as we strive
to keep repositories well-maintained, they shouldn't have too many files
in the general case anyway. And even if the du(1)-based implementation
was faster at some point, this does not yet take into account global
contention created around the command spawning token that we need to
acquire in order to spawn du(1).
Note that due to different rounding behaviour between du(1) and our own
implementation, we have to increase the size of test data we are writing
in one of our tests.
Changelog: changed
|
|
structerr: Convert `New()` to return `Unknown` error codes
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5521
Merged-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
|
testhelper: Fix certificate being generated for the wrong entity
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5519
Merged-by: karthik nayak <knayak@gitlab.com>
Approved-by: Pavlo Strokov <pstrokov@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
|
We're about to change the implementation of the disk-usage-based
RepositorySize calculations from spawning du(1) to in-process. Implement
a benchmark so that we can measure the impact of this.
|
|
gitaly: Do not start gitaly-ruby
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5497
Merged-by: Justin Tobler <jtobler@gitlab.com>
Approved-by: Will Chandler <wchandler@gitlab.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>
Co-authored-by: Toon Claes <toon@gitlab.com>
|
|
git/stats: Fix flaky timings in HTTP push tests
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5524
Merged-by: Justin Tobler <jtobler@gitlab.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
|
featureflag: Default `tx_restore_custom_hooks` on
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5522
Merged-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Co-authored-by: Justin Tobler <jtobler@gitlab.com>
|
|
We're manually plugging together different parts of the repository
service server even though we have a helper function that performs all
of the necessary steps for us already. Convert the tests to use it
instead to simplify the setup.
|
|
Remove the `!gitaly_testa_sha256` build tag so that we can start testing
the CreateFork RPC with the SHA256 object format.
|
|
Two of the Createfork tests are still using seeded repositories even
though they don't really depend on any of their data. Convert the tests
to use unseeded repositories so that we can start testing with the
SHA256 object format.
|
|
In order to have CreateFork connect to another Gitaly node we need to
write connection information into the gRPC context. In some of our tests
though we leak that information across different testcases. While not
really a problem, it only works correctly by chance.
Fix this by converting the leaking contexts to be testcase-cocal.
|
|
One of the tests for CreateFork verifies that things work as expected
when connecting to a server that is listening on an encrypted TLS
socket. This test is quite complex as it creates the certificate, sets
up a certificate pool, and explicitly connects via the TLS listen
address with transport credentials.
But ultimately, this complexity is not needed at all as we use
`client.Dial()` to connect to the server, which automatically handles
the setup of the transport credentials when connecting to a TLS-based
socket. And as we set up the `SSL_CERT_FILE` environment variable
already so that it points to the generated certificate, it already gets
picked up by the `client` package via the `x509.SystemCertPool()`.
Simplify the test and remove all the needless setup.
|
|
The gittest package needs to connect to the gRPC server when either
creating repositories via the Repository service or when retrieving the
replica path of a repository. Both of these usecases are implemented via
`dialService()`, which right now only supports connecting to a server
that is listening on its socket path. This restricts the usefulness of
this option for tests which use a different server configuration.
Improve the logic to also support dialing of servers that listen on
either the TCP or the TLS socket. While at it, mark the function as a
test helper.
|
|
Part of what the HTTP push statistics provide is the information at
which time we have entered specific sections. As these timings naturally
fluctuate, the test for this only asserts that we have strict ordering
of these times. This is flaky though:
=== FAIL: internal/git/stats TestPerformHTTPPush/branch_deletion (0.40s)
http_push_test.go:190:
Error Trace: /builds/gitlab-org/gitaly/internal/git/stats/http_push_test.go:190
Error: Should be true
Test: TestPerformHTTPPush/branch_deletion
Messages: expected to receive "response-body" packet before before "2023-03-16 14:00:22.746688075 +0000 UTC m=+5.430460188", but received at "2023-03-16 14:00:22.746688075 +0000 UTC m=+5.430460188"
The issue here is that it can happen that two packages are seemingly
received at the exact same time, but because we require times to be
strictly increasing the assertion fails.
Fix this flake by using `require.GreaterOrEqual()` instead.
|
|
catfile: Add encoding information to GitCommit
Closes #836
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5506
Merged-by: Will Chandler <wchandler@gitlab.com>
Approved-by: Will Chandler <wchandler@gitlab.com>
Reviewed-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Co-authored-by: arkn98 <2424696-arkn98@users.noreply.gitlab.com>
|
|
Currently the `propose returns if context is canceled before admission`
subtest in `TestTransactionManager` is flaky due to a race between
context cancellation and tranaction admission into the manager. To
resolve this, `SkipStartManager` is added to the step configuration to
run the subtest without starting the transaction manager. This prevents
transactions from being admitted to the manager and requires the context
to be cancelled before the test continues. With the race condtion
resolved, the quarantine is removed from the test.
Also the `StartManager` field is renamed to `RestartManager` to improve
the readability of the test since it more accurately reflects what is
happening during test execution.
|
|
docs: Document Gitaly major module version
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5493
Merged-by: John Cai <jcai@gitlab.com>
Approved-by: Evan Read <eread@gitlab.com>
Reviewed-by: Evan Read <eread@gitlab.com>
|
|
We don't need gitaly-ruby no more, so no longer start it when Gitaly is
launched.
Epic: https://gitlab.com/groups/gitlab-org/-/epics/7874
|
|
tools/goimports: Update module golang.org/x/tools to v0.6.0
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5377
Merged-by: John Cai <jcai@gitlab.com>
Approved-by: Sami Hiltunen <shiltunen@gitlab.com>
Approved-by: John Cai <jcai@gitlab.com>
Co-authored-by: GitLab Renovate Bot <gitlab-bot@gitlab.com>
|
|
tools/golangci-lint: Update module golang.org/x/tools to v0.7.0
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5504
Merged-by: John Cai <jcai@gitlab.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>
Approved-by: John Cai <jcai@gitlab.com>
Co-authored-by: GitLab Renovate Bot <gitlab-bot@gitlab.com>
|
|
The `structerr.New()` constructor returns errors with an `Internal`
code. This makes for a somewhat weird interface though: `New()` shall be
used in cases where the actual error condition is not quite clear, but
by already setting an `Internal` error code we'll ultimately propagate
that code even if an ancestor in the callchain would know better.
Adjust the function to instead return `Unknown` errors. Together with
the change to always override `Unknown` errors with more specific ones,
this makes for a new calling convention of how to construct errors:
always use `New()`, unless you know the actual root cause. Like this,
the first caller that knows more about the root cause will override the
`Unknown` error.
Document this calling convention.
|
|
When wrapping errors with a gRPC code, then we always propagate the code
of the wrapped error. This ensures that we always return the most
specific error code, as you would usually know best about the exact
circumstances deep down the call chain.
This behaviour does not quite make sense with `Unknown` error codes
though, which by definition aren't specific. So let's change the error
semantics so that we always override `Unknown` error codes if we see
them.
Note that this requires a bunch of changes to our tests. Those tests
aren't really all that realistic though: none of our production code
ever generates `Unknown` errors intentionally, and in the case where we
return an error from gRPC services that doesn't have an error code yet
we'll instead convert them to `Internal` errors. These tests are thus
fixed by explicitly returning `Internal` errors there.
|
|
Remove the `NewUnknown()` constructor. There are no users of this
function, and explicitly creating errors with `Unknown` error codes does
not make a lot of sense.
Instead, we're about to change semantics of the `New()` constructor to
return `Unknown` error codes that would always get overridden by more
specific error codes when wrapped.
|
|
Transactionality is being added to the `SetCustomHooks` RPC via the
`tx_restore_custom_hooks` feature flag. This RPC will be invoked with a
CLI client by GitLab administrators to set custom hooks for a
repository. Since this CLI originates the RPC invocations, feature flag
values are not propagated through the RPC context. This change default
enables the feature flag so tranactions can be enabled for the
`set-hooks` CLI.
|
|
In one of the tests for ApplyGitattributes we're stubbing the
transaction service with a custom voting function. This voting function
is returning a plain error, which ultimately gets converted into an
`Unknown` error. This is something that typically wouldn't happen in
production systems though as all `Unknown` errors returned by gRPC
services would be converted to `Internal`.
Convert the error to instead have an explicit error code. This prepares
for an upcoming change where we modify error code semantics for errors
with an `Unknown` error code.
|
|
When writing tags via `localrepo.WriteTag()`, we have a safety check
that checks whether the newly written tag is formatted correctly. The
intent of the check is mostly to verify that it's impossible to inject
tag fields via any of the parameters by checking for additional newlines
that shouldn't actually be there.
In case this check fails we return an error with `Unknown` type and a
Ruby-esque error message. Let's stop doing that though -- the calling
side is never checking for this exact error anyway. And given that this
error condition indicates that the parameters passed by the caller are
invalid, let's also give it a proper error code.
Changelog: changed
|
|
The `GenerateCerts()` helper function generates two certificates:
the root certificate that acts as certificate authority, and the entity
certificate that is signed by the root certificate. The server should
then ultimately use the entity certificate for its operation.
The way we generate certificates is wrong though: we use the root
certificate as template and sign it with the entity's certificate. This
also explains why all the parameters that should theoretically be set
for the entity are instead set in the root certificate.
Fix this bug so that we correctly use the entity certificate with proper
parameters as template and sign it with the root certificate.
|
|
The variable names used by the function to generate certificates are a
bit confusing. Most importantly, using both "root" and "ca" for the
certificate authority makes one think that those are two different
things.
Rename the variables to clarify that there really only are two entities:
the "root" certificate that is supposed to sign the certificate of the
actual "entity".
|
|
Enable Gitaly custom linters
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5500
Merged-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
|
|
Makefile: Treat `PROTO_DEST_DIR` as 'new' in diff
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5515
Merged-by: karthik nayak <knayak@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Co-authored-by: Will Chandler <wchandler@gitlab.com>
|
|
featureflag: Remove unused FetchSourceBranchQuarantined flag
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5518
Merged-by: karthik nayak <knayak@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
