Commit a18eea8c authored by Stan Hu's avatar Stan Hu

Automatically cleanup stale worktrees and lock files upon a push

git 2.16 will fail badly if there are stale worktrees.

Closes #44115
parent 44f4a674
......@@ -421,7 +421,7 @@ group :ed25519 do
end
# Gitaly GRPC client
gem 'gitaly-proto', '~> 0.91.0', require: 'gitaly'
gem 'gitaly-proto', '~> 0.94.0', require: 'gitaly'
gem 'grpc', '~> 1.10.0'
# Locked until https://github.com/google/protobuf/issues/4210 is closed
......
......@@ -290,7 +290,7 @@ GEM
po_to_json (>= 1.0.0)
rails (>= 3.2.0)
gherkin-ruby (0.3.2)
gitaly-proto (0.91.0)
gitaly-proto (0.94.0)
google-protobuf (~> 3.1)
grpc (~> 1.0)
github-linguist (5.3.3)
......@@ -1061,7 +1061,7 @@ DEPENDENCIES
gettext (~> 3.2.2)
gettext_i18n_rails (~> 1.8.0)
gettext_i18n_rails_js (~> 1.3)
gitaly-proto (~> 0.91.0)
gitaly-proto (~> 0.94.0)
github-linguist (~> 5.3.3)
gitlab-flowdock-git-hook (~> 1.0.1)
gitlab-markup (~> 1.6.2)
......
---
title: Automatically cleanup stale worktrees and lock files upon a push
merge_request:
author:
type: fixed
......@@ -1369,6 +1369,18 @@ module Gitlab
raise CommandError.new(e)
end
def clean_stale_repository_files
gitaly_migrate(:repository_cleanup, status: Gitlab::GitalyClient::MigrationStatus::OPT_OUT) do |is_enabled|
gitaly_repository_client.cleanup if is_enabled && exists?
end
rescue Gitlab::Git::CommandError => e # Don't fail if we can't cleanup
Rails.logger.error("Unable to clean repository on storage #{storage} with path #{path}: #{e.message}")
Gitlab::Metrics.counter(
:failed_repository_cleanup_total,
'Number of failed repository cleanup events'
).increment
end
def branch_names_contains_sha(sha)
gitaly_migrate(:branch_names_contains_sha) do |is_enabled|
if is_enabled
......@@ -1463,6 +1475,33 @@ module Gitlab
run_git!(['rev-list', '--max-count=1', oldrev, "^#{newrev}"])
end
def with_worktree(worktree_path, branch, sparse_checkout_files: nil, env:)
base_args = %w(worktree add --detach)
# Note that we _don't_ want to test for `.present?` here: If the caller
# passes an non nil empty value it means it still wants sparse checkout
# but just isn't interested in any file, perhaps because it wants to
# checkout files in by a changeset but that changeset only adds files.
if sparse_checkout_files
# Create worktree without checking out
run_git!(base_args + ['--no-checkout', worktree_path], env: env)
worktree_git_path = run_git!(%w(rev-parse --git-dir), chdir: worktree_path).chomp
configure_sparse_checkout(worktree_git_path, sparse_checkout_files)
# After sparse checkout configuration, checkout `branch` in worktree
run_git!(%W(checkout --detach #{branch}), chdir: worktree_path, env: env)
else
# Create worktree and checkout `branch` in it
run_git!(base_args + [worktree_path, branch], env: env)
end
yield
ensure
FileUtils.rm_rf(worktree_path) if File.exist?(worktree_path)
FileUtils.rm_rf(worktree_git_path) if worktree_git_path && File.exist?(worktree_git_path)
end
private
def local_write_ref(ref_path, ref, old_ref: nil, shell: true)
......@@ -1549,33 +1588,6 @@ module Gitlab
File.exist?(path) && !clean_stuck_worktree(path)
end
def with_worktree(worktree_path, branch, sparse_checkout_files: nil, env:)
base_args = %w(worktree add --detach)
# Note that we _don't_ want to test for `.present?` here: If the caller
# passes an non nil empty value it means it still wants sparse checkout
# but just isn't interested in any file, perhaps because it wants to
# checkout files in by a changeset but that changeset only adds files.
if sparse_checkout_files
# Create worktree without checking out
run_git!(base_args + ['--no-checkout', worktree_path], env: env)
worktree_git_path = run_git!(%w(rev-parse --git-dir), chdir: worktree_path).chomp
configure_sparse_checkout(worktree_git_path, sparse_checkout_files)
# After sparse checkout configuration, checkout `branch` in worktree
run_git!(%W(checkout --detach #{branch}), chdir: worktree_path, env: env)
else
# Create worktree and checkout `branch` in it
run_git!(base_args + [worktree_path, branch], env: env)
end
yield
ensure
FileUtils.rm_rf(worktree_path) if File.exist?(worktree_path)
FileUtils.rm_rf(worktree_git_path) if worktree_git_path && File.exist?(worktree_git_path)
end
def clean_stuck_worktree(path)
return false unless File.mtime(path) < 15.minutes.ago
......
......@@ -238,6 +238,11 @@ module Gitlab
end
def check_change_access!(changes)
# If there are worktrees with a HEAD pointing to a non-existent object,
# calls to `git rev-list --all` will fail in git 2.15+. This should also
# clear stale lock files.
project.repository.clean_stale_repository_files
changes_list = Gitlab::ChangesList.new(changes)
# Iterate over all changes to find if user allowed all of them to be applied
......
......@@ -19,6 +19,11 @@ module Gitlab
response.exists
end
def cleanup
request = Gitaly::CleanupRequest.new(repository: @gitaly_repo)
GitalyClient.call(@storage, :repository_service, :cleanup, request)
end
def garbage_collect(create_bitmap)
request = Gitaly::GarbageCollectRequest.new(repository: @gitaly_repo, create_bitmap: create_bitmap)
GitalyClient.call(@storage, :repository_service, :garbage_collect, request)
......
......@@ -2257,6 +2257,39 @@ describe Gitlab::Git::Repository, seed_helper: true do
end
end
describe '#clean_stale_repository_files' do
let(:worktree_path) { File.join(repository.path, 'worktrees', 'delete-me') }
it 'cleans up the files' do
repository.with_worktree(worktree_path, 'master', env: ENV) do
FileUtils.touch(worktree_path, mtime: Time.now - 8.hours)
# git rev-list --all will fail in git 2.16 if HEAD is pointing to a non-existent object,
# but the HEAD must be 40 characters long or git will ignore it.
File.write(File.join(worktree_path, 'HEAD'), Gitlab::Git::BLANK_SHA)
# git 2.16 fails with "fatal: bad object HEAD"
expect { repository.rev_list(including: :all) }.to raise_error(Gitlab::Git::Repository::GitError)
repository.clean_stale_repository_files
expect { repository.rev_list(including: :all) }.not_to raise_error
expect(File.exist?(worktree_path)).to be_falsey
end
end
it 'increments a counter upon an error' do
expect(repository.gitaly_repository_client).to receive(:cleanup).and_raise(Gitlab::Git::CommandError)
counter = double(:counter)
expect(counter).to receive(:increment)
expect(Gitlab::Metrics).to receive(:counter).with(:failed_repository_cleanup_total,
'Number of failed repository cleanup events').and_return(counter)
repository.clean_stale_repository_files
end
end
describe '#delete_remote_branches' do
subject do
repository.delete_remote_branches('downstream-remote', ['master'])
......
......@@ -855,6 +855,20 @@ describe Gitlab::GitAccess do
admin: { push_protected_branch: false, push_all: false, merge_into_protected_branch: false }))
end
end
context 'when pushing to a project' do
let(:project) { create(:project, :public, :repository) }
let(:changes) { "#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/heads/wow" }
before do
project.add_developer(user)
end
it 'cleans up the files' do
expect(project.repository).to receive(:clean_stale_repository_files).and_call_original
expect { push_access_check }.not_to raise_error
end
end
end
describe 'build authentication abilities' do
......
......@@ -17,6 +17,16 @@ describe Gitlab::GitalyClient::RepositoryService do
end
end
describe '#cleanup' do
it 'sends a cleanup message' do
expect_any_instance_of(Gitaly::RepositoryService::Stub)
.to receive(:cleanup)
.with(gitaly_request_with_path(storage_name, relative_path), kind_of(Hash))
client.cleanup
end
end
describe '#garbage_collect' do
it 'sends a garbage_collect message' do
expect_any_instance_of(Gitaly::RepositoryService::Stub)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment