From beff16e3b6ce637d8c16390070aa0abe3198d0ae Mon Sep 17 00:00:00 2001 From: nmilojevic1 Date: Tue, 28 Apr 2020 16:55:55 +0200 Subject: [PATCH 1/3] Disable object_storage for export task - log error if there are import_export_shared errors - extract disable_upload_object_storage for both import and export task --- lib/gitlab/import_export/project/base_task.rb | 18 ++++++++++++++++++ .../import_export/project/export_task.rb | 11 +++++++++-- .../import_export/project/import_task.rb | 18 ------------------ 3 files changed, 27 insertions(+), 20 deletions(-) diff --git a/lib/gitlab/import_export/project/base_task.rb b/lib/gitlab/import_export/project/base_task.rb index 5c75eca2bed393..f11fb6915cf96f 100644 --- a/lib/gitlab/import_export/project/base_task.rb +++ b/lib/gitlab/import_export/project/base_task.rb @@ -26,6 +26,24 @@ def measurement_options } end + def disable_upload_object_storage + overwrite_uploads_setting('background_upload', false) do + overwrite_uploads_setting('direct_upload', false) do + yield + end + end + end + + def overwrite_uploads_setting(key, value) + old_value = Settings.uploads.object_store[key] + Settings.uploads.object_store[key] = value + + yield + + ensure + Settings.uploads.object_store[key] = old_value + end + def success(message) logger.info(message) diff --git a/lib/gitlab/import_export/project/export_task.rb b/lib/gitlab/import_export/project/export_task.rb index 22efd0cb8a3e4a..2ead9774347918 100644 --- a/lib/gitlab/import_export/project/export_task.rb +++ b/lib/gitlab/import_export/project/export_task.rb @@ -19,6 +19,8 @@ def export .execute(Gitlab::ImportExport::AfterExportStrategies::MoveFileStrategy.new(archive_path: file_path), measurement_options) end + return error(project.import_export_shared.errors.join(', ')) if project.import_export_shared.errors.any? + success('Done!') end @@ -32,8 +34,13 @@ def file_path_exists? def with_export with_request_store do - ::Gitlab::GitalyClient.allow_n_plus_1_calls do - yield + # We are disabling ObjectStorage for `export` + # since when direct upload is enabled, remote storage will be used + # and Gitlab::ImportExport::AfterExportStrategies::MoveFileStrategy will fail to copy exported archive + disable_upload_object_storage do + ::Gitlab::GitalyClient.allow_n_plus_1_calls do + yield + end end end end diff --git a/lib/gitlab/import_export/project/import_task.rb b/lib/gitlab/import_export/project/import_task.rb index 101cc5f5ab0a52..20f7ac1eb18781 100644 --- a/lib/gitlab/import_export/project/import_task.rb +++ b/lib/gitlab/import_export/project/import_task.rb @@ -67,24 +67,6 @@ def execute_sidekiq_job Sidekiq::Worker.drain_all end - def disable_upload_object_storage - overwrite_uploads_setting('background_upload', false) do - overwrite_uploads_setting('direct_upload', false) do - yield - end - end - end - - def overwrite_uploads_setting(key, value) - old_value = Settings.uploads.object_store[key] - Settings.uploads.object_store[key] = value - - yield - - ensure - Settings.uploads.object_store[key] = old_value - end - def full_path "#{namespace.full_path}/#{project_path}" end -- GitLab From ce1893e3a5903c2a22030d00b66d73c3ef63d2f5 Mon Sep 17 00:00:00 2001 From: nmilojevic1 Date: Wed, 29 Apr 2020 13:28:24 +0200 Subject: [PATCH 2/3] Add specs for object_storage - Extract to shared context and shared example - Fix import_rake_task_spec - Rescue Exception in export_rake task --- .../import_export/project/export_task.rb | 2 + .../import_export/project/export_task_spec.rb | 39 +++++++++++++-- .../import_export/project/import_task_spec.rb | 47 +++---------------- ...rake_task_object_storage_shared_context.rb | 20 ++++++++ ...ake_task_object_storage_shared_examples.rb | 25 ++++++++++ 5 files changed, 88 insertions(+), 45 deletions(-) create mode 100644 spec/support/shared_contexts/lib/gitlab/import_export/project/rake_task_object_storage_shared_context.rb create mode 100644 spec/support/shared_examples/lib/gitlab/import_export/project/rake_task_object_storage_shared_examples.rb diff --git a/lib/gitlab/import_export/project/export_task.rb b/lib/gitlab/import_export/project/export_task.rb index 2ead9774347918..4919ace0742fe2 100644 --- a/lib/gitlab/import_export/project/export_task.rb +++ b/lib/gitlab/import_export/project/export_task.rb @@ -22,6 +22,8 @@ def export return error(project.import_export_shared.errors.join(', ')) if project.import_export_shared.errors.any? success('Done!') + rescue Gitlab::ImportExport::Error => e + error(e.message) end private diff --git a/spec/lib/gitlab/import_export/project/export_task_spec.rb b/spec/lib/gitlab/import_export/project/export_task_spec.rb index 7c091c4363798c..eb446ebeea2997 100644 --- a/spec/lib/gitlab/import_export/project/export_task_spec.rb +++ b/spec/lib/gitlab/import_export/project/export_task_spec.rb @@ -10,6 +10,7 @@ let(:file_path) { 'spec/fixtures/gitlab/import_export/test_project_export.tar.gz' } let(:project) { create(:project, creator: user, namespace: user.namespace) } let(:project_name) { project.name } + let(:rake_task) { described_class.new(task_params) } let(:task_params) do { @@ -21,7 +22,7 @@ } end - subject { described_class.new(task_params).export } + subject { rake_task.export } context 'when project is found' do let(:project) { create(:project, creator: user, namespace: user.namespace) } @@ -29,9 +30,13 @@ around do |example| example.run ensure - File.delete(file_path) + File.delete(file_path) if File.exists?(file_path) end + include_context 'rake task object storage shared context' + + it_behaves_like 'rake task with disabled object_storage', ::Projects::ImportExport::ExportService, :success + it 'performs project export successfully' do expect { subject }.to output(/Done!/).to_stdout @@ -39,8 +44,6 @@ expect(File).to exist(file_path) end - - it_behaves_like 'measurable' end context 'when project is not found' do @@ -66,4 +69,32 @@ expect(subject).to eq(false) end end + + context 'when after export strategy fails' do + before do + allow_next_instance_of(Gitlab::ImportExport::AfterExportStrategies::MoveFileStrategy) do |after_export_strategy| + allow(after_export_strategy).to receive(:strategy_execute).and_raise(Gitlab::ImportExport::AfterExportStrategies::BaseAfterExportStrategy::StrategyError) + end + end + + it 'error is logged' do + expect(rake_task).to receive(:error).and_call_original + + expect(subject).to eq(false) + end + end + + context 'when saving services fail' do + before do + allow_next_instance_of(::Projects::ImportExport::ExportService) do |service| + allow(service).to receive(:execute).and_raise(Gitlab::ImportExport::Error) + end + end + + it 'error is logged' do + expect(rake_task).to receive(:error).and_call_original + + expect(subject).to eq(false) + end + end end diff --git a/spec/lib/gitlab/import_export/project/import_task_spec.rb b/spec/lib/gitlab/import_export/project/import_task_spec.rb index 4f4fcd3ad8adf4..d83cc173388e32 100644 --- a/spec/lib/gitlab/import_export/project/import_task_spec.rb +++ b/spec/lib/gitlab/import_export/project/import_task_spec.rb @@ -8,7 +8,7 @@ let!(:user) { create(:user, username: username) } let(:measurement_enabled) { false } let(:project) { Project.find_by_full_path("#{namespace_path}/#{project_name}") } - let(:import_task) { described_class.new(task_params) } + let(:rake_task) { described_class.new(task_params) } let(:task_params) do { username: username, @@ -19,29 +19,16 @@ } end - before do - allow(Settings.uploads.object_store).to receive(:[]=).and_call_original - end - - around do |example| - old_direct_upload_setting = Settings.uploads.object_store['direct_upload'] - old_background_upload_setting = Settings.uploads.object_store['background_upload'] - - Settings.uploads.object_store['direct_upload'] = true - Settings.uploads.object_store['background_upload'] = true - - example.run - - Settings.uploads.object_store['direct_upload'] = old_direct_upload_setting - Settings.uploads.object_store['background_upload'] = old_background_upload_setting - end - - subject { import_task.import } + subject { rake_task.import } context 'when project import is valid' do let(:project_name) { 'import_rake_test_project' } let(:file_path) { 'spec/fixtures/gitlab/import_export/lightweight_project_export.tar.gz' } + include_context 'rake task object storage shared context' + + it_behaves_like 'rake task with disabled object_storage', ::Projects::GitlabProjectsImportService, :execute_sidekiq_job + it 'performs project import successfully' do expect { subject }.to output(/Done!/).to_stdout expect { subject }.not_to raise_error @@ -53,28 +40,6 @@ expect(project.import_state.status).to eq('finished') end - it 'disables direct & background upload only during project creation' do - expect_next_instance_of(Projects::GitlabProjectsImportService) do |service| - expect(service).to receive(:execute).and_wrap_original do |m| - expect(Settings.uploads.object_store['background_upload']).to eq(false) - expect(Settings.uploads.object_store['direct_upload']).to eq(false) - - m.call - end - end - - expect(import_task).to receive(:execute_sidekiq_job).and_wrap_original do |m| - expect(Settings.uploads.object_store['background_upload']).to eq(true) - expect(Settings.uploads.object_store['direct_upload']).to eq(true) - expect(Settings.uploads.object_store).not_to receive(:[]=).with('backgroud_upload', false) - expect(Settings.uploads.object_store).not_to receive(:[]=).with('direct_upload', false) - - m.call - end - - subject - end - it_behaves_like 'measurable' end diff --git a/spec/support/shared_contexts/lib/gitlab/import_export/project/rake_task_object_storage_shared_context.rb b/spec/support/shared_contexts/lib/gitlab/import_export/project/rake_task_object_storage_shared_context.rb new file mode 100644 index 00000000000000..ab8c6f289b2f74 --- /dev/null +++ b/spec/support/shared_contexts/lib/gitlab/import_export/project/rake_task_object_storage_shared_context.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +RSpec.shared_context 'rake task object storage shared context' do + before do + allow(Settings.uploads.object_store).to receive(:[]=).and_call_original + end + + around do |example| + old_direct_upload_setting = Settings.uploads.object_store['direct_upload'] + old_background_upload_setting = Settings.uploads.object_store['background_upload'] + + Settings.uploads.object_store['direct_upload'] = true + Settings.uploads.object_store['background_upload'] = true + + example.run + + Settings.uploads.object_store['direct_upload'] = old_direct_upload_setting + Settings.uploads.object_store['background_upload'] = old_background_upload_setting + end +end diff --git a/spec/support/shared_examples/lib/gitlab/import_export/project/rake_task_object_storage_shared_examples.rb b/spec/support/shared_examples/lib/gitlab/import_export/project/rake_task_object_storage_shared_examples.rb new file mode 100644 index 00000000000000..72b67a006430c3 --- /dev/null +++ b/spec/support/shared_examples/lib/gitlab/import_export/project/rake_task_object_storage_shared_examples.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'rake task with disabled object_storage' do |service_class, method| + it 'disables direct & background upload only for service call' do + expect_next_instance_of(service_class) do |service| + expect(service).to receive(:execute).and_wrap_original do |m| + expect(Settings.uploads.object_store['background_upload']).to eq(false) + expect(Settings.uploads.object_store['direct_upload']).to eq(false) + + m.call + end + end + + expect(rake_task).to receive(method).and_wrap_original do |m, *args| + expect(Settings.uploads.object_store['background_upload']).to eq(true) + expect(Settings.uploads.object_store['direct_upload']).to eq(true) + expect(Settings.uploads.object_store).not_to receive(:[]=).with('backgroud_upload', false) + expect(Settings.uploads.object_store).not_to receive(:[]=).with('direct_upload', false) + + m.call(*args) + end + + subject + end +end -- GitLab From 566e3e7f4b9b6f0b89745a30183681d3032976df Mon Sep 17 00:00:00 2001 From: nmilojevic1 Date: Wed, 29 Apr 2020 14:55:10 +0200 Subject: [PATCH 3/3] Fix disable upload object storage - Fix specs for import export task - Replace File.exists with File.exist --- lib/gitlab/import_export/project/base_task.rb | 6 ++---- .../lib/gitlab/import_export/project/export_task_spec.rb | 2 +- .../project/rake_task_object_storage_shared_context.rb | 9 +++------ .../project/rake_task_object_storage_shared_examples.rb | 9 +++------ 4 files changed, 9 insertions(+), 17 deletions(-) diff --git a/lib/gitlab/import_export/project/base_task.rb b/lib/gitlab/import_export/project/base_task.rb index f11fb6915cf96f..f6afbfcc50dcde 100644 --- a/lib/gitlab/import_export/project/base_task.rb +++ b/lib/gitlab/import_export/project/base_task.rb @@ -27,10 +27,8 @@ def measurement_options end def disable_upload_object_storage - overwrite_uploads_setting('background_upload', false) do - overwrite_uploads_setting('direct_upload', false) do - yield - end + overwrite_uploads_setting('enabled', false) do + yield end end diff --git a/spec/lib/gitlab/import_export/project/export_task_spec.rb b/spec/lib/gitlab/import_export/project/export_task_spec.rb index eb446ebeea2997..dc8eb54dc14573 100644 --- a/spec/lib/gitlab/import_export/project/export_task_spec.rb +++ b/spec/lib/gitlab/import_export/project/export_task_spec.rb @@ -30,7 +30,7 @@ around do |example| example.run ensure - File.delete(file_path) if File.exists?(file_path) + File.delete(file_path) if File.exist?(file_path) end include_context 'rake task object storage shared context' diff --git a/spec/support/shared_contexts/lib/gitlab/import_export/project/rake_task_object_storage_shared_context.rb b/spec/support/shared_contexts/lib/gitlab/import_export/project/rake_task_object_storage_shared_context.rb index ab8c6f289b2f74..dc1a52e362987f 100644 --- a/spec/support/shared_contexts/lib/gitlab/import_export/project/rake_task_object_storage_shared_context.rb +++ b/spec/support/shared_contexts/lib/gitlab/import_export/project/rake_task_object_storage_shared_context.rb @@ -6,15 +6,12 @@ end around do |example| - old_direct_upload_setting = Settings.uploads.object_store['direct_upload'] - old_background_upload_setting = Settings.uploads.object_store['background_upload'] + old_object_store_setting = Settings.uploads.object_store['enabled'] - Settings.uploads.object_store['direct_upload'] = true - Settings.uploads.object_store['background_upload'] = true + Settings.uploads.object_store['enabled'] = true example.run - Settings.uploads.object_store['direct_upload'] = old_direct_upload_setting - Settings.uploads.object_store['background_upload'] = old_background_upload_setting + Settings.uploads.object_store['enabled'] = old_object_store_setting end end diff --git a/spec/support/shared_examples/lib/gitlab/import_export/project/rake_task_object_storage_shared_examples.rb b/spec/support/shared_examples/lib/gitlab/import_export/project/rake_task_object_storage_shared_examples.rb index 72b67a006430c3..d6dc89a2c153a5 100644 --- a/spec/support/shared_examples/lib/gitlab/import_export/project/rake_task_object_storage_shared_examples.rb +++ b/spec/support/shared_examples/lib/gitlab/import_export/project/rake_task_object_storage_shared_examples.rb @@ -4,18 +4,15 @@ it 'disables direct & background upload only for service call' do expect_next_instance_of(service_class) do |service| expect(service).to receive(:execute).and_wrap_original do |m| - expect(Settings.uploads.object_store['background_upload']).to eq(false) - expect(Settings.uploads.object_store['direct_upload']).to eq(false) + expect(Settings.uploads.object_store['enabled']).to eq(false) m.call end end expect(rake_task).to receive(method).and_wrap_original do |m, *args| - expect(Settings.uploads.object_store['background_upload']).to eq(true) - expect(Settings.uploads.object_store['direct_upload']).to eq(true) - expect(Settings.uploads.object_store).not_to receive(:[]=).with('backgroud_upload', false) - expect(Settings.uploads.object_store).not_to receive(:[]=).with('direct_upload', false) + expect(Settings.uploads.object_store['enabled']).to eq(true) + expect(Settings.uploads.object_store).not_to receive(:[]=).with('enabled', false) m.call(*args) end -- GitLab