| Age | Commit message (Collapse) | Author |
|---|
|
Add two new helpers `helper.ErrAborted()` and `helper.ErrAbortedf()` to
generate gRPC errors with an `codes.Aborted` error code.
|
|
gRPC HealthCheck messages are quite noisy, dominate the log volume in
Gitaly, and usually are not that useful. We now disable them by
default and add documentation on how to enable them.
Closes https://gitlab.com/gitlab-org/gitaly/-/issues/3428
Changelog: changed
|
|
testhelper.Context() currently return a cancellation function as a
second return value. Majority of the tests do not need to explicitly
cancel the context but they simply defer its cancellation to clean up
after the test. Given this, we can reduce the test verbosity and make
testhelper.Context easier to compose by removing the unnecessary second
return value. This adds a t.Cleanup function to automatically cancel the
context at the end of the tests and omits the returned cancellation
function.
Tests which simply `defer cancel()` have had the extra call removed. Some
tests explicitly call the cancellation, and these tests have been modified
to add context.WithCancel around the testhelper.Context call. There are a
few loctions where testing.TB was passed down to test helpers that create the
context.
|
|
Because check of the authentication token depends on the time
we need to make sure it is synced on the praefect machine and
all gitaly machines that belong to the cluster. That is why
a new check point is added to the 'check' sub-command of the
praefect binary. The task should be run on the praefect node
and doesn't require praefect to be up and running. It is possible
to configure the URL of the NTP service and acceptable time
offset via env variables NTP_HOST and DRIFT_THRESHOLD.
The check has fatal severity because the cluster won't work
correctly if auth checks fail continuously for each request.
Closes: https://gitlab.com/gitlab-org/gitlab/-/issues/342574
Changelog: added
|
|
Extend helpers for parsing env var with
GetDuration function that handles time.Duration
type.
|
|
Prior to this change, ModifyEnvironment was returning a function to
revert the changes done. This was typically passed to defer or to
t.Cleanup().
This change moves calling t.Cleanup() to the ModifyEnvironment function
itself.
|
|
Much of the flakiness we're seeing every day is caused by timeouts which
were too tight. This is only natural: our own developer machines are
much beefier than the CI runners we use, and furthermore they typically
don't suffer from noisy neighbours. So any timeout that works for our
machine is likely to not work for CI workers. It is questionable why
we'd want to have timeouts in the first place: most tests really
shouldn't verify how fast a certain operation is, but instead they
should verify that it does what we expect it to do. Otherwise, if one
cares about speed, one should write a benchmark instead.
One valid reason is to detect tests which are stuck completely, but the
Go runtime handles this for us already with the default 10 minute
timeout for tests. Other usecases which want to test for example
cancellation should really try hard to avoid using timeouts for this,
but instead the system under test should be designed in a way that makes
it possible to easily test it, for example by using manual tickers which
can be injected from a test.
Forbid usage of a subset of functions which we historically used for
creating contexts with timeouts. Most of the tests which used this have
been refactored to not do so anymore, with the exception of some tests
which explicitly want to exercise deadlines themselves.
This is only a first step: we also want to discourage the use of
`time.After()` and `time.Sleep()`. These are left for a different commit
series though.
|
|
This adds logging to SSHUploadPack that shows the stdout response
size. This matches what we already have for PostUploadPack*.
Changelog: other
|
|
Personally, whenever I want to compare gRPC errors and/or messages, I
stand there asking myself where the helper I want is located again. This
is caused by a split we have in test helpers related to gRPC: while we
do have `testhelper.RequireGrpcError()`, checking for actual errors is
done via `testassert.GrpcEqualErr()`. I always have to look up which of
both does what and where it's located.
Fix this by absorbing the helpers from the testassert package into the
testhelper package.
|
|
While our RPCs which create repositories all do mostly the same thing
except for how the repository is seeded, their behaviour is wildly
different when it comes to how preexisting repositories are handled.
E.g. `CreateRepository()` is happy with the repository already existing,
`CreateRepositoryFromBundle()` is happy with the target path to exist as
long as it is an empty directory, and the others disallow the target
path existing altogether. This behaviour is confusing, inconsistent and
has likely grown organically over time.
The issue with this divergent behaviour is that we cannot really
guarantee atomic transactional semantics for the former two RPC calls:
especially in `CreateRepository()`, it is impossible to guarantee that
we either do no changes at all if the call will fail, or to do only
specific changes. This keeps us from implementing proper two-phase
voting in Gitaly Cluster given that we cannot roll back changes, and
neither can we meaningfully lock the repository for any additional
changes.
To fix this, we must assert that the target repository path does not
exist at the time of creation and use proper locking at repository
creation time. This ensures that we can assert an exact state of each
repository on which we want to vote, it ensures that no other RPC calls
modify the repository (it does not exist and cannot be created
concurrently given it is locked), and it ensures that we can roll back
changes in case an error happens by using a temporary repo into which
all changes will first be written.
Implement a new helper function `createRepository()` which handles all
this logic for us:
1. It asserts that the target path does not exist.
2. It creates a temporary repository.
3. This temporary repository is getting seeded by the caller via a
provided callback function.
4. We compute the vote, which is the complete contents of the
repository. This will guarantee that all nodes are about to
perform the same change.
5. The target repository path is locked now, where we assert after
the lock has been taken that no other concurrent RPC call has
created it meanwhile. This ensures that no two concurrent calls
can ever touch this repository and thus it cannot be modified by
anything else.
6. We vote on the state computed in step 4.
7. If the vote was successful, we know that other nodes did end up
with the same result. We thus rename the temporary directory into
place.
8. We do a finalizing vote to assert that we have performed the
change.
This mechanism is thus race-free and can be reused across the different
RPCs. While it is a breaking change that will first require changes in
Ruby, there really is no other way to provide atomicity in the context
of Gitaly Cluster.
|
|
Most of the linting exception we have are about missing documentation of
public variables or comments. Let's inline all of these -- it's hard to
keep track of exceptions and keep the list up-to-date. By having a
`//nolint` directive at the site where the document is missing it
becomes a breeze to directly clean it up. Furthermore, it also acts as a
reminder that one might want to add a comment when one stumbles over any
of these comments.
|
|
Resetting `time.Timer`s is quite tricky in cases where the tick hasn't
been consumed by the caller given that one first has to stop the ticker,
then consume the event in case there is any and finally the ticker can
be reenabled by calling reset. Given that this is not quite obvious
without taking a closer look at `time.Timer`'s documentation this is
easy enough to miss.
Adjust our own `Reset()` wrapper in the helper package to do this dance
such that `Reset()` will reset the timer in all cases.
|
|
The FindLocalBranches accepts a page token in it's request. The caller
can fill this in to get the records from a certain offset. Prior to this
change a lexicographical comparison was used to determine the records
that come *after* the page token. This was done to make sure the page
token will work even if some records were removed in mean time.
This method works fine when the refs are requested in alphabetical
order, but that's not always the case. With this change the comparison
will use an exact match on the page token. When the page token is not
found, it raises an error indicating the page token was not found.
The change is behind a default-disabled feature flag and rolled out
through [1].
1. https://gitlab.com/gitlab-org/gitaly/-/issues/3817
|
|
Setup of a test package's main function is quite repetitive: we call a
separate `testMain()` function such that we can use `defer` calls, this
function calls `MustHaveNoChildProcess()` and `Configure()`, and then we
return the result of `m.Run()`. This is almost always exactly the same,
with some exceptions where we need to have additional setup code.
Unify this code via a single `testhelper.Run()` function which does all
of this. It allows us greater flexibility in the setup code such that we
can easily perform additional validation after the tests without having
to modify all callsites.
Note that this change removes calls to the goleak package in some
places. These will resurface in a later commit in this patch series,
where we instead move this call into `testhelper.Run()` such that it is
executed by default.
|
|
The helpers to inject and extract Gitaly server information from the
context are currently located in the generic "helper" package, even
though required definitions are part of the "storage" package already.
Move them over into the storage package to move related code closer to
each other.
|
|
While the helpers `IncomingToOutgoing()` and `OutgoingToIncoming()` both
fall into the metadata categories, they're still part of the generic
"helper" package. Move them over into the "metadata" package to make
them easier to find.
|
|
global: Support Go-1.17-style build tags
See merge request gitlab-org/gitaly!3804
|
|
With Go 1.17, new syntax was introduced for build tags which has the
intent to be much easier to use compared to the previous `+build` ones.
To ease the migration, Go 1.17 supports both old-style and new-style
build tags, where the recommended migration path is to have both as long
as projects support older Go versions which don't yet know about the new
syntax.
Migrate our codebase to use both both styles. While we don't yet support
Go 1.17 officially, it doesn't hurt to be prepared, and furthermore it
fixes linting issues I have been observing on my machine.
|
|
This commit adds a helper method for wrapping an error with AlreadyExists
status code if the error isn't already a status.
|
|
The gRPC error model allows for two different usages: first the standard
error model where one can return a simple error message associated with
a code. And then the richer error model, where structured data can be
added to errors such that details about the failure mode can passed to
the caller [1]. In Gitaly, we always use the simple error model.
This simple error model is quite limiting though. For example in the
operations service, we have resorted to pass error information to the
caller by embedding error details into the normal response. While not
elegant, it has served us alright in the past. But this design is
awkward, has issues with Praefect if it inspects proxied errors, and is
just bad design in general.
To fix this, this commit implements a new helper function which will
enrich a gRPC status error with additional details. These details can be
an arbitrary protobuf message. Like this, we can pass up arbitrary
structured data to the caller, which will allow us to fix these
shortcomings.
[1]: https://www.grpc.io/docs/guides/error/
|
|
Our helper functions to create gRPC errors is lacking wrappers for
`PermissionDenied` error codes. Introduce them to prepare for a
subsequent patch, where we start to return this code from
`UserMergeBranch`.
|
|
The dependency github.com/golang/protobuf is deprecated and should
be replaced with google.golang.org/protobuf.
This change replaces usage of the ptypes package. The replacement
packages has a different structure and it requires code changes
to be done to migrate.
The exclusion rules removed from the golanci-lint tool configuration
file to prevent future use of the deprecated package.
|
|
The dependency github.com/golang/protobuf is deprecated and should
be replaced with google.golang.org/protobuf.
This change replaces usage of the proto package. The packages have
incompatible API that is why the code changes were done to migrate.
The exclusion rule removed from the golanci-lint tool configuration
file to prevent future use of the deprecated package.
|
|
The `ErrPreconditionFailed{,f}()` helpers are misnamed given that the
gRPC error code is "FailedPrecondition", not "PreconditionFailed".
Rename the functions to match the error code and adjust its callers.
While at it, this commit also converts callsites which needlessly use
`ErrPreconditionFailed(fmt.Errorf(...))` or
`ErrFailedPrecondition(errors.New(...))` to
`ErrFailedPreconditionf(...)`.
|
|
There are no external callsites of `DecorateError()` anymore given that
they've all been converted to use our higher-level error functions.
Make the function private and rename it to `wrapError()` to not grow new
users of it again.
|
|
We're missing some error codes for our gRPC error helpers, forcing
callers to use `DecorateError()` instead. Add them as required by
existing callsites such that we can convert them and internalize
`DecorateError()`.
|
|
We have two kinds of helpers to create errors with gRPC codes: those
which directly take an error and those which take a formatting
directive. These two tests have slightly different semantics: while the
former one will only override error codes in case none was set on the
error, the latter will always set the error code even if it was passed a
"%w" verb. This difference is very subtle and thus easy to miss if one
isn't prepared, making the interface feel fragile.
This commit thus adjusts the formatting helpers to also pay attention to
any preexisting gRPC error codes of "%w" directives by trying to unwrap
the error returned by `fmt.Errorf()`: if it returns an error which has a
gRPC code different from OK and Unknown, then we'll retain that error
code.
This change bridges the gap between both sets of helpers and also makes
the formatting helpers much more useful: in case one wants to retain
error codes, the callers uses "%w", otherwise "%v".
|
|
One of the regression tests we have asserts that the current
implementation of `ErrPreconditionFailedf()` matches what we had before
it had been refactored. While this test was a nice demonstrator back
when the interfaces were refactored, nowadays it doesn't serve much of a
purpose: we know it behaves the same, so there's not much of a need to
retain it now.
Drop the testcase.
|
|
The tests for `Errorf` helpers check for inequality of an error code,
which is much weaker than checking for equality. Convert the test to use
an equality check instead to cast into stone expected semantics.
|
|
We've got two kinds of helper functions to format errors with gRPC codes:
those which do formatting, and those which don't. Given that their
implementation is the same across each of these sets (except for the
error code), it makes more sense to group them by their formatting
nature instead of by their error code.
Reorder them for improved readability.
|
|
The `DecorateError()` helper function explicitly checks whether the
passed error is `nil`. Given that `GrpcCode()` always returns OK in case
it's passed a `nil` error though, and that `DecorateError()` requires
the code to be Unknown in order to wrap it, this condition is needless.
Simplify the code by dropping the needless condition.
|
|
Except for a single use in Praefec's tests, the "storage" module is only
used by Gitaly. Despite that, it's still located at `internal/storage`
and thus indicates that it may be a generic component for reuse.
Move it into `internal/gitaly` to clarify that it is indeed an
implementation detail of Gitaly, only.
|
|
These helpers should make parsing typed environment variables more
consistent.
|
|
|
|
|
|
|
|
logging: Drop topLevelGroup field
Closes #3639
See merge request gitlab-org/gitaly!3556
|
|
The topLevelGroup used to be important for logging as GitLab used to
store repositories in a directory structure mimicking the URL path. For
example `gitlab-org/gitaly` used to be stored at
`gitlab-org/gitaly.git`. Since 14.0 GitLab will no longer use the legacy
storage format this field will always read `@hashed`.
If a field is always a constant, why log it? This change stops logging
it.
Changelog: changed
|
|
The new "v14" version of the Gitaly module is named to match
the next GitLab release. The module versioning is needed in
order to pull gitaly as a dependency in other projects. The
change updates all imports to include v14 version. The go.mod
file was modified as well after go mod tidy execution. And
the changes in dependency licenses are reflected in the NOTICE
file.
Part of: https://gitlab.com/gitlab-org/gitaly/-/issues/3177
|
|
The command package has a function for suppressing the cancellation
of the parent context. This commit moves the functionality to the `helper`
package as it's not strictly only command related. This replaces the existing
tests with more concise ones that do not use time.Sleep.
|
|
Now that the reference-transaction hook can be set up with a
`repository.GitRepo` instead of with a `gitalypb.Repository`, this
commit removes the `helper.ProtoRepoFromRepo()` function and instead
just passes down the interface directly. This gets rid of a potentially
dangerous function which could have lead us to hand down
partially-populated protos to unsuspecting callees.
|
|
The `testhelper.GetTemporaryGitalySocketFileName()` function currently
returns an error. Given that all callers use `require.NoError()` on that
error, let's convert it to instead receive a `testing.TB` and not return
an error.
|
|
Cleanup usages of the global config.Config variable
See merge request gitlab-org/gitaly!2969
|
|
Improve test coverage for helper.Err*() functions
See merge request gitlab-org/gitaly!2934
|
|
The change made in 777a12cfd (wrap errors with grpc context,
2020-05-07) intended to preserve existing GRPC error codes, but as
these tests demonstrate it did the opposite.
We now likely have new code relying on that behavior, so let's
document it in the relevant comments and test for it.
The reason this happens is not because of the removal of the explicit
DecorateError() in that commit, but because of the addition of a
wrapper and the behavior of %v and %w.
The addition of the oldPreconditionFailedf() function demonstrates
that. I.e. the regression didn't have to do with the removal of
DecorateError() (a mere refactoring, we now call
ErrPreconditionFailed() which calls it for us), but the way we're
wrapping the error. See https://blog.golang.org/go1.13-errors and the
%v and %w tests here. I.e. you can still get to the wrapped value, but
what we're going to unpack by default is a different matter.
To test this try:
# Fails, the old behavior
git checkout 495a384d4 -- internal/helper/error.go
go test -v ./internal/helper/ -run TestError -count=1
# OK, the new (buggy) behavior
git checkout -f 777a12cfd -- internal/helper/error.go
go test -v ./internal/helper/ -run TestError -count=1
|
|
The tests just below use "errorf", let's do the same in the first
test. This makes a subsequent diff more readable, as the diff
machinery would otherwise mix & match the two functions in the diff.
|
|
The test just above this uses an "input" variable like this. Let's do
the same for consistency.
|
|
The only consumer of the "Unimplemented" variable is this one
response, let's just move it over there. This commit was split off
from https://gitlab.com/gitlab-org/gitaly/-/merge_requests/2934
|
|
Part of: https://gitlab.com/gitlab-org/gitaly/-/issues/2699
|
|
helper.GetPath is not used anymore in the production
code so we remove it and refactor all usages of it.
Part of: https://gitlab.com/gitlab-org/gitaly/-/issues/2699
|
