From ee4af0c64cdf00d2c34ce7feb773e057f9758cff Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Sat, 22 Dec 2018 04:01:24 +0100 Subject: Only set as `read_only` when starting the per-project migration 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). --- app/models/project.rb | 26 +++++++++++-- .../hashed_storage/migrate_repository_service.rb | 14 +++++++ .../unreleased/53966-hashed-storage-read-only.yml | 5 +++ spec/lib/gitlab/hashed_storage/migrator_spec.rb | 32 +++++++-------- spec/models/project_spec.rb | 45 ++++++++++++++++++++-- .../migrate_repository_service_spec.rb | 14 +++++++ 6 files changed, 114 insertions(+), 22 deletions(-) create mode 100644 changelogs/unreleased/53966-hashed-storage-read-only.yml 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 -- cgit v1.2.1