Merge branch '53966-hashed-storage-read-only' into 'master'

Hashed Storage: Only set as `read_only` when starting the per-project migration

See merge request gitlab-org/gitlab-ce!24128

app/models/project.rb

@@ -1789,6 +1789,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
@@ -1903,15 +1921,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

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

changelogs/unreleased/53966-hashed-storage-read-only.yml

+---
+title: 'Hashed Storage: Only set as `read_only` when starting the per-project migration'
+merge_request: 24128
+author:
+type: changed

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)
-      expect(project.reload.repository_read_only?).to be_truthy
+    it 'migrate project' do
+      perform_enqueued_jobs do
+        subject.migrate(project)
+      end
+      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

spec/models/project_spec.rb

@@ -2411,6 +2411,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) }
@@ -3144,6 +3158,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 }
@@ -3204,10 +3245,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)

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