diff --git a/app/finders/ci/runners_finder.rb b/app/finders/ci/runners_finder.rb index 5d597f94f72c044c304ce68b29b9c4b14f611ac5..a514d2e8d034343a6cb84bfc63ceb8ef0224ab5d 100644 --- a/app/finders/ci/runners_finder.rb +++ b/app/finders/ci/runners_finder.rb @@ -53,9 +53,13 @@ def group_runners 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 diff --git a/app/models/ci/namespace_mirror.rb b/app/models/ci/namespace_mirror.rb index 8a4be3139e83dae0d34556633f4678e1288aa9f7..cff63c00e07b530cf02e817f6df00d9d37989f6e 100644 --- a/app/models/ci/namespace_mirror.rb +++ b/app/models/ci/namespace_mirror.rb @@ -10,6 +10,8 @@ class NamespaceMirror < ApplicationRecord 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 diff --git a/app/models/ci/project_mirror.rb b/app/models/ci/project_mirror.rb index d6aaa3f50c142c468f54dd4afd79d87de35af860..9000d1791a6c7e6d8c61512394f6f49bd7fe842e 100644 --- a/app/models/ci/project_mirror.rb +++ b/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 }, diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 6ba0a00ddae7aecba8f86b382f2b58832b69e1d2..193583d7c5dac1e6c02d674fc4108c9ec855941a 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -95,7 +95,26 @@ class Runner < Ci::ApplicationRecord 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 @@ class Runner < Ci::ApplicationRecord .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 @@ class Runner < Ci::ApplicationRecord .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 @@ class Runner < Ci::ApplicationRecord .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( [ diff --git a/config/feature_flags/development/ci_find_runners_by_ci_mirrors.yml b/config/feature_flags/development/ci_find_runners_by_ci_mirrors.yml new file mode 100644 index 0000000000000000000000000000000000000000..337e6b11408047014daa3c79021d23a78e897b44 --- /dev/null +++ b/config/feature_flags/development/ci_find_runners_by_ci_mirrors.yml @@ -0,0 +1,8 @@ +--- +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 diff --git a/ee/lib/analytics/devops_adoption/snapshot_calculator.rb b/ee/lib/analytics/devops_adoption/snapshot_calculator.rb index 0ad5d021102a2150941352f57ff1325836cf6983..7c5b0c59d982a2c5f724cbbc885cbb2af1d642eb 100644 --- a/ee/lib/analytics/devops_adoption/snapshot_calculator.rb +++ b/ee/lib/analytics/devops_adoption/snapshot_calculator.rb @@ -62,7 +62,7 @@ def merge_request_approved 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 diff --git a/lib/api/ci/runners.rb b/lib/api/ci/runners.rb index ef712c84804ec2171c94445e1217a733c4ffb4f0..f39c139b4facf1727fbbb8fc3e014dc9373afc50 100644 --- a/lib/api/ci/runners.rb +++ b/lib/api/ci/runners.rb @@ -229,7 +229,7 @@ class Runners < ::API::Base 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 diff --git a/spec/factories/namespaces.rb b/spec/factories/namespaces.rb index 2b3dabc07d86af8e2cd0556f2e2a93d267f9f88d..e88bb634898b1a824b75b7f8e10abf08c069910a 100644 --- a/spec/factories/namespaces.rb +++ b/spec/factories/namespaces.rb @@ -11,6 +11,14 @@ 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) diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb index 981f10e8260ec32b0af11da36d98598de3804dbc..f34f6b67ef88f21ab2baa60c0cc8043a8cbce52c 100644 --- a/spec/factories/projects.rb +++ b/spec/factories/projects.rb @@ -101,6 +101,9 @@ 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 diff --git a/spec/finders/ci/runners_finder_spec.rb b/spec/finders/ci/runners_finder_spec.rb index 7e3c1abd6d1e94fab3624b5be75d1cb254afe8c6..dac244e4300d39984582bce6c75a9886827b7117 100644 --- a/spec/finders/ci/runners_finder_spec.rb +++ b/spec/finders/ci/runners_finder_spec.rb @@ -206,7 +206,7 @@ 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 @@ 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 } diff --git a/spec/models/ci/namespace_mirror_spec.rb b/spec/models/ci/namespace_mirror_spec.rb index b4c71f51377f1d87aa15bd66fb556789345d7dfa..fd0581341b73f86ca63b53e959e8c5ea03273f92 100644 --- a/spec/models/ci/namespace_mirror_spec.rb +++ b/spec/models/ci/namespace_mirror_spec.rb @@ -8,50 +8,77 @@ 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 @@ 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 @@ 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 diff --git a/spec/models/ci/project_mirror_spec.rb b/spec/models/ci/project_mirror_spec.rb index 199285b036c3ecf24dbb26b6c556299a50d45953..5ef520b4230c13b924d15430ddd8768ba05f8ef3 100644 --- a/spec/models/ci/project_mirror_spec.rb +++ b/spec/models/ci/project_mirror_spec.rb @@ -8,12 +8,36 @@ 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 @@ 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 } diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 5142f70fa2c233782a0594c8d793355c59a9e0d0..868551d31e835f9bb7e07224773fffe1882a0031 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -1358,7 +1358,7 @@ def does_db_update 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 @@ def does_db_update 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 @@ def does_db_update 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 @@ def does_db_update 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 diff --git a/spec/services/ci/process_sync_events_service_spec.rb b/spec/services/ci/process_sync_events_service_spec.rb index 00b670ff54f28b2a57f53a637f6f7d099b00f582..c4d4a2a4776e988e49c050778a22b81a5cdd41e0 100644 --- a/spec/services/ci/process_sync_events_service_spec.rb +++ b/spec/services/ci/process_sync_events_service_spec.rb @@ -28,10 +28,10 @@ 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 @@ 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