| Age | Commit message (Collapse) | Author |
|---|
|
|
|
There are several RPCs that need to fetch a specific SHA from a remote
repo over gitaly internal ssh. This extraction is intended to centralise
this code.
|
|
repository: Remove feature flag for direct fetches
Closes #3685
See merge request gitlab-org/gitaly!3716
|
|
Add Ruby 3 based test job
Closes #3715
See merge request gitlab-org/gitaly!3702
|
|
|
|
lint: Fix deprecated import github.com/golang/protobuf/jsonpb
See merge request gitlab-org/gitaly!3710
|
|
repository: Remove feature flag for atomic repo creation from bundles
Closes #3686
See merge request gitlab-org/gitaly!3717
|
|
helper: Improve gRPC error interfaces and retain error codes in formatting directives
See merge request gitlab-org/gitaly!3708
|
|
repository: Fix SetFullPath writing multiple entries
See merge request gitlab-org/gitaly!3712
|
|
When not using the `--atomic` flag for git-fetch(1), then Git will
create a separate reference transaction per reference that is to be
fetched. This can be extremely expensive in case we fetch thousands or
even tens of thousands of references given that we'll have to execute
the gitaly-hooks binary each time, connect to the hooks service, cast
votes and then wait for quorum. This was fixed via the introduction of
`CreateRepositoryFromBundleAtomicFetch`, which causes us to pass that
flag.
This feature has been default-enabled for three weeks in production now,
and furthermore it's been backported to v13.12 and v14.0. No problems
were observed in production, and it fixed the performance issue for
customers.
Given its exposure, let's remove the feature flag without taking the
intermediate step of first default-enabling it.
Changelog: performance
|
|
With the `ReplicateRepositoryDirectFetch` feature flag, we have fixed
`FetchInternalRemote()` to work in a context where called via Praefect.
Previously, we have propagated transactional information via an RPC call,
which then resulted in the remote side trying to cast votes against a
Gitaly connection, which cannot work. With the new code, we don't do an
RPC call but instead just call the relevant function directly.
This feature has been default-enabled for three weeks in production now,
and furthermore it's been backported to v13.12 and v14.0. No problems
were observed in production, and it fixed the original issue for
customers.
Given its exposure, let's remove the feature flag without taking the
intermediate step of first default-enabling it.
Changelog: fixed
|
|
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.
|
|
Now that we have all required helper functions to wrap errors with gRPC
codes, this commit converts all callers which use `DecorateError()` to
instead use the higher-level helper functions.
|
|
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.
|
|
The `config.Add()` function is not used anywhere in our codebase, and
most callers would actually want to use `config.Set()` because the
former one will add multi-value config entries. Remove `config.Add()`
and its localrepo implementation.
|
|
[ci skip]
|
|
The new `SetFullPath()` RPC uses `git.Config.Add()` to write the path
into the gitconfig, which internally uses `git config --add`. What this
does is to amend the config entry to the gitconfig while keeping all old
entries, effectively turning the config entry into a multivar.
Fix this by using the new `git.Config.Set()` function instead, which
will replace any preexisting values.
Changelog: fixed
|
|
The `git.Config` interface currently only provides a function to append
multivalue config entries to the gitconfig. In the general case though,
it's more likely that the user wants to set a single entry, where any
preexisting values are overridden.
Implement a new `config.Set()` function which does exactly that.
|
|
[ci skip]
|
|
repository: Implement new `SetFullPath()` RPC
See merge request gitlab-org/gitaly!3706
|
|
Implement new ListAllBlobs RPC
Closes #3716
See merge request gitlab-org/gitaly!3703
|
|
The dependency github.com/golang/protobuf is deprecated and should
be replaced with google.golang.org/protobuf.
This change replaces usage of the jsonpb package with encoding/protojson.
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.
|
|
Since quite some time, we have tried to get rid of calls which modify
the on-disk gitconfig file. For one these include calls which add or
remove remotes, which are soon to be removed from Gitaly now that Rails
doesn't call them anymore. On the other hand, this also includes calls
which modify the gitconfig directly. There are currently two upstream
callers of those RPCs:
- The first usecase is to set up JWT credentials for `FetchRemote()`
calls. Those credentials are passed via HTTP headers, which is in
fact already supported by `FetchRemote` without having to manually
write them into the gitconfig. Those are thus easy to replace.
- The second usecase is to set "gitlab.fullpath": this key simply
records the repository's path in GitLab such that it's easy to see
for admins what repository this is. This has been implemented to
bridge the gap between the old storage layout and our new hashed
storages.
This commit addresses the second usecase by introducing a new RPC
`SetFullPath()`. Instead of allowing callers to modify the gitconfig
arbitrarily, this will only allow callers to explicitly set
"gitlab.fullpath". With this in place, we can eventually remove
`AddConfig()` and `RemoveConfig()` given that they're not required
anymore.
Changelog: added
|
|
Implement a new ListAllBlobs RPC, which is the equivalent of
ListAllLFSPointers and ListAllCommits but for blobs: given a repository,
it will return all blobs no matter whether they're reachable or not.
This will eventually be used to efficiently enumerate all new blobs in
Rails' access checks.
Changelog: feature
|
|
We're about to add another RPC which sends out blobs, where logic to
process the blobs will be largely the same as for `ListBlobs()`. To
avoid code duplication, this commit splits out most of the logic to
retrieve blobs, split them up into multiple messages if required, and
then finally to pass them back to the caller.
|
|
Add a new ListAllBlobs RPC which lists all blobs of the repository, no
matter whether they're reachable via any of the references or not. The
design is the same as for ListAllLFSPointers.
|
|
While not likely, it could be that a repository is missing a gitconfig
due to whatever reason. While we can mostly cope with it just fine, in
the context of transactional voting it would mean that voting fails
because we cannot retrieve the file's contents to compute the vote.
Fix this issue by handling the case where the config doesn't exist via a
constructed vote on an artificial value.
Changelog: fixed
|
|
Fix various UpdateRemoteMirror bugs
Closes #3508, #3505, #3503, #3504, and #3502
See merge request gitlab-org/gitaly!3699
|
|
backup: support of the Cloud storages
Closes #3633
See merge request gitlab-org/gitaly!3687
|
|
Fixes for tag-related RPCs
See merge request gitlab-org/gitaly!3694
|
|
tracing: Improve instrumentation of catfile.Batch
See merge request gitlab-org/gitaly!3688
|
|
UpdateRemoteMirror currently may encounter symbolic references when
comparing the source and the mirror repositories. This can cause various
issues when the RPC later tries to push symbolic refs, which then get resolved
to the real references. As handling the symbolic refs is fairly undefined and
Git repositories should generally not contain other symbolic references
than the HEADs, this commit changes UpdateRemoteMirror to completely ignore
any symbolic refs it encounters. This avoids various failure cases that can
come up when trying to mirror them.
Changelog: fixed
|
|
GetRemoteReferences currently doesn't differentiate symbolic refs from
normal references. This makes it difficult for calling code to handle
symbolic references separately. UpdateRemoteMirror should ignore symbolic
references. This commit extends GetRemoteReferences to return whether or
not a reference is a symbolic ref.
|
|
UpdateRemoteMirror currently may force push over diverged refs even if
KeepDivergentRefs is set in the request. This can happen as the RPC
first checks the refs on the remote to see if they have diverged or not.
After these checks, it then force pushes to all refs that have not
diverged. If the refs diverged on the mirror between the RPC checking
them and the force push, the push would overwrite the diverged refs.
This commit prevents accidentally force pushing over the diverged refs
by doing a normal push instead of a force push if KeepDivergentRefs is
set. This then causes the RPC to fail instead of overwriting the refs if
the race manifested.
Changelog: fixed
|
|
localrepo's Push force pushes by default. This commit makes it an
optional behavior. Updates also the only call site of the function in
UpdateRemoteMirror to always push with force to keep the existing
behavior, until changed in a later commit.
|
|
UpdateRemoteMirror's Ruby implementation did not handle the case when
there were no branches in the repository that source repository when
mirroring and failed when trying to access its default branch. The RPC
then failed with a NoMethodError when trying to invoke a method on a
Nil object. The Go implementation matched the behavior to pass the existing
tests. Now that the Go implementation is enabled always, we can drop
this behavior and allow the repository mirroring to work even if there
are no branches in the source repository.
Changelog: fixed
|
|
UpdateRemoteMirror's Ruby implementation did not use fully qualified names
when pushing references to the mirror. If a branch and tag had the same name,
the resulting git push command would be `git push <remote> <name> <name>`. Git
doesn't allow for this and fails with 'src refspec master matches more than one'.
The Go port of the RPC matched the behavior in order to keep compatiblity with the
tests. Now that the feature flag is removed, we can drop this behavior and allow
pushing to work even if a tag and branch share the same name as the Go implementation
pushes using fully qualified ref names.
Changelog: fixed
|
|
The Ruby implementation of UpdateRemoteMirror didn't handle references
by their fully qualified names. This caused the mirroring to fail when
attempting to push the tag with `git push <remote> tag` as command
expected the tags name to follow. The Go port mirrored the behavior to
keep the existing tests passing. Now that the feature flag is removed,
we can drop this behavior and allow the mirroring to work regardless of
the branch name.
Changelog: fixed
|
|
|
|
Remove redundant test setup
Closes #3703
See merge request gitlab-org/gitaly!3698
|
|
remote: Remove hardcoded branch name from error
See merge request gitlab-org/gitaly!3701
|
|
Currently if an existing branch/tag we're mirroring matches an existing
reference the error message will always refer to 'master'. This is
confusing to users as there's no guarantee this is the problem ref.
This commit updates the error message to use the name of the reference
that triggered the failure.
The error message checked in
'fails_if_tag_and_branch_named_the_same_update_remote_mirror_test' is
shorted to end at 'refs/' because the failed ref returned is
non-deterministic. Either 'refs/heads/master' or 'refs/tags/master' may
be returned here.
Changelog: fixed
|
|
[ci skip]
|
|
into 'master'
featureflag: Enable CreateRepositoryFromBundleAtomicFetch by default
See merge request gitlab-org/gitaly!3696
|
