From 8e37eae05f790132492c8c4002adfcd6d2c53cfb Mon Sep 17 00:00:00 2001 From: bmarjanovic Date: Tue, 12 Nov 2024 17:40:48 +0100 Subject: [PATCH 1/8] Add migration to delete orphaned groups --- .../delete_orphaned_groups.yml | 8 ++++ ...1112163029_queue_delete_orphaned_groups.rb | 26 +++++++++++++ db/schema_migrations/20241112163029 | 1 + .../delete_orphaned_groups.rb | 20 ++++++++++ .../delete_orphaned_groups_spec.rb | 39 +++++++++++++++++++ ...63029_queue_delete_orphaned_groups_spec.rb | 31 +++++++++++++++ 6 files changed, 125 insertions(+) create mode 100644 db/docs/batched_background_migrations/delete_orphaned_groups.yml create mode 100644 db/post_migrate/20241112163029_queue_delete_orphaned_groups.rb create mode 100644 db/schema_migrations/20241112163029 create mode 100644 lib/gitlab/background_migration/delete_orphaned_groups.rb create mode 100644 spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb create mode 100644 spec/migrations/20241112163029_queue_delete_orphaned_groups_spec.rb diff --git a/db/docs/batched_background_migrations/delete_orphaned_groups.yml b/db/docs/batched_background_migrations/delete_orphaned_groups.yml new file mode 100644 index 00000000000000..110eb8b30de677 --- /dev/null +++ b/db/docs/batched_background_migrations/delete_orphaned_groups.yml @@ -0,0 +1,8 @@ +--- +migration_job_name: DeleteOrphanedGroups +description: Deletes orhpaned groups whose parent's does not exist +feature_category: groups_and_projects +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/172420 +milestone: '17.6' +queued_migration_version: 20241112163029 +finalized_by: # version of the migration that finalized this BBM diff --git a/db/post_migrate/20241112163029_queue_delete_orphaned_groups.rb b/db/post_migrate/20241112163029_queue_delete_orphaned_groups.rb new file mode 100644 index 00000000000000..2ce89c6264f128 --- /dev/null +++ b/db/post_migrate/20241112163029_queue_delete_orphaned_groups.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +class QueueDeleteOrphanedGroups < Gitlab::Database::Migration[2.2] + milestone '17.6' + restrict_gitlab_migration gitlab_schema: :gitlab_main + + MIGRATION = "DeleteOrphanedGroups" + DELAY_INTERVAL = 2.minutes + BATCH_SIZE = 1000 + SUB_BATCH_SIZE = 100 + + def up + queue_batched_background_migration( + MIGRATION, + :namespaces, + :id, + job_interval: DELAY_INTERVAL, + batch_size: BATCH_SIZE, + sub_batch_size: SUB_BATCH_SIZE + ) + end + + def down + delete_batched_background_migration(MIGRATION, :namespaces, :id, []) + end +end diff --git a/db/schema_migrations/20241112163029 b/db/schema_migrations/20241112163029 new file mode 100644 index 00000000000000..f95805a988d7f6 --- /dev/null +++ b/db/schema_migrations/20241112163029 @@ -0,0 +1 @@ +ce1cea13a912cd65923aea1757bc0c073e425d20ac6e7bae620f66cee518bdb7 \ No newline at end of file diff --git a/lib/gitlab/background_migration/delete_orphaned_groups.rb b/lib/gitlab/background_migration/delete_orphaned_groups.rb new file mode 100644 index 00000000000000..7836fe55320e85 --- /dev/null +++ b/lib/gitlab/background_migration/delete_orphaned_groups.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +module Gitlab + module BackgroundMigration + class DeleteOrphanedGroups < BatchedMigrationJob + operation_name :delete_all + feature_category :groups_and_projects + + scope_to ->(relation) do + relation.where.not(parent_id: nil) + .joins("LEFT JOIN namespaces AS parent ON namespaces.parent_id = parent.id") + .where(parent: { id: nil }) + end + + def perform + each_sub_batch(&:delete_all) + end + end + end +end diff --git a/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb b/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb new file mode 100644 index 00000000000000..6391c43ba46a1e --- /dev/null +++ b/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::BackgroundMigration::DeleteOrphanedGroups, feature_category: :groups_and_projects do + let(:namespaces) { table(:namespaces) } + let!(:parent) { namespaces.create!(name: 'Group1', type: 'Group', path: 'space1') } + + subject(:background_migration) do + described_class.new( + start_id: namespaces.without(parent).minimum(:id), + end_id: namespaces.maximum(:id), + batch_table: :namespaces, + batch_column: :id, + sub_batch_size: 1, + pause_ms: 0, + connection: ApplicationRecord.connection + ).perform + end + + describe '#perform' do + it 'destroys namespaces whose parent\'s do not exist' do + # To keep the test database's schema valid, the dropping will be performing inside a transaction. + ApplicationRecord.connection.transaction do + ApplicationRecord.connection.execute("ALTER TABLE namespaces DROP CONSTRAINT fk_7f813d8c90;") + namespaces.create!(name: 'project_1', path: 'project_1', type: 'Project', parent_id: parent.id) + namespaces.create!(name: 'project_2', path: 'project_2', type: 'Project', parent_id: parent.id) + namespaces.create!(name: 'project_3', path: 'project_3', type: 'Project', parent_id: parent.id) + namespaces.create!(name: 'project_4', path: 'project_4', type: 'Project', parent_id: parent.id) + + parent.destroy! + + expect { background_migration }.to change { namespaces.count }.from(4).to(0) + + ApplicationRecord.connection.execute("ABORT") + end + end + end +end diff --git a/spec/migrations/20241112163029_queue_delete_orphaned_groups_spec.rb b/spec/migrations/20241112163029_queue_delete_orphaned_groups_spec.rb new file mode 100644 index 00000000000000..38f262a5817b88 --- /dev/null +++ b/spec/migrations/20241112163029_queue_delete_orphaned_groups_spec.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +require 'spec_helper' +require_migration! + +RSpec.describe QueueDeleteOrphanedGroups, migration: :gitlab_main, feature_category: :groups_and_projects do + let!(:migration) { described_class::MIGRATION } + + describe '#up' do + it 'schedules background migration' do + migrate! + + expect(migration).to have_scheduled_batched_migration( + table_name: :namespaces, + column_name: :id, + interval: described_class::DELAY_INTERVAL, + batch_size: described_class::BATCH_SIZE, + sub_batch_size: described_class::SUB_BATCH_SIZE + ) + end + end + + describe '#down' do + it 'removes scheduled background migrations' do + migrate! + schema_migrate_down! + + expect(migration).not_to have_scheduled_batched_migration + end + end +end -- GitLab From 06654dae0c46c8223253925e1b4800fa05323bca Mon Sep 17 00:00:00 2001 From: bmarjanovic Date: Mon, 18 Nov 2024 17:20:11 +0100 Subject: [PATCH 2/8] Update background migration --- .../delete_orphaned_groups.yml | 2 +- .../20241112163029_queue_delete_orphaned_groups.rb | 4 +++- .../background_migration/delete_orphaned_groups.rb | 4 ++-- .../delete_orphaned_groups_spec.rb | 10 +++++----- 4 files changed, 11 insertions(+), 9 deletions(-) diff --git a/db/docs/batched_background_migrations/delete_orphaned_groups.yml b/db/docs/batched_background_migrations/delete_orphaned_groups.yml index 110eb8b30de677..c8d84dc1c66b7b 100644 --- a/db/docs/batched_background_migrations/delete_orphaned_groups.yml +++ b/db/docs/batched_background_migrations/delete_orphaned_groups.yml @@ -3,6 +3,6 @@ migration_job_name: DeleteOrphanedGroups description: Deletes orhpaned groups whose parent's does not exist feature_category: groups_and_projects introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/172420 -milestone: '17.6' +milestone: '17.7' queued_migration_version: 20241112163029 finalized_by: # version of the migration that finalized this BBM diff --git a/db/post_migrate/20241112163029_queue_delete_orphaned_groups.rb b/db/post_migrate/20241112163029_queue_delete_orphaned_groups.rb index 2ce89c6264f128..4e2d7d702b40ec 100644 --- a/db/post_migrate/20241112163029_queue_delete_orphaned_groups.rb +++ b/db/post_migrate/20241112163029_queue_delete_orphaned_groups.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class QueueDeleteOrphanedGroups < Gitlab::Database::Migration[2.2] - milestone '17.6' + milestone '17.7' restrict_gitlab_migration gitlab_schema: :gitlab_main MIGRATION = "DeleteOrphanedGroups" @@ -10,6 +10,8 @@ class QueueDeleteOrphanedGroups < Gitlab::Database::Migration[2.2] SUB_BATCH_SIZE = 100 def up + return unless Gitlab.com_except_jh? && !Gitlab.staging? + queue_batched_background_migration( MIGRATION, :namespaces, diff --git a/lib/gitlab/background_migration/delete_orphaned_groups.rb b/lib/gitlab/background_migration/delete_orphaned_groups.rb index 7836fe55320e85..94600d43c0d77b 100644 --- a/lib/gitlab/background_migration/delete_orphaned_groups.rb +++ b/lib/gitlab/background_migration/delete_orphaned_groups.rb @@ -3,11 +3,11 @@ module Gitlab module BackgroundMigration class DeleteOrphanedGroups < BatchedMigrationJob - operation_name :delete_all + operation_name :delete_orphaned_group_records feature_category :groups_and_projects scope_to ->(relation) do - relation.where.not(parent_id: nil) + relation.where(type: 'Group').where.not(parent_id: nil) .joins("LEFT JOIN namespaces AS parent ON namespaces.parent_id = parent.id") .where(parent: { id: nil }) end diff --git a/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb b/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb index 6391c43ba46a1e..21e140b8b6148c 100644 --- a/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb +++ b/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb @@ -4,7 +4,7 @@ RSpec.describe Gitlab::BackgroundMigration::DeleteOrphanedGroups, feature_category: :groups_and_projects do let(:namespaces) { table(:namespaces) } - let!(:parent) { namespaces.create!(name: 'Group1', type: 'Group', path: 'space1') } + let!(:parent) { namespaces.create!(name: 'Group', type: 'Group', path: 'space1') } subject(:background_migration) do described_class.new( @@ -23,10 +23,10 @@ # To keep the test database's schema valid, the dropping will be performing inside a transaction. ApplicationRecord.connection.transaction do ApplicationRecord.connection.execute("ALTER TABLE namespaces DROP CONSTRAINT fk_7f813d8c90;") - namespaces.create!(name: 'project_1', path: 'project_1', type: 'Project', parent_id: parent.id) - namespaces.create!(name: 'project_2', path: 'project_2', type: 'Project', parent_id: parent.id) - namespaces.create!(name: 'project_3', path: 'project_3', type: 'Project', parent_id: parent.id) - namespaces.create!(name: 'project_4', path: 'project_4', type: 'Project', parent_id: parent.id) + namespaces.create!(name: 'Group 1', path: 'group_1', type: 'Group', parent_id: parent.id) + namespaces.create!(name: 'Group 2', path: 'group_2', type: 'Group', parent_id: parent.id) + namespaces.create!(name: 'Group 3', path: 'group_3', type: 'Group', parent_id: parent.id) + namespaces.create!(name: 'Group 4', path: 'group_4', type: 'Group', parent_id: parent.id) parent.destroy! -- GitLab From 74dcf7a9c6ad488bac031a133e05e79d8e35fd5b Mon Sep 17 00:00:00 2001 From: bmarjanovic Date: Tue, 19 Nov 2024 12:57:30 +0100 Subject: [PATCH 3/8] Fixes failing specs --- .../delete_orphaned_groups.rb | 13 ++--- ...63029_queue_delete_orphaned_groups_spec.rb | 54 ++++++++++++------- 2 files changed, 43 insertions(+), 24 deletions(-) diff --git a/lib/gitlab/background_migration/delete_orphaned_groups.rb b/lib/gitlab/background_migration/delete_orphaned_groups.rb index 94600d43c0d77b..75d74f41329f06 100644 --- a/lib/gitlab/background_migration/delete_orphaned_groups.rb +++ b/lib/gitlab/background_migration/delete_orphaned_groups.rb @@ -6,14 +6,15 @@ class DeleteOrphanedGroups < BatchedMigrationJob operation_name :delete_orphaned_group_records feature_category :groups_and_projects - scope_to ->(relation) do - relation.where(type: 'Group').where.not(parent_id: nil) - .joins("LEFT JOIN namespaces AS parent ON namespaces.parent_id = parent.id") - .where(parent: { id: nil }) - end + scope_to ->(relation) { relation.where(type: 'Group') } def perform - each_sub_batch(&:delete_all) + each_sub_batch do |sub_batch| + sub_batch.where.not(parent_id: nil) + .joins("LEFT JOIN namespaces AS parent ON namespaces.parent_id = parent.id") + .where(parent: { id: nil }) + .delete_all + end end end end diff --git a/spec/migrations/20241112163029_queue_delete_orphaned_groups_spec.rb b/spec/migrations/20241112163029_queue_delete_orphaned_groups_spec.rb index 38f262a5817b88..d4e514b53dd2d9 100644 --- a/spec/migrations/20241112163029_queue_delete_orphaned_groups_spec.rb +++ b/spec/migrations/20241112163029_queue_delete_orphaned_groups_spec.rb @@ -4,28 +4,46 @@ require_migration! RSpec.describe QueueDeleteOrphanedGroups, migration: :gitlab_main, feature_category: :groups_and_projects do - let!(:migration) { described_class::MIGRATION } - - describe '#up' do - it 'schedules background migration' do - migrate! - - expect(migration).to have_scheduled_batched_migration( - table_name: :namespaces, - column_name: :id, - interval: described_class::DELAY_INTERVAL, - batch_size: described_class::BATCH_SIZE, - sub_batch_size: described_class::SUB_BATCH_SIZE - ) + let!(:batched_migration) { described_class::MIGRATION } + + it 'does not schedule a new batched migration' do + reversible_migration do |migration| + migration.before -> { + expect(batched_migration).not_to have_scheduled_batched_migration + } + + migration.after -> { + expect(batched_migration).not_to have_scheduled_batched_migration + } end end - describe '#down' do - it 'removes scheduled background migrations' do - migrate! - schema_migrate_down! + context 'when executed on .com' do + before do + allow(Gitlab).to receive(:com_except_jh?).and_return(true) + end + + describe '#up' do + it 'schedules background migration' do + migrate! + + expect(batched_migration).to have_scheduled_batched_migration( + table_name: :namespaces, + column_name: :id, + interval: described_class::DELAY_INTERVAL, + batch_size: described_class::BATCH_SIZE, + sub_batch_size: described_class::SUB_BATCH_SIZE + ) + end + end + + describe '#down' do + it 'removes scheduled background migrations' do + migrate! + schema_migrate_down! - expect(migration).not_to have_scheduled_batched_migration + expect(batched_migration).not_to have_scheduled_batched_migration + end end end end -- GitLab From 7c9161567fc1d46f5840e369a6568d13c0902a5b Mon Sep 17 00:00:00 2001 From: bmarjanovic Date: Tue, 19 Nov 2024 13:24:20 +0100 Subject: [PATCH 4/8] Add usage, and fix tests --- .../delete_orphaned_groups.rb | 10 ++++++- .../delete_orphaned_groups_spec.rb | 29 +++++++++++-------- 2 files changed, 26 insertions(+), 13 deletions(-) diff --git a/lib/gitlab/background_migration/delete_orphaned_groups.rb b/lib/gitlab/background_migration/delete_orphaned_groups.rb index 75d74f41329f06..8d8ae674c86db9 100644 --- a/lib/gitlab/background_migration/delete_orphaned_groups.rb +++ b/lib/gitlab/background_migration/delete_orphaned_groups.rb @@ -13,9 +13,17 @@ def perform sub_batch.where.not(parent_id: nil) .joins("LEFT JOIN namespaces AS parent ON namespaces.parent_id = parent.id") .where(parent: { id: nil }) - .delete_all + .pluck(:id).each do |orphaned_group_id| + ::GroupDestroyWorker.perform_async(orphaned_group_id, admin_bot.id) + end end end + + private + + def admin_bot + @_admin_bot ||= Users::Internal.admin_bot + end end end end diff --git a/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb b/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb index 21e140b8b6148c..b84b1414884e67 100644 --- a/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb +++ b/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb @@ -19,21 +19,26 @@ end describe '#perform' do - it 'destroys namespaces whose parent\'s do not exist' do - # To keep the test database's schema valid, the dropping will be performing inside a transaction. - ApplicationRecord.connection.transaction do - ApplicationRecord.connection.execute("ALTER TABLE namespaces DROP CONSTRAINT fk_7f813d8c90;") - namespaces.create!(name: 'Group 1', path: 'group_1', type: 'Group', parent_id: parent.id) - namespaces.create!(name: 'Group 2', path: 'group_2', type: 'Group', parent_id: parent.id) - namespaces.create!(name: 'Group 3', path: 'group_3', type: 'Group', parent_id: parent.id) - namespaces.create!(name: 'Group 4', path: 'group_4', type: 'Group', parent_id: parent.id) + it 'enqueues ::GroupDestroyWorker for each group whose parent\'s do not exist' do + # Remove constraint so we can create invalid records + ApplicationRecord.connection.execute("ALTER TABLE namespaces DROP CONSTRAINT fk_7f813d8c90;") + group_1 = namespaces.create!(name: 'Group 1', path: 'group_1', type: 'Group', parent_id: parent.id) + group_2 = namespaces.create!(name: 'Group 2', path: 'group_2', type: 'Group', parent_id: parent.id) + group_3 = namespaces.create!(name: 'Group 3', path: 'group_3', type: 'Group', parent_id: parent.id) + group_4 = namespaces.create!(name: 'Group 4', path: 'group_4', type: 'Group', parent_id: parent.id) - parent.destroy! + parent.destroy! - expect { background_migration }.to change { namespaces.count }.from(4).to(0) + # Re-create constraint + ApplicationRecord.connection.execute("ALTER TABLE ONLY namespaces ADD CONSTRAINT fk_7f813d8c90 + FOREIGN KEY (parent_id) REFERENCES namespaces(id) ON DELETE RESTRICT NOT VALID;") - ApplicationRecord.connection.execute("ABORT") - end + expect(::GroupDestroyWorker).to receive(:perform_async).with(group_1.id, ::Users::Internal.admin_bot.id) + expect(::GroupDestroyWorker).to receive(:perform_async).with(group_2.id, ::Users::Internal.admin_bot.id) + expect(::GroupDestroyWorker).to receive(:perform_async).with(group_3.id, ::Users::Internal.admin_bot.id) + expect(::GroupDestroyWorker).to receive(:perform_async).with(group_4.id, ::Users::Internal.admin_bot.id) + + background_migration end end end -- GitLab From 47b9baef6f4643bd7c4ba79b8a319da54c197077 Mon Sep 17 00:00:00 2001 From: bmarjanovic Date: Wed, 20 Nov 2024 12:39:37 +0100 Subject: [PATCH 5/8] Apply revievers feedback --- lib/gitlab/background_migration/delete_orphaned_groups.rb | 6 +++--- .../background_migration/delete_orphaned_groups_spec.rb | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/gitlab/background_migration/delete_orphaned_groups.rb b/lib/gitlab/background_migration/delete_orphaned_groups.rb index 8d8ae674c86db9..a032ad40180b68 100644 --- a/lib/gitlab/background_migration/delete_orphaned_groups.rb +++ b/lib/gitlab/background_migration/delete_orphaned_groups.rb @@ -6,15 +6,15 @@ class DeleteOrphanedGroups < BatchedMigrationJob operation_name :delete_orphaned_group_records feature_category :groups_and_projects - scope_to ->(relation) { relation.where(type: 'Group') } + scope_to ->(relation) { relation.where(type: 'Group').where.not(parent_id: nil) } def perform each_sub_batch do |sub_batch| - sub_batch.where.not(parent_id: nil) + sub_batch .joins("LEFT JOIN namespaces AS parent ON namespaces.parent_id = parent.id") .where(parent: { id: nil }) .pluck(:id).each do |orphaned_group_id| - ::GroupDestroyWorker.perform_async(orphaned_group_id, admin_bot.id) + ::GroupDestroyWorker.perform(orphaned_group_id, admin_bot.id) end end end diff --git a/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb b/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb index b84b1414884e67..31dd1f6cfd7bd4 100644 --- a/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb +++ b/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb @@ -33,10 +33,10 @@ ApplicationRecord.connection.execute("ALTER TABLE ONLY namespaces ADD CONSTRAINT fk_7f813d8c90 FOREIGN KEY (parent_id) REFERENCES namespaces(id) ON DELETE RESTRICT NOT VALID;") - expect(::GroupDestroyWorker).to receive(:perform_async).with(group_1.id, ::Users::Internal.admin_bot.id) - expect(::GroupDestroyWorker).to receive(:perform_async).with(group_2.id, ::Users::Internal.admin_bot.id) - expect(::GroupDestroyWorker).to receive(:perform_async).with(group_3.id, ::Users::Internal.admin_bot.id) - expect(::GroupDestroyWorker).to receive(:perform_async).with(group_4.id, ::Users::Internal.admin_bot.id) + expect(::GroupDestroyWorker).to receive(:perform).with(group_1.id, ::Users::Internal.admin_bot.id) + expect(::GroupDestroyWorker).to receive(:perform).with(group_2.id, ::Users::Internal.admin_bot.id) + expect(::GroupDestroyWorker).to receive(:perform).with(group_3.id, ::Users::Internal.admin_bot.id) + expect(::GroupDestroyWorker).to receive(:perform).with(group_4.id, ::Users::Internal.admin_bot.id) background_migration end -- GitLab From b8c838936979137a49c3348d862f2aa95fe465b7 Mon Sep 17 00:00:00 2001 From: bmarjanovic Date: Thu, 21 Nov 2024 13:11:31 +0100 Subject: [PATCH 6/8] Include project namespaces in background migration --- .../delete_orphaned_groups.rb | 11 ++++-- .../delete_orphaned_groups_spec.rb | 39 ++++++++++++------- 2 files changed, 34 insertions(+), 16 deletions(-) diff --git a/lib/gitlab/background_migration/delete_orphaned_groups.rb b/lib/gitlab/background_migration/delete_orphaned_groups.rb index a032ad40180b68..5ccc2ff7202869 100644 --- a/lib/gitlab/background_migration/delete_orphaned_groups.rb +++ b/lib/gitlab/background_migration/delete_orphaned_groups.rb @@ -6,15 +6,20 @@ class DeleteOrphanedGroups < BatchedMigrationJob operation_name :delete_orphaned_group_records feature_category :groups_and_projects - scope_to ->(relation) { relation.where(type: 'Group').where.not(parent_id: nil) } + scope_to ->(relation) { relation.where.not(parent_id: nil) } def perform each_sub_batch do |sub_batch| sub_batch .joins("LEFT JOIN namespaces AS parent ON namespaces.parent_id = parent.id") .where(parent: { id: nil }) - .pluck(:id).each do |orphaned_group_id| - ::GroupDestroyWorker.perform(orphaned_group_id, admin_bot.id) + .pluck(:id, :type).each do |orphaned_group_id, type| + case type + when ::Group.sti_name + ::GroupDestroyWorker.perform(orphaned_group_id, admin_bot.id) + when Namespaces::ProjectNamespace.sti_name + ::Namespaces::ProjectNamespace.delete(orphaned_group_id) + end end end end diff --git a/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb b/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb index 31dd1f6cfd7bd4..1e5108493d5cc5 100644 --- a/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb +++ b/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb @@ -19,24 +19,37 @@ end describe '#perform' do - it 'enqueues ::GroupDestroyWorker for each group whose parent\'s do not exist' do - # Remove constraint so we can create invalid records + before do + # Remove constraint to allow creation of invalid records ApplicationRecord.connection.execute("ALTER TABLE namespaces DROP CONSTRAINT fk_7f813d8c90;") - group_1 = namespaces.create!(name: 'Group 1', path: 'group_1', type: 'Group', parent_id: parent.id) - group_2 = namespaces.create!(name: 'Group 2', path: 'group_2', type: 'Group', parent_id: parent.id) - group_3 = namespaces.create!(name: 'Group 3', path: 'group_3', type: 'Group', parent_id: parent.id) - group_4 = namespaces.create!(name: 'Group 4', path: 'group_4', type: 'Group', parent_id: parent.id) + end + + after do + # Re-create constraint after the test + ApplicationRecord.connection.execute(<<~SQL) + ALTER TABLE ONLY namespaces ADD CONSTRAINT fk_7f813d8c90 + FOREIGN KEY (parent_id) REFERENCES namespaces(id) ON DELETE RESTRICT NOT VALID; + SQL + end + + it 'processes orphaned groups and projects correctly' do + orphaned_groups = (1..4).map do |i| + namespaces.create!(name: "Group #{i}", path: "group_#{i}", type: 'Group', parent_id: parent.id) + end + + orphaned_projects = (1..4).map do |i| + namespaces.create!(name: "Project #{i}", path: "project_#{i}", type: 'Project', parent_id: parent.id) + end parent.destroy! - # Re-create constraint - ApplicationRecord.connection.execute("ALTER TABLE ONLY namespaces ADD CONSTRAINT fk_7f813d8c90 - FOREIGN KEY (parent_id) REFERENCES namespaces(id) ON DELETE RESTRICT NOT VALID;") + orphaned_groups.each do |group| + expect(::GroupDestroyWorker).to receive(:perform).with(group.id, ::Users::Internal.admin_bot.id) + end - expect(::GroupDestroyWorker).to receive(:perform).with(group_1.id, ::Users::Internal.admin_bot.id) - expect(::GroupDestroyWorker).to receive(:perform).with(group_2.id, ::Users::Internal.admin_bot.id) - expect(::GroupDestroyWorker).to receive(:perform).with(group_3.id, ::Users::Internal.admin_bot.id) - expect(::GroupDestroyWorker).to receive(:perform).with(group_4.id, ::Users::Internal.admin_bot.id) + orphaned_projects.each do |project| + expect(::Namespaces::ProjectNamespace).to receive(:delete).with(project.id) + end background_migration end -- GitLab From 6bf43caa1ab0b1b37eab980ec20b7dc4af14678b Mon Sep 17 00:00:00 2001 From: bmarjanovic Date: Mon, 25 Nov 2024 16:00:58 +0100 Subject: [PATCH 7/8] Fixes orphaned groups migrations --- .../background_migration/delete_orphaned_groups.rb | 11 +++-------- .../delete_orphaned_groups_spec.rb | 13 ++----------- 2 files changed, 5 insertions(+), 19 deletions(-) diff --git a/lib/gitlab/background_migration/delete_orphaned_groups.rb b/lib/gitlab/background_migration/delete_orphaned_groups.rb index 5ccc2ff7202869..a032ad40180b68 100644 --- a/lib/gitlab/background_migration/delete_orphaned_groups.rb +++ b/lib/gitlab/background_migration/delete_orphaned_groups.rb @@ -6,20 +6,15 @@ class DeleteOrphanedGroups < BatchedMigrationJob operation_name :delete_orphaned_group_records feature_category :groups_and_projects - scope_to ->(relation) { relation.where.not(parent_id: nil) } + scope_to ->(relation) { relation.where(type: 'Group').where.not(parent_id: nil) } def perform each_sub_batch do |sub_batch| sub_batch .joins("LEFT JOIN namespaces AS parent ON namespaces.parent_id = parent.id") .where(parent: { id: nil }) - .pluck(:id, :type).each do |orphaned_group_id, type| - case type - when ::Group.sti_name - ::GroupDestroyWorker.perform(orphaned_group_id, admin_bot.id) - when Namespaces::ProjectNamespace.sti_name - ::Namespaces::ProjectNamespace.delete(orphaned_group_id) - end + .pluck(:id).each do |orphaned_group_id| + ::GroupDestroyWorker.perform(orphaned_group_id, admin_bot.id) end end end diff --git a/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb b/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb index 1e5108493d5cc5..86b8405712d77c 100644 --- a/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb +++ b/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb @@ -20,7 +20,7 @@ describe '#perform' do before do - # Remove constraint to allow creation of invalid records + # Remove constraint so we can create invalid records ApplicationRecord.connection.execute("ALTER TABLE namespaces DROP CONSTRAINT fk_7f813d8c90;") end @@ -32,25 +32,16 @@ SQL end - it 'processes orphaned groups and projects correctly' do + it 'enqueues ::GroupDestroyWorker for each group whose parent\'s do not exist' do orphaned_groups = (1..4).map do |i| namespaces.create!(name: "Group #{i}", path: "group_#{i}", type: 'Group', parent_id: parent.id) end - - orphaned_projects = (1..4).map do |i| - namespaces.create!(name: "Project #{i}", path: "project_#{i}", type: 'Project', parent_id: parent.id) - end - parent.destroy! orphaned_groups.each do |group| expect(::GroupDestroyWorker).to receive(:perform).with(group.id, ::Users::Internal.admin_bot.id) end - orphaned_projects.each do |project| - expect(::Namespaces::ProjectNamespace).to receive(:delete).with(project.id) - end - background_migration end end -- GitLab From 9300d2582c069cccb8fb2b388f808710aa1af223 Mon Sep 17 00:00:00 2001 From: bmarjanovic Date: Wed, 27 Nov 2024 09:14:14 +0100 Subject: [PATCH 8/8] Adds missing spec --- .../delete_orphaned_groups_spec.rb | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb b/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb index 86b8405712d77c..ea6536b1959fad 100644 --- a/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb +++ b/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb @@ -5,6 +5,7 @@ RSpec.describe Gitlab::BackgroundMigration::DeleteOrphanedGroups, feature_category: :groups_and_projects do let(:namespaces) { table(:namespaces) } let!(:parent) { namespaces.create!(name: 'Group', type: 'Group', path: 'space1') } + let!(:group) { namespaces.create!(name: 'GitLab', type: 'Group', path: 'group1') } subject(:background_migration) do described_class.new( @@ -34,7 +35,10 @@ it 'enqueues ::GroupDestroyWorker for each group whose parent\'s do not exist' do orphaned_groups = (1..4).map do |i| - namespaces.create!(name: "Group #{i}", path: "group_#{i}", type: 'Group', parent_id: parent.id) + namespaces.create!(name: "Group #{i}", path: "orphaned_group_#{i}", type: 'Group', parent_id: parent.id) + end + groups = (1..4).map do |i| + namespaces.create!(name: "Group #{i}", path: "group_#{i}", type: 'Group', parent_id: group.id) end parent.destroy! @@ -42,6 +46,10 @@ expect(::GroupDestroyWorker).to receive(:perform).with(group.id, ::Users::Internal.admin_bot.id) end + groups.each do |group| + expect(::GroupDestroyWorker).not_to receive(:perform).with(group.id, ::Users::Internal.admin_bot.id) + end + background_migration end end -- GitLab