From 9a81550feddd907f8796362d604f63ed66ad80a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Javier=20L=C3=B3pez?= Date: Mon, 30 Jul 2018 16:55:28 +0000 Subject: Create GPG commit signature in bulk --- app/models/project.rb | 4 +++ app/services/git_push_service.rb | 14 ++++---- app/workers/create_gpg_signature_worker.rb | 16 ++++++--- ...ce-post-receive-create-gpg-siganture-worker.yml | 5 +++ spec/models/project_spec.rb | 10 ++++++ spec/services/git_push_service_spec.rb | 12 +++++-- spec/workers/create_gpg_signature_worker_spec.rb | 38 +++++++++++++++------- 7 files changed, 76 insertions(+), 23 deletions(-) create mode 100644 changelogs/unreleased/fj-37736-improve-performance-post-receive-create-gpg-siganture-worker.yml diff --git a/app/models/project.rb b/app/models/project.rb index b876270116e..da30d2fbb4f 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -548,6 +548,10 @@ class Project < ActiveRecord::Base repository.commit_by(oid: oid) end + def commits_by(oids:) + repository.commits_by(oids: oids) + end + # ref can't be HEAD, can only be branch/tag name or SHA def latest_successful_builds_for(ref = default_branch) latest_pipeline = pipelines.latest_successful_for(ref) diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb index 29c8ce5fea3..f0587667d37 100644 --- a/app/services/git_push_service.rb +++ b/app/services/git_push_service.rb @@ -76,7 +76,7 @@ class GitPushService < BaseService else paths = Set.new - @push_commits.last(PROCESS_COMMIT_LIMIT).each do |commit| + last_pushed_commits.each do |commit| commit.raw_deltas.each do |diff| paths << diff.new_path end @@ -92,7 +92,7 @@ class GitPushService < BaseService end def update_signatures - commit_shas = @push_commits.last(PROCESS_COMMIT_LIMIT).map(&:sha) + commit_shas = last_pushed_commits.map(&:sha) return if commit_shas.empty? @@ -103,16 +103,14 @@ class GitPushService < BaseService commit_shas = Gitlab::Git::Commit.shas_with_signatures(project.repository, commit_shas) - commit_shas.each do |sha| - CreateGpgSignatureWorker.perform_async(sha, project.id) - end + CreateGpgSignatureWorker.perform_async(commit_shas, project.id) end # Schedules processing of commit messages. def process_commit_messages default = default_branch? - @push_commits.last(PROCESS_COMMIT_LIMIT).each do |commit| + last_pushed_commits.each do |commit| if commit.matches_cross_reference_regex? ProcessCommitWorker .perform_async(project.id, current_user.id, commit.to_hash, default) @@ -206,4 +204,8 @@ class GitPushService < BaseService def branch_name @branch_name ||= Gitlab::Git.ref_name(params[:ref]) end + + def last_pushed_commits + @last_pushed_commits ||= @push_commits.last(PROCESS_COMMIT_LIMIT) + end end diff --git a/app/workers/create_gpg_signature_worker.rb b/app/workers/create_gpg_signature_worker.rb index a2da1bda11f..91833941070 100644 --- a/app/workers/create_gpg_signature_worker.rb +++ b/app/workers/create_gpg_signature_worker.rb @@ -3,15 +3,23 @@ class CreateGpgSignatureWorker include ApplicationWorker - def perform(commit_sha, project_id) + def perform(commit_shas, project_id) + return if commit_shas.empty? + project = Project.find_by(id: project_id) return unless project - commit = project.commit(commit_sha) + commits = project.commits_by(oids: commit_shas) - return unless commit + return if commits.empty? # This calculates and caches the signature in the database - Gitlab::Gpg::Commit.new(commit).signature + commits.each do |commit| + begin + Gitlab::Gpg::Commit.new(commit).signature + rescue => e + Rails.logger.error("Failed to create signature for commit #{commit.id}. Error: #{e.message}") + end + end end end diff --git a/changelogs/unreleased/fj-37736-improve-performance-post-receive-create-gpg-siganture-worker.yml b/changelogs/unreleased/fj-37736-improve-performance-post-receive-create-gpg-siganture-worker.yml new file mode 100644 index 00000000000..0b35c5c6786 --- /dev/null +++ b/changelogs/unreleased/fj-37736-improve-performance-post-receive-create-gpg-siganture-worker.yml @@ -0,0 +1,5 @@ +--- +title: Performing Commit GPG signature calculation in bulk +merge_request: 20870 +author: +type: performance diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index b75ca91b007..ef4167a3912 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -3877,6 +3877,16 @@ describe Project do end end + context '#commits_by' do + let(:project) { create(:project, :repository) } + let(:commits) { project.repository.commits('HEAD', limit: 3).commits } + let(:commit_shas) { commits.map(&:id) } + + it 'retrieves several commits from the repository by oid' do + expect(project.commits_by(oids: commit_shas)).to eq commits + end + end + def rugged_config Gitlab::GitalyClient::StorageSettings.allow_disk_access do project.repository.rugged.config diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb index 3f62e7e61e1..657afb2b2b5 100644 --- a/spec/services/git_push_service_spec.rb +++ b/spec/services/git_push_service_spec.rb @@ -761,7 +761,7 @@ describe GitPushService, services: true do end it 'does not queue a CreateGpgSignatureWorker' do - expect(CreateGpgSignatureWorker).not_to receive(:perform_async).with(sample_commit.id, project.id) + expect(CreateGpgSignatureWorker).not_to receive(:perform_async) execute_service(project, user, oldrev, newrev, ref) end @@ -769,7 +769,15 @@ describe GitPushService, services: true do context 'when the signature is not yet cached' do it 'queues a CreateGpgSignatureWorker' do - expect(CreateGpgSignatureWorker).to receive(:perform_async).with(sample_commit.id, project.id) + expect(CreateGpgSignatureWorker).to receive(:perform_async).with([sample_commit.id], project.id) + + execute_service(project, user, oldrev, newrev, ref) + end + + it 'can queue several commits to create the gpg signature' do + allow(Gitlab::Git::Commit).to receive(:shas_with_signatures).and_return([sample_commit.id, another_sample_commit.id]) + + expect(CreateGpgSignatureWorker).to receive(:perform_async).with([sample_commit.id, another_sample_commit.id], project.id) execute_service(project, user, oldrev, newrev, ref) end diff --git a/spec/workers/create_gpg_signature_worker_spec.rb b/spec/workers/create_gpg_signature_worker_spec.rb index aa6c347d738..647a4bcc18e 100644 --- a/spec/workers/create_gpg_signature_worker_spec.rb +++ b/spec/workers/create_gpg_signature_worker_spec.rb @@ -2,21 +2,37 @@ require 'spec_helper' describe CreateGpgSignatureWorker do let(:project) { create(:project, :repository) } + let(:commits) { project.repository.commits('HEAD', limit: 3).commits } + let(:commit_shas) { commits.map(&:id) } context 'when GpgKey is found' do - let(:commit_sha) { '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33' } + let(:gpg_commit) { instance_double(Gitlab::Gpg::Commit) } + + before do + allow(Project).to receive(:find_by).with(id: project.id).and_return(project) + allow(project).to receive(:commits_by).with(oids: commit_shas).and_return(commits) + end + + subject { described_class.new.perform(commit_shas, project.id) } it 'calls Gitlab::Gpg::Commit#signature' do - commit = instance_double(Commit) - gpg_commit = instance_double(Gitlab::Gpg::Commit) + commits.each do |commit| + expect(Gitlab::Gpg::Commit).to receive(:new).with(commit).and_return(gpg_commit).once + end - allow(Project).to receive(:find_by).with(id: project.id).and_return(project) - allow(project).to receive(:commit).with(commit_sha).and_return(commit) + expect(gpg_commit).to receive(:signature).exactly(commits.size).times + + subject + end + + it 'can recover form exception and continue the siganture frocess' do + allow(gpg_commit).to receive(:signature) + allow(Gitlab::Gpg::Commit).to receive(:new).and_return(gpg_commit) + allow(Gitlab::Gpg::Commit).to receive(:new).with(commits.first).and_raise(StandardError) - expect(Gitlab::Gpg::Commit).to receive(:new).with(commit).and_return(gpg_commit) - expect(gpg_commit).to receive(:signature) + expect(gpg_commit).to receive(:signature).exactly(2).times - described_class.new.perform(commit_sha, project.id) + subject end end @@ -24,7 +40,7 @@ describe CreateGpgSignatureWorker do let(:nonexisting_commit_sha) { '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a34' } it 'does not raise errors' do - expect { described_class.new.perform(nonexisting_commit_sha, project.id) }.not_to raise_error + expect { described_class.new.perform([nonexisting_commit_sha], project.id) }.not_to raise_error end end @@ -32,13 +48,13 @@ describe CreateGpgSignatureWorker do let(:nonexisting_project_id) { -1 } it 'does not raise errors' do - expect { described_class.new.perform(anything, nonexisting_project_id) }.not_to raise_error + expect { described_class.new.perform(commit_shas, nonexisting_project_id) }.not_to raise_error end it 'does not call Gitlab::Gpg::Commit#signature' do expect_any_instance_of(Gitlab::Gpg::Commit).not_to receive(:signature) - described_class.new.perform(anything, nonexisting_project_id) + described_class.new.perform(commit_shas, nonexisting_project_id) end end end -- cgit v1.2.3