diff --git a/app/services/ci/create_trace_artifact_service.rb b/app/services/ci/create_trace_artifact_service.rb index 280a2c3afa4c91fefcf2deeb93b5492a094567a0..ffde824972c7a820e1e90fdb7552781f68d23860 100644 --- a/app/services/ci/create_trace_artifact_service.rb +++ b/app/services/ci/create_trace_artifact_service.rb @@ -4,13 +4,33 @@ 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) + break unless stream.file? + + clone_file!(stream.path, JobArtifactUploader.workhorse_upload_path) do |clone_path| + create_job_trace!(job, clone_path) + FileUtils.rm(stream.path) end end end + + 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 + end + + def clone_file!(src_path, 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 + end end end 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 0000000000000000000000000000000000000000..11f5bc17759247f4c5b6b83c8a5adca6ed020341 --- /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 diff --git a/spec/services/ci/create_trace_artifact_service_spec.rb b/spec/services/ci/create_trace_artifact_service_spec.rb index 847a88920fee4780b78c373206f627621082f4fe..8c5e8e438c73b4c723eec9c5193dc0e8801852a6 100644 --- a/spec/services/ci/create_trace_artifact_service_spec.rb +++ b/spec/services/ci/create_trace_artifact_service_spec.rb @@ -4,40 +4,60 @@ 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 } } + 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 } - 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.exist?(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.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') 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.exist?(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