Commit 70987ae1 authored by Marius Bobin's avatar Marius Bobin

Destroy trace chunks and data when deleting pipelines

Changelog: fixed
parent aed836d7
...@@ -69,6 +69,7 @@ module Ci ...@@ -69,6 +69,7 @@ module Ci
has_many :builds, foreign_key: :commit_id, inverse_of: :pipeline has_many :builds, foreign_key: :commit_id, inverse_of: :pipeline
has_many :generic_commit_statuses, foreign_key: :commit_id, inverse_of: :pipeline, class_name: 'GenericCommitStatus' has_many :generic_commit_statuses, foreign_key: :commit_id, inverse_of: :pipeline, class_name: 'GenericCommitStatus'
has_many :job_artifacts, through: :builds has_many :job_artifacts, through: :builds
has_many :build_trace_chunks, class_name: 'Ci::BuildTraceChunk', through: :builds, source: :trace_chunks
has_many :trigger_requests, dependent: :destroy, foreign_key: :commit_id # rubocop:disable Cop/ActiveRecordDependent has_many :trigger_requests, dependent: :destroy, foreign_key: :commit_id # rubocop:disable Cop/ActiveRecordDependent
has_many :variables, class_name: 'Ci::PipelineVariable' has_many :variables, class_name: 'Ci::PipelineVariable'
has_many :deployments, through: :builds has_many :deployments, through: :builds
...@@ -130,6 +131,7 @@ module Ci ...@@ -130,6 +131,7 @@ module Ci
after_create :keep_around_commits, unless: :importing? after_create :keep_around_commits, unless: :importing?
use_fast_destroy :job_artifacts use_fast_destroy :job_artifacts
use_fast_destroy :build_trace_chunks
# We use `Enums::Ci::Pipeline.sources` here so that EE can more easily extend # We use `Enums::Ci::Pipeline.sources` here so that EE can more easily extend
# this `Hash` with new values. # this `Hash` with new values.
......
...@@ -9,9 +9,11 @@ module Ci ...@@ -9,9 +9,11 @@ module Ci
pipeline.cancel_running if pipeline.cancelable? pipeline.cancel_running if pipeline.cancelable?
# Ci::Pipeline#destroy triggers `use_fast_destroy :job_artifacts` and # The pipeline, the builds, job and pipeline artifacts all get destroyed here.
# ci_builds has ON DELETE CASCADE to ci_pipelines. The pipeline, the builds, # Ci::Pipeline#destroy triggers fast destroy on job_artifacts and
# job and pipeline artifacts all get destroyed here. # build_trace_chunks to remove the records and data stored in object storage.
# ci_builds records are deleted using ON DELETE CASCADE from ci_pipelines
#
pipeline.reset.destroy! pipeline.reset.destroy!
ServiceResponse.success(message: 'Pipeline not found') ServiceResponse.success(message: 'Pipeline not found')
......
...@@ -281,6 +281,7 @@ ci_pipelines: ...@@ -281,6 +281,7 @@ ci_pipelines:
- dast_site_profiles_pipeline - dast_site_profiles_pipeline
- package_build_infos - package_build_infos
- package_file_build_infos - package_file_build_infos
- build_trace_chunks
ci_refs: ci_refs:
- project - project
- ci_pipelines - ci_pipelines
......
...@@ -49,9 +49,8 @@ RSpec.describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state, :clean_git ...@@ -49,9 +49,8 @@ RSpec.describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state, :clean_git
end end
context 'FastDestroyAll' do context 'FastDestroyAll' do
let(:project) { create(:project) } let(:pipeline) { create(:ci_pipeline) }
let(:pipeline) { create(:ci_pipeline, project: project) } let!(:build) { create(:ci_build, :running, :trace_live, pipeline: pipeline) }
let!(:build) { create(:ci_build, :running, :trace_live, pipeline: pipeline, project: project) }
let(:subjects) { build.trace_chunks } let(:subjects) { build.trace_chunks }
describe 'Forbid #destroy and #destroy_all' do describe 'Forbid #destroy and #destroy_all' do
...@@ -84,13 +83,7 @@ RSpec.describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state, :clean_git ...@@ -84,13 +83,7 @@ RSpec.describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state, :clean_git
expect(external_data_counter).to be > 0 expect(external_data_counter).to be > 0
expect(subjects.count).to be > 0 expect(subjects.count).to be > 0
::Gitlab::Database::QueryAnalyzers::PreventCrossDatabaseModification.allow_cross_database_modification_within_transaction(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/350185') do expect { pipeline.destroy! }.not_to raise_error
# This should use to prevent cross-DB modification
# but due to https://gitlab.com/gitlab-org/gitlab/-/issues/350185
# the build trace chunks are not destroyed by Projects::DestroyService
# Change to: expect { Projects::DestroyService.new(project, project.owner).execute }.to eq(true)
expect { project.destroy! }.not_to raise_error
end
expect(subjects.count).to eq(0) expect(subjects.count).to eq(0)
expect(external_data_counter).to eq(0) expect(external_data_counter).to eq(0)
...@@ -858,13 +851,7 @@ RSpec.describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state, :clean_git ...@@ -858,13 +851,7 @@ RSpec.describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state, :clean_git
context 'when project is destroyed' do context 'when project is destroyed' do
let(:subject) do let(:subject) do
::Gitlab::Database::QueryAnalyzers::PreventCrossDatabaseModification.allow_cross_database_modification_within_transaction(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/350185') do Projects::DestroyService.new(project, project.owner).execute
# This should use to prevent cross-DB modification
# but due to https://gitlab.com/gitlab-org/gitlab/-/issues/350185
# the build trace chunks are not destroyed by Projects::DestroyService
# Change to: Projects::DestroyService.new(project, project.owner).execute
project.destroy!
end
end end
it_behaves_like 'deletes all build_trace_chunk and data in redis' it_behaves_like 'deletes all build_trace_chunk and data in redis'
......
...@@ -31,6 +31,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do ...@@ -31,6 +31,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
it { is_expected.to have_many(:statuses_order_id_desc) } it { is_expected.to have_many(:statuses_order_id_desc) }
it { is_expected.to have_many(:bridges) } it { is_expected.to have_many(:bridges) }
it { is_expected.to have_many(:job_artifacts).through(:builds) } it { is_expected.to have_many(:job_artifacts).through(:builds) }
it { is_expected.to have_many(:build_trace_chunks).through(:builds) }
it { is_expected.to have_many(:auto_canceled_pipelines) } it { is_expected.to have_many(:auto_canceled_pipelines) }
it { is_expected.to have_many(:auto_canceled_jobs) } it { is_expected.to have_many(:auto_canceled_jobs) }
it { is_expected.to have_many(:sourced_pipelines) } it { is_expected.to have_many(:sourced_pipelines) }
......
...@@ -66,6 +66,28 @@ RSpec.describe ::Ci::DestroyPipelineService do ...@@ -66,6 +66,28 @@ RSpec.describe ::Ci::DestroyPipelineService do
expect { subject }.to change { Ci::DeletedObject.count } expect { subject }.to change { Ci::DeletedObject.count }
end end
end end
context 'when job has trace chunks' do
let(:connection_params) { Gitlab.config.artifacts.object_store.connection.symbolize_keys }
let(:connection) { ::Fog::Storage.new(connection_params) }
before do
stub_object_storage(connection_params: connection_params, remote_directory: 'artifacts')
stub_artifacts_object_storage
end
let!(:trace_chunk) { create(:ci_build_trace_chunk, :fog_with_data, build: build) }
it 'destroys associated trace chunks' do
subject
expect { trace_chunk.reload }.to raise_error(ActiveRecord::RecordNotFound)
end
it 'removes data from object store' do
expect { subject }.to change { Ci::BuildTraceChunks::Fog.new.data(trace_chunk) }
end
end
end end
context 'when pipeline is in cancelable state' do context 'when pipeline is in cancelable state' do
......
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