| Age | Commit message (Collapse) | Author |
|---|
|
There's at least one editor (GNU Emacs) which does not like how comments
in recipes are denoted at the moment. This commit changes the syntax and
silences all comments to all users.
|
|
Makefile: Random set of improvements
See merge request gitlab-org/gitaly!4158
|
|
When executing `make git`, then Gitaly provides a default set of build
options. These include all kinds of things, but most importantly we
disable a bunch of options which we don't need at runtime. In case
downstream distributions of Gitaly want to build Git with a bunch of
additional options though, they have no other way to do so than to copy
the default set of options and then append whatever they want to change
to this set. This is quite fragile, and any changes done in Gitaly won't
be reflected in downstream distributions like this.
Improve this by adding two new build variables GIT_APPEND_BUILD_OPTIONS
and LIBGIT2_APPEND_BUILD_OPTIONS, which allow appending additional
options without overriding the defaults.
|
|
Use gotestsum instead of go-junit-report
See merge request gitlab-org/gitaly!4151
|
|
With the introduction of gotestsum, the default output format has
changed to a new "short" summary. While this format is indeed nicer to
look at, it reports less information about what is going on and which
tests ran.
Add a new `TEST_FORMAT` variable which allows to change the formatting.
Note that this commit also drops the `-v` flag from `TEST_OPTIONS`: this
flag is not respected anymore because verbosity is now controlled by
gotestsum.
|
|
In the Makefile, GIT_VERSION is overwritten when it is default. This
makes the junit filename no longer match what is specified in
gitlab-ci.yml. Instead here we are simplifying down to a static
filename. There shouldn't be any risk of the report being overwritten
since CI runs each permutation of the test matrix in separate
containers.
|
|
prepare-tests now depends on gotestsum since it is now always used to
run `go test` and so test reports are always generated.
|
|
praefect: Track database schema in Git
See merge request gitlab-org/gitaly!4130
|
|
After cloning test repos, we replace their references with our own
vision of references by removing all loose ones and moving in a
pre-packed packed-refs file. As a result, we get a bunch of dangling
objects because we effectively move back the repository in time even
though we already downloaded all objects. While not bad per-se, it
creates quite some output when we run git-fsck(1) to check the repo for
consistency.
Silence these messages by adding the `--no-dangling` switch.
|
|
We're using which(1) to detect the location of the system-installed Git.
This utility is not POSIX-compliant, which instead specifies that one
ought to use command(3P) instead.
Switch over to use `command -v` to fix the incompatibility and not
depend on which(1) being installed.
|
|
We have two different variables tracking the GIT_PREFIX: first the
GIT_PREFIX itself, which can be set by the user to point wherever the
user wants to install the compiled Git distribution to. And then we have
GIT_DEFAULT_PREFIX, which is the default value of GIT_PREFIX. There is
one special thing about GIT_DEFAULT_PREFIX though: upon installing Git,
we always remove the GIT_DEFAULT_PREFIX first regardless of whether
we're actually using the default prefix or not.
While it is clear that we _never_ want to remove GIT_PREFIX before
installing into it given that it may point e.g. to `/usr/local`, it's
questionable what the intent is in removing the default prefix first.
If I am about to guess, then we likely want to ensure that developers
never end up with Git installations of mixed versions given that the Git
version changes much more frequently on a developer machine, may it be
because of normal upgrades of the Git version or because one is in the
process of bisecting. As such, it is not too unlikely that the user will
end up with such mixed Git installations. And given that distros of
Gitaly will hopefully use proper packaging, we should assume that they
do not run such mixed installations either.
Even so, it does not make sense to delete the default prefix in case the
current prefix is different. So let's only delete it in case GIT_PREFIX
and GIT_DEFAULT_PREFIX are the same.
|
|
Tests nowadays don't generate any files in the source tree anymore, but
our cleanup target still refers to one such location. Remove it.
|
|
The GIT_INSTALL_DIR variable points into our own build directory and is
the default directory into which Git will be installed in case the user
didn't override GIT_PREFIX. The naming of this variable doesn't help at
all in underestanding what it does though and frequently leads to
confusion which of both variables should be used.
Rename the variable to GIT_DEFAULT_PREFIX to clarify what it does.
|
|
When installing Git via `make git`, we first clean up the
GIT_INSTALL_DIR by removing it completely. This is expected, as it is
the default location of the Git installation located in our own build
directory, and we want to have a tidy installation in there. After the
cleanup, we do recreate the target directory though, which is completely
unnecessary: the Makefile target would create it for us anyway, and in
case the default installation directory isn't used we wouldn't use that
directory anyway.
Stop creating the directory and let the Makefile target handle it for
us.
|
|
When installing Git via `make git`, then we have two variables which
guide where things will be installed to: GIT_PREFIX is user-controlled
and determines where Git should be installed to, and GIT_INSTALL_DIR is
the default value in case GIT_PREFIX is not set. So ultimately, Git will
always be installed into GIT_PREFIX.
The rules we have for installing Git use GIT_INSTALL_DIR as target
though, so when one installs Git with a manually-chosen GIT_PREFIX, then
the target mismatches the location where Git actually ends up. This used
to work alright given that we never did anything with the target except
for executing `make install` in the Git project, but with the recent to
introduce a bundled Git version we started to touch(1) the target. This
thus now fails.
Fix this by using GIT_PREFIX instead of GIT_INSTALL_DIR.
Changelog: fixed
|
|
Now that we can build bundled Git binaries, this commit introduces
support into the Git command factory to support them. If a new option
"git.use_bundled_binaries" is set to `true`, then we automatically
derive the Git binary's path from the binary directory of Gitaly.
Note that we must go through some hoops though given that the bundled
binaries have a "gitaly-" prefix. While it's not much of a problem when
executing Git directly, some Git commands require it to find auxiliary
helper binaries like "git-remote-http" or "git-receive-pack". To support
these cases, we thus create a temporary directory and symlink all
binaries Git expects to be present into it. By pointing GIT_EXEC_PATH at
this directory, we can thus trick Git into picking the correct set of
binaries.
This new bundled mode will required configuration changes by admins.
|
|
In order to allow the use of feature flags to toggle different Git
versions at runtime, Gitaly needs to take much more ownership of
binaries such that it knows where to find the Git binaries without
having the admin to always set up different paths whenever there are two
different versions of Git. As a first step towards taking more
ownership, this commit introduces the new `WITH_BUNDLED_GIT` variable to
our Makefile: if set, we will build and install required Git binaries.
In order to not accidentally overwrite any preexisting binaries, the
required binaries all have a "gitaly-" prefix.
Note that ideally, we'd want to install only the "git" binary, which
nowadays contains almost all of the functionality required by us. One
exception though is cURL-based remote helpers, which are still split out
into a separate binary "git-remote-http". Given that we do interact with
HTTP-based Git servers we thus have to build and install both binaries.
This target is currently completely optional, and we want to continue to
support running Gitaly with an external Git executable not built by
Gitaly. It may eventually replace the current "make git" target though,
which sets up a complete Git installation for us.
|
|
The no-changes Makefile target verifies that there are no changes in the
working tree, which is used by some CI jobs to verify whether generated
files are up-to-date or not. The target parses git-status(1) for this,
but this is not really all that helpful for us in case there _were_
changes: the caller wouldn't know what was changed, but only see the
failed Makefile target.
Improve the target by using `git diff --exit-code` instead: this is
simpler given we don't have to do post-processing via awk(1), and it
prints the diff such that callers know what has changed by just taking a
simple look at the CI job's output.
|
|
Given that our Praefect database schema is seeded by migrations, there
is no single place which gives an overview over the complete schema.
While this is information that can be easily obtained by connecting to a
Praefect database, this workaround doesn't really work across different
Praefect versions given that one would have to re-seed the database for
each version one is about to investigate. This makes it hard to get info
about the current database schema and to compare changes to the schema
between different versions.
Introduce a new script "generate-praefect-schema" to fix this issue. The
script connects to a Postgres server, creates a new database, seeds the
database by executing `praefect sql-migrate` and then dumps the
resulting schema. Furthermore, a new Makefile target automates this and
writes the dump into our `_support` directory.
The intent is that whenever the database changes, the author of those
changes must update the schema in our `_support` folder such that it's
easily possible to diff the schema across different versions in our Git
history.
To ensure that the schema is always up to date, a new CI job is added
which performs the dump and then checks that no changes occurred.
|
|
Update `gofumpt` to v0.2.0, which adds a new bunch of formatting rules:
- Composite literals should not have leading or trailing empty
lines.
- No empty lines following an assignment operator.
- Functions using an empty line for readability should use a `) {` line instead.
- Remove unnecessary empty lines from interfaces.
We only have a small set of code which didn't follow these new rules.
Reformat them.
|
|
Upgrade golangci-lint to v1.43, which is the newest released version at
the time of writing this commit.
|
|
We've grown a bunch of patches which get applied to Git such that we can
land improvements in production faster. These patches get applied by
default, and in fact they also get applied when the user specifies a
different Git version than the default one. This is quite annoying given
that the patches are unlikely to apply on an arbitrary Git version, and
it thus requires callers to always pass `GIT_PATCHES=` in order to not
apply patches when using a custom version. Furthermore, it's a tad
dangerous: if the user explicitly requests a different Git version and
doesn't override GIT_PATCHES, then we'd also amend the current extra
version to indicate the Gitaly patch level.
Fix this issue by only applying patches by default in case the default
Git version is in use. Users can still explicitly request patching by
setting GIT_APPLY_DEFAULT_PATCHES, if they want to.
|
|
Even though the default version of Git is nowadays specified by our
Makefile and built ad-hoc, we still specify the same default version in
our CI instructions. This is needless effort, and sure enough we have
already diverged in that CI uses Git v2.31.0 while the default version
is Git v2.31.1.
With the recent change to interpret an empty GIT_VERSION to mean the
default version we can fix this though: in theory, we could just set the
GIT_VERSION to the empty string by default in our CI instructions and
then we'd automatically get the default Git version. It's a bit unhandy
though given that we'd now end up with weird cache and artifact names
because the Git version is embedded in their names. Instead, introduce
another way to request the default Git version which is to pass
GIT_VERSION=default.
While at it, this commit fixes another bug with setting the Git version:
while we correctly handle the case where the Git version is passed via
environment variables and tweak it accordingly, this doesn't work when
invoking `make GIT_VERSION=default`. This is because by default, make(1)
does not assign to variables which have been explicitly defined by the
user. This issue is fixed by adding an explicit `override` directive.
|
|
|
|
In some environments it is impossible to unset
environment variable, and no `GIT_VERSION=` works.
This allows to pass empty value that indicates
that a default should be used. For example allowing
to execute: `make git GIT_VERSION=`.
Changelog: fixed
|
|
With commit eb6fd6056 (Makefile: Stop installing binaries into source
dir, 2021-05-07), we stopped installing Gitaly binaries into its root
directory given that all users should either use the installed location,
or alternatively use the directory in "_build/bin". This change had been
reverted due to a regression, where the init.d script in Rails was still
using the old location of Gitaly [1]. The issue has since been fixed by
adjusting paths in the script to point into our build directory.
Reintroduce the change and stop installing binaries into the root
directory. To avoid existing installations from silently continuing to
use the old, outdated binaries, we also delete them from the source
directory.
[1]: https://gitlab.com/gitlab-org/gitlab/-/issues/331758
|
|
Upgrade libgit2 to v1.2.0
See merge request gitlab-org/gitaly!3966
|
|
libgit2 v1.2.0 has been released on September 1st. Next to a bunch of
new features and performance improvements, this release also contains a
fix for unpacking objects from packfiles which sometimes led to errors.
This is an error we have seen once or twice in the past.
Upgrade our version of libgit2 to v1.2.0, along with the correspongind
update of Git2Go to v32 and Rugged to v1.2.0.
|
|
Git v2.31.1 has been released on October 12th with a big range of fixes.
Most relevant to us, it also contains fixes for git-update-ref(1) to
properly flush output when having a conversation with `--stdin`, which
we're currently carrying a custom patch for.
Bump our Git version to v2.31.1 and drop the custom patch. Furthermore,
adjust `FlushesUpdaterefStatus()` to reflect that v2.31.1 now support
flushing.
|
|
During the bundle step of the gitaly-ruby gems it tries to guess whether
it is part of a gitlab-development-kit. If it isn't it installs gems as
a "deployment", which will install the gems in vendor/bundle instead of
in $GEM_HOME. To detect this, it looks for .gdk-install-root in the
parent directory. This behavior is unexpected to many and also seems to
cause issues for some users.
Added to that, recently a change [1] was made to gitlab-org/gitlab to
install gems in $GEM_HOME when running tests. This speeds up the
installation of Gitaly. This change made it so the gems are never
installed as a "deployment"
This change removes the code to determine whether it's a deployment and
makes the gems install in $GEM_HOME unconditionally.
1. https://gitlab.com/gitlab-org/gitlab/-/commit/77f41fff0363ec0b1bc687e8df02995729e8218e
Issue: https://gitlab.com/gitlab-org/gitlab-development-kit/-/issues/1315
|
|
Starting with Go 1.16, it is possible to use version suffixes for `go
install`. This gives us the possibility to instead tool dependencies
without having to play tricks via separate per-tool `go.mod` files like
we currently do, and is indeed the recommended way to install tools now.
Quoting the release notes [1]:
go install, with or without a version suffix (as described above),
is now the recommended way to build and install packages in module
mode. go get should be used with the -d flag to adjust the current
module's dependencies without building packages, and use of go get
to build and install packages is deprecated. In a future release,
the -d flag will always be enabled.
Adapt the Makefile to use `go install` instead of `go get` and get rid
of our previous workaround which created separate per-tool module
directories.
[1]: https://golang.org/doc/go1.16
|
|
Prior to these patches, Git used unbuffered writes for upload-pack ref
advertisements, with one or two write syscalls per ref. In
repositories with many refs that adds up to a lot of syscalls. These
patches add stdio buffering for the ref advertisement writes.
Also see gitlab-com/gl-infra/scalability#1257.
Changelog: performance
|
|
When executing git-update-ref(1) with the `--stdin` flag, then the user
can queue updates and, since e48cf33b61 (update-ref: implement
interactive transaction handling, 2020-04-02), interactively drive the
transaction's state via a set of transactional verbs. This interactivity
is somewhat broken though: while the caller can use these verbs to drive
the transaction's state, the status messages which confirm that a verb
has been processed is not flushed. The caller may thus be left hanging
waiting for the acknowledgement.
This bug keeps us from using those status updates in the updateref
package, which effectively means that we cannot be sure whether the
state transition was successful until we try to commit changes. Add the
baseline for fixing this bug by applying the patch to our Git version.
|
|
For quite some time we're aware of the fact that mirror-fetches into
repositories with many refs are exceedingly slow. Most importantly, this
issue poses problems for our replication strategy where replication jobs
take so much time that replication targets are likely to be out of date
immediately after they have received a replication jobs because the
primary node has received additional mutators while the replication
target was fetching changes.
To address this problem, we have upstreamed a patch series into git.git
which speeds fetches up somewhat. Most importantly, this patch series
optimizes the way git-fetch(1) enumerates refs by making better use of
the commit-graph. The result is that mirror-fetches in the benchmarking
repository gitlab-org/gitlab have been sped up from originally 56s to
25s.
While it is unlikely that this speedup alone will fix our replication
issue, it is definitely an important step towards improving the
situation.
Changelog: performance
|
|
Add documentation on existing Git patches such that it is easy to tell
what they do and when they have been merged into upstream's "next"
branch.
|
|
When we're not applying any custom patches, then we typically wouldn't
want to define a custom GitLab patch level because otherwise it may
easily mislead anybody to think that custom patches have indeed been
applied. This isn't currently working as expected though: if one builds
git with `make git GIT_PATCHES=`, then we do not apply any patches but
still have the GIT_EXTRA_VERSION defined.
The root cause of this is a misunderstanding of the `ifndef` directive:
the expectation is that this will only evaluate to `true` if the value
has not been previously defined. But what it really does is to return
`true` in case the value is either undefined or evaluates to the empty
string. As a result, we'd set up GIT_PATCHES and GIT_EXTRA_VERSION even
when the caller has explicitly set GIT_PATCHES to be the empty string.
While GIT_PATCHES won't be appended to because values which have been
defined by the user won't be overridden by default, GIT_EXTRA_VERSION
will be set up. So this construct only worked by chance.
Fix this bug by explicitly testing whether GIT_PATCHES is undefined via
the `$(origin ...)` function. While at it, we also fix all the other
occurrences where this may eventually cause unintended behaviour.
|
|
A rough rule we have for our Makefile is that all steps which aren't
producing the primary target of the rule and which are comparatively
short-lived should be silenced. We have a bunch of steps which aren't
currently following this rule, so let's silence them now.
|
|
With Gitaly now being the single source of truth for the Git version
deployed in production, we have started to make a lot more use of
applying patches on top of our self-built Git version. The policy is and
will remain to only ever apply patches which have been accepted
upstream, but it is sensible to allow for backporting patches such that
we can make use of them faster and test whether they have the expected
benefits.
This has uncovered a problem though: it's impossible for us to detect
which patches have been applied on top. This makes it hard for admins to
see whether they are running a custom version of Git or the mainline
version, and it is a problem for Gitaly because we cannot detect what
the current version supports in case we backport important fixes which
require changes both in Git and Gitaly.
Introduce a new Git extra version into our build process. This extra
version is a simple counter "gl${PATCHLEVEL}", where each newly added
set of Git patches must from now on increment the extra version. With
this version in place, we can fix both of above usecases.
When setting the Git version, we need to be careful to not break our
Git version parsing. But luckily, the current code is very lenient with
regards to parsing the fourth tuple: we only recognize the "rc" prefix,
but ignore everything else. Adding the GitLab patch level as this fourth
tuple is thus backwards compatible with older Gitaly versions, which
will simply ignore it. The end result is thus:
$ git --version
git version 2.33.0.gl1
It still needs an admin to correlate the set of patches with the patch
level, but this should be an easy enough task and is easier to maintain
on our side compared to somehow encoded "real" capabilities into the
version string.
Changelog: added
|
|
git: Speed up connectivity checks
Closes git#92
See merge request gitlab-org/gitaly!3810
|
|
Back in ce3f2ee42 (Lint: split up "make lint" and "make lint-strict"
configs, 2021-01-19), we have split up our linting rules into a "normal"
set which needs to be satisfied for each CI run and a "strict" set which
only runs on scheduled pipelines. I highly doubt that anybody ever takes
a look at the strict linter job: there are no MRs which clean up any of
its additional violations.
Let's remove this job and be done with it. Anybody wishing to fix up
linting rules can just play with our normal linting rules and is
unlikely to use CI for this anyway.
|
|
Add the gofumpt linting rule to golangci. This allows us to replace our
"check-formatting" Makefile target with a run of golangci-lint.
|
|
When executing the formatter, we use `find_go_sources` to find the
set of Go files which should be formatted. This function excludes a
bunch of files which shouldn't ever be formatted, like for example
generated protobuf files. These exclusions have been too broad though
and caused us to never format sources of our protoc-gen-gitaly command,
which is also contained in "proto/go/".
Fix this by only excluding "proto/go/gitalypb/", which contains all
generated sources, and run the formatter on the newly found sources.
|
|
We have hosted our own formatter via the gitalyfmt package. While this
has served us alright, there are (subjectively) better alternatives to
having our own formatter.
One of these alternatives is gofumpt, which is a stricter version of
gofmt adding a few additional rules on top:
- No empty lines at the beginning or end of a function.
- No empty lines around a lone statement (or comment) in a block.
- No empty lines before a simple error check.
- Composite literals should use newlines consistently.
- Empty field lists should use a single line.
- std imports must be in a separate group at the top.
- Short case clauses should take a single line.
- Multiline top-level declarations must be separated by empty lines.
- Single var declarations should not be grouped with parentheses.
- Contiguous top-level declarations should be grouped together.
- Simple var-declaration statements should use short assignments.
- The -s code simplification flag is enabled by default.
- Octal integer literals should use the 0o prefix on modules using Go 1.13 and later.
- Comments which aren't Go directives should start with a
whitespace.
To me, these rules all sound agreeable and should enforce a stricter
coding style across the project. So let's switch over to use this
formatter and remove our own homegrown one.
|
|
When a user pushes commits into a Git repository, then Git will do
a connectivity check to see whether it has all objects needed to satisfy
the ref updates. This check can be quite expensive depending on how many
refs the target repo has given that it is implemented as `git rev-list
--not --all`, which loads all objects pointed to by preexisting refs. In
repositories like gitlab-org/gitlab with about 2.3M refs, this typically
takes about 8 seconds. But the worst is that the user typically doesn't
even know what's going on given that there is no progress bar being
displayed.
We have thus upstreamed a set of patches which speed up the connectivity
checks. There are two major performance optimizations:
1. We stop sorting inputs in the connectivity check, which gives a
30% speedup.
2. Instead of loading referenced objects via the object database, we
use the commit-graph. This is another 30% speedup.
In total, this improves the time required from nearly 8 to 3 seconds.
Patches have been merged to upstream's "next" branch and are thus likely
going to be part of the next release. Given that this is still a few
months out, this commit backports those patches such that we can reap
the benefits earlier.
Changelog: performance
|
|
Makefile: Upgrade default Git version to v2.33.0
See merge request gitlab-org/gitaly!3791
|
|
Upgrade the default Git version to v2.33.0, released on August 16th.
This Git version has been tested as part of our nightly pipelines for
quite some time already given that we always test "master" and "next"
branches.
Changelog: changed
|
|
Now that we build Praefect binaries ad-hoc, there is no need for
building them in our Makefile anymore. This commit thus drops the
prerequisite and renames the now-misnamed "GITALY_TEST_PRAEFECT_BIN"
environment variable to "GITALY_TEST_WITH_PRAEFECT".
|
|
Upgrade golangci-lint to v1.42.0
See merge request gitlab-org/gitaly!3786
|
|
Upgrade golangci-lint to v1.42.0, which is the newest released version.
|
|
Changelog: removed
|
