using shared path for project import uploads and refactored gitlab remove export workerfix/ha-mode-import-issue

8 files changed, 104 insertions, 13 deletions

diff --git a/CHANGELOG b/CHANGELOG
index c099c63ce86..dd3ee006593 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -47,6 +47,9 @@ v 8.11.0 (unreleased)
   - Fix RequestProfiler::Middleware error when code is reloaded in development
   - Catch what warden might throw when profiling requests to re-throw it
 
+v 8.10.4 (unreleased)
+  - Fix Import/Export project import not working in HA mode
+
 v 8.10.3
   - Fix Import/Export issue importing milestones and labels not associated properly. !5426
   - Fix timing problems running imports on production. !5523
diff --git a/app/controllers/import/gitlab_projects_controller.rb b/app/controllers/import/gitlab_projects_controller.rb
index 30df1fb2fec..3ec173abcdb 100644
--- a/app/controllers/import/gitlab_projects_controller.rb
+++ b/app/controllers/import/gitlab_projects_controller.rb
@@ -12,13 +12,14 @@ class Import::GitlabProjectsController < Import::BaseController
       return redirect_back_or_default(options: { alert: "You need to upload a GitLab project export archive." })
     end
 
-    imported_file = project_params[:file].path + "-import"
+    import_upload_path = Gitlab::ImportExport.import_upload_path(filename: project_params[:file].original_filename)
 
-    FileUtils.copy_entry(project_params[:file].path, imported_file)
+    FileUtils.mkdir_p(File.dirname(import_upload_path))
+    FileUtils.copy_entry(project_params[:file].path, import_upload_path)
 
     @project = Gitlab::ImportExport::ProjectCreator.new(project_params[:namespace_id],
                                                         current_user,
-                                                        File.expand_path(imported_file),
+                                                        import_upload_path,
                                                         project_params[:path]).execute
 
     if @project.saved?
diff --git a/app/models/project.rb b/app/models/project.rb
index 83b848ded8b..a18aef09acd 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -378,11 +378,6 @@ class Project < ActiveRecord::Base
 
       joins(join_body).reorder('join_note_counts.amount DESC')
     end
-
-    # Deletes gitlab project export files older than 24 hours
-    def remove_gitlab_exports!
-      Gitlab::Popen.popen(%W(find #{Gitlab::ImportExport.storage_path} -not -path #{Gitlab::ImportExport.storage_path} -mmin +1440 -delete))
-    end
   end
 
   def repository_storage_path
diff --git a/app/services/import_export_clean_up_service.rb b/app/services/import_export_clean_up_service.rb
new file mode 100644
index 00000000000..4be182c4a7f
--- /dev/null
+++ b/app/services/import_export_clean_up_service.rb
@@ -0,0 +1,24 @@
+class ImportExportCleanUpService
+  LAST_MODIFIED_TIME_IN_MINUTES = 1440
+
+  def initialize(mmin = LAST_MODIFIED_TIME_IN_MINUTES)
+    @mmin = mmin
+    @path = Gitlab::ImportExport.storage_path
+  end
+
+  def execute
+    Gitlab::Metrics.measure(:import_export_clean_up) do
+      return unless File.directory?(path)
+
+      clean_up_export_files
+    end
+  end
+
+  private
+
+  attr_reader :mmin, :path
+
+  def clean_up_export_files
+    Gitlab::Popen.popen(%W(find #{path} -not -path #{path} -mmin +#{LAST_MODIFIED_TIME_IN_MINUTES} -delete))
+  end
+end
diff --git a/app/workers/gitlab_remove_project_export_worker.rb b/app/workers/import_export_project_cleanup_worker.rb
index 1d91897d520..72e3a9ae734 100644
--- a/app/workers/gitlab_remove_project_export_worker.rb
+++ b/app/workers/import_export_project_cleanup_worker.rb
@@ -1,9 +1,9 @@
-class GitlabRemoveProjectExportWorker
+class ImportExportProjectCleanupWorker
   include Sidekiq::Worker
 
   sidekiq_options queue: :default
 
   def perform
-    Project.remove_gitlab_exports!
+    ImportExportCleanUpService.new.execute
   end
 end
diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb
index 49130f37b31..deac3b0f0f9 100644
--- a/config/initializers/1_settings.rb
+++ b/config/initializers/1_settings.rb
@@ -287,9 +287,9 @@ Settings.cron_jobs['admin_email_worker']['job_class'] = 'AdminEmailWorker'
 Settings.cron_jobs['repository_archive_cache_worker'] ||= Settingslogic.new({})
 Settings.cron_jobs['repository_archive_cache_worker']['cron'] ||= '0 * * * *'
 Settings.cron_jobs['repository_archive_cache_worker']['job_class'] = 'RepositoryArchiveCacheWorker'
-Settings.cron_jobs['gitlab_remove_project_export_worker'] ||= Settingslogic.new({})
-Settings.cron_jobs['gitlab_remove_project_export_worker']['cron'] ||= '0 * * * *'
-Settings.cron_jobs['gitlab_remove_project_export_worker']['job_class'] = 'GitlabRemoveProjectExportWorker'
+Settings.cron_jobs['import_export_project_cleanup_worker'] ||= Settingslogic.new({})
+Settings.cron_jobs['import_export_project_cleanup_worker']['cron'] ||= '0 * * * *'
+Settings.cron_jobs['import_export_project_cleanup_worker']['job_class'] = 'ImportExportProjectCleanupWorker'
 Settings.cron_jobs['requests_profiles_worker'] ||= Settingslogic.new({})
 Settings.cron_jobs['requests_profiles_worker']['cron'] ||= '0 0 * * *'
 Settings.cron_jobs['requests_profiles_worker']['job_class'] = 'RequestsProfilesWorker'
diff --git a/lib/gitlab/import_export.rb b/lib/gitlab/import_export.rb
index 48b2c43ac21..84147b4789f 100644
--- a/lib/gitlab/import_export.rb
+++ b/lib/gitlab/import_export.rb
@@ -13,6 +13,10 @@ module Gitlab
       File.join(Settings.shared['path'], 'tmp/project_exports')
     end
 
+    def import_upload_path(filename: )
+      File.join(storage_path, 'uploads', filename)
+    end
+
     def project_filename
       "project.json"
     end
diff --git a/spec/services/import_export_clean_up_service_spec.rb b/spec/services/import_export_clean_up_service_spec.rb
new file mode 100644
index 00000000000..21f6135d01d
--- /dev/null
+++ b/spec/services/import_export_clean_up_service_spec.rb
@@ -0,0 +1,64 @@
+require 'spec_helper'
+
+describe ImportExportCleanUpService, services: true do
+  describe '#execute' do
+    subject(:service) { described_class.new }
+
+    let(:tmp_import_export_folder) { 'tmp/project_exports' }
+
+    context 'when the import/export directory does not exist' do
+      it 'does not remove any archives' do
+        path = '/invalid/path/'
+        stub_repository_downloads_path(path)
+
+        expect(File).to receive(:directory?).with(path + tmp_import_export_folder).and_return(false).at_least(:once)
+        expect(service).not_to receive(:clean_up_export_files)
+
+        service.execute
+      end
+    end
+
+    context 'when the import/export directory exists' do
+      it 'removes old files' do
+        in_directory_with_files(mtime: 2.days) do |dir, files|
+          service.execute
+
+          files.each { |file| expect(File.exist?(file)).to eq false }
+          expect(File.directory?(dir)).to eq false
+        end
+      end
+
+      it 'does not remove new files' do
+        in_directory_with_files(mtime: 2.hours) do |dir, files|
+          service.execute
+
+          files.each { |file| expect(File.exist?(file)).to eq true }
+          expect(File.directory?(dir)).to eq true
+        end
+      end
+    end
+
+    def in_directory_with_files(mtime:)
+      Dir.mktmpdir do |tmpdir|
+        stub_repository_downloads_path(tmpdir)
+        dir = File.join(tmpdir, tmp_import_export_folder, 'subfolder')
+        FileUtils.mkdir_p(dir)
+
+        files = FileUtils.touch(file_list(dir) + [dir], mtime: Time.now - mtime)
+
+        yield(dir, files)
+      end
+    end
+
+    def stub_repository_downloads_path(path)
+      new_shared_settings = Settings.shared.merge('path' => path)
+      allow(Settings).to receive(:shared).and_return(new_shared_settings)
+    end
+
+    def file_list(dir)
+      Array.new(5) do |num|
+        File.join(dir, "random-#{num}.tar.gz")
+      end
+    end
+  end
+end