From 97ff86e07cdfce1915d574772f80e21263ad43e6 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 3 Jun 2015 11:50:08 +0200 Subject: 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 --- app/controllers/projects_controller.rb | 17 ++++----- app/services/projects/destroy_service.rb | 65 +++++++++++++++++++++++++------- 2 files changed, 59 insertions(+), 23 deletions(-) (limited to 'app') 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' + + Project.transaction do + project.destroy! - GitlabShellWorker.perform_async( - :remove_repository, - project.path_with_namespace + ".wiki" - ) + unless remove_repository(repo_path) + raise_error('Failed to remove project repository. Please try again or contact administrator') + end + + 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 - project.satellite.destroy + private - log_info("Project \"#{project.name}\" was removed") - system_hook_service.execute_hooks_for(project, :destroy) - true + 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 -- cgit v1.2.1 From 58ab8a4a9d0e051cf6355c1b9a4d8dc13fc892cc Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 3 Jun 2015 15:09:12 +0200 Subject: Fix tests and increase delay time before remove repository Signed-off-by: Dmitriy Zaporozhets --- app/services/projects/destroy_service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'app') 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 -- cgit v1.2.1