| Age | Commit message (Collapse) | Author |
|---|
|
Currently when we call gitaly-git2go, we lose feature flags since they
are in the context inside the request that comes from Rails. In order to
allow gitaly-git2go to benefit from feature flags, we can pass it in
through an environment variable much like we do for gitaly-hooks.
In order to test this, add a new subcommand for gitaly-git2go that
simply returns which feature flags are set in the context. Since we only
gets built for tests, build this in only with the `test` build tag.
Changelog: changed
|
|
catfile: Fix deadlock between reading a new object and accessing it
See merge request gitlab-org/gitaly!4590
|
|
When reading the object info fails we're currently returning the request
queue back to the state where it pretends to not be reading an object
anymore. This means that it is now not considered dirty anymore and may
be used for additional requests, or even be returned to the catfile
cache. This is dangerous though: we do not know why reading the object
info has failed, and chances are high that the error is not recoverable.
And in that case we certainly don't want to return the catfile process
to the cache.
Fix this isssue by not unsetting `isReadingObject` when `ReadObject()`
fails. The only exception is when we got a `NotFound` error, which is a
graceful failure and doesn't dirty the git-cat-file(1) process.
|
|
The dirtiness tracking in request queue is somewhat hard to understand
as it bounces between multiple types. The Object type is tracking
the number of bytes read. The queue retains a reference to the latest
read object to check whether it has been fully read or not. This back
and forth can be simplified by managing the dirtiness state completely
in the queue. This commit simplifies the tracking by marking the queue
dirty when an object is returned, and composing the readers in a manner
that flicks the dirtiness bit off once the reading has been completed.
This moves all of the dirtiness logic to the queue, making it easier to
understand.
The object is reading from the stdout of a cat-file process. Closing the
cat-file process also closes the underlying reader the Object is using.
There's no need to have a separate closing mechanism for the Object so
the close and isClosed is removed as part of this change. They were only
called from the queue and it is no longer tracking the currentObject so
it can't close the object separately.
The behavior to return os.ErrClosed on reading when the queue is closed is
still retained as some tests rely on it. A later commit can adjust the tests
to remove the assertions and the behavior. The tests incorrectly assert
that nothing could be read after the process is closed. The io is buffered
and it is possible that something can still be read from the buffer even after
the underlying process is already closed.
|
|
ci: Capture panic stack traces
See merge request gitlab-org/gitaly!4593
|
|
|
|
[ci skip]
|
|
[ci skip]
|
|
[ci skip]
|
|
When we read in a new object via `ReadObject()`, then we must perform an
I/O operation to learn about its header. This operation is currently
done under the `currentObjectLock()`, but this can easily cause us to
deadlock when the I/O operation stalls. Consequentially, it's impossible
to access this object now anymore by other callers. Most importantly,
this means that both `close()` and `isDirty()` are blocked from making
any progress. But because those are used to determine whether processes
of the request queue should be killed in the first place we are now in a
deadlock-state.
Fix this issue by splitting up the lock into a read-lock for the current
object whose scope is a lot more fine-grained and only used when we
access the field or when we write to it. The second atomic lock is used
to only lock writing access across the whole lifetime of `ReadObject()`
so that no other caller can call it at the same time.
Changelog: fixed
|
|
The `short` formatting option for `gotestsum` will usually suppress
panics and their stack traces, making it difficult to understand why a
panicking test failed.
Using `standard-verbose` will display the full trace, but this also
gives us the full output of `go test -v`, removing the readability
improvements that `gotestsum` provides.
To work around this, we use the `--jsonfile` option to write out the
full output of `go test -json`, then search for panics in an
`after_script` section with results in a pre-collapsed section.
To avoid unused log files piling up in local testing, by default the
full job output is written to `/dev/null`, with this setting overriden
in the CI config.
|
|
ci: Hopefully decrease flakiness in `test-with-praefect`
See merge request gitlab-org/gitaly!4583
|
|
streamcache: Unlock waiters after cache keys have been pruned
See merge request gitlab-org/gitaly!4589
|
|
ci: Fix test reports not being uploaded
See merge request gitlab-org/gitaly!4594
|
|
With 1c112bb7c (ci: Move test reports into temporary directory,
2022-01-13), we have moved our test reports into a temporary directory
to prepare for running tests as an unprivileged user. This change broke
our ability to upload these test reports as artifacts though because
GitLab only supports artifacts which are relative to the build root.
Fix this issue by instead writing test reports into a new directory in
our build root that's writeable by the unprivileged test user. While at
it, pull out the test user ID into a separate variable so that it can be
documented better.
|
|
We regularly get CI failures when running with Praefect, which is most
likely caused by an exhaustion of the database's connection pool.
Increase the limit so that we can hopefully get to a more stable state.
Note that we cannot set `PGOPTIONS` here as that causes the Postgres
services to not come up. Instead, we work around this by manually adding
the configuration to the executed command.
|
|
We always return the currently pending object's dirty state in case the
request queue has an object set. In case the object has been fully read
without yet having been closed though this won't account for the queue's
outstanding requests. So ultimately, the queue may now says its clean
even though it still got requests pending.
Fix this issue by not returning early in case there is an object. This
causes us to correctly discard any such processes instead of returning
them to the cache with still-pending requests. This is an issue we have
indeed been observing in RPCs which have limits, like ListLFSPointers.
Amend one of our tests to enable cache reuse of catfile processes, which
does surface the issue previous to this fix.
Changelog: fixed
|
|
gitaly-lfs-smudge: Refactor code to be more readily extensible
See merge request gitlab-org/gitaly!4587
|
|
Remove the unused `to io.Writer` parameter from `handleSmudge()`. We
alreay return an `io.ReadCloser` that the contents can be read from, so
there is no need to receive a writer.
|
|
The `GetArchive()` RPC can optionally convert LFS pointers to their
actual contents via the gitaly-lfs-smudge filter. The configuration of
this filter happens via a set of environment variables, which we have
replaced with a single structure in this commit series.
Convert `GetArchive()` to use the new `smudge.Config` to inject the
environment.
|
|
Move the configuration-related code of gitaly-lfs-smudge into a separate
package so that we can access and inject it in Gitaly's services.
|
|
The gitaly-lfs-smudge binary accepts its configuration as a set of
environment variables, which is required so that we can pass it through
Git. Using separate environment variables doesn't scale though: there is
no easily accessible documentation about what is accepted and what is
not, and furthermore every new configuration needs another environment
variable.
Refactor our code to accept a single `GITALY_LFS_SMUDGE_CONFIG` variable
that contains everything needed by gitaly-lfs-smudge to operate. The old
environment variables still exist for backwards-compatibility reasons,
but are overridden in case the new environment variable exists.
|
|
The gitaly-lfs-smudge binary accepts a set of environment variables to
configure it. Because of this the configuration of the binary is kind of
hard to extend: for every piece of information that is required, we need
to add a new environment variable. Ultimately, this leads to a design
where we have lots of magic environment variables injected with no clear
indicator about which ones are used and which ones aren't, similar to
the case we had with gitaly-hooks before introducing the hooks payload.
Refactor the code and create a central `Config` struct which holds all
the configuration required. This prepares us for a fix where we can do
the same like we did for the hooks payload where we only inject a single
JSON-encoded environment variable that can then be parsed into this
struct.
|
|
Add a helper function that can extract environment variables by their
key from an array of environment variables.
|
|
We're about to refactor the way the configuration is handled, which also
includes moving the configuration code into a separate package. Split it
out into a separate file for now to prepare for this move.
|
|
We're about to move loading the configuration out of `smudge()` so that
it doesn't need to depend on any global state anymore. Prepare for this
by pulling out a separate `run()` function so that we can continue to
log any errors and exit with an error code in a single location, only.
|
|
The way `smudge()` handles errors is a bit convoluted right now: in many
paths it will both log the error and return it to the caller, where the
caller should then use it as an indicator to decide whether to exit with
an error code or whether it should exit successfully.
Refactor the code so that we only log any such errors in our `main()`
function to simplify this logic and make it more Go-like. Also, now that
logging is mostly done in the `main()` function, move the initialization
of the logging system close to it.
|
|
We're currently verifying that some of our low-level functions log
certain events into the logfile. This makes it hard to refactor the
code, and ultimately we really should only exercise the system under
test instead of also verifying that some other components like logging
work as expected. As another hurdle, this is effectively testing global
state as the loggers are configured globally.
Remove these assertions from the lower-level unit tests to allow for
easier refactoring of the code. Logging is tested already via our new
high-level tests which verify that executing the binary does the right
thing.
|
|
Because gitaly-lfs-smudge is executed by Git instead of by us we can't
directly log to either stdout or stderr. Instead, we're currently forced
to log into a separate file. The location of that file is controlled by
an environment variable: if present we set up logging, if not we skip
over the configuration.
There's a bug here though: we return a `nil` closer in case we skip
configuring the log and don't initialize the logger at all. First, this
leads to a panic because we unconditionally try to close the closer. And
second, even if this worked, because we don't configure the logger we'd
now log to stdout directly. The result is that the logs would end up in
the final blob's contents as seen by Git.
Fix this bug by always initializing the logger. In case no file is
given, we ask it to simply write to `io.Discard`.
|
|
While we do have tests which verify that parts of the gitaly-lfs-smudge
command run as expected, we don't have any which test the final binary.
This makes it hard to test for some specific scenarios and doesn't give
us an easy way to verify that it works as expected.
Add such tests to improve our test coverage.
|
|
Move functions required for the test setup into a separate file
according to our coding style.
|
|
gitaly/config: Add option to ignore gitconfig files
Closes #4242
See merge request gitlab-org/gitaly!4588
|
|
Parallelize CI jobs
Closes #3123
See merge request gitlab-org/gitaly!4571
|
|
The verify step currently depends on the build step as it will fail
if the Go modules are not downloaded prior to the job executing. If
the modules are not present, golang-ci will fail with a timeout as
it exceeds the timeout while downloading the dependencies. This commit
removes the dependency on build and instead downloads the modules
prior to executing golang-ci. In the general case when the cache is
available, this will allow the verify step to execute in parallel with
the build steps.
|
|
gosec-sast is adding some packages that don't seem necessary. This
commit removes them.
|
|
secret_detection unnecessarily disables inheritance from default.
This commit removes the unnecessary config.
|
|
License scanning is adding some packages that don't seem necessary.
This commit removes them.
|
|
gosec-sast job moves the GOPATH and the cached modules outside the
project directory as the scan will otherwise also scan through the
sources of all the dependencies. This leads to the runtime of the
scan growing massively. The job is currently dependent on the cache
existing from the build step as it unconditionally moves the cache
folder, failing if it doesn't exist. This commit prevents the job
from failing if the cache didn't extract the modules, breaking the
dependency on the build jobs and adds the missing documentation for
the hack.
|
|
The CI drops privileges while running tests. This prevents creating
the required directories to store the test coverage files, thus the
test:coverage job doesn't work without the cache that already
contains the required directories.
To allow the test:coverage job to also run in parallel with the build
steps, this commit overrides the output directory of the coverage
files in the test jobs so they can be created even when the privileges
are dropped, similarly to what is being done with the test result
output with TEST_REPORT variable.
|
|
COVERAGE_DIR in our Makefile controls where the test coverage reports
are outputted. The other variables that control test artifact output
directories begin with TEST_ and are grouped together. This commit
renames COVERAGE_DIR to TEST_COVERAGE_DIR to align it with all the
other variable names. Additionally, it defines the variable with ?=
so the value is only set if it is not already defined. This allows
for the variable to be configured via environment variables.
|
|
This commit adds a regexp to parse the total coverage from the
test:coverage job so GitLab can show it gets shown in the UI and
included in the coverage history of the project.
|
|
Some of the jobs unnecessarily declare that they don't use a cache.
Jobs don't have caches by default, so let's remove the redundant
declarations.
|
|
The testing stage doesn't actually depend on any of the build stage's
artifacts. It only uses the cache which the build stage writes. The
caches are only expired if the relevant files changes like go.sum or
the Gemfile.lock. These caches merely speed up the testing jobs when
they are present. As such, we can remove the build dependency from the
test jobs. Most of the time, the files that make up the cache key are
not changed and thus the both the build step and the test step end up
using the cache. This speeds up the general case by running the steps
in parallel and making test failures visible earlier.
In the rarer case where the cache keys are changed, the test steps will
end up performing some extra work that the cache avoids. This seems like
a fair trade-off to speed up the general case.
There are three exceptions to this though:
'test:coverage' fails without the cache in place as it fails with permission
errors creating the directory for the test coverage report.
'verify' fails with a timeout if the cache is not in place.
'gosec_sast' depends on some files from the cache being moved into the right
place.
As the above jobs fail without the cache, the dependencies are still left
in place for them.
|
|
license_scanning, gemnasium-dependency_scanning and secret_detection
don't use the caches from the earlier build stage, yet they depend
currently on the build stage. Remove the unnecessary dependencies so
they can be executed earlier.
|
|
The QA jobs in our pipeline do not depend on any of the other stages.
The QA stage has been the last one which has thus delayed the execution
of jobs in that stage. Since we are now modeling the dependencies with
the 'needs', this commit removes the unneeded dependency.
|
|
GitLab CI supports a feature that allows for explicitly marking job
dependencies. This allows for visualizing the job dependency graph
and running CI jobs in parallel that do not depend on each other.
This commit adds the needs key where needed to denote job dependencies.
For now, we just model the existing dependency graph by having each stage
depend on the earlier. Later on, we can remove some of the unnecessary
dependencies to speed up the pipeline execution.
|
|
Make Git build with SHA1 and SHA256 routines in FIPS mode
Closes #4210
See merge request gitlab-org/gitaly!4570
|
|
When running our tests, we want Git to use a well-known configuration.
It is thus mandatory that it never picks up the user's gitconfig files,
or otherwise tests may fail due to misconfiguration. To fix this, we
have added a hack that checks whether the currently executing binary has
a `.test` suffix: if it does we assume to be running our tests, and thus
inject a set of environment variable to ignore the gitconfig files.
We now have a better alternative with the newly introduced configuration
to ignore gitconfig files. So let's remove the hack and set this entry
as required. This also proves that the configuration works as expected.
|
|
Fix a cyclic dependency in the gittest package's commit tests so that we
can use the `setup()` function. This change prepares us to drop the test
hack where we inject `GIT_CONFIG_SYSTEM` et al. based on whether the
suffix of the current binary is `.test`.
|
|
We're deprecating use of gitconfig files which exist in the filesystem
in favor of adding `[[git.config]]` entries to Gitaly's `config.toml`.
Add a new option that allows distributions to opt-in to this new
behaviour so that they can migrate earlier if they decide to. This will
become the default with release 16.0.
Changelog: added
|
