diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 24f3161bd35e8d9ce99a0cbbc6e6cd4c7ec47bc3..cdeccedc1471c50ad16baa2fdd47bd05546ab1f7 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -2,6 +2,7 @@
module Ci
class Build < Ci::Processable
+ prepend Ci::BulkInsertableTags
include Ci::Metadatable
include Ci::Contextable
include TokenAuthenticatable
@@ -434,10 +435,6 @@ def runnable?
true
end
- def save_tags
- super unless Thread.current['ci_bulk_insert_tags']
- end
-
def archived?
return true if degenerated?
diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb
index 61194c9b7d11c44fe2cf240b06a0c9179747e32d..e0180880760a4f7208b14d4ed12e2aa0c6797c8a 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -2,6 +2,7 @@
module Ci
class Runner < Ci::ApplicationRecord
+ prepend Ci::BulkInsertableTags
include Gitlab::SQL::Pattern
include RedisCacheable
include ChronicDurationAttribute
diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb
index ac9d8c39bd2df8f5dd3957f5a811da681b6396ea..d08d303912d3d79ada646f3a10381075a767df35 100644
--- a/app/models/commit_status.rb
+++ b/app/models/commit_status.rb
@@ -220,10 +220,6 @@ def self.locking_enabled?
false
end
- def self.bulk_insert_tags!(statuses)
- Gitlab::Ci::Tags::BulkInsert.new(statuses).insert!
- end
-
def locking_enabled?
will_save_change_to_status?
end
diff --git a/app/models/concerns/ci/bulk_insertable_tags.rb b/app/models/concerns/ci/bulk_insertable_tags.rb
new file mode 100644
index 0000000000000000000000000000000000000000..453b3b3fbc9b69beab7a5744812ab01224d65d39
--- /dev/null
+++ b/app/models/concerns/ci/bulk_insertable_tags.rb
@@ -0,0 +1,24 @@
+# frozen_string_literal: true
+
+module Ci
+ module BulkInsertableTags
+ extend ActiveSupport::Concern
+
+ BULK_INSERT_TAG_THREAD_KEY = 'ci_bulk_insert_tags'
+
+ class << self
+ def with_bulk_insert_tags
+ previous = Thread.current[BULK_INSERT_TAG_THREAD_KEY]
+ Thread.current[BULK_INSERT_TAG_THREAD_KEY] = true
+ yield
+ ensure
+ Thread.current[BULK_INSERT_TAG_THREAD_KEY] = previous
+ end
+ end
+
+ # overrides save_tags from acts-as-taggable
+ def save_tags
+ super unless Thread.current[BULK_INSERT_TAG_THREAD_KEY]
+ end
+ end
+end
diff --git a/app/services/ci/runners/register_runner_service.rb b/app/services/ci/runners/register_runner_service.rb
index 196d2de1a65df9abc7ccb79c39787085ca64be11..6588cd7e24810262e01c1f8baf0198753c640933 100644
--- a/app/services/ci/runners/register_runner_service.rb
+++ b/app/services/ci/runners/register_runner_service.rb
@@ -8,7 +8,19 @@ def execute(registration_token, attributes)
return unless runner_type_attrs
- ::Ci::Runner.create(attributes.merge(runner_type_attrs))
+ runner = ::Ci::Runner.new(attributes.merge(runner_type_attrs))
+
+ Ci::BulkInsertableTags.with_bulk_insert_tags do
+ Ci::Runner.transaction do
+ if runner.save
+ Gitlab::Ci::Tags::BulkInsert.bulk_insert_tags!([runner])
+ else
+ raise ActiveRecord::Rollback
+ end
+ end
+ end
+
+ runner
end
private
diff --git a/lib/gitlab/ci/pipeline/chain/create.rb b/lib/gitlab/ci/pipeline/chain/create.rb
index 71dfc1a676cac623be70f57e075de3f9c52090b8..207b4b5ff8b0a1fddd8c059ab32e9d67d4534921 100644
--- a/lib/gitlab/ci/pipeline/chain/create.rb
+++ b/lib/gitlab/ci/pipeline/chain/create.rb
@@ -11,10 +11,10 @@ class Create < Chain::Base
def perform!
logger.instrument_with_sql(:pipeline_save) do
BulkInsertableAssociations.with_bulk_insert do
- with_bulk_insert_tags do
+ ::Ci::BulkInsertableTags.with_bulk_insert_tags do
pipeline.transaction do
pipeline.save!
- CommitStatus.bulk_insert_tags!(statuses)
+ Gitlab::Ci::Tags::BulkInsert.bulk_insert_tags!(statuses)
end
end
end
@@ -29,14 +29,6 @@ def break?
private
- def with_bulk_insert_tags
- previous = Thread.current['ci_bulk_insert_tags']
- Thread.current['ci_bulk_insert_tags'] = true
- yield
- ensure
- Thread.current['ci_bulk_insert_tags'] = previous
- end
-
def statuses
strong_memoize(:statuses) do
pipeline
diff --git a/lib/gitlab/ci/tags/bulk_insert.rb b/lib/gitlab/ci/tags/bulk_insert.rb
index 870bd0fc0a2aba24181dddb08ceecc0f745a77ec..2e56e47f5b8da3745fc8ed1c26c41bacf8e88352 100644
--- a/lib/gitlab/ci/tags/bulk_insert.rb
+++ b/lib/gitlab/ci/tags/bulk_insert.rb
@@ -9,33 +9,37 @@ class BulkInsert
TAGGINGS_BATCH_SIZE = 1000
TAGS_BATCH_SIZE = 500
- def initialize(statuses)
- @statuses = statuses
+ def self.bulk_insert_tags!(taggables)
+ Gitlab::Ci::Tags::BulkInsert.new(taggables).insert!
+ end
+
+ def initialize(taggables)
+ @taggables = taggables
end
def insert!
- return false if tag_list_by_status.empty?
+ return false if tag_list_by_taggable.empty?
persist_build_tags!
end
private
- attr_reader :statuses
+ attr_reader :taggables
- def tag_list_by_status
- strong_memoize(:tag_list_by_status) do
- statuses.each.with_object({}) do |status, acc|
- tag_list = status.tag_list
+ def tag_list_by_taggable
+ strong_memoize(:tag_list_by_taggable) do
+ taggables.each.with_object({}) do |taggable, acc|
+ tag_list = taggable.tag_list
next unless tag_list
- acc[status] = tag_list
+ acc[taggable] = tag_list
end
end
end
def persist_build_tags!
- all_tags = tag_list_by_status.values.flatten.uniq.reject(&:blank?)
+ all_tags = tag_list_by_taggable.values.flatten.uniq.reject(&:blank?)
tag_records_by_name = create_tags(all_tags).index_by(&:name)
taggings = build_taggings_attributes(tag_records_by_name)
@@ -65,24 +69,24 @@ def create_tags(tags)
# rubocop: enable CodeReuse/ActiveRecord
def build_taggings_attributes(tag_records_by_name)
- taggings = statuses.flat_map do |status|
- tag_list = tag_list_by_status[status]
+ taggings = taggables.flat_map do |taggable|
+ tag_list = tag_list_by_taggable[taggable]
next unless tag_list
tags = tag_records_by_name.values_at(*tag_list)
- taggings_for(tags, status)
+ taggings_for(tags, taggable)
end
taggings.compact!
taggings
end
- def taggings_for(tags, status)
+ def taggings_for(tags, taggable)
tags.map do |tag|
{
tag_id: tag.id,
- taggable_type: CommitStatus.name,
- taggable_id: status.id,
+ taggable_type: taggable.class.base_class.name,
+ taggable_id: taggable.id,
created_at: Time.current,
context: 'tags'
}
diff --git a/spec/lib/gitlab/ci/pipeline/chain/create_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/create_spec.rb
index 9057c4e99df2f948097df315d7abe2f64b762048..7b97dde3808a30732d25dfafed2c6686b1f4c32c 100644
--- a/spec/lib/gitlab/ci/pipeline/chain/create_spec.rb
+++ b/spec/lib/gitlab/ci/pipeline/chain/create_spec.rb
@@ -77,7 +77,7 @@
context 'without tags' do
it 'extracts an empty tag list' do
- expect(CommitStatus)
+ expect(Gitlab::Ci::Tags::BulkInsert)
.to receive(:bulk_insert_tags!)
.with([job])
.and_call_original
@@ -95,7 +95,7 @@
end
it 'bulk inserts tags' do
- expect(CommitStatus)
+ expect(Gitlab::Ci::Tags::BulkInsert)
.to receive(:bulk_insert_tags!)
.with([job])
.and_call_original
diff --git a/spec/lib/gitlab/ci/tags/bulk_insert_spec.rb b/spec/lib/gitlab/ci/tags/bulk_insert_spec.rb
index 063376499e23d8f8fa1c027d2f7acb9533c08eef..72574d501763d2754c2f86695b56b639b11fa953 100644
--- a/spec/lib/gitlab/ci/tags/bulk_insert_spec.rb
+++ b/spec/lib/gitlab/ci/tags/bulk_insert_spec.rb
@@ -18,7 +18,7 @@
let(:error_message) do
<<~MESSAGE
A mechanism depending on internals of 'act-as-taggable-on` has been designed
- to bulk insert tags for Ci::Build records.
+ to bulk insert tags for Ci::Build/Ci::Runner records.
Please review the code carefully before updating the gem version
https://gitlab.com/gitlab-org/gitlab/-/issues/350053
MESSAGE
@@ -27,6 +27,21 @@
it { expect(ActsAsTaggableOn::VERSION).to eq(acceptable_version), error_message }
end
+ describe '.bulk_insert_tags!' do
+ let(:inserter) { instance_double(described_class) }
+
+ it 'delegates to bulk insert class' do
+ expect(Gitlab::Ci::Tags::BulkInsert)
+ .to receive(:new)
+ .with(statuses)
+ .and_return(inserter)
+
+ expect(inserter).to receive(:insert!)
+
+ described_class.bulk_insert_tags!(statuses)
+ end
+ end
+
describe '#insert!' do
context 'without tags' do
it { expect(service.insert!).to be_falsey }
@@ -45,6 +60,17 @@
expect(other_job.reload.tag_list).to match_array(%w[tag2 tag3 tag4])
end
+ it 'persists taggings' do
+ service.insert!
+
+ expect(job.taggings.size).to eq(2)
+ expect(other_job.taggings.size).to eq(3)
+
+ expect(Ci::Build.tagged_with('tag1')).to include(job)
+ expect(Ci::Build.tagged_with('tag2')).to include(job, other_job)
+ expect(Ci::Build.tagged_with('tag3')).to include(other_job)
+ end
+
context 'when batching inserts for tags' do
before do
stub_const("#{described_class}::TAGS_BATCH_SIZE", 2)
@@ -83,6 +109,15 @@
expect(job.reload.tag_list).to match_array(%w[tag1 tag2])
expect(other_job.reload.tag_list).to be_empty
end
+
+ it 'persists taggings' do
+ service.insert!
+
+ expect(job.taggings.size).to eq(2)
+
+ expect(Ci::Build.tagged_with('tag1')).to include(job)
+ expect(Ci::Build.tagged_with('tag2')).to include(job)
+ end
end
end
end
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 6456ad3c1ece0ab91c6303693619d047243bca07..1ecf7e8b21646e23867bbe42acd6693a086484d1 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -2083,6 +2083,27 @@
end
end
+ describe '#save_tags' do
+ let(:build) { create(:ci_build, tag_list: ['tag']) }
+
+ it 'saves tags' do
+ build.save!
+
+ expect(build.tags.count).to eq(1)
+ expect(build.tags.first.name).to eq('tag')
+ end
+
+ context 'with BulkInsertableTags.with_bulk_insert_tags' do
+ it 'does not save_tags' do
+ Ci::BulkInsertableTags.with_bulk_insert_tags do
+ build.save!
+ end
+
+ expect(build.tags).to be_empty
+ end
+ end
+ end
+
describe '#has_tags?' do
context 'when build has tags' do
subject { create(:ci_build, tag_list: ['tag']) }
diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb
index 74d8b012b29a89e6909edafaf79477bcbcf31d5c..c5cb67929e24ce6416ec8b24ad1a18bf911acc02 100644
--- a/spec/models/ci/runner_spec.rb
+++ b/spec/models/ci/runner_spec.rb
@@ -1193,6 +1193,40 @@ def does_db_update
end
end
+ describe '#save_tags' do
+ let(:runner) { build(:ci_runner, tag_list: ['tag']) }
+
+ it 'saves tags' do
+ runner.save!
+
+ expect(runner.tags.count).to eq(1)
+ expect(runner.tags.first.name).to eq('tag')
+ end
+
+ context 'with BulkInsertableTags.with_bulk_insert_tags' do
+ it 'does not save_tags' do
+ Ci::BulkInsertableTags.with_bulk_insert_tags do
+ runner.save!
+ end
+
+ expect(runner.tags).to be_empty
+ end
+
+ context 'over TAG_LIST_MAX_LENGTH' do
+ let(:tag_list) { (1..described_class::TAG_LIST_MAX_LENGTH + 1).map { |i| "tag#{i}" } }
+ let(:runner) { build(:ci_runner, tag_list: tag_list) }
+
+ it 'fails validation if over tag limit' do
+ Ci::BulkInsertableTags.with_bulk_insert_tags do
+ expect { runner.save! }.to raise_error(ActiveRecord::RecordInvalid)
+ end
+
+ expect(runner.tags).to be_empty
+ end
+ end
+ end
+ end
+
describe '#has_tags?' do
context 'when runner has tags' do
subject { create(:ci_runner, tag_list: ['tag']) }
diff --git a/spec/models/commit_status_spec.rb b/spec/models/commit_status_spec.rb
index dbb15fad2469ad37790f327d5bf134fc42f1c626..9f58d3c1a80e9010dbd727d651c1e16655c74f52 100644
--- a/spec/models/commit_status_spec.rb
+++ b/spec/models/commit_status_spec.rb
@@ -984,22 +984,6 @@ def create_status(**opts)
end
end
- describe '.bulk_insert_tags!' do
- let(:statuses) { double('statuses') }
- let(:inserter) { double('inserter') }
-
- it 'delegates to bulk insert class' do
- expect(Gitlab::Ci::Tags::BulkInsert)
- .to receive(:new)
- .with(statuses)
- .and_return(inserter)
-
- expect(inserter).to receive(:insert!)
-
- described_class.bulk_insert_tags!(statuses)
- end
- end
-
describe '#expire_etag_cache!' do
it 'expires the etag cache' do
expect_next_instance_of(Gitlab::EtagCaching::Store) do |etag_store|
diff --git a/spec/models/concerns/ci/bulk_insertable_tags_spec.rb b/spec/models/concerns/ci/bulk_insertable_tags_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..23f0831403df81620f1296f4646c28c3808d8854
--- /dev/null
+++ b/spec/models/concerns/ci/bulk_insertable_tags_spec.rb
@@ -0,0 +1,66 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Ci::BulkInsertableTags do
+ let(:taggable_class) do
+ Class.new do
+ prepend Ci::BulkInsertableTags
+
+ attr_reader :tags_saved
+
+ def save_tags
+ @tags_saved = true
+ end
+ end
+ end
+
+ let(:record) { taggable_class.new }
+
+ describe '.with_bulk_insert_tags' do
+ it 'changes the thread key to true' do
+ expect(Thread.current['ci_bulk_insert_tags']).to be_nil
+
+ described_class.with_bulk_insert_tags do
+ expect(Thread.current['ci_bulk_insert_tags']).to eq(true)
+ end
+
+ expect(Thread.current['ci_bulk_insert_tags']).to be_nil
+ end
+ end
+
+ describe '#save_tags' do
+ it 'calls super' do
+ record.save_tags
+
+ expect(record.tags_saved).to eq(true)
+ end
+
+ it 'does not call super with BulkInsertableTags.with_bulk_insert_tags' do
+ described_class.with_bulk_insert_tags do
+ record.save_tags
+ end
+
+ expect(record.tags_saved).to be_nil
+ end
+
+ it 'isolates bulk insert behavior between threads' do
+ record2 = taggable_class.new
+
+ t1 = Thread.new do
+ described_class.with_bulk_insert_tags do
+ record.save_tags
+ end
+ end
+
+ t2 = Thread.new do
+ record2.save_tags
+ end
+
+ [t1, t2].each(&:join)
+
+ expect(record.tags_saved).to be_nil
+ expect(record2.tags_saved).to eq(true)
+ end
+ end
+end
diff --git a/spec/services/ci/runners/register_runner_service_spec.rb b/spec/services/ci/runners/register_runner_service_spec.rb
index f43fd8230788d95803fc7aea8d7db963e8ddb038..03dcf851e5316d3c91eaba5f88f6e15e5828a55d 100644
--- a/spec/services/ci/runners/register_runner_service_spec.rb
+++ b/spec/services/ci/runners/register_runner_service_spec.rb
@@ -13,7 +13,7 @@
stub_application_setting(valid_runner_registrars: ApplicationSetting::VALID_RUNNER_REGISTRAR_TYPES)
end
- subject { described_class.new.execute(token, args) }
+ subject(:runner) { described_class.new.execute(token, args) }
context 'when no token is provided' do
let(:token) { '' }
@@ -83,6 +83,9 @@
expect(subject.platform).to eq args[:platform]
expect(subject.architecture).to eq args[:architecture]
expect(subject.ip_address).to eq args[:ip_address]
+
+ expect(Ci::Runner.tagged_with('tag1')).to include(subject)
+ expect(Ci::Runner.tagged_with('tag2')).to include(subject)
end
end
@@ -230,5 +233,41 @@
end
end
end
+
+ context 'when tags are provided' do
+ let(:token) { registration_token }
+
+ let(:args) do
+ { tag_list: %w(tag1 tag2) }
+ end
+
+ it 'creates runner with tags' do
+ expect(runner).to be_persisted
+
+ expect(runner.tags).to contain_exactly(
+ an_object_having_attributes(name: 'tag1'),
+ an_object_having_attributes(name: 'tag2')
+ )
+ end
+
+ it 'creates tags in bulk' do
+ expect(Gitlab::Ci::Tags::BulkInsert).to receive(:bulk_insert_tags!).and_call_original
+
+ expect(runner).to be_persisted
+ end
+
+ context 'and tag list exceeds limit' do
+ let(:args) do
+ { tag_list: (1..Ci::Runner::TAG_LIST_MAX_LENGTH + 1).map { |i| "tag#{i}" } }
+ end
+
+ it 'does not create any tags' do
+ expect(Gitlab::Ci::Tags::BulkInsert).not_to receive(:bulk_insert_tags!)
+
+ expect(runner).not_to be_persisted
+ expect(runner.tags).to be_empty
+ end
+ end
+ end
end
end