From 39bc2b6079310ff2b9131ef49e602c0101df2e73 Mon Sep 17 00:00:00 2001 From: Furkan Ayhan Date: Tue, 28 Dec 2021 16:25:33 +0300 Subject: [PATCH 1/3] Resolve cross-db queries for Runner scopes The changes are behind the FF ci_find_runners_by_ci_mirrors --- app/models/ci/runner.rb | 7 +++++++ .../analytics/devops_adoption/snapshot_calculator.rb | 4 +++- .../devops_adoption/snapshot_calculator_spec.rb | 12 +++++++++++- lib/api/ci/runners.rb | 7 ++++++- spec/models/ci/runner_spec.rb | 10 ++++++++++ spec/requests/api/ci/runners_spec.rb | 12 +++++++++++- 6 files changed, 48 insertions(+), 4 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 1c4693772313aa..d928357b010194 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -112,6 +112,13 @@ class Runner < Ci::ApplicationRecord from("(#{union_sql}) #{table_name}") } + scope :belonging_to_group_and_ancestors, -> (group_id) { + group_self_and_ancestors_ids = ::Group.find(id: group_id).self_and_ancestor_ids + + joins(:runner_namespaces) + .where(ci_runner_namespaces: { namespace_id: group_self_and_ancestors_ids }) + } + # deprecated # split this into: belonging_to_group & belonging_to_group_and_ancestors scope :legacy_belonging_to_group, -> (group_id, include_ancestors: false) { diff --git a/ee/lib/analytics/devops_adoption/snapshot_calculator.rb b/ee/lib/analytics/devops_adoption/snapshot_calculator.rb index 7c5b0c59d982a2..ccb2898b4cde1e 100644 --- a/ee/lib/analytics/devops_adoption/snapshot_calculator.rb +++ b/ee/lib/analytics/devops_adoption/snapshot_calculator.rb @@ -61,7 +61,9 @@ def merge_request_approved # rubocop: enable CodeReuse/ActiveRecord def runner_configured - ::Gitlab::Database.allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/337541') do + if ::Feature.enabled?(:ci_find_runners_by_ci_mirrors, enabled_namespace.namespace, default_enabled: :yaml) + Ci::Runner.active.belonging_to_group_or_project_descendants(enabled_namespace.namespace.id).exists? + else Ci::Runner.active.legacy_belonging_to_group_or_project(snapshot_groups, snapshot_project_ids).exists? end end diff --git a/ee/spec/lib/analytics/devops_adoption/snapshot_calculator_spec.rb b/ee/spec/lib/analytics/devops_adoption/snapshot_calculator_spec.rb index 16f15df015a623..b6d909358977ce 100644 --- a/ee/spec/lib/analytics/devops_adoption/snapshot_calculator_spec.rb +++ b/ee/spec/lib/analytics/devops_adoption/snapshot_calculator_spec.rb @@ -61,7 +61,7 @@ it { is_expected.to eq false } end - describe 'runner_configured' do + shared_examples 'runner_configured' do subject { data[:runner_configured] } let!(:inactive_runner) { create(:ci_runner, :project, active: false) } @@ -77,6 +77,16 @@ it { is_expected.to eq false } end + it_behaves_like 'runner_configured' + + 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 'runner_configured' + end + describe 'pipeline_succeeded' do subject { data[:pipeline_succeeded] } diff --git a/lib/api/ci/runners.rb b/lib/api/ci/runners.rb index f39c139b4facf1..f21782a698fab6 100644 --- a/lib/api/ci/runners.rb +++ b/lib/api/ci/runners.rb @@ -229,7 +229,12 @@ class Runners < ::API::Base use :pagination end get ':id/runners' do - runners = ::Ci::Runner.legacy_belonging_to_group(user_group.id, include_ancestors: true) + runners = if ::Feature.enabled?(:ci_find_runners_by_ci_mirrors, user_group, default_enabled: :yaml) + ::Ci::Runner.belonging_to_group_and_ancestors(user_group.id) + else + ::Ci::Runner.legacy_belonging_to_group(user_group.id, include_ancestors: true) + end + runners = apply_filter(runners, params) present paginate(runners), with: Entities::Ci::Runner diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 8f66978c311740..6830a8daa3b537 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -1438,6 +1438,16 @@ def does_db_update end end + describe '.belonging_to_group_and_ancestors' do + let_it_be(:parent_group) { create(:group) } + let_it_be(:parent_runner) { create(:ci_runner, :group, groups: [parent_group]) } + let_it_be(:group) { create(:group, parent: parent_group) } + + it 'returns the group runner from the parent group' do + expect(described_class.belonging_to_group_and_ancestors(group.id)).to contain_exactly(parent_runner) + end + end + describe '.belonging_to_group_or_project_descendants' do it 'returns the specific group runners' do group1 = create(:group) diff --git a/spec/requests/api/ci/runners_spec.rb b/spec/requests/api/ci/runners_spec.rb index 6ca380a3cb92f2..305c0bd9df0685 100644 --- a/spec/requests/api/ci/runners_spec.rb +++ b/spec/requests/api/ci/runners_spec.rb @@ -980,7 +980,7 @@ def update_runner(id, user, args) end end - describe 'GET /groups/:id/runners' do + shared_context 'GET /groups/:id/runners' do context 'authorized user with maintainer privileges' do it 'returns all runners' do get api("/groups/#{group.id}/runners", user) @@ -1048,6 +1048,16 @@ def update_runner(id, user, args) end end + it_behaves_like 'GET /groups/:id/runners' + + 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 'GET /groups/:id/runners' + end + describe 'POST /projects/:id/runners' do context 'authorized user' do let_it_be(:project_runner2) { create(:ci_runner, :project, projects: [project2]) } -- GitLab From 3d2d6ceb73edf7c6424655f0c74cbb654b5a13aa Mon Sep 17 00:00:00 2001 From: Furkan Ayhan Date: Wed, 12 Jan 2022 18:06:04 +0300 Subject: [PATCH 2/3] Fix the failing tests --- app/models/ci/runner.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index d928357b010194..01063544e2aa6d 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -113,7 +113,7 @@ class Runner < Ci::ApplicationRecord } scope :belonging_to_group_and_ancestors, -> (group_id) { - group_self_and_ancestors_ids = ::Group.find(id: group_id).self_and_ancestor_ids + group_self_and_ancestors_ids = ::Group.find_by(id: group_id)&.self_and_ancestor_ids joins(:runner_namespaces) .where(ci_runner_namespaces: { namespace_id: group_self_and_ancestors_ids }) -- GitLab From e44211aa46fe08253bf4851f08ccf5036fafdda6 Mon Sep 17 00:00:00 2001 From: Furkan Ayhan Date: Mon, 17 Jan 2022 22:59:41 +0300 Subject: [PATCH 3/3] Add allow_cross_joins_across_databases back --- ee/lib/analytics/devops_adoption/snapshot_calculator.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ee/lib/analytics/devops_adoption/snapshot_calculator.rb b/ee/lib/analytics/devops_adoption/snapshot_calculator.rb index ccb2898b4cde1e..fe53bc4575b3fc 100644 --- a/ee/lib/analytics/devops_adoption/snapshot_calculator.rb +++ b/ee/lib/analytics/devops_adoption/snapshot_calculator.rb @@ -64,7 +64,9 @@ def runner_configured if ::Feature.enabled?(:ci_find_runners_by_ci_mirrors, enabled_namespace.namespace, default_enabled: :yaml) Ci::Runner.active.belonging_to_group_or_project_descendants(enabled_namespace.namespace.id).exists? else - Ci::Runner.active.legacy_belonging_to_group_or_project(snapshot_groups, snapshot_project_ids).exists? + ::Gitlab::Database.allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/337541') do + Ci::Runner.active.legacy_belonging_to_group_or_project(snapshot_groups, snapshot_project_ids).exists? + end end end -- GitLab