Commit 6a3b8eab authored by Nick Thomas's avatar Nick Thomas

Make the repository read-only while running cleanup

We want to perform some potentially-racy actions on the repository, so
we need to make it read-only while they run to rule out any chance of
data loss.

Ideally, Gitaly would be able to enforce the read-only status itself,
but we really need mandatory praefect before we can do it there.
parent ef2ff241
......@@ -18,14 +18,13 @@ module Projects
end
def cleanup
cleanup_params = params.require(:project).permit(:bfg_object_map)
result = Projects::UpdateService.new(project, current_user, cleanup_params).execute
bfg_object_map = params.require(:project).require(:bfg_object_map)
result = Projects::CleanupService.enqueue(project, current_user, bfg_object_map)
if result[:status] == :success
RepositoryCleanupWorker.perform_async(project.id, current_user.id) # rubocop:disable CodeReuse/Worker
flash[:notice] = _('Repository cleanup has started. You will receive an email once the cleanup operation is complete.')
else
flash[:alert] = _('Failed to upload object map file')
flash[:alert] = status.fetch(:message, _('Failed to upload object map file'))
end
redirect_to project_settings_repository_path(project)
......
......@@ -11,6 +11,24 @@ module Projects
include Gitlab::Utils::StrongMemoize
class << self
def enqueue(project, current_user, bfg_object_map)
Projects::UpdateService.new(project, current_user, bfg_object_map: bfg_object_map).execute.tap do |result|
next unless result[:status] == :success
project.set_repository_read_only!
RepositoryCleanupWorker.perform_async(project.id, current_user.id)
end
rescue Project::RepositoryReadOnlyError => err
{ status: :error, message: (_('Failed to make repository read-only. %{reason}') % { reason: err.message }) }
end
def cleanup_after(project)
project.bfg_object_map.remove!
project.set_repository_writable!
end
end
# Attempt to clean up the project following the push. Warning: this is
# destructive!
#
......@@ -29,7 +47,7 @@ module Projects
# time. Better to feel the pain immediately.
project.repository.expire_all_method_caches
project.bfg_object_map.remove!
self.class.cleanup_after(project)
end
private
......
......@@ -27,8 +27,9 @@ class RepositoryCleanupWorker # rubocop:disable Scalability/IdempotentWorker
project = Project.find(project_id)
user = User.find(user_id)
# Ensure the file is removed
project.bfg_object_map.remove!
# Ensure the file is removed and the repository is made read-write again
Projects::CleanupService.cleanup_after(project)
notification_service.repository_cleanup_failure(project, user, error)
end
......
---
title: Make the repository read-only while running cleanup
merge_request: 45058
author:
type: changed
......@@ -202,6 +202,12 @@ To purge files from GitLab storage:
## Repository cleanup
NOTE: **Note:**
Safely cleaning the repository requires it to be made read-only for the duration
of the operation. This happens automatically, but submitting the cleanup request
will fail if any writes are ongoing, so cancel any outstanding `git push`
operations before continuing.
> [Introduced](https://gitlab.com/gitlab-org/gitlab-foss/-/issues/19376) in GitLab 11.6.
Repository cleanup allows you to upload a text file of objects and GitLab will remove internal Git
......
......@@ -11055,6 +11055,9 @@ msgstr ""
msgid "Failed to load stacktrace."
msgstr ""
msgid "Failed to make repository read-only. %{reason}"
msgstr ""
msgid "Failed to mark this issue as a duplicate because referenced issue was not found."
msgstr ""
......
......@@ -23,13 +23,15 @@ RSpec.describe Projects::Settings::RepositoryController do
describe 'PUT cleanup' do
let(:object_map) { fixture_file_upload('spec/fixtures/bfg_object_map.txt') }
it 'enqueues a RepositoryCleanupWorker' do
allow(RepositoryCleanupWorker).to receive(:perform_async)
it 'enqueues a project cleanup' do
expect(Projects::CleanupService)
.to receive(:enqueue)
.with(project, user, anything)
.and_return(status: :success)
put :cleanup, params: { namespace_id: project.namespace, project_id: project, project: { object_map: object_map } }
put :cleanup, params: { namespace_id: project.namespace, project_id: project, project: { bfg_object_map: object_map } }
expect(response).to redirect_to project_settings_repository_path(project)
expect(RepositoryCleanupWorker).to have_received(:perform_async).once
end
end
......
......@@ -3,14 +3,84 @@
require 'spec_helper'
RSpec.describe Projects::CleanupService do
subject(:service) { described_class.new(project) }
describe '.enqueue' do
let_it_be(:project) { create(:project, :repository) }
let_it_be(:user) { create(:user) }
let(:object_map_file) { fixture_file_upload('spec/fixtures/bfg_object_map.txt') }
subject(:enqueue) { described_class.enqueue(project, user, object_map_file) }
it 'makes the repository read-only' do
expect { enqueue }
.to change(project, :repository_read_only?)
.from(false)
.to(true)
end
it 'sets the bfg_object_map of the project' do
enqueue
expect(project.bfg_object_map.read).to eq(object_map_file.read)
end
it 'enqueues a RepositoryCleanupWorker' do
enqueue
expect(RepositoryCleanupWorker.jobs.count).to eq(1)
end
it 'returns success' do
expect(enqueue[:status]).to eq(:success)
end
it 'returns an error if making the repository read-only fails' do
project.set_repository_read_only!
expect(enqueue[:status]).to eq(:error)
end
it 'returns an error if updating the project fails' do
expect_next_instance_of(Projects::UpdateService) do |service|
expect(service).to receive(:execute).and_return(status: :error)
end
expect(enqueue[:status]).to eq(:error)
expect(project.reload.repository_read_only?).to be_falsy
end
end
describe '.cleanup_after' do
let(:project) { create(:project, :repository, bfg_object_map: fixture_file_upload('spec/fixtures/bfg_object_map.txt')) }
let(:object_map) { project.bfg_object_map }
let(:cleaner) { service.__send__(:repository_cleaner) }
subject(:cleanup_after) { described_class.cleanup_after(project) }
subject(:service) { described_class.new(project) }
before do
project.set_repository_read_only!
end
it 'sets the repository read-write' do
expect { cleanup_after }.to change(project, :repository_read_only?).from(true).to(false)
end
it 'removes the BFG object map' do
cleanup_after
expect(project.bfg_object_map).not_to be_exist
end
end
describe '#execute' do
let(:project) { create(:project, :repository, bfg_object_map: fixture_file_upload('spec/fixtures/bfg_object_map.txt')) }
let(:object_map) { project.bfg_object_map }
let(:cleaner) { service.__send__(:repository_cleaner) }
before do
project.set_repository_read_only!
end
it 'runs the apply_bfg_object_map_stream gitaly RPC' do
expect(cleaner).to receive(:apply_bfg_object_map_stream).with(kind_of(IO))
......@@ -37,6 +107,13 @@ RSpec.describe Projects::CleanupService do
expect(object_map.exists?).to be_falsy
end
it 'makes the repository read-write again' do
expect { service.execute }
.to change(project, :repository_read_only?)
.from(true)
.to(false)
end
context 'with a tainted merge request diff' do
let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
let(:diff) { merge_request.merge_request_diff }
......
......@@ -40,6 +40,8 @@ RSpec.describe RepositoryCleanupWorker do
describe '#sidekiq_retries_exhausted' do
let(:job) { { 'args' => [project.id, user.id], 'error_message' => 'Error' } }
subject(:sidekiq_retries_exhausted) { described_class.sidekiq_retries_exhausted_block.call(job, StandardError.new) }
it 'does not send a failure notification for a RecordNotFound error' do
expect(NotificationService).not_to receive(:new)
......@@ -51,7 +53,13 @@ RSpec.describe RepositoryCleanupWorker do
expect(service).to receive(:repository_cleanup_failure).with(project, user, 'Error')
end
described_class.sidekiq_retries_exhausted_block.call(job, StandardError.new)
sidekiq_retries_exhausted
end
it 'cleans up the attempt' do
expect(Projects::CleanupService).to receive(:cleanup_after).with(project)
sidekiq_retries_exhausted
end
end
end
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