Merge branch '350185-fast-detroy-trace-chunks-with-pipeline' into 'master'

Destroy trace chunks and data when deleting pipelines

See merge request gitlab-org/gitlab!78116

app/models/ci/pipeline.rb

@@ -69,6 +69,7 @@ module Ci
     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 :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 :variables, class_name: 'Ci::PipelineVariable'
     has_many :deployments, through: :builds
@@ -130,6 +131,7 @@ module Ci
     after_create :keep_around_commits, unless: :importing?
 
     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
     # this `Hash` with new values.

app/services/ci/destroy_pipeline_service.rb

@@ -9,9 +9,11 @@ module Ci
 
       pipeline.cancel_running if pipeline.cancelable?
 
-      # Ci::Pipeline#destroy triggers `use_fast_destroy :job_artifacts` and
-      # ci_builds has ON DELETE CASCADE to ci_pipelines. The pipeline, the builds,
-      # job and pipeline artifacts all get destroyed here.
+      # The pipeline, the builds, job and pipeline artifacts all get destroyed here.
+      # Ci::Pipeline#destroy triggers fast destroy on job_artifacts and
+      # 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!
 
       ServiceResponse.success(message: 'Pipeline not found')

spec/lib/gitlab/import_export/all_models.yml

@@ -281,6 +281,7 @@ ci_pipelines:
 - dast_site_profiles_pipeline
 - package_build_infos
 - package_file_build_infos
+- build_trace_chunks
 ci_refs:
 - project
 - ci_pipelines

spec/models/ci/build_trace_chunk_spec.rb

@@ -49,9 +49,8 @@ RSpec.describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state, :clean_git
   end
 
   context 'FastDestroyAll' do
-    let(:project) { create(:project) }
-    let(:pipeline) { create(:ci_pipeline, project: project) }
-    let!(:build) { create(:ci_build, :running, :trace_live, pipeline: pipeline, project: project) }
+    let(:pipeline) { create(:ci_pipeline) }
+    let!(:build) { create(:ci_build, :running, :trace_live, pipeline: pipeline) }
     let(:subjects) { build.trace_chunks }
 
     describe 'Forbid #destroy and #destroy_all' do
@@ -84,13 +83,7 @@ RSpec.describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state, :clean_git
         expect(external_data_counter).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
-          # 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 { pipeline.destroy! }.not_to raise_error
 
         expect(subjects.count).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
 
     context 'when project is destroyed' do
       let(:subject) do
-        ::Gitlab::Database::QueryAnalyzers::PreventCrossDatabaseModification.allow_cross_database_modification_within_transaction(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/350185') do
-          # 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
+        Projects::DestroyService.new(project, project.owner).execute
       end
 
       it_behaves_like 'deletes all build_trace_chunk and data in redis'

spec/models/ci/pipeline_spec.rb

@@ -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(:bridges) }
   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_jobs) }
   it { is_expected.to have_many(:sourced_pipelines) }

spec/services/ci/destroy_pipeline_service_spec.rb

@@ -66,6 +66,28 @@ RSpec.describe ::Ci::DestroyPipelineService do
           expect { subject }.to change { Ci::DeletedObject.count }
         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
 
     context 'when pipeline is in cancelable state' do