Merge branch 'ab-remove-transaction-from-artifact-deletion' into 'master'

Remove project destroy transaction behind flag

See merge request gitlab-org/gitlab!39367

app/services/projects/destroy_service.rb

@@ -28,7 +28,7 @@ module Projects
 
       Projects::UnlinkForkService.new(project, current_user).execute
 
-      attempt_destroy_transaction(project)
+      attempt_destroy(project)
 
       system_hook_service.execute_hooks_for(project, :destroy)
       log_info("Project \"#{project.full_path}\" was deleted")
@@ -98,14 +98,21 @@ module Projects
       log_error("Deletion failed on #{project.full_path} with the following message: #{message}")
     end
 
-    def attempt_destroy_transaction(project)
+    def attempt_destroy(project)
       unless remove_registry_tags
         raise_error(s_('DeleteProject|Failed to remove some tags in project container registry. Please try again or contact administrator.'))
       end
 
       project.leave_pool_repository
 
-      Project.transaction do
+      if Gitlab::Ci::Features.project_transactionless_destroy?(project)
+        destroy_project_related_records(project)
+      else
+        Project.transaction { destroy_project_related_records(project) }
+      end
+    end
+
+    def destroy_project_related_records(project)
       log_destroy_event
       trash_relation_repositories!
       trash_project_repositories!
@@ -119,7 +126,6 @@ module Projects
       project.destroy_dependent_associations_in_batches(exclude: [:container_repositories, :snippets])
       project.destroy!
     end
-    end
 
     def log_destroy_event
       log_info("Attempting to destroy #{project.full_path} (#{project.id})")

ee/app/services/ee/projects/destroy_service.rb

@@ -17,10 +17,30 @@ module EE
         end
       end
 
-      override :log_destroy_event
-      def log_destroy_event
-        super
+      # Removes physical repository in a Geo replicated secondary node
+      # There is no need to do any database operation as it will be
+      # replicated by itself.
+      def geo_replicate
+        return unless ::Gitlab::Geo.secondary?
+
+        # Flush the cache for both repositories. This has to be done _before_
+        # removing the physical repositories as some expiration code depends on
+        # Git data (e.g. a list of branch names).
+        flush_caches(project)
+
+        trash_project_repositories!
+
+        log_info("Project \"#{project.name}\" was removed")
+      end
+
+      private
+
+      override :destroy_project_related_records
+      def destroy_project_related_records(project)
+        super && log_destroy_events
+      end
 
+      def log_destroy_events
         log_geo_event(project)
         log_audit_event(project)
       end
@@ -39,24 +59,6 @@ module EE
         ).create!
       end
 
-      # Removes physical repository in a Geo replicated secondary node
-      # There is no need to do any database operation as it will be
-      # replicated by itself.
-      def geo_replicate
-        return unless ::Gitlab::Geo.secondary?
-
-        # Flush the cache for both repositories. This has to be done _before_
-        # removing the physical repositories as some expiration code depends on
-        # Git data (e.g. a list of branch names).
-        flush_caches(project)
-
-        trash_project_repositories!
-
-        log_info("Project \"#{project.name}\" was removed")
-      end
-
-      private
-
       def log_audit_event(project)
         ::AuditEventService.new(
           current_user,

ee/spec/services/projects/destroy_service_spec.rb

@@ -20,16 +20,18 @@ RSpec.describe Projects::DestroyService do
     stub_container_registry_tags(repository: :any, tags: [])
   end
 
+  shared_examples 'project destroy ee' do
     context 'when project is a mirror' do
-    it 'decrements capacity if mirror was scheduled' do
-      max_capacity = Gitlab::CurrentSettings.mirror_max_capacity
-      project_mirror = create(:project, :mirror, :repository, :import_scheduled)
+      let(:max_capacity) { Gitlab::CurrentSettings.mirror_max_capacity }
+      let_it_be(:project_mirror) { create(:project, :mirror, :repository, :import_scheduled) }
+      let(:result) { described_class.new(project_mirror, project_mirror.owner, {}).execute }
 
+      before do
         Gitlab::Mirror.increment_capacity(project_mirror.id)
+      end
 
-      expect do
-        described_class.new(project_mirror, project_mirror.owner, {}).execute
-      end.to change { Gitlab::Mirror.available_capacity }.from(max_capacity - 1).to(max_capacity)
+      it 'decrements capacity if mirror was scheduled' do
+        expect {result}.to change { Gitlab::Mirror.available_capacity }.from(max_capacity - 1).to(max_capacity)
       end
     end
 
@@ -44,15 +46,14 @@ RSpec.describe Projects::DestroyService do
       it 'logs an event to the Geo event log' do
         # Run Sidekiq immediately to check that renamed repository will be removed
         Sidekiq::Testing.inline! do
-        expect(subject).to receive(:log_destroy_event).and_call_original
+          expect(subject).to receive(:log_destroy_events).and_call_original
           expect { subject.execute }.to change(Geo::RepositoryDeletedEvent, :count).by(1)
         end
       end
 
       it 'does not log event to the Geo log if project deletion fails' do
         expect(subject).to receive(:log_destroy_event).and_call_original
-      expect_any_instance_of(Project)
-        .to receive(:destroy!).and_raise(StandardError.new('Other error message'))
+        expect(project).to receive(:destroy!).and_raise(StandardError.new('Other error message'))
 
         Sidekiq::Testing.inline! do
           expect { subject.execute }.not_to change(Geo::RepositoryDeletedEvent, :count)
@@ -63,9 +64,9 @@ RSpec.describe Projects::DestroyService do
     context 'audit events' do
       include_examples 'audit event logging' do
         let(:operation) { subject.execute }
+
         let(:fail_condition!) do
-        expect_any_instance_of(Project)
-          .to receive(:destroy!).and_raise(StandardError.new('Other error message'))
+          expect(project).to receive(:destroy!).and_raise(StandardError.new('Other error message'))
         end
 
         let(:attributes) do
@@ -95,4 +96,17 @@ RSpec.describe Projects::DestroyService do
         expect { subject.execute }.to change(AuditEvent, :count)
       end
     end
+  end
+
+  context 'when project_transactionless_destroy enabled' do
+    it_behaves_like 'project destroy ee'
+  end
+
+  context 'when project_transactionless_destroy disabled', :sidekiq_inline do
+    before do
+      stub_feature_flags(project_transactionless_destroy: false)
+    end
+
+    it_behaves_like 'project destroy ee'
+  end
 end

lib/gitlab/ci/features.rb

@@ -79,6 +79,10 @@ module Gitlab
       def self.expand_names_for_cross_pipeline_artifacts?(project)
         ::Feature.enabled?(:ci_expand_names_for_cross_pipeline_artifacts, project)
       end
+
+      def self.project_transactionless_destroy?(project)
+        Feature.enabled?(:project_transactionless_destroy, project, default_enabled: false)
+      end
     end
   end
 end

spec/services/projects/destroy_service_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Projects::DestroyService do
+RSpec.describe Projects::DestroyService, :aggregate_failures do
   include ProjectForksHelper
 
   let_it_be(:user) { create(:user) }
@@ -60,6 +60,7 @@ RSpec.describe Projects::DestroyService do
     end
   end
 
+  shared_examples 'project destroy' do
     it_behaves_like 'deleting the project'
 
     it 'invalidates personal_project_count cache' do
@@ -371,6 +372,41 @@ RSpec.describe Projects::DestroyService do
       end
     end
 
+    context 'error while destroying', :sidekiq_inline do
+      let!(:pipeline) { create(:ci_pipeline, project: project) }
+      let!(:builds) { create_list(:ci_build, 2, :artifacts, pipeline: pipeline) }
+      let!(:build_trace) { create(:ci_build_trace_chunk, build: builds[0]) }
+
+      it 'deletes on retry' do
+        # We can expect this to timeout for very large projects
+        # TODO: remove allow_next_instance_of: https://gitlab.com/gitlab-org/gitlab/-/issues/220440
+        allow_any_instance_of(Ci::Build).to receive(:destroy).and_raise('boom')
+        destroy_project(project, user, {})
+
+        allow_any_instance_of(Ci::Build).to receive(:destroy).and_call_original
+        destroy_project(project, user, {})
+
+        expect(Project.unscoped.all).not_to include(project)
+        expect(project.gitlab_shell.repository_exists?(project.repository_storage, path + '.git')).to be_falsey
+        expect(project.gitlab_shell.repository_exists?(project.repository_storage, remove_path + '.git')).to be_falsey
+        expect(project.all_pipelines).to be_empty
+        expect(project.builds).to be_empty
+      end
+    end
+  end
+
+  context 'when project_transactionless_destroy enabled' do
+    it_behaves_like 'project destroy'
+  end
+
+  context 'when project_transactionless_destroy disabled', :sidekiq_inline do
+    before do
+      stub_feature_flags(project_transactionless_destroy: false)
+    end
+
+    it_behaves_like 'project destroy'
+  end
+
   def destroy_project(project, user, params = {})
     described_class.new(project, user, params).public_send(async ? :async_execute : :execute)
   end