Merge branch 'ci-track-shared-runners-usage-duration' into 'master'

Track shared runners duration when job completes

See merge request gitlab-org/gitlab!72316

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

@@ -6,6 +6,22 @@ module Ci
       # Calculates consumption and updates the project and namespace statistics(legacy)
       # or ProjectMonthlyUsage and NamespaceMonthlyUsage(not legacy) based on the passed build.
       def execute(build)
+        if Feature.enabled?(:ci_always_track_shared_runners_usage, build.project, default_enabled: :yaml)
+          return unless build.complete?
+          return unless build.duration&.positive?
+          return unless build.shared_runner_build?
+
+          ci_minutes_consumed = ::Gitlab::Ci::Minutes::BuildConsumption.new(build, build.duration).amount
+
+          update_usage(build, ci_minutes_consumed)
+        else
+          legacy_update_minutes(build)
+        end
+      end
+
+      private
+
+      def legacy_update_minutes(build)
         return unless build.cost_factor_enabled?
         return unless build.complete?
         return unless build.duration&.positive?
@@ -14,13 +30,12 @@ module Ci
 
         return unless consumption > 0
 
-        update_minutes(consumption, build)
+        ::Ci::Minutes::UpdateProjectAndNamespaceUsageWorker.perform_async(consumption, project.id, namespace.id, build.id)
       end
 
-      private
-
-      def update_minutes(consumption, build)
-        ::Ci::Minutes::UpdateProjectAndNamespaceUsageWorker.perform_async(consumption, project.id, namespace.id, build.id)
+      def update_usage(build, ci_minutes_consumed)
+        ::Ci::Minutes::UpdateProjectAndNamespaceUsageWorker
+          .perform_async(ci_minutes_consumed, project.id, namespace.id, build.id, { duration: build.duration })
       end
 
       def namespace

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

@@ -21,7 +21,8 @@ module Ci
 
         ensure_idempotency { track_monthly_usage(consumption, duration.to_i) }
 
-        send_minutes_email_notification
+        # No need to check notification if consumption hasn't changed
+        send_minutes_email_notification if consumption > 0
       end
 
       def idempotency_cache_key
@@ -47,6 +48,8 @@ module Ci
       end
 
       def legacy_track_usage_of_monthly_minutes(consumption)
+        return unless consumption > 0
+
         consumption_in_seconds = consumption.minutes.to_i
 
         update_legacy_project_minutes(consumption_in_seconds)

ee/app/workers/ci/minutes/update_project_and_namespace_usage_worker.rb

@@ -16,7 +16,7 @@ module Ci
       def perform(consumption, project_id, namespace_id, build_id, params = {})
         ::Ci::Minutes::UpdateProjectAndNamespaceUsageService
           .new(project_id, namespace_id, build_id)
-          .execute(consumption, params[:duration].to_i)
+          .execute(consumption, params['duration'].to_i)
       end
     end
   end

ee/config/feature_flags/development/ci_always_track_shared_runners_usage.yml

+---
+name: ci_always_track_shared_runners_usage
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/72316
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/343258
+milestone: '14.4'
+type: development
+group: group::pipeline execution
+default_enabled: false

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

@@ -5,34 +5,23 @@ require 'spec_helper'
 RSpec.describe Ci::Minutes::UpdateBuildMinutesService do
   include ::Ci::MinutesHelpers
 
-  let(:namespace) { create(:namespace, shared_runners_minutes_limit: 100) }
+  let(:namespace) { create(:group, shared_runners_minutes_limit: 100) }
   let(:project) { create(:project, :private, namespace: namespace) }
   let(:pipeline) { create(:ci_pipeline, project: project) }
 
   let(:build) do
+    build_created_at = 2.hours.ago
     create(:ci_build, :success,
       runner: runner, pipeline: pipeline,
-      started_at: 2.hours.ago, finished_at: 1.hour.ago)
+      started_at: build_created_at, finished_at: build_created_at + 1.hour)
   end
 
-  let(:namespace_amount_used) { Ci::Minutes::NamespaceMonthlyUsage.find_or_create_current(namespace_id: namespace.id).amount_used }
-  let(:project_amount_used) { Ci::Minutes::ProjectMonthlyUsage.find_or_create_current(project_id: project.id).amount_used }
+  let(:namespace_usage) { Ci::Minutes::NamespaceMonthlyUsage.find_or_create_current(namespace_id: namespace.id) }
+  let(:project_usage) { Ci::Minutes::ProjectMonthlyUsage.find_or_create_current(project_id: project.id) }
 
   subject { described_class.new(project, nil).execute(build) }
 
   describe '#execute', :sidekiq_inline do
-    shared_examples 'new tracking matches legacy tracking' do
-      it 'stores the same information in both legacy and new tracking' do
-        subject
-
-        expect(namespace_amount_used)
-          .to eq((namespace.reload.namespace_statistics.shared_runners_seconds.to_f / 60).round(2))
-
-        expect(project_amount_used)
-          .to eq((project.statistics.reload.shared_runners_seconds.to_f / 60).round(2))
-      end
-    end
-
     shared_examples 'does nothing' do
       it 'does not update legacy statistics' do
         subject
@@ -48,44 +37,47 @@ RSpec.describe Ci::Minutes::UpdateBuildMinutesService do
       it 'does not update project monthly usage' do
         expect { subject }.not_to change { Ci::Minutes::ProjectMonthlyUsage.count }
       end
+    end
 
-      it 'does not send an email' do
-        allow(Gitlab).to receive(:com?).and_return(true)
+    shared_examples 'updates usage' do |expected_ci_minutes, expected_shared_runners_duration|
+      it 'updates legacy statistics', :aggregate_failures do
+        subject
 
-        expect(Ci::Minutes::EmailNotificationService).not_to receive(:new)
+        expect(project.statistics.reload.shared_runners_seconds)
+          .to eq(expected_ci_minutes * 60)
 
-        subject
+        expect(namespace.namespace_statistics.reload.shared_runners_seconds)
+          .to eq(expected_ci_minutes * 60)
       end
-    end
-
-    context 'with shared runner' do
-      let(:cost_factor) { 2.0 }
-      let(:runner) { create(:ci_runner, :instance, private_projects_minutes_cost_factor: cost_factor) }
 
-      it 'creates a statistics and sets duration with applied cost factor' do
+      it 'stores the same information in both legacy and new tracking' do
         subject
 
-        expect(project.statistics.reload.shared_runners_seconds)
-          .to eq(build.duration.to_i * 2)
+        expect(namespace_usage.amount_used.to_f)
+          .to eq((namespace.reload.namespace_statistics.shared_runners_seconds.to_f / 60).round(2))
 
-        expect(namespace.namespace_statistics.reload.shared_runners_seconds)
-          .to eq(build.duration.to_i * 2)
+        expect(project_usage.amount_used.to_f)
+          .to eq((project.statistics.reload.shared_runners_seconds.to_f / 60).round(2))
       end
 
-      it 'tracks the usage on a monthly basis' do
+      it 'tracks the usage on a monthly basis', :aggregate_failures do
         subject
 
-        expect(namespace_amount_used).to eq((60 * 2).to_f)
-        expect(project_amount_used).to eq((60 * 2).to_f)
+        expect(namespace_usage.amount_used.to_f).to eq(expected_ci_minutes)
+        expect(namespace_usage.shared_runners_duration).to eq(expected_shared_runners_duration)
+
+        expect(project_usage.amount_used.to_f).to eq(expected_ci_minutes)
+        expect(project_usage.shared_runners_duration).to eq(expected_shared_runners_duration)
       end
+    end
 
-      it_behaves_like 'new tracking matches legacy tracking'
+    context 'with shared runner' do
+      let(:cost_factor) { 2.0 }
+      let(:runner) { create(:ci_runner, :instance, private_projects_minutes_cost_factor: cost_factor) }
 
-      context 'when on .com' do
-        before do
-          allow(Gitlab).to receive(:com?).and_return(true)
-        end
+      it_behaves_like 'updates usage', 120, 1.hour
 
+      context 'when on .com', :saas do
         it 'sends an email' do
           expect_next_instance_of(Ci::Minutes::EmailNotificationService) do |service|
             expect(service).to receive(:execute)
@@ -107,95 +99,68 @@ RSpec.describe Ci::Minutes::UpdateBuildMinutesService do
         end
       end
 
-      context 'when consumption is 0' do
-        let(:build) do
-          create(:ci_build, :success,
-            runner: runner, pipeline: pipeline,
-            started_at: Time.current, finished_at: Time.current)
-        end
-
-        it_behaves_like 'does nothing'
-      end
-
-      context 'when statistics and usage have existing amounts' do
-        let(:usage_in_seconds) { 100 }
-        let(:usage_in_minutes) { (100.to_f / 60).round(2) }
-
+      context 'when consumption is 0', :saas do
         before do
-          set_ci_minutes_used(namespace, usage_in_minutes)
-
-          project.statistics.update!(shared_runners_seconds: usage_in_seconds)
-          create(:ci_project_monthly_usage, project: project, amount_used: usage_in_minutes)
-        end
-
-        it 'updates statistics and adds duration with applied cost factor' do
-          subject
-
-          expect(project.statistics.reload.shared_runners_seconds)
-            .to eq(usage_in_seconds + build.duration.to_i * 2)
-
-          expect(namespace.namespace_statistics.reload.shared_runners_seconds)
-            .to eq(usage_in_seconds + build.duration.to_i * 2)
+          allow_next_instance_of(::Gitlab::Ci::Minutes::BuildConsumption) do |consumption|
+            allow(consumption).to receive(:amount).and_return(0)
+          end
         end
 
-        it 'tracks the usage on a monthly basis' do
+        it 'does not update legacy statistics' do
           subject
 
-          expect(namespace_amount_used).to eq(usage_in_minutes + 60 * 2)
-          expect(project_amount_used).to eq(usage_in_minutes + 60 * 2)
+          expect(project.statistics.reload.shared_runners_seconds).to eq(0)
+          expect(namespace.namespace_statistics).to be_nil
         end
 
-        it_behaves_like 'new tracking matches legacy tracking'
-      end
-
-      context 'when group is subgroup' do
-        let(:root_ancestor) { create(:group, shared_runners_minutes_limit: 100) }
-        let(:namespace) { create(:group, parent: root_ancestor) }
-
-        let(:namespace_amount_used) { Ci::Minutes::NamespaceMonthlyUsage.find_or_create_current(namespace_id: root_ancestor.id).amount_used }
+        it 'updates only the shared runners duration' do
+          expect { subject }.to change { Ci::Minutes::NamespaceMonthlyUsage.count }
 
-        it 'creates a statistics in root group' do
-          subject
+          expect(namespace_usage.amount_used).to eq(0)
+          expect(namespace_usage.shared_runners_duration).to eq(build.duration)
 
-          expect(root_ancestor.namespace_statistics.reload.shared_runners_seconds)
-            .to eq(build.duration.to_i * 2)
+          expect(project_usage.amount_used).to eq(0)
+          expect(project_usage.shared_runners_duration).to eq(build.duration)
         end
 
-        it 'tracks the usage on a monthly basis' do
-          subject
-
-          expect(namespace_amount_used).to eq(60 * 2)
-          expect(project_amount_used).to eq(60 * 2)
-        end
+        it 'does not send an email' do
+          expect(Ci::Minutes::EmailNotificationService).not_to receive(:new)
 
-        it 'stores the same information in both legacy and new tracking' do
           subject
+        end
 
-          expect(namespace_amount_used)
-            .to eq((root_ancestor.namespace_statistics.reload.shared_runners_seconds.to_f / 60).round(2))
+        context 'when feature flag ci_always_track_shared_runners_usage is disabled' do
+          before do
+            stub_feature_flags(ci_always_track_shared_runners_usage: false)
+          end
 
-          expect(project_amount_used)
-            .to eq((project.statistics.reload.shared_runners_seconds.to_f / 60).round(2))
+          it_behaves_like 'does nothing'
         end
       end
 
-      context 'when live tracking exists for the build', :redis do
-        before do
-          allow(Gitlab).to receive(:com?).and_return(true)
+      context 'when statistics and usage have existing amounts' do
+        let(:existing_ci_minutes) { 100 }
+        let(:existing_shared_runners_duration) { 200 }
 
-          build.update!(status: :running)
+        before do
+          set_ci_minutes_used(namespace, existing_ci_minutes, existing_shared_runners_duration)
 
-          freeze_time do
-            ::Ci::Minutes::TrackLiveConsumptionService.new(build).tap do |service|
-              service.time_last_tracked_consumption!((build.duration.to_i - 5.minutes).ago)
-              service.execute
-            end
-          end
+          project.statistics.update!(shared_runners_seconds: existing_ci_minutes * 60)
 
-          build.update!(status: :success)
+          create(:ci_project_monthly_usage,
+            project: project,
+            amount_used: existing_ci_minutes,
+            shared_runners_duration: existing_shared_runners_duration)
         end
 
-        it_behaves_like 'new tracking matches legacy tracking'
+        it_behaves_like 'updates usage', 220, 200 + 1.hour
+      end
+
+      context 'when group is subgroup' do
+        let(:subgroup) { create(:group, parent: namespace) }
+        let(:project) { create(:project, :private, group: subgroup) }
+
+        it_behaves_like 'updates usage', 120, 1.hour
       end
     end
 

ee/spec/support/ci/minutes_helpers.rb

@@ -3,7 +3,7 @@
 module Ci
   module MinutesHelpers
     # TODO: Remove with https://gitlab.com/gitlab-org/gitlab/-/issues/277452
-    def set_ci_minutes_used(namespace, minutes)
+    def set_ci_minutes_used(namespace, minutes, shared_runners_duration = 0)
       if namespace.namespace_statistics
         namespace.namespace_statistics.update!(shared_runners_seconds: minutes.minutes)
       else
@@ -12,7 +12,7 @@ module Ci
 
       ::Ci::Minutes::NamespaceMonthlyUsage
         .find_or_create_current(namespace_id: namespace.id)
-        .update!(amount_used: minutes)
+        .update!(amount_used: minutes, shared_runners_duration: shared_runners_duration)
     end
   end
 end

ee/spec/workers/ci/minutes/update_project_and_namespace_usage_worker_spec.rb

@@ -25,7 +25,7 @@ RSpec.describe Ci::Minutes::UpdateProjectAndNamespaceUsageWorker do
           subject
         end
 
-        it 'updates monthly usage but not shared_runners_duration' do
+        it 'updates monthly usage but not shared_runners_duration', :aggregate_failures do
           subject
 
           namespace_usage = Ci::Minutes::NamespaceMonthlyUsage.find_by(namespace: namespace)
@@ -38,7 +38,7 @@ RSpec.describe Ci::Minutes::UpdateProjectAndNamespaceUsageWorker do
         end
       end
 
-      it 'does not behave idempotently for legacy statistics update' do
+      it 'does not behave idempotently for legacy statistics update', :aggregate_failures do
         expect(::Ci::Minutes::UpdateProjectAndNamespaceUsageService).to receive(:new).twice.and_call_original
 
         subject
@@ -49,7 +49,7 @@ RSpec.describe Ci::Minutes::UpdateProjectAndNamespaceUsageWorker do
     end
 
     context 'when duration param is passed in' do
-      subject { perform_multiple([consumption, project.id, namespace.id, build.id, { duration: duration }]) }
+      subject { perform_multiple([consumption, project.id, namespace.id, build.id, { 'duration' => duration }]) }
 
       context 'behaves idempotently for monthly usage update' do
         it 'executes UpdateProjectAndNamespaceUsageService' do
@@ -60,7 +60,7 @@ RSpec.describe Ci::Minutes::UpdateProjectAndNamespaceUsageWorker do
           subject
         end
 
-        it 'updates monthly usage and shared_runners_duration' do
+        it 'updates monthly usage and shared_runners_duration', :aggregate_failures do
           subject
 
           namespace_usage = Ci::Minutes::NamespaceMonthlyUsage.find_by(namespace: namespace)
@@ -73,7 +73,7 @@ RSpec.describe Ci::Minutes::UpdateProjectAndNamespaceUsageWorker do
         end
       end
 
-      it 'does not behave idempotently for legacy statistics update' do
+      it 'does not behave idempotently for legacy statistics update', :aggregate_failures do
         expect(::Ci::Minutes::UpdateProjectAndNamespaceUsageService).to receive(:new).twice.and_call_original
 
         subject