diff options
| -rw-r--r-- | Gemfile | 2 | ||||
| -rw-r--r-- | Gemfile.lock | 4 | ||||
| -rw-r--r-- | changelogs/unreleased/sh-add-cleanup-rpc-gitaly.yml | 5 | ||||
| -rw-r--r-- | lib/gitlab/git/repository.rb | 66 | ||||
| -rw-r--r-- | lib/gitlab/git_access.rb | 5 | ||||
| -rw-r--r-- | lib/gitlab/gitaly_client/repository_service.rb | 5 | ||||
| -rw-r--r-- | spec/lib/gitlab/git/repository_spec.rb | 33 | ||||
| -rw-r--r-- | spec/lib/gitlab/git_access_spec.rb | 14 | ||||
| -rw-r--r-- | spec/lib/gitlab/gitaly_client/repository_service_spec.rb | 10 |
9 files changed, 114 insertions, 30 deletions
@@ -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 diff --git a/Gemfile.lock b/Gemfile.lock index 55e7bd9492a..1e6fe7638a9 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -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) diff --git a/changelogs/unreleased/sh-add-cleanup-rpc-gitaly.yml b/changelogs/unreleased/sh-add-cleanup-rpc-gitaly.yml new file mode 100644 index 00000000000..81b48fc255b --- /dev/null +++ b/changelogs/unreleased/sh-add-cleanup-rpc-gitaly.yml @@ -0,0 +1,5 @@ +--- +title: Automatically cleanup stale worktrees and lock files upon a push +merge_request: +author: +type: fixed diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index a12907d1e6d..5678c28cf3a 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -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 diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 6a01957184d..01f8b22b2b6 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -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 diff --git a/lib/gitlab/gitaly_client/repository_service.rb b/lib/gitlab/gitaly_client/repository_service.rb index e1bc2f9ab61..b5a734aaef6 100644 --- a/lib/gitlab/gitaly_client/repository_service.rb +++ b/lib/gitlab/gitaly_client/repository_service.rb @@ -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) diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index 382a22b93a3..f934bf9a6b0 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -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']) diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index b845abab5ef..c870997e274 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -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 diff --git a/spec/lib/gitlab/gitaly_client/repository_service_spec.rb b/spec/lib/gitlab/gitaly_client/repository_service_spec.rb index 1c41dbcb9ef..111b1b35fa0 100644 --- a/spec/lib/gitlab/gitaly_client/repository_service_spec.rb +++ b/spec/lib/gitlab/gitaly_client/repository_service_spec.rb @@ -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) |