Commit 965d0394 authored by Grzegorz Bizon's avatar Grzegorz Bizon

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
parents bf4073d5 1fd10d4f
...@@ -19,7 +19,7 @@ module Ci ...@@ -19,7 +19,7 @@ module Ci
has_one :last_deployment, -> { order('deployments.id DESC') }, as: :deployable, class_name: 'Deployment' has_one :last_deployment, -> { order('deployments.id DESC') }, as: :deployable, class_name: 'Deployment'
has_many :trace_sections, class_name: 'Ci::BuildTraceSection' 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_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 has_one :job_artifacts_archive, -> { where(file_type: Ci::JobArtifact.file_types[:archive]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id
......
module Ci module Ci
class BuildTraceChunk < ActiveRecord::Base class BuildTraceChunk < ActiveRecord::Base
include FastDestroyAll
extend Gitlab::Ci::Model extend Gitlab::Ci::Model
belongs_to :build, class_name: "Ci::Build", foreign_key: :build_id belongs_to :build, class_name: "Ci::Build", foreign_key: :build_id
after_destroy :redis_delete_data, if: :redis?
default_value_for :data_store, :redis default_value_for :data_store, :redis
WriteError = Class.new(StandardError) WriteError = Class.new(StandardError)
...@@ -21,6 +20,38 @@ module Ci ...@@ -21,6 +20,38 @@ module Ci
db: 2 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 # Data is memoized for optimizing #size and #end_offset
def data def data
...@@ -63,7 +94,7 @@ module Ci ...@@ -63,7 +94,7 @@ module Ci
break unless size > 0 break unless size > 0
self.update!(raw_data: data, data_store: :db) self.update!(raw_data: data, data_store: :db)
redis_delete_data self.class.redis_delete_data([redis_data_key])
end end
end end
...@@ -121,22 +152,14 @@ module Ci ...@@ -121,22 +152,14 @@ module Ci
end end
end end
def redis_delete_data
Gitlab::Redis::SharedState.with do |redis|
redis.del(redis_data_key)
end
end
def redis_data_key def redis_data_key
"gitlab:ci:trace:#{build_id}:chunks:#{chunk_index}" self.class.redis_data_key(build_id, chunk_index)
end
def redis_lock_key
"trace_write:#{build_id}:chunks:#{chunk_index}"
end end
def in_lock 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 retry_count = 0
until uuid = lease.try_obtain until uuid = lease.try_obtain
...@@ -151,7 +174,7 @@ module Ci ...@@ -151,7 +174,7 @@ module Ci
self.reload if self.persisted? self.reload if self.persisted?
return yield return yield
ensure ensure
Gitlab::ExclusiveLease.cancel(redis_lock_key, uuid) Gitlab::ExclusiveLease.cancel(write_lock_key, uuid)
end end
end end
end end
##
# 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
...@@ -22,6 +22,7 @@ class Project < ActiveRecord::Base ...@@ -22,6 +22,7 @@ class Project < ActiveRecord::Base
include DeploymentPlatform include DeploymentPlatform
include ::Gitlab::Utils::StrongMemoize include ::Gitlab::Utils::StrongMemoize
include ChronicDurationAttribute include ChronicDurationAttribute
include FastDestroyAll::Helpers
extend Gitlab::ConfigHelper extend Gitlab::ConfigHelper
...@@ -81,6 +82,9 @@ class Project < ActiveRecord::Base ...@@ -81,6 +82,9 @@ class Project < ActiveRecord::Base
after_update :update_forks_visibility_level after_update :update_forks_visibility_level
before_destroy :remove_private_deploy_keys before_destroy :remove_private_deploy_keys
use_fast_destroy :build_trace_chunks
after_destroy -> { run_after_commit { remove_pages } } after_destroy -> { run_after_commit { remove_pages } }
after_destroy :remove_exports after_destroy :remove_exports
...@@ -225,6 +229,7 @@ class Project < ActiveRecord::Base ...@@ -225,6 +229,7 @@ class Project < ActiveRecord::Base
# still using `dependent: :destroy` here. # still using `dependent: :destroy` here.
has_many :builds, class_name: 'Ci::Build', inverse_of: :project, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent 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_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 :runner_projects, class_name: 'Ci::RunnerProject'
has_many :runners, through: :runner_projects, source: :runner, class_name: 'Ci::Runner' has_many :runners, through: :runner_projects, source: :runner, class_name: 'Ci::Runner'
has_many :variables, class_name: 'Ci::Variable' has_many :variables, class_name: 'Ci::Variable'
......
---
title: Destroy build_chunks efficiently with FastDestroyAll module
merge_request: 18575
author:
type: performance
...@@ -100,7 +100,7 @@ module Gitlab ...@@ -100,7 +100,7 @@ module Gitlab
FileUtils.rm(trace_path, force: true) FileUtils.rm(trace_path, force: true)
end end
job.trace_chunks.destroy_all job.trace_chunks.fast_destroy_all
job.erase_old_trace! job.erase_old_trace!
end end
......
...@@ -141,7 +141,7 @@ module Gitlab ...@@ -141,7 +141,7 @@ module Gitlab
@size = offset @size = offset
# remove all next chunks # 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 # truncate current chunk
current_chunk.truncate(chunk_offset) current_chunk.truncate(chunk_offset)
...@@ -158,7 +158,7 @@ module Gitlab ...@@ -158,7 +158,7 @@ module Gitlab
end end
def destroy! def destroy!
trace_chunks.destroy_all trace_chunks.fast_destroy_all
@tell = @size = 0 @tell = @size = 0
ensure ensure
invalidate_chunk_cache invalidate_chunk_cache
......
...@@ -276,6 +276,7 @@ project: ...@@ -276,6 +276,7 @@ project:
- import_state - import_state
- members_and_requesters - members_and_requesters
- build_trace_section_names - build_trace_section_names
- build_trace_chunks
- root_of_fork_network - root_of_fork_network
- fork_network_member - fork_network_member
- fork_network - fork_network
......
...@@ -14,6 +14,21 @@ describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state do ...@@ -14,6 +14,21 @@ describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state do
stub_feature_flags(ci_enable_live_trace: true) stub_feature_flags(ci_enable_live_trace: true)
end 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 describe 'CHUNK_SIZE' do
it 'Chunk size can not be changed without special care' do it 'Chunk size can not be changed without special care' do
expect(described_class::CHUNK_SIZE).to eq(128.kilobytes) expect(described_class::CHUNK_SIZE).to eq(128.kilobytes)
......
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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment