Merge branch '29289-project-destroy-clean-up-after-failure' into 'master'
Handle errors while a project is being deleted asynchronously. Closes #29289 See merge request !11088
This commit is contained in:
commit
5de3ec64da
|
@ -15,40 +15,48 @@ module Projects
|
||||||
def execute
|
def execute
|
||||||
return false unless can?(current_user, :remove_project, project)
|
return false unless can?(current_user, :remove_project, project)
|
||||||
|
|
||||||
repo_path = project.path_with_namespace
|
|
||||||
wiki_path = repo_path + '.wiki'
|
|
||||||
|
|
||||||
# Flush the cache for both repositories. This has to be done _before_
|
# Flush the cache for both repositories. This has to be done _before_
|
||||||
# removing the physical repositories as some expiration code depends on
|
# removing the physical repositories as some expiration code depends on
|
||||||
# Git data (e.g. a list of branch names).
|
# Git data (e.g. a list of branch names).
|
||||||
flush_caches(project, wiki_path)
|
flush_caches(project)
|
||||||
|
|
||||||
Projects::UnlinkForkService.new(project, current_user).execute
|
Projects::UnlinkForkService.new(project, current_user).execute
|
||||||
|
|
||||||
Project.transaction do
|
attempt_destroy_transaction(project)
|
||||||
project.team.truncate
|
|
||||||
project.destroy!
|
|
||||||
|
|
||||||
unless remove_legacy_registry_tags
|
|
||||||
raise_error('Failed to remove some tags in project container registry. Please try again or contact administrator.')
|
|
||||||
end
|
|
||||||
|
|
||||||
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
|
|
||||||
|
|
||||||
log_info("Project \"#{project.path_with_namespace}\" was removed")
|
|
||||||
system_hook_service.execute_hooks_for(project, :destroy)
|
system_hook_service.execute_hooks_for(project, :destroy)
|
||||||
|
log_info("Project \"#{project.full_path}\" was removed")
|
||||||
|
|
||||||
true
|
true
|
||||||
|
rescue => error
|
||||||
|
attempt_rollback(project, error.message)
|
||||||
|
false
|
||||||
|
rescue Exception => error # rubocop:disable Lint/RescueException
|
||||||
|
# Project.transaction can raise Exception
|
||||||
|
attempt_rollback(project, error.message)
|
||||||
|
raise
|
||||||
end
|
end
|
||||||
|
|
||||||
private
|
private
|
||||||
|
|
||||||
|
def repo_path
|
||||||
|
project.path_with_namespace
|
||||||
|
end
|
||||||
|
|
||||||
|
def wiki_path
|
||||||
|
repo_path + '.wiki'
|
||||||
|
end
|
||||||
|
|
||||||
|
def trash_repositories!
|
||||||
|
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
|
||||||
|
|
||||||
def remove_repository(path)
|
def remove_repository(path)
|
||||||
# Skip repository removal. We use this flag when remove user or group
|
# Skip repository removal. We use this flag when remove user or group
|
||||||
return true if params[:skip_repo] == true
|
return true if params[:skip_repo] == true
|
||||||
|
@ -70,6 +78,26 @@ module Projects
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
def attempt_rollback(project, message)
|
||||||
|
return unless project
|
||||||
|
|
||||||
|
project.update_attributes(delete_error: message, pending_delete: false)
|
||||||
|
log_error("Deletion failed on #{project.full_path} with the following message: #{message}")
|
||||||
|
end
|
||||||
|
|
||||||
|
def attempt_destroy_transaction(project)
|
||||||
|
Project.transaction do
|
||||||
|
unless remove_legacy_registry_tags
|
||||||
|
raise_error('Failed to remove some tags in project container registry. Please try again or contact administrator.')
|
||||||
|
end
|
||||||
|
|
||||||
|
trash_repositories!
|
||||||
|
|
||||||
|
project.team.truncate
|
||||||
|
project.destroy!
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
##
|
##
|
||||||
# This method makes sure that we correctly remove registry tags
|
# This method makes sure that we correctly remove registry tags
|
||||||
# for legacy image repository (when repository path equals project path).
|
# for legacy image repository (when repository path equals project path).
|
||||||
|
@ -96,7 +124,7 @@ module Projects
|
||||||
"#{path}+#{project.id}#{DELETED_FLAG}"
|
"#{path}+#{project.id}#{DELETED_FLAG}"
|
||||||
end
|
end
|
||||||
|
|
||||||
def flush_caches(project, wiki_path)
|
def flush_caches(project)
|
||||||
project.repository.before_delete
|
project.repository.before_delete
|
||||||
|
|
||||||
Repository.new(wiki_path, project).before_delete
|
Repository.new(wiki_path, project).before_delete
|
||||||
|
|
|
@ -0,0 +1,6 @@
|
||||||
|
- project = local_assigns.fetch(:project)
|
||||||
|
- return unless project.delete_error.present?
|
||||||
|
|
||||||
|
.project-deletion-failed-message.alert.alert-warning
|
||||||
|
This project was scheduled for deletion, but failed with the following message:
|
||||||
|
= project.delete_error
|
|
@ -0,0 +1,8 @@
|
||||||
|
- project = local_assigns.fetch(:project)
|
||||||
|
- flash_message_container = show_new_nav? ? :new_global_flash : :flash_message
|
||||||
|
|
||||||
|
= content_for flash_message_container do
|
||||||
|
= render partial: 'deletion_failed', locals: { project: project }
|
||||||
|
- if current_user && can?(current_user, :download_code, project)
|
||||||
|
= render 'shared/no_ssh'
|
||||||
|
= render 'shared/no_password'
|
|
@ -1,10 +1,6 @@
|
||||||
- @no_container = true
|
- @no_container = true
|
||||||
- flash_message_container = show_new_nav? ? :new_global_flash : :flash_message
|
|
||||||
|
|
||||||
= content_for flash_message_container do
|
= render partial: 'flash_messages', locals: { project: @project }
|
||||||
- if current_user && can?(current_user, :download_code, @project)
|
|
||||||
= render 'shared/no_ssh'
|
|
||||||
= render 'shared/no_password'
|
|
||||||
|
|
||||||
= render "projects/head"
|
= render "projects/head"
|
||||||
= render "home_panel"
|
= render "home_panel"
|
||||||
|
|
|
@ -1,15 +1,11 @@
|
||||||
- @no_container = true
|
- @no_container = true
|
||||||
- breadcrumb_title "Project"
|
- breadcrumb_title "Project"
|
||||||
- @content_class = "limit-container-width" unless fluid_layout
|
- @content_class = "limit-container-width" unless fluid_layout
|
||||||
- flash_message_container = show_new_nav? ? :new_global_flash : :flash_message
|
|
||||||
|
|
||||||
= content_for :meta_tags do
|
= content_for :meta_tags do
|
||||||
= auto_discovery_link_tag(:atom, project_path(@project, rss_url_options), title: "#{@project.name} activity")
|
= auto_discovery_link_tag(:atom, project_path(@project, rss_url_options), title: "#{@project.name} activity")
|
||||||
|
|
||||||
= content_for flash_message_container do
|
= render partial: 'flash_messages', locals: { project: @project }
|
||||||
- if current_user && can?(current_user, :download_code, @project)
|
|
||||||
= render 'shared/no_ssh'
|
|
||||||
= render 'shared/no_password'
|
|
||||||
|
|
||||||
= render "projects/head"
|
= render "projects/head"
|
||||||
= render "projects/last_push"
|
= render "projects/last_push"
|
||||||
|
|
|
@ -3,14 +3,11 @@ class ProjectDestroyWorker
|
||||||
include DedicatedSidekiqQueue
|
include DedicatedSidekiqQueue
|
||||||
|
|
||||||
def perform(project_id, user_id, params)
|
def perform(project_id, user_id, params)
|
||||||
begin
|
project = Project.find(project_id)
|
||||||
project = Project.unscoped.find(project_id)
|
|
||||||
rescue ActiveRecord::RecordNotFound
|
|
||||||
return
|
|
||||||
end
|
|
||||||
|
|
||||||
user = User.find(user_id)
|
user = User.find(user_id)
|
||||||
|
|
||||||
::Projects::DestroyService.new(project, user, params.symbolize_keys).execute
|
::Projects::DestroyService.new(project, user, params.symbolize_keys).execute
|
||||||
|
rescue ActiveRecord::RecordNotFound => error
|
||||||
|
logger.error("Failed to delete project (#{project_id}): #{error.message}")
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
|
@ -0,0 +1,4 @@
|
||||||
|
---
|
||||||
|
title: Handle errors while a project is being deleted asynchronously.
|
||||||
|
merge_request: 11088
|
||||||
|
author:
|
|
@ -0,0 +1,7 @@
|
||||||
|
class AddColumnDeleteErrorToProjects < ActiveRecord::Migration
|
||||||
|
DOWNTIME = false
|
||||||
|
|
||||||
|
def change
|
||||||
|
add_column :projects, :delete_error, :text
|
||||||
|
end
|
||||||
|
end
|
|
@ -1134,6 +1134,7 @@ ActiveRecord::Schema.define(version: 20170724214302) do
|
||||||
t.integer "cached_markdown_version"
|
t.integer "cached_markdown_version"
|
||||||
t.datetime "last_repository_updated_at"
|
t.datetime "last_repository_updated_at"
|
||||||
t.string "ci_config_path"
|
t.string "ci_config_path"
|
||||||
|
t.text "delete_error"
|
||||||
end
|
end
|
||||||
|
|
||||||
add_index "projects", ["ci_id"], name: "index_projects_on_ci_id", using: :btree
|
add_index "projects", ["ci_id"], name: "index_projects_on_ci_id", using: :btree
|
||||||
|
|
|
@ -0,0 +1,20 @@
|
||||||
|
require 'spec_helper'
|
||||||
|
|
||||||
|
describe 'Project show page', feature: true do
|
||||||
|
context 'when project pending delete' do
|
||||||
|
let(:project) { create(:project, :empty_repo, pending_delete: true) }
|
||||||
|
|
||||||
|
before do
|
||||||
|
sign_in(project.owner)
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'shows error message if deletion for project fails' do
|
||||||
|
project.update_attributes(delete_error: "Something went wrong", pending_delete: false)
|
||||||
|
|
||||||
|
visit project_path(project)
|
||||||
|
|
||||||
|
expect(page).to have_selector('.project-deletion-failed-message')
|
||||||
|
expect(page).to have_content("This project was scheduled for deletion, but failed with the following message: #{project.delete_error}")
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
|
@ -396,6 +396,7 @@ Project:
|
||||||
- build_allow_git_fetch
|
- build_allow_git_fetch
|
||||||
- last_repository_updated_at
|
- last_repository_updated_at
|
||||||
- ci_config_path
|
- ci_config_path
|
||||||
|
- delete_error
|
||||||
Author:
|
Author:
|
||||||
- name
|
- name
|
||||||
ProjectFeature:
|
ProjectFeature:
|
||||||
|
|
|
@ -36,6 +36,27 @@ describe Projects::DestroyService, services: true do
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
shared_examples 'handles errors thrown during async destroy' do |error_message|
|
||||||
|
it 'does not allow the error to bubble up' do
|
||||||
|
expect do
|
||||||
|
Sidekiq::Testing.inline! { destroy_project(project, user, {}) }
|
||||||
|
end.not_to raise_error
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'unmarks the project as "pending deletion"' do
|
||||||
|
Sidekiq::Testing.inline! { destroy_project(project, user, {}) }
|
||||||
|
|
||||||
|
expect(project.reload.pending_delete).to be(false)
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'stores an error message in `projects.delete_error`' do
|
||||||
|
Sidekiq::Testing.inline! { destroy_project(project, user, {}) }
|
||||||
|
|
||||||
|
expect(project.reload.delete_error).to be_present
|
||||||
|
expect(project.delete_error).to include(error_message)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
context 'Sidekiq inline' do
|
context 'Sidekiq inline' do
|
||||||
before do
|
before do
|
||||||
# Run sidekiq immediatly to check that renamed repository will be removed
|
# Run sidekiq immediatly to check that renamed repository will be removed
|
||||||
|
@ -89,10 +110,51 @@ describe Projects::DestroyService, services: true do
|
||||||
end
|
end
|
||||||
|
|
||||||
it_behaves_like 'deleting the project with pipeline and build'
|
it_behaves_like 'deleting the project with pipeline and build'
|
||||||
end
|
|
||||||
|
|
||||||
context 'with execute' do
|
context 'errors' do
|
||||||
it_behaves_like 'deleting the project with pipeline and build'
|
context 'when `remove_legacy_registry_tags` fails' do
|
||||||
|
before do
|
||||||
|
expect_any_instance_of(Projects::DestroyService)
|
||||||
|
.to receive(:remove_legacy_registry_tags).and_return(false)
|
||||||
|
end
|
||||||
|
|
||||||
|
it_behaves_like 'handles errors thrown during async destroy', "Failed to remove some tags"
|
||||||
|
end
|
||||||
|
|
||||||
|
context 'when `remove_repository` fails' do
|
||||||
|
before do
|
||||||
|
expect_any_instance_of(Projects::DestroyService)
|
||||||
|
.to receive(:remove_repository).and_return(false)
|
||||||
|
end
|
||||||
|
|
||||||
|
it_behaves_like 'handles errors thrown during async destroy', "Failed to remove project repository"
|
||||||
|
end
|
||||||
|
|
||||||
|
context 'when `execute` raises expected error' do
|
||||||
|
before do
|
||||||
|
expect_any_instance_of(Project)
|
||||||
|
.to receive(:destroy!).and_raise(StandardError.new("Other error message"))
|
||||||
|
end
|
||||||
|
|
||||||
|
it_behaves_like 'handles errors thrown during async destroy', "Other error message"
|
||||||
|
end
|
||||||
|
|
||||||
|
context 'when `execute` raises unexpected error' do
|
||||||
|
before do
|
||||||
|
expect_any_instance_of(Project)
|
||||||
|
.to receive(:destroy!).and_raise(Exception.new("Other error message"))
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'allows error to bubble up and rolls back project deletion' do
|
||||||
|
expect do
|
||||||
|
Sidekiq::Testing.inline! { destroy_project(project, user, {}) }
|
||||||
|
end.to raise_error
|
||||||
|
|
||||||
|
expect(project.reload.pending_delete).to be(false)
|
||||||
|
expect(project.delete_error).to include("Other error message")
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
describe 'container registry' do
|
describe 'container registry' do
|
||||||
|
@ -119,8 +181,7 @@ describe Projects::DestroyService, services: true do
|
||||||
expect_any_instance_of(ContainerRepository)
|
expect_any_instance_of(ContainerRepository)
|
||||||
.to receive(:delete_tags!).and_return(false)
|
.to receive(:delete_tags!).and_return(false)
|
||||||
|
|
||||||
expect{ destroy_project(project, user) }
|
expect(destroy_project(project, user)).to be false
|
||||||
.to raise_error(ActiveRecord::RecordNotDestroyed)
|
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
@ -145,8 +206,7 @@ describe Projects::DestroyService, services: true do
|
||||||
expect_any_instance_of(ContainerRepository)
|
expect_any_instance_of(ContainerRepository)
|
||||||
.to receive(:delete_tags!).and_return(false)
|
.to receive(:delete_tags!).and_return(false)
|
||||||
|
|
||||||
expect { destroy_project(project, user) }
|
expect(destroy_project(project, user)).to be false
|
||||||
.to raise_error(Projects::DestroyService::DestroyError)
|
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
|
@ -1,24 +1,36 @@
|
||||||
require 'spec_helper'
|
require 'spec_helper'
|
||||||
|
|
||||||
describe ProjectDestroyWorker do
|
describe ProjectDestroyWorker do
|
||||||
let(:project) { create(:project, :repository) }
|
let(:project) { create(:project, :repository, pending_delete: true) }
|
||||||
let(:path) { project.repository.path_to_repo }
|
let(:path) { project.repository.path_to_repo }
|
||||||
|
|
||||||
subject { described_class.new }
|
subject { described_class.new }
|
||||||
|
|
||||||
describe "#perform" do
|
describe '#perform' do
|
||||||
it "deletes the project" do
|
it 'deletes the project' do
|
||||||
subject.perform(project.id, project.owner.id, {})
|
subject.perform(project.id, project.owner.id, {})
|
||||||
|
|
||||||
expect(Project.all).not_to include(project)
|
expect(Project.all).not_to include(project)
|
||||||
expect(Dir.exist?(path)).to be_falsey
|
expect(Dir.exist?(path)).to be_falsey
|
||||||
end
|
end
|
||||||
|
|
||||||
it "deletes the project but skips repo deletion" do
|
it 'deletes the project but skips repo deletion' do
|
||||||
subject.perform(project.id, project.owner.id, { "skip_repo" => true })
|
subject.perform(project.id, project.owner.id, { "skip_repo" => true })
|
||||||
|
|
||||||
expect(Project.all).not_to include(project)
|
expect(Project.all).not_to include(project)
|
||||||
expect(Dir.exist?(path)).to be_truthy
|
expect(Dir.exist?(path)).to be_truthy
|
||||||
end
|
end
|
||||||
|
|
||||||
|
it 'does not raise error when project could not be found' do
|
||||||
|
expect do
|
||||||
|
subject.perform(-1, project.owner.id, {})
|
||||||
|
end.not_to raise_error
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'does not raise error when user could not be found' do
|
||||||
|
expect do
|
||||||
|
subject.perform(project.id, -1, {})
|
||||||
|
end.not_to raise_error
|
||||||
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
Loading…
Reference in New Issue