Merge branch 'remove-trace-efficiently' into 'master'

Remove redundant query when removing trace

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

5 files changed, 140 insertions, 14 deletions

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index bf93a2caf72..19949f83351 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -386,6 +386,10 @@ module Ci
       trace.exist?
     end
 
+    def has_old_trace?
+      old_trace.present?
+    end
+
     def trace=(data)
       raise NotImplementedError
     end
@@ -395,6 +399,8 @@ module Ci
     end
 
     def erase_old_trace!
+      return unless has_old_trace?
+
       update_column(:trace, nil)
     end
 
diff --git a/changelogs/unreleased/remove-trace-efficiently.yml b/changelogs/unreleased/remove-trace-efficiently.yml
new file mode 100644
index 00000000000..a6ba6d28dce
--- /dev/null
+++ b/changelogs/unreleased/remove-trace-efficiently.yml
@@ -0,0 +1,5 @@
+---
+title: Remove redundant query when removing trace
+merge_request: 20324
+author:
+type: performance
diff --git a/lib/gitlab/ci/trace.rb b/lib/gitlab/ci/trace.rb
index 6be5a59dd38..ee54b893598 100644
--- a/lib/gitlab/ci/trace.rb
+++ b/lib/gitlab/ci/trace.rb
@@ -101,14 +101,17 @@ module Gitlab
       end
 
       def erase!
-        trace_artifact&.destroy
-
-        paths.each do |trace_path|
-          FileUtils.rm(trace_path, force: true)
-        end
-
-        job.trace_chunks.fast_destroy_all
-        job.erase_old_trace!
+        ##
+        # Erase the archived trace
+        trace_artifact&.destroy!
+
+        ##
+        # Erase the live trace
+        job.trace_chunks.fast_destroy_all # Destroy chunks of a live trace
+        FileUtils.rm_f(current_path) if current_path # Remove a trace file of a live trace
+        job.erase_old_trace! if job.has_old_trace? # Remove a trace in database of a live trace
+      ensure
+        @current_path = nil
       end
 
       def archive!
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 0da1234ee3b..090ca159e08 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -514,6 +514,22 @@ describe Ci::Build do
     end
   end
 
+  describe '#has_old_trace?' do
+    subject { build.has_old_trace? }
+
+    context 'when old trace exists' do
+      before do
+        build.update_column(:trace, 'old trace')
+      end
+
+      it { is_expected.to be_truthy }
+    end
+
+    context 'when old trace does not exist' do
+      it { is_expected.to be_falsy }
+    end
+  end
+
   describe '#trace=' do
     it "expect to fail trace=" do
       expect { build.trace = "new" }.to raise_error(NotImplementedError)
@@ -533,16 +549,32 @@ describe Ci::Build do
   end
 
   describe '#erase_old_trace!' do
-    subject { build.send(:read_attribute, :trace) }
+    subject { build.erase_old_trace! }
 
-    before do
-      build.send(:write_attribute, :trace, 'old trace')
+    context 'when old trace exists' do
+      before do
+        build.update_column(:trace, 'old trace')
+      end
+
+      it "erases old trace" do
+        subject
+
+        expect(build.old_trace).to be_nil
+      end
+
+      it "executes UPDATE query" do
+        recorded = ActiveRecord::QueryRecorder.new { subject }
+
+        expect(recorded.log.select { |l| l.match?(/UPDATE.*ci_builds/) }.count).to eq(1)
+      end
     end
 
-    it "expect to receive data from database" do
-      build.erase_old_trace!
+    context 'when old trace does not exist' do
+      it 'does not execute UPDATE query' do
+        recorded = ActiveRecord::QueryRecorder.new { subject }
 
-      is_expected.to be_nil
+        expect(recorded.log.select { |l| l.match?(/UPDATE.*ci_builds/) }.count).to eq(0)
+      end
     end
   end
 
diff --git a/spec/support/shared_examples/ci_trace_shared_examples.rb b/spec/support/shared_examples/ci_trace_shared_examples.rb
index 8a91e05b2d0..94e82b8ce90 100644
--- a/spec/support/shared_examples/ci_trace_shared_examples.rb
+++ b/spec/support/shared_examples/ci_trace_shared_examples.rb
@@ -611,6 +611,55 @@ shared_examples_for 'trace with disabled live trace feature' do
       end
     end
   end
+
+  describe '#erase!' do
+    subject { trace.erase! }
+
+    context 'when it is a live trace' do
+      context 'when trace is stored in database' do
+        let(:build) { create(:ci_build) }
+
+        before do
+          build.update_column(:trace, 'sample trace')
+        end
+
+        it { expect(trace.raw).not_to be_nil }
+
+        it "removes trace" do
+          subject
+
+          expect(trace.raw).to be_nil
+        end
+      end
+
+      context 'when trace is stored in file storage' do
+        let(:build) { create(:ci_build, :trace_live) }
+
+        it { expect(trace.raw).not_to be_nil }
+
+        it "removes trace" do
+          subject
+
+          expect(trace.raw).to be_nil
+        end
+      end
+    end
+
+    context 'when it is an archived trace' do
+      let(:build) { create(:ci_build, :trace_artifact) }
+
+      it "has trace at first" do
+        expect(trace.raw).not_to be_nil
+      end
+
+      it "removes trace" do
+        subject
+
+        build.reload
+        expect(trace.raw).to be_nil
+      end
+    end
+  end
 end
 
 shared_examples_for 'trace with enabled live trace feature' do
@@ -798,4 +847,35 @@ shared_examples_for 'trace with enabled live trace feature' do
       end
     end
   end
+
+  describe '#erase!' do
+    subject { trace.erase! }
+
+    context 'when it is a live trace' do
+      let(:build) { create(:ci_build, :trace_live) }
+
+      it { expect(trace.raw).not_to be_nil }
+
+      it "removes trace" do
+        subject
+
+        expect(trace.raw).to be_nil
+      end
+    end
+
+    context 'when it is an archived trace' do
+      let(:build) { create(:ci_build, :trace_artifact) }
+
+      it "has trace at first" do
+        expect(trace.raw).not_to be_nil
+      end
+
+      it "removes trace" do
+        subject
+
+        build.reload
+        expect(trace.raw).to be_nil
+      end
+    end
+  end
 end