From 801e42715c801c8460849a8ae18a3272515f36ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Fri, 23 Feb 2018 13:07:25 +0100 Subject: [PATCH 01/10] Minimal fix for artifacts service --- .../ci/create_trace_artifact_service.rb | 31 ++++++++++++++++--- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/app/services/ci/create_trace_artifact_service.rb b/app/services/ci/create_trace_artifact_service.rb index 280a2c3afa4..bfc5c1c0732 100644 --- a/app/services/ci/create_trace_artifact_service.rb +++ b/app/services/ci/create_trace_artifact_service.rb @@ -4,13 +4,34 @@ module Ci return if job.job_artifacts_trace job.trace.read do |stream| - if stream.file? - job.create_job_artifacts_trace!( - project: job.project, - file_type: :trace, - file: stream) + return unless stream.file? + + temp_file!(JobArtifactUploader.workhorse_upload_path) do |temp_path| + FileUtils.cp(stream.path, temp_path) + create_job_trace!(temp_path) + FileUtils.rm(stream.path) end end end + + private + + def create_job_trace!(path) + job.create_job_artifacts_trace!( + project: job.project, + file_type: :trace, + file: UploadedFile.new(path, 'build.log', 'application/octet-stream') + ) + end + + def temp_file!(temp_dir) + FileUtils.mkdir_p(temp_dir) + temp_file = Tempfile.new('file', temp_dir) + temp_file&.close + yield(temp_file.path) + ensure + temp_file&.close + temp_file&.unlink + end end end From 0fca3bce812f2c37891b62f80b754099d369f92f Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 23 Feb 2018 23:31:01 +0900 Subject: [PATCH 02/10] Copy original file to temp file always to prevent data loss --- .../ci/create_trace_artifact_service.rb | 8 ++-- .../ci/create_trace_artifact_service_spec.rb | 42 +++++++++++++------ 2 files changed, 33 insertions(+), 17 deletions(-) diff --git a/app/services/ci/create_trace_artifact_service.rb b/app/services/ci/create_trace_artifact_service.rb index bfc5c1c0732..d31d8b63da6 100644 --- a/app/services/ci/create_trace_artifact_service.rb +++ b/app/services/ci/create_trace_artifact_service.rb @@ -8,7 +8,7 @@ module Ci temp_file!(JobArtifactUploader.workhorse_upload_path) do |temp_path| FileUtils.cp(stream.path, temp_path) - create_job_trace!(temp_path) + create_job_trace!(job, temp_path) FileUtils.rm(stream.path) end end @@ -16,17 +16,17 @@ module Ci private - def create_job_trace!(path) + def create_job_trace!(job, path) job.create_job_artifacts_trace!( project: job.project, file_type: :trace, - file: UploadedFile.new(path, 'build.log', 'application/octet-stream') + file: UploadedFile.new(path, 'job.log', 'application/octet-stream') ) end def temp_file!(temp_dir) FileUtils.mkdir_p(temp_dir) - temp_file = Tempfile.new('file', temp_dir) + temp_file = Tempfile.new('legacy-trace-tmp-', temp_dir) temp_file&.close yield(temp_file.path) ensure diff --git a/spec/services/ci/create_trace_artifact_service_spec.rb b/spec/services/ci/create_trace_artifact_service_spec.rb index 847a88920fe..0628e6d112e 100644 --- a/spec/services/ci/create_trace_artifact_service_spec.rb +++ b/spec/services/ci/create_trace_artifact_service_spec.rb @@ -4,40 +4,56 @@ describe Ci::CreateTraceArtifactService do describe '#execute' do subject { described_class.new(nil, nil).execute(job) } - let(:job) { create(:ci_build) } - context 'when the job does not have trace artifact' do context 'when the job has a trace file' do - before do - allow_any_instance_of(Gitlab::Ci::Trace) - .to receive(:default_path) { expand_fixture_path('trace/sample_trace') } + let!(:job) { create(:ci_build, :trace_live) } + let!(:legacy_path) { job.trace.read { |stream| return stream.path } } - allow_any_instance_of(JobArtifactUploader).to receive(:move_to_cache) { false } - allow_any_instance_of(JobArtifactUploader).to receive(:move_to_store) { false } - end + it { expect(File.exists?(legacy_path)).to be_truthy } it 'creates trace artifact' do expect { subject }.to change { Ci::JobArtifact.count }.by(1) - expect(job.job_artifacts_trace.read_attribute(:file)).to eq('sample_trace') + expect(File.exists?(legacy_path)).to be_falsy + expect(File.exists?(job.job_artifacts_trace.file.path)).to be_truthy + expect(job.job_artifacts_trace.exists?).to be_truthy + expect(job.job_artifacts_trace.file.filename).to eq('job.log') end - context 'when the job has already had trace artifact' do + context 'when failed to create trace artifact record' do before do - create(:ci_job_artifact, :trace, job: job) + # When ActiveRecord error happens + allow_any_instance_of(Ci::JobArtifact).to receive(:save).and_return(false) + allow_any_instance_of(Ci::JobArtifact).to receive_message_chain(:errors, :full_messages) + .and_return("Error") + + subject rescue nil + + job.reload end - it 'does not create trace artifact' do - expect { subject }.not_to change { Ci::JobArtifact.count } + it 'keeps legacy trace and removes trace artifact' do + expect(File.exists?(legacy_path)).to be_truthy + expect(job.job_artifacts_trace).to be_nil end end end context 'when the job does not have a trace file' do + let!(:job) { create(:ci_build) } + it 'does not create trace artifact' do expect { subject }.not_to change { Ci::JobArtifact.count } end end end + + context 'when the job has already had trace artifact' do + let!(:job) { create(:ci_build, :trace_artifact) } + + it 'does not create trace artifact' do + expect { subject }.not_to change { Ci::JobArtifact.count } + end + end end end From 414a638bc2460274e5b977094c327b7be6747576 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 23 Feb 2018 23:58:58 +0900 Subject: [PATCH 03/10] Check if the latest permanent file exists before remove file. Add tests. --- .../ci/create_trace_artifact_service.rb | 27 +++++++++---------- .../ci/create_trace_artifact_service_spec.rb | 20 ++++++++++++-- 2 files changed, 31 insertions(+), 16 deletions(-) diff --git a/app/services/ci/create_trace_artifact_service.rb b/app/services/ci/create_trace_artifact_service.rb index d31d8b63da6..00151f4bd1c 100644 --- a/app/services/ci/create_trace_artifact_service.rb +++ b/app/services/ci/create_trace_artifact_service.rb @@ -6,28 +6,27 @@ module Ci job.trace.read do |stream| return unless stream.file? - temp_file!(JobArtifactUploader.workhorse_upload_path) do |temp_path| - FileUtils.cp(stream.path, temp_path) - create_job_trace!(job, temp_path) - FileUtils.rm(stream.path) + temp_file!(stream.path, JobArtifactUploader.workhorse_upload_path) do |temp_path| + job.create_job_artifacts_trace!( + project: job.project, + file_type: :trace, + file: UploadedFile.new(temp_path, 'job.log', 'application/octet-stream') + ) end + + raise 'Trace artifact not found' unless job.job_artifacts_trace.file.exists? + + FileUtils.rm(stream.path) end end private - def create_job_trace!(job, path) - job.create_job_artifacts_trace!( - project: job.project, - file_type: :trace, - file: UploadedFile.new(path, 'job.log', 'application/octet-stream') - ) - end - - def temp_file!(temp_dir) + def temp_file!(src_file, temp_dir) FileUtils.mkdir_p(temp_dir) - temp_file = Tempfile.new('legacy-trace-tmp-', temp_dir) + temp_file = Tempfile.new('trace-tmp-', temp_dir) temp_file&.close + FileUtils.cp(src_file, temp_file.path) yield(temp_file.path) ensure temp_file&.close diff --git a/spec/services/ci/create_trace_artifact_service_spec.rb b/spec/services/ci/create_trace_artifact_service_spec.rb index 0628e6d112e..f928deb8632 100644 --- a/spec/services/ci/create_trace_artifact_service_spec.rb +++ b/spec/services/ci/create_trace_artifact_service_spec.rb @@ -8,6 +8,9 @@ describe Ci::CreateTraceArtifactService do context 'when the job has a trace file' do let!(:job) { create(:ci_build, :trace_live) } let!(:legacy_path) { job.trace.read { |stream| return stream.path } } + let!(:legacy_checksum) { Digest::SHA256.file(legacy_path).hexdigest } + let(:new_path) { job.job_artifacts_trace.file.path } + let(:new_checksum) { Digest::SHA256.file(new_path).hexdigest } it { expect(File.exists?(legacy_path)).to be_truthy } @@ -15,8 +18,9 @@ describe Ci::CreateTraceArtifactService do expect { subject }.to change { Ci::JobArtifact.count }.by(1) expect(File.exists?(legacy_path)).to be_falsy - expect(File.exists?(job.job_artifacts_trace.file.path)).to be_truthy - expect(job.job_artifacts_trace.exists?).to be_truthy + expect(File.exists?(new_path)).to be_truthy + expect(new_checksum).to eq(legacy_checksum) + expect(job.job_artifacts_trace.file.exists?).to be_truthy expect(job.job_artifacts_trace.file.filename).to eq('job.log') end @@ -37,6 +41,18 @@ describe Ci::CreateTraceArtifactService do expect(job.job_artifacts_trace).to be_nil end end + + context 'when migrated trace artifact file is not found' do + before do + allow_any_instance_of(CarrierWave::SanitizedFile).to receive(:exists?) { false } + end + + it 'raises an error' do + expect { subject }.to raise_error('Trace artifact not found') + + expect(File.exists?(legacy_path)).to be_truthy + end + end end context 'when the job does not have a trace file' do From 1670a9fe98dc636cdf455f4177d359934b83b073 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Sat, 24 Feb 2018 00:02:13 +0900 Subject: [PATCH 04/10] Fixed static analysys --- app/services/ci/create_trace_artifact_service.rb | 2 +- spec/services/ci/create_trace_artifact_service_spec.rb | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/services/ci/create_trace_artifact_service.rb b/app/services/ci/create_trace_artifact_service.rb index 00151f4bd1c..baa33de2857 100644 --- a/app/services/ci/create_trace_artifact_service.rb +++ b/app/services/ci/create_trace_artifact_service.rb @@ -4,7 +4,7 @@ module Ci return if job.job_artifacts_trace job.trace.read do |stream| - return unless stream.file? + return unless stream.file? # rubocop:disable Lint/NonLocalExitFromIterator temp_file!(stream.path, JobArtifactUploader.workhorse_upload_path) do |temp_path| job.create_job_artifacts_trace!( diff --git a/spec/services/ci/create_trace_artifact_service_spec.rb b/spec/services/ci/create_trace_artifact_service_spec.rb index f928deb8632..dc22165c9e2 100644 --- a/spec/services/ci/create_trace_artifact_service_spec.rb +++ b/spec/services/ci/create_trace_artifact_service_spec.rb @@ -12,13 +12,13 @@ describe Ci::CreateTraceArtifactService do let(:new_path) { job.job_artifacts_trace.file.path } let(:new_checksum) { Digest::SHA256.file(new_path).hexdigest } - it { expect(File.exists?(legacy_path)).to be_truthy } + it { expect(File.exist?(legacy_path)).to be_truthy } it 'creates trace artifact' do expect { subject }.to change { Ci::JobArtifact.count }.by(1) - expect(File.exists?(legacy_path)).to be_falsy - expect(File.exists?(new_path)).to be_truthy + expect(File.exist?(legacy_path)).to be_falsy + expect(File.exist?(new_path)).to be_truthy expect(new_checksum).to eq(legacy_checksum) expect(job.job_artifacts_trace.file.exists?).to be_truthy expect(job.job_artifacts_trace.file.filename).to eq('job.log') @@ -37,7 +37,7 @@ describe Ci::CreateTraceArtifactService do end it 'keeps legacy trace and removes trace artifact' do - expect(File.exists?(legacy_path)).to be_truthy + expect(File.exist?(legacy_path)).to be_truthy expect(job.job_artifacts_trace).to be_nil end end @@ -50,7 +50,7 @@ describe Ci::CreateTraceArtifactService do it 'raises an error' do expect { subject }.to raise_error('Trace artifact not found') - expect(File.exists?(legacy_path)).to be_truthy + expect(File.exist?(legacy_path)).to be_truthy end end end From 76d70841926d21832040d6e7801ed67a7e7f88ee Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Sat, 24 Feb 2018 00:04:23 +0900 Subject: [PATCH 05/10] Add chnagelog --- changelogs/unreleased/minimal-fix-for-artifacts-service.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/minimal-fix-for-artifacts-service.yml diff --git a/changelogs/unreleased/minimal-fix-for-artifacts-service.yml b/changelogs/unreleased/minimal-fix-for-artifacts-service.yml new file mode 100644 index 00000000000..11f5bc17759 --- /dev/null +++ b/changelogs/unreleased/minimal-fix-for-artifacts-service.yml @@ -0,0 +1,5 @@ +--- +title: Prevent trace artifact migration to incur data loss +merge_request: 17313 +author: +type: fixed From aa603812e3d4917107af69d114ea1ab4050aebb7 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Sat, 24 Feb 2018 01:44:45 +0900 Subject: [PATCH 06/10] Bring back the initial commit --- .../ci/create_trace_artifact_service.rb | 27 ++++++++++--------- .../ci/create_trace_artifact_service_spec.rb | 12 --------- 2 files changed, 14 insertions(+), 25 deletions(-) diff --git a/app/services/ci/create_trace_artifact_service.rb b/app/services/ci/create_trace_artifact_service.rb index baa33de2857..3a6d0f6f8e5 100644 --- a/app/services/ci/create_trace_artifact_service.rb +++ b/app/services/ci/create_trace_artifact_service.rb @@ -4,29 +4,30 @@ module Ci return if job.job_artifacts_trace job.trace.read do |stream| - return unless stream.file? # rubocop:disable Lint/NonLocalExitFromIterator + break unless stream.file? - temp_file!(stream.path, JobArtifactUploader.workhorse_upload_path) do |temp_path| - job.create_job_artifacts_trace!( - project: job.project, - file_type: :trace, - file: UploadedFile.new(temp_path, 'job.log', 'application/octet-stream') - ) + temp_file!(JobArtifactUploader.workhorse_upload_path) do |temp_path| + FileUtils.cp(stream.path, temp_path) + create_job_trace!(job, temp_path) + FileUtils.rm(stream.path) end - - raise 'Trace artifact not found' unless job.job_artifacts_trace.file.exists? - - FileUtils.rm(stream.path) end end private - def temp_file!(src_file, temp_dir) + def create_job_trace!(job, path) + job.create_job_artifacts_trace!( + project: job.project, + file_type: :trace, + file: UploadedFile.new(path, 'job.log', 'application/octet-stream') + ) + end + + def temp_file!(temp_dir) FileUtils.mkdir_p(temp_dir) temp_file = Tempfile.new('trace-tmp-', temp_dir) temp_file&.close - FileUtils.cp(src_file, temp_file.path) yield(temp_file.path) ensure temp_file&.close diff --git a/spec/services/ci/create_trace_artifact_service_spec.rb b/spec/services/ci/create_trace_artifact_service_spec.rb index dc22165c9e2..8c5e8e438c7 100644 --- a/spec/services/ci/create_trace_artifact_service_spec.rb +++ b/spec/services/ci/create_trace_artifact_service_spec.rb @@ -41,18 +41,6 @@ describe Ci::CreateTraceArtifactService do expect(job.job_artifacts_trace).to be_nil end end - - context 'when migrated trace artifact file is not found' do - before do - allow_any_instance_of(CarrierWave::SanitizedFile).to receive(:exists?) { false } - end - - it 'raises an error' do - expect { subject }.to raise_error('Trace artifact not found') - - expect(File.exist?(legacy_path)).to be_truthy - end - end end context 'when the job does not have a trace file' do From 4a54cc4cd3558413296df5ec14db6788e91e8586 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Sat, 24 Feb 2018 02:04:32 +0900 Subject: [PATCH 07/10] Change FileUtils.cp to FileUtils.copy --- app/services/ci/create_trace_artifact_service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/ci/create_trace_artifact_service.rb b/app/services/ci/create_trace_artifact_service.rb index 3a6d0f6f8e5..dae741a371c 100644 --- a/app/services/ci/create_trace_artifact_service.rb +++ b/app/services/ci/create_trace_artifact_service.rb @@ -7,7 +7,7 @@ module Ci break unless stream.file? temp_file!(JobArtifactUploader.workhorse_upload_path) do |temp_path| - FileUtils.cp(stream.path, temp_path) + FileUtils.copy(stream.path, temp_path) create_job_trace!(job, temp_path) FileUtils.rm(stream.path) end From 28c6a6b04436230e6e9b29855e4d4d4f88d9a554 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Sat, 24 Feb 2018 02:32:20 +0900 Subject: [PATCH 08/10] Use Dir.mktmpdir --- .../ci/create_trace_artifact_service.rb | 29 +++++++++---------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/app/services/ci/create_trace_artifact_service.rb b/app/services/ci/create_trace_artifact_service.rb index dae741a371c..ffde824972c 100644 --- a/app/services/ci/create_trace_artifact_service.rb +++ b/app/services/ci/create_trace_artifact_service.rb @@ -6,9 +6,8 @@ module Ci job.trace.read do |stream| break unless stream.file? - temp_file!(JobArtifactUploader.workhorse_upload_path) do |temp_path| - FileUtils.copy(stream.path, temp_path) - create_job_trace!(job, temp_path) + clone_file!(stream.path, JobArtifactUploader.workhorse_upload_path) do |clone_path| + create_job_trace!(job, clone_path) FileUtils.rm(stream.path) end end @@ -17,21 +16,21 @@ module Ci private def create_job_trace!(job, path) - job.create_job_artifacts_trace!( - project: job.project, - file_type: :trace, - file: UploadedFile.new(path, 'job.log', 'application/octet-stream') - ) + File.open(path) do |stream| + job.create_job_artifacts_trace!( + project: job.project, + file_type: :trace, + file: stream) + end end - def temp_file!(temp_dir) + def clone_file!(src_path, temp_dir) FileUtils.mkdir_p(temp_dir) - temp_file = Tempfile.new('trace-tmp-', temp_dir) - temp_file&.close - yield(temp_file.path) - ensure - temp_file&.close - temp_file&.unlink + Dir.mktmpdir('tmp-trace', temp_dir) do |dir_path| + temp_path = File.join(dir_path, "job.log") + FileUtils.copy(src_path, temp_path) + yield(temp_path) + end end end end From 34e16110e0fbe29c40fe984d4715ca5dcdaef508 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Sat, 24 Feb 2018 02:35:03 +0900 Subject: [PATCH 09/10] Revert "Use Dir.mktmpdir" This reverts commit 28c6a6b04436230e6e9b29855e4d4d4f88d9a554. --- .../ci/create_trace_artifact_service.rb | 29 ++++++++++--------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/app/services/ci/create_trace_artifact_service.rb b/app/services/ci/create_trace_artifact_service.rb index ffde824972c..dae741a371c 100644 --- a/app/services/ci/create_trace_artifact_service.rb +++ b/app/services/ci/create_trace_artifact_service.rb @@ -6,8 +6,9 @@ module Ci job.trace.read do |stream| break unless stream.file? - clone_file!(stream.path, JobArtifactUploader.workhorse_upload_path) do |clone_path| - create_job_trace!(job, clone_path) + temp_file!(JobArtifactUploader.workhorse_upload_path) do |temp_path| + FileUtils.copy(stream.path, temp_path) + create_job_trace!(job, temp_path) FileUtils.rm(stream.path) end end @@ -16,21 +17,21 @@ module Ci private def create_job_trace!(job, path) - File.open(path) do |stream| - job.create_job_artifacts_trace!( - project: job.project, - file_type: :trace, - file: stream) - end + job.create_job_artifacts_trace!( + project: job.project, + file_type: :trace, + file: UploadedFile.new(path, 'job.log', 'application/octet-stream') + ) end - def clone_file!(src_path, temp_dir) + def temp_file!(temp_dir) FileUtils.mkdir_p(temp_dir) - Dir.mktmpdir('tmp-trace', temp_dir) do |dir_path| - temp_path = File.join(dir_path, "job.log") - FileUtils.copy(src_path, temp_path) - yield(temp_path) - end + temp_file = Tempfile.new('trace-tmp-', temp_dir) + temp_file&.close + yield(temp_file.path) + ensure + temp_file&.close + temp_file&.unlink end end end From 8a56c5a1ac744a29a589c5a2ee1f26929f74105d Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Sat, 24 Feb 2018 02:40:41 +0900 Subject: [PATCH 10/10] Revert "Revert "Use Dir.mktmpdir"" This reverts commit 34e16110e0fbe29c40fe984d4715ca5dcdaef508. --- .../ci/create_trace_artifact_service.rb | 29 +++++++++---------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/app/services/ci/create_trace_artifact_service.rb b/app/services/ci/create_trace_artifact_service.rb index dae741a371c..ffde824972c 100644 --- a/app/services/ci/create_trace_artifact_service.rb +++ b/app/services/ci/create_trace_artifact_service.rb @@ -6,9 +6,8 @@ module Ci job.trace.read do |stream| break unless stream.file? - temp_file!(JobArtifactUploader.workhorse_upload_path) do |temp_path| - FileUtils.copy(stream.path, temp_path) - create_job_trace!(job, temp_path) + clone_file!(stream.path, JobArtifactUploader.workhorse_upload_path) do |clone_path| + create_job_trace!(job, clone_path) FileUtils.rm(stream.path) end end @@ -17,21 +16,21 @@ module Ci private def create_job_trace!(job, path) - job.create_job_artifacts_trace!( - project: job.project, - file_type: :trace, - file: UploadedFile.new(path, 'job.log', 'application/octet-stream') - ) + File.open(path) do |stream| + job.create_job_artifacts_trace!( + project: job.project, + file_type: :trace, + file: stream) + end end - def temp_file!(temp_dir) + def clone_file!(src_path, temp_dir) FileUtils.mkdir_p(temp_dir) - temp_file = Tempfile.new('trace-tmp-', temp_dir) - temp_file&.close - yield(temp_file.path) - ensure - temp_file&.close - temp_file&.unlink + Dir.mktmpdir('tmp-trace', temp_dir) do |dir_path| + temp_path = File.join(dir_path, "job.log") + FileUtils.copy(src_path, temp_path) + yield(temp_path) + end end end end