From 3f6667c2a5376d2411940c135879300f0928f468 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 30 Nov 2016 03:11:40 +0800 Subject: [PATCH 1/4] Wrap deleting project with Gitlab::OptimisticLocking.retry_lock We need to retry upon ActiveRecord::StaleObjectError because pipelines or builds could be updated and raising an error, and it doesn't hurt to remove everything while CI is still running. Tests incoming. Closes #24766 --- app/services/projects/destroy_service.rb | 134 +++++++++++++---------- 1 file changed, 76 insertions(+), 58 deletions(-) diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb index a08c6fcd94b1..f7cadb6a8abf 100644 --- a/app/services/projects/destroy_service.rb +++ b/app/services/projects/destroy_service.rb @@ -2,97 +2,115 @@ module Projects class DestroyService < BaseService include Gitlab::ShellAdapter - class DestroyError < StandardError; end - DELETED_FLAG = '+deleted' - def async_execute - project.transaction do - project.update_attribute(:pending_delete, true) - job_id = ProjectDestroyWorker.perform_async(project.id, current_user.id, params) - Rails.logger.info("User #{current_user.id} scheduled destruction of project #{project.path_with_namespace} with job ID #{job_id}") - end - end - - def execute - return false unless can?(current_user, :remove_project, project) + class DestroyError < StandardError; end - project.team.truncate + Executor = Struct.new(:service, :project) do + def execute + project.team.truncate - repo_path = project.path_with_namespace - wiki_path = repo_path + '.wiki' + repo_path = project.path_with_namespace + wiki_path = repo_path + '.wiki' - # Flush the cache for both repositories. This has to be done _before_ - # removing the physical repositories as some expiration code depends on - # Git data (e.g. a list of branch names). - flush_caches(project, wiki_path) + # Flush the cache for both repositories. This has to be done _before_ + # removing the physical repositories as some expiration code depends on + # Git data (e.g. a list of branch names). + flush_caches(wiki_path) - Projects::UnlinkForkService.new(project, current_user).execute + Projects::UnlinkForkService.new(project, service.current_user).execute - Project.transaction do project.destroy! unless remove_registry_tags - raise_error('Failed to remove project container registry. Please try again or contact administrator') + raise_error('Failed to remove project container registry') end unless remove_repository(repo_path) - raise_error('Failed to remove project repository. Please try again or contact administrator') + raise_error('Failed to remove project repository') end unless remove_repository(wiki_path) - raise_error('Failed to remove wiki repository. Please try again or contact administrator') + raise_error('Failed to remove wiki repository') end + + service.log_info( + "Project \"#{project.path_with_namespace}\" was removed") + service.system_hook_service.execute_hooks_for(project, :destroy) + + true end - log_info("Project \"#{project.path_with_namespace}\" was removed") - system_hook_service.execute_hooks_for(project, :destroy) - true - end + private - private + def flush_caches(wiki_path) + project.repository.before_delete - def remove_repository(path) - # Skip repository removal. We use this flag when remove user or group - return true if params[:skip_repo] == true + Repository.new(wiki_path, project).before_delete + end - # There is a possibility project does not have repository or wiki - return true unless gitlab_shell.exists?(project.repository_storage_path, path + '.git') + def remove_registry_tags + return true unless Gitlab.config.registry.enabled - new_path = removal_path(path) + project.container_registry_repository.delete_tags + end - if gitlab_shell.mv_repository(project.repository_storage_path, path, new_path) - log_info("Repository \"#{path}\" moved to \"#{new_path}\"") - GitlabShellWorker.perform_in(5.minutes, :remove_repository, project.repository_storage_path, new_path) - else - false + def remove_repository(path) + # Skip repository removal. We use this flag when remove user or group + return true if service.params[:skip_repo] == true + + # There is a possibility project does not have repository or wiki + return true unless + service.gitlab_shell.exists?( + project.repository_storage_path, path + '.git') + + new_path = removal_path(path) + + if service.gitlab_shell.mv_repository( + project.repository_storage_path, path, new_path) + service.log_info("Repository \"#{path}\" moved to \"#{new_path}\"") + GitlabShellWorker.perform_in( + 5.minutes, + :remove_repository, + project.repository_storage_path, + new_path) + else + false + end end - end - def remove_registry_tags - return true unless Gitlab.config.registry.enabled + def raise_error(message) + raise DestroyError.new( + "#{message}. Please try again or contact administrator") + end - project.container_registry_repository.delete_tags + # 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 - def raise_error(message) - raise DestroyError.new(message) + def async_execute + Project.transaction do + project.update_attribute(:pending_delete, true) + job_id = ProjectDestroyWorker.perform_async(project.id, current_user.id, params) + Rails.logger.info("User #{current_user.id} scheduled destruction of project #{project.path_with_namespace} with job ID #{job_id}") + end 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 + def execute + return false unless can?(current_user, :remove_project, project) - def flush_caches(project, wiki_path) - project.repository.before_delete + Gitlab::OptimisticLocking.retry_lock(project) do |subject| + Executor.new(self, subject).execute + end - Repository.new(wiki_path, project).before_delete + true end end end -- GitLab From 7075f47b08aad496ff644e8e132b4f142958e4c7 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 1 Dec 2016 16:53:11 +0800 Subject: [PATCH 2/4] No need to use transaction here, feedback: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7831#note_19224305 --- app/services/projects/destroy_service.rb | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb index f7cadb6a8abf..494747f1a54b 100644 --- a/app/services/projects/destroy_service.rb +++ b/app/services/projects/destroy_service.rb @@ -96,11 +96,9 @@ def removal_path(path) end def async_execute - Project.transaction do - project.update_attribute(:pending_delete, true) - job_id = ProjectDestroyWorker.perform_async(project.id, current_user.id, params) - Rails.logger.info("User #{current_user.id} scheduled destruction of project #{project.path_with_namespace} with job ID #{job_id}") - end + project.update_attribute(:pending_delete, true) + job_id = ProjectDestroyWorker.perform_async(project.id, current_user.id, params) + Rails.logger.info("User #{current_user.id} scheduled destruction of project #{project.path_with_namespace} with job ID #{job_id}") end def execute -- GitLab From 6a16243d539f6339b6cdeff9f5d10f67cd852724 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 1 Dec 2016 16:53:33 +0800 Subject: [PATCH 3/4] Add a test for retrying upon StaleObjectError --- .../services/projects/destroy_service_spec.rb | 41 ++++++++++++++++--- 1 file changed, 35 insertions(+), 6 deletions(-) diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb index 90771825f5ce..3a8da5e15241 100644 --- a/spec/services/projects/destroy_service_spec.rb +++ b/spec/services/projects/destroy_service_spec.rb @@ -50,15 +50,44 @@ context 'delete with pipeline' do # which has optimistic locking let!(:pipeline) { create(:ci_pipeline, project: project) } - before do - expect(project).to receive(:destroy!).and_call_original - - perform_enqueued_jobs do - destroy_project(project, user, {}) + context 'when pipeline stays intact' do + before do + perform_enqueued_jobs do + destroy_project(project, user, {}) + end end + + it_behaves_like 'deleting the project' end - it_behaves_like 'deleting the project' + context 'when pipeline was updated in the middle' do + before do + run_pipeline_before_destroy = Module.new do + def destroy! + flag = :@run_pipeline_before_destroy_called + + unless instance_variable_defined?(flag) + Ci::Pipeline.find(pipelines.to_a.first.id).run + instance_variable_set(flag, true) + end + + super + end + end + + project.singleton_class.prepend(run_pipeline_before_destroy) + + expect(project).to receive(:destroy!).and_call_original.twice + + perform_enqueued_jobs do + destroy_project(project, user, {}) + end + end + + describe 'retries upon ActiveRecord::StaleObjectError' do + it_behaves_like 'deleting the project' + end + end end context 'container registry' do -- GitLab From b19b3b99c813acd3ce871a41baf09a426f415f27 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 1 Dec 2016 18:27:40 +0800 Subject: [PATCH 4/4] Use simple class without a block --- app/services/projects/destroy_service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb index 494747f1a54b..e57f3d4e11ba 100644 --- a/app/services/projects/destroy_service.rb +++ b/app/services/projects/destroy_service.rb @@ -4,7 +4,7 @@ class DestroyService < BaseService DELETED_FLAG = '+deleted' - class DestroyError < StandardError; end + DestroyError = Class.new(StandardError) Executor = Struct.new(:service, :project) do def execute -- GitLab