Merge branch 'sh-reduce-gitaly-calls-in-ci-build' into 'master'

Reduce Gitaly calls in BuildHooksWorker

See merge request gitlab-org/gitlab!20365

app/models/ci/build.rb

@@ -662,9 +662,8 @@ module Ci
     def execute_hooks
       return unless project
 
-      build_data = Gitlab::DataBuilder::Build.build(self)
-      project.execute_hooks(build_data.dup, :job_hooks)
-      project.execute_services(build_data.dup, :job_hooks)
+      project.execute_hooks(build_data.dup, :job_hooks) if project.has_active_hooks?(:job_hooks)
+      project.execute_services(build_data.dup, :job_hooks) if project.has_active_services?(:job_hooks)
     end
 
     def browsable_artifacts?
@@ -873,6 +872,10 @@ module Ci
 
     private
 
+    def build_data
+      @build_data ||= Gitlab::DataBuilder::Build.build(self)
+    end
+
     def successful_deployment_status
       if deployment&.last?
         :last

changelogs/unreleased/sh-reduce-gitaly-calls-in-ci-build.yml

+---
+title: Reduce Gitaly calls in BuildHooksWorker
+merge_request: 20365
+author:
+type: performance

spec/controllers/projects/jobs_controller_spec.rb

@@ -154,7 +154,7 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do
           .and_return(merge_request)
       end
 
-      it 'does not serialize builds in exposed stages', :sidekiq_might_not_need_inline do
+      it 'does not serialize builds in exposed stages' do
         get_show_json
 
         json_response.dig('pipeline', 'details', 'stages').tap do |stages|
@@ -183,7 +183,7 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do
         context 'job is cancelable' do
           let(:job) { create(:ci_build, :running, pipeline: pipeline) }
 
-          it 'cancel_path is present with correct redirect', :sidekiq_might_not_need_inline do
+          it 'cancel_path is present with correct redirect' do
             expect(response).to have_gitlab_http_status(:ok)
             expect(response).to match_response_schema('job/job_details')
             expect(json_response['cancel_path']).to include(CGI.escape(json_response['build_path']))
@@ -193,7 +193,7 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do
         context 'with web terminal' do
           let(:job) { create(:ci_build, :running, :with_runner_session, pipeline: pipeline) }
 
-          it 'exposes the terminal path', :sidekiq_might_not_need_inline do
+          it 'exposes the terminal path' do
             expect(response).to have_gitlab_http_status(:ok)
             expect(response).to match_response_schema('job/job_details')
             expect(json_response['terminal_path']).to match(%r{/terminal})
@@ -268,7 +268,7 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do
           project.add_maintainer(user) # Need to be a maintianer to view cluster.path
         end
 
-        it 'exposes the deployment information', :sidekiq_might_not_need_inline do
+        it 'exposes the deployment information' do
           get_show_json
 
           expect(response).to have_gitlab_http_status(:ok)
@@ -292,7 +292,7 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do
             sign_in(user)
           end
 
-          it 'user can edit runner', :sidekiq_might_not_need_inline do
+          it 'user can edit runner' do
             get_show_json
 
             expect(response).to have_gitlab_http_status(:ok)
@@ -312,7 +312,7 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do
             sign_in(user)
           end
 
-          it 'user can not edit runner', :sidekiq_might_not_need_inline do
+          it 'user can not edit runner' do
             get_show_json
 
             expect(response).to have_gitlab_http_status(:ok)
@@ -331,7 +331,7 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do
             sign_in(user)
           end
 
-          it 'user can not edit runner', :sidekiq_might_not_need_inline do
+          it 'user can not edit runner' do
             get_show_json
 
             expect(response).to have_gitlab_http_status(:ok)
@@ -412,7 +412,7 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do
       context 'when job has trace' do
         let(:job) { create(:ci_build, :running, :trace_live, pipeline: pipeline) }
 
-        it "has_trace is true", :sidekiq_might_not_need_inline do
+        it "has_trace is true" do
           get_show_json
 
           expect(response).to match_response_schema('job/job_details')
@@ -458,7 +458,7 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do
           create(:ci_pipeline_variable, pipeline: pipeline, key: :TRIGGER_KEY_1, value: 'TRIGGER_VALUE_1')
         end
 
-        context 'user is a maintainer', :sidekiq_might_not_need_inline do
+        context 'user is a maintainer' do
           before do
             project.add_maintainer(user)
 
@@ -512,7 +512,7 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do
 
     def get_show_json
       expect { get_show(id: job.id, format: :json) }
-        .not_to change { Gitlab::GitalyClient.get_request_count }
+        .to change { Gitlab::GitalyClient.get_request_count }.by(1) # ListCommitsByOid
     end
 
     def get_show(**extra_params)

spec/controllers/projects/pipelines_controller_spec.rb

@@ -93,7 +93,7 @@ describe Projects::PipelinesController do
     end
 
     context 'when performing gitaly calls', :request_store do
-      it 'limits the Gitaly requests', :sidekiq_might_not_need_inline do
+      it 'limits the Gitaly requests' do
         # Isolate from test preparation (Repository#exists? is also cached in RequestStore)
         RequestStore.end!
         RequestStore.clear!
@@ -101,8 +101,9 @@ describe Projects::PipelinesController do
 
         expect(::Gitlab::GitalyClient).to receive(:allow_ref_name_caching).and_call_original
 
+        # ListCommitsByOid, RepositoryExists, HasLocalBranches
         expect { get_pipelines_index_json }
-          .to change { Gitlab::GitalyClient.get_request_count }.by(2)
+          .to change { Gitlab::GitalyClient.get_request_count }.by(3)
       end
     end
 

spec/models/ci/build_spec.rb

@@ -4063,4 +4063,54 @@ describe Ci::Build do
       expect(job.invalid_dependencies).to eq([pre_stage_job_invalid])
     end
   end
+
+  describe '#execute_hooks' do
+    context 'with project hooks' do
+      before do
+        create(:project_hook, project: project, job_events: true)
+      end
+
+      it 'execute hooks' do
+        expect_any_instance_of(ProjectHook).to receive(:async_execute)
+
+        build.execute_hooks
+      end
+    end
+
+    context 'without relevant project hooks' do
+      before do
+        create(:project_hook, project: project, job_events: false)
+      end
+
+      it 'does not execute a hook' do
+        expect_any_instance_of(ProjectHook).not_to receive(:async_execute)
+
+        build.execute_hooks
+      end
+    end
+
+    context 'with project services' do
+      before do
+        create(:service, active: true, job_events: true, project: project)
+      end
+
+      it 'execute services' do
+        expect_any_instance_of(Service).to receive(:async_execute)
+
+        build.execute_hooks
+      end
+    end
+
+    context 'without relevant project services' do
+      before do
+        create(:service, active: true, job_events: false, project: project)
+      end
+
+      it 'execute services' do
+        expect_any_instance_of(Service).not_to receive(:async_execute)
+
+        build.execute_hooks
+      end
+    end
+  end
 end