Merge branch 'mo-add-minutes-exceeded-to-pending-builds' into 'master'

Denormalize remaining CI minutes to CI::PendingBuild

See merge request gitlab-org/gitlab!64443

app/models/ci/pending_build.rb

@@ -4,6 +4,9 @@ module Ci
   class PendingBuild < Ci::ApplicationRecord
     belongs_to :project
     belongs_to :build, class_name: 'Ci::Build'
+    belongs_to :namespace, inverse_of: :pending_builds, class_name: 'Namespace'
+
+    validates :namespace, presence: true
 
     scope :ref_protected, -> { where(protected: true) }
     scope :queued_before, ->(time) { where(arel_table[:created_at].lt(time)) }
@@ -20,7 +23,8 @@ module Ci
       args = {
         build: build,
         project: build.project,
-        protected: build.protected?
+        protected: build.protected?,
+        namespace: build.project.namespace
       }
 
       if Feature.enabled?(:ci_pending_builds_maintain_shared_runners_data, type: :development, default_enabled: :yaml)
@@ -54,3 +58,5 @@ module Ci
     private_class_method :builds_access_level?
   end
 end
+
+Ci::PendingBuild.prepend_mod_with('Ci::PendingBuild')

app/models/namespace.rb

@@ -34,6 +34,7 @@ class Namespace < ApplicationRecord
 
   has_many :runner_namespaces, inverse_of: :namespace, class_name: 'Ci::RunnerNamespace'
   has_many :runners, through: :runner_namespaces, source: :runner, class_name: 'Ci::Runner'
+  has_many :pending_builds, class_name: 'Ci::PendingBuild'
   has_one :onboarding_progress
 
   # This should _not_ be `inverse_of: :namespace`, because that would also set

config/feature_flags/development/ci_pending_builds_maintain_ci_minutes_data.yml

+---
+name: ci_pending_builds_maintain_ci_minutes_data
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/64443
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/332951
+milestone: '14.2'
+type: development
+group: group::pipeline execution
+default_enabled: false

db/migrate/20210706151154_add_remaining_ci_minutes_to_ci_pending_build.rb

+# frozen_string_literal: true
+
+class AddRemainingCiMinutesToCiPendingBuild < ActiveRecord::Migration[6.1]
+  include Gitlab::Database::MigrationHelpers
+
+  disable_ddl_transaction!
+
+  def up
+    with_lock_retries do
+      add_column(:ci_pending_builds, :namespace_id, :bigint)
+      add_column(:ci_pending_builds, :minutes_exceeded, :boolean, null: false, default: false)
+    end
+  end
+
+  def down
+    with_lock_retries do
+      remove_column(:ci_pending_builds, :minutes_exceeded)
+      remove_column(:ci_pending_builds, :namespace_id)
+    end
+  end
+end

db/migrate/20210706151446_add_namespace_foreign_key_to_ci_pending_build.rb

+# frozen_string_literal: true
+
+class AddNamespaceForeignKeyToCiPendingBuild < ActiveRecord::Migration[6.1]
+  include Gitlab::Database::MigrationHelpers
+
+  disable_ddl_transaction!
+  INDEX_NAME = 'index_ci_pending_builds_on_namespace_id'
+
+  def up
+    add_concurrent_index(:ci_pending_builds, :namespace_id, name: INDEX_NAME)
+    add_concurrent_foreign_key(:ci_pending_builds, :namespaces, column: :namespace_id, on_delete: :cascade)
+  end
+
+  def down
+    remove_foreign_key_if_exists(:ci_pending_builds, column: :namespace_id)
+    remove_concurrent_index_by_name(:ci_pending_builds, INDEX_NAME)
+  end
+end

db/schema_migrations/20210706151154

+fc330cf9875a423db87748e84c574f2208e164945b56361a563f2085d324f610
\ No newline at end of file

db/schema_migrations/20210706151446

+4400cd95cf149a7abc759ca412b0d87c81bc405719999ce60502869d21d17aaa
\ No newline at end of file

db/structure.sql

@@ -10901,7 +10901,9 @@ CREATE TABLE ci_pending_builds (
     project_id bigint NOT NULL,
     created_at timestamp with time zone DEFAULT now() NOT NULL,
     protected boolean DEFAULT false NOT NULL,
-    instance_runners_enabled boolean DEFAULT false NOT NULL
+    instance_runners_enabled boolean DEFAULT false NOT NULL,
+    namespace_id bigint,
+    minutes_exceeded boolean DEFAULT false NOT NULL
 );
 
 CREATE SEQUENCE ci_pending_builds_id_seq
@@ -23267,6 +23269,8 @@ CREATE INDEX index_ci_pending_builds_id_on_protected_partial ON ci_pending_build
 
 CREATE UNIQUE INDEX index_ci_pending_builds_on_build_id ON ci_pending_builds USING btree (build_id);
 
+CREATE INDEX index_ci_pending_builds_on_namespace_id ON ci_pending_builds USING btree (namespace_id);
+
 CREATE INDEX index_ci_pending_builds_on_project_id ON ci_pending_builds USING btree (project_id);
 
 CREATE INDEX index_ci_pipeline_artifacts_failed_verification ON ci_pipeline_artifacts USING btree (verification_retry_at NULLS FIRST) WHERE (verification_state = 3);
@@ -26698,6 +26702,9 @@ ALTER TABLE ONLY ci_daily_build_group_report_results
 ALTER TABLE ONLY merge_requests
     ADD CONSTRAINT fk_fd82eae0b9 FOREIGN KEY (head_pipeline_id) REFERENCES ci_pipelines(id) ON DELETE SET NULL;
 
+ALTER TABLE ONLY ci_pending_builds
+    ADD CONSTRAINT fk_fdc0137e4a FOREIGN KEY (namespace_id) REFERENCES namespaces(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY project_import_data
     ADD CONSTRAINT fk_ffb9ee3a10 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE;
 

ee/app/models/ci/minutes/quota.rb

@@ -48,6 +48,13 @@ module Ci
         enabled? && total_minutes_used >= total_minutes
       end
 
+      # TODO: merge this with minutes_used_up? in
+      # https://gitlab.com/gitlab-org/gitlab/-/issues/332933.
+      # This method is agnostic from Project#shared_runners_enabled
+      def actual_minutes_used_up?
+        limit_enabled? && total_minutes_used >= total_minutes
+      end
+
       def total_minutes
         @total_minutes ||= monthly_minutes + purchased_minutes
       end
@@ -74,6 +81,14 @@ module Ci
 
       private
 
+      # TODO: rename to `enabled?`
+      # https://gitlab.com/gitlab-org/gitlab/-/issues/332933
+      def limit_enabled?
+        strong_memoize(:limit_enabled) do
+          namespace.root? && !!total_minutes.nonzero?
+        end
+      end
+
       def minutes_limit
         return monthly_minutes if enabled?
 

ee/app/models/ee/ci/pending_build.rb

+# frozen_string_literal: true
+
+module EE
+  module Ci
+    module PendingBuild
+      extend ActiveSupport::Concern
+
+      class_methods do
+        extend ::Gitlab::Utils::Override
+
+        override :args_from_build
+        def args_from_build(build)
+          return super unless ::Feature.enabled?(
+            :ci_pending_builds_maintain_ci_minutes_data,
+            build&.project&.root_namespace,
+            type: :development,
+            default_enabled: :yaml
+          )
+
+          super.merge(minutes_exceeded: minutes_exceeded?(build.project))
+        end
+
+        private
+
+        def minutes_exceeded?(project)
+          ::Ci::Runner.any_shared_runners_with_enabled_cost_factor?(project) &&
+            project.ci_minutes_quota.actual_minutes_used_up?
+        end
+      end
+    end
+  end
+end

ee/app/models/ee/ci/runner.rb

@@ -5,6 +5,18 @@ module EE
     module Runner
       extend ActiveSupport::Concern
 
+      prepended do
+        def self.any_shared_runners_with_enabled_cost_factor?(project)
+          if project.public?
+            return true if project.force_cost_factor?
+
+            instance_type.where('public_projects_minutes_cost_factor > 0').exists?
+          else
+            instance_type.where('private_projects_minutes_cost_factor > 0').exists?
+          end
+        end
+      end
+
       def cost_factor_for_project(project)
         cost_factor.for_project(project)
       end

ee/app/services/ci/minutes/additional_packs/create_service.rb

@@ -15,9 +15,13 @@ module Ci
         def execute
           authorize_current_user!
 
-          return successful_response if additional_pack.persisted?
+          if additional_pack.persisted? || save_additional_pack
+            reset_ci_minutes!
 
-          save_additional_pack ? successful_response : error_response
+            successful_response
+          else
+            error_response
+          end
         end
 
         private
@@ -58,6 +62,10 @@ module Ci
         def error_response
           error('Unable to save additional pack')
         end
+
+        def reset_ci_minutes!
+          ::Ci::Minutes::RefreshCachedDataService.new(namespace).execute
+        end
       end
     end
   end

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

+# frozen_string_literal: true
+
+module Ci
+  module Minutes
+    class RefreshCachedDataService
+      def initialize(root_namespace)
+        @root_namespace = root_namespace
+      end
+
+      def execute
+        return unless @root_namespace
+
+        reset_ci_minutes_cache!
+        update_pending_builds!
+      rescue StandardError => e
+        ::Gitlab::ErrorTracking.track_exception(
+          e,
+          root_namespace_id: @root_namespace.id
+        )
+      end
+
+      def reset_ci_minutes_cache!
+        ::Gitlab::Ci::Minutes::CachedQuota.new(@root_namespace).expire!
+      end
+
+      # rubocop: disable CodeReuse/ActiveRecord
+      def update_pending_builds!
+        return unless ::Feature.enabled?(:ci_pending_builds_maintain_ci_minutes_data, @root_namespace, type: :development, default_enabled: :yaml)
+
+        minutes_exceeded = @root_namespace.ci_minutes_quota.actual_minutes_used_up?
+        all_namespaces = @root_namespace.self_and_descendant_ids
+
+        ::Ci::PendingBuild.where(namespace: all_namespaces).update_all(minutes_exceeded: minutes_exceeded)
+      end
+      # rubocop: enable CodeReuse/ActiveRecord
+    end
+  end
+end

ee/app/services/clear_namespace_shared_runners_minutes_service.rb

@@ -11,7 +11,7 @@ class ClearNamespaceSharedRunnersMinutesService < BaseService
       shared_runners_seconds: 0,
       shared_runners_seconds_last_reset: Time.current
     ).tap do
-      ::Gitlab::Ci::Minutes::CachedQuota.new(@namespace).expire!
+      ::Ci::Minutes::RefreshCachedDataService.new(@namespace).execute
     end
   end
   # rubocop: enable CodeReuse/ActiveRecord

ee/lib/ee/api/namespaces.rb

@@ -30,7 +30,7 @@ module EE
             end
 
             if params[:extra_shared_runners_minutes_limit].present? || params[:shared_runners_minutes_limit].present?
-              ::Gitlab::Ci::Minutes::CachedQuota.new(namespace).expire!
+              ::Ci::Minutes::RefreshCachedDataService.new(namespace).execute
             end
 
             namespace.update(update_attrs)

ee/spec/models/ci/minutes/quota_spec.rb

@@ -326,6 +326,25 @@ RSpec.describe Ci::Minutes::Quota do
     end
   end
 
+  describe '#actual_minutes_used_up?' do
+    subject { quota.actual_minutes_used_up? }
+
+    where(:minutes_used, :minutes_limit, :result, :title) do
+      100 | 0   | false | 'limit not enabled'
+      99  | 100 | false | 'total minutes not used'
+      101 | 100 | true  | 'total minutes used'
+    end
+
+    with_them do
+      before do
+        allow(namespace).to receive(:shared_runners_seconds).and_return(minutes_used.minutes)
+        namespace.shared_runners_minutes_limit = minutes_limit
+      end
+
+      it { is_expected.to eq(result) }
+    end
+  end
+
   describe '#total_minutes' do
     subject { quota.total_minutes }
 

ee/spec/models/ee/ci/pending_build_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Ci::PendingBuild do
+  let_it_be(:project) { create(:project) }
+  let_it_be(:pipeline) { create(:ci_pipeline, project: project) }
+
+  let(:build) { create(:ci_build, :created, pipeline: pipeline, project: project) }
+
+  describe '.upsert_from_build!' do
+    shared_examples 'ci minutes not available' do
+      it 'sets minutes_exceeded to true' do
+        expect { described_class.upsert_from_build!(build) }.to change(Ci::PendingBuild, :count).by(1)
+
+        expect(described_class.last.minutes_exceeded).to be_truthy
+      end
+    end
+
+    shared_examples 'ci minutes available' do
+      it 'sets minutes_exceeded to false' do
+        expect { described_class.upsert_from_build!(build) }.to change(Ci::PendingBuild, :count).by(1)
+
+        expect(described_class.last.minutes_exceeded).to be_falsey
+      end
+    end
+
+    context 'when ci minutes are not available' do
+      before do
+        allow_next_instance_of(::Ci::Minutes::Quota) do |instance|
+          allow(instance).to receive(:actual_minutes_used_up?).and_return(true)
+        end
+      end
+
+      context 'when project matches shared runners with cost factor enabled' do
+        before do
+          allow(::Ci::Runner).to receive(:any_shared_runners_with_enabled_cost_factor?).and_return(true)
+        end
+
+        context 'when ci_pending_builds_maintain_ci_minutes_data is enabled' do
+          it_behaves_like 'ci minutes not available'
+        end
+
+        context 'when ci_pending_builds_maintain_ci_minutes_data is disabled' do
+          before do
+            stub_feature_flags(ci_pending_builds_maintain_ci_minutes_data: false)
+          end
+
+          it_behaves_like 'ci minutes available'
+        end
+      end
+
+      context 'when project does not matches shared runners with cost factor enabled' do
+        context 'when ci_pending_builds_maintain_ci_minutes_data is enabled' do
+          it_behaves_like 'ci minutes available'
+        end
+
+        context 'when ci_pending_builds_maintain_ci_minutes_data is disabled' do
+          before do
+            stub_feature_flags(ci_pending_builds_maintain_ci_minutes_data: false)
+          end
+
+          it_behaves_like 'ci minutes available'
+        end
+      end
+    end
+
+    context 'when ci minutes are available' do
+      context 'when ci_pending_builds_maintain_ci_minutes_data is enabled' do
+        it_behaves_like 'ci minutes available'
+      end
+
+      context 'when ci_pending_builds_maintain_ci_minutes_data is disabled' do
+        before do
+          stub_feature_flags(ci_pending_builds_maintain_ci_minutes_data: false)
+        end
+
+        it_behaves_like 'ci minutes available'
+      end
+    end
+
+    context 'when using shared runners with cost factor disabled' do
+      context 'with new project' do
+        it_behaves_like 'ci minutes available'
+      end
+    end
+  end
+end

ee/spec/models/ee/ci/runner_spec.rb

@@ -161,4 +161,64 @@ RSpec.describe EE::Ci::Runner do
       end
     end
   end
+
+  describe '.any_shared_runners_with_enabled_cost_factor' do
+    subject(:runners) { Ci::Runner.any_shared_runners_with_enabled_cost_factor?(project) }
+
+    let_it_be(:namespace) { create(:group) }
+
+    context 'when project is public' do
+      let_it_be(:project) { create(:project, namespace: namespace, visibility_level: ::Gitlab::VisibilityLevel::PUBLIC) }
+      let_it_be(:runner) { create(:ci_runner, :instance, public_projects_minutes_cost_factor: 0.0) }
+
+      context 'when cost factor is forced' do
+        before do
+          allow(project).to receive(:force_cost_factor?).and_return(true)
+        end
+
+        it 'returns true' do
+          expect(runners).to be_truthy
+        end
+      end
+
+      context 'when cost factor is not forced' do
+        context 'when public cost factor is greater than zero' do
+          before do
+            runner.update!(public_projects_minutes_cost_factor: 1.0)
+          end
+
+          it 'returns true' do
+            expect(runners).to be_truthy
+          end
+        end
+
+        context 'when public cost factor is zero' do
+          it 'returns false' do
+            expect(runners).to be_falsey
+          end
+        end
+      end
+    end
+
+    context 'when project is private' do
+      let_it_be(:project) { create(:project, namespace: namespace, visibility_level: ::Gitlab::VisibilityLevel::PRIVATE) }
+      let_it_be(:runner) { create(:ci_runner, :instance, private_projects_minutes_cost_factor: 1.0) }
+
+      context 'when private cost factor is greater than zero' do
+        it 'returns true' do
+          expect(runners).to be_truthy
+        end
+      end
+
+      context 'when private cost factor is zero' do
+        before do
+          runner.update!(private_projects_minutes_cost_factor: 0.0)
+        end
+
+        it 'returns false' do
+          expect(runners).to be_falsey
+        end
+      end
+    end
+  end
 end

ee/spec/services/ci/minutes/additional_packs/create_service_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe Ci::Minutes::AdditionalPacks::CreateService do
+  include AfterNextHelpers
+
   describe '#execute' do
     let_it_be(:namespace) { create(:namespace) }
     let_it_be(:admin) { build(:user, :admin) }
@@ -62,6 +64,12 @@ RSpec.describe Ci::Minutes::AdditionalPacks::CreateService do
           expect(pack.number_of_minutes).to eq params[:number_of_minutes]
         end
 
+        it 'kicks off reset ci minutes service' do
+          expect_next(::Ci::Minutes::RefreshCachedDataService).to receive(:execute)
+
+          result
+        end
+
         it 'returns success' do
           expect(result[:status]).to eq :success
         end

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

+# frozen_string_literal: true
+require 'spec_helper'
+
+RSpec.describe Ci::Minutes::RefreshCachedDataService do
+  include AfterNextHelpers
+
+  let_it_be(:project_1) { create(:project) }
+  let_it_be(:root_namespace) { project_1.root_namespace }
+  let_it_be(:build_1) { create(:ci_build, :pending, project: project_1) }
+  let_it_be(:build_2) { create(:ci_build, :pending) }
+  let_it_be(:pending_build_1) { create(:ci_pending_build, build: build_1, project: build_1.project, minutes_exceeded: true) }
+  let_it_be(:pending_build_2) { create(:ci_pending_build, build: build_2, project: build_2.project, minutes_exceeded: true) }
+
+  describe '#execute' do
+    subject { described_class.new(root_namespace).execute }
+
+    context 'when root_namespace is nil' do
+      let(:root_namespace) { nil }
+
+      it 'does nothing' do
+        expect { subject }.not_to raise_error
+
+        expect_next(::Gitlab::Ci::Minutes::CachedQuota).not_to receive(:expire!)
+
+        expect(pending_build_1.reload.minutes_exceeded).to be_truthy
+        expect(pending_build_2.reload.minutes_exceeded).to be_truthy
+      end
+    end
+
+    context 'when user purchases more ci minutes for a given namespace' do
+      before do
+        allow_next_instance_of(::Ci::Minutes::Quota) do |instance|
+          allow(instance).to receive(:actual_minutes_used_up?).and_return(false)
+        end
+      end
+
+      it 'updates relevant pending builds' do
+        subject
+
+        expect(pending_build_1.reload.minutes_exceeded).to be_falsey
+        expect(pending_build_2.reload.minutes_exceeded).to be_truthy
+      end
+
+      context 'when ci_pending_builds_maintain_ci_minutes_data is disabled' do
+        before do
+          stub_feature_flags(ci_pending_builds_maintain_ci_minutes_data: false)
+        end
+
+        it 'does not update pending builds' do
+          subject
+
+          expect(pending_build_1.reload.minutes_exceeded).to be_truthy
+          expect(pending_build_2.reload.minutes_exceeded).to be_truthy
+        end
+      end
+
+      it 'expires the CachedQuota' do
+        expect_next(::Gitlab::Ci::Minutes::CachedQuota).to receive(:expire!)
+
+        subject
+      end
+    end
+  end
+end

spec/factories/ci/pending_builds.rb

@@ -6,5 +6,7 @@ FactoryBot.define do
     project
     protected { build.protected }
     instance_runners_enabled { true }
+    namespace { project.namespace }
+    minutes_exceeded { false }
   end
 end

spec/models/ci/pending_build_spec.rb

@@ -8,6 +8,12 @@ RSpec.describe Ci::PendingBuild do
 
   let(:build) { create(:ci_build, :created, pipeline: pipeline) }
 
+  describe 'associations' do
+    it { is_expected.to belong_to :project }
+    it { is_expected.to belong_to :build }
+    it { is_expected.to belong_to :namespace }
+  end
+
   describe '.upsert_from_build!' do
     context 'another pending entry does not exist' do
       it 'creates a new pending entry' do

spec/models/namespace_spec.rb

@@ -23,6 +23,7 @@ RSpec.describe Namespace do
     it { is_expected.to have_one :package_setting_relation }
     it { is_expected.to have_one :onboarding_progress }
     it { is_expected.to have_one :admin_note }
+    it { is_expected.to have_many :pending_builds }
   end
 
   describe 'validations' do