Merge branch 'ab-introduce-feature-flag-for-cancel-pipelines-on-delete' into 'master'

Introduce feature flag for cancel pipelines on delete

See merge request gitlab-org/gitlab!65586

app/services/ci/destroy_pipeline_service.rb

@@ -7,7 +7,9 @@ module Ci
 
       Ci::ExpirePipelineCacheService.new.execute(pipeline, delete: true)
 
-      pipeline.destroy!
+      pipeline.cancel_running if pipeline.cancelable? && ::Feature.enabled?(:cancel_pipelines_prior_to_destroy, default_enabled: :yaml)
+
+      pipeline.reset.destroy!
 
       ServiceResponse.success(message: 'Pipeline not found')
     rescue ActiveRecord::RecordNotFound

config/feature_flags/development/cancel_pipelines_prior_to_destroy.yml

+---
+name: cancel_pipelines_prior_to_destroy
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/65586
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/335338
+milestone: '14.1'
+type: development
+group: group::continuous integration
+default_enabled: false

ee/app/models/ee/ci/build.rb

@@ -48,6 +48,18 @@ module EE
             .for_ref(ref)
             .for_project_paths(project_path)
         end
+
+        state_machine :status do
+          after_transition any => [:success, :failed, :canceled] do |build|
+            build.run_after_commit do
+              # TODO(Issue #331891): before enabling this feature flag. Move update consumption to async while keeping consumption calculation sync.
+              # This will ensure consumption is calculated before related records are deleted.
+              if ::Feature.enabled?(:cancel_pipelines_prior_to_destroy, default_enabled: :yaml)
+                ::Ci::Minutes::UpdateBuildMinutesService.new(build.project, nil).execute(build)
+              end
+            end
+          end
+        end
       end
 
       override :variables

ee/app/services/ci/minutes/update_build_minutes_service.rb

@@ -17,11 +17,18 @@ module Ci
 
         track_usage_of_monthly_minutes(consumption)
 
+        send_minutes_email_notification
+
         compare_with_live_consumption(build, consumption)
       end
 
       private
 
+      def send_minutes_email_notification
+        # `perform reset` on `project` because otherwise `Namespace#namespace_statistics` will return stale data.
+        ::Ci::Minutes::EmailNotificationService.new(@project.reset).execute if ::Gitlab.com?
+      end
+
       def legacy_track_usage_of_monthly_minutes(consumption)
         ProjectStatistics.update_counters(project_statistics,
           shared_runners_seconds: consumption)

ee/app/workers/ee/build_finished_worker.rb

@@ -3,10 +3,9 @@
 module EE
   module BuildFinishedWorker
     def process_build(build)
-      ::Ci::Minutes::UpdateBuildMinutesService.new(build.project, nil).execute(build)
-      # We need to use `reset` on `project` because their AR associations have been cached
-      # and `Namespace#namespace_statistics` will return stale data.
-      ::Ci::Minutes::EmailNotificationService.new(build.project.reset).execute if ::Gitlab.com?
+      unless ::Feature.enabled?(:cancel_pipelines_prior_to_destroy, default_enabled: :yaml)
+        ::Ci::Minutes::UpdateBuildMinutesService.new(build.project, nil).execute(build)
+      end
 
       unless build.project.requirements.empty?
         RequirementsManagement::ProcessRequirementsReportsWorker.perform_async(build.id)

ee/spec/models/ci/build_spec.rb

@@ -88,19 +88,33 @@ RSpec.describe Ci::Build do
     it_behaves_like 'depends on runner presence and type'
   end
 
-  context 'updates pipeline minutes' do
-    let(:job) { create(:ci_build, :running, pipeline: pipeline) }
+  shared_context 'updates minutes' do
+    context 'updates pipeline minutes' do
+      let(:job) { create(:ci_build, :running, pipeline: pipeline) }
 
-    %w(success drop cancel).each do |event|
-      it "for event #{event}", :sidekiq_might_not_need_inline do
-        expect(Ci::Minutes::UpdateBuildMinutesService)
-          .to receive(:new).and_call_original
+      %w(success drop cancel).each do |event|
+        it "for event #{event}" do
+          expect(Ci::Minutes::UpdateBuildMinutesService)
+            .to receive(:new).and_call_original
 
-        job.public_send(event)
+          job.public_send(event)
+        end
       end
     end
   end
 
+  context 'when cancel_pipelines_prior_to_destroy is enabled' do
+    include_context 'updates minutes'
+  end
+
+  context 'when cancel_pipelines_prior_to_destroy is disabled', :sidekiq_inline do
+    before do
+      stub_feature_flags(cancel_pipelines_prior_to_destroy: false)
+    end
+
+    include_context 'updates minutes'
+  end
+
   describe '#variables' do
     subject { job.variables }
 

ee/spec/services/ci/minutes/update_build_minutes_service_spec.rb

@@ -54,6 +54,32 @@ RSpec.describe Ci::Minutes::UpdateBuildMinutesService do
 
       it_behaves_like 'new tracking matches legacy tracking'
 
+      context 'when on .com' do
+        before do
+          allow(Gitlab).to receive(:com?).and_return(true)
+        end
+
+        it 'sends an email' do
+          expect_next_instance_of(Ci::Minutes::EmailNotificationService) do |service|
+            expect(service).to receive(:execute)
+          end
+
+          subject
+        end
+      end
+
+      context 'when not on .com' do
+        before do
+          allow(Gitlab).to receive(:com?).and_return(false)
+        end
+
+        it 'does not send an email' do
+          expect(Ci::Minutes::EmailNotificationService).not_to receive(:new)
+
+          subject
+        end
+      end
+
       context 'when feature flag ci_minutes_monthly_tracking is disabled' do
         before do
           stub_feature_flags(ci_minutes_monthly_tracking: false)

ee/spec/workers/build_finished_worker_spec.rb

@@ -27,21 +27,27 @@ RSpec.describe BuildFinishedWorker do
         allow_any_instance_of(EE::Project).to receive(:shared_runners_minutes_limit_enabled?).and_return(true)
       end
 
-      it 'updates the project stats' do
-        expect { subject }.to change { project_stats.reload.shared_runners_seconds }
-      end
+      context 'when cancel_pipelines_prior_to_destroy is disabled' do
+        before do
+          stub_feature_flags(cancel_pipelines_prior_to_destroy: false)
+        end
 
-      it 'updates the namespace stats' do
-        expect { subject }.to change { namespace_stats.reload.shared_runners_seconds }
-      end
+        it 'updates the project stats' do
+          expect { subject }.to change { project_stats.reload.shared_runners_seconds }
+        end
 
-      it 'notifies the owners of Groups' do
-        namespace.update_attribute(:shared_runners_minutes_limit, 2000)
-        namespace_stats.update_attribute(:shared_runners_seconds, 2100 * 60)
+        it 'updates the namespace stats' do
+          expect { subject }.to change { namespace_stats.reload.shared_runners_seconds }
+        end
 
-        expect(CiMinutesUsageMailer).to receive(:notify).once.with(namespace, [namespace.owner.email]).and_return(spy)
+        it 'notifies the owners of Groups' do
+          namespace.update_attribute(:shared_runners_minutes_limit, 2000)
+          namespace_stats.update_attribute(:shared_runners_seconds, 2100 * 60)
 
-        subject
+          expect(CiMinutesUsageMailer).to receive(:notify).once.with(namespace, [namespace.owner.email]).and_return(spy)
+
+          subject
+        end
       end
     end
 

spec/features/projects/pipelines/pipeline_spec.rb

@@ -438,30 +438,44 @@ RSpec.describe 'Pipeline', :js do
       end
     end
 
-    context 'deleting pipeline' do
-      context 'when user can not delete' do
-        before do
-          visit_pipeline
+    shared_context 'delete pipeline' do
+      context 'deleting pipeline' do
+        context 'when user can not delete' do
+          before do
+            visit_pipeline
+          end
+
+          it { expect(page).not_to have_button('Delete') }
         end
 
-        it { expect(page).not_to have_button('Delete') }
-      end
+        context 'when deleting' do
+          before do
+            group.add_owner(user)
 
-      context 'when deleting' do
-        before do
-          group.add_owner(user)
+            visit_pipeline
 
-          visit_pipeline
+            click_button 'Delete'
+            click_button 'Delete pipeline'
+          end
 
-          click_button 'Delete'
-          click_button 'Delete pipeline'
+          it 'redirects to pipeline overview page', :sidekiq_inline do
+            expect(page).to have_content('The pipeline has been deleted')
+            expect(current_path).to eq(project_pipelines_path(project))
+          end
         end
+      end
+    end
 
-        it 'redirects to pipeline overview page', :sidekiq_might_not_need_inline do
-          expect(page).to have_content('The pipeline has been deleted')
-          expect(current_path).to eq(project_pipelines_path(project))
-        end
+    context 'when cancel_pipelines_prior_to_destroy is enabled' do
+      include_context 'delete pipeline'
+    end
+
+    context 'when cancel_pipelines_prior_to_destroy is disabled' do
+      before do
+        stub_feature_flags(cancel_pipelines_prior_to_destroy: false)
       end
+
+      include_context 'delete pipeline'
     end
 
     context 'when pipeline ref does not exist in repository anymore' do

spec/services/ci/destroy_pipeline_service_spec.rb

@@ -67,6 +67,30 @@ RSpec.describe ::Ci::DestroyPipelineService do
         end
       end
     end
+
+    context 'when pipeline is in cancelable state' do
+      before do
+        allow(pipeline).to receive(:cancelable?).and_return(true)
+      end
+
+      it 'cancels the pipeline' do
+        expect(pipeline).to receive(:cancel_running)
+
+        subject
+      end
+
+      context 'when cancel_pipelines_prior_to_destroy is disabled' do
+        before do
+          stub_feature_flags(cancel_pipelines_prior_to_destroy: false)
+        end
+
+        it "doesn't cancel the pipeline" do
+          expect(pipeline).not_to receive(:cancel_running)
+
+          subject
+        end
+      end
+    end
   end
 
   context 'user is not owner' do