Commit f33dd1b5 authored by Stan Hu's avatar Stan Hu Committed by Kamil Trzciński

Reduce Gitaly calls in BuildHooksWorker

`Ci::Build.execute_hooks` always made Gitaly calls regardless of whether
a project had any hooks or services. Now we lazily only perform these
calls only if there are active hooks or services.

This is similar to what we did for `PostReceive` in
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/31741/diffs.

Relates to
https://gitlab.com/gitlab-com/gl-infra/infrastructure/issues/8407
parent 9121f475
......@@ -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
......
---
title: Reduce Gitaly calls in BuildHooksWorker
merge_request: 20365
author:
type: performance
......@@ -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)
......
......@@ -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
......
......@@ -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
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