Commit daff635d authored by Kamil Trzciński's avatar Kamil Trzciński Committed by Clement Ho

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
parent b88c906c
......@@ -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
......
......@@ -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
......@@ -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
---
title: Set artifact working directory to be in the destination store to prevent unnecessary I/O
merge_request:
author:
......@@ -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
......@@ -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
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
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment