Commit 849cee1a authored by James Fargher's avatar James Fargher

Move SameFilesystemError into service

Previously this error was raised in a job which would cause sidekiq to
retry, and not be logged consistently.
parent f8b27591
......@@ -5,12 +5,15 @@ module Projects
include Gitlab::ShellAdapter
Error = Class.new(StandardError)
SameFilesystemError = Class.new(Error)
def initialize(project)
@project = project
end
def execute(new_repository_storage_key)
raise SameFilesystemError if same_filesystem?(project.repository.storage, new_repository_storage_key)
mirror_repositories(new_repository_storage_key)
mark_old_paths_for_archive
......@@ -33,6 +36,10 @@ module Projects
private
def same_filesystem?(old_storage, new_storage)
Gitlab::GitalyClient.filesystem_id(old_storage) == Gitlab::GitalyClient.filesystem_id(new_storage)
end
def mirror_repositories(new_repository_storage_key)
mirror_repository(new_repository_storage_key)
......
......@@ -3,21 +3,11 @@
class ProjectUpdateRepositoryStorageWorker # rubocop:disable Scalability/IdempotentWorker
include ApplicationWorker
SameFilesystemError = Class.new(StandardError)
feature_category :gitaly
def perform(project_id, new_repository_storage_key)
project = Project.find(project_id)
raise SameFilesystemError if same_filesystem?(project.repository.storage, new_repository_storage_key)
::Projects::UpdateRepositoryStorageService.new(project).execute(new_repository_storage_key)
end
private
def same_filesystem?(old_storage, new_storage)
Gitlab::GitalyClient.filesystem_id(old_storage) == Gitlab::GitalyClient.filesystem_id(new_storage)
end
end
......@@ -311,6 +311,8 @@ describe Projects::ForkService do
fork_before_move = fork_project(project)
# Stub everything required to move a project to a Gitaly shard that does not exist
allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('default').and_call_original
allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('test_second_storage').and_return(SecureRandom.uuid)
stub_storage_settings('test_second_storage' => { 'path' => TestEnv::SECOND_STORAGE_PATH })
allow_any_instance_of(Gitlab::Git::Repository).to receive(:create_repository)
.and_return(true)
......
......@@ -20,6 +20,8 @@ describe Projects::UpdateRepositoryStorageService do
let(:project_repository_double) { double(:repository) }
before do
allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('default').and_call_original
allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('test_second_storage').and_return(SecureRandom.uuid)
allow(Gitlab::Git::Repository).to receive(:new).and_call_original
allow(Gitlab::Git::Repository).to receive(:new)
.with('test_second_storage', project.repository.raw.relative_path, project.repository.gl_repository, project.repository.full_path)
......@@ -49,17 +51,20 @@ describe Projects::UpdateRepositoryStorageService do
end
end
context 'when the project is already on the target storage' do
context 'when the filesystems are the same' do
it 'bails out and does nothing' do
result = subject.execute(project.repository_storage)
expect(result[:status]).to eq(:error)
expect(result[:message]).to match(/repository and source have the same storage/)
expect(result[:message]).to match(/SameFilesystemError/)
end
end
context 'when the move fails' do
it 'unmarks the repository as read-only without updating the repository storage' do
allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('default').and_call_original
allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('test_second_storage').and_return(SecureRandom.uuid)
expect(project_repository_double).to receive(:create_repository)
.and_return(true)
expect(project_repository_double).to receive(:replicate)
......@@ -77,6 +82,9 @@ describe Projects::UpdateRepositoryStorageService do
context 'when the checksum does not match' do
it 'unmarks the repository as read-only without updating the repository storage' do
allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('default').and_call_original
allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('test_second_storage').and_return(SecureRandom.uuid)
expect(project_repository_double).to receive(:create_repository)
.and_return(true)
expect(project_repository_double).to receive(:replicate)
......@@ -97,6 +105,9 @@ describe Projects::UpdateRepositoryStorageService do
let!(:pool) { create(:pool_repository, :ready, source_project: project) }
it 'leaves the pool' do
allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('default').and_call_original
allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('test_second_storage').and_return(SecureRandom.uuid)
expect(project_repository_double).to receive(:create_repository)
.and_return(true)
expect(project_repository_double).to receive(:replicate)
......
......@@ -22,6 +22,9 @@ RSpec.shared_examples 'moves repository to another storage' do |repository_type|
context 'when the move succeeds', :clean_gitlab_redis_shared_state do
before do
allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('default').and_call_original
allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('test_second_storage').and_return(SecureRandom.uuid)
allow(project_repository_double).to receive(:create_repository)
.and_return(true)
allow(project_repository_double).to receive(:replicate)
......@@ -83,17 +86,19 @@ RSpec.shared_examples 'moves repository to another storage' do |repository_type|
end
end
context 'when the project is already on the target storage' do
context 'when the filesystems are the same' do
it 'bails out and does nothing' do
result = subject.execute(project.repository_storage)
expect(result[:status]).to eq(:error)
expect(result[:message]).to match(/repository and source have the same storage/)
expect(result[:message]).to match(/SameFilesystemError/)
end
end
context "when the move of the #{repository_type} repository fails" do
it 'unmarks the repository as read-only without updating the repository storage' do
allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('default').and_call_original
allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('test_second_storage').and_return(SecureRandom.uuid)
allow(project_repository_double).to receive(:create_repository)
.and_return(true)
allow(project_repository_double).to receive(:replicate)
......@@ -119,6 +124,8 @@ RSpec.shared_examples 'moves repository to another storage' do |repository_type|
context "when the checksum of the #{repository_type} repository does not match" do
it 'unmarks the repository as read-only without updating the repository storage' do
allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('default').and_call_original
allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('test_second_storage').and_return(SecureRandom.uuid)
allow(project_repository_double).to receive(:create_repository)
.and_return(true)
allow(project_repository_double).to receive(:replicate)
......
......@@ -9,33 +9,12 @@ describe ProjectUpdateRepositoryStorageWorker do
subject { described_class.new }
describe "#perform" do
context 'when source and target repositories are on different filesystems' do
before do
allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('default').and_call_original
allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('new_storage').and_return(SecureRandom.uuid)
it "calls the update repository storage service" do
expect_next_instance_of(Projects::UpdateRepositoryStorageService) do |instance|
expect(instance).to receive(:execute).with('new_storage')
end
it "calls the update repository storage service" do
expect_next_instance_of(Projects::UpdateRepositoryStorageService) do |instance|
expect(instance).to receive(:execute).with('new_storage')
end
subject.perform(project.id, 'new_storage')
end
end
context 'when source and target repositories are on the same filesystems' do
let(:filesystem_id) { SecureRandom.uuid }
before do
allow(Gitlab::GitalyClient).to receive(:filesystem_id).and_return(filesystem_id)
end
it 'raises an error' do
expect_any_instance_of(::Projects::UpdateRepositoryStorageService).not_to receive(:new)
expect { subject.perform(project.id, 'new_storage') }.to raise_error(ProjectUpdateRepositoryStorageWorker::SameFilesystemError)
end
subject.perform(project.id, 'new_storage')
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment