Merge branch 'ci-remove-no-matching-runner-failure-action' into 'master'

Drop builds with `ci_quota_exceeded` failure reason

See merge request gitlab-org/gitlab!63542

app/models/concerns/enums/ci/commit_status.rb

@@ -24,7 +24,7 @@ module Enums
           project_deleted: 15,
           ci_quota_exceeded: 16,
           pipeline_loop_detected: 17,
-          no_matching_runner: 18,
+          no_matching_runner: 18, # not used anymore, but cannot be deleted because of old data
           insufficient_bridge_permissions: 1_001,
           downstream_bridge_project_not_found: 1_002,
           invalid_bridge_trigger: 1_003,

app/services/ci/pipeline_creation/start_pipeline_service.rb

@@ -10,9 +10,10 @@ module Ci
       end
 
       def execute
-        DropNotRunnableBuildsService.new(pipeline).execute
         Ci::ProcessPipelineService.new(pipeline).execute
       end
     end
   end
 end
+
+::Ci::PipelineCreation::StartPipelineService.prepend_mod_with('Ci::PipelineCreation::StartPipelineService')

app/services/ci/pipeline_creation/drop_not_runnable_builds_service.rb → ee/app/services/ci/pipeline_creation/drop_not_runnable_builds_service.rb

@@ -3,7 +3,7 @@
 module Ci
   module PipelineCreation
     class DropNotRunnableBuildsService
-      include Gitlab::Utils::StrongMemoize
+      include ::Gitlab::Utils::StrongMemoize
 
       def initialize(pipeline)
         @pipeline = pipeline
@@ -16,40 +16,58 @@ module Ci
       def execute
         return unless ::Feature.enabled?(:ci_drop_new_builds_when_ci_quota_exceeded, project, default_enabled: :yaml)
         return unless pipeline.created?
+        return unless project.shared_runners_enabled?
+        return unless project.ci_minutes_quota.minutes_used_up?
 
-        load_runners
         validate_build_matchers
       end
 
       private
 
       attr_reader :pipeline
-      attr_reader :instance_runners, :private_runners
       delegate :project, to: :pipeline
 
-      def load_runners
-        @instance_runners, @private_runners = project
-          .all_runners
-          .active
-          .online
-          .runner_matchers
-          .partition(&:instance_type?)
+      def validate_build_matchers
+        build_ids = pipeline
+          .build_matchers
+          .filter_map { |matcher| matcher.build_ids if should_drop?(matcher) }
+          .flatten
+
+        drop_all_builds(build_ids, :ci_quota_exceeded)
       end
 
-      def validate_build_matchers
-        pipeline.build_matchers.each do |build_matcher|
-          failure_reason = validate_build_matcher(build_matcher)
-          next unless failure_reason
+      def should_drop?(build_matcher)
+        matches_instance_runners_and_quota_used_up?(build_matcher) &&
+          !matches_private_runners?(build_matcher)
+      end
 
-          drop_all_builds(build_matcher.build_ids, failure_reason)
+      def matches_instance_runners_and_quota_used_up?(build_matcher)
+        instance_runners.any? do |matcher|
+          matcher.matches?(build_matcher) &&
+            !matcher.matches_quota?(build_matcher)
+        end
       end
+
+      def matches_private_runners?(build_matcher)
+        private_runners.any? { |matcher| matcher.matches?(build_matcher) }
       end
 
-      def validate_build_matcher(build_matcher)
-        return if matching_private_runners?(build_matcher)
-        return if matching_instance_runners?(build_matcher)
+      def instance_runners
+        strong_memoize(:instance_runners) do
+          runner_matchers.select(&:instance_type?)
+        end
+      end
+
+      def private_runners
+        strong_memoize(:private_runners) do
+          runner_matchers.reject(&:instance_type?)
+        end
+      end
 
-        matching_failure_reason(build_matcher)
+      def runner_matchers
+        strong_memoize(:runner_matchers) do
+          project.all_runners.active.online.runner_matchers
+        end
       end
 
       ##
@@ -58,34 +76,12 @@ module Ci
       # transition to other states by `PipelineProcessWorker` running async.
       #
       def drop_all_builds(build_ids, failure_reason)
-        pipeline.builds.id_in(build_ids).each do |build|
-          build.drop(failure_reason, skip_pipeline_processing: true)
-        end
-      end
-
-      def matching_private_runners?(build_matcher)
-        private_runners
-          .find { |matcher| matcher.matches?(build_matcher) }
-          .present?
-      end
+        return if build_ids.empty?
 
-      def matching_instance_runners?(build_matcher)
-        instance_runners
-          .find { |matcher| matching_criteria(matcher, build_matcher) }
-          .present?
+        pipeline.builds.id_in(build_ids).each do |build|
+          build.drop!(failure_reason, skip_pipeline_processing: true)
         end
-
-      # Overridden in EE
-      def matching_criteria(runner_matcher, build_matcher)
-        runner_matcher.matches?(build_matcher)
-      end
-
-      # Overridden in EE
-      def matching_failure_reason(build_matcher)
-        :no_matching_runner
       end
     end
   end
 end
-
-Ci::PipelineCreation::DropNotRunnableBuildsService.prepend_mod_with('Ci::PipelineCreation::DropNotRunnableBuildsService')

ee/app/services/ee/ci/pipeline_creation/drop_not_runnable_builds_service.rb

-# frozen_string_literal: true
-
-module EE
-  module Ci
-    module PipelineCreation
-      module DropNotRunnableBuildsService
-        extend ::Gitlab::Utils::Override
-
-        private
-
-        override :matching_criteria
-        def matching_criteria(runner_matcher, build_matcher)
-          super && runner_matcher.matches_quota?(build_matcher)
-        end
-
-        override :matching_failure_reason
-        def matching_failure_reason(build_matcher)
-          if build_matcher.project.shared_runners_enabled_but_unavailable?
-            :ci_quota_exceeded
-          else
-            :no_matching_runner
-          end
-        end
-      end
-    end
-  end
-end

ee/app/services/ee/ci/pipeline_creation/start_pipeline_service.rb

+# frozen_string_literal: true
+
+module EE
+  module Ci
+    module PipelineCreation
+      module StartPipelineService
+        extend ::Gitlab::Utils::Override
+
+        override :execute
+        def execute
+          ::Ci::PipelineCreation::DropNotRunnableBuildsService.new(pipeline).execute
+          super
+        end
+      end
+    end
+  end
+end

ee/spec/services/ci/create_pipeline_service/runnable_builds_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Ci::CreatePipelineService, :sidekiq_inline do
+  let_it_be(:namespace) { create(:namespace, :with_used_build_minutes_limit) }
+  let_it_be(:project) { create(:project, :repository, namespace: namespace) }
+  let_it_be(:user) { project.owner }
+  let_it_be(:instance_runner) { create(:ci_runner, :instance, :online) }
+
+  let(:service) do
+    described_class.new(project, user, { ref: 'refs/heads/master' })
+  end
+
+  let(:config) do
+    <<~EOY
+    job1:
+     stage: build
+     script:
+       - echo "deploy runner 123"
+
+    job2:
+      stage: test
+      script:
+        - echo "run on runner 123"
+      tags:
+        - "123"
+    EOY
+  end
+
+  before do
+    project.add_developer(user)
+    stub_ci_pipeline_yaml_file(config)
+  end
+
+  it 'drops builds that match shared runners' do
+    pipeline = create_pipeline!
+
+    job1 = pipeline.builds.find_by_name('job1')
+    job2 = pipeline.builds.find_by_name('job2')
+
+    expect(job1).to be_failed
+    expect(job1.failure_reason).to eq('ci_quota_exceeded')
+    expect(job2).not_to be_failed
+  end
+
+  context 'with private runners' do
+    let_it_be(:private_runner) do
+      create(:ci_runner, :project, :online, projects: [project])
+    end
+
+    it 'does not drop the builds' do
+      pipeline = create_pipeline!
+
+      job1 = pipeline.builds.find_by_name('job1')
+      job2 = pipeline.builds.find_by_name('job2')
+
+      expect(job1).not_to be_failed
+      expect(job2).not_to be_failed
+    end
+  end
+
+  def create_pipeline!
+    service.execute(:push)
+  end
+end

ee/spec/services/ci/pipeline_creation/drop_not_runnable_builds_service_spec.rb

@@ -3,14 +3,21 @@
 require 'spec_helper'
 
 RSpec.describe Ci::PipelineCreation::DropNotRunnableBuildsService do
+  let_it_be(:group) { create(:group) }
+  let_it_be(:project) { create(:project, group: group) }
+
   let_it_be_with_reload(:pipeline) do
-    create(:ci_pipeline, status: :created)
+    create(:ci_pipeline, project: project, status: :created)
   end
 
   let_it_be_with_reload(:job) do
     create(:ci_build, project: pipeline.project, pipeline: pipeline)
   end
 
+  let_it_be_with_reload(:job_with_tags) do
+    create(:ci_build, :tags, project: pipeline.project, pipeline: pipeline)
+  end
+
   let_it_be(:instance_runner) do
     create(:ci_runner,
       :online,
@@ -22,19 +29,28 @@ RSpec.describe Ci::PipelineCreation::DropNotRunnableBuildsService do
   describe '#execute' do
     subject(:execute) { described_class.new(pipeline).execute }
 
-    shared_examples 'available CI quota' do
+    shared_examples 'jobs allowed to run' do
       it 'does not drop the jobs' do
-        expect { execute }.not_to change { job.reload.status }
+        expect { execute }
+          .to not_change { job.reload.status }
+          .and not_change { job_with_tags.reload.status }
       end
     end
 
     shared_examples 'limit exceeded' do
+      before do
+        allow(pipeline.project).to receive(:ci_minutes_quota)
+          .and_return(double('quota', minutes_used_up?: true))
+      end
+
       it 'drops the job with ci_quota_exceeded reason' do
         execute
-        job.reload
+        [job, job_with_tags].each(&:reload)
 
         expect(job).to be_failed
         expect(job.failure_reason).to eq('ci_quota_exceeded')
+
+        expect(job_with_tags).to be_pending
       end
 
       context 'when shared runners are disabled' do
@@ -42,67 +58,75 @@ RSpec.describe Ci::PipelineCreation::DropNotRunnableBuildsService do
           pipeline.project.update!(shared_runners_enabled: false)
         end
 
-        it 'drops the job with no_matching_runner reason' do
-          execute
-          job.reload
+        it_behaves_like 'jobs allowed to run'
+      end
 
-          expect(job).to be_failed
-          expect(job.failure_reason).to eq('no_matching_runner')
+      context 'with project runners' do
+        let_it_be(:project_runner) do
+          create(:ci_runner, :online, runner_type: :project_type, projects: [project])
         end
+
+        it_behaves_like 'jobs allowed to run'
       end
+
+      context 'with group runners' do
+        let_it_be(:group_runner) do
+          create(:ci_runner, :online, runner_type: :group_type, groups: [group])
         end
 
-    context 'with public projects' do
+        it_behaves_like 'jobs allowed to run'
+      end
+
+      context 'when the feature flag is disabled' do
         before do
-        pipeline.project.update!(visibility_level: ::Gitlab::VisibilityLevel::PUBLIC)
+          stub_feature_flags(ci_drop_new_builds_when_ci_quota_exceeded: false)
         end
 
-      it_behaves_like 'available CI quota'
+        it_behaves_like 'jobs allowed to run'
+      end
 
-      context 'when the CI quota is exceeded' do
+      context 'when the pipeline status is running' do
         before do
-          allow(pipeline.project).to receive(:ci_minutes_quota)
-            .and_return(double('quota', minutes_used_up?: true))
+          pipeline.update!(status: :running)
         end
 
-        it 'does not drop the jobs' do
-          expect { execute }.not_to change { job.reload.status }
-        end
+        it_behaves_like 'jobs allowed to run'
       end
     end
 
-    context 'with internal projects' do
+    context 'with public projects' do
       before do
-        pipeline.project.update!(visibility_level: ::Gitlab::VisibilityLevel::INTERNAL)
+        pipeline.project.update!(visibility_level: ::Gitlab::VisibilityLevel::PUBLIC)
       end
 
-      it_behaves_like 'available CI quota'
+      it_behaves_like 'jobs allowed to run'
 
-      context 'when the Ci quota is exceeded' do
+      context 'when the CI quota is exceeded' do
         before do
           allow(pipeline.project).to receive(:ci_minutes_quota)
             .and_return(double('quota', minutes_used_up?: true))
         end
 
-        it_behaves_like 'limit exceeded'
+        it_behaves_like 'jobs allowed to run'
       end
     end
 
-    context 'with private projects' do
+    context 'with internal projects' do
       before do
-        pipeline.project.update!(visibility_level: ::Gitlab::VisibilityLevel::PRIVATE)
+        pipeline.project.update!(visibility_level: ::Gitlab::VisibilityLevel::INTERNAL)
       end
 
-      it_behaves_like 'available CI quota'
+      it_behaves_like 'jobs allowed to run'
+      it_behaves_like 'limit exceeded'
+    end
 
-      context 'when the Ci quota is exceeded' do
+    context 'with private projects' do
       before do
-          allow(pipeline.project).to receive(:ci_minutes_quota)
-            .and_return(double('quota', minutes_used_up?: true))
+        pipeline.project.update!(visibility_level: ::Gitlab::VisibilityLevel::PRIVATE)
       end
 
+      it_behaves_like 'jobs allowed to run'
       it_behaves_like 'limit exceeded'
     end
   end
-  end
 end

ee/spec/services/ci/pipeline_creation/start_pipeline_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Ci::PipelineCreation::StartPipelineService do
+  let(:pipeline) { build(:ci_pipeline) }
+
+  subject(:service) { described_class.new(pipeline) }
+
+  describe '#execute' do
+    it 'calls the pipeline runners matching validation service' do
+      expect(Ci::PipelineCreation::DropNotRunnableBuildsService)
+        .to receive(:new)
+        .with(pipeline)
+        .and_return(double('service', execute: true))
+
+      service.execute
+    end
+  end
+end

ee/spec/services/ci/process_pipeline_service_spec.rb

@@ -29,11 +29,6 @@ RSpec.describe Ci::ProcessPipelineService, '#execute' do
       stub_ci_pipeline_to_return_yaml_file
     end
 
-    context 'when there is a runner available' do
-      let_it_be(:runner) do
-        create(:ci_runner, :online, tag_list: %w[ruby postgres mysql])
-      end
-
     it 'creates a downstream cross-project pipeline' do
       service.execute
       Sidekiq::Worker.drain_all
@@ -53,27 +48,6 @@ RSpec.describe Ci::ProcessPipelineService, '#execute' do
     end
   end
 
-    context 'with no runners' do
-      it 'creates a failed downstream cross-project pipeline' do
-        service.execute
-        Sidekiq::Worker.drain_all
-
-        expect_statuses(%w[test pending], %w[cross created], %w[deploy created])
-
-        update_build_status(:test, :success)
-        Sidekiq::Worker.drain_all
-
-        expect_statuses(%w[test success], %w[cross success], %w[deploy pending])
-
-        expect(downstream.ci_pipelines).to be_one
-        expect(downstream.ci_pipelines.first).to be_failed
-        expect(downstream.builds).not_to be_empty
-        expect(downstream.builds).to all be_failed
-        expect(downstream.builds.map(&:failure_reason)).to all eq('no_matching_runner')
-      end
-    end
-  end
-
   def expect_statuses(*expected)
     statuses = pipeline.statuses
       .where(name: expected.map(&:first))

ee/spec/workers/ci/initial_pipeline_process_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Ci::InitialPipelineProcessWorker do
+  describe '#perform' do
+    let_it_be(:namespace) { create(:namespace, :with_used_build_minutes_limit) }
+    let_it_be(:project) { create(:project, namespace: namespace) }
+    let_it_be_with_reload(:pipeline) do
+      create(:ci_pipeline, :with_job, project: project, status: :created)
+    end
+
+    let_it_be(:instance_runner) { create(:ci_runner, :instance, :online) }
+
+    include_examples 'an idempotent worker' do
+      let(:job_args) { pipeline.id }
+
+      context 'when the project is out of CI minutes' do
+        it 'marks the pipeline as failed' do
+          expect(pipeline).to be_created
+
+          subject
+
+          expect(pipeline.reload).to be_failed
+        end
+      end
+    end
+  end
+end

qa/qa/specs/features/browser_ui/4_verify/pipeline/create_and_process_pipeline_spec.rb

 # frozen_string_literal: true
 
 module QA
-  # TODO: Remove `:requires_admin` meta when the feature flag is removed
-  RSpec.describe 'Verify', :runner, :requires_admin do
+  RSpec.describe 'Verify', :runner do
     describe 'Pipeline creation and processing' do
       let(:executor) { "qa-runner-#{Time.now.to_i}" }
       let(:max_wait) { 30 }
-      let(:feature_flag) { :ci_drop_new_builds_when_ci_quota_exceeded }
 
       let(:project) do
         Resource::Project.fabricate_via_api! do |project|
@@ -28,8 +26,6 @@ module QA
 
       it 'users creates a pipeline which gets processed', testcase: 'https://gitlab.com/gitlab-org/quality/testcases/-/issues/1849' do
         # TODO: Convert back to :smoke once proved to be stable. Related issue: https://gitlab.com/gitlab-org/gitlab/-/issues/300909
-        tags_mismatch_status = Runtime::Feature.enabled?(feature_flag, project: project) ? :failed : :pending
-
         Flow::Login.sign_in
 
         Resource::Repository::Commit.fabricate_via_api! do |commit|
@@ -75,7 +71,7 @@ module QA
         {
           'test-success': :passed,
           'test-failure': :failed,
-          'test-tags-mismatch': tags_mismatch_status,
+          'test-tags-mismatch': :pending,
           'test-artifacts': :passed
         }.each do |job, status|
           Page::Project::Pipeline::Show.perform do |pipeline|

spec/services/ci/create_pipeline_service/needs_spec.rb

@@ -202,11 +202,6 @@ RSpec.describe Ci::CreatePipelineService do
         YAML
       end
 
-      context 'when there are runners matching the builds' do
-        before do
-          create(:ci_runner, :online)
-        end
-
       it 'creates a pipeline with build_a and test_b pending; deploy_b manual', :sidekiq_inline do
         processables = pipeline.processables
 
@@ -225,17 +220,6 @@ RSpec.describe Ci::CreatePipelineService do
       end
     end
 
-      context 'when there are no runners matching the builds' do
-        it 'creates a pipeline but all jobs failed', :sidekiq_inline do
-          processables = pipeline.processables
-
-          expect(pipeline).to be_created_successfully
-          expect(processables).to all be_failed
-          expect(processables.map(&:failure_reason)).to all eq('no_matching_runner')
-        end
-      end
-    end
-
     context 'when needs is empty hash' do
       let(:config) do
         <<~YAML

spec/services/ci/pipeline_creation/drop_not_runnable_builds_service_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Ci::PipelineCreation::DropNotRunnableBuildsService do
-  let_it_be(:group) { create(:group) }
-  let_it_be(:project) { create(:project, group: group) }
-
-  let_it_be_with_reload(:pipeline) do
-    create(:ci_pipeline, project: project, status: :created)
-  end
-
-  let_it_be_with_reload(:job) do
-    create(:ci_build, project: project, pipeline: pipeline)
-  end
-
-  describe '#execute' do
-    subject(:execute) { described_class.new(pipeline).execute }
-
-    shared_examples 'jobs allowed to run' do
-      it 'does not drop the jobs' do
-        expect { execute }.not_to change { job.reload.status }
-      end
-    end
-
-    context 'when the feature flag is disabled' do
-      before do
-        stub_feature_flags(ci_drop_new_builds_when_ci_quota_exceeded: false)
-      end
-
-      it_behaves_like 'jobs allowed to run'
-    end
-
-    context 'when the pipeline status is running' do
-      before do
-        pipeline.update!(status: :running)
-      end
-
-      it_behaves_like 'jobs allowed to run'
-    end
-
-    context 'when there are no runners available' do
-      let_it_be(:offline_project_runner) do
-        create(:ci_runner, runner_type: :project_type, projects: [project])
-      end
-
-      it 'drops the job' do
-        execute
-        job.reload
-
-        expect(job).to be_failed
-        expect(job.failure_reason).to eq('no_matching_runner')
-      end
-    end
-
-    context 'with project runners' do
-      let_it_be(:project_runner) do
-        create(:ci_runner, :online, runner_type: :project_type, projects: [project])
-      end
-
-      it_behaves_like 'jobs allowed to run'
-    end
-
-    context 'with group runners' do
-      let_it_be(:group_runner) do
-        create(:ci_runner, :online, runner_type: :group_type, groups: [group])
-      end
-
-      it_behaves_like 'jobs allowed to run'
-    end
-
-    context 'with instance runners' do
-      let_it_be(:instance_runner) do
-        create(:ci_runner, :online, runner_type: :instance_type)
-      end
-
-      it_behaves_like 'jobs allowed to run'
-    end
-  end
-end

spec/services/ci/pipeline_creation/start_pipeline_service_spec.rb

@@ -8,15 +8,6 @@ RSpec.describe Ci::PipelineCreation::StartPipelineService do
   subject(:service) { described_class.new(pipeline) }
 
   describe '#execute' do
-    it 'calls the pipeline runners matching validation service' do
-      expect(Ci::PipelineCreation::DropNotRunnableBuildsService)
-        .to receive(:new)
-        .with(pipeline)
-        .and_return(double('service', execute: true))
-
-      service.execute
-    end
-
     it 'calls the pipeline process service' do
       expect(Ci::ProcessPipelineService)
         .to receive(:new)

spec/workers/ci/initial_pipeline_process_worker_spec.rb

@@ -11,11 +11,6 @@ RSpec.describe Ci::InitialPipelineProcessWorker do
     include_examples 'an idempotent worker' do
       let(:job_args) { pipeline.id }
 
-      context 'when there are runners available' do
-        before do
-          create(:ci_runner, :online)
-        end
-
       it 'marks the pipeline as pending' do
         expect(pipeline).to be_created
 
@@ -24,14 +19,5 @@ RSpec.describe Ci::InitialPipelineProcessWorker do
         expect(pipeline.reload).to be_pending
       end
     end
-
-      it 'marks the pipeline as failed' do
-        expect(pipeline).to be_created
-
-        subject
-
-        expect(pipeline.reload).to be_failed
-      end
-    end
   end
 end