diff --git a/app/models/namespaces/traversal/linear.rb b/app/models/namespaces/traversal/linear.rb index 394e01843fdd86d7d652c9fe5940060f66a65d75..19daac030ac1259045ca9bcf80e6807c96627f74 100644 --- a/app/models/namespaces/traversal/linear.rb +++ b/app/models/namespaces/traversal/linear.rb @@ -233,7 +233,8 @@ def set_traversal_ids return if is_a?(Namespaces::ProjectNamespace) self.transient_traversal_ids = if parent_id - parent.traversal_ids + [id] + # remove safe navigation and `.to_a` with https://gitlab.com/gitlab-org/gitlab/-/issues/508611 + parent&.traversal_ids.to_a + [id] else [id] end diff --git a/app/services/groups/destroy_service.rb b/app/services/groups/destroy_service.rb index e868d2a2fa97fb1141705af64ed33eb4f5825895..dc6f49eb0009539f1a5a709f8232a9a677cd2a74 100644 --- a/app/services/groups/destroy_service.rb +++ b/app/services/groups/destroy_service.rb @@ -118,7 +118,7 @@ def publish_event event = Groups::GroupDeletedEvent.new( data: { group_id: group.id, - root_namespace_id: group.root_ancestor.id + root_namespace_id: group.root_ancestor&.id.to_i # remove safe navigation and `.to_i` with https://gitlab.com/gitlab-org/gitlab/-/issues/508611 } ) diff --git a/db/docs/batched_background_migrations/delete_orphaned_groups.yml b/db/docs/batched_background_migrations/delete_orphaned_groups.yml index c8d84dc1c66b7b5fdca38b7eeb6184eac5480522..b91d5698f15b7a18cc7a48a4e3d3d4fddf525567 100644 --- a/db/docs/batched_background_migrations/delete_orphaned_groups.yml +++ b/db/docs/batched_background_migrations/delete_orphaned_groups.yml @@ -4,5 +4,5 @@ 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.7' -queued_migration_version: 20241112163029 +queued_migration_version: 20241206154945 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 4e2d7d702b40ecc9a9fa9662d21de8b5a37ea5c6..1f42797685a5ea7030b41e78a6c3abf4ba1718d6 100644 --- a/db/post_migrate/20241112163029_queue_delete_orphaned_groups.rb +++ b/db/post_migrate/20241112163029_queue_delete_orphaned_groups.rb @@ -10,19 +10,12 @@ 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, - :id, - job_interval: DELAY_INTERVAL, - batch_size: BATCH_SIZE, - sub_batch_size: SUB_BATCH_SIZE - ) + # no-op because there was a bug in the original migration, which has been + # fixed by https://gitlab.com/gitlab-org/gitlab/-/merge_requests/174995 end def down - delete_batched_background_migration(MIGRATION, :namespaces, :id, []) + # no-op because there was a bug in the original migration, which has been + # fixed in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/174995 end end diff --git a/db/post_migrate/20241206154945_queue_requeue_delete_orphaned_groups.rb b/db/post_migrate/20241206154945_queue_requeue_delete_orphaned_groups.rb new file mode 100644 index 0000000000000000000000000000000000000000..1646b0cf7a905366f17068a6c5504a7583363ed8 --- /dev/null +++ b/db/post_migrate/20241206154945_queue_requeue_delete_orphaned_groups.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +class QueueRequeueDeleteOrphanedGroups < Gitlab::Database::Migration[2.2] + milestone '17.7' + restrict_gitlab_migration gitlab_schema: :gitlab_main + + MIGRATION = "DeleteOrphanedGroups" + DELAY_INTERVAL = 2.minutes + BATCH_SIZE = 1000 + SUB_BATCH_SIZE = 100 + + def up + return unless Gitlab.com_except_jh? && !Gitlab.staging? + + # Clear previous background migration execution from QueueDeleteOrphanedGroups + delete_batched_background_migration(MIGRATION, :namespaces, :id, []) + + 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/20241206154945 b/db/schema_migrations/20241206154945 new file mode 100644 index 0000000000000000000000000000000000000000..698f5d6872e140d42f4fa2752bf3a5aba0f524ea --- /dev/null +++ b/db/schema_migrations/20241206154945 @@ -0,0 +1 @@ +276b46068ee45bbafbe5c51cf466cf2cee0ec0c08d273899ccba3b5e05c93f1d \ No newline at end of file diff --git a/ee/app/models/concerns/elastic/maintain_elasticsearch_on_group_update.rb b/ee/app/models/concerns/elastic/maintain_elasticsearch_on_group_update.rb index 854f938bdab8382ae38412ff8597debdfb868dfb..18015f0ccb006adbc722e2b96ba593387c24a944 100644 --- a/ee/app/models/concerns/elastic/maintain_elasticsearch_on_group_update.rb +++ b/ee/app/models/concerns/elastic/maintain_elasticsearch_on_group_update.rb @@ -25,7 +25,8 @@ def maintain_group_associations_on_update def maintain_group_associations_on_destroy delete_group_wiki_in_elastic if use_elasticsearch? - delete_group_associations + # remove if condition with https://gitlab.com/gitlab-org/gitlab/-/issues/508611 + delete_group_associations if root_ancestor end def sync_group_wiki_in_elastic diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb index cb2590e3495da73cd77e0c83c5f86975b1a89d15..da2bc6783b5732f77a82439dcbf0b404079351dd 100644 --- a/ee/app/models/ee/group.rb +++ b/ee/app/models/ee/group.rb @@ -1155,7 +1155,7 @@ def execute_subgroup_hooks(event) # When this subgroup is removed, there is no point in this subgroup's webhook itself being notified # that `self` was removed. Rather, we should only care about notifying its ancestors # and hence we need to trigger the hooks starting only from its `parent` group. - parent.execute_hooks(data, :subgroup_hooks) + parent&.execute_hooks(data, :subgroup_hooks) # remove safe navigation with https://gitlab.com/gitlab-org/gitlab/-/issues/508611 end end diff --git a/lib/gitlab/background_migration/delete_orphaned_groups.rb b/lib/gitlab/background_migration/delete_orphaned_groups.rb index a032ad40180b684cc93867480887a760a26df342..428867ad400257d6a91f48df8eba301b6b5f0e42 100644 --- a/lib/gitlab/background_migration/delete_orphaned_groups.rb +++ b/lib/gitlab/background_migration/delete_orphaned_groups.rb @@ -14,7 +14,7 @@ def perform .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) + ::GroupDestroyWorker.perform_async(orphaned_group_id, admin_bot.id) end end end diff --git a/lib/gitlab/hook_data/subgroup_builder.rb b/lib/gitlab/hook_data/subgroup_builder.rb index a620219675a4af98221ecd5fa82f9e0981d0ccde..5ce4151f0b7da882422f94ff6265e3782b13e9c0 100644 --- a/lib/gitlab/hook_data/subgroup_builder.rb +++ b/lib/gitlab/hook_data/subgroup_builder.rb @@ -34,6 +34,8 @@ def event_data(event) def group_data parent = group.parent + return super unless parent + super.merge( parent_group_id: parent.id, parent_name: parent.name, 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 f94d9256352a71ebf2806ab89690155cbf5c093a..717ebb1546a7b3351c0ebf0c842bed569cf4b741 100644 --- a/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb +++ b/spec/lib/gitlab/background_migration/delete_orphaned_groups_spec.rb @@ -3,10 +3,15 @@ require 'spec_helper' RSpec.describe Gitlab::BackgroundMigration::DeleteOrphanedGroups, feature_category: :groups_and_projects do - let(:organization) { table(:organizations).create!(name: 'organization', path: 'organization') } let(:namespaces) { table(:namespaces) } - let!(:parent) { namespaces.create!(name: 'Group', type: 'Group', path: 'space1', organization_id: organization.id) } - let!(:group) { namespaces.create!(name: 'GitLab', type: 'Group', path: 'group1', organization_id: organization.id) } + let!(:parent) { namespaces.create!(name: 'Group', type: 'Group', path: 'space1') } + let!(:group) { namespaces.create!(name: 'GitLab', type: 'Group', path: 'group1') } + let!(:admin_bot) { ::Users::Internal.admin_bot } + let!(:orphaned_groups) do + (1..4).map do |i| + namespaces.create!(name: "Group #{i}", path: "orphaned_group_#{i}", type: 'Group', parent_id: parent.id) + end + end subject(:background_migration) do described_class.new( @@ -24,6 +29,7 @@ before do # Remove constraint so we can create invalid records ApplicationRecord.connection.execute("ALTER TABLE namespaces DROP CONSTRAINT fk_7f813d8c90;") + (1..4).map { |i| namespaces.create!(name: "Group #{i}", path: "group_#{i}", type: 'Group', parent_id: group.id) } end after do @@ -34,26 +40,11 @@ SQL end - 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: "orphaned_group_#{i}", type: 'Group', parent_id: parent.id, - organization_id: organization.id) - end - groups = (1..4).map do |i| - namespaces.create!(name: "Group #{i}", path: "group_#{i}", type: 'Group', parent_id: group.id, - organization_id: organization.id) - end + it 'enqueues ::GroupDestroyWorker for each group whose parent\'s do not exist and destroys them', :sidekiq_inline do parent.destroy! - orphaned_groups.each do |group| - 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 + expect { background_migration }.to change { namespaces.count }.from(10).to(6) + .and change { namespaces.where(id: orphaned_groups.pluck(:id)).count }.from(4).to(0) 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 d4e514b53dd2d9683d73ac793e2de02688c692de..ce37b8a865c5f9ad773c1e480f7fc113124b89d0 100644 --- a/spec/migrations/20241112163029_queue_delete_orphaned_groups_spec.rb +++ b/spec/migrations/20241112163029_queue_delete_orphaned_groups_spec.rb @@ -8,42 +8,8 @@ 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 - - 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(batched_migration).not_to have_scheduled_batched_migration - end + migration.before -> { expect(batched_migration).not_to have_scheduled_batched_migration } + migration.after -> { expect(batched_migration).not_to have_scheduled_batched_migration } end end end diff --git a/spec/migrations/20241206154945_queue_requeue_delete_orphaned_groups_spec.rb b/spec/migrations/20241206154945_queue_requeue_delete_orphaned_groups_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..47aee132a0eb5dfa2e84f50435607bf6d0a0adec --- /dev/null +++ b/spec/migrations/20241206154945_queue_requeue_delete_orphaned_groups_spec.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +require 'spec_helper' +require_migration! + +RSpec.describe QueueRequeueDeleteOrphanedGroups, migration: :gitlab_main, feature_category: :groups_and_projects do + 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 + + 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(batched_migration).not_to have_scheduled_batched_migration + end + end + end +end