From 3b372463f06e6ac0cdf13dd39efe16bd3d83e8a9 Mon Sep 17 00:00:00 2001 From: Adrien Kohlbecker Date: Fri, 30 Jul 2021 15:53:25 +0200 Subject: [PATCH 1/6] Remove project/ci_* join on Runner.belonging_to_group_or_project Changelog: changed --- app/models/ci/runner.rb | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 4c5e4837d0687a..880b4cbf4f5f3c 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -88,12 +88,10 @@ class Runner < Ci::ApplicationRecord scope :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 }) - project_runners = joins(:runner_projects).where(ci_runner_projects: { project_id: project_id }) + group_runner_ids = Ci::RunnerNamespace.where(namespace_id: groups).pluck(:runner_id) + project_runner_ids = Ci::RunnerProject.where(project_id: project_id).pluck(:runner_id) - union_sql = ::Gitlab::SQL::Union.new([group_runners, project_runners]).to_sql - - from("(#{union_sql}) #{table_name}") + where(id: [group_runner_ids, project_runner_ids].flatten) } scope :belonging_to_parent_group_of_project, -> (project_id) { -- GitLab From 924d08a323136f6999832e13ee07253dc7dfc8d5 Mon Sep 17 00:00:00 2001 From: Adrien Kohlbecker Date: Wed, 4 Aug 2021 13:54:45 +0200 Subject: [PATCH 2/6] Fix query --- app/models/ci/runner.rb | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 880b4cbf4f5f3c..41be19ef5762af 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -86,12 +86,15 @@ class Runner < Ci::ApplicationRecord } scope :belonging_to_group_or_project, -> (group_id, project_id) { - groups = ::Group.where(id: group_id) + group_ids = ::Group.where(id: group_id).ids + project_ids = ::Project.where(id: project_id).ids + + 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 }) - group_runner_ids = Ci::RunnerNamespace.where(namespace_id: groups).pluck(:runner_id) - project_runner_ids = Ci::RunnerProject.where(project_id: project_id).pluck(:runner_id) + union_sql = ::Gitlab::SQL::Union.new([group_runners, project_runners]).to_sql - where(id: [group_runner_ids, project_runner_ids].flatten) + from("(#{union_sql}) #{table_name}") } scope :belonging_to_parent_group_of_project, -> (project_id) { -- GitLab From aa8adf0b8a79f39598b8d791d6f605236823944c Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Tue, 21 Sep 2021 15:15:00 +0200 Subject: [PATCH 3/6] Initial CI project/namespace hierarchy implementation Missing: - record lifecycle management in Rails app logic - tests - migration optimizations --- app/finders/ci/runners_finder.rb | 3 +- app/models/ci/namespace_hierarchy.rb | 12 +++++++ app/models/ci/project_hierarchy.rb | 11 +++++++ app/models/ci/runner.rb | 11 +++---- app/models/ci/runner_project.rb | 4 +-- ...0131736_create_ci_namespace_hierarchies.rb | 15 +++++++++ ...0920134647_add_ci_namespace_hierarchies.rb | 30 ++++++++++++++++++ ...921095325_create_ci_project_hierarchies.rb | 15 +++++++++ ...210921095839_add_ci_project_hierarchies.rb | 31 +++++++++++++++++++ db/schema_migrations/20210920131736 | 1 + db/schema_migrations/20210920134647 | 1 + db/schema_migrations/20210921095325 | 1 + db/schema_migrations/20210921095839 | 1 + db/structure.sql | 20 ++++++++++++ .../devops_adoption/snapshot_calculator.rb | 4 +-- spec/models/ci/namespace_hierarchy_spec.rb | 7 +++++ spec/models/ci/project_hierarchy_spec.rb | 7 +++++ 17 files changed, 161 insertions(+), 13 deletions(-) create mode 100644 app/models/ci/namespace_hierarchy.rb create mode 100644 app/models/ci/project_hierarchy.rb create mode 100644 db/migrate/20210920131736_create_ci_namespace_hierarchies.rb create mode 100644 db/migrate/20210920134647_add_ci_namespace_hierarchies.rb create mode 100644 db/migrate/20210921095325_create_ci_project_hierarchies.rb create mode 100644 db/migrate/20210921095839_add_ci_project_hierarchies.rb create mode 100644 db/schema_migrations/20210920131736 create mode 100644 db/schema_migrations/20210920134647 create mode 100644 db/schema_migrations/20210921095325 create mode 100644 db/schema_migrations/20210921095839 create mode 100644 spec/models/ci/namespace_hierarchy_spec.rb create mode 100644 spec/models/ci/project_hierarchy_spec.rb diff --git a/app/finders/ci/runners_finder.rb b/app/finders/ci/runners_finder.rb index 8bc2a47a024fd9..b38db61abdde80 100644 --- a/app/finders/ci/runners_finder.rb +++ b/app/finders/ci/runners_finder.rb @@ -53,8 +53,7 @@ def group_runners 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) + Ci::Runner.belonging_to_group_or_descendants(@group) else raise ArgumentError, 'Invalid membership filter' end diff --git a/app/models/ci/namespace_hierarchy.rb b/app/models/ci/namespace_hierarchy.rb new file mode 100644 index 00000000000000..56e1c3277a28b8 --- /dev/null +++ b/app/models/ci/namespace_hierarchy.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +module Ci + # This model represents a shadow table of the main database's namespaces table (though only represents namespaces containing runners). + # It allows us to navigate the namespace hierarchy on the ci database without resorting to a JOIN. + class NamespaceHierarchy < ApplicationRecord + scope :belonging_to_namespace, -> (namespace_id) { + where('traversal_ids && ARRAY[?]::int[]', namespace_id) + .select('traversal_ids[array_length(traversal_ids, 1)]') + } + end +end diff --git a/app/models/ci/project_hierarchy.rb b/app/models/ci/project_hierarchy.rb new file mode 100644 index 00000000000000..5955023324e8cf --- /dev/null +++ b/app/models/ci/project_hierarchy.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module Ci + # This model represents a shadow table of the main database's namespaces table (though only represents projects containing runners). + # It allows us to navigate the namespace hierarchy on the ci database for a project without resorting to a JOIN. + class ProjectHierarchy < ApplicationRecord + scope :belonging_to_namespace, -> (namespace_id) { + where('traversal_ids && ARRAY[?]::int[]', namespace_id).select(:project_xid) + } + end +end diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 41be19ef5762af..6eddb170c74f79 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -85,14 +85,13 @@ class Runner < Ci::ApplicationRecord joins(:runner_namespaces).where(ci_runner_namespaces: { namespace_id: groups }) } - scope :belonging_to_group_or_project, -> (group_id, project_id) { - group_ids = ::Group.where(id: group_id).ids - project_ids = ::Project.where(id: project_id).ids + scope :belonging_to_group_or_descendants, -> (group_id) { + groups = ::Group.where(id: group_id).ids - 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 }) + runners_on_groups = joins(:runner_namespaces).where(ci_runner_namespaces: { namespace_id: Ci::NamespaceHierarchy.belonging_to_namespace(groups) }) + runners_on_projects = belonging_to_project(Ci::ProjectHierarchy.belonging_to_namespace(groups)) - union_sql = ::Gitlab::SQL::Union.new([group_runners, project_runners]).to_sql + union_sql = ::Gitlab::SQL::Union.new([runners_on_groups, runners_on_projects]).to_sql from("(#{union_sql}) #{table_name}") } diff --git a/app/models/ci/runner_project.rb b/app/models/ci/runner_project.rb index e1c435e9b1f0e7..72dff9fc7625c0 100644 --- a/app/models/ci/runner_project.rb +++ b/app/models/ci/runner_project.rb @@ -13,10 +13,10 @@ class RunnerProject < Ci::ApplicationRecord belongs_to :runner, inverse_of: :runner_projects belongs_to :project, inverse_of: :runner_projects + validates :runner_id, uniqueness: { scope: :project_id } + def recent_runners ::Ci::Runner.belonging_to_project(project_id).recent end - - validates :runner_id, uniqueness: { scope: :project_id } end end diff --git a/db/migrate/20210920131736_create_ci_namespace_hierarchies.rb b/db/migrate/20210920131736_create_ci_namespace_hierarchies.rb new file mode 100644 index 00000000000000..f57b4a8f215c54 --- /dev/null +++ b/db/migrate/20210920131736_create_ci_namespace_hierarchies.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class CreateCiNamespaceHierarchies < Gitlab::Database::Migration[1.0] + TABLE_NAME = :ci_namespace_hierarchies + INDEX_NAME = "index_gin_#{TABLE_NAME}" + + def change + create_table TABLE_NAME, id: false, primary_key: :namespace_xid do |t| + t.integer :namespace_xid, null: false, primary_key: true, default: nil + t.integer :traversal_ids, array: true, default: [], null: false + + t.index :traversal_ids, name: INDEX_NAME, using: :gin + end + end +end diff --git a/db/migrate/20210920134647_add_ci_namespace_hierarchies.rb b/db/migrate/20210920134647_add_ci_namespace_hierarchies.rb new file mode 100644 index 00000000000000..cccb9c53d5879b --- /dev/null +++ b/db/migrate/20210920134647_add_ci_namespace_hierarchies.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +class AddCiNamespaceHierarchies < Gitlab::Database::Migration[1.0] + TABLE_NAME = :ci_namespace_hierarchies + + disable_ddl_transaction! + + def up + with_lock_retries do + execute(<<~SQL) + INSERT INTO #{TABLE_NAME} (namespace_xid, traversal_ids) + SELECT + namespace_id AS namespace_xid, + namespaces.traversal_ids AS traversal_ids + FROM + ci_runner_namespaces + INNER JOIN namespaces ON ci_runner_namespaces.namespace_id = namespaces.id + ON CONFLICT(namespace_xid) DO NOTHING + SQL + end + end + + def down + with_lock_retries do + execute(<<~SQL) + DELETE FROM #{TABLE_NAME} + SQL + end + end +end diff --git a/db/migrate/20210921095325_create_ci_project_hierarchies.rb b/db/migrate/20210921095325_create_ci_project_hierarchies.rb new file mode 100644 index 00000000000000..c99fade7c8402a --- /dev/null +++ b/db/migrate/20210921095325_create_ci_project_hierarchies.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class CreateCiProjectHierarchies < Gitlab::Database::Migration[1.0] + TABLE_NAME = :ci_project_hierarchies + INDEX_NAME = "index_gin_#{TABLE_NAME}" + + def change + create_table TABLE_NAME, id: false, primary_key: :project_xid do |t| + t.integer :project_xid, null: false, primary_key: true, default: nil + t.integer :traversal_ids, array: true, default: [], null: false + + t.index :traversal_ids, name: INDEX_NAME, using: :gin + end + end +end diff --git a/db/migrate/20210921095839_add_ci_project_hierarchies.rb b/db/migrate/20210921095839_add_ci_project_hierarchies.rb new file mode 100644 index 00000000000000..5dac4589c345ce --- /dev/null +++ b/db/migrate/20210921095839_add_ci_project_hierarchies.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +class AddCiProjectHierarchies < Gitlab::Database::Migration[1.0] + disable_ddl_transaction! + + TABLE_NAME = :ci_project_hierarchies + + def up + with_lock_retries do + execute(<<~SQL) + INSERT INTO #{TABLE_NAME} (project_xid, traversal_ids) + SELECT + projects.id AS project_xid, + namespaces.traversal_ids AS traversal_ids + FROM + ci_runner_projects + INNER JOIN projects ON ci_runner_projects.project_id = projects.id + INNER JOIN namespaces ON projects.namespace_id = namespaces.id + ON CONFLICT(project_xid) DO NOTHING + SQL + end + end + + def down + with_lock_retries do + execute(<<~SQL) + DELETE FROM #{TABLE_NAME} + SQL + end + end +end diff --git a/db/schema_migrations/20210920131736 b/db/schema_migrations/20210920131736 new file mode 100644 index 00000000000000..6585b2f698b809 --- /dev/null +++ b/db/schema_migrations/20210920131736 @@ -0,0 +1 @@ +cd745a4e843bc225e27d16108a13d3717fd10b7702c884fcd4efa756bfa54809 \ No newline at end of file diff --git a/db/schema_migrations/20210920134647 b/db/schema_migrations/20210920134647 new file mode 100644 index 00000000000000..d39bf7151baafd --- /dev/null +++ b/db/schema_migrations/20210920134647 @@ -0,0 +1 @@ +a4709d6afb563378d5fc76250b2a6e853997934dd16d5400cd33ef3b93dc29ec \ No newline at end of file diff --git a/db/schema_migrations/20210921095325 b/db/schema_migrations/20210921095325 new file mode 100644 index 00000000000000..903fc27f9912bd --- /dev/null +++ b/db/schema_migrations/20210921095325 @@ -0,0 +1 @@ +5b6df07bf86a45be69e755557cea730b756709faf7dea34fc6fc28cc2f395eb7 \ No newline at end of file diff --git a/db/schema_migrations/20210921095839 b/db/schema_migrations/20210921095839 new file mode 100644 index 00000000000000..128bdb128327ed --- /dev/null +++ b/db/schema_migrations/20210921095839 @@ -0,0 +1 @@ +32731b47d5979b4ec58a0d67149e428401d72e6ce477bef42d903fb61f9ba872 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 80689a24f2a5dd..75b253c662852c 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -11597,6 +11597,11 @@ CREATE SEQUENCE ci_minutes_additional_packs_id_seq ALTER SEQUENCE ci_minutes_additional_packs_id_seq OWNED BY ci_minutes_additional_packs.id; +CREATE TABLE ci_namespace_hierarchies ( + namespace_xid integer NOT NULL, + traversal_ids integer[] DEFAULT '{}'::integer[] NOT NULL +); + CREATE TABLE ci_namespace_monthly_usages ( id bigint NOT NULL, namespace_id bigint NOT NULL, @@ -11843,6 +11848,11 @@ CREATE SEQUENCE ci_platform_metrics_id_seq ALTER SEQUENCE ci_platform_metrics_id_seq OWNED BY ci_platform_metrics.id; +CREATE TABLE ci_project_hierarchies ( + project_xid integer NOT NULL, + traversal_ids integer[] DEFAULT '{}'::integer[] NOT NULL +); + CREATE TABLE ci_project_monthly_usages ( id bigint NOT NULL, project_id bigint NOT NULL, @@ -22463,6 +22473,9 @@ ALTER TABLE ONLY ci_job_variables ALTER TABLE ONLY ci_minutes_additional_packs ADD CONSTRAINT ci_minutes_additional_packs_pkey PRIMARY KEY (id); +ALTER TABLE ONLY ci_namespace_hierarchies + ADD CONSTRAINT ci_namespace_hierarchies_pkey PRIMARY KEY (namespace_xid); + ALTER TABLE ONLY ci_namespace_monthly_usages ADD CONSTRAINT ci_namespace_monthly_usages_pkey PRIMARY KEY (id); @@ -22496,6 +22509,9 @@ ALTER TABLE ONLY ci_pipelines ALTER TABLE ONLY ci_platform_metrics ADD CONSTRAINT ci_platform_metrics_pkey PRIMARY KEY (id); +ALTER TABLE ONLY ci_project_hierarchies + ADD CONSTRAINT ci_project_hierarchies_pkey PRIMARY KEY (project_xid); + ALTER TABLE ONLY ci_project_monthly_usages ADD CONSTRAINT ci_project_monthly_usages_pkey PRIMARY KEY (id); @@ -25173,8 +25189,12 @@ CREATE INDEX index_geo_reset_checksum_events_on_project_id ON geo_reset_checksum CREATE INDEX index_geo_upload_deleted_events_on_upload_id ON geo_upload_deleted_events USING btree (upload_id); +CREATE INDEX index_gin_ci_namespace_hierarchies ON ci_namespace_hierarchies USING gin (traversal_ids); + CREATE INDEX index_gin_ci_pending_builds_on_namespace_traversal_ids ON ci_pending_builds USING gin (namespace_traversal_ids); +CREATE INDEX index_gin_ci_project_hierarchies ON ci_project_hierarchies USING gin (traversal_ids); + CREATE INDEX index_gitlab_subscription_histories_on_gitlab_subscription_id ON gitlab_subscription_histories USING btree (gitlab_subscription_id); CREATE INDEX index_gitlab_subscriptions_on_end_date_and_namespace_id ON gitlab_subscriptions USING btree (end_date, namespace_id); diff --git a/ee/lib/analytics/devops_adoption/snapshot_calculator.rb b/ee/lib/analytics/devops_adoption/snapshot_calculator.rb index 0ad5d021102a21..620640fef14f3f 100644 --- a/ee/lib/analytics/devops_adoption/snapshot_calculator.rb +++ b/ee/lib/analytics/devops_adoption/snapshot_calculator.rb @@ -61,9 +61,7 @@ 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 - Ci::Runner.active.belonging_to_group_or_project(snapshot_groups, snapshot_project_ids).exists? - end + Ci::Runner.active.belonging_to_group_or_descendants(enabled_namespace.namespace).exists? end def pipeline_succeeded diff --git a/spec/models/ci/namespace_hierarchy_spec.rb b/spec/models/ci/namespace_hierarchy_spec.rb new file mode 100644 index 00000000000000..db6a051c512b7d --- /dev/null +++ b/spec/models/ci/namespace_hierarchy_spec.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::NamespaceHierarchy do + pending "add some examples to (or delete) #{__FILE__}" +end diff --git a/spec/models/ci/project_hierarchy_spec.rb b/spec/models/ci/project_hierarchy_spec.rb new file mode 100644 index 00000000000000..b017dd1ab8ed6a --- /dev/null +++ b/spec/models/ci/project_hierarchy_spec.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::ProjectHierarchy do + pending "add some examples to (or delete) #{__FILE__}" +end -- GitLab From bb8f696d0b00e53e7d3b1d6994eb2be2acc0891a Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Wed, 22 Sep 2021 13:25:25 +0200 Subject: [PATCH 4/6] Add minimal traversal_ids sync capability - Sync on runner association create/destroy - Add some quick tests (to be reviewed/refactored) --- app/models/ci/runner_namespace.rb | 19 ++++++++ app/models/ci/runner_project.rb | 21 ++++++++ spec/models/ci/runner_namespace_spec.rb | 64 ++++++++++++++++++++++++ spec/models/ci/runner_project_spec.rb | 65 +++++++++++++++++++++++++ spec/models/ci/runner_spec.rb | 34 ++++++++++++- 5 files changed, 202 insertions(+), 1 deletion(-) diff --git a/app/models/ci/runner_namespace.rb b/app/models/ci/runner_namespace.rb index d1353b97ed9af9..d128d6b115983a 100644 --- a/app/models/ci/runner_namespace.rb +++ b/app/models/ci/runner_namespace.rb @@ -14,6 +14,9 @@ class RunnerNamespace < Ci::ApplicationRecord belongs_to :namespace, inverse_of: :runner_namespaces, class_name: '::Namespace' belongs_to :group, class_name: '::Group', foreign_key: :namespace_id + after_create :insert_namespace_hierarchy + after_destroy :delete_namespace_hierarchy + validates :runner_id, uniqueness: { scope: :namespace_id } validate :group_runner_type @@ -26,5 +29,21 @@ def recent_runners def group_runner_type errors.add(:runner, 'is not a group runner') unless runner&.group_type? end + + def insert_namespace_hierarchy + Ci::NamespaceHierarchy.insert( + { + namespace_xid: namespace_id, + traversal_ids: namespace.self_and_ancestor_ids(hierarchy_order: :desc) + }, + unique_by: :namespace_xid + ) + end + + def delete_namespace_hierarchy + Ci::NamespaceHierarchy + .where(namespace_xid: namespace_id) + .delete_by('NOT EXISTS(?)', self.class.select(1).where(namespace_id: namespace_id)) + end end end diff --git a/app/models/ci/runner_project.rb b/app/models/ci/runner_project.rb index 72dff9fc7625c0..d4874f69a7be47 100644 --- a/app/models/ci/runner_project.rb +++ b/app/models/ci/runner_project.rb @@ -13,10 +13,31 @@ class RunnerProject < Ci::ApplicationRecord belongs_to :runner, inverse_of: :runner_projects belongs_to :project, inverse_of: :runner_projects + after_create :insert_project_hierarchy + after_destroy :delete_project_hierarchy + validates :runner_id, uniqueness: { scope: :project_id } def recent_runners ::Ci::Runner.belonging_to_project(project_id).recent end + + private + + def insert_project_hierarchy + Ci::ProjectHierarchy.insert( + { + project_xid: project_id, + traversal_ids: project.namespace&.self_and_ancestor_ids(hierarchy_order: :desc) + }, + unique_by: :project_xid + ) + end + + def delete_project_hierarchy + Ci::ProjectHierarchy + .where(project_xid: project_id) + .delete_by('NOT EXISTS(?)', self.class.select(1).where(project_id: project_id)) + end end end diff --git a/spec/models/ci/runner_namespace_spec.rb b/spec/models/ci/runner_namespace_spec.rb index 4e7cf7a3cb3ca0..2b91d8ee955791 100644 --- a/spec/models/ci/runner_namespace_spec.rb +++ b/spec/models/ci/runner_namespace_spec.rb @@ -12,4 +12,68 @@ subject { build(:ci_runner_namespace, group: create(:group, :nested), runner: create(:ci_runner, :group)) } end + + context 'namespace_hierarchy lifetime management' do + let_it_be(:group) { create(:group) } + let_it_be(:sub_group) { create(:group, parent: group) } + + subject { ::Ci::NamespaceHierarchy.where(namespace_xid: sub_group.id).to_a } + + context 'when no runner_namespace is created' do + it 'no matching NamespaceHierarchy model exists' do + is_expected.to eq([]) + end + end + + context 'when single runner_namespace is created' do + let_it_be(:runner_sub_group) { create(:ci_runner_namespace, namespace: sub_group, runner: create(:ci_runner, :group)) } + + it 'creates matching NamespaceHierarchy model' do + is_expected.to match([an_instance_of(::Ci::NamespaceHierarchy)]) + end + + specify 'matching NamespaceHierarchy model contains expected traversal_ids' do + expect(subject.first.traversal_ids).to eq([group.id, sub_group.id]) + end + + context 'when runner_namespace is destroyed' do + before do + runner_sub_group.destroy! + end + + specify 'the matching NamespaceHierarchy model is also deleted' do + is_expected.to be_empty + end + end + end + + context 'when multiple runner_namespaces are created' do + let_it_be(:runner_namespace_1) { create(:ci_runner_namespace, namespace: sub_group, runner: create(:ci_runner, :group)) } + let_it_be(:runner_namespace_2) { create(:ci_runner_namespace, namespace: sub_group, runner: create(:ci_runner, :group)) } + + it 'creates single matching NamespaceHierarchy model' do + is_expected.to match([an_instance_of(::Ci::NamespaceHierarchy)]) + end + + context 'when runner_namespace_2 is destroyed' do + before do + runner_namespace_2.destroy! + end + + specify 'the matching NamespaceHierarchy model is not deleted' do + is_expected.not_to be_empty + end + + context 'when all runner_namespaces are destroyed' do + before do + ::Ci::RunnerNamespace.destroy_by(namespace_id: sub_group.id) + end + + specify 'the matching NamespaceHierarchy model is also deleted' do + is_expected.to be_empty + end + end + end + end + end end diff --git a/spec/models/ci/runner_project_spec.rb b/spec/models/ci/runner_project_spec.rb index fef1416a84a6c7..5e7f10296e408f 100644 --- a/spec/models/ci/runner_project_spec.rb +++ b/spec/models/ci/runner_project_spec.rb @@ -12,4 +12,69 @@ subject { build(:ci_runner_project, project: create(:project), runner: create(:ci_runner, :project)) } end + + context 'project_hierarchy lifetime management' do + let_it_be(:group) { create(:group) } + let_it_be(:sub_group) { create(:group, parent: group) } + let_it_be(:project) { create(:project, group: sub_group) } + + subject { ::Ci::ProjectHierarchy.where(project_xid: project.id).to_a } + + context 'when no runner_project is created' do + it 'no matching ProjectHierarchy model exists' do + is_expected.to eq([]) + end + end + + context 'when single runner_project is created' do + let_it_be(:runner_project) { create(:ci_runner_project, project: project, runner: create(:ci_runner, :project)) } + + it 'creates matching ProjectHierarchy model' do + is_expected.to match([an_instance_of(::Ci::ProjectHierarchy)]) + end + + specify 'matching ProjectHierarchy model contains expected traversal_ids' do + expect(subject.first.traversal_ids).to eq([group.id, sub_group.id]) + end + + context 'when runner_project is destroyed' do + before do + runner_project.destroy! + end + + specify 'the matching ProjectHierarchy model is also deleted' do + is_expected.to be_empty + end + end + end + + context 'when multiple runner_projects are created' do + let_it_be(:runner_project_1) { create(:ci_runner_project, project: project, runner: create(:ci_runner, :project)) } + let_it_be(:runner_project_2) { create(:ci_runner_project, project: project, runner: create(:ci_runner, :project)) } + + it 'creates single matching ProjectHierarchy model' do + is_expected.to match([an_instance_of(::Ci::ProjectHierarchy)]) + end + + context 'when runner_project_2 is destroyed' do + before do + runner_project_2.destroy! + end + + specify 'the matching ProjectHierarchy model is not deleted' do + is_expected.not_to be_empty + end + + context 'when all runner_projects are destroyed' do + before do + ::Ci::RunnerProject.destroy_by(project_id: project.id) + end + + specify 'the matching ProjectHierarchy model is also deleted' do + is_expected.to be_empty + end + end + end + end + end end diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 5364dfc972c9e2..30765a4c99c162 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -6,7 +6,7 @@ it_behaves_like 'having unique enum values' describe 'groups association' do - # Due to other assoctions such as projects this whole spec is allowed to + # Due to other associations such as projects this whole spec is allowed to # generate cross-database queries. So we have this temporary spec to # validate that at least groups association does not generate cross-DB # queries. @@ -1174,4 +1174,36 @@ def does_db_update end end end + + describe '.belonging_to_group_or_descendants' do + subject { described_class.belonging_to_group_or_descendants(group.id) } + + let_it_be(:group) { create(:group) } + + context 'runner belonging to group' do + let_it_be(:group_runner) { create(:ci_runner, :group, groups: [group]) } + + it 'returns the group runner' do + is_expected.to contain_exactly(group_runner) + end + + context 'runner belonging to descendant group' do + let_it_be(:child_group) { create(:group, parent: group) } + let_it_be(:child_group_runner) { create(:ci_runner, :group, groups: [child_group]) } + + it 'returns the group runner from the child group' do + is_expected.to contain_exactly(group_runner, child_group_runner) + end + + context 'runner belonging to project on descendant group' do + let_it_be(:project) { create(:project, group: child_group) } + let_it_be(:child_project_runner) { create(:ci_runner, :project, projects: [project]) } + + it 'returns the project runner from the child group project' do + is_expected.to contain_exactly(group_runner, child_group_runner, child_project_runner) + end + end + end + end + end end -- GitLab From 402aa4e6bd6d7d688e60e073c8268fb8f5c3c674 Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Thu, 23 Sep 2021 19:43:53 +0200 Subject: [PATCH 5/6] Address project/group moves --- app/models/ci/namespace_hierarchy.rb | 10 +- app/models/ci/project_hierarchy.rb | 18 +++- app/models/ci/runner.rb | 4 +- app/models/namespace.rb | 14 +++ app/models/project.rb | 12 +++ spec/models/ci/namespace_hierarchy_spec.rb | 119 ++++++++++++++++++++- spec/models/ci/project_hierarchy_spec.rb | 85 ++++++++++++++- 7 files changed, 256 insertions(+), 6 deletions(-) diff --git a/app/models/ci/namespace_hierarchy.rb b/app/models/ci/namespace_hierarchy.rb index 56e1c3277a28b8..2a105968ba7086 100644 --- a/app/models/ci/namespace_hierarchy.rb +++ b/app/models/ci/namespace_hierarchy.rb @@ -5,8 +5,16 @@ module Ci # It allows us to navigate the namespace hierarchy on the ci database without resorting to a JOIN. class NamespaceHierarchy < ApplicationRecord scope :belonging_to_namespace, -> (namespace_id) { + return where('array_length(traversal_ids, 1) = 1') unless namespace_id + where('traversal_ids && ARRAY[?]::int[]', namespace_id) - .select('traversal_ids[array_length(traversal_ids, 1)]') } + + class << self + def update_traversal_ids(old_self_and_ancestor_ids, new_self_and_ancestor_ids) + where('traversal_ids @> ARRAY[?]::int[]', old_self_and_ancestor_ids) + .update_all("traversal_ids = ARRAY#{new_self_and_ancestor_ids}::int[] || traversal_ids[#{old_self_and_ancestor_ids.length + 1}:]") + end + end end end diff --git a/app/models/ci/project_hierarchy.rb b/app/models/ci/project_hierarchy.rb index 5955023324e8cf..ba34c567269ee5 100644 --- a/app/models/ci/project_hierarchy.rb +++ b/app/models/ci/project_hierarchy.rb @@ -5,7 +5,23 @@ module Ci # It allows us to navigate the namespace hierarchy on the ci database for a project without resorting to a JOIN. class ProjectHierarchy < ApplicationRecord scope :belonging_to_namespace, -> (namespace_id) { - where('traversal_ids && ARRAY[?]::int[]', namespace_id).select(:project_xid) + where('traversal_ids && ARRAY[?]::int[]', namespace_id) } + + class << self + def update_project_traversal_ids(project_id, old_self_and_ancestor_ids, new_self_and_ancestor_ids) + update_all_traversal_ids(where(project_xid: project_id), old_self_and_ancestor_ids, new_self_and_ancestor_ids) + end + + def update_namespace_traversal_ids(old_self_and_ancestor_ids, new_self_and_ancestor_ids) + update_all_traversal_ids(belonging_to_namespace(old_self_and_ancestor_ids), old_self_and_ancestor_ids, new_self_and_ancestor_ids) + end + + private + + def update_all_traversal_ids(assoc, old_self_and_ancestor_ids, new_self_and_ancestor_ids) + assoc.update_all("traversal_ids = ARRAY#{new_self_and_ancestor_ids}::int[] || traversal_ids[#{old_self_and_ancestor_ids.length + 1}:]") + end + end end end diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 6eddb170c74f79..afc9d30d54ca0c 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -88,8 +88,8 @@ class Runner < Ci::ApplicationRecord scope :belonging_to_group_or_descendants, -> (group_id) { groups = ::Group.where(id: group_id).ids - runners_on_groups = joins(:runner_namespaces).where(ci_runner_namespaces: { namespace_id: Ci::NamespaceHierarchy.belonging_to_namespace(groups) }) - runners_on_projects = belonging_to_project(Ci::ProjectHierarchy.belonging_to_namespace(groups)) + runners_on_groups = joins(:runner_namespaces).where(ci_runner_namespaces: { namespace_id: Ci::NamespaceHierarchy.belonging_to_namespace(groups).select(:namespace_xid) }) + runners_on_projects = belonging_to_project(Ci::ProjectHierarchy.belonging_to_namespace(groups).select(:project_xid)) union_sql = ::Gitlab::SQL::Union.new([runners_on_groups, runners_on_projects]).to_sql diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 3ffd377251cd50..a857b1e404e4a3 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -90,6 +90,7 @@ class Namespace < ApplicationRecord before_create :sync_share_with_group_lock_with_parent before_update :sync_share_with_group_lock_with_parent, if: :parent_changed? after_update :force_share_with_group_lock_on_descendants, if: -> { saved_change_to_share_with_group_lock? && share_with_group_lock? } + after_update :update_ci_namespace_hierarchy, if: :saved_change_to_parent_id? # Legacy Storage specific hooks @@ -569,6 +570,19 @@ def force_share_with_group_lock_on_descendants .update_all(share_with_group_lock: true) end + def update_ci_namespace_hierarchy + old_parent = Namespace.find(parent_id_previously_was) if parent_id_previously_was + + old_traversal_ids = traversal_ids_from_parent_namespace(old_parent) + new_traversal_ids = traversal_ids_from_parent_namespace(parent) + ::Ci::NamespaceHierarchy.update_traversal_ids(old_traversal_ids, new_traversal_ids) + ::Ci::ProjectHierarchy.update_namespace_traversal_ids(old_traversal_ids, new_traversal_ids) + end + + def traversal_ids_from_parent_namespace(namespace) + (namespace&.self_and_ancestor_ids(hierarchy_order: :desc) || []) + [id] + end + def write_projects_repository_config all_projects.find_each do |project| project.set_full_path diff --git a/app/models/project.rb b/app/models/project.rb index 126fcc8d7e6558..3289c6bf1bfb79 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -99,6 +99,8 @@ class Project < ApplicationRecord before_save :ensure_runners_token + after_update :update_ci_project_hierarchy, if: :saved_change_to_namespace_id? + after_save :update_project_statistics, if: :saved_change_to_namespace_id? after_save :create_import_state, if: ->(project) { project.import? && project.import_state.nil? } @@ -1719,6 +1721,16 @@ def update_forks_visibility_level end end + def update_ci_project_hierarchy + prev_namespace = Namespace.find_by(id: namespace_id_previously_was) + + ::Ci::ProjectHierarchy.update_project_traversal_ids( + project_id, + prev_namespace&.self_and_ancestor_ids || [namespace_id_previously_was], + namespace.self_and_ancestor_ids + ) + end + def allowed_to_share_with_group? !namespace.share_with_group_lock end diff --git a/spec/models/ci/namespace_hierarchy_spec.rb b/spec/models/ci/namespace_hierarchy_spec.rb index db6a051c512b7d..d67132d8fecd10 100644 --- a/spec/models/ci/namespace_hierarchy_spec.rb +++ b/spec/models/ci/namespace_hierarchy_spec.rb @@ -3,5 +3,122 @@ require 'spec_helper' RSpec.describe Ci::NamespaceHierarchy do - pending "add some examples to (or delete) #{__FILE__}" + describe '#belonging_to_namespace' do + subject { described_class.belonging_to_namespace(group) } + + let_it_be(:parent_group) { create(:group) } + let_it_be(:group) { create(:group, parent: parent_group) } + + context 'with no runners registered' do + it 'returns no namespace records' do + is_expected.to be_empty + end + end + + context 'with 2 runners registered on same group' do + let_it_be(:group_runner1) { create(:ci_runner, :group, groups: [group]) } + let_it_be(:group_runner2) { create(:ci_runner, :group, groups: [group]) } + + it 'returns one namespace record' do + is_expected.to all(be_a(described_class)) + is_expected.to match_array([have_attributes(namespace_xid: group.id, traversal_ids: [parent_group.id, group.id])]) + end + end + + context 'with 2 runners registered on different groups' do + let_it_be(:group2) { create(:group, parent: parent_group) } + let_it_be(:group_runner1) { create(:ci_runner, :group, groups: [group]) } + let_it_be(:group_runner2) { create(:ci_runner, :group, groups: [group2]) } + + it 'returns one namespace record' do + is_expected.to all(be_a(described_class)) + is_expected.to match_array([ + have_attributes(namespace_xid: group.id, traversal_ids: [parent_group.id, group.id]) + ]) + end + + specify 'group2 should have its own namespace record' do + group2_self_and_ancestors = described_class.belonging_to_namespace(group2) + + expect(group2_self_and_ancestors).to match_array([ + have_attributes(namespace_xid: group2.id, traversal_ids: [parent_group.id, group2.id]) + ]) + end + end + end + + describe 'hierarchy change triggers' do + let(:records_from_old_and_new_parents) do + described_class.from(< Date: Tue, 28 Sep 2021 18:15:45 +0200 Subject: [PATCH 6/6] Address project/group deletions --- app/models/ci/namespace_hierarchy.rb | 4 ++++ app/models/ci/project_hierarchy.rb | 4 ++++ app/models/namespace.rb | 5 +++++ app/models/project.rb | 5 +++++ spec/models/ci/namespace_hierarchy_spec.rb | 20 ++++++++++++++++++++ spec/models/ci/project_hierarchy_spec.rb | 20 ++++++++++++++++++++ 6 files changed, 58 insertions(+) diff --git a/app/models/ci/namespace_hierarchy.rb b/app/models/ci/namespace_hierarchy.rb index 2a105968ba7086..c48c7909055a00 100644 --- a/app/models/ci/namespace_hierarchy.rb +++ b/app/models/ci/namespace_hierarchy.rb @@ -15,6 +15,10 @@ def update_traversal_ids(old_self_and_ancestor_ids, new_self_and_ancestor_ids) where('traversal_ids @> ARRAY[?]::int[]', old_self_and_ancestor_ids) .update_all("traversal_ids = ARRAY#{new_self_and_ancestor_ids}::int[] || traversal_ids[#{old_self_and_ancestor_ids.length + 1}:]") end + + def delete_traversal_ids(namespace_id) + belonging_to_namespace(namespace_id).delete_all + end end end end diff --git a/app/models/ci/project_hierarchy.rb b/app/models/ci/project_hierarchy.rb index ba34c567269ee5..39e564db1b06b9 100644 --- a/app/models/ci/project_hierarchy.rb +++ b/app/models/ci/project_hierarchy.rb @@ -13,6 +13,10 @@ def update_project_traversal_ids(project_id, old_self_and_ancestor_ids, new_self update_all_traversal_ids(where(project_xid: project_id), old_self_and_ancestor_ids, new_self_and_ancestor_ids) end + def delete_project_traversal_ids(project_id) + where(project_xid: project_id).delete_all + end + def update_namespace_traversal_ids(old_self_and_ancestor_ids, new_self_and_ancestor_ids) update_all_traversal_ids(belonging_to_namespace(old_self_and_ancestor_ids), old_self_and_ancestor_ids, new_self_and_ancestor_ids) end diff --git a/app/models/namespace.rb b/app/models/namespace.rb index a857b1e404e4a3..eedad8e1d9e3a5 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -91,6 +91,7 @@ class Namespace < ApplicationRecord before_update :sync_share_with_group_lock_with_parent, if: :parent_changed? after_update :force_share_with_group_lock_on_descendants, if: -> { saved_change_to_share_with_group_lock? && share_with_group_lock? } after_update :update_ci_namespace_hierarchy, if: :saved_change_to_parent_id? + after_destroy :delete_ci_namespace_hierarchy # Legacy Storage specific hooks @@ -583,6 +584,10 @@ def traversal_ids_from_parent_namespace(namespace) (namespace&.self_and_ancestor_ids(hierarchy_order: :desc) || []) + [id] end + def delete_ci_namespace_hierarchy + ::Ci::NamespaceHierarchy.delete_traversal_ids(id) + end + def write_projects_repository_config all_projects.find_each do |project| project.set_full_path diff --git a/app/models/project.rb b/app/models/project.rb index 3289c6bf1bfb79..e45f0ff346ef4f 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -100,6 +100,7 @@ class Project < ApplicationRecord before_save :ensure_runners_token after_update :update_ci_project_hierarchy, if: :saved_change_to_namespace_id? + after_destroy :delete_ci_project_hierarchy after_save :update_project_statistics, if: :saved_change_to_namespace_id? @@ -1731,6 +1732,10 @@ def update_ci_project_hierarchy ) end + def delete_ci_project_hierarchy + ::Ci::ProjectHierarchy.delete_project_traversal_ids(project_id) + end + def allowed_to_share_with_group? !namespace.share_with_group_lock end diff --git a/spec/models/ci/namespace_hierarchy_spec.rb b/spec/models/ci/namespace_hierarchy_spec.rb index d67132d8fecd10..a7ac18e72cac98 100644 --- a/spec/models/ci/namespace_hierarchy_spec.rb +++ b/spec/models/ci/namespace_hierarchy_spec.rb @@ -119,6 +119,26 @@ ]) end end + + context 'when group is destroyed' do + let(:new_parent) { } + + it 'destroys the relevant namespace hierarchy record' do + group.destroy! + + expect(records_from_old_and_new_parents.reload).to be_empty + end + end + + context 'when group runner is destroyed' do + let(:new_parent) { old_parent } + + it 'destroys the relevant namespace hierarchy record' do + group_runner.runner_namespaces.destroy_all # rubocop: disable Cop/DestroyAll + + expect(records_from_old_and_new_parents.reload).to be_empty + end + end end end end diff --git a/spec/models/ci/project_hierarchy_spec.rb b/spec/models/ci/project_hierarchy_spec.rb index d370ce4afa12e0..7e0c9052a79b6f 100644 --- a/spec/models/ci/project_hierarchy_spec.rb +++ b/spec/models/ci/project_hierarchy_spec.rb @@ -85,6 +85,26 @@ ]) end end + + context 'when project is destroyed' do + let(:new_parent) { } + + it 'updates the namespace hierarchy to point to the root' do + project.destroy! + + expect(records_from_old_and_new_parents.reload).to be_empty + end + end + + context 'when project runner is destroyed' do + let(:new_parent) { old_parent } + + it 'destroys the relevant namespace hierarchy record' do + project_runner.destroy! + + expect(records_from_old_and_new_parents.reload).to be_empty + end + end end end end -- GitLab