Merge branch '220104-read-only-repository-cleanup' into 'master'

Make the repository read-only while running cleanup

See merge request gitlab-org/gitlab!45058

app/controllers/projects/settings/repository_controller.rb

@@ -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)

app/services/projects/cleanup_service.rb

@@ -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

app/workers/repository_cleanup_worker.rb

@@ -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
 

changelogs/unreleased/220104-read-only-repository-cleanup.yml

+---
+title: Make the repository read-only while running cleanup
+merge_request: 45058
+author:
+type: changed

doc/user/project/repository/reducing_the_repo_size_using_git.md

@@ -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

locale/gitlab.pot

@@ -11165,6 +11165,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 ""
 

spec/controllers/projects/settings/repository_controller_spec.rb

@@ -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
 

spec/services/projects/cleanup_service_spec.rb

@@ -3,14 +3,84 @@
 require 'spec_helper'
 
 RSpec.describe Projects::CleanupService 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 }
+  subject(:service) { described_class.new(project) }
 
-  let(:cleaner) { service.__send__(:repository_cleaner) }
+  describe '.enqueue' do
+    let_it_be(:project) { create(:project, :repository) }
+    let_it_be(:user) { create(:user) }
 
-  subject(:service) { described_class.new(project) }
+    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')) }
+
+    subject(:cleanup_after) { described_class.cleanup_after(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 }

spec/workers/repository_cleanup_worker_spec.rb

@@ -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