diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2019-09-19 03:06:10 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2019-09-19 03:06:10 +0000 |
commit | 2c352d252d072549e77d9de71fb9efe7bae94da6 (patch) | |
tree | 5db2b4820519bd7b1e808a47c5a2d69de48cbeb2 | |
parent | a7706bcb567ee31c6454c4197354b3210839b564 (diff) | |
download | gitlab-ce-2c352d252d072549e77d9de71fb9efe7bae94da6.tar.gz |
Add latest changes from gitlab-org/gitlab@master
9 files changed, 146 insertions, 42 deletions
diff --git a/app/services/import_export_clean_up_service.rb b/app/services/import_export_clean_up_service.rb index 3ecb51b60d0..66ac7dac4ca 100644 --- a/app/services/import_export_clean_up_service.rb +++ b/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 diff --git a/lib/gitlab/import_export/after_export_strategies/base_after_export_strategy.rb b/lib/gitlab/import_export/after_export_strategies/base_after_export_strategy.rb index c5fb39b7b52..b30258123d4 100644 --- a/lib/gitlab/import_export/after_export_strategies/base_after_export_strategy.rb +++ b/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 diff --git a/lib/gitlab/import_export/after_export_strategies/download_notification_strategy.rb b/lib/gitlab/import_export/after_export_strategies/download_notification_strategy.rb index 1b391314a74..39a6090ad87 100644 --- a/lib/gitlab/import_export/after_export_strategies/download_notification_strategy.rb +++ b/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 diff --git a/lib/gitlab/import_export/after_export_strategies/web_upload_strategy.rb b/lib/gitlab/import_export/after_export_strategies/web_upload_strategy.rb index aaa70f0b36d..fd98bc2caad 100644 --- a/lib/gitlab/import_export/after_export_strategies/web_upload_strategy.rb +++ b/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) diff --git a/lib/gitlab/import_export/shared.rb b/lib/gitlab/import_export/shared.rb index 725c1101d70..9144a4406cd 100644 --- a/lib/gitlab/import_export/shared.rb +++ b/lib/gitlab/import_export/shared.rb @@ -1,10 +1,32 @@ # 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 diff --git a/spec/features/projects/import_export/export_file_spec.rb b/spec/features/projects/import_export/export_file_spec.rb index a1002f38936..7618a2bdea3 100644 --- a/spec/features/projects/import_export/export_file_spec.rb +++ b/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| diff --git a/spec/lib/gitlab/import_export/after_export_strategies/base_after_export_strategy_spec.rb b/spec/lib/gitlab/import_export/after_export_strategies/base_after_export_strategy_spec.rb index 9a442de2900..a3d2880182d 100644 --- a/spec/lib/gitlab/import_export/after_export_strategies/base_after_export_strategy_spec.rb +++ b/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 diff --git a/spec/lib/gitlab/import_export/shared_spec.rb b/spec/lib/gitlab/import_export/shared_spec.rb index 2c288cff6ef..da2c96dcf51 100644 --- a/spec/lib/gitlab/import_export/shared_spec.rb +++ b/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') } diff --git a/spec/requests/api/project_export_spec.rb b/spec/requests/api/project_export_spec.rb index 1d2f81a397d..7de8935097a 100644 --- a/spec/requests/api/project_export_spec.rb +++ b/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 |