diff options
-rw-r--r-- | app/models/ci/build.rb | 2 | ||||
-rw-r--r-- | app/models/ci/build_trace_chunk.rb | 55 | ||||
-rw-r--r-- | app/models/concerns/fast_destroy_all.rb | 91 | ||||
-rw-r--r-- | app/models/project.rb | 5 | ||||
-rw-r--r-- | changelogs/unreleased/live-trace-v2-efficient-destroy-all.yml | 5 | ||||
-rw-r--r-- | lib/gitlab/ci/trace.rb | 2 | ||||
-rw-r--r-- | lib/gitlab/ci/trace/chunked_io.rb | 4 | ||||
-rw-r--r-- | spec/lib/gitlab/import_export/all_models.yml | 1 | ||||
-rw-r--r-- | spec/models/ci/build_trace_chunk_spec.rb | 15 | ||||
-rw-r--r-- | spec/support/shared_examples/fast_destroy_all.rb | 38 |
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 |