Merge branch 'ci-check-matchable-runners-on-builds-retry' into 'master'

Check CI quota when builds are retried [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!62702

app/models/ci/build.rb

@@ -136,6 +136,7 @@ module Ci
 
     scope :eager_load_job_artifacts, -> { includes(:job_artifacts) }
     scope :eager_load_job_artifacts_archive, -> { includes(:job_artifacts_archive) }
+    scope :eager_load_tags, -> { includes(:tags) }
 
     scope :eager_load_everything, -> do
       includes(
@@ -759,6 +760,14 @@ module Ci
       self.token && ActiveSupport::SecurityUtils.secure_compare(token, self.token)
     end
 
+    def tag_list
+      if tags.loaded?
+        tags.map(&:name)
+      else
+        super
+      end
+    end
+
     def has_tags?
       tag_list.any?
     end

app/services/ci/retry_build_service.rb

@@ -18,6 +18,9 @@ module Ci
       build.ensure_scheduling_type!
 
       reprocess!(build).tap do |new_build|
+        check_assignable_runners!(new_build)
+        next if new_build.failed?
+
         Gitlab::OptimisticLocking.retry_lock(new_build, name: 'retry_build', &:enqueue)
         AfterRequeueJobService.new(project, current_user).execute(build)
 
@@ -54,6 +57,8 @@ module Ci
       end
     end
 
+    def check_assignable_runners!(build); end
+
     def create_build!(attributes)
       build = project.builds.new(attributes)
       build.assign_attributes(::Gitlab::Ci::Pipeline::Seed::Build.environment_attributes_for(build))

app/services/ci/retry_pipeline_service.rb

@@ -13,8 +13,8 @@ module Ci
 
       pipeline.ensure_scheduling_type!
 
-      pipeline.retryable_builds.preload_needs.find_each do |build|
-        next unless can?(current_user, :update_build, build)
+      builds_relation(pipeline).find_each do |build|
+        next unless can_be_retried?(build)
 
         Ci::RetryBuildService.new(project, current_user)
           .reprocess!(build)
@@ -36,5 +36,17 @@ module Ci
         .new(pipeline)
         .execute
     end
+
+    private
+
+    def builds_relation(pipeline)
+      pipeline.retryable_builds.preload_needs
+    end
+
+    def can_be_retried?(build)
+      can?(current_user, :update_build, build)
+    end
   end
 end
+
+Ci::RetryPipelineService.prepend_mod_with('Ci::RetryPipelineService')

config/feature_flags/development/ci_quota_check_on_retries.yml

+---
+name: ci_quota_check_on_retries
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/62702
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/333765
+milestone: '14.0'
+type: development
+group: group::pipeline execution
+default_enabled: false

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

@@ -3,10 +3,9 @@
 module Ci
   module PipelineCreation
     class DropNotRunnableBuildsService
-      include ::Gitlab::Utils::StrongMemoize
-
       def initialize(pipeline)
         @pipeline = pipeline
+        @runner_minutes = Gitlab::Ci::Minutes::RunnersAvailability.new(pipeline.project)
       end
 
       ##
@@ -16,8 +15,6 @@ 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?
 
         validate_build_matchers
       end
@@ -25,51 +22,18 @@ module Ci
       private
 
       attr_reader :pipeline
+      attr_reader :runner_minutes
       delegate :project, to: :pipeline
 
       def validate_build_matchers
         build_ids = pipeline
           .build_matchers
-          .filter_map { |matcher| matcher.build_ids if should_drop?(matcher) }
+          .filter_map { |matcher| matcher.build_ids unless runner_minutes.available?(matcher) }
           .flatten
 
         drop_all_builds(build_ids, :ci_quota_exceeded)
       end
 
-      def should_drop?(build_matcher)
-        matches_instance_runners_and_quota_used_up?(build_matcher) &&
-          !matches_private_runners?(build_matcher)
-      end
-
-      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 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
-
-      def runner_matchers
-        strong_memoize(:runner_matchers) do
-          project.all_runners.active.online.runner_matchers
-        end
-      end
-
       ##
       # We skip pipeline processing until we drop all required builds. Otherwise
       # as we drop the first build, the remaining builds to be dropped could

ee/app/services/ee/ci/retry_build_service.rb

@@ -37,6 +37,16 @@ module EE
           raise ::Gitlab::Access::AccessDeniedError, 'Credit card required to be on file in order to retry a build'
         end
       end
+
+      override :check_assignable_runners!
+      def check_assignable_runners!(build)
+        return unless ::Feature.enabled?(:ci_quota_check_on_retries, project, default_enabled: :yaml)
+
+        runner_minutes = ::Gitlab::Ci::Minutes::RunnersAvailability.new(project)
+        return if runner_minutes.available?(build.build_matcher)
+
+        build.drop!(:ci_quota_exceeded)
+      end
     end
   end
 end

ee/app/services/ee/ci/retry_pipeline_service.rb

+# frozen_string_literal: true
+
+module EE
+  module Ci
+    module RetryPipelineService
+      extend ::Gitlab::Utils::Override
+      include ::Gitlab::Utils::StrongMemoize
+
+      private
+
+      override :builds_relation
+      def builds_relation(pipeline)
+        super.eager_load_tags
+      end
+
+      override :can_be_retried?
+      def can_be_retried?(build)
+        super && !ci_minutes_exceeded?(build)
+      end
+
+      def ci_minutes_exceeded?(build)
+        ::Feature.enabled?(:ci_quota_check_on_retries, project, default_enabled: :yaml) &&
+          !runner_minutes.available?(build.build_matcher)
+      end
+
+      def runner_minutes
+        strong_memoize(:runner_minutes) do
+          ::Gitlab::Ci::Minutes::RunnersAvailability.new(project)
+        end
+      end
+    end
+  end
+end

ee/lib/gitlab/ci/minutes/runners_availability.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    module Minutes
+      class RunnersAvailability
+        include ::Gitlab::Utils::StrongMemoize
+
+        def initialize(project)
+          @project = project
+        end
+
+        def available?(build_matcher)
+          return true unless project.shared_runners_enabled?
+
+          !quota_exceeded?(build_matcher)
+        end
+
+        private
+
+        attr_reader :project
+
+        def quota_exceeded?(build_matcher)
+          matches_instance_runners_and_quota_used_up?(build_matcher) &&
+            !matches_private_runners?(build_matcher)
+        end
+
+        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 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
+
+        def runner_matchers
+          strong_memoize(:runner_matchers) do
+            project.all_runners.active.online.runner_matchers
+          end
+        end
+      end
+    end
+  end
+end

ee/spec/lib/gitlab/ci/minutes/runners_availability_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Ci::Minutes::RunnersAvailability do
+  using RSpec::Parameterized::TableSyntax
+
+  let_it_be(:instance_runner) { create(:ci_runner, :instance, :online) }
+
+  let(:build) { build_stubbed(:ci_build, project: project) }
+  let(:minutes) { described_class.new(project) }
+
+  describe '#available?' do
+    where(:shared_runners_enabled, :minutes_quota, :private_runner_available, :result) do
+      true  | :with_not_used_build_minutes_limit  | false | true
+      true  | :with_not_used_build_minutes_limit  | true  | true
+      true  | :with_used_build_minutes_limit      | false | false
+      true  | :with_used_build_minutes_limit      | true  | true
+      false | :with_used_build_minutes_limit      | false | true
+      false | :with_used_build_minutes_limit      | true  | true
+      false | :with_not_used_build_minutes_limit  | true  | true
+      false | :with_not_used_build_minutes_limit  | false | true
+    end
+
+    with_them do
+      let!(:namespace) { create(:namespace, minutes_quota) }
+      let!(:project) { create(:project, namespace: namespace, shared_runners_enabled: shared_runners_enabled) }
+      let!(:private_runner) { create(:ci_runner, :project, :online, projects: [project], active: private_runner_available) }
+
+      subject { minutes.available?(build.build_matcher) }
+
+      it { is_expected.to eq(result) }
+    end
+  end
+
+  context 'N+1 queries' do
+    let_it_be(:project) { create(:project) }
+    let_it_be(:private_runner) { create(:ci_runner, :project, :online, projects: [project]) }
+
+    it 'caches records loaded from database' do
+      ActiveRecord::QueryRecorder.new(skip_cached: false) do
+        minutes.available?(build.build_matcher)
+      end
+
+      expect { minutes.available?(build.build_matcher) }.not_to exceed_all_query_limit(0)
+    end
+  end
+end

ee/spec/services/ci/retry_build_service_spec.rb

@@ -2,6 +2,18 @@
 require 'spec_helper'
 
 RSpec.describe Ci::RetryBuildService do
+  let_it_be(:user) { create(:user) }
+
+  let(:build) { create(:ci_build, project: project) }
+
+  subject(:service) { described_class.new(project, user) }
+
+  before do
+    stub_not_protect_default_branch
+
+    project.add_developer(user)
+  end
+
   it_behaves_like 'restricts access to protected environments'
 
   describe '#reprocess' do
@@ -9,12 +21,8 @@ RSpec.describe Ci::RetryBuildService do
       let_it_be(:namespace) { create(:namespace) }
       let_it_be(:ultimate_plan) { create(:ultimate_plan) }
       let_it_be(:plan_limits) { create(:plan_limits, plan: ultimate_plan) }
-      let_it_be(:user) { create(:user) }
 
       let(:project) { create(:project, namespace: namespace, creator: user) }
-      let(:build) { create(:ci_build, project: project) }
-
-      subject(:service) { described_class.new(project, user) }
 
       let(:new_build) do
         travel_to(1.second.from_now) do
@@ -22,12 +30,6 @@ RSpec.describe Ci::RetryBuildService do
         end
       end
 
-      before do
-        stub_not_protect_default_branch
-
-        project.add_developer(user)
-      end
-
       context 'dast' do
         let(:dast_site_profile) { create(:dast_site_profile, project: project) }
         let(:dast_scanner_profile) { create(:dast_scanner_profile, project: project) }
@@ -120,4 +122,46 @@ RSpec.describe Ci::RetryBuildService do
       end
     end
   end
+
+  describe '#execute' do
+    let(:new_build) do
+      travel_to(1.second.from_now) do
+        service.execute(build)
+      end
+    end
+
+    context 'when the CI quota is exceeded' do
+      let_it_be(:namespace) { create(:namespace, :with_used_build_minutes_limit) }
+      let_it_be(:project) { create(:project, namespace: namespace, creator: user) }
+
+      context 'when there are no runners available' do
+        it { expect(new_build).not_to be_failed }
+      end
+
+      context 'when shared runners are available' do
+        let_it_be(:runner) { create(:ci_runner, :instance, :online) }
+
+        it 'fails the build' do
+          expect(new_build).to be_failed
+          expect(new_build.failure_reason).to eq('ci_quota_exceeded')
+        end
+
+        context 'with private runners' do
+          let_it_be(:private_runner) do
+            create(:ci_runner, :project, :online, projects: [project])
+          end
+
+          it { expect(new_build).not_to be_failed }
+        end
+
+        context 'when the feature is disabled' do
+          before do
+            stub_feature_flags(ci_quota_check_on_retries: false)
+          end
+
+          it { expect(new_build).not_to be_failed }
+        end
+      end
+    end
+  end
 end

ee/spec/services/ci/retry_pipeline_service_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+RSpec.describe Ci::RetryPipelineService do
+  let_it_be(:runner) { create(:ci_runner, :instance, :online) }
+  let_it_be(:user) { create(:user) }
+
+  let(:pipeline) { create(:ci_pipeline, project: project) }
+  let(:service) { described_class.new(project, user) }
+
+  before do
+    project.add_developer(user)
+
+    create(:protected_branch, :developers_can_merge,
+           name: pipeline.ref, project: project)
+  end
+
+  context 'when the namespace is out of CI minutes' do
+    let_it_be(:namespace) { create(:namespace, :with_used_build_minutes_limit) }
+    let_it_be(:project) { create(:project, namespace: namespace) }
+    let_it_be(:private_runner) do
+      create(:ci_runner, :project, :online, projects: [project],
+        tag_list: ['ruby'], run_untagged: false)
+    end
+
+    before do
+      create_build('rspec 1', :failed)
+      create_build('rspec 2', :canceled, tag_list: ['ruby'])
+    end
+
+    it 'retries the builds with available runners' do
+      service.execute(pipeline)
+
+      expect(pipeline.statuses.count).to eq(3)
+      expect(build('rspec 1')).to be_failed
+      expect(build('rspec 2')).to be_pending
+      expect(pipeline.reload).to be_running
+    end
+
+    context 'when the feature flag is disabled' do
+      before do
+        stub_feature_flags(ci_quota_check_on_retries: false)
+      end
+
+      it 'enqueues all builds' do
+        service.execute(pipeline)
+
+        expect(build('rspec 1')).to be_pending
+        expect(build('rspec 2')).to be_pending
+        expect(pipeline.reload).to be_running
+      end
+    end
+  end
+
+  def build(name)
+    pipeline.reload.statuses.latest.find_by(name: name)
+  end
+
+  def create_build(name, status, **opts)
+    create(:ci_build, name: name, status: status, pipeline: pipeline, **opts) do |build|
+      ::Ci::ProcessPipelineService.new(pipeline).execute
+    end
+  end
+end

spec/models/ci/build_spec.rb

@@ -1966,6 +1966,23 @@ RSpec.describe Ci::Build do
     end
   end
 
+  describe '#tag_list' do
+    let_it_be(:build) { create(:ci_build, tag_list: ['tag']) }
+
+    context 'when tags are preloaded' do
+      it 'does not trigger queries' do
+        build_with_tags = described_class.eager_load_tags.id_in([build]).to_a.first
+
+        expect { build_with_tags.tag_list }.not_to exceed_all_query_limit(0)
+        expect(build_with_tags.tag_list).to eq(['tag'])
+      end
+    end
+
+    context 'when tags are not preloaded' do
+      it { expect(described_class.find(build.id).tag_list).to eq(['tag']) }
+    end
+  end
+
   describe '#has_tags?' do
     context 'when build has tags' do
       subject { create(:ci_build, tag_list: ['tag']) }