Commit 4fd848fc authored by Oswaldo Ferreira's avatar Oswaldo Ferreira

Cleanup stale +deleted repo paths on project removal

1. When removing projects, we can end-up leaving the +deleted
repo path dirty and not successfully removing the non-deleted
namespace (mv process is not atomic and can be killed without
fully moving the path).

2. In order to solve that, we're adding a clean-up phase on
ensure which will schedule possible staled +deleted path deletion.
Note that we don't check the current state (if there is or not a
repo) in order to schedule the deletion. That's intentional
in order to leverage Gitlab::GitalyClient::NamespaceService#remove
idempotency and ensure consistency.
parent 61d2c326
......@@ -7,9 +7,16 @@ module Projects
DestroyError = Class.new(StandardError)
DELETED_FLAG = '+deleted'.freeze
REPO_REMOVAL_DELAY = 5.minutes.to_i
def async_execute
project.update_attribute(:pending_delete, true)
# Ensure no repository +deleted paths are kept,
# regardless of any issue with the ProjectDestroyWorker
# job process.
schedule_stale_repos_removal
job_id = ProjectDestroyWorker.perform_async(project.id, current_user.id, params)
Rails.logger.info("User #{current_user.id} scheduled destruction of project #{project.full_path} with job ID #{job_id}")
end
......@@ -92,14 +99,23 @@ module Projects
log_info(%Q{Repository "#{path}" moved to "#{new_path}" for project "#{project.full_path}"})
project.run_after_commit do
# self is now project
GitlabShellWorker.perform_in(5.minutes, :remove_repository, self.repository_storage, new_path)
GitlabShellWorker.perform_in(REPO_REMOVAL_DELAY, :remove_repository, self.repository_storage, new_path)
end
else
false
end
end
def schedule_stale_repos_removal
repo_paths = [removal_path(repo_path), removal_path(wiki_path)]
# Ideally it should wait until the regular removal phase finishes,
# so let's delay it a bit further.
repo_paths.each do |path|
GitlabShellWorker.perform_in(REPO_REMOVAL_DELAY * 2, :remove_repository, project.repository_storage, path)
end
end
def rollback_repository(old_path, new_path)
# There is a possibility project does not have repository or wiki
return true unless repo_exists?(old_path)
......@@ -113,13 +129,11 @@ module Projects
end
# rubocop: enable CodeReuse/ActiveRecord
# rubocop: disable CodeReuse/ActiveRecord
def mv_repository(from_path, to_path)
return true unless gitlab_shell.exists?(project.repository_storage, from_path + '.git')
return true unless repo_exists?(from_path)
gitlab_shell.mv_repository(project.repository_storage, from_path, to_path)
end
# rubocop: enable CodeReuse/ActiveRecord
def attempt_rollback(project, message)
return unless project
......
---
title: Cleanup stale +deleted repo paths on project removal (adjusts project removal bug)
merge_request: 24269
author:
type: fixed
......@@ -281,6 +281,40 @@ describe Projects::DestroyService do
end
end
context 'repository +deleted path removal' do
def removal_path(path)
"#{path}+#{project.id}#{described_class::DELETED_FLAG}"
end
context 'regular phase' do
it 'schedules +deleted removal of existing repos' do
service = described_class.new(project, user, {})
allow(service).to receive(:schedule_stale_repos_removal)
expect(GitlabShellWorker).to receive(:perform_in)
.with(5.minutes, :remove_repository, project.repository_storage, removal_path(project.disk_path))
service.execute
end
end
context 'stale cleanup' do
let!(:async) { true }
it 'schedules +deleted wiki and repo removal' do
allow(ProjectDestroyWorker).to receive(:perform_async)
expect(GitlabShellWorker).to receive(:perform_in)
.with(10.minutes, :remove_repository, project.repository_storage, removal_path(project.disk_path))
expect(GitlabShellWorker).to receive(:perform_in)
.with(10.minutes, :remove_repository, project.repository_storage, removal_path(project.wiki.disk_path))
destroy_project(project, user, {})
end
end
end
context '#attempt_restore_repositories' do
let(:path) { project.disk_path + '.git' }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment