From daff635d75eb47cbe0c801e04e0a79e65f11f446 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Mon, 12 Jun 2017 19:31:00 +0000 Subject: Merge branch 'sh-fix-refactor-uploader-work-dir' into 'master' Set artifact working directory to be in the destination store to prevent unnecessary I/O Closes #33274 See merge request !11905 --- app/uploaders/artifact_uploader.rb | 4 +++ app/uploaders/gitlab_uploader.rb | 19 +++++++++++ app/uploaders/lfs_object_uploader.rb | 12 ------- .../sh-fix-refactor-uploader-work-dir.yml | 4 +++ spec/uploaders/artifact_uploader_spec.rb | 15 ++++++--- spec/uploaders/gitlab_uploader_spec.rb | 15 +++++++++ spec/uploaders/lfs_object_uploader_spec.rb | 39 +++++++++++++--------- 7 files changed, 77 insertions(+), 31 deletions(-) create mode 100644 changelogs/unreleased/sh-fix-refactor-uploader-work-dir.yml diff --git a/app/uploaders/artifact_uploader.rb b/app/uploaders/artifact_uploader.rb index 3bc0408f557..14addb6cf14 100644 --- a/app/uploaders/artifact_uploader.rb +++ b/app/uploaders/artifact_uploader.rb @@ -23,6 +23,10 @@ class ArtifactUploader < GitlabUploader File.join(self.class.local_artifacts_store, 'tmp/cache') end + def work_dir + File.join(self.class.local_artifacts_store, 'tmp/work') + end + private def default_local_path diff --git a/app/uploaders/gitlab_uploader.rb b/app/uploaders/gitlab_uploader.rb index e4e6d6f46b1..136ec6cc6af 100644 --- a/app/uploaders/gitlab_uploader.rb +++ b/app/uploaders/gitlab_uploader.rb @@ -53,4 +53,23 @@ class GitlabUploader < CarrierWave::Uploader::Base def exists? file.try(:exists?) end + + # Override this if you don't want to save files by default to the Rails.root directory + def work_dir + # Default path set by CarrierWave: + # https://github.com/carrierwaveuploader/carrierwave/blob/v1.0.0/lib/carrierwave/uploader/cache.rb#L182 + CarrierWave.tmp_path + end + + private + + # To prevent files from moving across filesystems, override the default + # implementation: + # http://github.com/carrierwaveuploader/carrierwave/blob/v1.0.0/lib/carrierwave/uploader/cache.rb#L181-L183 + def workfile_path(for_file = original_filename) + # To be safe, keep this directory outside of the the cache directory + # because calling CarrierWave.clean_cache_files! will remove any files in + # the cache directory. + File.join(work_dir, @cache_id, version_name.to_s, for_file) + end end diff --git a/app/uploaders/lfs_object_uploader.rb b/app/uploaders/lfs_object_uploader.rb index 02589959c2f..d11ebf0f9ca 100644 --- a/app/uploaders/lfs_object_uploader.rb +++ b/app/uploaders/lfs_object_uploader.rb @@ -16,16 +16,4 @@ class LfsObjectUploader < GitlabUploader def work_dir File.join(Gitlab.config.lfs.storage_path, 'tmp', 'work') end - - private - - # To prevent LFS files from moving across filesystems, override the default - # implementation: - # http://github.com/carrierwaveuploader/carrierwave/blob/v1.0.0/lib/carrierwave/uploader/cache.rb#L181-L183 - def workfile_path(for_file = original_filename) - # To be safe, keep this directory outside of the the cache directory - # because calling CarrierWave.clean_cache_files! will remove any files in - # the cache directory. - File.join(work_dir, @cache_id, version_name.to_s, for_file) - end end diff --git a/changelogs/unreleased/sh-fix-refactor-uploader-work-dir.yml b/changelogs/unreleased/sh-fix-refactor-uploader-work-dir.yml new file mode 100644 index 00000000000..255608bd89a --- /dev/null +++ b/changelogs/unreleased/sh-fix-refactor-uploader-work-dir.yml @@ -0,0 +1,4 @@ +--- +title: Set artifact working directory to be in the destination store to prevent unnecessary I/O +merge_request: +author: diff --git a/spec/uploaders/artifact_uploader_spec.rb b/spec/uploaders/artifact_uploader_spec.rb index 24e2e3a9f0e..b3fac65c55e 100644 --- a/spec/uploaders/artifact_uploader_spec.rb +++ b/spec/uploaders/artifact_uploader_spec.rb @@ -17,22 +17,29 @@ describe ArtifactUploader do describe '.artifacts_upload_path' do subject { described_class.artifacts_upload_path } - + it { is_expected.to start_with(path) } it { is_expected.to end_with('tmp/uploads/') } end describe '#store_dir' do subject { uploader.store_dir } - + it { is_expected.to start_with(path) } it { is_expected.to end_with("#{job.project_id}/#{job.id}") } end describe '#cache_dir' do subject { uploader.cache_dir } - + + it { is_expected.to start_with(path) } + it { is_expected.to end_with('/tmp/cache') } + end + + describe '#work_dir' do + subject { uploader.work_dir } + it { is_expected.to start_with(path) } - it { is_expected.to end_with('tmp/cache') } + it { is_expected.to end_with('/tmp/work') } end end diff --git a/spec/uploaders/gitlab_uploader_spec.rb b/spec/uploaders/gitlab_uploader_spec.rb index 78e9d9cf46c..a144b39f74f 100644 --- a/spec/uploaders/gitlab_uploader_spec.rb +++ b/spec/uploaders/gitlab_uploader_spec.rb @@ -53,4 +53,19 @@ describe GitlabUploader do expect(subject.move_to_store).to eq(true) end end + + describe '#cache!' do + it 'moves the file from the working directory to the cache directory' do + # One to get the work dir, the other to remove it + expect(subject).to receive(:workfile_path).exactly(2).times.and_call_original + # Test https://github.com/carrierwavesubject/carrierwave/blob/v1.0.0/lib/carrierwave/sanitized_file.rb#L200 + expect(FileUtils).to receive(:mv).with(anything, /^#{subject.work_dir}/).and_call_original + expect(FileUtils).to receive(:mv).with(/^#{subject.work_dir}/, /#{subject.cache_dir}/).and_call_original + + fixture = Rails.root.join('spec', 'fixtures', 'rails_sample.jpg') + subject.cache!(fixture_file_upload(fixture)) + + expect(subject.file.path).to match(/#{subject.cache_dir}/) + end + end end diff --git a/spec/uploaders/lfs_object_uploader_spec.rb b/spec/uploaders/lfs_object_uploader_spec.rb index c3b72e7d677..7088bc23334 100644 --- a/spec/uploaders/lfs_object_uploader_spec.rb +++ b/spec/uploaders/lfs_object_uploader_spec.rb @@ -1,21 +1,9 @@ require 'spec_helper' describe LfsObjectUploader do - let(:uploader) { described_class.new(build_stubbed(:empty_project)) } - - describe '#cache!' do - it 'caches the file in the cache directory' do - # One to get the work dir, the other to remove it - expect(uploader).to receive(:workfile_path).exactly(2).times.and_call_original - expect(FileUtils).to receive(:mv).with(anything, /^#{uploader.work_dir}/).and_call_original - expect(FileUtils).to receive(:mv).with(/^#{uploader.work_dir}/, /^#{uploader.cache_dir}/).and_call_original - - fixture = Rails.root.join('spec', 'fixtures', 'rails_sample.jpg') - uploader.cache!(fixture_file_upload(fixture)) - - expect(uploader.file.path).to start_with(uploader.cache_dir) - end - end + let(:lfs_object) { create(:lfs_object, :with_file) } + let(:uploader) { described_class.new(lfs_object) } + let(:path) { Gitlab.config.lfs.storage_path } describe '#move_to_cache' do it 'is true' do @@ -28,4 +16,25 @@ describe LfsObjectUploader do expect(uploader.move_to_store).to eq(true) end end + + describe '#store_dir' do + subject { uploader.store_dir } + + it { is_expected.to start_with(path) } + it { is_expected.to end_with("#{lfs_object.oid[0, 2]}/#{lfs_object.oid[2, 2]}") } + end + + describe '#cache_dir' do + subject { uploader.cache_dir } + + it { is_expected.to start_with(path) } + it { is_expected.to end_with('/tmp/cache') } + end + + describe '#work_dir' do + subject { uploader.work_dir } + + it { is_expected.to start_with(path) } + it { is_expected.to end_with('/tmp/work') } + end end -- cgit v1.2.1