Merge branch '207986-follow-up-export-rake-task' into 'master'

Resolve "Follow-up from "Export rake task""

Closes #207986

See merge request gitlab-org/gitlab!26100

lib/gitlab/import_export/project/base_task.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module ImportExport
+    module Project
+      class BaseTask
+        include Gitlab::WithRequestStore
+
+        def initialize(opts, logger: Logger.new($stdout))
+          @project_path = opts.fetch(:project_path)
+          @file_path    = opts.fetch(:file_path)
+          @namespace    = Namespace.find_by_full_path(opts.fetch(:namespace_path))
+          @current_user = User.find_by_username(opts.fetch(:username))
+          @measurement_enabled = opts.fetch(:measurement_enabled)
+          @measurement = Gitlab::Utils::Measuring.new(logger: logger) if @measurement_enabled
+          @logger = logger
+        end
+
+        private
+
+        attr_reader :measurement, :project, :namespace, :current_user, :file_path, :project_path, :logger
+
+        def measurement_enabled?
+          @measurement_enabled
+        end
+
+        def success(message)
+          logger.info(message)
+
+          true
+        end
+
+        def error(message)
+          logger.error(message)
+
+          false
+        end
+      end
+    end
+  end
+end

lib/gitlab/import_export/project/export_task.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module ImportExport
+    module Project
+      class ExportTask < BaseTask
+        def initialize(*)
+          super
+
+          @project = namespace.projects.find_by_path(@project_path)
+        end
+
+        def export
+          return error("Project with path: #{project_path} was not found. Please provide correct project path") unless project
+          return error("Invalid file path: #{file_path}. Please provide correct file path") unless file_path_exists?
+
+          with_export do
+            ::Projects::ImportExport::ExportService.new(project, current_user)
+              .execute(Gitlab::ImportExport::AfterExportStrategies::MoveFileStrategy.new(archive_path: file_path))
+          end
+
+          success('Done!')
+        end
+
+        private
+
+        def file_path_exists?
+          directory = File.dirname(file_path)
+
+          Dir.exist?(directory)
+        end
+
+        def with_export
+          with_request_store do
+            ::Gitlab::GitalyClient.allow_n_plus_1_calls do
+              measurement_enabled? ? measurement.with_measuring { yield } : yield
+            end
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/import_export/project/import_task.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module ImportExport
+    module Project
+      class ImportTask < BaseTask
+        def import
+          show_import_start_message
+
+          run_isolated_sidekiq_job
+
+          show_import_failures_count
+
+          return error(project.import_state.last_error) if project.import_state&.last_error
+          return error(project.errors.full_messages.to_sentence) if project.errors.any?
+
+          success('Done!')
+        end
+
+        private
+
+        # We want to ensure that all Sidekiq jobs are executed
+        # synchronously as part of that process.
+        # This ensures that all expensive operations do not escape
+        # to general Sidekiq clusters/nodes.
+        def with_isolated_sidekiq_job
+          Sidekiq::Testing.fake! do
+            with_request_store do
+              # If you are attempting to import a large project into a development environment,
+              # you may see Gitaly throw an error about too many calls or invocations.
+              # This is due to a n+1 calls limit being set for development setups (not enforced in production)
+              # https://gitlab.com/gitlab-org/gitlab/-/merge_requests/24475#note_283090635
+              # For development setups, this code-path will be excluded from n+1 detection.
+              ::Gitlab::GitalyClient.allow_n_plus_1_calls do
+                measurement_enabled? ? measurement.with_measuring { yield } : yield
+              end
+            end
+
+            true
+          end
+        end
+
+        def run_isolated_sidekiq_job
+          with_isolated_sidekiq_job do
+            @project = create_project
+
+            execute_sidekiq_job
+          end
+        end
+
+        def create_project
+          # We are disabling ObjectStorage for `import`
+          # as it is too slow to handle big archives:
+          # 1. DB transaction timeouts on upload
+          # 2. Download of archive before unpacking
+          disable_upload_object_storage do
+            service = Projects::GitlabProjectsImportService.new(
+              current_user,
+              {
+                namespace_id: namespace.id,
+                path:         project_path,
+                file:         File.open(file_path)
+              }
+            )
+
+            service.execute
+          end
+        end
+
+        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
+
+        def show_import_start_message
+          logger.info "Importing GitLab export: #{file_path} into GitLab" \
+            " #{full_path}" \
+            " as #{current_user.name}"
+        end
+
+        def show_import_failures_count
+          return unless project.import_failures.exists?
+
+          logger.info "Total number of not imported relations: #{project.import_failures.count}"
+        end
+      end
+    end
+  end
+end

lib/gitlab/utils/measuring.rb

@@ -59,14 +59,15 @@ module Gitlab
       end
 
       def duration_in_numbers(duration_in_seconds)
+        milliseconds = duration_in_seconds.in_milliseconds % 1.second.in_milliseconds
         seconds = duration_in_seconds % 1.minute
         minutes = (duration_in_seconds / 1.minute) % (1.hour / 1.minute)
         hours = duration_in_seconds / 1.hour
 
         if hours == 0
-          "%02d:%02d" % [minutes, seconds]
+          "%02d:%02d:%03d" % [minutes, seconds, milliseconds]
         else
-          "%02d:%02d:%02d" % [hours, minutes, seconds]
+          "%02d:%02d:%02d:%03d" % [hours, minutes, seconds, milliseconds]
         end
       end
     end

lib/tasks/gitlab/import_export/export.rake

 # frozen_string_literal: true
 
-require 'gitlab/with_request_store'
-
 # Export project to archive
 #
 # @example
@@ -14,81 +12,36 @@ namespace :gitlab do
       # Load it here to avoid polluting Rake tasks with Sidekiq test warnings
       require 'sidekiq/testing'
 
+      logger = Logger.new($stdout)
+
+      begin
         warn_user_is_not_gitlab
 
-      if ENV['IMPORT_DEBUG'].present?
-        ActiveRecord::Base.logger = Logger.new(STDOUT)
+        if ENV['EXPORT_DEBUG'].present?
+          ActiveRecord::Base.logger = logger
           Gitlab::Metrics::Exporter::SidekiqExporter.instance.start
+          logger.level = Logger::DEBUG
+        else
+          logger.level = Logger::INFO
         end
 
-      GitlabProjectExport.new(
+        task = Gitlab::ImportExport::Project::ExportTask.new(
           namespace_path: args.namespace_path,
           project_path:   args.project_path,
           username:       args.username,
           file_path:      args.archive_path,
-        measurement_enabled: Gitlab::Utils.to_boolean(args.measurement_enabled)
-      ).export
-    end
-  end
-end
-
-class GitlabProjectExport
-  include Gitlab::WithRequestStore
+          measurement_enabled: Gitlab::Utils.to_boolean(args.measurement_enabled),
+          logger: logger
+        )
 
-  def initialize(opts)
-    @project_path = opts.fetch(:project_path)
-    @file_path = opts.fetch(:file_path)
-    @current_user = User.find_by_username(opts.fetch(:username))
-    namespace = Namespace.find_by_full_path(opts.fetch(:namespace_path))
-    @project = namespace.projects.find_by_path(@project_path)
-    @measurement_enabled = opts.fetch(:measurement_enabled)
-    @measurable = Gitlab::Utils::Measuring.new if @measurement_enabled
-  end
-
-  def export
-    validate_project
-    validate_file_path
-
-    with_export do
-      ::Projects::ImportExport::ExportService.new(project, current_user)
-        .execute(Gitlab::ImportExport::AfterExportStrategies::MoveFileStrategy.new(archive_path: file_path))
-    end
+        success = task.export
 
-    puts 'Done!'
+        exit(success)
       rescue StandardError => e
-    puts "Exception: #{e.message}"
-    puts e.backtrace
-    exit 1
-  end
-
-  private
-
-  attr_reader :measurable, :project, :current_user, :file_path, :project_path
-
-  def validate_project
-    unless project
-      puts "Error: Project with path: #{project_path} was not found. Please provide correct project path"
-      exit 1
-    end
-  end
-
-  def validate_file_path
-    directory = File.dirname(file_path)
-    unless Dir.exist?(directory)
-      puts "Error: Invalid file path: #{file_path}. Please provide correct file path"
+        logger.error "Exception: #{e.message}"
+        logger.debug e.backtrace
         exit 1
       end
     end
-
-  def with_export
-    with_request_store do
-      ::Gitlab::GitalyClient.allow_n_plus_1_calls do
-        measurement_enabled? ? measurable.with_measuring { yield } : yield
-      end
-    end
-  end
-
-  def measurement_enabled?
-    @measurement_enabled
   end
 end

lib/tasks/gitlab/import_export/import.rake

 # frozen_string_literal: true
 
-require 'gitlab/with_request_store'
-
 # Import large project archives
 #
 # This task:
@@ -18,148 +16,36 @@ namespace :gitlab do
       # Load it here to avoid polluting Rake tasks with Sidekiq test warnings
       require 'sidekiq/testing'
 
+      logger = Logger.new($stdout)
+
+      begin
         warn_user_is_not_gitlab
 
         if ENV['IMPORT_DEBUG'].present?
-        ActiveRecord::Base.logger = Logger.new(STDOUT)
+          ActiveRecord::Base.logger = logger
+          Gitlab::Metrics::Exporter::SidekiqExporter.instance.start
+          logger.level = Logger::DEBUG
+        else
+          logger.level = Logger::INFO
         end
 
-      GitlabProjectImport.new(
+        task = Gitlab::ImportExport::Project::ImportTask.new(
           namespace_path: args.namespace_path,
           project_path:   args.project_path,
           username:       args.username,
           file_path:      args.archive_path,
-        measurement_enabled: Gitlab::Utils.to_boolean(args.measurement_enabled)
-      ).import
-    end
-  end
-end
-
-class GitlabProjectImport
-  include Gitlab::WithRequestStore
-
-  def initialize(opts)
-    @project_path = opts.fetch(:project_path)
-    @file_path    = opts.fetch(:file_path)
-    @namespace    = Namespace.find_by_full_path(opts.fetch(:namespace_path))
-    @current_user = User.find_by_username(opts.fetch(:username))
-    @measurement_enabled = opts.fetch(:measurement_enabled)
-    @measurement = Gitlab::Utils::Measuring.new if @measurement_enabled
-  end
-
-  def import
-    show_import_start_message
-
-    run_isolated_sidekiq_job
+          measurement_enabled: Gitlab::Utils.to_boolean(args.measurement_enabled),
+          logger: logger
+        )
 
-    show_import_failures_count
+        success = task.import
 
-    if project&.import_state&.last_error
-      puts "ERROR: #{project.import_state.last_error}"
-      exit 1
-    elsif project.errors.any?
-      puts "ERROR: #{project.errors.full_messages.join(', ')}"
-      exit 1
-    else
-      puts 'Done!'
-    end
+        exit(success)
       rescue StandardError => e
-    puts "Exception: #{e.message}"
-    puts e.backtrace
+        logger.error "Exception: #{e.message}"
+        logger.debug e.backtrace
         exit 1
       end
-
-  private
-
-  attr_reader :measurement, :project, :namespace, :current_user, :file_path, :project_path
-
-  def measurement_enabled?
-    @measurement_enabled
-  end
-
-  # We want to ensure that all Sidekiq jobs are executed
-  # synchronously as part of that process.
-  # This ensures that all expensive operations do not escape
-  # to general Sidekiq clusters/nodes.
-  def with_isolated_sidekiq_job
-    Sidekiq::Testing.fake! do
-      with_request_store do
-        # If you are attempting to import a large project into a development environment,
-        # you may see Gitaly throw an error about too many calls or invocations.
-        # This is due to a n+1 calls limit being set for development setups (not enforced in production)
-        # https://gitlab.com/gitlab-org/gitlab/-/merge_requests/24475#note_283090635
-        # For development setups, this code-path will be excluded from n+1 detection.
-        ::Gitlab::GitalyClient.allow_n_plus_1_calls do
-          measurement_enabled? ? measurement.with_measuring { yield } : yield
-        end
-      end
-
-      true
-    end
     end
-
-  def run_isolated_sidekiq_job
-    with_isolated_sidekiq_job do
-      @project = create_project
-
-      execute_sidekiq_job
-    end
-  end
-
-  def create_project
-    # We are disabling ObjectStorage for `import`
-    # as it is too slow to handle big archives:
-    # 1. DB transaction timeouts on upload
-    # 2. Download of archive before unpacking
-    disable_upload_object_storage do
-      service = Projects::GitlabProjectsImportService.new(
-        current_user,
-        {
-          namespace_id: namespace.id,
-          path:         project_path,
-          file:         File.open(file_path)
-        }
-      )
-
-      service.execute
-    end
-  end
-
-  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
-
-  def show_import_start_message
-    puts "Importing GitLab export: #{file_path} into GitLab" \
-      " #{full_path}" \
-      " as #{current_user.name}"
-  end
-
-  def show_import_failures_count
-    return unless project.import_failures.exists?
-
-    puts "Total number of not imported relations: #{project.import_failures.count}"
   end
 end

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

+# frozen_string_literal: true
+
+require 'rake_helper'
+
+describe Gitlab::ImportExport::Project::ExportTask do
+  let(:username) { 'root' }
+  let(:namespace_path) { username }
+  let!(:user) { create(:user, username: username) }
+  let(:measurement_enabled) { false }
+  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(:task_params) do
+    {
+      username: username,
+      namespace_path: namespace_path,
+      project_path: project_name,
+      file_path: file_path,
+      measurement_enabled: measurement_enabled
+    }
+  end
+
+  subject { described_class.new(task_params).export }
+
+  context 'when project is found' do
+    let(:project) { create(:project, creator: user, namespace: user.namespace) }
+
+    around do |example|
+      example.run
+    ensure
+      File.delete(file_path)
+    end
+
+    it 'performs project export successfully' do
+      expect { subject }.to output(/Done!/).to_stdout
+
+      expect(subject).to eq(true)
+
+      expect(File).to exist(file_path)
+    end
+
+    it_behaves_like 'measurable'
+  end
+
+  context 'when project is not found' do
+    let(:project_name) { 'invalid project name' }
+
+    it 'logs an error' do
+      expect { subject }.to output(/Project with path: #{project_name} was not found. Please provide correct project path/).to_stdout
+    end
+
+    it 'returns false' do
+      expect(subject).to eq(false)
+    end
+  end
+
+  context 'when file path is invalid' do
+    let(:file_path) { '/invalid_file_path/test_project_export.tar.gz' }
+
+    it 'logs an error' do
+      expect { subject }.to output(/Invalid file path: #{file_path}. Please provide correct file path/ ).to_stdout
+    end
+
+    it 'returns false' do
+      expect(subject).to eq(false)
+    end
+  end
+end

spec/tasks/gitlab/import_export/import_rake_spec.rb → spec/lib/gitlab/import_export/project/import_task_spec.rb

@@ -2,19 +2,25 @@
 
 require 'rake_helper'
 
-describe 'gitlab:import_export:import rake task' do
+describe Gitlab::ImportExport::Project::ImportTask do
   let(:username) { 'root' }
   let(:namespace_path) { username }
   let!(:user) { create(:user, username: username) }
   let(:measurement_enabled) { false }
-  let(:task_params) { [username, namespace_path, project_name, archive_path, measurement_enabled] }
   let(:project) { Project.find_by_full_path("#{namespace_path}/#{project_name}") }
+  let(:import_task) { described_class.new(task_params) }
+  let(:task_params) do
+    {
+      username: username,
+      namespace_path: namespace_path,
+      project_path: project_name,
+      file_path: file_path,
+      measurement_enabled: measurement_enabled
+    }
+  end
 
   before do
-    Rake.application.rake_require('tasks/gitlab/import_export/import')
     allow(Settings.uploads.object_store).to receive(:[]=).and_call_original
-    allow_any_instance_of(GitlabProjectImport).to receive(:exit)
-      .and_raise(RuntimeError, 'exit not handled')
   end
 
   around do |example|
@@ -30,15 +36,16 @@ describe 'gitlab:import_export:import rake task' do
     Settings.uploads.object_store['background_upload'] = old_background_upload_setting
   end
 
-  subject { run_rake_task('gitlab:import_export:import', task_params) }
+  subject { import_task.import }
 
   context 'when project import is valid' do
     let(:project_name) { 'import_rake_test_project' }
-    let(:archive_path) { 'spec/fixtures/gitlab/import_export/lightweight_project_export.tar.gz' }
+    let(:file_path) { 'spec/fixtures/gitlab/import_export/lightweight_project_export.tar.gz' }
 
     it 'performs project import successfully' do
       expect { subject }.to output(/Done!/).to_stdout
       expect { subject }.not_to raise_error
+      expect(subject).to eq(true)
 
       expect(project.merge_requests.count).to be > 0
       expect(project.issues.count).to be > 0
@@ -56,8 +63,7 @@ describe 'gitlab:import_export:import rake task' do
         end
       end
 
-      expect_next_instance_of(GitlabProjectImport) do |importer|
-        expect(importer).to receive(:execute_sidekiq_job).and_wrap_original do |m|
+      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)
@@ -65,7 +71,6 @@ describe 'gitlab:import_export:import rake task' do
 
         m.call
       end
-      end
 
       subject
     end
@@ -75,13 +80,13 @@ describe 'gitlab:import_export:import rake task' do
 
   context 'when project import is invalid' do
     let(:project_name) { 'import_rake_invalid_test_project' }
-    let(:archive_path) { 'spec/fixtures/gitlab/import_export/corrupted_project_export.tar.gz' }
+    let(:file_path) { 'spec/fixtures/gitlab/import_export/corrupted_project_export.tar.gz' }
     let(:not_imported_message) { /Total number of not imported relations: 1/ }
-    let(:error) { /Validation failed: Notes is invalid/ }
 
     it 'performs project import successfully' do
       expect { subject }.to output(not_imported_message).to_stdout
       expect { subject }.not_to raise_error
+      expect(subject).to eq(true)
 
       expect(project.merge_requests).to be_empty
       expect(project.import_state.last_error).to be_nil

spec/support/shared_examples/tasks/gitlab/import_export/measurable_shared_examples.rb

@@ -18,7 +18,7 @@ RSpec.shared_examples 'measurable' do
   end
 
   context 'when measurement is not provided' do
-    let(:task_params) { [username, namespace_path, project_name, archive_path] }
+    let(:measurement_enabled) { nil }
 
     it 'does not output measurement results' do
       expect { subject }.not_to output(/Measuring enabled.../).to_stdout

spec/tasks/gitlab/import_export/export_rake_spec.rb

-# frozen_string_literal: true
-
-require 'rake_helper'
-
-describe 'gitlab:import_export:export rake task' do
-  let(:username) { 'root' }
-  let(:namespace_path) { username }
-  let!(:user) { create(:user, username: username) }
-  let(:measurement_enabled) { false }
-  let(:task_params) { [username, namespace_path, project_name, archive_path, measurement_enabled] }
-
-  before do
-    Rake.application.rake_require('tasks/gitlab/import_export/export')
-  end
-
-  subject { run_rake_task('gitlab:import_export:export', task_params) }
-
-  context 'when project is found' do
-    let(:project) { create(:project, creator: user, namespace: user.namespace) }
-    let(:project_name) { project.name }
-    let(:archive_path) { 'spec/fixtures/gitlab/import_export/test_project_export.tar.gz' }
-
-    around do |example|
-      example.run
-    ensure
-      File.delete(archive_path)
-    end
-
-    it 'performs project export successfully' do
-      expect { subject }.to output(/Done!/).to_stdout
-
-      expect(File).to exist(archive_path)
-    end
-
-    it_behaves_like 'measurable'
-  end
-end