This project is mirrored from https://gitlab.com/gitlab-org/gitaly.git.
Pull mirroring updated .
- Dec 20, 2022
-
-
Patrick Steinhardt authored
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
-
Patrick Steinhardt authored
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 bd0f8995 (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`.
-
Patrick Steinhardt authored
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>
-
Toon Claes authored
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>
-
Patrick Steinhardt authored
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
-
Patrick Steinhardt authored
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.
-
Patrick Steinhardt authored
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.
-
Patrick Steinhardt authored
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.
-
Patrick Steinhardt authored
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.
-
Patrick Steinhardt authored
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.
-
Patrick Steinhardt authored
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.
-
Patrick Steinhardt authored
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.
-
Patrick Steinhardt authored
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>
-
- Dec 19, 2022
-
-
Patrick Steinhardt authored
Replace calls to `helper.ErrInvalidArgumentf()` with `structerr.NewInvalidArgument()` and remove the former function.
-
Patrick Steinhardt authored
Replace calls to `helper.ErrNotFoundf()` with `structerr.NewNotFound()` and remove the former function.
-
Patrick Steinhardt authored
helper: Replace `helper.ErrInternalf()` with `structerr` package See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5195 Merged-by:
Patrick Steinhardt <psteinhardt@gitlab.com> Approved-by:
Quang-Minh Nguyen <qmnguyen@gitlab.com>
-
Toon Claes authored
Because use of the catfile cache can now be enabled with a feature flag, we can compare the performance impact on FindLicense. While running the benchmark we count the number of cat-file commands spawned. When using the catfile cache this should be at 1 at most. goos: linux goarch: amd64 pkg: gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/service/repository cpu: 11th Gen Intel(R) Core(TM) i7-1165G7 @ 2.80GHz BenchmarkFindLicense BenchmarkFindLicense/localrepo_read_object_cached=false BenchmarkFindLicense/localrepo_read_object_cached=false/gitlab-org/gitlab.git BenchmarkFindLicense/localrepo_read_object_cached=false/gitlab-org/gitlab.git-8 42 27799844 ns/op BenchmarkFindLicense/localrepo_read_object_cached=false/stress.git BenchmarkFindLicense/localrepo_read_object_cached=false/stress.git-8 2 699074767 ns/op BenchmarkFindLicense/localrepo_read_object_cached=true BenchmarkFindLicense/localrepo_read_object_cached=true/gitlab-org/gitlab.git BenchmarkFindLicense/localrepo_read_object_cached=true/gitlab-org/gitlab.git-8 60 17576759 ns/op BenchmarkFindLicense/localrepo_read_object_cached=true/stress.git BenchmarkFindLicense/localrepo_read_object_cached=true/stress.git-8 3 525983032 ns/op With the use of the catfile cache the response time is almost halved for gitlab-org/gitlab.git. For the crafted stress.git repo the improvement is less significant because most time there is spent in analyzing the content, and less in fetching the content for the repo.
-
Toon Claes authored
A localrepo already has access to the catfile cache, and ReadCommit() already uses it to read objects. ReadObject() still was spawning a git-cat-file(1) process itself. This change modifies ReadObject() to use the catfile cache. This might avoid actually spawning a new process. We verify this with the newly introduced gittest.CountingCommandFactory. The change is behind a feature flag rolled out through [1]. [1]: https://gitlab.com/gitlab-org/gitaly/-/issues/4662 Milestone: 15.7 Label: maintenance::performance
-
Toon Claes authored
CountingCommandFactory embeds a regular git command factory, but it keeps count of each call of New*() with their git command.
-
Will Chandler authored
cgroups: Fix layering violations See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5181 Merged-by:
Will Chandler <wchandler@gitlab.com> Approved-by:
Will Chandler <wchandler@gitlab.com> Reviewed-by:
Patrick Steinhardt <psteinhardt@gitlab.com> Reviewed-by:
Will Chandler <wchandler@gitlab.com> Co-authored-by:
Patrick Steinhardt <psteinhardt@gitlab.com>
-
Patrick Steinhardt authored
Replace calls to `helper.ErrInternalf()` with `structerr.NewInternal()` and remove the former function.
-
Patrick Steinhardt authored
The cgroups manager accepts a `repository.GitRepo` as argument, which is used to compute a cgroup key that will always be the same for multiple different Git commands as long as they're executed in the same repo. As a result we will always put them into the same cgroup bucket. Having this knowledge in the cgroups manager is a layering violation though: the manager really shouldn't know about repositories at all, but only care about commands and the bucket they're put into. Introduce a new option `WithCgroupKey()` that allows callers to override the cgroup key used to derive the bucket. Like this, we can pass through the per-repository cgroup key from the Git command factory through the command package into the cgroups manager without anybody but the Git command factory actually knowing about repositories.
-
Patrick Steinhardt authored
The command package and the cgroups package have a cyclic dependency: - The `AddCommand()` function of the `cgroups.Manager` receives a `command.Command` as input. - The `command` package needs to have a `cgroups.Manager` so that it can add the command to it. The cylic dependency is currently solved by using an interface in the `command` package that only has the `AddCommand()` function. But it would be preferable to just not have this cyclic dependency at all. Refactor the code to instead accept an `exec.Cmd` in `AddCommand()` to break the cyclic dependency.
-
Patrick Steinhardt authored
One of the cgroups tests that verifies whether metrics are exposed as expected also verifies that we get a log message. This log message is not generated by the cgroups package though, but it's generated by the command package. So it really is not the business of the cgroups tests to verify that we see this log message. Stop asserting the log message. This change also prepares us for a fix to a cyclic dependency between the command and cgroups package.
-
Quang-Minh Nguyen authored
helper: Convert more error-formatting functions to use `structerr` package See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5185 Merged-by:
Quang-Minh Nguyen <qmnguyen@gitlab.com> Approved-by:
Quang-Minh Nguyen <qmnguyen@gitlab.com> Approved-by:
James Fargher <proglottis@gmail.com> Reviewed-by:
Quang-Minh Nguyen <qmnguyen@gitlab.com> Co-authored-by:
Patrick Steinhardt <psteinhardt@gitlab.com>
-
- Dec 16, 2022
-
-
karthik nayak authored
golancilint: add paralleltest linter Closes #4673 See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5184 Merged-by:
karthik nayak <knayak@gitlab.com> Approved-by:
Sami Hiltunen <shiltunen@gitlab.com> Reviewed-by:
Patrick Steinhardt <psteinhardt@gitlab.com> Reviewed-by:
Sami Hiltunen <shiltunen@gitlab.com> Reviewed-by:
karthik nayak <knayak@gitlab.com>
-
Karthik Nayak authored
In the last few commits we reinitialized the test variable wherever needed and also fixed tests. This is due to a bug/feature around how Go treats looped variables (https://gist.github.com/posener/92a55c4cd441fc5e5e85f27bca008721). To avoid this mistake in the future, let's add a linter check around this issue. This is done by adding the `paralleltest` linter. We also enable `ignore-missing` because we don't want to complain about missing usage of `t.Parallel()` (for now...).
-
Karthik Nayak authored
In the tests TestFindAllTags_sorted and TestServer_ListRefs, we don't reinitialize the test variable, this can cause problems to go undetected (https://gist.github.com/posener/92a55c4cd441fc5e5e85f27bca008721). Reinitialize the test variable.
-
Karthik Nayak authored
In TestStreamLimitHandler, we don't reinitialize the test variable, this can cause problems to go undetected (https://gist.github.com/posener/92a55c4cd441fc5e5e85f27bca008721). Reinitialize the test variable and fix the broken tests.
-
Karthik Nayak authored
In TestGarbageCollectDeletesFileLocks, we don't reinitialize the test variable, this can cause problems to go undetected (https://gist.github.com/posener/92a55c4cd441fc5e5e85f27bca008721).
-
Karthik Nayak authored
In TestUserApplyPatch, we don't reinitialize the test variable, this can cause problems to go undetected (https://gist.github.com/posener/92a55c4cd441fc5e5e85f27bca008721). Fix the test and reinitialize the variable.
-
Karthik Nayak authored
In TestUpdateRemoteMirror, we don't reinitialize the test variable, this can cause problems to go undetected (https://gist.github.com/posener/92a55c4cd441fc5e5e85f27bca008721). Fix the test and reinitialize the variable.
-
Karthik Nayak authored
In TestPruneIfNeeded, we don't reinitialize the test variable, this can cause problems to go undetected (https://gist.github.com/posener/92a55c4cd441fc5e5e85f27bca008721). Fix the test and reinitialize the variable, do note that this change removes the only test which was checking for `pruned_objects: success`. So add a new test which creates a lot of looseObjects and also sets their time to before the StaleObjectsGracePeriod.
-
- Dec 15, 2022
-
-
Stan Hu authored
ruby: Update dependency google-protobuf to '~> 3.21.12' See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5190 Merged-by:
Stan Hu <stanhu@gmail.com> Approved-by:
Stan Hu <stanhu@gmail.com> Co-authored-by:
GitLab Renovate Bot <gitlab-bot@gitlab.com>
-
Sami Hiltunen authored
tests: Remove unneeded seed repositories See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5183 Merged-by:
Sami Hiltunen <shiltunen@gitlab.com> Approved-by:
James Fargher <proglottis@gmail.com> Reviewed-by:
James Fargher <proglottis@gmail.com> Co-authored-by:
Patrick Steinhardt <psteinhardt@gitlab.com>
-
karthik nayak authored
git: Simplify command interface See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5180 Merged-by:
karthik nayak <knayak@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:
Patrick Steinhardt <psteinhardt@gitlab.com>
-
Pavlo Strokov authored
tests: Speed up some of our most expensive tests (pt2) See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5179 Merged-by:
Pavlo Strokov <pstrokov@gitlab.com> Approved-by:
Pavlo Strokov <pstrokov@gitlab.com> Reviewed-by:
James Fargher <proglottis@gmail.com> Reviewed-by:
Patrick Steinhardt <psteinhardt@gitlab.com> Reviewed-by:
Pavlo Strokov <pstrokov@gitlab.com> Co-authored-by:
Patrick Steinhardt <psteinhardt@gitlab.com>
-
Patrick Steinhardt authored
Parallelize some of our most expensive tests to speed up test execution.
-
Patrick Steinhardt authored
We use the touch(1) executable to update mtimes of on-disk files in one of our tests. Update the test to instead just use `os.Chtimes()`.
-
Patrick Steinhardt authored
One of the tests for RepackFull uses mtimes of packfiles as a proxy to test whether the RPC call did what it was supposed to do. This is quite a weak proxy though for what we actually care for, which is whether we have repacked all packfiles into a single one. Refactor the test to stop checking for mtimes and instead use the packfile count.
-