diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2020-05-06 09:09:36 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2020-05-06 09:09:36 +0300 |
commit | 0c4b9cacd575b3e71e41a13f042062b3adcb4caf (patch) | |
tree | fd06e55f276c1fa42327a41860e328b46a8cb8ee /spec | |
parent | 6c005ae9783ad6752e3082d05e1053d20d8da92a (diff) |
Add latest changes from gitlab-org/gitlab@master
Diffstat (limited to 'spec')
12 files changed, 347 insertions, 48 deletions
diff --git a/spec/factories/project_repository_storage_moves.rb b/spec/factories/project_repository_storage_moves.rb new file mode 100644 index 00000000000..aa8576834eb --- /dev/null +++ b/spec/factories/project_repository_storage_moves.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :project_repository_storage_move, class: 'ProjectRepositoryStorageMove' do + project + + source_storage_name { 'default' } + destination_storage_name { 'default' } + + trait :scheduled do + state { ProjectRepositoryStorageMove.state_machines[:state].states[:scheduled].value } + end + end +end diff --git a/spec/lib/gitlab/exclusive_lease_helpers/sleeping_lock_spec.rb b/spec/lib/gitlab/exclusive_lease_helpers/sleeping_lock_spec.rb new file mode 100644 index 00000000000..8917eeec56f --- /dev/null +++ b/spec/lib/gitlab/exclusive_lease_helpers/sleeping_lock_spec.rb @@ -0,0 +1,102 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::ExclusiveLeaseHelpers::SleepingLock, :clean_gitlab_redis_shared_state do + include ::ExclusiveLeaseHelpers + + let(:timeout) { 1.second } + let(:delay) { 0.1.seconds } + let(:key) { SecureRandom.hex(10) } + + subject { described_class.new(key, timeout: timeout, delay: delay) } + + describe '#retried?' do + before do + stub_exclusive_lease(key, 'uuid') + end + + context 'we have not made any attempts' do + it { is_expected.not_to be_retried } + end + + context 'we just made a single (initial) attempt' do + it 'is not considered a retry' do + subject.send(:try_obtain) + + is_expected.not_to be_retried + end + end + + context 'made multiple attempts' do + it 'is considered a retry' do + 2.times { subject.send(:try_obtain) } + + is_expected.to be_retried + end + end + end + + describe '#obtain' do + context 'when the lease is not held' do + before do + stub_exclusive_lease(key, 'uuid') + end + + it 'obtains the lease on the first attempt, without sleeping' do + expect(subject).not_to receive(:sleep) + + subject.obtain(10) + + expect(subject).not_to be_retried + end + end + + context 'when the lease is held elsewhere' do + let!(:lease) { stub_exclusive_lease_taken(key) } + let(:max_attempts) { 7 } + + it 'retries to obtain a lease and raises an error' do + expect(subject).to receive(:sleep).with(delay).exactly(max_attempts - 1).times + expect(lease).to receive(:try_obtain).exactly(max_attempts).times + + expect { subject.obtain(max_attempts) }.to raise_error('Failed to obtain a lock') + end + + context 'when the delay is computed from the attempt number' do + let(:delay) { ->(n) { 3 * n } } + + it 'uses the computation to determine the sleep length' do + expect(subject).to receive(:sleep).with(3).once + expect(subject).to receive(:sleep).with(6).once + expect(subject).to receive(:sleep).with(9).once + expect(lease).to receive(:try_obtain).exactly(4).times + + expect { subject.obtain(4) }.to raise_error('Failed to obtain a lock') + end + end + + context 'when lease is granted after retry' do + it 'knows that it retried' do + expect(subject).to receive(:sleep).with(delay).exactly(3).times + expect(lease).to receive(:try_obtain).exactly(3).times { nil } + expect(lease).to receive(:try_obtain).once { 'obtained' } + + subject.obtain(max_attempts) + + expect(subject).to be_retried + end + end + end + + describe 'cancel' do + let!(:lease) { stub_exclusive_lease(key, 'uuid') } + + it 'cancels the lease' do + expect(lease).to receive(:cancel) + + subject.cancel + end + end + end +end diff --git a/spec/lib/gitlab/exclusive_lease_helpers_spec.rb b/spec/lib/gitlab/exclusive_lease_helpers_spec.rb index 065468f6b06..9914518cda5 100644 --- a/spec/lib/gitlab/exclusive_lease_helpers_spec.rb +++ b/spec/lib/gitlab/exclusive_lease_helpers_spec.rb @@ -22,9 +22,7 @@ describe Gitlab::ExclusiveLeaseHelpers, :clean_gitlab_redis_shared_state do end context 'when the lease is not obtained yet' do - before do - stub_exclusive_lease(unique_key, 'uuid') - end + let!(:lease) { stub_exclusive_lease(unique_key, 'uuid') } it 'calls the given block' do expect { |b| class_instance.in_lock(unique_key, &b) }.to yield_with_args(false) @@ -37,7 +35,7 @@ describe Gitlab::ExclusiveLeaseHelpers, :clean_gitlab_redis_shared_state do end it 'cancels the exclusive lease after the block' do - expect_to_cancel_exclusive_lease(unique_key, 'uuid') + expect(lease).to receive(:cancel).once subject end @@ -81,11 +79,21 @@ describe Gitlab::ExclusiveLeaseHelpers, :clean_gitlab_redis_shared_state do end end + context 'when we specify no retries' do + let(:options) { { retries: 0 } } + + it 'never sleeps' do + expect(class_instance).not_to receive(:sleep) + + expect { subject }.to raise_error('Failed to obtain a lock') + end + end + context 'when sleep second is specified' do let(:options) { { retries: 1, sleep_sec: 0.05.seconds } } it 'receives the specified argument' do - expect(class_instance).to receive(:sleep).with(0.05.seconds).twice + expect_any_instance_of(Object).to receive(:sleep).with(0.05.seconds).once expect { subject }.to raise_error('Failed to obtain a lock') end @@ -95,9 +103,8 @@ describe Gitlab::ExclusiveLeaseHelpers, :clean_gitlab_redis_shared_state do let(:options) { { retries: 2, sleep_sec: ->(num) { 0.1 + num } } } it 'receives the specified argument' do - expect(class_instance).to receive(:sleep).with(1.1.seconds).once - expect(class_instance).to receive(:sleep).with(2.1.seconds).once - expect(class_instance).to receive(:sleep).with(3.1.seconds).once + expect_any_instance_of(Object).to receive(:sleep).with(1.1.seconds).once + expect_any_instance_of(Object).to receive(:sleep).with(2.1.seconds).once expect { subject }.to raise_error('Failed to obtain a lock') end diff --git a/spec/lib/gitlab/exclusive_lease_spec.rb b/spec/lib/gitlab/exclusive_lease_spec.rb index 0739f622af5..2c0bb23a0b6 100644 --- a/spec/lib/gitlab/exclusive_lease_spec.rb +++ b/spec/lib/gitlab/exclusive_lease_spec.rb @@ -21,6 +21,27 @@ describe Gitlab::ExclusiveLease, :clean_gitlab_redis_shared_state do end end + describe '.redis_shared_state_key' do + it 'provides a namespaced key' do + expect(described_class.redis_shared_state_key(unique_key)) + .to start_with(described_class::PREFIX) + .and include(unique_key) + end + end + + describe '.ensure_prefixed_key' do + it 'does not double prefix a key' do + prefixed = described_class.redis_shared_state_key(unique_key) + + expect(described_class.ensure_prefixed_key(unique_key)) + .to eq(described_class.ensure_prefixed_key(prefixed)) + end + + it 'raises errors when there is no key' do + expect { described_class.ensure_prefixed_key(nil) }.to raise_error(described_class::NoKey) + end + end + describe '#renew' do it 'returns true when we have the existing lease' do lease = described_class.new(unique_key, timeout: 3600) @@ -61,18 +82,61 @@ describe Gitlab::ExclusiveLease, :clean_gitlab_redis_shared_state do end end - describe '.cancel' do - it 'can cancel a lease' do - uuid = new_lease(unique_key) - expect(uuid).to be_present - expect(new_lease(unique_key)).to eq(false) + describe 'cancellation' do + def new_lease(key) + described_class.new(key, timeout: 3600) + end - described_class.cancel(unique_key, uuid) - expect(new_lease(unique_key)).to be_present + shared_examples 'cancelling a lease' do + let(:lease) { new_lease(unique_key) } + + it 'releases the held lease' do + uuid = lease.try_obtain + expect(uuid).to be_present + expect(new_lease(unique_key).try_obtain).to eq(false) + + cancel_lease(uuid) + + expect(new_lease(unique_key).try_obtain).to be_present + end end - def new_lease(key) - described_class.new(key, timeout: 3600).try_obtain + describe '.cancel' do + def cancel_lease(uuid) + described_class.cancel(release_key, uuid) + end + + context 'when called with the unprefixed key' do + it_behaves_like 'cancelling a lease' do + let(:release_key) { unique_key } + end + end + + context 'when called with the prefixed key' do + it_behaves_like 'cancelling a lease' do + let(:release_key) { described_class.redis_shared_state_key(unique_key) } + end + end + + it 'does not raise errors when given a nil key' do + expect { described_class.cancel(nil, nil) }.not_to raise_error + end + end + + describe '#cancel' do + def cancel_lease(_uuid) + lease.cancel + end + + it_behaves_like 'cancelling a lease' + + it 'is safe to call even if the lease was never obtained' do + lease = new_lease(unique_key) + + lease.cancel + + expect(new_lease(unique_key).try_obtain).to be_present + end end end diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index 562351f8fb9..d2790a6b858 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -489,6 +489,7 @@ project: - compliance_framework_setting - metrics_users_starred_dashboards - alert_management_alerts +- repository_storage_moves award_emoji: - awardable - user diff --git a/spec/models/project_repository_storage_move_spec.rb b/spec/models/project_repository_storage_move_spec.rb new file mode 100644 index 00000000000..b25f4dfd634 --- /dev/null +++ b/spec/models/project_repository_storage_move_spec.rb @@ -0,0 +1,63 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ProjectRepositoryStorageMove, type: :model do + describe 'associations' do + it { is_expected.to belong_to(:project) } + end + + describe 'validations' do + it { is_expected.to validate_presence_of(:project) } + it { is_expected.to validate_presence_of(:state) } + it { is_expected.to validate_presence_of(:source_storage_name) } + it { is_expected.to validate_presence_of(:destination_storage_name) } + + context 'source_storage_name inclusion' do + subject { build(:project_repository_storage_move, source_storage_name: 'missing') } + + it "does not allow repository storages that don't match a label in the configuration" do + expect(subject).not_to be_valid + expect(subject.errors[:source_storage_name].first).to match(/is not included in the list/) + end + end + + context 'destination_storage_name inclusion' do + subject { build(:project_repository_storage_move, destination_storage_name: 'missing') } + + it "does not allow repository storages that don't match a label in the configuration" do + expect(subject).not_to be_valid + expect(subject.errors[:destination_storage_name].first).to match(/is not included in the list/) + end + end + end + + describe 'state transitions' do + using RSpec::Parameterized::TableSyntax + + context 'when in the default state' do + subject(:storage_move) { create(:project_repository_storage_move, project: project, destination_storage_name: 'test_second_storage') } + + let(:project) { create(:project) } + + before do + stub_storage_settings('test_second_storage' => { 'path' => 'tmp/tests/extra_storage' }) + end + + context 'and transits to scheduled' do + it 'triggers ProjectUpdateRepositoryStorageWorker' do + expect(ProjectUpdateRepositoryStorageWorker).to receive(:perform_async).with(project.id, 'test_second_storage', repository_storage_move_id: storage_move.id) + + storage_move.schedule! + end + end + + context 'and transits to started' do + it 'does not allow the transition' do + expect { storage_move.start! } + .to raise_error(StateMachines::InvalidTransition) + end + end + end + end +end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 8c2323eb0d8..b8c814a8ae1 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -115,6 +115,7 @@ describe Project do it { is_expected.to have_many(:alert_management_alerts) } it { is_expected.to have_many(:jira_imports) } it { is_expected.to have_many(:metrics_users_starred_dashboards).inverse_of(:project) } + it { is_expected.to have_many(:repository_storage_moves) } it_behaves_like 'model with repository' do let_it_be(:container) { create(:project, :repository, path: 'somewhere') } @@ -2837,12 +2838,16 @@ describe Project do end it 'schedules the transfer of the repository to the new storage and locks the project' do - expect(ProjectUpdateRepositoryStorageWorker).to receive(:perform_async).with(project.id, 'test_second_storage') + expect(ProjectUpdateRepositoryStorageWorker).to receive(:perform_async).with(project.id, 'test_second_storage', repository_storage_move_id: anything) project.change_repository_storage('test_second_storage') project.save! expect(project).to be_repository_read_only + expect(project.repository_storage_moves.last).to have_attributes( + source_storage_name: "default", + destination_storage_name: "test_second_storage" + ) end it "doesn't schedule the transfer if the repository is already read-only" do diff --git a/spec/services/projects/fork_service_spec.rb b/spec/services/projects/fork_service_spec.rb index c8354f6ba4e..112a41c773b 100644 --- a/spec/services/projects/fork_service_spec.rb +++ b/spec/services/projects/fork_service_spec.rb @@ -320,7 +320,13 @@ describe Projects::ForkService do allow_any_instance_of(Gitlab::Git::Repository).to receive(:checksum) .and_return(::Gitlab::Git::BLANK_SHA) - Projects::UpdateRepositoryStorageService.new(project).execute('test_second_storage') + storage_move = create( + :project_repository_storage_move, + :scheduled, + project: project, + destination_storage_name: 'test_second_storage' + ) + Projects::UpdateRepositoryStorageService.new(storage_move).execute fork_after_move = fork_project(project) pool_repository_before_move = PoolRepository.joins(:shard) .find_by(source_project: project, shards: { name: 'default' }) diff --git a/spec/services/projects/update_repository_storage_service_spec.rb b/spec/services/projects/update_repository_storage_service_spec.rb index 97df388fc15..b58dac9a6ce 100644 --- a/spec/services/projects/update_repository_storage_service_spec.rb +++ b/spec/services/projects/update_repository_storage_service_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' describe Projects::UpdateRepositoryStorageService do include Gitlab::ShellAdapter - subject { described_class.new(project) } + subject { described_class.new(repository_storage_move) } describe "#execute" do let(:time) { Time.now } @@ -17,6 +17,8 @@ describe Projects::UpdateRepositoryStorageService do context 'without wiki and design repository' do let(:project) { create(:project, :repository, repository_read_only: true, wiki_enabled: false) } + let(:destination) { 'test_second_storage' } + let(:repository_storage_move) { create(:project_repository_storage_move, :scheduled, project: project, destination_storage_name: destination) } let!(:checksum) { project.repository.checksum } let(:project_repository_double) { double(:repository) } @@ -42,9 +44,9 @@ describe Projects::UpdateRepositoryStorageService do expect(project_repository_double).to receive(:checksum) .and_return(checksum) - result = subject.execute('test_second_storage') + result = subject.execute - expect(result[:status]).to eq(:success) + expect(result).to be_success expect(project).not_to be_repository_read_only expect(project.repository_storage).to eq('test_second_storage') expect(gitlab_shell.repository_exists?('default', old_path)).to be(false) @@ -53,11 +55,13 @@ describe Projects::UpdateRepositoryStorageService do end context 'when the filesystems are the same' do + let(:destination) { project.repository_storage } + it 'bails out and does nothing' do - result = subject.execute(project.repository_storage) + result = subject.execute - expect(result[:status]).to eq(:error) - expect(result[:message]).to match(/SameFilesystemError/) + expect(result).to be_error + expect(result.message).to match(/SameFilesystemError/) end end @@ -73,9 +77,9 @@ describe Projects::UpdateRepositoryStorageService do .and_raise(Gitlab::Git::CommandError) expect(GitlabShellWorker).not_to receive(:perform_async) - result = subject.execute('test_second_storage') + result = subject.execute - expect(result[:status]).to eq(:error) + expect(result).to be_error expect(project).not_to be_repository_read_only expect(project.repository_storage).to eq('default') end @@ -94,9 +98,9 @@ describe Projects::UpdateRepositoryStorageService do .and_return('not matching checksum') expect(GitlabShellWorker).not_to receive(:perform_async) - result = subject.execute('test_second_storage') + result = subject.execute - expect(result[:status]).to eq(:error) + expect(result).to be_error expect(project).not_to be_repository_read_only expect(project.repository_storage).to eq('default') end @@ -116,9 +120,9 @@ describe Projects::UpdateRepositoryStorageService do expect(project_repository_double).to receive(:checksum) .and_return(checksum) - result = subject.execute('test_second_storage') + result = subject.execute - expect(result[:status]).to eq(:success) + expect(result).to be_success expect(project.repository_storage).to eq('test_second_storage') expect(project.reload_pool_repository).to be_nil end @@ -129,6 +133,8 @@ describe Projects::UpdateRepositoryStorageService do include_examples 'moves repository to another storage', 'wiki' do let(:project) { create(:project, :repository, repository_read_only: true, wiki_enabled: true) } let(:repository) { project.wiki.repository } + let(:destination) { 'test_second_storage' } + let(:repository_storage_move) { create(:project_repository_storage_move, :scheduled, project: project, destination_storage_name: destination) } before do project.create_wiki diff --git a/spec/support/helpers/exclusive_lease_helpers.rb b/spec/support/helpers/exclusive_lease_helpers.rb index 77703e20602..076e3cc3851 100644 --- a/spec/support/helpers/exclusive_lease_helpers.rb +++ b/spec/support/helpers/exclusive_lease_helpers.rb @@ -9,7 +9,8 @@ module ExclusiveLeaseHelpers Gitlab::ExclusiveLease, try_obtain: uuid, exists?: true, - renew: renew + renew: renew, + cancel: nil ) allow(Gitlab::ExclusiveLease) diff --git a/spec/support/shared_examples/services/projects/update_repository_storage_service_shared_examples.rb b/spec/support/shared_examples/services/projects/update_repository_storage_service_shared_examples.rb index d6166ac8188..0e6ecf49cd0 100644 --- a/spec/support/shared_examples/services/projects/update_repository_storage_service_shared_examples.rb +++ b/spec/support/shared_examples/services/projects/update_repository_storage_service_shared_examples.rb @@ -47,9 +47,9 @@ RSpec.shared_examples 'moves repository to another storage' do |repository_type| old_repository_path = repository.full_path - result = subject.execute('test_second_storage') + result = subject.execute - expect(result[:status]).to eq(:success) + expect(result).to be_success expect(project).not_to be_repository_read_only expect(project.repository_storage).to eq('test_second_storage') expect(gitlab_shell.repository_exists?('default', old_project_repository_path)).to be(false) @@ -62,7 +62,7 @@ RSpec.shared_examples 'moves repository to another storage' do |repository_type| end it 'does not enqueue a GC run' do - expect { subject.execute('test_second_storage') } + expect { subject.execute } .not_to change(GitGarbageCollectWorker.jobs, :count) end end @@ -75,23 +75,25 @@ RSpec.shared_examples 'moves repository to another storage' do |repository_type| it 'does not enqueue a GC run if housekeeping is disabled' do stub_application_setting(housekeeping_enabled: false) - expect { subject.execute('test_second_storage') } + expect { subject.execute } .not_to change(GitGarbageCollectWorker.jobs, :count) end it 'enqueues a GC run' do - expect { subject.execute('test_second_storage') } + expect { subject.execute } .to change(GitGarbageCollectWorker.jobs, :count).by(1) end end end context 'when the filesystems are the same' do + let(:destination) { project.repository_storage } + it 'bails out and does nothing' do - result = subject.execute(project.repository_storage) + result = subject.execute - expect(result[:status]).to eq(:error) - expect(result[:message]).to match(/SameFilesystemError/) + expect(result).to be_error + expect(result.message).to match(/SameFilesystemError/) end end @@ -114,9 +116,9 @@ RSpec.shared_examples 'moves repository to another storage' do |repository_type| expect(GitlabShellWorker).not_to receive(:perform_async) - result = subject.execute('test_second_storage') + result = subject.execute - expect(result[:status]).to eq(:error) + expect(result).to be_error expect(project).not_to be_repository_read_only expect(project.repository_storage).to eq('default') end @@ -142,9 +144,9 @@ RSpec.shared_examples 'moves repository to another storage' do |repository_type| expect(GitlabShellWorker).not_to receive(:perform_async) - result = subject.execute('test_second_storage') + result = subject.execute - expect(result[:status]).to eq(:error) + expect(result).to be_error expect(project).not_to be_repository_read_only expect(project.repository_storage).to eq('default') end diff --git a/spec/workers/project_update_repository_storage_worker_spec.rb b/spec/workers/project_update_repository_storage_worker_spec.rb index 57a4c2128b3..31d0d955759 100644 --- a/spec/workers/project_update_repository_storage_worker_spec.rb +++ b/spec/workers/project_update_repository_storage_worker_spec.rb @@ -9,12 +9,40 @@ describe ProjectUpdateRepositoryStorageWorker do subject { described_class.new } describe "#perform" do - it "calls the update repository storage service" do - expect_next_instance_of(Projects::UpdateRepositoryStorageService) do |instance| - expect(instance).to receive(:execute).with('new_storage') + let(:service) { double(:update_repository_storage_service) } + + before do + allow(Gitlab.config.repositories.storages).to receive(:keys).and_return(%w[default test_second_storage]) + end + + context 'without repository storage move' do + it "calls the update repository storage service" do + expect(Projects::UpdateRepositoryStorageService).to receive(:new).and_return(service) + expect(service).to receive(:execute) + + expect do + subject.perform(project.id, 'test_second_storage') + end.to change(ProjectRepositoryStorageMove, :count).by(1) + + storage_move = project.repository_storage_moves.last + expect(storage_move).to have_attributes( + source_storage_name: "default", + destination_storage_name: "test_second_storage" + ) end + end + + context 'with repository storage move' do + let!(:repository_storage_move) { create(:project_repository_storage_move) } - subject.perform(project.id, 'new_storage') + it "calls the update repository storage service" do + expect(Projects::UpdateRepositoryStorageService).to receive(:new).and_return(service) + expect(service).to receive(:execute) + + expect do + subject.perform(nil, nil, repository_storage_move_id: repository_storage_move.id) + end.not_to change(ProjectRepositoryStorageMove, :count) + end end end end |