Age | Commit message (Collapse) | Author |
|
hooks: Fix flaky test on MacOS
Closes #4806
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5415
Merged-by: Justin Tobler <jtobler@gitlab.com>
Approved-by: Pavlo Strokov <pstrokov@gitlab.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>
Co-authored-by: Will Chandler <wchandler@gitlab.com>
|
|
'4797-flaky-test-testtransactionmanager-invalid_reference_aborts_the_entire_transaction-flaky-2' into 'master'
gitaly: Don't wrap `updateref.ErrInvalidReferenceFormat`
Closes #4797
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5393
Merged-by: John Cai <jcai@gitlab.com>
Approved-by: Sami Hiltunen <shiltunen@gitlab.com>
Approved-by: John Cai <jcai@gitlab.com>
Co-authored-by: Karthik Nayak <knayak@gitlab.com>
|
|
Fix quirky problems with tracing
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5406
Merged-by: Justin Tobler <jtobler@gitlab.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>
Reviewed-by: Will Chandler <wchandler@gitlab.com>
Co-authored-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
|
|
[ci skip]
|
|
We have observed intermittent failures on
`TestServer_PackObjectsHook_separateContext` on our MacOS runners. The
error occurs ~10% of the time on MacOS, but has been completely
unreproducible on Linux.
This appears to be a long-standing issue with FreeBSD, dating back to
2005[0], which MacOS has inherited. Unix domain sockets that are closed
after a partial read may return `ENOTCONN`, instead of `EPIPE` as seen
on Linux. This is exactly the scenario performed in this test.
Using the original reproduction script provided in the linked bug
report, the `ENOTCONN` issue occurs ~10% of the time, matching Gitaly's
failure rate.
Given that this functionality works as expected on Linux and the OS
itself is what is non-deterministic, the simplest fix is to accept
either `EPIPE` or `ENOTCONN` when on MacOS. We are forced to check for
the expected error string instead of `errors.Is(err, syscall.ENOTCONN)`
as the syscall information is lost when returned over gRPC to the test
program.
[0] https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=79138
|
|
As mentioned in the previous commit, `require` calls `t.FailNow()`,
which cannot safely be used in helper goroutines. Convert
``TestServer_PackObjectsHook_separateContext` to use `assert` outside
of the main goroutine.
|
|
Most of our testhelper functions use the `require` family of assertions,
which internally use use `t.FailNow()`. However, this is not safe for
use goroutines other than the one actually running the test function.
Calling it from a helper goroutine will not determinstically stop the
test and may lead to spurious failures from the race detector.
Add an `AssertGrpcCode` function that uses `assert`, which can safely be
called from a helper goroutine.
|
|
Git Catfile cache is an optimization. It re-uses git-cat-file process if
it's already open by other goroutines. As a result, the process's life
span may be stretched outside the request starting the process. At
present, that span is attributed to the first request. That makes the
timing visualization of the whole trace looks weird. This commit
detaches that span from the starting request.
|
|
In the prior commit, some new utility methods were implemented to wrap
around tracing libraries. This commit replaces the existing calls to
opentracing library by new methods. Some some advanced usage still holds
references to opentracing, though.
|
|
Tracing is now powered by opentracing and labkit. They work fine, but
lack of some functionality, such as discarding all spans of path. In
addition, in the future, we may need to switch the tracing library. It
makes sense to implement a thin wrapper providing minimal interfaces.
This commit implements some foundation methods for this purpose.
|
|
go: Update module go.uber.org/goleak to v1.2.1
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5409
Merged-by: James Fargher <proglottis@gmail.com>
Approved-by: Will Chandler <wchandler@gitlab.com>
Approved-by: James Fargher <proglottis@gmail.com>
Co-authored-by: GitLab Renovate Bot <gitlab-bot@gitlab.com>
|
|
go: Update module github.com/golang-jwt/jwt/v4 to v4.5.0
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5405
Merged-by: Will Chandler <wchandler@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Approved-by: Will Chandler <wchandler@gitlab.com>
Co-authored-by: GitLab Renovate Bot <gitlab-bot@gitlab.com>
|
|
go: Update github.com/ProtonMail/go-crypto digest to 7d5c6f0
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5404
Merged-by: Will Chandler <wchandler@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Approved-by: Will Chandler <wchandler@gitlab.com>
Co-authored-by: GitLab Renovate Bot <gitlab-bot@gitlab.com>
|
|
[ci skip]
|
|
'master'
gitaly: Delete log entry after being applied
Closes #4735
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5341
Merged-by: Will Chandler <wchandler@gitlab.com>
Approved-by: Sami Hiltunen <shiltunen@gitlab.com>
Approved-by: Will Chandler <wchandler@gitlab.com>
Reviewed-by: Will Chandler <wchandler@gitlab.com>
Reviewed-by: Sami Hiltunen <shiltunen@gitlab.com>
Reviewed-by: karthik nayak <knayak@gitlab.com>
Co-authored-by: Karthik Nayak <knayak@gitlab.com>
|
|
blob: Generate repository with LFS test data
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5402
Merged-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Reviewed-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Reviewed-by: Justin Tobler <jtobler@gitlab.com>
Co-authored-by: Justin Tobler <jtobler@gitlab.com>
|
|
|
|
Now with the ability to generate test repositories with LFS pointers on
the fly, update the LFS tests to stop using seed repositories.
|
|
Currently tests in `lfs_pointers_test.go` rely on seeded repositories
for test data. This caused an issue when a seeded repository was updated
resulting in `TestListAllLFSPointers` consistently failing. This change
adds `setupWithLFS()` which configures a git repository with LFS
pointers for test data.
|
|
Add John Cai to support template
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5407
Merged-by: John Cai <jcai@gitlab.com>
Approved-by: John Cai <jcai@gitlab.com>
Co-authored-by: Mark Wood <mwood@gitlab.com>
|
|
'4698-featureflag-enable-use-of-quarantined-repository-in-fetchsourcebranch' into 'master'
featureflag: Remove `FetchSourceBranchQuarantined`
Closes #4698
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5262
Merged-by: karthik nayak <knayak@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
|
|
|
The flag `FetchSourceBranchQuarantined` is rolled out, now we can delete
the flag and modify the code/tests to remove the previous logic.
|
|
|
|
|
|
go: Update github.com/ProtonMail/go-crypto digest to 81033d7
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5394
Merged-by: Pavlo Strokov <pstrokov@gitlab.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>
Approved-by: Pavlo Strokov <pstrokov@gitlab.com>
Co-authored-by: GitLab Renovate Bot <gitlab-bot@gitlab.com>
|
|
middleware: Change method lookup warning log to debug
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5395
Merged-by: Toon Claes <toon@gitlab.com>
Approved-by: Toon Claes <toon@gitlab.com>
Approved-by: Sami Hiltunen <shiltunen@gitlab.com>
Co-authored-by: James Fargher <jfargher@gitlab.com>
|
|
hooks: Implement custom hooks replication
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5248
Merged-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Reviewed-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Reviewed-by: Justin Tobler <jtobler@gitlab.com>
Co-authored-by: Justin Tobler <jtobler@gitlab.com>
|
|
Currently the `ReplicateRepository` RPC does not include the replication
of custom hooks for a repository. This change updates the RPC to include
replication of repository hooks stored in the `custom_hooks` directory
when the `gitaly_replicate_repository_hooks` feature flag is enabled.
The hooks replication is handled by `setCustomHooksTransaction()` from
the `SetCustomHooks` RPC which enables atomicity and transactionality.
|
|
Currently the `SetCustomHooks` RPC is set up in such a way that requires
a tar archive to be provided as input. In a future commit the
`ReplicateRepository` RPC will be updated to replicate hooks and
leverage the logic from `SetCustomHooks` to set hooks for the
repository. Since some repositories may not contain any custom hooks,
the logic needs to be updated to support this additional use case.
This change updates `extractHooks()` to be able to support receiving an
empty `io.Reader` as input without producing an error. Since the version
of tar used at runtime is system dependent, the code is generalized to
support both GNU and BSD tar.
|
|
Currently tests for the `SetCustomHooks` RPC rely on pregenerated tar
archives present in the `testdata/` directory. These archives can
instead be generated on the fly by reusing the tar archiving logic from
the `GetCustomHooks` RPC. This change factors out this logic into
`archive.Write()` which gets used by `GetCustomHooks` and several tests
to generate tar archives.
|
|
In `TransactionManager`, we don't Prune a log entry after it is
applied. This means the key lies in the database forever while providing
no additional value. Let's start pruning entries in the log after being
applied, we do this in `TransactionManager.applyLogEntry` wherein after
the transaction is applied we go ahead and delete the entry.
Since the entries are deleted, some of the tests need modification to
check the state of the database before deletion, modify all such tests.
|
|
The `hookContext` structure is provided to the handler of every
synchronization points. When we introduce the deletion of entries in the
write-ahead log, we want to check the state of the database and
pass/fail the test. To do this, we need the {database, test} structs.
Let's add them to the `hookContext`.
We override the default `testing.TB` object with our custom
`testingHook` struct, which overrides calls to `FailNow()` with calls to
`Fail()`, since the former can only be run in the main goroutine. This
allows us to now not worry about using `FailNow()` (this is the function
called by the `require` library).
|
|
The `TestTransactionManager` only adds hooks when it first creates the
manager. This means if the manager was created in step X, the hooks from
step X+1 won't be added unless the manages is stopped/started. To avoid
this issue, we create a new `applyHooks` function and call this at the
beginning of each step.
|
|
|
|
go: Update module github.com/jackc/pgx/v4 to v4.18.0
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5390
Merged-by: Sami Hiltunen <shiltunen@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: Sami Hiltunen <shiltunen@gitlab.com>
Co-authored-by: GitLab Renovate Bot <gitlab-bot@gitlab.com>
|
|
remote: Support for SHA256 object format
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5368
Merged-by: Pavlo Strokov <pstrokov@gitlab.com>
Approved-by: Pavlo Strokov <pstrokov@gitlab.com>
Reviewed-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Reviewed-by: Pavlo Strokov <pstrokov@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
|
tracker: Fix racey error insertion in test
Closes #4796
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5391
Merged-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>
Reviewed-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Co-authored-by: Will Chandler <wchandler@gitlab.com>
|
|
Disable background verifier in tests
Closes #4773
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5399
Merged-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Co-authored-by: Sami Hiltunen <shiltunen@gitlab.com>
|
|
git/stats: Fix race when verifying clone timings
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5401
Merged-by: Justin Tobler <jtobler@gitlab.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
|
`TestErrorTracker_ClearErrors` would occasionally flake when the
insertion time of the second write error exactly matched `now`, causing
all write errors to be cleared.
Use the new `incr{Read,Write}ErrTime()` methods to ensure that the
errors times fall before or after our cutoff time.
|
|
The `Incr{Read,Write}ErrTime()` methods on `errorTracker` will use
`time.Now()` as the time of error. For production use this is a good
default and we should keep it, but when testing the `clear()` method
having a way to deterministicly set the time of error is useful. We have
experienced flakes when errors are inserted at _exactly_ the same time
as the cutoff.
Add internal methods `incr{Read,Write}ErrTime` that allow the caller to
specify the time of error. The public methods wrap around this with
`time.Now()` for convience.
|
|
This reverts commit ae63665f49c5acaf65bf847932843b583b485c4f.
|
|
Praefect's background verifier was recently changed to delete
invalid metadata by default. This can cause test flakes as some test
cases purposefully create such states. Given it would be surprising
to have the background verifier issue RPCs and delete state during
a test, disable it in the tests.
|
|
One of the things that the git/stats package does is to record timings
for various different steps in a fetch. As we cannot tell how long it
takes for each of these steps to complete the best we can do in the test
is to verify that timings are in the expected order.
The check we have for this is flaky though as it may happen that two
packets are observed in the same instant, but we test for times being in
strictly-increasing order:
=== FAIL: internal/git/stats TestClone (0.50s)
http_clone_test.go:73:
Error Trace: /builds/gitlab-org/gitaly/internal/git/stats/http_clone_test.go:73
Error: Should be true
Test: TestClone
Messages: post: expect time to receive first progress message (134.178921ms) to be greater than previous value 134.178921ms
Fix this flake by also allowing for the same time stamp.
|
|
housekeeping: Enable writing MIDX by default
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5397
Merged-by: karthik nayak <knayak@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
|
localrepo: Allow unrelated histories in merge tree
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5385
Merged-by: karthik nayak <knayak@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Reviewed-by: karthik nayak <knayak@gitlab.com>
Co-authored-by: John Cai <jcai@gitlab.com>
|
|
In `prepareReferenceTransaction` we first call `updater.Update` and then
`updater.Prepare`. In `updater.Prepare` we check for
`updateref.ErrInvalidReferenceFormat` and if found, we wrap it with
`gitaly.InvalidReferenceFormatError`.
This is flaky, since we don't check for the same in `updater.Update`.
Since the error from git can occur in either the `Update` or `Prepare`
stage, we could end up with either `updateref.ErrInvalidReferenceFormat`
or `gitaly.InvalidReferenceFormatError`. Let's fix this racy-flake by
just relying and testing for `updateref.ErrInvalidReferenceFormat`.
|
|
We have recently introduced the logic to write multi-pack indices during
repository maintenance. This new data structure allows Git to look up
objects more efficiently when there are many packfiles in a repository
by providing an index that spans across all the packfiles.
The code has been rolled out to production systems without any observed
issues. As this is a significant new data structure though we're playing
it safe and release the feature with its flag default enabled.
Changelog: changed
|
|
All RPCs of the remote service now support the SHA256 object format.
Adapt our tests to test with it.
|