Skip to content

Speed up commit-based access checks for new refs again

Patrick Steinhardt requested to merge pks-access-checks-use-filtered-quarantine into master

This issue is to rollout the feature on production, that is currently behind the filter_quarantined_commits feature flag.

With d86514f1 (checks: Fix revalidation of preexisting commits, 2021-12-14), we had to disable use of the quarantine directory to find new commits for our access checks in case any pushed reference has a blank old revision, which is the case for all branch creations. The reason for this is that the quarantine directory may contain commits which indeed exist in the target repository already, and if we didn't have an old revision we couldn't find the set of new commits by doing an in-memory graph walk because we had no cut-off point.

As was known back then, this change is a huge performance regression: if we don't use the quarantine directory, we instead use git rev-list --not --all $newrev, which is forced to walk all references which exist in the repository to mark preexisting commits as uninteresting. This of course scales with both the number of commits and with the number of references. In our own gitlab-org/gitlab, this takes around 23 seconds to compute, which is plain unacceptable for our access checks. On the other side, using the quarantine directory for this only takes about 5 milliseconds.

To fix this issue, Gitaly has implemented a new CheckObjectsExist() RPC. Given a set of revisions, Gitaly will determine for each of them whether they exist or not in a specific repository. Equipped with this RPC, we can now filter down the set of quarantined commits to the set of actually-new commits by asking the repository whether it knows the quarantined objects already. Note though: we must make sure that the repository we send as part of this query does not contain any reference to the quarantine directory, or otherwise we'd wrongfully claim that all quarantine objects exist already.

The feature flag enables this new logic and should thus significantly reduce the time it takes to perform access checks in repositories like gitlab-org/gitlab.

Fixes gitaly#3751 (closed).

Fixes #360630 (closed).

Edited by Patrick Steinhardt

Merge request reports