diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 8738e094510..d2402b55184 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -47,6 +47,25 @@ module Ci scope :manual_actions, ->() { where(when: :manual, status: COMPLETED_STATUSES + [:manual]) } scope :ref_protected, -> { where(protected: true) } + scope :matches_tag_ids, -> (tag_ids) do + matcher = ::ActsAsTaggableOn::Tagging + .where(taggable_type: CommitStatus) + .where(context: 'tags') + .where('taggable_id = ci_builds.id') + .where.not(tag_id: tag_ids).select('1') + + where("NOT EXISTS (?)", matcher) + end + + scope :with_any_tags, -> do + matcher = ::ActsAsTaggableOn::Tagging + .where(taggable_type: CommitStatus) + .where(context: 'tags') + .where('taggable_id = ci_builds.id').select('1') + + where("EXISTS (?)", matcher) + end + mount_uploader :legacy_artifacts_file, LegacyArtifactUploader, mount_on: :artifacts_file mount_uploader :legacy_artifacts_metadata, LegacyArtifactUploader, mount_on: :artifacts_metadata diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index d39610a8995..dcbb397fb78 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -112,7 +112,7 @@ module Ci def can_pick?(build) return false if self.ref_protected? && !build.protected? - assignable_for?(build.project) && accepting_tags?(build) + assignable_for?(build.project_id) && accepting_tags?(build) end def only_for?(project) @@ -171,8 +171,8 @@ module Ci end end - def assignable_for?(project) - is_shared? || projects.exists?(id: project.id) + def assignable_for?(project_id) + is_shared? || projects.exists?(id: project_id) end def accepting_tags?(build) diff --git a/app/services/ci/register_job_service.rb b/app/services/ci/register_job_service.rb index b8db709211a..2ef76e03031 100644 --- a/app/services/ci/register_job_service.rb +++ b/app/services/ci/register_job_service.rb @@ -22,6 +22,16 @@ module Ci valid = true + if Feature.enabled?('ci_job_request_with_tags_matcher') + # pick builds that does not have other tags than runner's one + builds = builds.matches_tag_ids(runner.tags.ids) + + # pick builds that have at least one tag + unless runner.run_untagged? + builds = builds.with_any_tags + end + end + builds.find do |build| next unless runner.can_pick?(build) diff --git a/changelogs/unreleased/perform-sql-matching-of-tags.yml b/changelogs/unreleased/perform-sql-matching-of-tags.yml new file mode 100644 index 00000000000..39f8a867a4d --- /dev/null +++ b/changelogs/unreleased/perform-sql-matching-of-tags.yml @@ -0,0 +1,5 @@ +--- +title: Perform SQL matching of Build&Runner tags to greatly speed-up job picking +merge_request: +author: +type: performance diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 9070692abfe..26d33663dad 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -1921,4 +1921,77 @@ describe Ci::Build do end end end + + describe '.matches_tag_ids' do + set(:build) { create(:ci_build, project: project, user: user) } + let(:tag_ids) { ::ActsAsTaggableOn::Tag.named_any(tag_list).ids } + + subject { described_class.where(id: build).matches_tag_ids(tag_ids) } + + before do + build.update(tag_list: build_tag_list) + end + + context 'when have different tags' do + let(:build_tag_list) { %w(A B) } + let(:tag_list) { %w(C D) } + + it "does not match a build" do + is_expected.not_to contain_exactly(build) + end + end + + context 'when have a subset of tags' do + let(:build_tag_list) { %w(A B) } + let(:tag_list) { %w(A B C D) } + + it "does match a build" do + is_expected.to contain_exactly(build) + end + end + + context 'when build does not have tags' do + let(:build_tag_list) { [] } + let(:tag_list) { %w(C D) } + + it "does match a build" do + is_expected.to contain_exactly(build) + end + end + + context 'when does not have a subset of tags' do + let(:build_tag_list) { %w(A B C) } + let(:tag_list) { %w(C D) } + + it "does not match a build" do + is_expected.not_to contain_exactly(build) + end + end + end + + describe '.matches_tags' do + set(:build) { create(:ci_build, project: project, user: user) } + + subject { described_class.where(id: build).with_any_tags } + + before do + build.update(tag_list: tag_list) + end + + context 'when does have tags' do + let(:tag_list) { %w(A B) } + + it "does match a build" do + is_expected.to contain_exactly(build) + end + end + + context 'when does not have tags' do + let(:tag_list) { [] } + + it "does not match a build" do + is_expected.not_to contain_exactly(build) + end + end + end end diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb index 5ac30111ec9..decdd577226 100644 --- a/spec/services/ci/register_job_service_spec.rb +++ b/spec/services/ci/register_job_service_spec.rb @@ -15,16 +15,14 @@ module Ci describe '#execute' do context 'runner follow tag list' do it "picks build with the same tag" do - pending_job.tag_list = ["linux"] - pending_job.save - specific_runner.tag_list = ["linux"] + pending_job.update(tag_list: ["linux"]) + specific_runner.update(tag_list: ["linux"]) expect(execute(specific_runner)).to eq(pending_job) end it "does not pick build with different tag" do - pending_job.tag_list = ["linux"] - pending_job.save - specific_runner.tag_list = ["win32"] + pending_job.update(tag_list: ["linux"]) + specific_runner.update(tag_list: ["win32"]) expect(execute(specific_runner)).to be_falsey end @@ -33,13 +31,12 @@ module Ci end it "does not pick build with tag" do - pending_job.tag_list = ["linux"] - pending_job.save + pending_job.update(tag_list: ["linux"]) expect(execute(specific_runner)).to be_falsey end it "pick build without tag" do - specific_runner.tag_list = ["win32"] + specific_runner.update(tag_list: ["win32"]) expect(execute(specific_runner)).to eq(pending_job) end end @@ -172,7 +169,7 @@ module Ci context 'when first build is stalled' do before do - pending_job.lock_version = 10 + pending_job.update(lock_version: 0) end subject { described_class.new(specific_runner).execute } @@ -182,7 +179,7 @@ module Ci before do allow_any_instance_of(Ci::RegisterJobService).to receive(:builds_for_specific_runner) - .and_return([pending_job, other_build]) + .and_return(Ci::Build.where(id: [pending_job, other_build])) end it "receives second build from the queue" do @@ -194,7 +191,7 @@ module Ci context 'when single build is in queue' do before do allow_any_instance_of(Ci::RegisterJobService).to receive(:builds_for_specific_runner) - .and_return([pending_job]) + .and_return(Ci::Build.where(id: pending_job)) end it "does not receive any valid result" do @@ -205,7 +202,7 @@ module Ci context 'when there is no build in queue' do before do allow_any_instance_of(Ci::RegisterJobService).to receive(:builds_for_specific_runner) - .and_return([]) + .and_return(Ci::Build.none) end it "does not receive builds but result is valid" do