From cb8a425ba42e9be23b8712ed29b1db2dcc6bd139 Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Sat, 28 May 2016 19:54:17 -0700
Subject: [PATCH] Fix bug where destroying a namespace would not always destroy
projects
There is a race condition in DestroyGroupService now that projects are deleted asynchronously:
1. User attempts to delete group
2. DestroyGroupService iterates through all projects and schedules a Sidekiq job to delete each Project
3. DestroyGroupService destroys the Group, leaving all its projects without a namespace
4. Projects::DestroyService runs later but the can?(current_user,
:remove_project) is `false` because the user no longer has permission to
destroy projects with no namespace.
5. This leaves the project in pending_delete state with no namespace/group.
Projects without a namespace or group also adds another problem: it's not possible to destroy the container
registry tags, since container_registry_path_with_namespace is the wrong value.
The fix is to destroy the group asynchronously and to run execute directly on Projects::DestroyService.
Closes #17893
---
CHANGELOG | 1 +
app/controllers/admin/groups_controller.rb | 4 +-
app/controllers/groups_controller.rb | 4 +-
app/models/namespace.rb | 2 +
app/services/delete_user_service.rb | 7 ++-
app/services/destroy_group_service.rb | 16 ++++-
app/workers/group_destroy_worker.rb | 17 ++++++
db/migrate/20140407135544_fix_namespaces.rb | 10 +++-
...0805041956_add_deleted_at_to_namespaces.rb | 12 ++++
db/schema.rb | 2 +
.../admin/groups_controller_spec.rb | 24 ++++++++
spec/controllers/groups_controller_spec.rb | 29 ++++++++++
spec/requests/api/users_spec.rb | 2 +
spec/services/delete_user_service_spec.rb | 6 +-
spec/services/destroy_group_service_spec.rb | 58 ++++++++++++-------
spec/workers/group_destroy_worker_spec.rb | 19 ++++++
16 files changed, 179 insertions(+), 34 deletions(-)
create mode 100644 app/workers/group_destroy_worker.rb
create mode 100644 db/migrate/20160805041956_add_deleted_at_to_namespaces.rb
create mode 100644 spec/controllers/admin/groups_controller_spec.rb
create mode 100644 spec/workers/group_destroy_worker_spec.rb
diff --git a/CHANGELOG b/CHANGELOG
index 28834c1129ad5..85270bc6001a8 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -92,6 +92,7 @@ v 8.11.0 (unreleased)
- Bump gitlab_git to lazy load compare commits
- Reduce number of queries made for merge_requests/:id/diffs
- Sensible state specific default sort order for issues and merge requests !5453 (tomb0y)
+ - Fix bug where destroying a namespace would not always destroy projects
- Fix RequestProfiler::Middleware error when code is reloaded in development
- Catch what warden might throw when profiling requests to re-throw it
- Avoid commit lookup on diff_helper passing existing local variable to the helper method
diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb
index f3a88a8e6c8cd..4ce18321649da 100644
--- a/app/controllers/admin/groups_controller.rb
+++ b/app/controllers/admin/groups_controller.rb
@@ -48,9 +48,9 @@ def members_update
end
def destroy
- DestroyGroupService.new(@group, current_user).execute
+ DestroyGroupService.new(@group, current_user).async_execute
- redirect_to admin_groups_path, notice: 'Group was successfully deleted.'
+ redirect_to admin_groups_path, alert: "Group '#{@group.name}' was scheduled for deletion."
end
private
diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index 6780a6d4d8731..cb82d62616c86 100644
--- a/app/controllers/groups_controller.rb
+++ b/app/controllers/groups_controller.rb
@@ -87,9 +87,9 @@ def update
end
def destroy
- DestroyGroupService.new(@group, current_user).execute
+ DestroyGroupService.new(@group, current_user).async_execute
- redirect_to root_path, alert: "Group '#{@group.name}' was successfully deleted."
+ redirect_to root_path, alert: "Group '#{@group.name}' was scheduled for deletion."
end
protected
diff --git a/app/models/namespace.rb b/app/models/namespace.rb
index 8b52cc824cd6c..7c29d27ce9784 100644
--- a/app/models/namespace.rb
+++ b/app/models/namespace.rb
@@ -1,4 +1,6 @@
class Namespace < ActiveRecord::Base
+ acts_as_paranoid
+
include Sortable
include Gitlab::ShellAdapter
diff --git a/app/services/delete_user_service.rb b/app/services/delete_user_service.rb
index 2f237de813c6b..eaff88d64632c 100644
--- a/app/services/delete_user_service.rb
+++ b/app/services/delete_user_service.rb
@@ -21,6 +21,11 @@ def execute(user, options = {})
::Projects::DestroyService.new(project, current_user, skip_repo: true).async_execute
end
- user.destroy
+ # Destroy the namespace after destroying the user since certain methods may depend on the namespace existing
+ namespace = user.namespace
+ user_data = user.destroy
+ namespace.really_destroy!
+
+ user_data
end
end
diff --git a/app/services/destroy_group_service.rb b/app/services/destroy_group_service.rb
index a4ebccb560650..0081364b8aaae 100644
--- a/app/services/destroy_group_service.rb
+++ b/app/services/destroy_group_service.rb
@@ -5,13 +5,23 @@ def initialize(group, user)
@group, @current_user = group, user
end
+ def async_execute
+ group.transaction do
+ # 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
+ end
+
def execute
group.projects.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 this repositories
- ::Projects::DestroyService.new(project, current_user, skip_repo: true).async_execute
+ # that contain all these repositories
+ ::Projects::DestroyService.new(project, current_user, skip_repo: true).execute
end
- group.destroy
+ group.really_destroy!
end
end
diff --git a/app/workers/group_destroy_worker.rb b/app/workers/group_destroy_worker.rb
new file mode 100644
index 0000000000000..5048746f09ba5
--- /dev/null
+++ b/app/workers/group_destroy_worker.rb
@@ -0,0 +1,17 @@
+class GroupDestroyWorker
+ include Sidekiq::Worker
+
+ sidekiq_options queue: :default
+
+ def perform(group_id, user_id)
+ begin
+ group = Group.with_deleted.find(group_id)
+ rescue ActiveRecord::RecordNotFound
+ return
+ end
+
+ user = User.find(user_id)
+
+ DestroyGroupService.new(group, user).execute
+ end
+end
diff --git a/db/migrate/20140407135544_fix_namespaces.rb b/db/migrate/20140407135544_fix_namespaces.rb
index 9137496669837..0026ce645a661 100644
--- a/db/migrate/20140407135544_fix_namespaces.rb
+++ b/db/migrate/20140407135544_fix_namespaces.rb
@@ -1,8 +1,14 @@
# rubocop:disable all
class FixNamespaces < ActiveRecord::Migration
+ DOWNTIME = false
+
def up
- Namespace.where('name <> path and type is null').each do |namespace|
- namespace.update_attribute(:name, namespace.path)
+ namespaces = exec_query('SELECT id, path FROM namespaces WHERE name <> path and type is null')
+
+ namespaces.each do |row|
+ id = row['id']
+ path = row['path']
+ exec_query("UPDATE namespaces SET name = '#{path}' WHERE id = #{id}")
end
end
diff --git a/db/migrate/20160805041956_add_deleted_at_to_namespaces.rb b/db/migrate/20160805041956_add_deleted_at_to_namespaces.rb
new file mode 100644
index 0000000000000..a853de3abfbed
--- /dev/null
+++ b/db/migrate/20160805041956_add_deleted_at_to_namespaces.rb
@@ -0,0 +1,12 @@
+class AddDeletedAtToNamespaces < ActiveRecord::Migration
+ include Gitlab::Database::MigrationHelpers
+
+ DOWNTIME = false
+
+ disable_ddl_transaction!
+
+ def change
+ add_column :namespaces, :deleted_at, :datetime
+ add_concurrent_index :namespaces, :deleted_at
+ end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 6c85e1e9dba66..1de2cdcf026bf 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -640,9 +640,11 @@
t.boolean "share_with_group_lock", default: false
t.integer "visibility_level", default: 20, null: false
t.boolean "request_access_enabled", default: true, null: false
+ t.datetime "deleted_at"
end
add_index "namespaces", ["created_at"], name: "index_namespaces_on_created_at", using: :btree
+ add_index "namespaces", ["deleted_at"], name: "index_namespaces_on_deleted_at", using: :btree
add_index "namespaces", ["name"], name: "index_namespaces_on_name", unique: true, using: :btree
add_index "namespaces", ["name"], name: "index_namespaces_on_name_trigram", using: :gin, opclasses: {"name"=>"gin_trgm_ops"}
add_index "namespaces", ["owner_id"], name: "index_namespaces_on_owner_id", using: :btree
diff --git a/spec/controllers/admin/groups_controller_spec.rb b/spec/controllers/admin/groups_controller_spec.rb
new file mode 100644
index 0000000000000..0239aea47fb83
--- /dev/null
+++ b/spec/controllers/admin/groups_controller_spec.rb
@@ -0,0 +1,24 @@
+require 'spec_helper'
+
+describe Admin::GroupsController do
+ let(:group) { create(:group) }
+ let(:project) { create(:project, namespace: group) }
+ let(:admin) { create(:admin) }
+
+ before do
+ sign_in(admin)
+ Sidekiq::Testing.fake!
+ end
+
+ describe 'DELETE #destroy' do
+ it 'schedules a group destroy' do
+ expect { delete :destroy, id: project.group.path }.to change(GroupDestroyWorker.jobs, :size).by(1)
+ end
+
+ it 'redirects to the admin group path' do
+ delete :destroy, id: project.group.path
+
+ expect(response).to redirect_to(admin_groups_path)
+ end
+ end
+end
diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb
index cd98fecd0c7fd..4ae6364207b98 100644
--- a/spec/controllers/groups_controller_spec.rb
+++ b/spec/controllers/groups_controller_spec.rb
@@ -75,4 +75,33 @@
end
end
end
+
+ describe 'DELETE #destroy' do
+ context 'as another user' do
+ it 'returns 404' do
+ sign_in(create(:user))
+
+ delete :destroy, id: group.path
+
+ expect(response.status).to eq(404)
+ end
+ end
+
+ context 'as the group owner' do
+ before do
+ Sidekiq::Testing.fake!
+ sign_in(user)
+ end
+
+ it 'schedules a group destroy' do
+ expect { delete :destroy, id: group.path }.to change(GroupDestroyWorker.jobs, :size).by(1)
+ end
+
+ it 'redirects to the root path' do
+ delete :destroy, id: group.path
+
+ expect(response).to redirect_to(root_path)
+ end
+ end
+ end
end
diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index e0e041b4e152e..0bbba64a6d581 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -564,12 +564,14 @@
end
describe "DELETE /users/:id" do
+ let!(:namespace) { user.namespace }
before { admin }
it "deletes user" do
delete api("/users/#{user.id}", admin)
expect(response).to have_http_status(200)
expect { User.find(user.id) }.to raise_error ActiveRecord::RecordNotFound
+ expect { Namespace.find(namespace.id) }.to raise_error ActiveRecord::RecordNotFound
expect(json_response['email']).to eq(user.email)
end
diff --git a/spec/services/delete_user_service_spec.rb b/spec/services/delete_user_service_spec.rb
index 630458f9efcb4..418a12a83a94b 100644
--- a/spec/services/delete_user_service_spec.rb
+++ b/spec/services/delete_user_service_spec.rb
@@ -9,9 +9,11 @@
context 'no options are given' do
it 'deletes the user' do
- DeleteUserService.new(current_user).execute(user)
+ user_data = DeleteUserService.new(current_user).execute(user)
- expect { User.find(user.id) }.to raise_error(ActiveRecord::RecordNotFound)
+ expect { user_data['email'].to eq(user.email) }
+ expect { User.find(user.id) }.to raise_error(ActiveRecord::RecordNotFound)
+ expect { Namespace.with_deleted.find(user.namespace.id) }.to raise_error(ActiveRecord::RecordNotFound)
end
it 'will delete the project in the near future' do
diff --git a/spec/services/destroy_group_service_spec.rb b/spec/services/destroy_group_service_spec.rb
index eca8ddd8ea4bc..da72464360490 100644
--- a/spec/services/destroy_group_service_spec.rb
+++ b/spec/services/destroy_group_service_spec.rb
@@ -7,38 +7,52 @@
let!(:gitlab_shell) { Gitlab::Shell.new }
let!(:remove_path) { group.path + "+#{group.id}+deleted" }
- context 'database records' do
- before do
- destroy_group(group, user)
+ shared_examples 'group destruction' do |async|
+ context 'database records' do
+ before do
+ destroy_group(group, user, async)
+ end
+
+ it { expect(Group.all).not_to include(group) }
+ it { expect(Project.all).not_to include(project) }
end
- it { expect(Group.all).not_to include(group) }
- it { expect(Project.all).not_to include(project) }
- end
+ context 'file system' do
+ context 'Sidekiq inline' do
+ before do
+ # Run sidekiq immediatly to check that renamed dir will be removed
+ Sidekiq::Testing.inline! { destroy_group(group, user, async) }
+ end
- context 'file system' do
- context 'Sidekiq inline' do
- before do
- # Run sidekiq immediatly to check that renamed dir will be removed
- Sidekiq::Testing.inline! { destroy_group(group, user) }
+ it { expect(gitlab_shell.exists?(project.repository_storage_path, group.path)).to be_falsey }
+ it { expect(gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_falsey }
end
- it { expect(gitlab_shell.exists?(project.repository_storage_path, group.path)).to be_falsey }
- it { expect(gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_falsey }
- end
+ context 'Sidekiq fake' do
+ before do
+ # Dont run sidekiq to check if renamed repository exists
+ Sidekiq::Testing.fake! { destroy_group(group, user, async) }
+ end
- context 'Sidekiq fake' do
- before do
- # Dont run sidekiq to check if renamed repository exists
- Sidekiq::Testing.fake! { destroy_group(group, user) }
+ it { expect(gitlab_shell.exists?(project.repository_storage_path, group.path)).to be_falsey }
+ it { expect(gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_truthy }
end
+ end
- it { expect(gitlab_shell.exists?(project.repository_storage_path, group.path)).to be_falsey }
- it { expect(gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_truthy }
+ def destroy_group(group, user, async)
+ if async
+ DestroyGroupService.new(group, user).async_execute
+ else
+ DestroyGroupService.new(group, user).execute
+ end
end
end
- def destroy_group(group, user)
- DestroyGroupService.new(group, user).execute
+ describe 'asynchronous delete' do
+ it_behaves_like 'group destruction', true
+ end
+
+ describe 'synchronous delete' do
+ it_behaves_like 'group destruction', false
end
end
diff --git a/spec/workers/group_destroy_worker_spec.rb b/spec/workers/group_destroy_worker_spec.rb
new file mode 100644
index 0000000000000..4e4eaf9b2f71c
--- /dev/null
+++ b/spec/workers/group_destroy_worker_spec.rb
@@ -0,0 +1,19 @@
+require 'spec_helper'
+
+describe GroupDestroyWorker do
+ let(:group) { create(:group) }
+ let(:user) { create(:admin) }
+ let!(:project) { create(:project, namespace: group) }
+
+ subject { GroupDestroyWorker.new }
+
+ describe "#perform" do
+ it "deletes the project" do
+ subject.perform(group.id, user.id)
+
+ expect(Group.all).not_to include(group)
+ expect(Project.all).not_to include(project)
+ expect(Dir.exist?(project.path)).to be_falsey
+ end
+ end
+end
--
GitLab