Merge branch 'live-trace-v2-efficient-destroy-all' into 'master'

Live trace: Use efficient destroy all (for `dependent: :destory` problem)

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

10 files changed, 198 insertions, 20 deletions

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 61a0299d4fb..61c10c427dd 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -19,7 +19,7 @@ module Ci
 
     has_one :last_deployment, -> { order('deployments.id DESC') }, as: :deployable, class_name: 'Deployment'
     has_many :trace_sections, class_name: 'Ci::BuildTraceSection'
-    has_many :trace_chunks, class_name: 'Ci::BuildTraceChunk', foreign_key: :build_id, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
+    has_many :trace_chunks, class_name: 'Ci::BuildTraceChunk', foreign_key: :build_id
 
     has_many :job_artifacts, class_name: 'Ci::JobArtifact', foreign_key: :job_id, dependent: :destroy, inverse_of: :job # rubocop:disable Cop/ActiveRecordDependent
     has_one :job_artifacts_archive, -> { where(file_type: Ci::JobArtifact.file_types[:archive]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id
diff --git a/app/models/ci/build_trace_chunk.rb b/app/models/ci/build_trace_chunk.rb
index d9e923daa71..4856f10846c 100644
--- a/app/models/ci/build_trace_chunk.rb
+++ b/app/models/ci/build_trace_chunk.rb
@@ -1,11 +1,10 @@
 module Ci
   class BuildTraceChunk < ActiveRecord::Base
+    include FastDestroyAll
     extend Gitlab::Ci::Model
 
     belongs_to :build, class_name: "Ci::Build", foreign_key: :build_id
 
-    after_destroy :redis_delete_data, if: :redis?
-
     default_value_for :data_store, :redis
 
     WriteError = Class.new(StandardError)
@@ -21,6 +20,38 @@ module Ci
       db: 2
     }
 
+    class << self
+      def redis_data_key(build_id, chunk_index)
+        "gitlab:ci:trace:#{build_id}:chunks:#{chunk_index}"
+      end
+
+      def redis_data_keys
+        redis.pluck(:build_id, :chunk_index).map do |data|
+          redis_data_key(data.first, data.second)
+        end
+      end
+
+      def redis_delete_data(keys)
+        return if keys.empty?
+
+        Gitlab::Redis::SharedState.with do |redis|
+          redis.del(keys)
+        end
+      end
+
+      ##
+      # FastDestroyAll concerns
+      def begin_fast_destroy
+        redis_data_keys
+      end
+
+      ##
+      # FastDestroyAll concerns
+      def finalize_fast_destroy(keys)
+        redis_delete_data(keys)
+      end
+    end
+
     ##
     # Data is memoized for optimizing #size and #end_offset
     def data
@@ -63,7 +94,7 @@ module Ci
         break unless size > 0
 
         self.update!(raw_data: data, data_store: :db)
-        redis_delete_data
+        self.class.redis_delete_data([redis_data_key])
       end
     end
 
@@ -121,22 +152,14 @@ module Ci
       end
     end
 
-    def redis_delete_data
-      Gitlab::Redis::SharedState.with do |redis|
-        redis.del(redis_data_key)
-      end
-    end
-
     def redis_data_key
-      "gitlab:ci:trace:#{build_id}:chunks:#{chunk_index}"
-    end
-
-    def redis_lock_key
-      "trace_write:#{build_id}:chunks:#{chunk_index}"
+      self.class.redis_data_key(build_id, chunk_index)
     end
 
     def in_lock
-      lease = Gitlab::ExclusiveLease.new(redis_lock_key, timeout: WRITE_LOCK_TTL)
+      write_lock_key = "trace_write:#{build_id}:chunks:#{chunk_index}"
+
+      lease = Gitlab::ExclusiveLease.new(write_lock_key, timeout: WRITE_LOCK_TTL)
       retry_count = 0
 
       until uuid = lease.try_obtain
@@ -151,7 +174,7 @@ module Ci
       self.reload if self.persisted?
       return yield
     ensure
-      Gitlab::ExclusiveLease.cancel(redis_lock_key, uuid)
+      Gitlab::ExclusiveLease.cancel(write_lock_key, uuid)
     end
   end
 end
diff --git a/app/models/concerns/fast_destroy_all.rb b/app/models/concerns/fast_destroy_all.rb
new file mode 100644
index 00000000000..7ea042c6742
--- /dev/null
+++ b/app/models/concerns/fast_destroy_all.rb
@@ -0,0 +1,91 @@
+##
+# This module is for replacing `dependent: :destroy` and `before_destroy` hooks.
+#
+# In general, `destroy_all` is inefficient because it calls each callback with `DELETE` queries i.e. O(n), whereas,
+# `delete_all` is efficient as it deletes all rows with a single `DELETE` query.
+#
+# It's better to use `delete_all` as our best practice, however,
+# if external data (e.g. ObjectStorage, FileStorage or Redis) are assosiated with database records,
+# it is difficult to accomplish it.
+#
+# This module defines a format to use `delete_all` and delete associated external data.
+# Here is an exmaple
+#
+# Situation
+# - `Project` has many `Ci::BuildTraceChunk` through `Ci::Build`
+# - `Ci::BuildTraceChunk` stores associated data in Redis, so it relies on `dependent: :destroy` and `before_destroy` for the deletion
+#
+# How to use
+# - Define `use_fast_destroy :build_trace_chunks` in `Project` model.
+# - Define `begin_fast_destroy` and `finalize_fast_destroy(params)` in `Ci::BuildTraceChunk` model.
+# - Use `fast_destroy_all` instead of `destroy` and `destroy_all`
+# - Remove `dependent: :destroy` and `before_destroy` as it's no longer need
+#
+# Expectation
+# - When a project is `destroy`ed, the associated trace_chunks will be deleted by `delete_all`,
+#   and the associated data will be removed, too.
+# - When `fast_destroy_all` is called, it also performns as same.
+module FastDestroyAll
+  extend ActiveSupport::Concern
+
+  ForbiddenActionError = Class.new(StandardError)
+
+  included do
+    before_destroy do
+      raise ForbiddenActionError, '`destroy` and `destroy_all` are forbbiden. Please use `fast_destroy_all`'
+    end
+  end
+
+  class_methods do
+    ##
+    # This method delete rows and associated external data efficiently
+    #
+    # This method can replace `destroy` and `destroy_all` without having `after_destroy` hook
+    def fast_destroy_all
+      params = begin_fast_destroy
+
+      delete_all
+
+      finalize_fast_destroy(params)
+    end
+
+    ##
+    # This method returns identifiers to delete associated external data (e.g. file paths, redis keys)
+    #
+    # This method must be defined in fast destroyable model
+    def begin_fast_destroy
+      raise NotImplementedError
+    end
+
+    ##
+    # This method deletes associated external data with the identifiers returned by `begin_fast_destroy`
+    #
+    # This method must be defined in fast destroyable model
+    def finalize_fast_destroy(params)
+      raise NotImplementedError
+    end
+  end
+
+  module Helpers
+    extend ActiveSupport::Concern
+
+    class_methods do
+      ##
+      # This method is to be defined on models which have fast destroyable models as children,
+      # and let us avoid to use `dependent: :destroy` hook
+      def use_fast_destroy(relation)
+        before_destroy(prepend: true) do
+          perform_fast_destroy(public_send(relation)) # rubocop:disable GitlabSecurity/PublicSend
+        end
+      end
+    end
+
+    def perform_fast_destroy(subject)
+      params = subject.begin_fast_destroy
+
+      run_after_commit do
+        subject.finalize_fast_destroy(params)
+      end
+    end
+  end
+end
diff --git a/app/models/project.rb b/app/models/project.rb
index b12b694aabd..37f1fcea280 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -22,6 +22,7 @@ class Project < ActiveRecord::Base
   include DeploymentPlatform
   include ::Gitlab::Utils::StrongMemoize
   include ChronicDurationAttribute
+  include FastDestroyAll::Helpers
 
   extend Gitlab::ConfigHelper
 
@@ -81,6 +82,9 @@ class Project < ActiveRecord::Base
   after_update :update_forks_visibility_level
 
   before_destroy :remove_private_deploy_keys
+
+  use_fast_destroy :build_trace_chunks
+
   after_destroy -> { run_after_commit { remove_pages } }
   after_destroy :remove_exports
 
@@ -225,6 +229,7 @@ class Project < ActiveRecord::Base
   # still using `dependent: :destroy` here.
   has_many :builds, class_name: 'Ci::Build', inverse_of: :project, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
   has_many :build_trace_section_names, class_name: 'Ci::BuildTraceSectionName'
+  has_many :build_trace_chunks, class_name: 'Ci::BuildTraceChunk', through: :builds, source: :trace_chunks
   has_many :runner_projects, class_name: 'Ci::RunnerProject'
   has_many :runners, through: :runner_projects, source: :runner, class_name: 'Ci::Runner'
   has_many :variables, class_name: 'Ci::Variable'
diff --git a/changelogs/unreleased/live-trace-v2-efficient-destroy-all.yml b/changelogs/unreleased/live-trace-v2-efficient-destroy-all.yml
new file mode 100644
index 00000000000..ab22739b73d
--- /dev/null
+++ b/changelogs/unreleased/live-trace-v2-efficient-destroy-all.yml
@@ -0,0 +1,5 @@
+---
+title: Destroy build_chunks efficiently with FastDestroyAll module
+merge_request: 18575
+author:
+type: performance
diff --git a/lib/gitlab/ci/trace.rb b/lib/gitlab/ci/trace.rb
index f46d1d39ea7..fe15fabc2e8 100644
--- a/lib/gitlab/ci/trace.rb
+++ b/lib/gitlab/ci/trace.rb
@@ -100,7 +100,7 @@ module Gitlab
           FileUtils.rm(trace_path, force: true)
         end
 
-        job.trace_chunks.destroy_all
+        job.trace_chunks.fast_destroy_all
         job.erase_old_trace!
       end
 
diff --git a/lib/gitlab/ci/trace/chunked_io.rb b/lib/gitlab/ci/trace/chunked_io.rb
index cd3d1364411..bfe0c2a2c26 100644
--- a/lib/gitlab/ci/trace/chunked_io.rb
+++ b/lib/gitlab/ci/trace/chunked_io.rb
@@ -141,7 +141,7 @@ module Gitlab
           @size = offset
 
           # remove all next chunks
-          trace_chunks.where('chunk_index > ?', chunk_index).destroy_all
+          trace_chunks.where('chunk_index > ?', chunk_index).fast_destroy_all
 
           # truncate current chunk
           current_chunk.truncate(chunk_offset)
@@ -158,7 +158,7 @@ module Gitlab
         end
 
         def destroy!
-          trace_chunks.destroy_all
+          trace_chunks.fast_destroy_all
           @tell = @size = 0
         ensure
           invalidate_chunk_cache
diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml
index 830d91de983..a08e0c93df9 100644
--- a/spec/lib/gitlab/import_export/all_models.yml
+++ b/spec/lib/gitlab/import_export/all_models.yml
@@ -276,6 +276,7 @@ project:
 - import_state
 - members_and_requesters
 - build_trace_section_names
+- build_trace_chunks
 - root_of_fork_network
 - fork_network_member
 - fork_network
diff --git a/spec/models/ci/build_trace_chunk_spec.rb b/spec/models/ci/build_trace_chunk_spec.rb
index 46d09dff52c..cbcf1e55979 100644
--- a/spec/models/ci/build_trace_chunk_spec.rb
+++ b/spec/models/ci/build_trace_chunk_spec.rb
@@ -14,6 +14,21 @@ describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state do
     stub_feature_flags(ci_enable_live_trace: true)
   end
 
+  context 'FastDestroyAll' do
+    let(:parent) { create(:project) }
+    let(:pipeline) { create(:ci_pipeline, project: parent) }
+    let(:build) { create(:ci_build, :running, :trace_live, pipeline: pipeline, project: parent) }
+    let(:subjects) { build.trace_chunks }
+
+    it_behaves_like 'fast destroyable'
+
+    def external_data_counter
+      Gitlab::Redis::SharedState.with do |redis|
+        redis.scan_each(match: "gitlab:ci:trace:*:chunks:*").to_a.size
+      end
+    end
+  end
+
   describe 'CHUNK_SIZE' do
     it 'Chunk size can not be changed without special care' do
       expect(described_class::CHUNK_SIZE).to eq(128.kilobytes)
diff --git a/spec/support/shared_examples/fast_destroy_all.rb b/spec/support/shared_examples/fast_destroy_all.rb
new file mode 100644
index 00000000000..5448ddcfe33
--- /dev/null
+++ b/spec/support/shared_examples/fast_destroy_all.rb
@@ -0,0 +1,38 @@
+shared_examples_for 'fast destroyable' do
+  describe 'Forbid #destroy and #destroy_all' do
+    it 'does not delete database rows and associted external data' do
+      expect(external_data_counter).to be > 0
+      expect(subjects.count).to be > 0
+
+      expect { subjects.first.destroy }.to raise_error('`destroy` and `destroy_all` are forbbiden. Please use `fast_destroy_all`')
+      expect { subjects.destroy_all }.to raise_error('`destroy` and `destroy_all` are forbbiden. Please use `fast_destroy_all`')
+
+      expect(subjects.count).to be > 0
+      expect(external_data_counter).to be > 0
+    end
+  end
+
+  describe '.fast_destroy_all' do
+    it 'deletes database rows and associted external data' do
+      expect(external_data_counter).to be > 0
+      expect(subjects.count).to be > 0
+
+      expect { subjects.fast_destroy_all }.not_to raise_error
+
+      expect(subjects.count).to eq(0)
+      expect(external_data_counter).to eq(0)
+    end
+  end
+
+  describe '.use_fast_destroy' do
+    it 'performs cascading delete with fast_destroy_all' do
+      expect(external_data_counter).to be > 0
+      expect(subjects.count).to be > 0
+
+      expect { parent.destroy }.not_to raise_error
+
+      expect(subjects.count).to eq(0)
+      expect(external_data_counter).to eq(0)
+    end
+  end
+end