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,13 +58,6 @@ 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 - end - it 'migrate project' do perform_enqueued_jobs do subject.migrate(project) @@ -71,5 +65,13 @@ describe Gitlab::HashedStorage::Migrator do expect(project.reload.hashed_storage?(:attachments)).to be_truthy end + + it 'has migrated project set as writable' do + perform_enqueued_jobs do + subject.migrate(project) + end + + 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