Merge branch 'fj-37736-improve-performance-post-receive-create-gpg-siganture-worker' into 'master'

Create GPG commit signature in bulk

See merge request gitlab-org/gitlab-ce!20870

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)

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

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

changelogs/unreleased/fj-37736-improve-performance-post-receive-create-gpg-siganture-worker.yml

+---
+title: Performing Commit GPG signature calculation in bulk
+merge_request: 20870
+author:
+type: performance

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

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

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