Merge branch 'disable-object-storage-for-export-rake-task' into 'master'

Disable object_storage for export task

Closes #216018

See merge request gitlab-org/gitlab!30637

lib/gitlab/import_export/project/base_task.rb

@@ -26,6 +26,22 @@ module Gitlab
           }
         end
 
+        def disable_upload_object_storage
+          overwrite_uploads_setting('enabled', false) do
+            yield
+          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)
 

lib/gitlab/import_export/project/export_task.rb

@@ -19,7 +19,11 @@ module Gitlab
               .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!')
+        rescue Gitlab::ImportExport::Error => e
+          error(e.message)
         end
 
         private
@@ -32,6 +36,10 @@ module Gitlab
 
         def with_export
           with_request_store do
+            # 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
@@ -40,4 +48,5 @@ module Gitlab
         end
       end
     end
+  end
 end

lib/gitlab/import_export/project/import_task.rb

@@ -67,24 +67,6 @@ module Gitlab
           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

spec/lib/gitlab/import_export/project/export_task_spec.rb

@@ -10,6 +10,7 @@ describe Gitlab::ImportExport::Project::ExportTask do
   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 @@ describe Gitlab::ImportExport::Project::ExportTask do
     }
   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 @@ describe Gitlab::ImportExport::Project::ExportTask do
     around do |example|
       example.run
     ensure
-      File.delete(file_path)
+      File.delete(file_path) if File.exist?(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 @@ describe Gitlab::ImportExport::Project::ExportTask do
 
       expect(File).to exist(file_path)
     end
-
-    it_behaves_like 'measurable'
   end
 
   context 'when project is not found' do
@@ -66,4 +69,32 @@ describe Gitlab::ImportExport::Project::ExportTask do
       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

spec/lib/gitlab/import_export/project/import_task_spec.rb

@@ -8,7 +8,7 @@ describe Gitlab::ImportExport::Project::ImportTask, :request_store do
   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 @@ describe Gitlab::ImportExport::Project::ImportTask, :request_store do
     }
   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 @@ describe Gitlab::ImportExport::Project::ImportTask, :request_store do
       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
 

spec/support/shared_contexts/lib/gitlab/import_export/project/rake_task_object_storage_shared_context.rb

+# 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_object_store_setting = Settings.uploads.object_store['enabled']
+
+    Settings.uploads.object_store['enabled'] = true
+
+    example.run
+
+    Settings.uploads.object_store['enabled'] = old_object_store_setting
+  end
+end

spec/support/shared_examples/lib/gitlab/import_export/project/rake_task_object_storage_shared_examples.rb

+# 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['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['enabled']).to eq(true)
+      expect(Settings.uploads.object_store).not_to receive(:[]=).with('enabled', false)
+
+      m.call(*args)
+    end
+
+    subject
+  end
+end