| Age | Commit message (Collapse) | Author |
|---|
|
This reverts merge request !4962
|
|
Add scheduled job to run tests on macOS shared runner
Closes #3800
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4646
Merged-by: Will Chandler <wchandler@gitlab.com>
Approved-by: Sami Hiltunen <shiltunen@gitlab.com>
Approved-by: Will Chandler <wchandler@gitlab.com>
Co-authored-by: john.mcdonnell <jmcdonnell@gitlab.com>
|
|
[ci skip]
|
|
'master'
Remove Wiki* RPC handlers - Part 1
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4955
Merged-by: Justin Tobler <jtobler@gitlab.com>
Approved-by: Sami Hiltunen <shiltunen@gitlab.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>
Co-authored-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
|
|
Further tidy up of Markdownlint errors
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4960
Merged-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Co-authored-by: Evan Read <eread@gitlab.com>
|
|
Praefect: Update voter state on failed node RPC
Closes #4088
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4880
Merged-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Co-authored-by: Justin Tobler <jtobler@gitlab.com>
|
|
Added feature flag `node_error_cancels_voter` that enables cancellation
of the voter associated with a failed node RPC. By canceling voters that
can no longer vote, the transaction can fail faster if quorum becomes
impossible. This flag is disabled by default.
|
|
|
|
'4452-feature-flag-roll-out-simplify_find_local_branches_response' into 'master'
featureflag: Remove SimplifyFindLocalBranchesResponse
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4946
Merged-by: Justin Tobler <jtobler@gitlab.com>
Approved-by: Christian Couder <chriscool@tuxfamily.org>
Approved-by: Justin Tobler <jtobler@gitlab.com>
Co-authored-by: Karthik Nayak <knayak@gitlab.com>
|
|
linguist: Implement more robust gitattributes handling
Closes #4522 and #4523
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4936
Merged-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Co-authored-by: Toon Claes <toon@gitlab.com>
|
|
golangci-lint: Update to use Go 1.18
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4963
Merged-by: Pavlo Strokov <pstrokov@gitlab.com>
Approved-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: Pavlo Strokov <pstrokov@gitlab.com>
Co-authored-by: Karthik Nayak <knayak@gitlab.com>
|
|
Makefile: Build git with gitlab-org/git
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4939
Merged-by: John Cai <jcai@gitlab.com>
Approved-by: Toon Claes <toon@gitlab.com>
Approved-by: Christian Couder <chriscool@tuxfamily.org>
|
|
git/stats: Refactor code parsing git-count-objects(1) to use struct
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4956
Merged-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Approved-by: Toon Claes <toon@gitlab.com>
|
|
golangci-lint doesn't work with Go 1.17 anymore, since it has
dependencies which set a minimum version of Go 1.18. This breaks `make
lint` locally since the tool errors out during installation.
Update the go.mod to `go 1.18` and run `go mod tidy` in the package.
|
|
We've seen Gitaly crash in production [1]. This change adds a test case
emulating the crash so we know the changes in previous commits fixes it.
The crash originally happened on [2].
[1]: https://gitlab.com/gitlab-com/gl-infra/production/-/issues/7864
[2]: https://gitlab.com/GodotBuilder/godot3
Issue: https://gitlab.com/gitlab-org/gitaly/-/issues/4523
|
|
We've been having some issues with the implementation of
gitattributes(5) parsing in go-git. Therefore in the previous commit
we've implemented an interface to git-check-attr(1).
With this change we switch to git's default implementation of checking
gitattributes.
Issue: https://gitlab.com/gitlab-org/gitaly/-/issues/4523
Changelog: fixed
|
|
These changes add the gitattributes package which enables callers to get
gitattributes(5) for a bunch of files. It implements a wrapper around
the git-check-attr(1) command and the handling of it's return values.
|
|
Change the prototype of the gitpipe.LsTree skipResult function so it
also can return an error and the gitpipe can be aborted.
|
|
Change the prototype of the gitpipe.DiffTree skipResult function so it
also can return an error and the gitpipe can be aborted.
|
|
We're about to add some tests that run with SHA256 repositories, so we
have to make sure testhelper.Run() is called.
|
|
Revert "Merge branch 'better_ambiguous_ref_errors' into 'master'"
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4962
Merged-by: Toon Claes <toon@gitlab.com>
Approved-by: Toon Claes <toon@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
|
This reverts commit wfb61edd3f (Merge branch
'better_ambiguous_ref_errors' into 'master', 2022-10-21). This commit
introduces improved error handling in the case where there are ambiguous
references. This fix caused a downstream regression in Rails' tests:
```
1) Branches::CreateService#bulk_create when an ambiguous branch name is provided returns an error that branch could not be created
Failure/Error: expect(subject[:message]).to match_array([err_msg])
expected collection contained: ["Failed to create branch 'ambiguous': 13:reference is ambiguous."]
actual collection contained: ["Branch already exists", "Failed to create branch 'ambiguous': invalid reference name 'master'"]
the missing elements were: ["Failed to create branch 'ambiguous': 13:reference is ambiguous."]
the extra elements were: ["Branch already exists", "Failed to create branch 'ambiguous': invalid reference name 'master'"]
# ./spec/services/branches/create_service_spec.rb:66:in `block (4 levels) in <main>'
# ./spec/spec_helper.rb:413:in `block (3 levels) in <top (required)>'
# ./spec/support/sidekiq_middleware.rb:9:in `with_sidekiq_server_middleware'
# ./spec/spec_helper.rb:405:in `block (2 levels) in <top (required)>'
# ./spec/spec_helper.rb:401:in `block (3 levels) in <top (required)>'
# ./lib/gitlab/application_context.rb:58:in `with_raw_context'
# ./spec/spec_helper.rb:401:in `block (2 levels) in <top (required)>'
# ./spec/spec_helper.rb:241:in `block (2 levels) in <top (required)>'
# ./spec/support/system_exit_detected.rb:7:in `block (2 levels) in <main>'
# ./spec/support/flaky_tests.rb:27:in `block (2 levels) in <main>'
# ./spec/support/database/prevent_cross_joins.rb:106:in `block (3 levels) in <main>'
# ./spec/support/database/prevent_cross_joins.rb:60:in `with_cross_joins_prevented'
# ./spec/support/database/prevent_cross_joins.rb:106:in `block (2 levels) in <main>'
# ./spec/support/caching.rb:32:in `block (2 levels) in <main>'
2) Branches::CreateService#execute when an ambiguous branch name is provided returns an error that branch could not be created
Failure/Error: expect(result[:message]).to eq(err_msg)
expected: "Failed to create branch 'feature': 13:reference is ambiguous."
got: "Failed to create branch 'feature': invalid reference name 'master'"
(compared using ==)
# ./spec/services/branches/create_service_spec.rb:195:in `block (4 levels) in <main>'
# ./spec/spec_helper.rb:413:in `block (3 levels) in <top (required)>'
# ./spec/support/sidekiq_middleware.rb:9:in `with_sidekiq_server_middleware'
# ./spec/spec_helper.rb:405:in `block (2 levels) in <top (required)>'
# ./spec/spec_helper.rb:401:in `block (3 levels) in <top (required)>'
# ./lib/gitlab/application_context.rb:58:in `with_raw_context'
# ./spec/spec_helper.rb:401:in `block (2 levels) in <top (required)>'
# ./spec/spec_helper.rb:241:in `block (2 levels) in <top (required)>'
# ./spec/support/system_exit_detected.rb:7:in `block (2 levels) in <main>'
# ./spec/support/flaky_tests.rb:27:in `block (2 levels) in <main>'
# ./spec/support/database/prevent_cross_joins.rb:106:in `block (3 levels) in <main>'
# ./spec/support/database/prevent_cross_joins.rb:60:in `with_cross_joins_prevented'
# ./spec/support/database/prevent_cross_joins.rb:106:in `block (2 levels) in <main>'
# ./spec/support/caching.rb:32:in `block (2 levels) in <main>'
```
|
|
go: Update module github.com/prometheus/client_model to v0.3.0
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4948
Merged-by: Pavlo Strokov <pstrokov@gitlab.com>
Approved-by: Will Chandler <wchandler@gitlab.com>
Approved-by: Pavlo Strokov <pstrokov@gitlab.com>
Co-authored-by: GitLab Renovate Bot <gitlab-bot@gitlab.com>
|
|
UserCreateBranch: Improve "reference is ambiguous" errors
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4950
Merged-by: Toon Claes <toon@gitlab.com>
Approved-by: Toon Claes <toon@gitlab.com>
Co-authored-by: James Fargher <jfargher@gitlab.com>
|
|
[ci skip]
|
|
|
|
In other RPCs that use updateReferenceWithHooks we need to fetch the ref
to figure out what the commit ID is for locking. In the case of a new
branch we always specify this commit ID as zero. Since we have no commit
ID to pass through, this extra reference check does not give us any
protection. The branch could still be created between the initial
reference check and the actual update.
Removing this extra check means that the error message is already marked
as failed precondition correctly. Although it appears that this error
message is less specific this is still and improvement:
* Having the error marked as a failed precondition means that the error
does not trickle up to sentry.
* Although the error looks user facing, gitlab-rails ignores the actual
message.
In future we could capture the actual git errors as part of
updateref.Error. For now the message is hardcoded as part of various
operations endpoints.
|
|
This test simply verifies the unwanted behaviour of this RPC.
Specifically that it returns an internal error instead of a failed
precondition.
|
|
This error does not only happen when the reference name given is not
specific enough, it can also happen when a ref has a path segments the
same as an existing ref. In this case it is useful to specify at least
one of the conflicting refs to reduce the time required to debug such
errors.
|
|
Currently when a secondary node RPC fails the error is ignored and the
transaction continues waiting for additional votes even if there are not
enough outstanding votes to reach quorum. If quorum becomes impossible,
due to the failed node, the transaction hangs until the context gets
canceled. This is not desirable as ideally once it has been established
that there is not enough outstanding votes to reach the required
threshold specified by the transaction, the transaction should be
canceled. This change adapts the `ErrHandler` function of the secondary
nodes to cancel the voter in the transaction associated with the failed
RPC. In this process the voter's result state is updated to
`VoteCanceled` and the subtransaction is checked to see if quorum can
still be achieved. If quorum is impossible the vote is failed and the
voters are unblocked. If quorum is still possible the voters remain
blocked waiting for further votes to decide the outcome.
|
|
Currently there is no means to cancel a voter and recheck for quroum in
a pending transaction. This change adds `CancelTransactionNodeVoter()`
to the `transactions.Manager` API as a means to interact with a
transaction and cancel the voter associated with the specified node.
|
|
Currently when new subtransactions are created the previously canceled
voters are not propagated to the new subtransaction. Once a voter has
been canceled it can no longer vote in the current and all future
subtransactions. Propagating canceled voter state ensures
subtransactions do not wait for an impossible quorum due to canceled
voters. This change separates the subtransaction creation logic out from
`getOrCreateSubtransaction()` into `createSubtransaction()` and also
propigates canceled voters from the previous subtransaction to the new
one.
|
|
Currently the logic to retrieve the pending subtransaction in a
transaction for a node is embedded in `getOrCreateSubtransaction()`.
With future changes to node RPC error handling this same functionality
will also be needed to cancel a voter associated with a failed node.
This change separates this logic into its own function so it can be
reused.
|
|
Currently voters can only be canceled if they have cast a vote.
Voters that have note yet voted should also have the ability to be
canceled in the event of a node RPC failure. This change updates
`updateVoterState()` to be able to cancel a voter and accurately track
vote counts regardless of whether the voter has voted.
|
|
When a node voter is updated a check is performed to determine whether
quorum has already been achieved or if quorum is still attainable. A
similar check is also performed to determine whether the blocked voters
in the pending subtransaction can be unblocked. Future changes to the
node RPC error handling will require that subtransactions signal voters
to unblock once quorum becomes impossible. This change consolidates the
quorum check logic into a single function that can be reused by
`updateVoterState()` and `mustSignalVoters()`.
|
|
|
|
Makefile: Fix race that causes us to miss Protobuf changes
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4959
Merged-by: Sami Hiltunen <shiltunen@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
|
Remove secret from HTTP request headers
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4947
Merged-by: Justin Tobler <jtobler@gitlab.com>
Approved-by: Pavlo Strokov <pstrokov@gitlab.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>
Co-authored-by: Igor Drozdov <idrozdov@gitlab.com>
|
|
Remove the featureflag SimplifyFindLocalBranchesResponse. This was made
default true in 15.5 and now we want to remove the code entirely in
15.6.
Remove test helper functions which are no longer required.
|
|
Now that we have fixed our Makefile target to detect out-of-date
Protobuf code, our CI does indeed complain about the generated code. As
it turns out the code is different depending on the Go version that's
used to compile it as Go 1.19 has made changes to gofmt. We're still
using Go 1.18 though.
Reformat the code with Go 1.18 to fix the failing job. While it is
suboptimal to have version-dependent code generation, it is still a lot
better than not having safety-guards around out-of-date code. Engineers
can also work around this by just fetching generated artifacts from the
failing pipeline.
|
|
ruby: Update dependency google-protobuf to '~> 3.21.8'
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4958
Merged-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: Stan Hu <stanhu@gmail.com>
Co-authored-by: GitLab Renovate Bot <gitlab-bot@gitlab.com>
|
|
The `check-proto` target gets executed as part of the `verify`
target by our CI to assert that code generated from our Protobuf
definitions matches the committed code and that it passes our linting
rules. But the former part to check for changes is racy: `check-proto`
depends on both `proto` and `no-proto-changes` in order to first
regenerate files and then check for changes. But if make(1) is executed
with multiple jobs these can run in parallel as `no-proto-changes` does
not depend on `proto`. Because executing git-diff(1) is faster than
generating the code the end result is that we don't ever notice
out-of-date code.
Fix this error by fixing the dependency chain so that `no-proto-changes`
depends on `proto`.
|
|
More tidy up of violations of new Markdownlint configuration
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4957
Merged-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Co-authored-by: Evan Read <eread@gitlab.com>
|
|
|
|
git: Extend config validation to allow URL keys
Closes #4547
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4952
Merged-by: James Fargher <proglottis@gmail.com>
Approved-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Co-authored-by: James Fargher <jfargher@gitlab.com>
|
|
tools/protoc: Upgrade protoc compiler to v21.7
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4945
Merged-by: Will Chandler <wchandler@gitlab.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>
Approved-by: Will Chandler <wchandler@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
|
[ci skip]
|
|
The stats package contains code that parses git-count-objects(1)'s
output into an unstructured map. This map is then used to provide
multiple different bits of information like the object count or gets
used to write a log message.
This information is more generally useful though. One usecase is to
precompute the current repository's shape in `OptimizeRepository()` and
pass it around for use by our heuristics that determine whether things
need repacking or not. There is also additional information that's not
provided by Git, like whether repositories have bitmaps or not, that we
could easily compute here.
But the current design where we parse information into an unstructured
map is holding us back: it is not discoverable, makes refactorings
unsafe, and is simply not a great interface to use more generally.
Refactor the code into a new `ObjectsInfo` structure instead to change
this. While it requires more intimacy with git-count-objects(1), this
seems like an acceptable tradeoff to make wider use of this interface in
the future.
Note that this also causes us to change the logged format. We don't
consider log messages to be part of our API though, and arguably the new
structure should be easier to understand (e.g. "loose_objects_size"
compared to the old "size").
|
|
We have two different code files that implement statistics specific to
the object database. Merge them into a single file "objects_info.go" to
make them easier to discover.
|
|
Inline the `hasBitmap()` helper functon into the public `HasBitmap()`
function. They both do the same, and the internal helper is not used by
anything.
|
