Merge branch...

Merge branch '35878-possible-pipeline-minutes-usage-not-reset-for-2019-11-for-some-groups' into 'master'

Raise exception if any namespaces runner minutes were not reset

Closes #35878

See merge request gitlab-org/gitlab!22636

app/models/namespace.rb

@@ -131,6 +131,11 @@ class Namespace < ApplicationRecord
       name = host.delete_suffix(gitlab_host)
       Namespace.find_by_full_path(name)
     end
+
+    # overridden in ee
+    def reset_ci_minutes!(namespace_id)
+      false
+    end
   end
 
   def visibility_level_field

ee/app/models/ee/namespace.rb

@@ -79,9 +79,70 @@ module EE
     end
 
     class_methods do
+      extend ::Gitlab::Utils::Override
+
+      NamespaceStatisticsNotResetError = Class.new(StandardError)
+
       def plans_with_feature(feature)
         LICENSE_PLANS_TO_NAMESPACE_PLANS.values_at(*License.plans_with_feature(feature))
       end
+
+      def reset_ci_minutes_in_batches!
+        each_batch do |namespaces|
+          namespace_ids = namespaces.pluck(:id)
+          reset_ci_minutes!(namespace_ids)
+        end
+      end
+
+      # ensure that recalculation of extra shared runners minutes occurs in the same
+      # transaction as the reset of the namespace statistics. If the transaction fails
+      # none of the changes apply but the numbers still remain consistent with each other.
+      override :reset_ci_minutes!
+      def reset_ci_minutes!(namespace_ids)
+        transaction do
+          recalculate_extra_shared_runners_minutes_limits!(namespace_ids)
+          reset_shared_runners_seconds!(namespace_ids)
+          reset_ci_minutes_notifications!(namespace_ids)
+        end
+        true
+      rescue ActiveRecord::ActiveRecordError
+        # We don't need to print thousands of namespace_ids
+        # in the message if all batches failed.
+        # A small batch would be sufficient for investigation.
+        failed_namespace_ids = namespace_ids.first(10)
+
+        raise EE::Namespace::NamespaceStatisticsNotResetError,
+          "#{namespace_ids.count} namespace shared runner minutes were not reset and the transaction was rolled back. Namespace Ids: #{failed_namespace_ids}"
+      end
+
+      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)"
+      end
+
+      def recalculate_extra_shared_runners_minutes_limits!(namespace_ids)
+        where(id: namespace_ids)
+          .with_shared_runners_minutes_limit
+          .with_extra_shared_runners_minutes_limit
+          .with_shared_runners_minutes_exceeding_default_limit
+          .update_all("extra_shared_runners_minutes_limit = #{extra_minutes_left_sql} FROM namespace_statistics")
+      end
+
+      def reset_shared_runners_seconds!(namespace_ids)
+        NamespaceStatistics
+          .where(namespace: namespace_ids)
+          .where.not(shared_runners_seconds: 0)
+          .update_all(shared_runners_seconds: 0, shared_runners_seconds_last_reset: Time.current)
+
+        ::ProjectStatistics
+          .where(namespace: namespace_ids)
+          .where.not(shared_runners_seconds: 0)
+          .update_all(shared_runners_seconds: 0, shared_runners_seconds_last_reset: Time.current)
+      end
+
+      def reset_ci_minutes_notifications!(namespace_ids)
+        where(id: namespace_ids)
+          .update_all(last_ci_minutes_notification_at: nil, last_ci_minutes_usage_notification_level: nil)
+      end
     end
 
     override :move_dir

ee/app/workers/clear_shared_runners_minutes_worker.rb

@@ -11,37 +11,11 @@ class ClearSharedRunnersMinutesWorker
   def perform
     return unless try_obtain_lease
 
-    Namespace.with_shared_runners_minutes_limit
-      .with_extra_shared_runners_minutes_limit
-      .with_shared_runners_minutes_exceeding_default_limit
-      .update_all("extra_shared_runners_minutes_limit = #{extra_minutes_left_sql} FROM namespace_statistics")
-
-    Namespace.with_ci_minutes_notification_sent.each_batch do |namespaces|
-      namespaces.update_all(last_ci_minutes_notification_at: nil, last_ci_minutes_usage_notification_level: nil)
-    end
-
-    Namespace.select(:id).each_batch do |namespaces|
-      Namespace.transaction do
-        reset_statistics(NamespaceStatistics, namespaces)
-        reset_statistics(ProjectStatistics, namespaces)
-      end
-    end
+    Namespace.reset_ci_minutes_in_batches!
   end
 
   private
 
-  # rubocop: disable CodeReuse/ActiveRecord
-  def reset_statistics(model, namespaces)
-    model.where(namespace: namespaces).where.not(shared_runners_seconds: 0).update_all(
-      shared_runners_seconds: 0,
-      shared_runners_seconds_last_reset: Time.now)
-  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)"
-  end
-
   def try_obtain_lease
     Gitlab::ExclusiveLease.new('gitlab_clear_shared_runners_minutes_worker',
       timeout: LEASE_TIMEOUT).try_obtain

ee/changelogs/unreleased/35878-possible-pipeline-minutes-usage-not-reset-for-2019-11-for-some-g.yml

+---
+title: Raise exception if any namespaces runner minutes were not reset
+merge_request: 22636
+author:
+type: added

ee/spec/models/ee/namespace_spec.rb

@@ -29,6 +29,101 @@ describe Namespace do
     end
   end
 
+  describe '.reset_ci_minutes_in_batches!' do
+    it 'returns when there were no failures' do
+      expect { described_class.reset_ci_minutes_in_batches! }.not_to raise_error
+    end
+
+    it 'raises an exception when with a list of namespace ids to investigate if there were any failures' do
+      failed_namespace = create(:namespace)
+
+      allow(described_class).to receive(:transaction).and_raise(ActiveRecord::ActiveRecordError)
+
+      expect { described_class.reset_ci_minutes_in_batches! }.to raise_error(
+        EE::Namespace::NamespaceStatisticsNotResetError,
+        "1 namespace shared runner minutes were not reset and the transaction was rolled back. Namespace Ids: [#{failed_namespace.id}]")
+    end
+  end
+
+  describe '.reset_ci_minutes!' do
+    it 'returns true if there were no exceptions to the db transaction' do
+      result = described_class.reset_ci_minutes!([])
+
+      expect(result).to be true
+    end
+
+    it 'raises an exception if anything in the transaction rolled back' do
+      namespace = create(:namespace)
+
+      allow(described_class).to receive(:transaction).and_raise(ActiveRecord::ActiveRecordError)
+
+      expect { described_class.reset_ci_minutes!([namespace.id]) }.to raise_error(
+        EE::Namespace::NamespaceStatisticsNotResetError,
+        "1 namespace shared runner minutes were not reset and the transaction was rolled back. Namespace Ids: [#{namespace.id}]")
+    end
+  end
+
+  describe '.recalculate_extra_shared_runners_minutes_limits!' do
+    context 'when the namespace had used runner minutes for the month' do
+      let(:namespace) { create(:namespace, shared_runners_minutes_limit: 5000, extra_shared_runners_minutes_limit: 5000) }
+
+      it 'updates the namespace extra_shared_runners_minutes_limit subtracting used minutes above the shared_runners_minutes_limit' do
+        minutes_used = 6000
+        create(:namespace_statistics, namespace: namespace, shared_runners_seconds: minutes_used * 60)
+
+        described_class.recalculate_extra_shared_runners_minutes_limits!([namespace.id])
+
+        expect(namespace.reload.extra_shared_runners_minutes_limit).to eq(4000)
+      end
+    end
+  end
+
+  describe '.reset_shared_runners_seconds!' do
+    let(:namespace) do
+      create(:namespace,
+        shared_runners_minutes_limit: 5000,
+        extra_shared_runners_minutes_limit: 5000)
+    end
+
+    subject do
+      described_class.reset_shared_runners_seconds!([namespace.id])
+    end
+
+    it 'resets NamespaceStatistics shared_runners_seconds and updates the timestamp' do
+      namespace_statistics = create(:namespace_statistics,
+        namespace: namespace,
+        shared_runners_seconds: 360000 )
+
+      expect { subject && namespace_statistics.reload }
+        .to change { namespace_statistics.shared_runners_seconds }.to(0)
+        .and change { namespace_statistics.shared_runners_seconds_last_reset }
+    end
+
+    it 'resets ProjectStatistics shared_runners_seconds and updates the timestamp' do
+      project_statistics = create(:project_statistics,
+        namespace: namespace,
+        shared_runners_seconds: 120)
+
+      expect { subject && project_statistics.reload }
+        .to change { project_statistics.shared_runners_seconds }.to(0)
+        .and change { project_statistics.shared_runners_seconds_last_reset }
+    end
+  end
+
+  describe 'reset_ci_minutes_notifications!' do
+    it 'updates the last_ci_minutes_notification_at and last_ci_minutes_usage_notification_level flags' do
+      namespace = create(:namespace,
+        last_ci_minutes_notification_at: Date.yesterday,
+        last_ci_minutes_usage_notification_level: 50 )
+
+      subject = described_class.reset_ci_minutes_notifications!([namespace.id])
+
+      expect { subject && namespace.reload }
+        .to change { namespace.last_ci_minutes_notification_at }.to(nil)
+        .and change { namespace.last_ci_minutes_usage_notification_level }.to(nil)
+    end
+  end
+
   describe '#use_elasticsearch?' do
     let(:namespace) { create :namespace }
 

ee/spec/workers/clear_shared_runners_minutes_worker_spec.rb

@@ -34,6 +34,21 @@ describe ClearSharedRunnersMinutesWorker do
 
         expect(statistics.reload.shared_runners_seconds_last_reset).to be_like_time(Time.now)
       end
+
+      context 'when there are namespaces that were not reset after the reset steps' do
+        let(:namespace_ids) { [namespace.id] }
+
+        before do
+          allow(Namespace).to receive(:each_batch).and_yield(Namespace.all)
+          allow(Namespace).to receive(:transaction).and_raise(ActiveRecord::ActiveRecordError)
+        end
+
+        it 'raises an exception' do
+          expect { worker.perform }.to raise_error(
+            EE::Namespace::NamespaceStatisticsNotResetError,
+            "#{namespace_ids.count} namespace shared runner minutes were not reset and the transaction was rolled back. Namespace Ids: #{namespace_ids}")
+        end
+      end
     end
 
     context 'when namespace statistics are defined' do