Merge branch 'minimal-fix-for-artifacts-service' into 'master'

Minimal fix for artifacts service

Closes #43022

See merge request gitlab-org/gitlab-ce!17313

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

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

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