Only set as `read_only` when starting the per-project migration53966-hashed-storage-read-only

In the previous code, we locked the project during the migration
scheduling step, which works fine for small setups, but can be
problematic in really big installations.

We now moved the logic to inside the worker, so we minimize the time a
project will be read-only. We also make sure we only do that if
reference counter is `0` (no current operation is in progress).

6 files changed, 114 insertions, 22 deletions

diff --git a/app/models/project.rb b/app/models/project.rb
index e2f010a0432..0187f2eb43f 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1786,6 +1786,24 @@ class Project < ActiveRecord::Base
     handle_update_attribute_error(e, value)
   end
 
+  # Tries to set repository as read_only, checking for existing Git transfers in progress beforehand
+  #
+  # @return [Boolean] true when set to read_only or false when an existing git transfer is in progress
+  def set_repository_read_only!
+    with_lock do
+      break false if git_transfer_in_progress?
+
+      update_column(:repository_read_only, true)
+    end
+  end
+
+  # Set repository as writable again
+  def set_repository_writable!
+    with_lock do
+      update_column(repository_read_only, false)
+    end
+  end
+
   def pushes_since_gc
     Gitlab::Redis::SharedState.with { |redis| redis.get(pushes_since_gc_redis_shared_state_key).to_i }
   end
@@ -1900,15 +1918,17 @@ class Project < ActiveRecord::Base
   def migrate_to_hashed_storage!
     return unless storage_upgradable?
 
-    update!(repository_read_only: true)
-
-    if repo_reference_count > 0 || wiki_reference_count > 0
+    if git_transfer_in_progress?
       ProjectMigrateHashedStorageWorker.perform_in(Gitlab::ReferenceCounter::REFERENCE_EXPIRE_TIME, id)
     else
       ProjectMigrateHashedStorageWorker.perform_async(id)
     end
   end
 
+  def git_transfer_in_progress?
+    repo_reference_count > 0 || wiki_reference_count > 0
+  end
+
   def storage_version=(value)
     super
 
diff --git a/app/services/projects/hashed_storage/migrate_repository_service.rb b/app/services/projects/hashed_storage/migrate_repository_service.rb
index f3e026ba38c..2d851866a18 100644
--- a/app/services/projects/hashed_storage/migrate_repository_service.rb
+++ b/app/services/projects/hashed_storage/migrate_repository_service.rb
@@ -2,6 +2,8 @@
 
 module Projects
   module HashedStorage
+    RepositoryMigrationError = Class.new(StandardError)
+
     class MigrateRepositoryService < BaseService
       include Gitlab::ShellAdapter
 
@@ -16,6 +18,8 @@ module Projects
       end
 
       def execute
+        try_to_set_repository_read_only!
+
         @old_storage_version = project.storage_version
         project.storage_version = ::Project::HASHED_STORAGE_FEATURES[:repository]
         project.ensure_storage_path_exists
@@ -48,6 +52,16 @@ module Projects
 
       private
 
+      def try_to_set_repository_read_only!
+        # Mitigate any push operation to start during migration
+        unless project.set_repository_read_only!
+          migration_error = "Target repository '#{old_disk_path}' cannot be made read-only as there is a git transfer in progress"
+          logger.error migration_error
+
+          raise RepositoryMigrationError, migration_error
+        end
+      end
+
       # rubocop: disable CodeReuse/ActiveRecord
       def has_wiki?
         gitlab_shell.exists?(project.repository_storage, "#{old_wiki_disk_path}.git")
diff --git a/changelogs/unreleased/53966-hashed-storage-read-only.yml b/changelogs/unreleased/53966-hashed-storage-read-only.yml
new file mode 100644
index 00000000000..2b6c9c49c85
--- /dev/null
+++ b/changelogs/unreleased/53966-hashed-storage-read-only.yml
@@ -0,0 +1,5 @@
+---
+title: 'Hashed Storage: Only set as `read_only` when starting the per-project migration'
+merge_request: 24128
+author:
+type: changed
diff --git a/spec/lib/gitlab/hashed_storage/migrator_spec.rb b/spec/lib/gitlab/hashed_storage/migrator_spec.rb
index 7eac2cacb90..01d43ed00a2 100644
--- a/spec/lib/gitlab/hashed_storage/migrator_spec.rb
+++ b/spec/lib/gitlab/hashed_storage/migrator_spec.rb
@@ -19,15 +19,6 @@ describe Gitlab::HashedStorage::Migrator do
       end
     end
 
-    it 'sets projects as read only' do
-      allow(ProjectMigrateHashedStorageWorker).to receive(:perform_async).twice
-      subject.bulk_migrate(ids.min, ids.max)
-
-      projects.each do |project|
-        expect(project.reload.repository_read_only?).to be_truthy
-      end
-    end
-
     it 'rescues and log exceptions' do
       allow_any_instance_of(Project).to receive(:migrate_to_hashed_storage!).and_raise(StandardError)
       expect { subject.bulk_migrate(ids.min, ids.max) }.not_to raise_error
@@ -40,6 +31,16 @@ describe Gitlab::HashedStorage::Migrator do
 
       subject.bulk_migrate(ids.min, ids.max)
     end
+
+    it 'has migrated projects set as writable' do
+      perform_enqueued_jobs do
+        subject.bulk_migrate(ids.min, ids.max)
+      end
+
+      projects.each do |project|
+        expect(project.reload.repository_read_only?).to be_falsey
+      end
+    end
   end
 
   describe '#migrate' do
@@ -57,19 +58,20 @@ describe Gitlab::HashedStorage::Migrator do
       expect { subject.migrate(project) }.not_to raise_error
     end
 
-    it 'sets project as read only' do
-      allow(ProjectMigrateHashedStorageWorker).to receive(:perform_async)
-      subject.migrate(project)
+    it 'migrate project' do
+      perform_enqueued_jobs do
+        subject.migrate(project)
+      end
 
-      expect(project.reload.repository_read_only?).to be_truthy
+      expect(project.reload.hashed_storage?(:attachments)).to be_truthy
     end
 
-    it 'migrate project' do
+    it 'has migrated project set as writable' do
       perform_enqueued_jobs do
         subject.migrate(project)
       end
 
-      expect(project.reload.hashed_storage?(:attachments)).to be_truthy
+      expect(project.reload.repository_read_only?).to be_falsey
     end
   end
 end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 72284035b57..7bf60083d10 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -2410,6 +2410,20 @@ describe Project do
     end
   end
 
+  describe '#set_repository_read_only!' do
+    let(:project) { create(:project) }
+
+    it 'returns true when there is no existing git transfer in progress' do
+      expect(project.set_repository_read_only!).to be_truthy
+    end
+
+    it 'returns false when there is an existing git transfer in progress' do
+      allow(project).to receive(:git_transfer_in_progress?) { true }
+
+      expect(project.set_repository_read_only!).to be_falsey
+    end
+  end
+
   describe '#pushes_since_gc' do
     let(:project) { create(:project) }
 
@@ -3143,6 +3157,33 @@ describe Project do
     end
   end
 
+  describe '#git_transfer_in_progress?' do
+    let(:project) { build(:project) }
+
+    subject { project.git_transfer_in_progress? }
+
+    it 'returns false when repo_reference_count and wiki_reference_count are 0' do
+      allow(project).to receive(:repo_reference_count) { 0 }
+      allow(project).to receive(:wiki_reference_count) { 0 }
+
+      expect(subject).to be_falsey
+    end
+
+    it 'returns true when repo_reference_count is > 0' do
+      allow(project).to receive(:repo_reference_count) { 2 }
+      allow(project).to receive(:wiki_reference_count) { 0 }
+
+      expect(subject).to be_truthy
+    end
+
+    it 'returns true when wiki_reference_count is > 0' do
+      allow(project).to receive(:repo_reference_count) { 0 }
+      allow(project).to receive(:wiki_reference_count) { 2 }
+
+      expect(subject).to be_truthy
+    end
+  end
+
   context 'legacy storage' do
     let(:project) { create(:project, :repository, :legacy_storage) }
     let(:gitlab_shell) { Gitlab::Shell.new }
@@ -3203,10 +3244,6 @@ describe Project do
         expect(project.migrate_to_hashed_storage!).to be_truthy
       end
 
-      it 'flags as read-only' do
-        expect { project.migrate_to_hashed_storage! }.to change { project.repository_read_only }.to(true)
-      end
-
       it 'does not validate project visibility' do
         expect(project).not_to receive(:visibility_level_allowed_as_fork)
         expect(project).not_to receive(:visibility_level_allowed_by_group)
diff --git a/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb b/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb
index 0e82194e9ee..b720f37ffdb 100644
--- a/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb
+++ b/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb
@@ -15,6 +15,20 @@ describe Projects::HashedStorage::MigrateRepositoryService do
       allow(service).to receive(:gitlab_shell) { gitlab_shell }
     end
 
+    context 'repository lock' do
+      it 'tries to lock the repository' do
+        expect(service).to receive(:try_to_set_repository_read_only!)
+
+        service.execute
+      end
+
+      it 'fails when a git operation is in progress' do
+        allow(project).to receive(:repo_reference_count) { 1 }
+
+        expect { service.execute }.to raise_error(Projects::HashedStorage::RepositoryMigrationError)
+      end
+    end
+
     context 'when succeeds' do
       it 'renames project and wiki repositories' do
         service.execute