Merge branch 'lazily-recalculate-ci-additional-minutes' into 'master'

Recalculate additional CI minutes lazily on a monthly basis

See merge request gitlab-org/gitlab!73338

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

@@ -35,6 +35,28 @@ module Ci
         purchased > 0
       end
 
+      def recalculate_remaining_purchased_minutes!
+        return unless should_recalculate_purchased_minutes?
+
+        # Since we reset CI minutes data lazily, we take the last known usage
+        # and not necessarily the previous month data because that represents
+        # last time we reset the data.
+        # Jan: monthly_minutes: 1_000, purchased_minutes: 500, minutes_used: 1_200
+        # Feb: no activity (no pipelines, no data read)
+        # Mar: reset and update purchased minutes to (1_000 + 500 - 1_200) = 300
+        previous_amount_used = Ci::Minutes::NamespaceMonthlyUsage
+          .previous_usage(namespace)
+          &.amount_used.to_i
+
+        return unless previous_amount_used > 0
+
+        # Do nothing if the namespace had not used all the monthly minutes
+        return if previous_amount_used < monthly
+
+        balance = [(total - previous_amount_used).to_i, 0].max
+        namespace.update!(extra_shared_runners_minutes_limit: balance)
+      end
+
       private
 
       attr_reader :namespace
@@ -42,6 +64,12 @@ module Ci
       def unlimited?
         total == 0
       end
+
+      def should_recalculate_purchased_minutes?
+        Feature.enabled?(:ci_reset_purchased_minutes_lazily, namespace, default_enabled: :yaml) &&
+          enabled? &&
+          any_purchased?
+      end
     end
   end
 end

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

@@ -12,19 +12,30 @@ module Ci
       scope :current_month, -> { where(date: beginning_of_month) }
       scope :for_namespace, -> (namespace) { where(namespace: namespace) }
 
+      def self.previous_usage(namespace)
+        for_namespace(namespace).where("#{quoted_table_name}.date < :date", date: beginning_of_month).order(:date).last
+      end
+
       def self.beginning_of_month(time = Time.current)
         time.utc.beginning_of_month
       end
 
-      # We should pretty much always use this method to access data for the current month
+      # We should always use this method to access data for the current month
       # since this will lazily create an entry if it doesn't exist.
       # For example, on the 1st of each month, when we update the usage for a namespace,
       # we will automatically generate new records and reset usage for the current month.
-      #
-      # Here we will also do any recalculation of additional minutes based on the
-      # previous month usage.
+      # This also recalculates any additional minutes based on the previous month usage.
       def self.find_or_create_current(namespace_id:)
-        current_month.safe_find_or_create_by(namespace_id: namespace_id)
+        current_usage = unsafe_find_current(namespace_id)
+        return current_usage if current_usage
+
+        current_month.for_namespace(namespace_id).create!.tap do
+          Namespace.find_by_id(namespace_id).try do |namespace|
+            Ci::Minutes::Limit.new(namespace).recalculate_remaining_purchased_minutes!
+          end
+        end
+      rescue ActiveRecord::RecordNotUnique
+        unsafe_find_current(namespace_id)
       end
 
       def self.increase_usage(usage, increments)
@@ -50,6 +61,13 @@ module Ci
       end
       private_class_method :update_current
 
+      # This is unsafe to use publicly because it would read the data
+      # without creating a new record if doesn't exist.
+      def self.unsafe_find_current(namespace)
+        current_month.for_namespace(namespace).take
+      end
+      private_class_method :unsafe_find_current
+
       def total_usage_notified?
         usage_notified?(Notification::PERCENTAGES.fetch(:exceeded))
       end

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

@@ -62,6 +62,8 @@ module Ci
       # 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)
+        return if Feature.enabled?(:ci_reset_purchased_minutes_lazily, default_enabled: :yaml)
+
         namespaces
           .joins(:namespace_statistics)
           .where(namespaces_arel[:extra_shared_runners_minutes_limit].gt(0))

ee/config/feature_flags/development/ci_reset_purchased_minutes_lazily.yml

+---
+name: ci_reset_purchased_minutes_lazily
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/73338
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/344821
+milestone: '14.5'
+type: development
+group: group::pipeline execution
+default_enabled: false

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

@@ -90,4 +90,46 @@ RSpec.describe Ci::Minutes::Limit do
       end
     end
   end
+
+  describe '#recalculate_remaining_purchased_minutes!' do
+    subject { limit.recalculate_remaining_purchased_minutes! }
+
+    where(:purchased_minutes, :namespace_monthly_limit, :previous_amount_used, :ff_enabled, :expected_purchased_limit) do
+      200 | 400 | 0   | true  | 200 # no minutes used
+      200 | 0   | 0   | true  | 200 # monthly limit disabled
+      0   | 0   | 0   | true  | 0   # monthly limit disabled and no purchased minutes
+      200 | 400 | nil | true  | 200 # no previous month usage
+      200 | 400 | 300 | true  | 200 # previous usage < monthly limit
+      200 | 400 | 500 | true  | 100 # previous usage > monthly limit => purchased minutes reduced
+      200 | 400 | 500 | false | 200 # same as above but FF disabled
+      0   | 400 | 500 | true  | 0   # no purchased minutes = nothing reduced
+      200 | 400 | 600 | true  | 0   # previous usage == total limit => purchased minutes reduced
+      200 | 400 | 600 | false | 200 # same as above but FF disabled
+      200 | 400 | 800 | true  | 0   # previous usage > total limit => purchased minutes reduced but not negative
+      200 | 400 | 800 | false | 200 # same as above but FF disabled
+    end
+
+    with_them do
+      before do
+        if previous_amount_used
+          create(:ci_namespace_monthly_usage,
+            namespace: namespace,
+            date: Ci::Minutes::NamespaceMonthlyUsage.beginning_of_month(2.months.ago),
+            amount_used: previous_amount_used)
+
+          create(:ci_namespace_monthly_usage,
+            namespace: namespace,
+            date: Ci::Minutes::NamespaceMonthlyUsage.beginning_of_month(3.months.ago),
+            amount_used: 5_000)
+        end
+
+        stub_feature_flags(ci_reset_purchased_minutes_lazily: ff_enabled)
+      end
+
+      it 'has the expected purchased minutes' do
+        subject
+        expect(namespace.extra_shared_runners_minutes_limit).to eq(expected_purchased_limit)
+      end
+    end
+  end
 end

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

@@ -3,7 +3,11 @@
 require 'spec_helper'
 
 RSpec.describe Ci::Minutes::NamespaceMonthlyUsage do
-  let_it_be(:namespace) { create(:namespace) }
+  let_it_be(:namespace) do
+    create(:namespace,
+      shared_runners_minutes_limit: 1_000,
+      extra_shared_runners_minutes_limit: 500)
+  end
 
   let_it_be_with_refind(:current_usage) do
     create(:ci_namespace_monthly_usage,
@@ -35,29 +39,133 @@ RSpec.describe Ci::Minutes::NamespaceMonthlyUsage do
           expect(subject.amount_used).to eq(0)
           expect(subject.namespace).to eq(namespace)
           expect(subject.date).to eq(described_class.beginning_of_month)
+          expect(subject.notification_level).to eq(::Ci::Minutes::Notification::PERCENTAGES.fetch(:not_set))
         end
       end
     end
 
+    shared_examples 'does not update the additional minutes' do
+      it 'does not update the additional minutes' do
+        expect { subject }
+          .not_to change { namespace.reload.extra_shared_runners_minutes_limit }
+      end
+    end
+
+    shared_examples 'attempts recalculation of additional minutes' do
+      context 'when namespace has any additional minutes' do
+        context 'when last known amount_used is greater than the monthly limit' do
+          before do
+            previous_usage.update!(amount_used: 1_200)
+          end
+
+          it 'recalculates the remaining additional minutes' do
+            expect { subject }
+              .to change { namespace.reload.extra_shared_runners_minutes_limit }
+              .from(500).to(300)
+          end
+
+          context 'when last known amount_used is greater than the total limit' do
+            before do
+              previous_usage.update!(amount_used: 2_000)
+            end
+
+            it 'recalculates the remaining additional minutes' do
+              expect { subject }
+                .to change { namespace.reload.extra_shared_runners_minutes_limit }
+                .from(500).to(0)
+            end
+          end
+
+          context 'when ci_reset_purchased_minutes_lazily feature flag is disabled' do
+            before do
+              stub_feature_flags(ci_reset_purchased_minutes_lazily: false)
+            end
+
+            it_behaves_like 'does not update the additional minutes'
+          end
+
+          context 'when limit is disabled' do
+            before do
+              namespace.update!(
+                shared_runners_minutes_limit: 0,
+                extra_shared_runners_minutes_limit: 0)
+            end
+
+            it_behaves_like 'does not update the additional minutes'
+          end
+        end
+
+        context 'when amount_used is lower than the monthly limit' do
+          before do
+            previous_usage.update!(amount_used: 900)
+          end
+
+          it_behaves_like 'does not update the additional minutes'
+        end
+      end
+
+      context 'when namespace does not have additional minutes' do
+        before do
+          namespace.update!(extra_shared_runners_minutes_limit: 0)
+        end
+
+        it_behaves_like 'does not update the additional minutes'
+      end
+    end
+
     context 'when namespace usage does not exist for current month' do
       before do
         current_usage.destroy!
       end
 
       it_behaves_like 'creates usage record'
+      it_behaves_like 'does not update the additional minutes'
+
+      context 'when namespace usage exists for previous month' do
+        let!(:previous_usage) do
+          create(:ci_namespace_monthly_usage,
+            namespace: namespace,
+            date: described_class.beginning_of_month(1.month.ago))
+        end
+
+        it_behaves_like 'creates usage record'
+        it_behaves_like 'attempts recalculation of additional minutes'
+      end
+
+      context 'when last known usage is more than 1 month ago' do
+        let!(:previous_usage) do
+          create(:ci_namespace_monthly_usage,
+            namespace: namespace,
+            date: described_class.beginning_of_month(3.months.ago))
+        end
+
+        it_behaves_like 'creates usage record'
+        it_behaves_like 'attempts recalculation of additional minutes'
+      end
 
       context 'when namespace usage exists for previous months' do
-        before do
-          create(:ci_namespace_monthly_usage, namespace: namespace, date: described_class.beginning_of_month(2.months.ago))
+        let!(:previous_usage) do
+          create(:ci_namespace_monthly_usage,
+            namespace: namespace,
+            date: described_class.beginning_of_month(1.month.ago))
+        end
+
+        let!(:old_usage) do
+          create(:ci_namespace_monthly_usage,
+            namespace: namespace,
+            date: described_class.beginning_of_month(2.months.ago),
+            amount_used: 2_000)
         end
 
         it_behaves_like 'creates usage record'
+        it_behaves_like 'attempts recalculation of additional minutes'
       end
 
       context 'when a usage for another namespace exists for the current month' do
         let!(:usage) { create(:ci_namespace_monthly_usage) }
 
         it_behaves_like 'creates usage record'
+        it_behaves_like 'does not update the additional minutes'
       end
     end
 
@@ -67,6 +175,8 @@ RSpec.describe Ci::Minutes::NamespaceMonthlyUsage do
           expect(subject).to eq(current_usage)
         end
       end
+
+      it_behaves_like 'does not update the additional minutes'
     end
   end
 
@@ -84,6 +194,23 @@ RSpec.describe Ci::Minutes::NamespaceMonthlyUsage do
     end
   end
 
+  describe '.previous_usage' do
+    subject { described_class.previous_usage(namespace) }
+
+    context 'when there are no usage records' do
+      it { is_expected.to be_nil }
+    end
+
+    context 'when there are usage records for the previous month' do
+      let(:current_month) { described_class.beginning_of_month }
+
+      let!(:previous_month_usage) { create(:ci_namespace_monthly_usage, namespace: namespace, amount_used: 200, date: current_month - 2.months) }
+      let!(:very_old_usage) { create(:ci_namespace_monthly_usage, namespace: namespace, amount_used: 300, date: current_month - 3.months) }
+
+      it { is_expected.to eq(previous_month_usage) }
+    end
+  end
+
   describe '.reset_current_usage', :aggregate_failures do
     subject { described_class.reset_current_usage(namespace) }
 

ee/spec/models/ee/namespace_spec.rb

@@ -678,22 +678,6 @@ RSpec.describe Namespace do
     end
   end
 
-  describe '#new_monthly_ci_minutes_enabled?' do
-    subject { namespace.new_monthly_ci_minutes_enabled? }
-
-    context 'when feature flag ci_use_new_monthly_minutes is enabled' do
-      it { is_expected.to be_truthy }
-    end
-
-    context 'when feature flag ci_use_new_monthly_minutes is disabled' do
-      before do
-        stub_feature_flags(ci_use_new_monthly_minutes: false)
-      end
-
-      it { is_expected.to be_falsy }
-    end
-  end
-
   describe '#shared_runners_minutes_limit_enabled?' do
     subject { namespace.shared_runners_minutes_limit_enabled? }
 

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

@@ -53,19 +53,43 @@ RSpec.describe Ci::Minutes::BatchResetService do
         subject
       end
 
-      it 'resets CI minutes and recalculates purchased minutes for the namespace exceeding the monthly minutes' do
-        subject
+      context 'when feature flag ci_reset_purchased_minutes_lazily is enabled' do
+        it 'resets CI minutes but does not recalculate 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 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
 
-          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
+      context 'when feature flag ci_reset_purchased_minutes_lazily is disabled' do
+        before do
+          stub_feature_flags(ci_reset_purchased_minutes_lazily: false)
+        end
+
+        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
+
+            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
       end
 
@@ -129,6 +153,7 @@ RSpec.describe Ci::Minutes::BatchResetService do
 
         before do
           allow(::Gitlab::CurrentSettings).to receive(:shared_runners_minutes).and_return(global_limit)
+          stub_feature_flags(ci_reset_purchased_minutes_lazily: false)
         end
 
         it 'does not recalculate purchased minutes for any namespaces' do

ee/spec/workers/ci/batch_reset_minutes_worker_spec.rb

@@ -66,16 +66,30 @@ RSpec.describe Ci::BatchResetMinutesWorker do
         let(:namespace) { last_namespace }
       end
 
-      it 'recalculates purchased minutes for the namespace exceeding the monthly minutes' do
-        subject
+      context 'when ci_reset_purchased_minutes_lazily is enabled' do
+        it 'does not recalculate purchased minutes for the namespace exceeding the monthly minutes' do
+          subject
 
-        expect(first_namespace.reset.extra_shared_runners_minutes_limit).to eq 30
+          expect(first_namespace.reset.extra_shared_runners_minutes_limit).to eq 50
+        end
       end
 
-      it 'does not recalculate purchased minutes for the namespace not exceeding the monthly minutes' do
-        subject
+      context 'when ci_reset_purchased_minutes_lazily is disabled' do
+        before do
+          stub_feature_flags(ci_reset_purchased_minutes_lazily: false)
+        end
+
+        it 'recalculates purchased minutes for the namespace exceeding the monthly minutes' do
+          subject
+
+          expect(first_namespace.reset.extra_shared_runners_minutes_limit).to eq 30
+        end
 
-        expect(last_namespace.reset.extra_shared_runners_minutes_limit).to eq 50
+        it 'does not recalculate purchased minutes for the namespace not exceeding the monthly minutes' do
+          subject
+
+          expect(last_namespace.reset.extra_shared_runners_minutes_limit).to eq 50
+        end
       end
     end
   end