summaryrefslogtreecommitdiff
path: root/spec/services/git
diff options
context:
space:
mode:
authorStan Hu <stanhu@gmail.com>2019-08-12 15:31:58 -0700
committerStan Hu <stanhu@gmail.com>2019-08-12 22:28:49 -0700
commit4e2bb4e5e7df1273a4d2fdd370b6c17a27c394d8 (patch)
tree6da4006221d92fd0f059e71e0dd6e7269ab38b6a /spec/services/git
parent6480bd6dc9f2e32bf3651606e836516cad65c2c8 (diff)
downloadgitlab-ce-4e2bb4e5e7df1273a4d2fdd370b6c17a27c394d8.tar.gz
Reduce Gitaly calls in PostReceivesh-optimize-commit-deltas-post-receive
This commit reduces I/O load and memory utilization during PostReceive for the common case when no project hooks or services are set up. We saw a Gitaly N+1 issue in `CommitDelta` when many tags or branches are pushed. We can reduce this overhead in the common case because we observe that most new projects do not have any Web hooks or services, especially when they are first created. Previously, `BaseHooksService` unconditionally iterated through the last 20 commits of each ref to build the `push_data` structure. The `push_data` structured was used in numerous places: 1. Building the push payload in `EventCreateService` 2. Creating a CI pipeline 3. Executing project Web or system hooks 4. Executing project services 5. As the return value of `BaseHooksService#execute` 6. `BranchHooksService#invalidated_file_types` We only need to generate the full `push_data` for items 3, 4, and 6. Item 1: `EventCreateService` only needs the last commit and doesn't actually need the commit deltas. Item 2: In addition, `Ci::CreatePipelineService` only needed a subset of the parameters. Item 5: The return value of `BaseHooksService#execute` also wasn't being used anywhere. Item 6: This is only used when pushing to the default branch, so if many tags are pushed we can save significant I/O here. Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/65878 Fic
Diffstat (limited to 'spec/services/git')
-rw-r--r--spec/services/git/base_hooks_service_spec.rb72
-rw-r--r--spec/services/git/branch_hooks_service_spec.rb4
-rw-r--r--spec/services/git/branch_push_service_spec.rb9
-rw-r--r--spec/services/git/tag_hooks_service_spec.rb6
4 files changed, 87 insertions, 4 deletions
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',