From a728acad8fdc60577f0fe4400b05e5eef93906a9 Mon Sep 17 00:00:00 2001
From: Alex Kalderimis <akalderimis@gitlab.com>
Date: Sun, 14 Aug 2022 23:44:55 +0000
Subject: [PATCH] Make the build hooks worker idempotent
Prior to this change, the payload was computed in the sidekiq worker
which may have different data due to replication lag.
This change computes the payload when we _enqueue_ the job, and skips
the use of the build-hooks worker, which is retained for backwards
compatibility, but is otherwise redundant.
Added: Adds execute_build_hooks_inline feature-flag
This adds a feature flag around the inline execution of
job hooks.
---
app/models/ci/build.rb | 16 +++++-
app/models/project.rb | 8 ++-
app/workers/build_hooks_worker.rb | 6 +-
app/workers/ci/build_finished_worker.rb | 3 +-
.../execute_build_hooks_inline.yml | 8 +++
.../ee/ci/build_finished_worker_spec.rb | 25 +++------
spec/models/ci/build_spec.rb | 56 ++++++++++++++++---
spec/models/project_spec.rb | 2 +-
spec/workers/build_hooks_worker_spec.rb | 4 +-
spec/workers/ci/build_finished_worker_spec.rb | 21 +++++--
10 files changed, 108 insertions(+), 41 deletions(-)
create mode 100644 config/feature_flags/development/execute_build_hooks_inline.yml
diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 2b72a4c4c89c4..87adedf936e05 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -194,7 +194,7 @@ def persisted_environment=(environment)
after_save :stick_build_if_status_changed
after_create unless: :importing? do |build|
- run_after_commit { BuildHooksWorker.perform_async(build) }
+ run_after_commit { build.feature_flagged_execute_hooks }
end
class << self
@@ -285,7 +285,7 @@ def clone_accessors
build.run_after_commit do
BuildQueueWorker.perform_async(id)
- BuildHooksWorker.perform_async(build)
+ build.feature_flagged_execute_hooks
end
end
@@ -313,7 +313,7 @@ def clone_accessors
build.run_after_commit do
build.ensure_persistent_ref
- BuildHooksWorker.perform_async(build)
+ build.feature_flagged_execute_hooks
end
end
@@ -780,10 +780,20 @@ def stuck?
pending? && !any_runners_online?
end
+ def feature_flagged_execute_hooks
+ if Feature.enabled?(:execute_build_hooks_inline, project)
+ execute_hooks
+ else
+ BuildHooksWorker.perform_async(self)
+ end
+ end
+
def execute_hooks
return unless project
return if user&.blocked?
+ ActiveRecord::Associations::Preloader.new.preload([self], { runner: :tags })
+
project.execute_hooks(build_data.dup, :job_hooks) if project.has_active_hooks?(:job_hooks)
project.execute_integrations(build_data.dup, :job_hooks) if project.has_active_integrations?(:job_hooks)
end
diff --git a/app/models/project.rb b/app/models/project.rb
index 28dd51fc67d61..b8a2fb13f9aa2 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1682,7 +1682,13 @@ def execute_integrations(data, hooks_scope = :push_hooks)
end
def has_active_hooks?(hooks_scope = :push_hooks)
- hooks.hooks_for(hooks_scope).any? || SystemHook.hooks_for(hooks_scope).any? || Gitlab::FileHook.any?
+ @has_active_hooks ||= {} # rubocop: disable Gitlab/PredicateMemoization
+
+ return @has_active_hooks[hooks_scope] if @has_active_hooks.key?(hooks_scope)
+
+ @has_active_hooks[hooks_scope] = hooks.hooks_for(hooks_scope).any? ||
+ SystemHook.hooks_for(hooks_scope).any? ||
+ Gitlab::FileHook.any?
end
def has_active_integrations?(hooks_scope = :push_hooks)
diff --git a/app/workers/build_hooks_worker.rb b/app/workers/build_hooks_worker.rb
index 5c08344bfe3ec..2c62aed72d6f3 100644
--- a/app/workers/build_hooks_worker.rb
+++ b/app/workers/build_hooks_worker.rb
@@ -13,9 +13,9 @@ class BuildHooksWorker # rubocop:disable Scalability/IdempotentWorker
# rubocop: disable CodeReuse/ActiveRecord
def perform(build_id)
- Ci::Build.includes({ runner: :tags })
- .find_by_id(build_id)
- .try(:execute_hooks)
+ build = Ci::Build.find_by_id(build_id)
+
+ build.execute_hooks if build
end
# rubocop: enable CodeReuse/ActiveRecord
diff --git a/app/workers/ci/build_finished_worker.rb b/app/workers/ci/build_finished_worker.rb
index 25c7637a79f14..36a50735fed2b 100644
--- a/app/workers/ci/build_finished_worker.rb
+++ b/app/workers/ci/build_finished_worker.rb
@@ -36,8 +36,7 @@ def process_build(build)
build.update_coverage
Ci::BuildReportResultService.new.execute(build)
- # We execute these async as these are independent operations.
- BuildHooksWorker.perform_async(build)
+ build.feature_flagged_execute_hooks
ChatNotificationWorker.perform_async(build.id) if build.pipeline.chat?
build.track_deployment_usage
build.track_verify_usage
diff --git a/config/feature_flags/development/execute_build_hooks_inline.yml b/config/feature_flags/development/execute_build_hooks_inline.yml
new file mode 100644
index 0000000000000..0389fca3bb1b5
--- /dev/null
+++ b/config/feature_flags/development/execute_build_hooks_inline.yml
@@ -0,0 +1,8 @@
+---
+name: execute_build_hooks_inline
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/93665
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/370387
+milestone: '15.3'
+type: development
+group: group::integrations
+default_enabled: false
diff --git a/ee/spec/workers/ee/ci/build_finished_worker_spec.rb b/ee/spec/workers/ee/ci/build_finished_worker_spec.rb
index dffe4f85ca86f..54f1a1afe2d90 100644
--- a/ee/spec/workers/ee/ci/build_finished_worker_spec.rb
+++ b/ee/spec/workers/ee/ci/build_finished_worker_spec.rb
@@ -4,7 +4,7 @@
RSpec.describe Ci::BuildFinishedWorker do
let_it_be(:ci_runner) { create(:ci_runner) }
- let_it_be(:build) { create(:ee_ci_build, :sast, :success, runner: ci_runner) }
+ let_it_be_with_reload(:build) { create(:ee_ci_build, :sast, :success, runner: ci_runner) }
let_it_be(:project) { build.project }
let_it_be(:namespace) { project.shared_runners_limit_namespace }
@@ -34,13 +34,10 @@ def project_stats
end
context 'when exception is raised in `super`' do
- before do
- allow(::BuildHooksWorker)
- .to receive(:perform_async)
- .and_raise(ArgumentError)
- end
-
it 'does not enqueue the worker in EE' do
+ allow(Ci::Build).to receive(:find_by_id).with(build.id).and_return(build)
+ allow(build).to receive(:execute_hooks).and_raise(ArgumentError)
+
expect { subject }.to raise_error(ArgumentError)
expect(::Security::TrackSecureScansWorker).not_to receive(:perform_async)
@@ -105,14 +102,6 @@ def project_stats
project.add_reporter(user)
end
- shared_examples 'schedules processing of requirement reports' do
- it do
- expect(RequirementsManagement::ProcessRequirementsReportsWorker).to receive(:perform_async)
-
- subject
- end
- end
-
shared_examples 'does not schedule processing of requirement reports' do
it do
expect(RequirementsManagement::ProcessRequirementsReportsWorker).not_to receive(:perform_async)
@@ -126,7 +115,11 @@ def project_stats
stub_licensed_features(requirements: true)
end
- it_behaves_like 'schedules processing of requirement reports'
+ it 'schedules processing of requirement reports' do
+ expect(RequirementsManagement::ProcessRequirementsReportsWorker).to receive(:perform_async)
+
+ subject
+ end
context 'when user has insufficient permissions to create test reports' do
before do
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 546cfd80fcf51..8b1d27687542d 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -4,6 +4,8 @@
RSpec.describe Ci::Build do
include Ci::TemplateHelpers
+ include AfterNextHelpers
+
let_it_be(:user) { create(:user) }
let_it_be(:group, reload: true) { create(:group) }
let_it_be(:project, reload: true) { create(:project, :repository, group: group) }
@@ -60,11 +62,36 @@
describe 'callbacks' do
context 'when running after_create callback' do
- it 'triggers asynchronous build hooks worker' do
- expect(BuildHooksWorker).to receive(:perform_async)
+ it 'executes hooks' do
+ expect_next(described_class).to receive(:execute_hooks)
create(:ci_build)
end
+
+ context 'when the execute_build_hooks_inline flag is disabled' do
+ before do
+ stub_feature_flags(execute_build_hooks_inline: false)
+ end
+
+ it 'uses the old job hooks worker' do
+ expect(::BuildHooksWorker).to receive(:perform_async).with(Ci::Build)
+
+ create(:ci_build)
+ end
+ end
+
+ context 'when the execute_build_hooks_inline flag is enabled for a project' do
+ before do
+ stub_feature_flags(execute_build_hooks_inline: project)
+ end
+
+ it 'executes hooks inline' do
+ expect(::BuildHooksWorker).not_to receive(:perform_async)
+ expect_next(described_class).to receive(:execute_hooks)
+
+ create(:ci_build, project: project)
+ end
+ end
end
end
@@ -1663,20 +1690,18 @@
end
it 'returns an expanded environment name with a list of variables' do
- expect(build).to receive(:simple_variables).once.and_call_original
-
is_expected.to eq('review/host')
end
context 'when build metadata has already persisted the expanded environment name' do
before do
- build.metadata.expanded_environment_name = 'review/host'
+ build.metadata.expanded_environment_name = 'review/foo'
end
it 'returns a persisted expanded environment name without a list of variables' do
expect(build).not_to receive(:simple_variables)
- is_expected.to eq('review/host')
+ is_expected.to eq('review/foo')
end
end
end
@@ -3872,8 +3897,20 @@
build.enqueue
end
- it 'queues BuildHooksWorker' do
- expect(BuildHooksWorker).to receive(:perform_async).with(build)
+ context 'when the execute_build_hooks_inline flag is disabled' do
+ before do
+ stub_feature_flags(execute_build_hooks_inline: false)
+ end
+
+ it 'queues BuildHooksWorker' do
+ expect(BuildHooksWorker).to receive(:perform_async).with(build)
+
+ build.enqueue
+ end
+ end
+
+ it 'executes hooks' do
+ expect(build).to receive(:execute_hooks)
build.enqueue
end
@@ -4624,6 +4661,7 @@ def run_job_without_exception
end
before do
+ allow(build).to receive(:execute_hooks)
stub_artifacts_object_storage
end
@@ -5564,7 +5602,7 @@ def run_job_without_exception
build.cancel_gracefully?
end
- let_it_be(:build) { create(:ci_build, pipeline: pipeline) }
+ let(:build) { create(:ci_build, pipeline: pipeline) }
it 'cannot cancel gracefully' do
expect(subject).to be false
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 2920278679ff0..0d11586c487ed 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -5889,7 +5889,7 @@ def has_external_wiki
end
describe '#has_active_hooks?' do
- let_it_be(:project) { create(:project) }
+ let_it_be_with_refind(:project) { create(:project) }
it { expect(project.has_active_hooks?).to be_falsey }
diff --git a/spec/workers/build_hooks_worker_spec.rb b/spec/workers/build_hooks_worker_spec.rb
index 426eb03638c22..80dc36d268faa 100644
--- a/spec/workers/build_hooks_worker_spec.rb
+++ b/spec/workers/build_hooks_worker_spec.rb
@@ -23,8 +23,8 @@
end
end
- describe '.perform_async' do
- it 'sends a message to the application logger, before performing', :sidekiq_inline do
+ describe '.perform_async', :sidekiq_inline do
+ it 'sends a message to the application logger, before performing' do
build = create(:ci_build)
expect(Gitlab::AppLogger).to receive(:info).with(
diff --git a/spec/workers/ci/build_finished_worker_spec.rb b/spec/workers/ci/build_finished_worker_spec.rb
index 201182636e7ee..5ddaabc3938e2 100644
--- a/spec/workers/ci/build_finished_worker_spec.rb
+++ b/spec/workers/ci/build_finished_worker_spec.rb
@@ -3,6 +3,8 @@
require 'spec_helper'
RSpec.describe Ci::BuildFinishedWorker do
+ include AfterNextHelpers
+
subject { described_class.new.perform(build.id) }
describe '#perform' do
@@ -16,17 +18,28 @@
it 'calculates coverage and calls hooks', :aggregate_failures do
expect(build).to receive(:update_coverage).ordered
- expect_next_instance_of(Ci::BuildReportResultService) do |build_report_result_service|
- expect(build_report_result_service).to receive(:execute).with(build)
- end
+ expect_next(Ci::BuildReportResultService).to receive(:execute).with(build)
- expect(BuildHooksWorker).to receive(:perform_async)
+ expect(build).to receive(:execute_hooks)
expect(ChatNotificationWorker).not_to receive(:perform_async)
expect(Ci::ArchiveTraceWorker).to receive(:perform_in)
subject
end
+ context 'when the execute_build_hooks_inline feature flag is disabled' do
+ before do
+ stub_feature_flags(execute_build_hooks_inline: false)
+ end
+
+ it 'uses the BuildHooksWorker' do
+ expect(build).not_to receive(:execute_hooks)
+ expect(BuildHooksWorker).to receive(:perform_async).with(build)
+
+ subject
+ end
+ end
+
context 'when build is failed' do
before do
build.update!(status: :failed)
--
GitLab