Commit fa931565 authored by Stan Hu's avatar Stan Hu

Defer project destroys within a namespace in Groups::DestroyService#async_execute

Group#destroy would actually hard-delete all associated projects even
though the acts_as_paranoia gem is used, preventing Projects::DestroyService
from doing any work.

We first noticed this while trying to log all projects deletion to the Geo
log.
parent 5a983ac4
...@@ -219,6 +219,12 @@ class Namespace < ActiveRecord::Base ...@@ -219,6 +219,12 @@ class Namespace < ActiveRecord::Base
parent.present? parent.present?
end end
def soft_delete_without_removing_associations
# We can't use paranoia's `#destroy` since this will hard-delete projects.
# Project uses `pending_delete` instead of the acts_as_paranoia gem.
self.deleted_at = Time.now
end
private private
def repository_storage_paths def repository_storage_paths
......
module Groups module Groups
class DestroyService < Groups::BaseService class DestroyService < Groups::BaseService
def async_execute def async_execute
# Soft delete via paranoia gem group.soft_delete_without_removing_associations
group.destroy
job_id = GroupDestroyWorker.perform_async(group.id, current_user.id) 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}") Rails.logger.info("User #{current_user.id} scheduled a deletion of group ID #{group.id} with job ID #{job_id}")
end end
......
---
title: Defer project destroys within a namespace in Groups::DestroyService#async_execute
merge_request:
author:
...@@ -342,6 +342,17 @@ describe Namespace, models: true do ...@@ -342,6 +342,17 @@ describe Namespace, models: true do
end end
end end
describe '#soft_delete_without_removing_associations' do
let(:project1) { create(:project_empty_repo, namespace: namespace) }
it 'updates the deleted_at timestamp but preserves projects' do
namespace.soft_delete_without_removing_associations
expect(Project.all).to include(project1)
expect(namespace.deleted_at).not_to be_nil
end
end
describe '#user_ids_for_project_authorizations' do describe '#user_ids_for_project_authorizations' do
it 'returns the user IDs for which to refresh authorizations' do it 'returns the user IDs for which to refresh authorizations' do
expect(namespace.user_ids_for_project_authorizations) expect(namespace.user_ids_for_project_authorizations)
......
...@@ -15,6 +15,14 @@ describe Groups::DestroyService, services: true do ...@@ -15,6 +15,14 @@ describe Groups::DestroyService, services: true do
group.add_user(user, Gitlab::Access::OWNER) group.add_user(user, Gitlab::Access::OWNER)
end end
def destroy_group(group, user, async)
if async
Groups::DestroyService.new(group, user).async_execute
else
Groups::DestroyService.new(group, user).execute
end
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
...@@ -30,37 +38,41 @@ describe Groups::DestroyService, services: true do ...@@ -30,37 +38,41 @@ describe Groups::DestroyService, services: true do
context 'file system' do context 'file system' do
context 'Sidekiq inline' do context 'Sidekiq inline' do
before do before do
# Run sidekiq immediatly to check that renamed dir will be removed # Run sidekiq immediately to check that renamed dir will be removed
Sidekiq::Testing.inline! { destroy_group(group, user, async) } Sidekiq::Testing.inline! { destroy_group(group, user, async) }
end end
it { expect(gitlab_shell.exists?(project.repository_storage_path, group.path)).to be_falsey } it 'verifies that paths have been deleted' do
it { expect(gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_falsey } expect(gitlab_shell.exists?(project.repository_storage_path, group.path)).to be_falsey
expect(gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_falsey
end
end end
end
end
describe 'asynchronous delete' do
it_behaves_like 'group destruction', true
context 'Sidekiq fake' do context 'Sidekiq fake' do
before do before do
# Don't run sidekiq to check if renamed repository exists # Don't run Sidekiq to verify that group and projects are not actually destroyed
Sidekiq::Testing.fake! { destroy_group(group, user, async) } Sidekiq::Testing.fake! { destroy_group(group, user, true) }
end end
it { expect(gitlab_shell.exists?(project.repository_storage_path, group.path)).to be_falsey } after do
it { expect(gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_truthy } # Clean up stale directories
end gitlab_shell.rm_namespace(project.repository_storage_path, group.path)
gitlab_shell.rm_namespace(project.repository_storage_path, remove_path)
end end
def destroy_group(group, user, async) it 'verifies original paths and projects still exist' do
if async expect(gitlab_shell.exists?(project.repository_storage_path, group.path)).to be_truthy
Groups::DestroyService.new(group, user).async_execute expect(gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_falsey
else expect(Project.unscoped.count).to eq(1)
Groups::DestroyService.new(group, user).execute expect(Group.unscoped.count).to eq(2)
end
end end
end end
describe 'asynchronous delete' do
it_behaves_like 'group destruction', true
context 'potential race conditions' do context 'potential race conditions' do
context "when the `GroupDestroyWorker` task runs immediately" do context "when the `GroupDestroyWorker` task runs immediately" do
it "deletes the group" do it "deletes the group" do
......
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