Merge branch 'sh-fix-import-export-shared-ee' into 'master'

[EE] Fix project exports clobbering concurrent export paths

Closes #14716

See merge request gitlab-org/gitlab!16280

app/services/import_export_clean_up_service.rb

@@ -12,16 +12,20 @@ class ImportExportCleanUpService
 
   def execute
     Gitlab::Metrics.measure(:import_export_clean_up) do
-      clean_up_export_object_files
-
-      break unless File.directory?(path)
-
-      clean_up_export_files
+      execute_cleanup
     end
   end
 
   private
 
+  def execute_cleanup
+    clean_up_export_object_files
+  ensure
+    # We don't want a failure in cleaning up object storage from
+    # blocking us from cleaning up temporary storage.
+    clean_up_export_files if File.directory?(path)
+  end
+
   def clean_up_export_files
     Gitlab::Popen.popen(%W(find #{path} -not -path #{path} -mmin +#{mmin} -delete))
   end

ee/changelogs/unreleased/sh-fix-import-export-shared-ee.yml

+---
+title: Fix project exports clobbering concurrent export paths
+merge_request: 16280
+author:
+type: fixed

ee/lib/ee/gitlab/import_export/after_export_strategies/custom_template_export_import_strategy.rb

@@ -30,7 +30,6 @@ module EE
             ::RepositoryImportWorker.new.perform(export_into_project_id)
           ensure
             export_file.close if export_file.respond_to?(:close)
-            project.remove_exports
           end
 
           def export_file

lib/gitlab/import_export/after_export_strategies/base_after_export_strategy.rb

@@ -10,11 +10,9 @@ module Gitlab
 
         StrategyError = Class.new(StandardError)
 
-        AFTER_EXPORT_LOCK_FILE_NAME = '.after_export_action'
-
         private
 
-        attr_reader :project, :current_user
+        attr_reader :project, :current_user, :lock_file
 
         public
 
@@ -29,8 +27,9 @@ module Gitlab
         def execute(current_user, project)
           @project = project
 
-          return unless @project.export_status == :finished
-
+          ensure_export_ready!
+          ensure_lock_files_path!
+          @lock_file = File.join(lock_files_path, SecureRandom.hex)
           @current_user = current_user
 
           if invalid?
@@ -48,19 +47,32 @@ module Gitlab
           false
         ensure
           delete_after_export_lock
+          delete_export_file
+          delete_archive_path
         end
 
         def to_json(options = {})
           @options.to_h.merge!(klass: self.class.name).to_json
         end
 
-        def self.lock_file_path(project)
-          return unless project.export_path || export_file_exists?
+        def ensure_export_ready!
+          raise StrategyError unless project.export_file_exists?
+        end
+
+        def ensure_lock_files_path!
+          FileUtils.mkdir_p(lock_files_path) unless Dir.exist?(lock_files_path)
+        end
+
+        def lock_files_path
+          project.import_export_shared.lock_files_path
+        end
 
-          lock_path = project.import_export_shared.archive_path
+        def archive_path
+          project.import_export_shared.archive_path
+        end
 
-          mkdir_p(lock_path)
-          File.join(lock_path, AFTER_EXPORT_LOCK_FILE_NAME)
+        def locks_present?
+          project.import_export_shared.locks_present?
         end
 
         protected
@@ -69,25 +81,33 @@ module Gitlab
           raise NotImplementedError
         end
 
+        def delete_export?
+          true
+        end
+
         private
 
+        def delete_export_file
+          return if locks_present? || !delete_export?
+
+          project.remove_exports
+        end
+
+        def delete_archive_path
+          FileUtils.rm_rf(archive_path) if File.directory?(archive_path)
+        end
+
         def create_or_update_after_export_lock
-          FileUtils.touch(self.class.lock_file_path(project))
+          FileUtils.touch(lock_file)
         end
 
         def delete_after_export_lock
-          lock_file = self.class.lock_file_path(project)
-
           FileUtils.rm(lock_file) if lock_file.present? && File.exist?(lock_file)
         end
 
         def log_validation_errors
           errors.full_messages.each { |msg| project.import_export_shared.add_error_message(msg) }
         end
-
-        def export_file_exists?
-          project.export_file_exists?
-        end
       end
     end
   end

lib/gitlab/import_export/after_export_strategies/download_notification_strategy.rb

@@ -4,6 +4,12 @@ module Gitlab
   module ImportExport
     module AfterExportStrategies
       class DownloadNotificationStrategy < BaseAfterExportStrategy
+        protected
+
+        def delete_export?
+          false
+        end
+
         private
 
         def strategy_execute

lib/gitlab/import_export/after_export_strategies/web_upload_strategy.rb

@@ -24,8 +24,6 @@ module Gitlab
 
         def strategy_execute
           handle_response_error(send_file)
-
-          project.remove_exports
         end
 
         def handle_response_error(response)

lib/gitlab/import_export/shared.rb

 # frozen_string_literal: true
-
+#
+# This class encapsulates the directories used by project import/export:
+#
+# 1. The project export job first generates the project metadata tree
+#    (e.g. `project.json) and repository bundle (e.g. `project.bundle`)
+#    inside a temporary `export_path`
+#    (e.g. /path/to/shared/tmp/project_exports/namespace/project/:randomA/:randomB).
+#
+# 2. The job then creates a tarball (e.g. `project.tar.gz`) in
+#    `archive_path` (e.g. /path/to/shared/tmp/project_exports/namespace/project/:randomA).
+#    CarrierWave moves this tarball files into its permanent location.
+#
+# 3. Lock files are used to indicate whether a project is in the
+#    `after_export` state.  These are stored in a directory
+#    (e.g. /path/to/shared/tmp/project_exports/namespace/project/locks. The
+#    number of lock files present signifies how many concurrent project
+#    exports are running. Note that this assumes the temporary directory
+#    is a shared mount:
+#    https://gitlab.com/gitlab-org/gitlab/issues/32203
+#
+# NOTE: Stale files should be cleaned up via ImportExportCleanupService.
 module Gitlab
   module ImportExport
     class Shared
       attr_reader :errors, :project
 
+      LOCKS_DIRECTORY = 'locks'
+
       def initialize(project)
         @project = project
         @errors = []
@@ -12,17 +34,27 @@ module Gitlab
       end
 
       def active_export_count
-        Dir[File.join(archive_path, '*')].count { |name| File.directory?(name) }
+        Dir[File.join(base_path, '*')].count { |name| File.basename(name) != LOCKS_DIRECTORY && File.directory?(name) }
       end
 
+      # The path where the project metadata and repository bundle is saved
       def export_path
         @export_path ||= Gitlab::ImportExport.export_path(relative_path: relative_path)
       end
 
+      # The path where the tarball is saved
       def archive_path
         @archive_path ||= Gitlab::ImportExport.export_path(relative_path: relative_archive_path)
       end
 
+      def base_path
+        @base_path ||= Gitlab::ImportExport.export_path(relative_path: relative_base_path)
+      end
+
+      def lock_files_path
+        @locks_files_path ||= File.join(base_path, LOCKS_DIRECTORY)
+      end
+
       def error(error)
         log_error(message: error.message, caller: caller[0].dup)
         log_debug(backtrace: error.backtrace&.join("\n"))
@@ -37,16 +69,24 @@ module Gitlab
       end
 
       def after_export_in_progress?
-        File.exist?(after_export_lock_file)
+        locks_present?
+      end
+
+      def locks_present?
+        Dir.exist?(lock_files_path) && !Dir.empty?(lock_files_path)
       end
 
       private
 
       def relative_path
-        File.join(relative_archive_path, SecureRandom.hex)
+        @relative_path ||= File.join(relative_archive_path, SecureRandom.hex)
       end
 
       def relative_archive_path
+        @relative_archive_path ||= File.join(@project.disk_path, SecureRandom.hex)
+      end
+
+      def relative_base_path
         @project.disk_path
       end
 
@@ -70,10 +110,6 @@ module Gitlab
       def filtered_error_message(message)
         Projects::ImportErrorFilter.filter_message(message)
       end
-
-      def after_export_lock_file
-        AfterExportStrategies::BaseAfterExportStrategy.lock_file_path(project)
-      end
     end
   end
 end

spec/features/projects/import_export/export_file_spec.rb

@@ -49,8 +49,7 @@ describe 'Import/Export - project export integration test', :js do
 
       expect(page).to have_content('Download export')
 
-      expect(file_permissions(project.export_path)).to eq(0700)
-
+      expect(project.export_status).to eq(:finished)
       expect(project.export_file.path).to include('tar.gz')
 
       in_directory_with_expanded_export(project) do |exit_status, tmpdir|

spec/lib/gitlab/import_export/after_export_strategies/base_after_export_strategy_spec.rb

@@ -24,14 +24,22 @@ describe Gitlab::ImportExport::AfterExportStrategies::BaseAfterExportStrategy do
 
       service.execute(user, project)
 
-      expect(lock_path_exist?).to be_truthy
+      expect(service.locks_present?).to be_truthy
     end
 
     context 'when the method succeeds' do
       it 'removes the lock file' do
         service.execute(user, project)
 
-        expect(lock_path_exist?).to be_falsey
+        expect(service.locks_present?).to be_falsey
+      end
+
+      it 'removes the archive path' do
+        FileUtils.mkdir_p(shared.archive_path)
+
+        service.execute(user, project)
+
+        expect(File.exist?(shared.archive_path)).to be_falsey
       end
     end
 
@@ -62,13 +70,21 @@ describe Gitlab::ImportExport::AfterExportStrategies::BaseAfterExportStrategy do
 
           service.execute(user, project)
         end
+
+        it 'removes the archive path' do
+          FileUtils.mkdir_p(shared.archive_path)
+
+          service.execute(user, project)
+
+          expect(File.exist?(shared.archive_path)).to be_falsey
+        end
       end
 
       context 'when an exception is raised' do
         it 'removes the lock' do
           expect { service.execute(user, project) }.to raise_error(NotImplementedError)
 
-          expect(lock_path_exist?).to be_falsey
+          expect(service.locks_present?).to be_falsey
         end
       end
     end
@@ -97,8 +113,4 @@ describe Gitlab::ImportExport::AfterExportStrategies::BaseAfterExportStrategy do
       expect(described_class.new(params).to_json).to eq result
     end
   end
-
-  def lock_path_exist?
-    File.exist?(described_class.lock_file_path(project))
-  end
 end

spec/lib/gitlab/import_export/shared_spec.rb

@@ -5,6 +5,35 @@ describe Gitlab::ImportExport::Shared do
   let(:project) { build(:project) }
   subject { project.import_export_shared }
 
+  context 'with a repository on disk' do
+    let(:project) { create(:project, :repository) }
+    let(:base_path) { %(/tmp/project_exports/#{project.disk_path}/) }
+
+    describe '#archive_path' do
+      it 'uses a random hash to avoid conflicts' do
+        expect(subject.archive_path).to match(/#{base_path}\h{32}/)
+      end
+
+      it 'memoizes the path' do
+        path = subject.archive_path
+
+        2.times { expect(subject.archive_path).to eq(path) }
+      end
+    end
+
+    describe '#export_path' do
+      it 'uses a random hash relative to project path' do
+        expect(subject.export_path).to match(/#{base_path}\h{32}\/\h{32}/)
+      end
+
+      it 'memoizes the path' do
+        path = subject.export_path
+
+        2.times { expect(subject.export_path).to eq(path) }
+      end
+    end
+  end
+
   describe '#error' do
     let(:error) { StandardError.new('Error importing into /my/folder Permission denied @ unlink_internal - /var/opt/gitlab/gitlab-rails/shared/a/b/c/uploads/file') }
 

spec/requests/api/project_export_spec.rb

@@ -30,7 +30,7 @@ describe API::ProjectExport do
     FileUtils.mkdir_p File.join(project_started.export_path, 'securerandom-hex')
 
     # simulate in after export action
-    FileUtils.touch Gitlab::ImportExport::AfterExportStrategies::BaseAfterExportStrategy.lock_file_path(project_after_export)
+    FileUtils.touch File.join(project_after_export.import_export_shared.lock_files_path, SecureRandom.hex)
   end
 
   after do