| Age | Commit message (Collapse) | Author |
|---|
|
Add a new execution environment for bundled Git v2.36.0.gl1. This
environment is still guarded by a feature flag.
Changelog: added
|
|
Install bundled Git v2.36.0.gl1 alongside v2.35.1.gl1. Note that we
carry forward a set of patches from the old version which hasn't made
it into the final release yet.
Changelog: added
|
|
While we already ensure that no seed test Git repositories are being
written into by running tests under a different user in our CI, we don't
check whether the repositories are used to spawn any read-only commands.
And this has in fact been fine until now: permissions were such that
this always worked.
This has changed with CVE-2022-24765 though: Git has started to operate
in repositories completely in case it is owned by a different user. As a
consequence, this breaks our testing infrastructure whenever a test is
trying to run a command in the test seed repositories directly given
that they're owned by `root`, and we ourselves run with UID 9999 in our
CI.
Luckily, commands like git-upload-pack(1) still work, which means that
we're still able to clone such repositories. And there are only very few
tests which don't first use e.g. `gittest.CloneRepository()` before
actually run commands directly in there, too, because we don't expose a
simple way to obtain the path of those seed repositories outside of the
`gittest` package. The only offenders are `gittest.ChecksumTestRepo()`
and `gittest.BundleTestRepo()`.
Refactor those two helper functions to instead operate on a cloned
repository instead of running commands in the seed repositories
directly. This fixes compatibility with all Git versions which include a
fix for above CVE.
|
|
CI: cache danger and dbschema dependencies
See merge request gitlab-org/gitaly!4412
|
|
git: Fix core.fsync incompatibility and fix corruption of loose references
Closes #4117 and #4125
See merge request gitlab-org/gitaly!4498
|
|
CI's dbschema step was caching only C/Go compiled dependencies
but missing ruby ones.
|
|
Danger dependencies were not previously cached on CI.
This adds the bundler directory with specific cache keys for danger.
|
|
limithandler: Return error from limit handler
See merge request gitlab-org/gitaly!4492
|
|
catfile: Make process cancellation synchronous to fix test race
See merge request gitlab-org/gitaly!4500
|
|
doc: Document process to upgrade the Git version
Closes #4191
See merge request gitlab-org/gitaly!4509
|
|
Start using protolint
See merge request gitlab-org/gitaly!4465
|
|
In c83f00059 (add concurrency queue limit 2022-01-24), we modified the
limit handler to return immediately with an error if certain limit
conditions are reached.
In the wrappedStream however, RecvMsg() merely logs the error. This will
block indefinitely, because when the limiter returns with an error, it
will no longer try to get a concurrency slot. Thus this function will
wait until the context is cancelled.
Fix this by returning an error when we get an error from the limiter.
Changelog: fixed
|
|
replicator: Drop logic to create and handle maintenance-style replication jobs
See merge request gitlab-org/gitaly!4494
|
|
config: Remove internal socket path configuration
Closes #4139
See merge request gitlab-org/gitaly!4504
|
|
Remove feature flag "command_stats_metrics", always enable feature
See merge request gitlab-org/gitaly!4484
|
|
When creating a new cached catfile process we need to decorrelate it
from the parent context. For one this means that we need to remove the
correlation ID and any tracing spans from the context. And second it
means we need to suppress cancellation of the parent so that our cached
process doesn't get killed when the RPC context is cancelled.
The way we do this right now is to use `context.Background()`: this just
creates a completely new background. Funny enough, we still try to strip
information from this new context, which doesn't make any sense given
that it's empty already. But this hints at an actual bug: the intent is
not to get a completely new context, but by using `context.Background()`
we also lose information such as feature flags here.
Fix this bug by not creating a completely new context. Instead, we just
suppress context cancellation via `helper.SuppressCancellation()` and
then continue to decorrelate it like we always used to do. Only that now
it actually does something.
Changelog: fixed
|
|
The catfile cache is using Goroutines to clean up processes. This is not
deterministic and thus frequently causes our tests to flake when we lose
one of the races caused by this. Furthermore, the asynchronicity makes
the code quite hard to understand.
Refactor the code to use synchronous cancellation. Instead of waiting
for the context to get cancelled, it is now the caller that is
responsible for killing the process. The caller transparently either
gets a cancellation function that
- kills the process directly in case it is an uncachable process.
- or returns the process to the cache in case it is a cachable
process.
This significantly simplifies the code and also makes it deterministic,
which fixes the flaky tests we have observed. Furthermore, this also
allows us to get rid of one more Goroutine per cached catfile process.
Note that this allows us to get rid of the signalling logic we had in
place previous to this change: there is no need to wait for processes to
return to the cache in our tests anymore because this is now guaranteed
after the cancellation function was called.
|
|
Both the `CatfileReader` and `CatfileInfoReader` have the same logic to
tear down the process when the context gets cancelled. This logic can
easily be unified into the cache: it already got a Goroutine that waits
for the context to get cancelled to decrement the process counter, so it
is a perfect fit to also host the other logic.
Refactor the code to do this. While this reduces complexity, it also
cuts the number of Goroutines in half.
|
|
The catfile cache still mentions "batch" in a lot of places, even though
what it's actually handling is processes that may or may not be a batch
process.
Adjust the variable names to instead say "process".
|
|
command: Add metric for spawning tokens
See merge request gitlab-org/gitaly!4487
|
|
With the ability to do feature-flag-based rollouts of the Git version
our process to upgrade Git versions has become a bit more complex.
Document it so that it's easy to follow.
|
|
git: Globally disable generation of server info for git-repack(1)
Closes #4123
See merge request gitlab-org/gitaly!4505
|
|
Previously this plugin was used for linting, but it actually generates
its own source files.
Simplifies the lint-proto job since CI already runs `make proto` there
is no need to duplicate work.
|
|
This makes the naming of all proto files consistent.
|
|
|
|
From https://github.com/yoheimuta/protolint this linter is configurable
and much more strict than protoc.
|
|
Makefile: Small improvements to how Git is built
See merge request gitlab-org/gitaly!4506
|
|
ci: Remove DOCKER_DRIVER=overlay2 from Gitaly CI as it is already the default
Closes #4196
See merge request gitlab-org/gitaly!4503
|
|
Since the inception of our `make git` target we have always passed the
`NO_INSTALL_HARDLINKS=YesPlease` build option. This option causes the
Git build system to create copies of the Git binaries instead of hard
links.
It is not clear why we should need to set this option though as it only
increases the size of the installed distribution by about 25MB without
much merit. So let's stop passing the build option so that Git's build
system can decide for itself how it should install the binaries.
|
|
Building either our full Git distribution or the bundled Git versions
always uses a `.version` file to verify whether we need to rebuild the
target or not: only if the commit, build options or patches have been
modified since the last time the `.version` file was generated will we
actually rebuild Git. This is an important optimization and works quite
well.
One case where it doesn't work well right now though is when building
different versions of distributed Git. Depending on which version the
user wants to build via `GIT_VERSION`, we use different directories to
build Git and thus also use different `.version` files. This has a catch
though: because the `.version` file will not be updated again when both
Git versions have already been built successfully, we now think that we
don't need to rebuild Git because its `Makefile` is still newer than the
`.version` file when we switch between versions.
Fix this bug by always using the same directory to build distributed
Git, regardless of its version. While this means more rebuilds, it's
simple and correct.
|
|
In 0d588e403 (git: Disable generation of server info in git-repack(1),
2022-03-11), we have started to disable generation of server info in
git-repack(1) by passing the `-n` flag to all invocations. This was the
best we could do because Git didn't provide any way to do this on a
global level. We have since upstreamed a new `repack.updateServerInfo`
config key though which allows us to do so.
The new config key was released with Git v2.36.0, so let's start
injecting it into git-repack(1) processes. Note that we cannot yet get
rid of the `-n` flag because the minimum required Git version is still
at v2.33.1, which doesn't understand the new config. But by injecting
the config via environment variables, Git will now also honor it in case
git-repack(1) was spawned via any intermediate command.
Further note that we cannot really test this either because we don't yet
have Git v2.36.0 as part of our CI infrastructure.
|
|
Signed-off-by: Takuya Noguchi <takninnovationresearch@gmail.com>
|
|
The replicator used to host logic to replicate maintenance-style RPCs,
and thus it also had some tests which exercised this logic. Now that
replication of those RPCs has been removed in favor of a best-effort
routing strategy, we're essentially not testing the replicator anymore
though, but the coordinator.
Move the test into the coordinator's test suite. While at it, refactor
it to be table-driven so that it's more readable and convert the setup
to use `runPraefectServer()`.
|
|
With commit 7a8b33aa7 (gitaly/config: Introduce runtime directory
configuration, 2022-03-17), we have introduced a new global runtime
directory that is supposed to hold all files and directories required by
Gitaly to operate correctly. This new directory supersedes the previous
internal runtime directory, which shouldn't be used anymore.
We have since converted the GDK, CNG and Omnibus accordingly and put up
a deprecation notice for v15.0. Let's thus remove the old configuration
option so that we now always use the runtime directory instead.
Changelog: removed
|
|
The function `GitalyInternalSocketPath()` exposed by Gitaly's
configuration is needlessly verbose with its `Gitaly` prefix. Rename it
to `InternalSocketPath()` instead.
|
|
Makefile: Drop bundled Git v2.33.1.gl3
See merge request gitlab-org/gitaly!4495
|
|
Instrument verification worker for observability
Closes #4097
See merge request gitlab-org/gitaly!4473
|
|
In the past, we had several incidents where repositories ended up with
corrupted loose references immediately after a server was experiencing a
hard shut-down. The root cause of this is that Git historically didn't
flush the contents of loose references to disk before renaming them into
place. This usage is explicitly documented to be invalid by various
filesystems in the Linux kernel. Most importantly, documentation of the
ext4 filesystem says that:
Many broken applications don't use fsync() when replacing existing
files via patterns such as fd = open("foo.new")/write(fd,..)/close(fd)/
rename("foo.new", "foo"), or worse yet, fd = open("foo",
O_TRUNC)/write(fd,..)/close(fd).
With Git v2.36.0 new infrastructure was introduced that provides finer
grained control of exactly what data should be flushed to disk via the
new `core.fsync` option. As part of that effort we have also upstreamed
patches which give us the ability to also flush references to disk
before they're renamed into place.
So let's finally fix this source of corruption on newer Git versions by
enabling fsync(3P) for references.
Changelog: fixed
|
|
Git v2.36.0 has introduced new infrastructure to allow administrators
more fine-grained control over what files Git will synchronize to disks.
This new infrastructure replaces the old `core.fsyncObjectFiles` config,
which we are currently injecting unconditionally. Unfortunately, the new
Git version has started to emit a warning in that case, which would both
be presented to the user and furthermore it would break a bunch of our
tests.
We're thus forced to conditionally inject either the old or the new
config depending on which Git version we're running. Let's do so by
injecting `core.fsync=objects,derived-metadata` instead when running Git
v2.36.0 or newer. This has the same semantics as what we had previously
with `core.fsyncObjectFiles=true`:
- `default` is what Git did by default previously, and expands to
`objects,derived-metadata,-loose-object`. Note that the current
documentation in gitconfig(1) is wrong as it pretends that it
instead expands to `committed,-loose-object`.
- Because we want to also synchronize loose objects though we can
drop the `-loose-object` part.
This fixes compatibility witih v2.36.0 while continuing to do the same
as we did before.
Changelog: fixed
|
|
Git v2.36.0 has introduced the new `core.fsync` infrastructure, which
allows the user more fine-grained control over what exactly should be
synchronized to disk. Add a function which allows us to check whether
this new infrastructure is supported by the current Git version.
|
|
Inline the global Git configuration. This is done so that we can only
conditionally add `core.fsyncObjectFiles`.
|
|
We're about to introduce a version check whenever the Git command
factory creates a new command, which means that the factory will start
to call `GitVersion()` on itself. This breaks the intercepting command
factory though, which will erroneously try to parse the version by using
the output of the intercepting script.
Fix this issue by writing a second wrapper script that detects the
version check in case the version check is not to be intercepted. If so,
then the script will call the real Git binary to determine the version.
|
|
The intercepting Git command factory is currently always using the
intercepting script to derive the current Git version. This only works
alright because we typically don't check for the Git version in most
callsites, and thus the intercepting script's output is never used to
determine the version except for some limited cases. This is about to
change though given that we'll introduce a Git version check into the
command factory itself, which will then break all tests which use the
intercepting command factory.
Add a new option to the intercepting command factory that allows the
caller to only optionally intercept the Git version. By default, we'll
now use the real Git version so that users will continue to work.
|
|
smarthttp: Add finalizing vote in PostReceivePack
Closes #4110
See merge request gitlab-org/gitaly!4488
|
|
It's currently only possible to pass options intended for the real Git
command factory when constructing an intercepting factory. We're about
to add another option though that changes behaviour of the intercepting
factory itself.
Introduce a new `InterceptingCommandFactoryOption` type to make this
more flexible and allow for the new usecase.
|
|
Due to a new incompatibility in Git with regards to the newly introduced
`core.fsync` option we'll have to inject different global options based
on the Git version. Consequentially, we are forced to detect the Git
version when spawning any Git commands. This creates a chicken-and-egg
problem though: we need to spawn a command via `newCommand()`, which in
turn tries to detect the Git version by running `newCommand()` to run
git-version(1).
Prepare for this upcoming version check by open-coding `GitVersion()` so
that it doesn't use `newCommand()` anymore. Instead, we simply assemble
the arguments ourselves and then use `command.New()`. This gets rid of
the cyclic dependency and paves the path for the new version check.
|
|
operations: Remove feature flag for transactional voting in UserSquash
Closes #4119
See merge request gitlab-org/gitaly!4496
|
|
When git commands take a long time, it's hard to know whether it's from
the actual process or that time is spent waiting for a spawn token. This
adds a metric that measures and emits the time spent waiting for a spawn
token.
Changelog: added
|
|
Ignore verification columns for read-only cache updates
Closes #4159
See merge request gitlab-org/gitaly!4468
|
|
When all updates are rejected, there will be no votes cast. This leads
to an unnecessary replication job. In this case, we want to cast a last
but empty vote. This is the same as how SSHReceivePack works.
Changelog: changed
|
