From 97ff86e07cdfce1915d574772f80e21263ad43e6 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 3 Jun 2015 11:50:08 +0200 Subject: [PATCH 1/3] Move repository when project is removed Ths commit does next: * When we remove project we move repository to path+deleted.git * Then we schedule removal of path+deleted with sidekiq * If repository move failed we abort project removal This should help us with NFS issue when project get removed but repository stayed. The full explanation of problem is below: * rm -rf project.git * rm -rf removes project.git/objects/foo * NFS server renames foo to foo.nfsXXXX because some NFS client (think * Unicorn) still has the file open * rm -rf exits, but project.git/objects/foo.nfsXXX still exists * Unicorn closes the file, the NFS client closes the file (foo), and the * NFS server removes foo.nfsXXX * the directory project.git/objects/ still exists => problem So now we move repository and even if repository removal failed Repository directory is moved so no bugs with project removed but repository directory taken. User still able to create new project with same name. From administrator perspective you can easily find stalled repositories by searching `*+deleted.git` Signed-off-by: Dmitriy Zaporozhets --- CHANGELOG | 1 + app/controllers/projects_controller.rb | 17 +++---- app/services/projects/destroy_service.rb | 65 +++++++++++++++++++----- lib/gitlab/backend/shell.rb | 14 +++-- 4 files changed, 70 insertions(+), 27 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index dbc54021d40..381369f0dea 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -40,6 +40,7 @@ v 7.12.0 (unreleased) - Add an option to automatically sign-in with an Omniauth provider - Better performance for web editor (switched from satellites to rugged) - GitLab CI service sends .gitlab-ci.yaml in each push call + - When remove project - move repository and schedule it removal v 7.11.4 - Fix missing bullets when creating lists diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index dc430351551..4ca5fc65459 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -97,18 +97,15 @@ class ProjectsController < ApplicationController return access_denied! unless can?(current_user, :remove_project, @project) ::Projects::DestroyService.new(@project, current_user, {}).execute + flash[:alert] = 'Project deleted.' - respond_to do |format| - format.html do - flash[:alert] = 'Project deleted.' - - if request.referer.include?('/admin') - redirect_to admin_namespaces_projects_path - else - redirect_to dashboard_path - end - end + if request.referer.include?('/admin') + redirect_to admin_namespaces_projects_path + else + redirect_to dashboard_path end + rescue Projects::DestroyService::DestroyError => ex + redirect_to edit_project_path(@project), alert: ex.message end def autocomplete_sources diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb index 7e1d753b021..53bf36b1010 100644 --- a/app/services/projects/destroy_service.rb +++ b/app/services/projects/destroy_service.rb @@ -1,28 +1,67 @@ module Projects class DestroyService < BaseService + include Gitlab::ShellAdapter + + class DestroyError < StandardError; end + + DELETED_FLAG = '+deleted' + def execute return false unless can?(current_user, :remove_project, project) project.team.truncate project.repository.expire_cache unless project.empty_repo? - if project.destroy - GitlabShellWorker.perform_async( - :remove_repository, - project.path_with_namespace - ) + repo_path = project.path_with_namespace + wiki_path = repo_path + '.wiki' - GitlabShellWorker.perform_async( - :remove_repository, - project.path_with_namespace + ".wiki" - ) + Project.transaction do + project.destroy! - project.satellite.destroy + unless remove_repository(repo_path) + raise_error('Failed to remove project repository. Please try again or contact administrator') + end - log_info("Project \"#{project.name}\" was removed") - system_hook_service.execute_hooks_for(project, :destroy) - true + unless remove_repository(wiki_path) + raise_error('Failed to remove wiki repository. Please try again or contact administrator') + end end + + project.satellite.destroy + log_info("Project \"#{project.name}\" was removed") + system_hook_service.execute_hooks_for(project, :destroy) + true + end + + private + + def remove_repository(path) + unless gitlab_shell.exists?(path + '.git') + return true + end + + new_path = removal_path(path) + + if gitlab_shell.mv_repository(path, new_path) + log_info("Repository \"#{path}\" moved to \"#{new_path}\"") + GitlabShellWorker.perform_in(30.seconds, :remove_repository, new_path) + else + false + end + end + + def raise_error(message) + raise DestroyError.new(message) + end + + # Build a path for removing repositories + # We use `+` because its not allowed by GitLab so user can not create + # project with name cookies+119+deleted and capture someone stalled repository + # + # gitlab/cookies.git -> gitlab/cookies+119+deleted.git + # + def removal_path(path) + "#{path}+#{project.id}#{DELETED_FLAG}" end end end diff --git a/lib/gitlab/backend/shell.rb b/lib/gitlab/backend/shell.rb index 530f9d93de4..172d4902add 100644 --- a/lib/gitlab/backend/shell.rb +++ b/lib/gitlab/backend/shell.rb @@ -244,6 +244,16 @@ module Gitlab end end + # Check if such directory exists in repositories. + # + # Usage: + # exists?('gitlab') + # exists?('gitlab/cookies.git') + # + def exists?(dir_name) + File.exists?(full_path(dir_name)) + end + protected def gitlab_shell_path @@ -264,10 +274,6 @@ module Gitlab File.join(repos_path, dir_name) end - def exists?(dir_name) - File.exists?(full_path(dir_name)) - end - def gitlab_shell_projects_path File.join(gitlab_shell_path, 'bin', 'gitlab-projects') end From fd1723f0fcd0e8d5dbea3fd5ec18f271e3d6da0d Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 3 Jun 2015 13:08:17 +0200 Subject: [PATCH 2/3] Add tests for project destroy service Signed-off-by: Dmitriy Zaporozhets --- .../services/projects/destroy_service_spec.rb | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 spec/services/projects/destroy_service_spec.rb diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb new file mode 100644 index 00000000000..cdf576cc0c1 --- /dev/null +++ b/spec/services/projects/destroy_service_spec.rb @@ -0,0 +1,34 @@ +require 'spec_helper' + +describe Projects::DestroyService do + let!(:user) { create(:user) } + let!(:project) { create(:project, namespace: user.namespace) } + let!(:path) { project.repository.path_to_repo } + let!(:remove_path) { path.sub(/\.git\Z/, "+#{project.id}+deleted.git") } + + context 'Sidekiq inline' do + before do + # Run sidekiq immediatly to check that renamed repository will be removed + Sidekiq::Testing.inline! { destroy_project(project, user, {}) } + end + + it { Project.all.should_not include(project) } + it { Dir.exists?(path).should be_falsey } + it { Dir.exists?(remove_path).should be_falsey } + end + + context 'Sidekiq fake' do + before do + # Dont run sidekiq to check if renamed repository exists + Sidekiq::Testing.fake! { destroy_project(project, user, {}) } + end + + it { Project.all.should_not include(project) } + it { Dir.exists?(path).should be_falsey } + it { Dir.exists?(remove_path).should be_truthy } + end + + def destroy_project(project, user, params) + Projects::DestroyService.new(project, user, params).execute + end +end From 58ab8a4a9d0e051cf6355c1b9a4d8dc13fc892cc Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 3 Jun 2015 15:09:12 +0200 Subject: [PATCH 3/3] Fix tests and increase delay time before remove repository Signed-off-by: Dmitriy Zaporozhets --- app/services/projects/destroy_service.rb | 2 +- spec/features/projects_spec.rb | 9 --------- spec/requests/api/projects_spec.rb | 9 ++------- 3 files changed, 3 insertions(+), 17 deletions(-) diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb index 53bf36b1010..29e8ba347d4 100644 --- a/app/services/projects/destroy_service.rb +++ b/app/services/projects/destroy_service.rb @@ -44,7 +44,7 @@ module Projects if gitlab_shell.mv_repository(path, new_path) log_info("Repository \"#{path}\" moved to \"#{new_path}\"") - GitlabShellWorker.perform_in(30.seconds, :remove_repository, new_path) + GitlabShellWorker.perform_in(5.minutes, :remove_repository, new_path) else false end diff --git a/spec/features/projects_spec.rb b/spec/features/projects_spec.rb index cae11be7cdd..24d4a67d50b 100644 --- a/spec/features/projects_spec.rb +++ b/spec/features/projects_spec.rb @@ -13,15 +13,6 @@ describe "Projects", feature: true, js: true do it "should remove project" do expect { remove_project }.to change {Project.count}.by(-1) end - - it 'should delete the project from disk' do - expect(GitlabShellWorker).to( - receive(:perform_async).with(:remove_repository, - /#{@project.path_with_namespace}/) - ).twice - - remove_project - end end def remove_project diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 46cd26eb927..dbfd72e5f19 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -57,14 +57,14 @@ describe API::API, api: true do expect(json_response.first['name']).to eq(project.name) expect(json_response.first['owner']['username']).to eq(user.username) end - + it 'should include the project labels as the tag_list' do get api('/projects', user) response.status.should == 200 json_response.should be_an Array json_response.first.keys.should include('tag_list') end - + context 'and using search' do it 'should return searched project' do get api('/projects', user), { search: project.name } @@ -792,11 +792,6 @@ describe API::API, api: true do describe 'DELETE /projects/:id' do context 'when authenticated as user' do it 'should remove project' do - expect(GitlabShellWorker).to( - receive(:perform_async).with(:remove_repository, - /#{project.path_with_namespace}/) - ).twice - delete api("/projects/#{project.id}", user) expect(response.status).to eq(200) end