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).
This commit is contained in:
parent
7380364240
commit
ee4af0c64c
|
@ -1786,6 +1786,24 @@ class Project < ActiveRecord::Base
|
||||||
handle_update_attribute_error(e, value)
|
handle_update_attribute_error(e, value)
|
||||||
end
|
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
|
def pushes_since_gc
|
||||||
Gitlab::Redis::SharedState.with { |redis| redis.get(pushes_since_gc_redis_shared_state_key).to_i }
|
Gitlab::Redis::SharedState.with { |redis| redis.get(pushes_since_gc_redis_shared_state_key).to_i }
|
||||||
end
|
end
|
||||||
|
@ -1900,15 +1918,17 @@ class Project < ActiveRecord::Base
|
||||||
def migrate_to_hashed_storage!
|
def migrate_to_hashed_storage!
|
||||||
return unless storage_upgradable?
|
return unless storage_upgradable?
|
||||||
|
|
||||||
update!(repository_read_only: true)
|
if git_transfer_in_progress?
|
||||||
|
|
||||||
if repo_reference_count > 0 || wiki_reference_count > 0
|
|
||||||
ProjectMigrateHashedStorageWorker.perform_in(Gitlab::ReferenceCounter::REFERENCE_EXPIRE_TIME, id)
|
ProjectMigrateHashedStorageWorker.perform_in(Gitlab::ReferenceCounter::REFERENCE_EXPIRE_TIME, id)
|
||||||
else
|
else
|
||||||
ProjectMigrateHashedStorageWorker.perform_async(id)
|
ProjectMigrateHashedStorageWorker.perform_async(id)
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
def git_transfer_in_progress?
|
||||||
|
repo_reference_count > 0 || wiki_reference_count > 0
|
||||||
|
end
|
||||||
|
|
||||||
def storage_version=(value)
|
def storage_version=(value)
|
||||||
super
|
super
|
||||||
|
|
||||||
|
|
|
@ -2,6 +2,8 @@
|
||||||
|
|
||||||
module Projects
|
module Projects
|
||||||
module HashedStorage
|
module HashedStorage
|
||||||
|
RepositoryMigrationError = Class.new(StandardError)
|
||||||
|
|
||||||
class MigrateRepositoryService < BaseService
|
class MigrateRepositoryService < BaseService
|
||||||
include Gitlab::ShellAdapter
|
include Gitlab::ShellAdapter
|
||||||
|
|
||||||
|
@ -16,6 +18,8 @@ module Projects
|
||||||
end
|
end
|
||||||
|
|
||||||
def execute
|
def execute
|
||||||
|
try_to_set_repository_read_only!
|
||||||
|
|
||||||
@old_storage_version = project.storage_version
|
@old_storage_version = project.storage_version
|
||||||
project.storage_version = ::Project::HASHED_STORAGE_FEATURES[:repository]
|
project.storage_version = ::Project::HASHED_STORAGE_FEATURES[:repository]
|
||||||
project.ensure_storage_path_exists
|
project.ensure_storage_path_exists
|
||||||
|
@ -48,6 +52,16 @@ module Projects
|
||||||
|
|
||||||
private
|
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
|
# rubocop: disable CodeReuse/ActiveRecord
|
||||||
def has_wiki?
|
def has_wiki?
|
||||||
gitlab_shell.exists?(project.repository_storage, "#{old_wiki_disk_path}.git")
|
gitlab_shell.exists?(project.repository_storage, "#{old_wiki_disk_path}.git")
|
||||||
|
|
|
@ -0,0 +1,5 @@
|
||||||
|
---
|
||||||
|
title: 'Hashed Storage: Only set as `read_only` when starting the per-project migration'
|
||||||
|
merge_request: 24128
|
||||||
|
author:
|
||||||
|
type: changed
|
|
@ -19,15 +19,6 @@ describe Gitlab::HashedStorage::Migrator do
|
||||||
end
|
end
|
||||||
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
|
it 'rescues and log exceptions' do
|
||||||
allow_any_instance_of(Project).to receive(:migrate_to_hashed_storage!).and_raise(StandardError)
|
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
|
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)
|
subject.bulk_migrate(ids.min, ids.max)
|
||||||
end
|
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
|
end
|
||||||
|
|
||||||
describe '#migrate' do
|
describe '#migrate' do
|
||||||
|
@ -57,13 +58,6 @@ describe Gitlab::HashedStorage::Migrator do
|
||||||
expect { subject.migrate(project) }.not_to raise_error
|
expect { subject.migrate(project) }.not_to raise_error
|
||||||
end
|
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
|
it 'migrate project' do
|
||||||
perform_enqueued_jobs do
|
perform_enqueued_jobs do
|
||||||
subject.migrate(project)
|
subject.migrate(project)
|
||||||
|
@ -71,5 +65,13 @@ describe Gitlab::HashedStorage::Migrator do
|
||||||
|
|
||||||
expect(project.reload.hashed_storage?(:attachments)).to be_truthy
|
expect(project.reload.hashed_storage?(:attachments)).to be_truthy
|
||||||
end
|
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
|
||||||
end
|
end
|
||||||
|
|
|
@ -2410,6 +2410,20 @@ describe Project do
|
||||||
end
|
end
|
||||||
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
|
describe '#pushes_since_gc' do
|
||||||
let(:project) { create(:project) }
|
let(:project) { create(:project) }
|
||||||
|
|
||||||
|
@ -3143,6 +3157,33 @@ describe Project do
|
||||||
end
|
end
|
||||||
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
|
context 'legacy storage' do
|
||||||
let(:project) { create(:project, :repository, :legacy_storage) }
|
let(:project) { create(:project, :repository, :legacy_storage) }
|
||||||
let(:gitlab_shell) { Gitlab::Shell.new }
|
let(:gitlab_shell) { Gitlab::Shell.new }
|
||||||
|
@ -3203,10 +3244,6 @@ describe Project do
|
||||||
expect(project.migrate_to_hashed_storage!).to be_truthy
|
expect(project.migrate_to_hashed_storage!).to be_truthy
|
||||||
end
|
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
|
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_as_fork)
|
||||||
expect(project).not_to receive(:visibility_level_allowed_by_group)
|
expect(project).not_to receive(:visibility_level_allowed_by_group)
|
||||||
|
|
|
@ -15,6 +15,20 @@ describe Projects::HashedStorage::MigrateRepositoryService do
|
||||||
allow(service).to receive(:gitlab_shell) { gitlab_shell }
|
allow(service).to receive(:gitlab_shell) { gitlab_shell }
|
||||||
end
|
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
|
context 'when succeeds' do
|
||||||
it 'renames project and wiki repositories' do
|
it 'renames project and wiki repositories' do
|
||||||
service.execute
|
service.execute
|
||||||
|
|
Loading…
Reference in New Issue