Commit 1f12aaf2 authored by Douwe Maan's avatar Douwe Maan Committed by Felipe Artur

Merge branch 'sh-namespace-cleanup-deleted-projects' into 'master'

Fix a number of race conditions that can occur during namespace deletion

See merge request !9294
parent 5ae32ae3
...@@ -35,7 +35,7 @@ class Namespace < ActiveRecord::Base ...@@ -35,7 +35,7 @@ class Namespace < ActiveRecord::Base
after_commit :refresh_access_of_projects_invited_groups, on: :update, if: -> { previous_changes.key?('share_with_group_lock') } after_commit :refresh_access_of_projects_invited_groups, on: :update, if: -> { previous_changes.key?('share_with_group_lock') }
# Save the storage paths before the projects are destroyed to use them on after destroy # Save the storage paths before the projects are destroyed to use them on after destroy
before_destroy(prepend: true) { @old_repository_storage_paths = repository_storage_paths } before_destroy(prepend: true) { prepare_for_destroy }
after_destroy :rm_dir after_destroy :rm_dir
scope :root, -> { where('type IS NULL') } scope :root, -> { where('type IS NULL') }
...@@ -217,6 +217,18 @@ class Namespace < ActiveRecord::Base ...@@ -217,6 +217,18 @@ class Namespace < ActiveRecord::Base
[owner_id] [owner_id]
end end
def parent_changed?
parent_id_changed?
end
def prepare_for_destroy
old_repository_storage_paths
end
def old_repository_storage_paths
@old_repository_storage_paths ||= repository_storage_paths
end
private private
def repository_storage_paths def repository_storage_paths
...@@ -230,7 +242,7 @@ class Namespace < ActiveRecord::Base ...@@ -230,7 +242,7 @@ class Namespace < ActiveRecord::Base
def rm_dir def rm_dir
# Remove the namespace directory in all storages paths used by member projects # Remove the namespace directory in all storages paths used by member projects
@old_repository_storage_paths.each do |repository_storage_path| old_repository_storage_paths.each do |repository_storage_path|
# Move namespace directory into trash. # Move namespace directory into trash.
# We will remove it later async # We will remove it later async
new_path = "#{path}+#{id}+deleted" new_path = "#{path}+#{id}+deleted"
......
...@@ -214,6 +214,8 @@ class Project < ActiveRecord::Base ...@@ -214,6 +214,8 @@ class Project < ActiveRecord::Base
# Scopes # Scopes
default_scope { where(pending_delete: false) } default_scope { where(pending_delete: false) }
scope :with_deleted, -> { unscope(where: :pending_delete) }
scope :sorted_by_activity, -> { reorder(last_activity_at: :desc) } scope :sorted_by_activity, -> { reorder(last_activity_at: :desc) }
scope :sorted_by_stars, -> { reorder('projects.star_count DESC') } scope :sorted_by_stars, -> { reorder('projects.star_count DESC') }
......
module Groups
class DestroyService < Groups::BaseService
def async_execute
# Soft delete via paranoia gem
group.destroy
job_id = GroupDestroyWorker.perform_async(group.id, current_user.id)
Rails.logger.info("User #{current_user.id} scheduled a deletion of group ID #{group.id} with job ID #{job_id}")
end
def execute
group.prepare_for_destroy
group.projects.with_deleted.each do |project|
# Execute the destruction of the models immediately to ensure atomic cleanup.
# Skip repository removal because we remove directory with namespace
# that contain all these repositories
::Projects::DestroyService.new(project, current_user, skip_repo: true).execute
end
group.children.each do |group|
DestroyService.new(group, current_user).async_execute
end
group.really_destroy!
end
end
end
...@@ -9,14 +9,18 @@ describe DestroyGroupService, services: true do ...@@ -9,14 +9,18 @@ describe DestroyGroupService, services: true do
let!(:gitlab_shell) { Gitlab::Shell.new } let!(:gitlab_shell) { Gitlab::Shell.new }
let!(:remove_path) { group.path + "+#{group.id}+deleted" } let!(:remove_path) { group.path + "+#{group.id}+deleted" }
before do
group.add_user(user, Gitlab::Access::OWNER)
end
shared_examples 'group destruction' do |async| shared_examples 'group destruction' do |async|
context 'database records' do context 'database records' do
before do before do
destroy_group(group, user, async) destroy_group(group, user, async)
end end
it { expect(Group.all).not_to include(group) } it { expect(Group.unscoped.all).not_to include(group) }
it { expect(Project.all).not_to include(project) } it { expect(Project.unscoped.all).not_to include(project) }
end end
context 'file system' do context 'file system' do
...@@ -32,7 +36,7 @@ describe DestroyGroupService, services: true do ...@@ -32,7 +36,7 @@ describe DestroyGroupService, services: true do
context 'Sidekiq fake' do context 'Sidekiq fake' do
before do before do
# Dont run sidekiq to check if renamed repository exists # Don't run sidekiq to check if renamed repository exists
Sidekiq::Testing.fake! { destroy_group(group, user, async) } Sidekiq::Testing.fake! { destroy_group(group, user, async) }
end end
...@@ -95,4 +99,13 @@ describe DestroyGroupService, services: true do ...@@ -95,4 +99,13 @@ describe DestroyGroupService, services: true do
describe 'synchronous delete' do describe 'synchronous delete' do
it_behaves_like 'group destruction', false it_behaves_like 'group destruction', false
end end
context 'projects in pending_delete' do
before do
project.pending_delete = true
project.save
end
it_behaves_like 'group destruction', false
end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment