Merge branch '336432-eliminate-cross-db-query-for-runners-by-group' into 'master'

Eliminate cross-db-join in RunnersFinder

See merge request gitlab-org/gitlab!74900

app/finders/ci/runners_finder.rb

@@ -53,9 +53,13 @@ module Ci
                  when :direct
                    Ci::Runner.belonging_to_group(@group.id)
                  when :descendants, nil
-                   # Getting all runners from the group itself and all its descendant groups/projects
-                   descendant_projects = Project.for_group_and_its_subgroups(@group)
-                   Ci::Runner.belonging_to_group_or_project(@group.self_and_descendants, descendant_projects)
+                   if ::Feature.enabled?(:ci_find_runners_by_ci_mirrors, @group, default_enabled: :yaml)
+                     Ci::Runner.belonging_to_group_or_project_descendants(@group.id)
+                   else
+                     # Getting all runners from the group itself and all its descendant groups/projects
+                     descendant_projects = Project.for_group_and_its_subgroups(@group)
+                     Ci::Runner.legacy_belonging_to_group_or_project(@group.self_and_descendants, descendant_projects)
+                   end
                  else
                    raise ArgumentError, 'Invalid membership filter'
                  end

app/models/ci/namespace_mirror.rb

@@ -10,6 +10,8 @@ module Ci
       where('traversal_ids @> ARRAY[?]::int[]', id)
     end
 
+    scope :by_namespace_id, -> (namespace_id) { where(namespace_id: namespace_id) }
+
     class << self
       def sync!(event)
         namespace = event.namespace

app/models/ci/project_mirror.rb

@@ -6,6 +6,9 @@ module Ci
   class ProjectMirror < ApplicationRecord
     belongs_to :project
 
+    scope :by_namespace_id, -> (namespace_id) { where(namespace_id: namespace_id) }
+    scope :by_project_id, -> (project_id) { where(project_id: project_id) }
+
     class << self
       def sync!(event)
         upsert({ project_id: event.project_id, namespace_id: event.project.namespace_id },

app/models/ci/runner.rb

@@ -95,7 +95,26 @@ module Ci
       joins(:runner_projects).where(ci_runner_projects: { project_id: project_id })
     }
 
-    scope :belonging_to_group, -> (group_id, include_ancestors: false) {
+    scope :belonging_to_group, -> (group_id) {
+      joins(:runner_namespaces)
+        .where(ci_runner_namespaces: { namespace_id: group_id })
+    }
+
+    scope :belonging_to_group_or_project_descendants, -> (group_id) {
+      group_ids = Ci::NamespaceMirror.contains_namespace(group_id).select(:namespace_id)
+      project_ids = Ci::ProjectMirror.by_namespace_id(group_ids).select(:project_id)
+
+      group_runners = joins(:runner_namespaces).where(ci_runner_namespaces: { namespace_id: group_ids })
+      project_runners = joins(:runner_projects).where(ci_runner_projects: { project_id: project_ids })
+
+      union_sql = ::Gitlab::SQL::Union.new([group_runners, project_runners]).to_sql
+
+      from("(#{union_sql}) #{table_name}")
+    }
+
+    # deprecated
+    # split this into: belonging_to_group & belonging_to_group_and_ancestors
+    scope :legacy_belonging_to_group, -> (group_id, include_ancestors: false) {
       groups = ::Group.where(id: group_id)
       groups = groups.self_and_ancestors if include_ancestors
 
@@ -104,7 +123,8 @@ module Ci
         .allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/336433')
     }
 
-    scope :belonging_to_group_or_project, -> (group_id, project_id) {
+    # deprecated
+    scope :legacy_belonging_to_group_or_project, -> (group_id, project_id) {
       groups = ::Group.where(id: group_id)
 
       group_runners = joins(:runner_namespaces).where(ci_runner_namespaces: { namespace_id: groups })
@@ -116,6 +136,7 @@ module Ci
         .allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/336433')
     }
 
+    # deprecated
     scope :belonging_to_parent_group_of_project, -> (project_id) {
       project_groups = ::Group.joins(:projects).where(projects: { id: project_id })
 
@@ -124,6 +145,7 @@ module Ci
         .allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/336433')
     }
 
+    # deprecated
     scope :owned_or_instance_wide, -> (project_id) do
       from_union(
         [

config/feature_flags/development/ci_find_runners_by_ci_mirrors.yml

+---
+name: ci_find_runners_by_ci_mirrors
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/74900
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/347226
+milestone: '14.7'
+type: development
+group: group::runner
+default_enabled: false

ee/lib/analytics/devops_adoption/snapshot_calculator.rb

@@ -62,7 +62,7 @@ module Analytics
 
       def runner_configured
         ::Gitlab::Database.allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/337541') do
-          Ci::Runner.active.belonging_to_group_or_project(snapshot_groups, snapshot_project_ids).exists?
+          Ci::Runner.active.legacy_belonging_to_group_or_project(snapshot_groups, snapshot_project_ids).exists?
         end
       end
 

lib/api/ci/runners.rb

@@ -229,7 +229,7 @@ module API
           use :pagination
         end
         get ':id/runners' do
-          runners = ::Ci::Runner.belonging_to_group(user_group.id, include_ancestors: true)
+          runners = ::Ci::Runner.legacy_belonging_to_group(user_group.id, include_ancestors: true)
           runners = apply_filter(runners, params)
 
           present paginate(runners), with: Entities::Ci::Runner

spec/factories/namespaces.rb

@@ -11,6 +11,14 @@ FactoryBot.define do
 
     owner { association(:user, strategy: :build, namespace: instance, username: path) }
 
+    after(:create) do |namespace, evaluator|
+      # simulating ::Namespaces::ProcessSyncEventsWorker because most tests don't run Sidekiq inline
+      # Note: we need to get refreshed `traversal_ids` it is updated via SQL query
+      #       in `Namespaces::Traversal::Linear#sync_traversal_ids` (see the NOTE in that method).
+      #       We cannot use `.reload` because it cleans other on-the-fly attributes.
+      namespace.create_ci_namespace_mirror!(traversal_ids: Namespace.find(namespace.id).traversal_ids) unless namespace.ci_namespace_mirror
+    end
+
     trait :with_aggregation_schedule do
       after(:create) do |namespace|
         create(:namespace_aggregation_schedules, namespace: namespace)

spec/factories/projects.rb

@@ -101,6 +101,9 @@ FactoryBot.define do
         import_state.last_error = evaluator.import_last_error
         import_state.save!
       end
+
+      # simulating ::Projects::ProcessSyncEventsWorker because most tests don't run Sidekiq inline
+      project.create_ci_project_mirror!(namespace_id: project.namespace_id) unless project.ci_project_mirror
     end
 
     trait :public do

spec/finders/ci/runners_finder_spec.rb

@@ -206,7 +206,7 @@ RSpec.describe Ci::RunnersFinder do
       sub_group_4.runners << runner_sub_group_4
     end
 
-    describe '#execute' do
+    shared_examples '#execute' do
       subject { described_class.new(current_user: user, params: params).execute }
 
       shared_examples 'membership equal to :descendants' do
@@ -349,6 +349,16 @@ RSpec.describe Ci::RunnersFinder do
       end
     end
 
+    it_behaves_like '#execute'
+
+    context 'when the FF ci_find_runners_by_ci_mirrors is disabled' do
+      before do
+        stub_feature_flags(ci_find_runners_by_ci_mirrors: false)
+      end
+
+      it_behaves_like '#execute'
+    end
+
     describe '#sort_key' do
       subject { described_class.new(current_user: user, params: params.merge(group: group)).sort_key }
 

spec/models/ci/namespace_mirror_spec.rb

@@ -8,50 +8,77 @@ RSpec.describe Ci::NamespaceMirror do
   let!(:group3) { create(:group, parent: group2) }
   let!(:group4) { create(:group, parent: group3) }
 
-  describe '.sync!' do
-    let!(:event) { namespace.sync_events.create! }
+  before do
+    # refreshing ci mirrors according to the parent tree above
+    Namespaces::SyncEvent.find_each { |event| Ci::NamespaceMirror.sync!(event) }
+
+    # checking initial situation. we need to reload to reflect the changes of event sync
+    expect(group1.reload.ci_namespace_mirror).to have_attributes(traversal_ids: [group1.id])
+    expect(group2.reload.ci_namespace_mirror).to have_attributes(traversal_ids: [group1.id, group2.id])
+    expect(group3.reload.ci_namespace_mirror).to have_attributes(traversal_ids: [group1.id, group2.id, group3.id])
+    expect(group4.reload.ci_namespace_mirror).to have_attributes(traversal_ids: [group1.id, group2.id, group3.id, group4.id])
+  end
+
+  context 'scopes' do
+    describe '.contains_namespace' do
+      let_it_be(:another_group) { create(:group) }
+
+      subject(:result) { described_class.contains_namespace(group2.id) }
+
+      it 'returns groups having group2.id in traversal_ids' do
+        expect(result.pluck(:namespace_id)).to contain_exactly(group2.id, group3.id, group4.id)
+      end
+    end
+
+    describe '.by_namespace_id' do
+      subject(:result) { described_class.by_namespace_id(group2.id) }
 
-    subject(:sync) { described_class.sync!(event.reload) }
+      it 'returns namesapce mirrors of namespace id' do
+        expect(result).to contain_exactly(group2.ci_namespace_mirror)
+      end
+    end
+  end
 
-    context 'when namespace hierarchy does not exist in the first place' do
+  describe '.sync!' do
+    subject(:sync) { described_class.sync!(Namespaces::SyncEvent.last) }
+
+    context 'when namespace mirror does not exist in the first place' do
       let(:namespace) { group3 }
 
-      it 'creates the hierarchy' do
-        expect { sync }.to change { described_class.count }.from(0).to(1)
+      before do
+        namespace.ci_namespace_mirror.destroy!
+        namespace.sync_events.create!
+      end
+
+      it 'creates the mirror' do
+        expect { sync }.to change { described_class.count }.from(3).to(4)
 
-        expect(namespace.ci_namespace_mirror).to have_attributes(traversal_ids: [group1.id, group2.id, group3.id])
+        expect(namespace.reload.ci_namespace_mirror).to have_attributes(traversal_ids: [group1.id, group2.id, group3.id])
       end
     end
 
-    context 'when namespace hierarchy does already exist' do
+    context 'when namespace mirror does already exist' do
       let(:namespace) { group3 }
 
       before do
-        described_class.create!(namespace: namespace, traversal_ids: [namespace.id])
+        namespace.sync_events.create!
       end
 
-      it 'updates the hierarchy' do
+      it 'updates the mirror' do
         expect { sync }.not_to change { described_class.count }
 
-        expect(namespace.ci_namespace_mirror).to have_attributes(traversal_ids: [group1.id, group2.id, group3.id])
+        expect(namespace.reload.ci_namespace_mirror).to have_attributes(traversal_ids: [group1.id, group2.id, group3.id])
       end
     end
 
-    # I did not extract this context to a `shared_context` because the behavior will change
-    # after implementing the TODO in `Ci::NamespaceMirror.sync!`
-    context 'changing the middle namespace' do
+    shared_context 'changing the middle namespace' do
       let(:namespace) { group2 }
 
       before do
-        described_class.create!(namespace_id: group1.id, traversal_ids: [group1.id])
-        described_class.create!(namespace_id: group2.id, traversal_ids: [group1.id, group2.id])
-        described_class.create!(namespace_id: group3.id, traversal_ids: [group1.id, group2.id, group3.id])
-        described_class.create!(namespace_id: group4.id, traversal_ids: [group1.id, group2.id, group3.id, group4.id])
-
-        group2.update!(parent: nil)
+        group2.update!(parent: nil) # creates a sync event
       end
 
-      it 'updates hierarchies for the base but wait for events for the children' do
+      it 'updates traversal_ids for the base and descendants' do
         expect { sync }.not_to change { described_class.count }
 
         expect(group1.reload.ci_namespace_mirror).to have_attributes(traversal_ids: [group1.id])
@@ -61,6 +88,8 @@ RSpec.describe Ci::NamespaceMirror do
       end
     end
 
+    it_behaves_like 'changing the middle namespace'
+
     context 'when the FFs sync_traversal_ids, use_traversal_ids and use_traversal_ids_for_ancestors are disabled' do
       before do
         stub_feature_flags(sync_traversal_ids: false,
@@ -68,27 +97,7 @@ RSpec.describe Ci::NamespaceMirror do
                            use_traversal_ids_for_ancestors: false)
       end
 
-      context 'changing the middle namespace' do
-        let(:namespace) { group2 }
-
-        before do
-          described_class.create!(namespace_id: group1.id, traversal_ids: [group1.id])
-          described_class.create!(namespace_id: group2.id, traversal_ids: [group1.id, group2.id])
-          described_class.create!(namespace_id: group3.id, traversal_ids: [group1.id, group2.id, group3.id])
-          described_class.create!(namespace_id: group4.id, traversal_ids: [group1.id, group2.id, group3.id, group4.id])
-
-          group2.update!(parent: nil)
-        end
-
-        it 'updates hierarchies for the base and descendants' do
-          expect { sync }.not_to change { described_class.count }
-
-          expect(group1.reload.ci_namespace_mirror).to have_attributes(traversal_ids: [group1.id])
-          expect(group2.reload.ci_namespace_mirror).to have_attributes(traversal_ids: [group2.id])
-          expect(group3.reload.ci_namespace_mirror).to have_attributes(traversal_ids: [group2.id, group3.id])
-          expect(group4.reload.ci_namespace_mirror).to have_attributes(traversal_ids: [group2.id, group3.id, group4.id])
-        end
-      end
+      it_behaves_like 'changing the middle namespace'
     end
   end
 end

spec/models/ci/project_mirror_spec.rb

@@ -8,12 +8,36 @@ RSpec.describe Ci::ProjectMirror do
 
   let!(:project) { create(:project, namespace: group2) }
 
+  context 'scopes' do
+    let_it_be(:another_project) { create(:project, namespace: group1) }
+
+    describe '.by_project_id' do
+      subject(:result) { described_class.by_project_id(project.id) }
+
+      it 'returns project mirrors of project' do
+        expect(result.pluck(:project_id)).to contain_exactly(project.id)
+      end
+    end
+
+    describe '.by_namespace_id' do
+      subject(:result) { described_class.by_namespace_id(group2.id) }
+
+      it 'returns project mirrors of namespace id' do
+        expect(result).to contain_exactly(project.ci_project_mirror)
+      end
+    end
+  end
+
   describe '.sync!' do
     let!(:event) { Projects::SyncEvent.create!(project: project) }
 
-    subject(:sync) { described_class.sync!(event.reload) }
+    subject(:sync) { described_class.sync!(event) }
+
+    context 'when project mirror does not exist in the first place' do
+      before do
+        project.ci_project_mirror.destroy!
+      end
 
-    context 'when project hierarchy does not exist in the first place' do
       it 'creates a ci_projects record' do
         expect { sync }.to change { described_class.count }.from(0).to(1)
 
@@ -21,11 +45,7 @@ RSpec.describe Ci::ProjectMirror do
       end
     end
 
-    context 'when project hierarchy does already exist' do
-      before do
-        described_class.create!(project_id: project.id, namespace_id: group1.id)
-      end
-
+    context 'when project mirror does already exist' do
       it 'updates the related ci_projects record' do
         expect { sync }.not_to change { described_class.count }
 

spec/models/ci/runner_spec.rb

@@ -1358,7 +1358,7 @@ RSpec.describe Ci::Runner do
     it { is_expected.to eq(contacted_at_stored) }
   end
 
-  describe '.belonging_to_group' do
+  describe '.legacy_belonging_to_group' do
     shared_examples 'returns group runners' do
       it 'returns the specific group runner' do
         group = create(:group)
@@ -1366,7 +1366,7 @@ RSpec.describe Ci::Runner do
         unrelated_group = create(:group)
         create(:ci_runner, :group, groups: [unrelated_group])
 
-        expect(described_class.belonging_to_group(group.id)).to contain_exactly(runner)
+        expect(described_class.legacy_belonging_to_group(group.id)).to contain_exactly(runner)
       end
 
       context 'runner belonging to parent group' do
@@ -1376,13 +1376,13 @@ RSpec.describe Ci::Runner do
 
         context 'when include_parent option is passed' do
           it 'returns the group runner from the parent group' do
-            expect(described_class.belonging_to_group(group.id, include_ancestors: true)).to contain_exactly(parent_runner)
+            expect(described_class.legacy_belonging_to_group(group.id, include_ancestors: true)).to contain_exactly(parent_runner)
           end
         end
 
         context 'when include_parent option is not passed' do
           it 'does not return the group runner from the parent group' do
-            expect(described_class.belonging_to_group(group.id)).to be_empty
+            expect(described_class.legacy_belonging_to_group(group.id)).to be_empty
           end
         end
       end
@@ -1398,4 +1398,38 @@ RSpec.describe Ci::Runner do
       it_behaves_like 'returns group runners'
     end
   end
+
+  describe '.belonging_to_group' do
+    it 'returns the specific group runner' do
+      group = create(:group)
+      runner = create(:ci_runner, :group, groups: [group])
+      unrelated_group = create(:group)
+      create(:ci_runner, :group, groups: [unrelated_group])
+
+      expect(described_class.belonging_to_group(group.id)).to contain_exactly(runner)
+    end
+  end
+
+  describe '.belonging_to_group_or_project_descendants' do
+    it 'returns the specific group runners' do
+      group1 = create(:group)
+      group2 = create(:group, parent: group1)
+      group3 = create(:group)
+
+      project1 = create(:project, namespace: group1)
+      project2 = create(:project, namespace: group2)
+      project3 = create(:project, namespace: group3)
+
+      runner1 = create(:ci_runner, :group, groups: [group1])
+      runner2 = create(:ci_runner, :group, groups: [group2])
+      _runner3 = create(:ci_runner, :group, groups: [group3])
+      runner4 = create(:ci_runner, :project, projects: [project1])
+      runner5 = create(:ci_runner, :project, projects: [project2])
+      _runner6 = create(:ci_runner, :project, projects: [project3])
+
+      expect(described_class.belonging_to_group_or_project_descendants(group1.id)).to contain_exactly(
+        runner1, runner2, runner4, runner5
+      )
+    end
+  end
 end

spec/services/ci/process_sync_events_service_spec.rb

@@ -28,10 +28,10 @@ RSpec.describe Ci::ProcessSyncEventsService do
       it 'consumes events' do
         expect { execute }.to change(Projects::SyncEvent, :count).from(2).to(0)
 
-        expect(project1.ci_project_mirror).to have_attributes(
+        expect(project1.reload.ci_project_mirror).to have_attributes(
           namespace_id: parent_group_1.id
         )
-        expect(project2.ci_project_mirror).to have_attributes(
+        expect(project2.reload.ci_project_mirror).to have_attributes(
           namespace_id: parent_group_2.id
         )
       end
@@ -88,10 +88,10 @@ RSpec.describe Ci::ProcessSyncEventsService do
         it 'consumes events' do
           expect { execute }.to change(Namespaces::SyncEvent, :count).from(2).to(0)
 
-          expect(group.ci_namespace_mirror).to have_attributes(
+          expect(group.reload.ci_namespace_mirror).to have_attributes(
             traversal_ids: [parent_group_1.id, parent_group_2.id, group.id]
           )
-          expect(parent_group_2.ci_namespace_mirror).to have_attributes(
+          expect(parent_group_2.reload.ci_namespace_mirror).to have_attributes(
             traversal_ids: [parent_group_1.id, parent_group_2.id]
           )
         end