From 2831981aaf7af84c372393c9605ed52a07c61b08 Mon Sep 17 00:00:00 2001 From: Robert May Date: Tue, 20 Oct 2020 13:15:23 +0100 Subject: [PATCH 1/8] Run PushRuleCheck in parallel Ensures the two push rule checks inside .validate! run in parallel as they both feature potentially-slow IO when loading data from Gitaly. --- .../ops/parallel_push_checks.yml | 7 +++ ee/lib/ee/gitlab/checks/push_rule_check.rb | 48 +++++++++++++++++++ .../ee/gitlab/checks/push_rule_check_spec.rb | 24 ++++++++-- 3 files changed, 76 insertions(+), 3 deletions(-) create mode 100644 ee/config/feature_flags/ops/parallel_push_checks.yml diff --git a/ee/config/feature_flags/ops/parallel_push_checks.yml b/ee/config/feature_flags/ops/parallel_push_checks.yml new file mode 100644 index 00000000000000..958cadc3241708 --- /dev/null +++ b/ee/config/feature_flags/ops/parallel_push_checks.yml @@ -0,0 +1,7 @@ +--- +name: parallel_push_checks +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/45668 +rollout_issue_url: +type: ops +group: group::source_code +default_enabled: false diff --git a/ee/lib/ee/gitlab/checks/push_rule_check.rb b/ee/lib/ee/gitlab/checks/push_rule_check.rb index 34edd6a9122d32..0764577c30f6cb 100644 --- a/ee/lib/ee/gitlab/checks/push_rule_check.rb +++ b/ee/lib/ee/gitlab/checks/push_rule_check.rb @@ -7,6 +7,16 @@ class PushRuleCheck < ::Gitlab::Checks::BaseChecker def validate! return unless push_rule + if ::Feature.enabled?(:parallel_push_checks, type: :ops) + run_checks_in_parallel! + else + run_checks_in_sequence! + end + end + + private + + def run_checks_in_sequence! if tag_name PushRules::TagCheck.new(change_access).validate! else @@ -15,6 +25,44 @@ def validate! PushRules::FileSizeCheck.new(change_access).validate! end + + def run_checks_in_parallel! + parallel_checks do + parallelize do + if tag_name + PushRules::TagCheck.new(change_access).validate! + else + PushRules::BranchCheck.new(change_access).validate! + end + end + + parallelize do + PushRules::FileSizeCheck.new(change_access).validate! + end + end + end + + def parallel_checks + @threads = [] + + yield + + @threads.each(&:join) + + ensure + @threads.each(&:exit) + end + + def parallelize + @threads << Thread.new do + Thread.current.tap do |t| + t.abort_on_exception = true + t.report_on_exception = false + end + + yield + end + end end end end diff --git a/ee/spec/lib/ee/gitlab/checks/push_rule_check_spec.rb b/ee/spec/lib/ee/gitlab/checks/push_rule_check_spec.rb index 1efdca234b24b9..f527c25282a9e6 100644 --- a/ee/spec/lib/ee/gitlab/checks/push_rule_check_spec.rb +++ b/ee/spec/lib/ee/gitlab/checks/push_rule_check_spec.rb @@ -7,10 +7,16 @@ let(:push_rule) { create(:push_rule, :commit_message) } - describe '#validate!' do + shared_examples "push checks" do before do - expect_any_instance_of(EE::Gitlab::Checks::PushRules::FileSizeCheck) - .to receive(:validate!) + allow_any_instance_of(EE::Gitlab::Checks::PushRules::FileSizeCheck) + .to receive(:validate!).and_return(nil) + end + + it "raises an error on failure" do + allow_any_instance_of(EE::Gitlab::Checks::PushRules::FileSizeCheck).to receive(:validate!).and_raise(Gitlab::GitAccess::ForbiddenError) + + expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError) end context 'when tag name exists' do @@ -43,4 +49,16 @@ end end end + + describe '#validate!' do + it_behaves_like "push checks" + + context ":parallel_push_checks feature is disabled" do + before do + stub_feature_flags(parallel_push_checks: false) + end + + it_behaves_like "push checks" + end + end end -- GitLab From 316b40f258788ff0ec93493f0f5abfe268137a8b Mon Sep 17 00:00:00 2001 From: Robert May Date: Tue, 20 Oct 2020 16:47:14 +0100 Subject: [PATCH 2/8] Add changelog entry --- changelogs/unreleased/parallel-push-check.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/parallel-push-check.yml diff --git a/changelogs/unreleased/parallel-push-check.yml b/changelogs/unreleased/parallel-push-check.yml new file mode 100644 index 00000000000000..4b1de04e77a361 --- /dev/null +++ b/changelogs/unreleased/parallel-push-check.yml @@ -0,0 +1,5 @@ +--- +title: Run PushRuleCheck in parallel +merge_request: 45668 +author: +type: performance -- GitLab From fe554b3bdb2b525898dc3e1b30c99e1b8689476e Mon Sep 17 00:00:00 2001 From: Robert May Date: Wed, 21 Oct 2020 11:34:27 +0100 Subject: [PATCH 3/8] Maintain original return value --- ee/lib/ee/gitlab/checks/push_rule_check.rb | 3 ++- ee/spec/lib/ee/gitlab/checks/push_rule_check_spec.rb | 8 ++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/ee/lib/ee/gitlab/checks/push_rule_check.rb b/ee/lib/ee/gitlab/checks/push_rule_check.rb index 0764577c30f6cb..3b5cc49d21bf37 100644 --- a/ee/lib/ee/gitlab/checks/push_rule_check.rb +++ b/ee/lib/ee/gitlab/checks/push_rule_check.rb @@ -40,6 +40,8 @@ def run_checks_in_parallel! PushRules::FileSizeCheck.new(change_access).validate! end end + + nil end def parallel_checks @@ -48,7 +50,6 @@ def parallel_checks yield @threads.each(&:join) - ensure @threads.each(&:exit) end diff --git a/ee/spec/lib/ee/gitlab/checks/push_rule_check_spec.rb b/ee/spec/lib/ee/gitlab/checks/push_rule_check_spec.rb index f527c25282a9e6..9476623a40a219 100644 --- a/ee/spec/lib/ee/gitlab/checks/push_rule_check_spec.rb +++ b/ee/spec/lib/ee/gitlab/checks/push_rule_check_spec.rb @@ -11,6 +11,14 @@ before do allow_any_instance_of(EE::Gitlab::Checks::PushRules::FileSizeCheck) .to receive(:validate!).and_return(nil) + allow_any_instance_of(EE::Gitlab::Checks::PushRules::TagCheck) + .to receive(:validate!).and_return(nil) + allow_any_instance_of(EE::Gitlab::Checks::PushRules::BranchCheck) + .to receive(:validate!).and_return(nil) + end + + it "returns nil on success" do + expect(subject.validate!).to be_nil end it "raises an error on failure" do -- GitLab From 9867b177c3d6e6191074bfd9b26c2b5299fc39dd Mon Sep 17 00:00:00 2001 From: Robert May Date: Thu, 22 Oct 2020 16:05:16 +0100 Subject: [PATCH 4/8] Add DB pool support and improvements --- ee/lib/ee/gitlab/checks/push_rule_check.rb | 64 +++++++++++++++------- 1 file changed, 43 insertions(+), 21 deletions(-) diff --git a/ee/lib/ee/gitlab/checks/push_rule_check.rb b/ee/lib/ee/gitlab/checks/push_rule_check.rb index 3b5cc49d21bf37..75a06d3ab6f21b 100644 --- a/ee/lib/ee/gitlab/checks/push_rule_check.rb +++ b/ee/lib/ee/gitlab/checks/push_rule_check.rb @@ -7,7 +7,7 @@ class PushRuleCheck < ::Gitlab::Checks::BaseChecker def validate! return unless push_rule - if ::Feature.enabled?(:parallel_push_checks, type: :ops) + if ::Feature.enabled?(:parallel_push_checks, project, type: :ops) run_checks_in_parallel! else run_checks_in_sequence! @@ -16,44 +16,64 @@ def validate! private - def run_checks_in_sequence! + # @return [Nil] returns nil unless an error is raised + # @raise [Gitlab::GitAccess::ForbiddenError] if check fails + def check_tag_or_branch! if tag_name PushRules::TagCheck.new(change_access).validate! else PushRules::BranchCheck.new(change_access).validate! end + end + # @return [Nil] returns nil unless an error is raised + # @raise [Gitlab::GitAccess::ForbiddenError] if check fails + def check_file_size! PushRules::FileSizeCheck.new(change_access).validate! end - def run_checks_in_parallel! - parallel_checks do - parallelize do - if tag_name - PushRules::TagCheck.new(change_access).validate! - else - PushRules::BranchCheck.new(change_access).validate! - end - end - - parallelize do - PushRules::FileSizeCheck.new(change_access).validate! - end - end - - nil + # Run the checks one after the other. + # + # @return [Nil] returns nil unless an error is raised + # @raise [Gitlab::GitAccess::ForbiddenError] if any check fails + def run_checks_in_sequence! + check_tag_or_branch! + check_file_size! end - def parallel_checks + # Run the checks in separate threads for performance benefits + # + # @return [Nil] returns nil unless an error is raised + # @raise [Gitlab::GitAccess::ForbiddenError] if any check fails + def run_checks_in_parallel! @threads = [] - yield + parallelize do + check_tag_or_branch! + end + + parallelize do + check_file_size! + end + # Block whilst waiting for threads, however if one errors + # it will exit early and raise the error immediately as + # we set `abort_on_exception` to true. @threads.each(&:join) + + nil ensure + # We want to make sure no threads are left dangling. + # Threads can exit early when an exception is raised + # and so we want to ensure any still running are exited + # as soon as possible. @threads.each(&:exit) end + # Runs a block inside a new thread. This thread will + # exit immediately upon an exception being raised. + # + # @raise [Gitlab::GitAccess::ForbiddenError] def parallelize @threads << Thread.new do Thread.current.tap do |t| @@ -61,7 +81,9 @@ def parallelize t.report_on_exception = false end - yield + ActiveRecord::Base.connection_pool.with_connection do + yield + end end end end -- GitLab From 3e722874a598622c3cf2fd088461791f10b06a5b Mon Sep 17 00:00:00 2001 From: Robert May Date: Thu, 22 Oct 2020 16:19:54 +0100 Subject: [PATCH 5/8] Slight spec tweak --- ee/spec/lib/ee/gitlab/checks/push_rule_check_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/spec/lib/ee/gitlab/checks/push_rule_check_spec.rb b/ee/spec/lib/ee/gitlab/checks/push_rule_check_spec.rb index 9476623a40a219..bb860b111a9e1d 100644 --- a/ee/spec/lib/ee/gitlab/checks/push_rule_check_spec.rb +++ b/ee/spec/lib/ee/gitlab/checks/push_rule_check_spec.rb @@ -22,7 +22,7 @@ end it "raises an error on failure" do - allow_any_instance_of(EE::Gitlab::Checks::PushRules::FileSizeCheck).to receive(:validate!).and_raise(Gitlab::GitAccess::ForbiddenError) + expect_any_instance_of(EE::Gitlab::Checks::PushRules::FileSizeCheck).to receive(:validate!).and_raise(Gitlab::GitAccess::ForbiddenError) expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError) end -- GitLab From 9f81c31d28880d32796cda8db77b1d79ecc72e14 Mon Sep 17 00:00:00 2001 From: Robert May Date: Thu, 22 Oct 2020 17:44:35 +0100 Subject: [PATCH 6/8] Relocate changelog --- {changelogs => ee/changelogs}/unreleased/parallel-push-check.yml | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename {changelogs => ee/changelogs}/unreleased/parallel-push-check.yml (100%) diff --git a/changelogs/unreleased/parallel-push-check.yml b/ee/changelogs/unreleased/parallel-push-check.yml similarity index 100% rename from changelogs/unreleased/parallel-push-check.yml rename to ee/changelogs/unreleased/parallel-push-check.yml -- GitLab From 7508795f447f1861b954907ebd4ef4bf200f7542 Mon Sep 17 00:00:00 2001 From: Robert May Date: Fri, 23 Oct 2020 10:37:43 +0100 Subject: [PATCH 7/8] Update database usage, name thread --- ee/lib/ee/gitlab/checks/push_rule_check.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/ee/lib/ee/gitlab/checks/push_rule_check.rb b/ee/lib/ee/gitlab/checks/push_rule_check.rb index 75a06d3ab6f21b..0759e23ce29a9c 100644 --- a/ee/lib/ee/gitlab/checks/push_rule_check.rb +++ b/ee/lib/ee/gitlab/checks/push_rule_check.rb @@ -77,13 +77,14 @@ def run_checks_in_parallel! def parallelize @threads << Thread.new do Thread.current.tap do |t| + t.name = "push_rule_check" t.abort_on_exception = true t.report_on_exception = false end - ActiveRecord::Base.connection_pool.with_connection do - yield - end + yield + ensure + ActiveRecord::Base.clear_active_connections! end end end -- GitLab From 382377b0512f12449c96d4e83b539affc22ba497 Mon Sep 17 00:00:00 2001 From: Robert May Date: Fri, 23 Oct 2020 11:37:52 +0100 Subject: [PATCH 8/8] Ignore faulty alignment cop --- ee/lib/ee/gitlab/checks/push_rule_check.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/lib/ee/gitlab/checks/push_rule_check.rb b/ee/lib/ee/gitlab/checks/push_rule_check.rb index 0759e23ce29a9c..441aeafa4a1012 100644 --- a/ee/lib/ee/gitlab/checks/push_rule_check.rb +++ b/ee/lib/ee/gitlab/checks/push_rule_check.rb @@ -83,7 +83,7 @@ def parallelize end yield - ensure + ensure # rubocop: disable Layout/RescueEnsureAlignment ActiveRecord::Base.clear_active_connections! end end -- GitLab