| Age | Commit message (Collapse) | Author |
|---|
|
|
|
Danger: Do not label devops::systems indiscriminantly
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6035
Merged-by: Andras Horvath <ahorvath@gitlab.com>
Approved-by: Andras Horvath <ahorvath@gitlab.com>
Co-authored-by: John Cai <jcai@gitlab.com>
|
|
.gitlab-ci.yml: Skip tests for non code changes
Closes #5419
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5997
Merged-by: Will Chandler <wchandler@gitlab.com>
Approved-by: Will Chandler <wchandler@gitlab.com>
Reviewed-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Co-authored-by: John Cai <jcai@gitlab.com>
|
|
housekeeping: Always enable geometric repacking strategy
Closes #5031
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6031
Merged-by: Toon Claes <toon@gitlab.com>
Approved-by: Toon Claes <toon@gitlab.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
|
tools/golangci-lint: Update module golang.org/x/tools to v0.11.0
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6034
Merged-by: Toon Claes <toon@gitlab.com>
Approved-by: Toon Claes <toon@gitlab.com>
Approved-by: Christian Couder <chriscool@tuxfamily.org>
Co-authored-by: GitLab Renovate Bot <gitlab-bot@gitlab.com>
|
|
Fix broken test GetCustomHooks_nonExistentHooks
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6020
Merged-by: Toon Claes <toon@gitlab.com>
Approved-by: James Fargher <jfargher@gitlab.com>
Co-authored-by: Sami Hiltunen <shiltunen@gitlab.com>
|
|
operations: Remove CommitFilesInGit feature flag
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5984
Merged-by: Toon Claes <toon@gitlab.com>
Approved-by: Toon Claes <toon@gitlab.com>
Reviewed-by: karthik nayak <knayak@gitlab.com>
Co-authored-by: John Cai <jcai@gitlab.com>
|
|
tools/goimports: Update module golang.org/x/tools to v0.11.0
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6033
Merged-by: karthik nayak <knayak@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Co-authored-by: GitLab Renovate Bot <gitlab-bot@gitlab.com>
|
|
Add missing fsyncs
Closes #5072
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5999
Merged-by: John Cai <jcai@gitlab.com>
Approved-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: John Cai <jcai@gitlab.com>
Reviewed-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Reviewed-by: Sami Hiltunen <shiltunen@gitlab.com>
Co-authored-by: Sami Hiltunen <shiltunen@gitlab.com>
|
|
Currently we add the devops::systems label irregardless of what group
the author is part of. triage-ops already takes care of applying labels
based on the MR author so this ends up being noisy and unnecessary.
Let's just not set devops::systems since triage-ops already sets the
group label.
|
|
Add operation type annotations on RemoveAll
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6030
Merged-by: Justin Tobler <jtobler@gitlab.com>
Approved-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>
Co-authored-by: Sami Hiltunen <shiltunen@gitlab.com>
|
|
We don't need the full pipeline to run for things in the .gitlab/
folder, nor any documentation in .md files. Create a rule to skip tests
if we detect such files.
|
|
Introduced generic error metadata and improve updateref errors
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6029
Merged-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Reviewed-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Reviewed-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Reviewed-by: karthik nayak <knayak@gitlab.com>
|
|
go: Update module gocloud.dev to v0.30.0
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5974
Merged-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Co-authored-by: GitLab Renovate Bot <gitlab-bot@gitlab.com>
|
|
When we have migrated the Operations service into Gitaly, many of the
RPCs were embedding errors into a successful response via a special
field. These errors were in fact consumed and often also displayed by
Rails, which meant that we were not in a position to actually change
them. This led to a bunch of error messages that don't really carry a
lot of information.
We have since migrated most of the RPCs to instead use structured errors
and are thus free to change those errors to be more meaningful. One such
instance is the dreaded "Could not update $ref. Please refresh and try
again." error message. This error can indeed be caused by a multitude
of different errors, but what the actual root cause is is quite unclear.
Refactor the code and get rid of this error, instead propagating the
actual root cause to the caller. This greatly clarifies the reason why a
reference update has failed and should help us debug such cases better
in the future.
Note that there is once instance left with `UserRevert()` where we have
to return the old-style error as it is still getting embedded into a
successful response. This commit simply inlines that error and adds a
TODO comment on top of it.
|
|
Convert the `NonCommitObjectError` to use error metadata instead of
embedding the information into the error message.
|
|
Convert the `NonExistentObjectError` to use error metadata instead
of embedding the information into the error message.
|
|
Convert the `InTransactionConflictError` to use error metadata instead
of embedding the information into the error message.
|
|
Convert the `FileDirectoryConflictError` to use error metadata instead
of embedding the information into the error message.
|
|
Convert the `InvalidReferenceFormatError` to use error metadata instead
of embedding the information into the error message.
|
|
Convert the `AlreadyLockedError` to use error metadata instead of
embedding the information into the error message.
|
|
We don't gracefully handle the case where updating a reference fails
because the expected value the reference points to is different from the
actual value. Detect this case and return a proper error type so that we
provide better error reporting.
|
|
In order to add metadata to an error one must create a `structerr.Error`
right now. This is a bit roundabout though in the case where you already
have a custom error type that you wish to add metadata to. While you can
play games and implement an `Unwrap()` function on your error type that
returns a `structerr.Error` with metadata, this is indeed quite tedious
and roundabout.
Instead, provide a new `structerr.ErrorMetadater` interface that can be
implemented by custom error types. This allows them to attach metadata
to the custom error type directly via a `ErrorMetadata()` function.
|
|
Callers that wish to extract error metadata from a generic error need to
manually extract the `structerr.Error` first and then call `Metadata()`
on it, which is a bit cumbersome. Furthermore, we're about to extend the
logic to also allow for any other error types to carry metadata via an
interface, which would require them to also be on the lookout for any
such interface types.
Refactor the code and pull out the logic to extract error metadata into
a standalone `structerr.ExtractMetadata()` function. Given a generic
error, it will return all error metadata that is contained in the error
chain. This makes it easy to extend the logic going forward.
Adjust existing callsites to use this new function.
|
|
Improve error reporting for locked references
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6012
Merged-by: Sami Hiltunen <shiltunen@gitlab.com>
Approved-by: Sami Hiltunen <shiltunen@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
|
Implement a request constructor in protoregistry
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6021
Merged-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Co-authored-by: Sami Hiltunen <shiltunen@gitlab.com>
|
|
go: Update module golang.org/x/sys to v0.10.0
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6024
Merged-by: Sami Hiltunen <shiltunen@gitlab.com>
Approved-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: Sami Hiltunen <shiltunen@gitlab.com>
Co-authored-by: GitLab Renovate Bot <gitlab-bot@gitlab.com>
|
|
Object pool removals are currently not synced which may lead to them
coming back in full or in a partial state after a crash. This commit
adds the missing sync.
Changelog: fixed
|
|
Quarantine directory's contents are not synced currently when they
get migrated to the repository's object database. This may lead to
some objects missing after a host crash. This commit adds the missing
syncs for the migrated objects and directories.
There's still a chance of repository corruption as the objects are not
migrated in a dependency order. We may successfully migrate a commit
into the target repository without its tree for example. This should be
handled separately.
Changelog: fixed
|
|
Moving a repository doesn't currently get synced to the disk. This
may lead to the repository not being in the expected location if the
host crashes. Add the missing syncs. Since we don't know if the entire
target directory hierarchy was just created, we'll sync it always.
Changelog: fixed
|
|
Repository removals are currently not synced to the disk. This may
lead to the repository appearing back on the disk either in full or
in corrupted state with deletion partially applied. As we are moving
the repository to a temporary directory before deleting it, we need
to just sync the move the removal from the parent directory in order
to avoid the issues.
Changelog: fixed
|
|
Repository creations are currently not flushed to disk. This can lead
to data loss if the host crashes before the writes have been persisted
to disks. Sync the writes the the disk prior to acknowleding a repository
creations to avoid this. As we don't know whether the directory hierarchy
in the storage existed, we sync them always regardless, and assume that
the root storage directory's entry in the parent directroy is synced
already.
Changelog: fixed
|
|
Run TestListLastCommitsForTree subtests sequentially
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6022
Merged-by: karthik nayak <knayak@gitlab.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Co-authored-by: Sami Hiltunen <shiltunen@gitlab.com>
|
|
Stop validating vanity repo exists in BackupRepository
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6017
Merged-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Approved-by: Christian Couder <chriscool@tuxfamily.org>
Approved-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Co-authored-by: James Fargher <jfargher@gitlab.com>
|
|
smarthttp: Enable SHA256 object format tests for PostReceivePack
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5990
Merged-by: Sami Hiltunen <shiltunen@gitlab.com>
Approved-by: Will Chandler <wchandler@gitlab.com>
Approved-by: Sami Hiltunen <shiltunen@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>
|
|
We have enabled geometric repacking by default in fdd48e61b9
(housekeeping: Default-enable geometric repacking, 2023-05-09), which is
almost two months ago at this time and didn't observe any negative
consequences in production systems. So let's remove the feature flag and
unconditionally enable geometric repacking.
|
|
git: Bump minimum version of Git to 2.41
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6016
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>
|
|
The `intercepted_method` option was used to mark a method as being handled
by Praefect. Since the operation type and scope annotations were mostly for
Praefect's proxy, marking a method as intecepted allowed for dropping the
annotations as they were not needed. As we've now annotated again the methods
that had the option set, let's remove the option as it is no longer needed.
|
|
|
|
RemoveAll is missing operation type and scope annotations. These are
needed to handle starting transactions for RPCs in a general manner.
The RPC is missing the annotations as it is intercepted by Praefect
and the annotations were previously only used by Praefect for deciding
how to proxy the requests. As Gitaly will soon need the annotations
itself, this commit adds the missing annotations.
TransactionService and PraefectInfoService service remain without
annotations. These are only implemented by Praefect and thus Gitaly
doesn't need annotations for them.
ServerService is also still left without annotations as there is no
suitable scope for them anymore. Previously they were annotated with
the now removed Server scope.
|
|
When WriteRef fails because it cannot lock the reference that is to be
updated it returns an internal error. Given that this is quite a likely
error scenario though and that we already have the infrastructure in
place to detect such issues we can do better though and return a more
telling error.
Refactor the RPC to instead return an explicit error with metadata. This
should make it easier for us to detect lock contention better going
forward.
|
|
When trying to delete a reference that is already locked we return a
structured error in DeleteRefs with `codes.FailedPrecondition`. This
fails the litmus test documented in `grpc/codes` though:
> A litmus test that may help a service implementor in deciding
> between FailedPrecondition, Aborted, and Unavailable:
>
> (a) Use Unavailable if the client can retry just the failing call.
> (b) Use Aborted if the client should retry at a higher-level
> (e.g., restarting a read-modify-write sequence).
> (c) Use FailedPrecondition if the client should not retry until
> the system state has been explicitly fixed. E.g., if an "rmdir"
> fails because the directory is non-empty, FailedPrecondition
> should be returned since the client should not retry unless
> they have first fixed up the directory by deleting files from it.
> (d) Use FailedPrecondition if the client performs conditional
> REST Get/Update/Delete on a resource and the resource on the
> server does not match the condition. E.g., conflicting
> read-modify-write on the same resource.
FailedPrecondition should be returned only when the client should not
retry, but given that we are looking at a locking issue that is usually
caused by concurrent calls holding the lock this is wrong. Instead,
Aborted is a better fit as a retry should typically work alright.
Refactor the code to do so.
|
|
When a reference is locked already, then git-update-ref(1) will fail and
tell us. This information is getting parsed by Gitaly and gets returned
to the caller via a special `AlreadyLockedError` so that they can handle
this case more gracefully.
The regular expression we use to parse the error message is lacking
though: while the error may happen in the `prepare` phase, it can also
happen in the `commit` phase when the caller skips explicit preparation
of the transaction. And as Git will prefix the phase to the error, we
need to recognize both of them. Right now, we only recognize `prepare`
errors though.
Fix the issue by extending the regular expression to also detect
`commit` phase errors.
|
|
The `AlreadyLockedError` is quite inconsistent with the other errors
declared in the `updateref` package as it abbreviates the embedded
reference name and is a pointer receiver. Refactor it to improve our
code hygiene.
|
|
The `GeometricRepackingSupportsAlternates()` function detects whether
the given Git version knows to perform geometric repacking in
repositories which are connected to an alternate object database. Since
the required minimum Git version has been increased to v2.41, this check
always returns true.
Remove the function and clean up its call sites.
|
|
|
|
|
|
repository: Make WriteRef compatible with SHA256 object format
See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6001
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>
|
|
The `MidxDeletesRedundantBitmaps()` function detects whether the given
Git version deletes redundant pack-based bitmaps when writing
multi-pack-indices. Since the required minimum Git version has been
increased to v2.41, this check always returns true.
Remove the function and clean up its call sites.
|
|
The `PatchIDRespectsBinaries()` function detects whether the given Git
version correctly handles binary diffs when computing a patch ID. Since
the required minimum Git version has been increased to v2.41, this check
always returns true.
Remove the function and clean up its call sites.
|
