| Age | Commit message (Collapse) | Author |
|---|
|
While some of our tool versions require the `v` prefix of their tag to
be present, others don't. This is inconsistent and a tag confusing.
Unify this to always require the prefix. This also gives us more
flexbiility given that it's now easily possible to e.g. override the
version with a commit ID.
|
|
Update protoc to v21.1. Note that this is quite a jump from our current
v3.17.3. This isn't all that scary, but a result of upstream changing
their versioning schema to drop the leading `3`. Our generated Go files
remain unchanged.
|
|
Update protoc-gen-go-grpc to v1.2.0. The most notable change is that we
now have headers in our generated gRPC services that indicate the
versions used to generate them. Other than that there are no changes in
our generated files.
|
|
Update protoc-gen-go to v1.28.0. No noticable changes come with this
release, and most notably there are no changes in our generated Go
files.
|
|
Update go-licenses to v1.2.1. One notable change is that the module now
prints out warnings for files it doesn't recognize.
|
|
Update gotestsum to v1.8.1. There aren't any noteworthy changes in our
context.
|
|
Update goimports to v0.1.10. The most important change in our context is
that it now uses a cache, which "should speed up goimports -w *.go
dramatically in cases where impotrs need to be added."
|
|
Update protolint to v0.38.1. The only interesting change is support for
Protocol buffer 3's optional fields, but we don't currently use them
anyway.
|
|
Update to the most recent version of golangci-lint, which is v1.46.1. No
interesting new linters have been added since v1.44.2.
|
|
[ci skip]
|
|
Enable feature flag exact_pagination_token_match by default
See merge request gitlab-org/gitaly!4606
|
|
proto: Introduce new UserDeleteBranchError message
See merge request gitlab-org/gitaly!4604
|
|
We're about to convert UserDeleteBranch to use structured errors. As a
first step we introduce a new UserDeleteBranchError message that hosts
all errors that are expected failures and which may thus be handled by
the client. The RPC is not yet converted to use the new error types
because we first have to update clients to handle them.
|
|
Improve test coverage for UserDeleteBranch to exercise failures caused
by concurrent writes to the same references and when the call to Rails'
`/internal/allowed` interface fails for access checks.
|
|
Contributes to https://gitlab.com/gitlab-org/gitaly/-/issues/3817
Changelog: changed
|
|
Modernize test names to match our current coding style more closely.
|
|
Modernize error messages returned by UserDeleteBranch to match our
modern coding style more closely.
|
|
Remove comment stating that we don't verify the branch name and/or user
in `UserCreateBranch()` while we do in `UserDeleteBranch()`: we do it in
both, so this comment is clearly stale.
|
|
The UserDeleteBranch RPC is missing documentation. Add it to document
its behaviour and expected error cases.
|
|
chore: Update Gemfile.lock to use bundler v2.3.15
See merge request gitlab-org/gitaly!4603
|
|
gitaly-lfs-smudge: Update git-lfs module and dependencies
See merge request gitlab-org/gitaly!4600
|
|
This is just to minimize the versions of bundler used for development.
The GDK runs `support/bundle-install` in this directory to obtain the
version of bundler needed.
This relates to https://gitlab.com/gitlab-org/gitlab/-/issues/364373.
|
|
[ci skip]
|
|
Regenerated via `make notice`
|
|
This version of git-lfs resolves
https://github.com/advisories/GHSA-4g4p-42wc-9f3m, although this
shouldn't affect Gitaly since it only occurs on Windows.
Relates to https://gitlab.com/gitlab-org/gitaly/-/issues/4208
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.
|
