Recalculate CI extra minutes when monthly minutes defaults to nil

This fixes a bug that skipped recalculating CI extra minutes
where namespaces.shared_runners_minutes_limit was `nil`.

ee/app/models/ee/namespace.rb

@@ -36,13 +36,6 @@ module EE
       scope :include_gitlab_subscription, -> { includes(:gitlab_subscription) }
       scope :join_gitlab_subscription, -> { joins("LEFT OUTER JOIN gitlab_subscriptions ON gitlab_subscriptions.namespace_id=namespaces.id") }
 
-      scope :requiring_ci_extra_minutes_recalculation, -> do
-        joins(:namespace_statistics)
-          .where('namespaces.shared_runners_minutes_limit > 0')
-          .where('namespaces.extra_shared_runners_minutes_limit > 0')
-          .where('namespace_statistics.shared_runners_seconds > (namespaces.shared_runners_minutes_limit * 60)')
-      end
-
       scope :with_feature_available_in_plan, -> (feature) do
         plans = plans_with_feature(feature)
         matcher = ::Plan.where(name: plans)

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

@@ -39,14 +39,44 @@ module Ci
         @errors << { count: namespaces.size, first_id: namespaces.first.id, last_id: namespaces.last.id }
       end
 
+      # This service is responsible for the logic that recalculates the extra shared runners
+      # minutes including how to deal with the cases where shared_runners_minutes_limit is `nil`.
+      # We prefer to keep the queries here rather than scatter them across classes.
+      # rubocop: disable CodeReuse/ActiveRecord
       def recalculate_extra_shared_runners_minutes_limits!(namespaces)
         namespaces
-          .requiring_ci_extra_minutes_recalculation
-          .update_all("extra_shared_runners_minutes_limit = #{extra_minutes_left_sql} FROM namespace_statistics")
+          .joins(:namespace_statistics)
+          .where(namespaces_arel[:extra_shared_runners_minutes_limit].gt(0))
+          .where(actual_shared_runners_minutes_limit.gt(0))
+          .where(namespaces_statistics_arel[:shared_runners_seconds].gt(actual_shared_runners_minutes_limit * 60))
+          .update_all("extra_shared_runners_minutes_limit = #{extra_minutes_left.to_sql} FROM namespace_statistics")
       end
+      # rubocop: enable CodeReuse/ActiveRecord
 
-      def extra_minutes_left_sql
-        "GREATEST((namespaces.shared_runners_minutes_limit + namespaces.extra_shared_runners_minutes_limit) - ROUND(namespace_statistics.shared_runners_seconds / 60.0), 0)"
+      def extra_minutes_left
+        shared_minutes_limit = actual_shared_runners_minutes_limit + namespaces_arel[:extra_shared_runners_minutes_limit]
+        used_minutes = arel_function("round", [namespaces_statistics_arel[:shared_runners_seconds] / Arel::Nodes::SqlLiteral.new('60.0')])
+
+        arel_function("greatest", [shared_minutes_limit - used_minutes, 0])
+      end
+
+      def actual_shared_runners_minutes_limit
+        namespaces_arel.coalesce(
+          namespaces_arel[:shared_runners_minutes_limit],
+          [::Gitlab::CurrentSettings.shared_runners_minutes.presence, 0].compact
+        )
+      end
+
+      def namespaces_arel
+        Namespace.arel_table
+      end
+
+      def namespaces_statistics_arel
+        NamespaceStatistics.arel_table
+      end
+
+      def arel_function(name, attrs)
+        Arel::Nodes::NamedFunction.new(name, attrs)
       end
 
       def reset_shared_runners_seconds!(namespaces)

ee/changelogs/unreleased/fix-ci-extra-minutes-recalculation.yml

+---
+title: Recalculate CI extra minutes when monthly minutes default to nil
+merge_request: 36046
+author:
+type: fixed

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

@@ -6,10 +6,14 @@ RSpec.describe Ci::Minutes::BatchResetService do
   let(:service) { described_class.new }
 
   describe '#execute!' do
-    def create_namespace_with_project(id, seconds_used)
+    let(:ids_range) { nil }
+
+    subject { service.execute!(ids_range: ids_range, batch_size: 3) }
+
+    def create_namespace_with_project(id, seconds_used, monthly_minutes_limit = nil)
       namespace = create(:namespace,
         id: id,
-        shared_runners_minutes_limit: 100,
+        shared_runners_minutes_limit: monthly_minutes_limit, # when `nil` it inherits the global limit
         extra_shared_runners_minutes_limit: 50,
         last_ci_minutes_notification_at: Time.current,
         last_ci_minutes_usage_notification_level: 30)
@@ -26,98 +30,142 @@ RSpec.describe Ci::Minutes::BatchResetService do
       namespace
     end
 
-    subject { service.execute!(ids_range: ids_range, batch_size: 3) }
+    context 'when global shared_runners_minutes is enabled' do
+      before do
+        allow(::Gitlab::CurrentSettings).to receive(:shared_runners_minutes).and_return(2_000)
+      end
 
-    let!(:namespace_1) { create_namespace_with_project(1, 120.minutes) }
-    let!(:namespace_2) { create_namespace_with_project(2, 120.minutes) }
-    let!(:namespace_3) { create_namespace_with_project(3, 120.minutes) }
-    let!(:namespace_4) { create_namespace_with_project(4, 90.minutes) }
-    let!(:namespace_5) { create_namespace_with_project(5, 90.minutes) }
-    let!(:namespace_6) { create_namespace_with_project(6, 90.minutes) }
+      let!(:namespace_1) { create_namespace_with_project(1, 2_020.minutes, nil) }
+      let!(:namespace_2) { create_namespace_with_project(2, 2_020.minutes, 2_000) }
+      let!(:namespace_3) { create_namespace_with_project(3, 2_020.minutes, 2_000) }
+      let!(:namespace_4) { create_namespace_with_project(4, 1_000.minutes, nil) }
+      let!(:namespace_5) { create_namespace_with_project(5, 1_000.minutes, 2_000) }
+      let!(:namespace_6) { create_namespace_with_project(6, 1_000.minutes, 0) }
 
-    context 'when ID range is provided' do
-      let(:ids_range) { (1..5) }
-      let(:namespaces_exceeding_minutes) { [namespace_1, namespace_2, namespace_3] }
-      let(:namespaces_not_exceeding_minutes) { [namespace_4, namespace_5] }
+      context 'when ID range is provided' do
+        let(:ids_range) { (1..5) }
+        let(:namespaces_exceeding_minutes) { [namespace_1, namespace_2, namespace_3] }
+        let(:namespaces_not_exceeding_minutes) { [namespace_4, namespace_5] }
 
-      it 'resets minutes in batches for the given range' do
-        expect(service).to receive(:reset_ci_minutes!).with([namespace_1, namespace_2, namespace_3])
-        expect(service).to receive(:reset_ci_minutes!).with([namespace_4, namespace_5])
+        it 'resets minutes in batches for the given range' do
+          expect(service).to receive(:reset_ci_minutes!).with([namespace_1, namespace_2, namespace_3])
+          expect(service).to receive(:reset_ci_minutes!).with([namespace_4, namespace_5])
 
-        subject
-      end
+          subject
+        end
 
-      it 'resets CI minutes and recalculates purchased minutes for the namespace exceeding the monthly minutes' do
-        subject
+        it 'resets CI minutes and recalculates purchased minutes for the namespace exceeding the monthly minutes' do
+          subject
 
-        namespaces_exceeding_minutes.each do |namespace|
-          namespace.reset
+          namespaces_exceeding_minutes.each do |namespace|
+            namespace.reset
+
+            expect(namespace.extra_shared_runners_minutes_limit).to eq 30
+            expect(namespace.namespace_statistics.shared_runners_seconds).to eq 0
+            expect(namespace.namespace_statistics.shared_runners_seconds_last_reset).to be_present
+            expect(ProjectStatistics.find_by(namespace: namespace).shared_runners_seconds).to eq 0
+            expect(ProjectStatistics.find_by(namespace: namespace).shared_runners_seconds_last_reset).to be_present
+            expect(namespace.last_ci_minutes_notification_at).to be_nil
+            expect(namespace.last_ci_minutes_usage_notification_level).to be_nil
+          end
+        end
+
+        it 'resets CI minutes but does not recalculate purchased minutes for the namespace not exceeding the monthly minutes' do
+          subject
 
-          expect(namespace.extra_shared_runners_minutes_limit).to eq 30
-          expect(namespace.namespace_statistics.shared_runners_seconds).to eq 0
-          expect(namespace.namespace_statistics.shared_runners_seconds_last_reset).to be_present
-          expect(ProjectStatistics.find_by(namespace: namespace).shared_runners_seconds).to eq 0
-          expect(ProjectStatistics.find_by(namespace: namespace).shared_runners_seconds_last_reset).to be_present
-          expect(namespace.last_ci_minutes_notification_at).to be_nil
-          expect(namespace.last_ci_minutes_usage_notification_level).to be_nil
+          namespaces_not_exceeding_minutes.each do |namespace|
+            namespace.reset
+
+            expect(namespace.extra_shared_runners_minutes_limit).to eq 50
+            expect(namespace.namespace_statistics.shared_runners_seconds).to eq 0
+            expect(namespace.namespace_statistics.shared_runners_seconds_last_reset).to be_present
+            expect(ProjectStatistics.find_by(namespace: namespace).shared_runners_seconds).to eq 0
+            expect(ProjectStatistics.find_by(namespace: namespace).shared_runners_seconds_last_reset).to be_present
+            expect(namespace.last_ci_minutes_notification_at).to be_nil
+            expect(namespace.last_ci_minutes_usage_notification_level).to be_nil
+          end
         end
       end
 
-      it 'resets CI minutes but does not recalculate purchased minutes for the namespace not exceeding the monthly minutes' do
-        subject
+      context 'when ID range is not provided' do
+        let(:ids_range) { nil }
 
-        namespaces_not_exceeding_minutes.each do |namespace|
-          namespace.reset
+        it 'resets minutes in batches for all namespaces' do
+          expect(service).to receive(:reset_ci_minutes!).with([namespace_1, namespace_2, namespace_3])
+          expect(service).to receive(:reset_ci_minutes!).with([namespace_4, namespace_5, namespace_6])
 
-          expect(namespace.extra_shared_runners_minutes_limit).to eq 50
-          expect(namespace.namespace_statistics.shared_runners_seconds).to eq 0
-          expect(namespace.namespace_statistics.shared_runners_seconds_last_reset).to be_present
-          expect(ProjectStatistics.find_by(namespace: namespace).shared_runners_seconds).to eq 0
-          expect(ProjectStatistics.find_by(namespace: namespace).shared_runners_seconds_last_reset).to be_present
-          expect(namespace.last_ci_minutes_notification_at).to be_nil
-          expect(namespace.last_ci_minutes_usage_notification_level).to be_nil
+          subject
+        end
+
+        it 'resets CI minutes and does not recalculate purchased minutes for the namespace having unlimited monthly minutes' do
+          subject
+
+          namespace_6.reset
+
+          expect(namespace_6.extra_shared_runners_minutes_limit).to eq 50
+          expect(namespace_6.namespace_statistics.shared_runners_seconds).to eq 0
+          expect(namespace_6.namespace_statistics.shared_runners_seconds_last_reset).to be_present
+          expect(ProjectStatistics.find_by(namespace: namespace_6).shared_runners_seconds).to eq 0
+          expect(ProjectStatistics.find_by(namespace: namespace_6).shared_runners_seconds_last_reset).to be_present
+          expect(namespace_6.last_ci_minutes_notification_at).to be_nil
+          expect(namespace_6.last_ci_minutes_usage_notification_level).to be_nil
         end
       end
-    end
 
-    context 'when ID range is not provided' do
-      let(:ids_range) { nil }
+      context 'when an ActiveRecordError is raised' do
+        let(:ids_range) { nil }
 
-      it 'resets minutes in batches for all namespaces' do
-        expect(service).to receive(:reset_ci_minutes!).with([namespace_1, namespace_2, namespace_3])
-        expect(service).to receive(:reset_ci_minutes!).with([namespace_4, namespace_5, namespace_6])
+        before do
+          expect(Namespace).to receive(:transaction).once.ordered.and_raise(ActiveRecord::ActiveRecordError)
+          expect(Namespace).to receive(:transaction).once.ordered.and_call_original
+        end
+
+        it 'continues its progress' do
+          expect(service).to receive(:reset_ci_minutes!).with([namespace_1, namespace_2, namespace_3]).and_call_original
+          expect(service).to receive(:reset_ci_minutes!).with([namespace_4, namespace_5, namespace_6]).and_call_original
+
+          expect(Gitlab::ErrorTracking).to receive(:track_and_raise_exception)
+          subject
+        end
 
-        subject
+        it 'raises exception with namespace details' do
+          expect(Gitlab::ErrorTracking).to receive(
+            :track_and_raise_exception
+          ).with(
+            Ci::Minutes::BatchResetService::BatchNotResetError.new(
+              'Some namespace shared runner minutes were not reset.'
+            ),
+            { namespace_ranges: [{ count: 3, first_id: 1, last_id: 3 }] }
+          ).once.and_call_original
+
+          expect { subject }.to raise_error(Ci::Minutes::BatchResetService::BatchNotResetError)
+        end
       end
     end
 
-    context 'when an ActiveRecordError is raised' do
-      let(:ids_range) { nil }
+    context 'when global shared_runners_minutes setting is nil and namespaces have no limits' do
+      using RSpec::Parameterized::TableSyntax
 
-      before do
-        expect(Namespace).to receive(:transaction).once.ordered.and_raise(ActiveRecord::ActiveRecordError)
-        expect(Namespace).to receive(:transaction).once.ordered.and_call_original
+      where(:global_limit, :namespace_limit) do
+        nil | 0
+        nil | nil
+        0   | 0
+        0   | nil
       end
 
-      it 'continues its progress' do
-        expect(service).to receive(:reset_ci_minutes!).with([namespace_1, namespace_2, namespace_3]).and_call_original
-        expect(service).to receive(:reset_ci_minutes!).with([namespace_4, namespace_5, namespace_6]).and_call_original
+      with_them do
+        let!(:namespace) { create_namespace_with_project(1, 100.minutes, namespace_limit) }
 
-        expect(Gitlab::ErrorTracking).to receive(:track_and_raise_exception)
-        subject
-      end
+        before do
+          allow(::Gitlab::CurrentSettings).to receive(:shared_runners_minutes).and_return(global_limit)
+        end
 
-      it 'raises exception with namespace details' do
-        expect(Gitlab::ErrorTracking).to receive(
-          :track_and_raise_exception
-        ).with(
-          Ci::Minutes::BatchResetService::BatchNotResetError.new(
-            'Some namespace shared runner minutes were not reset.'
-          ),
-          { namespace_ranges: [{ count: 3, first_id: 1, last_id: 3 }] }
-        ).once.and_call_original
-
-        expect { subject }.to raise_error(Ci::Minutes::BatchResetService::BatchNotResetError)
+        it 'does not recalculate purchased minutes for any namespaces' do
+          subject
+
+          namespace.reset
+          expect(namespace.extra_shared_runners_minutes_limit).to eq 50
+        end
       end
     end
   end