diff options
author | Nick Thomas <nick@gitlab.com> | 2019-08-14 17:38:28 +0300 |
---|---|---|
committer | Nick Thomas <nick@gitlab.com> | 2019-08-14 17:38:28 +0300 |
commit | 3cd40c4a807fef5f99c60853ab81f8729405e315 (patch) | |
tree | 2d644a6b3d4a2a57f4d94117b15b3a31cae1c97d | |
parent | 03c8bb125573d3910689d10b979a6aa59b680ff6 (diff) | |
parent | 4e2bb4e5e7df1273a4d2fdd370b6c17a27c394d8 (diff) |
Merge branch 'sh-optimize-commit-deltas-post-receive' into 'master'
Reduce Gitaly calls in PostReceive
Closes #65878
See merge request gitlab-org/gitlab-ce!31741
-rw-r--r-- | app/models/project.rb | 8 | ||||
-rw-r--r-- | app/services/git/base_hooks_service.rb | 45 | ||||
-rw-r--r-- | changelogs/unreleased/sh-optimize-commit-deltas-post-receive.yml | 5 | ||||
-rw-r--r-- | lib/gitlab/data_builder/push.rb | 5 | ||||
-rw-r--r-- | spec/lib/gitlab/data_builder/push_spec.rb | 34 | ||||
-rw-r--r-- | spec/models/project_spec.rb | 33 | ||||
-rw-r--r-- | spec/services/git/base_hooks_service_spec.rb | 72 | ||||
-rw-r--r-- | spec/services/git/branch_hooks_service_spec.rb | 4 | ||||
-rw-r--r-- | spec/services/git/branch_push_service_spec.rb | 9 | ||||
-rw-r--r-- | spec/services/git/tag_hooks_service_spec.rb | 6 | ||||
-rw-r--r-- | spec/workers/post_receive_spec.rb | 2 |
11 files changed, 205 insertions, 18 deletions
diff --git a/app/models/project.rb b/app/models/project.rb index a6e43efa1f3..0c57ed3e43a 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1230,6 +1230,14 @@ class Project < ApplicationRecord end end + def has_active_hooks?(hooks_scope = :push_hooks) + hooks.hooks_for(hooks_scope).any? || SystemHook.hooks_for(hooks_scope).any? + end + + def has_active_services?(hooks_scope = :push_hooks) + services.public_send(hooks_scope).any? # rubocop:disable GitlabSecurity/PublicSend + end + def valid_repo? repository.exists? rescue diff --git a/app/services/git/base_hooks_service.rb b/app/services/git/base_hooks_service.rb index d30df34e54b..1db18fcf401 100644 --- a/app/services/git/base_hooks_service.rb +++ b/app/services/git/base_hooks_service.rb @@ -19,7 +19,7 @@ module Git update_remote_mirrors - push_data + success end private @@ -33,7 +33,7 @@ module Git end def limited_commits - commits.last(PROCESS_COMMIT_LIMIT) + @limited_commits ||= commits.last(PROCESS_COMMIT_LIMIT) end def commits_count @@ -48,21 +48,25 @@ module Git [] end + # Push events in the activity feed only show information for the + # last commit. def create_events - EventCreateService.new.push(project, current_user, push_data) + EventCreateService.new.push(project, current_user, event_push_data) end def create_pipelines return unless params.fetch(:create_pipelines, true) Ci::CreatePipelineService - .new(project, current_user, push_data) + .new(project, current_user, base_params) .execute(:push, pipeline_options) end def execute_project_hooks - project.execute_hooks(push_data, hook_name) - project.execute_services(push_data, hook_name) + # Creating push_data invokes one CommitDelta RPC per commit. Only + # build this data if we actually need it. + project.execute_hooks(push_data, hook_name) if project.has_active_hooks?(hook_name) + project.execute_services(push_data, hook_name) if project.has_active_services?(hook_name) end def enqueue_invalidate_cache @@ -73,18 +77,35 @@ module Git ) end - def push_data - @push_data ||= Gitlab::DataBuilder::Push.build( - project: project, - user: current_user, + def base_params + { oldrev: params[:oldrev], newrev: params[:newrev], ref: params[:ref], - commits: limited_commits, + push_options: params[:push_options] || {} + } + end + + def push_data_params(commits:, with_changed_files: true) + base_params.merge( + project: project, + user: current_user, + commits: commits, message: event_message, commits_count: commits_count, - push_options: params[:push_options] || {} + with_changed_files: with_changed_files ) + end + + def event_push_data + # We only need the last commit for the event push, and we don't + # need the full deltas either. + @event_push_data ||= Gitlab::DataBuilder::Push.build( + push_data_params(commits: commits.last, with_changed_files: false)) + end + + def push_data + @push_data ||= Gitlab::DataBuilder::Push.build(push_data_params(commits: limited_commits)) # Dependent code may modify the push data, so return a duplicate each time @push_data.dup diff --git a/changelogs/unreleased/sh-optimize-commit-deltas-post-receive.yml b/changelogs/unreleased/sh-optimize-commit-deltas-post-receive.yml new file mode 100644 index 00000000000..cd63b9bf425 --- /dev/null +++ b/changelogs/unreleased/sh-optimize-commit-deltas-post-receive.yml @@ -0,0 +1,5 @@ +--- +title: Reduce Gitaly calls in PostReceive +merge_request: 31741 +author: +type: performance diff --git a/lib/gitlab/data_builder/push.rb b/lib/gitlab/data_builder/push.rb index 40bda3410e1..37fadb47736 100644 --- a/lib/gitlab/data_builder/push.rb +++ b/lib/gitlab/data_builder/push.rb @@ -60,7 +60,8 @@ module Gitlab # rubocop:disable Metrics/ParameterLists def build( project:, user:, ref:, oldrev: nil, newrev: nil, - commits: [], commits_count: nil, message: nil, push_options: {}) + commits: [], commits_count: nil, message: nil, push_options: {}, + with_changed_files: true) commits = Array(commits) @@ -75,7 +76,7 @@ module Gitlab # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/38259 commit_attrs = Gitlab::GitalyClient.allow_n_plus_1_calls do commits_limited.map do |commit| - commit.hook_attrs(with_changed_files: true) + commit.hook_attrs(with_changed_files: with_changed_files) end end diff --git a/spec/lib/gitlab/data_builder/push_spec.rb b/spec/lib/gitlab/data_builder/push_spec.rb index cc31f88d365..e8a9f0b06a8 100644 --- a/spec/lib/gitlab/data_builder/push_spec.rb +++ b/spec/lib/gitlab/data_builder/push_spec.rb @@ -3,9 +3,43 @@ require 'spec_helper' describe Gitlab::DataBuilder::Push do + include RepoHelpers + let(:project) { create(:project, :repository) } let(:user) { build(:user, public_email: 'public-email@example.com') } + describe '.build' do + let(:sample) { RepoHelpers.sample_compare } + let(:commits) { project.repository.commits_between(sample.commits.first, sample.commits.last) } + let(:subject) do + described_class.build(project: project, + user: user, + ref: sample.target_branch, + commits: commits, + commits_count: commits.length, + message: 'test message', + with_changed_files: with_changed_files) + end + + context 'with changed files' do + let(:with_changed_files) { true } + + it 'returns commit hook data' do + expect(subject[:project]).to eq(project.hook_attrs) + expect(subject[:commits].first.keys).to include(*%i(added removed modified)) + end + end + + context 'without changed files' do + let(:with_changed_files) { false } + + it 'returns commit hook data without include deltas' do + expect(subject[:project]).to eq(project.hook_attrs) + expect(subject[:commits].first.keys).not_to include(*%i(added removed modified)) + end + end + end + describe '.build_sample' do let(:data) { described_class.build_sample(project, user) } diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 83ce58235dd..2afe1253e29 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -4312,6 +4312,39 @@ describe Project do end end + describe '#has_active_hooks?' do + set(:project) { create(:project) } + + it { expect(project.has_active_hooks?).to be_falsey } + + it 'returns true when a matching push hook exists' do + create(:project_hook, push_events: true, project: project) + + expect(project.has_active_hooks?(:merge_request_events)).to be_falsey + expect(project.has_active_hooks?).to be_truthy + end + + it 'returns true when a matching system hook exists' do + create(:system_hook, push_events: true) + + expect(project.has_active_hooks?(:merge_request_events)).to be_falsey + expect(project.has_active_hooks?).to be_truthy + end + end + + describe '#has_active_services?' do + set(:project) { create(:project) } + + it { expect(project.has_active_services?).to be_falsey } + + it 'returns true when a matching service exists' do + create(:custom_issue_tracker_service, push_events: true, merge_requests_events: false, project: project) + + expect(project.has_active_services?(:merge_request_hooks)).to be_falsey + expect(project.has_active_services?).to be_truthy + end + end + describe '#badges' do let(:project_group) { create(:group) } let(:project) { create(:project, path: 'avatar', namespace: project_group) } diff --git a/spec/services/git/base_hooks_service_spec.rb b/spec/services/git/base_hooks_service_spec.rb index 4a2ec769116..874df9a68cd 100644 --- a/spec/services/git/base_hooks_service_spec.rb +++ b/spec/services/git/base_hooks_service_spec.rb @@ -14,6 +14,78 @@ describe Git::BaseHooksService do let(:newrev) { "8a2a6eb295bb170b34c24c76c49ed0e9b2eaf34b" } # gitlab-test: git rev-parse refs/tags/v1.1.0 let(:ref) { 'refs/tags/v1.1.0' } + describe '#execute_project_hooks' do + class TestService < described_class + def hook_name + :push_hooks + end + + def commits + [] + end + end + + let(:project) { create(:project, :repository) } + + subject { TestService.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref) } + + context '#execute_hooks' do + before do + expect(project).to receive(:has_active_hooks?).and_return(active) + end + + context 'active hooks' do + let(:active) { true } + + it 'executes the hooks' do + expect(subject).to receive(:push_data).at_least(:once).and_call_original + expect(project).to receive(:execute_hooks) + + subject.execute + end + end + + context 'inactive hooks' do + let(:active) { false } + + it 'does not execute the hooks' do + expect(subject).not_to receive(:push_data) + expect(project).not_to receive(:execute_hooks) + + subject.execute + end + end + end + + context '#execute_services' do + before do + expect(project).to receive(:has_active_services?).and_return(active) + end + + context 'active services' do + let(:active) { true } + + it 'executes the services' do + expect(subject).to receive(:push_data).at_least(:once).and_call_original + expect(project).to receive(:execute_services) + + subject.execute + end + end + + context 'inactive services' do + let(:active) { false } + + it 'does not execute the services' do + expect(subject).not_to receive(:push_data) + expect(project).not_to receive(:execute_services) + + subject.execute + end + end + end + end + describe 'with remote mirrors' do class TestService < described_class def commits diff --git a/spec/services/git/branch_hooks_service_spec.rb b/spec/services/git/branch_hooks_service_spec.rb index 23be400059e..8af51848b7b 100644 --- a/spec/services/git/branch_hooks_service_spec.rb +++ b/spec/services/git/branch_hooks_service_spec.rb @@ -25,7 +25,7 @@ describe Git::BranchHooksService do end describe "Git Push Data" do - subject(:push_data) { service.execute } + subject(:push_data) { service.send(:push_data) } it 'has expected push data attributes' do is_expected.to match a_hash_including( @@ -109,6 +109,7 @@ describe Git::BranchHooksService do expect(event.push_event_payload).to be_an_instance_of(PushEventPayload) expect(event.push_event_payload.commit_from).to eq(oldrev) expect(event.push_event_payload.commit_to).to eq(newrev) + expect(event.push_event_payload.commit_title).to eq('Change some files') expect(event.push_event_payload.ref).to eq('master') expect(event.push_event_payload.commit_count).to eq(1) end @@ -124,6 +125,7 @@ describe Git::BranchHooksService do expect(event.push_event_payload).to be_an_instance_of(PushEventPayload) expect(event.push_event_payload.commit_from).to be_nil expect(event.push_event_payload.commit_to).to eq(newrev) + expect(event.push_event_payload.commit_title).to eq('Initial commit') expect(event.push_event_payload.ref).to eq('master') expect(event.push_event_payload.commit_count).to be > 1 end diff --git a/spec/services/git/branch_push_service_spec.rb b/spec/services/git/branch_push_service_spec.rb index 6e39fa6b3c0..ad5d296f5c1 100644 --- a/spec/services/git/branch_push_service_spec.rb +++ b/spec/services/git/branch_push_service_spec.rb @@ -78,7 +78,10 @@ describe Git::BranchPushService, services: true do it "creates a new pipeline" do expect { subject }.to change { Ci::Pipeline.count } - expect(Ci::Pipeline.last).to be_push + + pipeline = Ci::Pipeline.last + expect(pipeline).to be_push + expect(Gitlab::Git::BRANCH_REF_PREFIX + pipeline.ref).to eq(ref) end end @@ -123,6 +126,10 @@ describe Git::BranchPushService, services: true do describe "Webhooks" do context "execute webhooks" do + before do + create(:project_hook, push_events: true, project: project) + end + it "when pushing a branch for the first time" do expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") diff --git a/spec/services/git/tag_hooks_service_spec.rb b/spec/services/git/tag_hooks_service_spec.rb index f5938a5c708..e362577d289 100644 --- a/spec/services/git/tag_hooks_service_spec.rb +++ b/spec/services/git/tag_hooks_service_spec.rb @@ -26,7 +26,8 @@ describe Git::TagHooksService, :service do describe 'System hooks' do it 'Executes system hooks' do - push_data = service.execute + push_data = service.send(:push_data) + expect(project).to receive(:has_active_hooks?).and_return(true) expect_next_instance_of(SystemHooksService) do |system_hooks_service| expect(system_hooks_service) @@ -40,6 +41,7 @@ describe Git::TagHooksService, :service do describe "Webhooks" do it "executes hooks on the project" do + expect(project).to receive(:has_active_hooks?).and_return(true) expect(project).to receive(:execute_hooks) service.execute @@ -61,7 +63,7 @@ describe Git::TagHooksService, :service do describe 'Push data' do shared_examples_for 'tag push data expectations' do - subject(:push_data) { service.execute } + subject(:push_data) { service.send(:push_data) } it 'has expected push data attributes' do is_expected.to match a_hash_including( object_kind: 'tag_push', diff --git a/spec/workers/post_receive_spec.rb b/spec/workers/post_receive_spec.rb index 4c9fc0e3a24..3b69b81f12e 100644 --- a/spec/workers/post_receive_spec.rb +++ b/spec/workers/post_receive_spec.rb @@ -273,6 +273,8 @@ describe PostReceive do end it "asks the project to trigger all hooks" do + create(:project_hook, push_events: true, tag_push_events: true, project: project) + create(:custom_issue_tracker_service, push_events: true, merge_requests_events: false, project: project) allow(Project).to receive(:find_by).and_return(project) expect(project).to receive(:execute_hooks).twice |