Merge branch '210570-change-calc-logic' into 'master'

Implement Shared Runner Minute Factors

See merge request gitlab-org/gitlab!27792

app/controllers/admin/runners_controller.rb

@@ -61,7 +61,15 @@ class Admin::RunnersController < Admin::ApplicationController
   end
 
   def runner_params
-    params.require(:runner).permit(Ci::Runner::FORM_EDITABLE)
+    params.require(:runner).permit(permitted_attrs)
+  end
+
+  def permitted_attrs
+    if Gitlab.com?
+      Ci::Runner::FORM_EDITABLE + Ci::Runner::MINUTES_COST_FACTOR_FIELDS
+    else
+      Ci::Runner::FORM_EDITABLE
+    end
   end
 
   # rubocop: disable CodeReuse/ActiveRecord

app/models/ci/runner.rb

@@ -35,6 +35,7 @@ module Ci
     AVAILABLE_SCOPES = (AVAILABLE_TYPES_LEGACY + AVAILABLE_TYPES + AVAILABLE_STATUSES).freeze
 
     FORM_EDITABLE = %i[description tag_list active run_untagged locked access_level maximum_timeout_human_readable].freeze
+    MINUTES_COST_FACTOR_FIELDS = %i[public_projects_minutes_cost_factor private_projects_minutes_cost_factor].freeze
 
     ignore_column :is_shared, remove_after: '2019-12-15', remove_with: '12.6'
 
@@ -137,6 +138,11 @@ module Ci
                                 numericality: { greater_than_or_equal_to: 600,
                                                 message: 'needs to be at least 10 minutes' }
 
+    validates :public_projects_minutes_cost_factor, :private_projects_minutes_cost_factor,
+      allow_nil: false,
+      numericality: { greater_than_or_equal_to: 0.0,
+                      message: 'needs to be non-negative' }
+
     # Searches for runners matching the given query.
     #
     # This method uses ILIKE on PostgreSQL and LIKE on MySQL.

app/views/admin/runners/show.html.haml

@@ -28,7 +28,7 @@
 %hr
 
 .append-bottom-20
-  = render 'shared/runners/form', runner: @runner, runner_form_url: admin_runner_path(@runner)
+  = render 'shared/runners/form', runner: @runner, runner_form_url: admin_runner_path(@runner), in_gitlab_com_admin_context: Gitlab.com?
 
 .row
   .col-md-6

app/views/shared/runners/_form.html.haml

@@ -47,5 +47,16 @@
     .col-sm-10
       = f.text_field :tag_list, value: runner.tag_list.sort.join(', '), class: 'form-control'
       .form-text.text-muted= _('You can set up jobs to only use Runners with specific tags. Separate tags with commas.')
+  - if local_assigns[:in_gitlab_com_admin_context]
+    .form-group.row
+      = label_tag :public_projects_minutes_cost_factor, class: 'col-form-label col-sm-2' do
+        = _('Public projects Minutes cost factor')
+      .col-sm-10
+        = f.text_field :public_projects_minutes_cost_factor, class: 'form-control'
+    .form-group.row
+      = label_tag :private_projects_minutes_cost_factor, class: 'col-form-label col-sm-2' do
+        = _('Private projects Minutes cost factor')
+      .col-sm-10
+        = f.text_field :private_projects_minutes_cost_factor, class: 'form-control'
   .form-actions
     = f.submit _('Save changes'), class: 'btn btn-success'

ee/app/models/ee/ci/build.rb

@@ -36,6 +36,14 @@ module EE
       end
 
       def shared_runners_minutes_limit_enabled?
+        if ::Feature.enabled?(:ci_minutes_enforce_quota_for_public_projects)
+          project.shared_runners_minutes_limit_enabled? && runner&.minutes_cost_factor(project.visibility_level)&.positive?
+        else
+          legacy_shared_runners_minutes_limit_enabled?
+        end
+      end
+
+      def legacy_shared_runners_minutes_limit_enabled?
         runner && runner.instance_type? && project.shared_runners_minutes_limit_enabled?
       end
 

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

@@ -8,6 +8,25 @@ module EE
 
         super
       end
+
+      def minutes_cost_factor(access_level)
+        return 0.0 unless instance_type?
+
+        case access_level
+        when ::Gitlab::VisibilityLevel::PUBLIC
+          public_projects_minutes_cost_factor
+        when ::Gitlab::VisibilityLevel::PRIVATE, ::Gitlab::VisibilityLevel::INTERNAL
+          private_projects_minutes_cost_factor
+        else
+          raise ArgumentError, 'Invalid visibility level'
+        end
+      end
+
+      def visibility_levels_without_minutes_quota
+        ::Gitlab::VisibilityLevel.options.values.reject do |visibility_level|
+          minutes_cost_factor(visibility_level).positive?
+        end
+      end
     end
   end
 end

ee/app/models/ee/project.rb

@@ -95,7 +95,13 @@ module EE
 
       has_many :sourced_pipelines, class_name: 'Ci::Sources::Project', foreign_key: :source_project_id
 
-      scope :with_shared_runners_limit_enabled, -> { with_shared_runners.non_public_only }
+      scope :with_shared_runners_limit_enabled, -> do
+        if ::Feature.enabled?(:ci_minutes_enforce_quota_for_public_projects)
+          with_shared_runners
+        else
+          with_shared_runners.non_public_only
+        end
+      end
 
       scope :mirror, -> { where(mirror: true) }
 
@@ -271,6 +277,15 @@ module EE
     end
 
     def shared_runners_minutes_limit_enabled?
+      if ::Feature.enabled?(:ci_minutes_enforce_quota_for_public_projects)
+        shared_runners_enabled? &&
+          shared_runners_limit_namespace.shared_runners_minutes_limit_enabled?
+      else
+        legacy_shared_runners_minutes_limit_enabled?
+      end
+    end
+
+    def legacy_shared_runners_minutes_limit_enabled?
       !public? && shared_runners_enabled? &&
         shared_runners_limit_namespace.shared_runners_minutes_limit_enabled?
     end

ee/app/services/ee/ci/register_job_service.rb

@@ -25,12 +25,31 @@ module EE
         end
       end
 
-      # rubocop: disable CodeReuse/ActiveRecord
       def builds_for_shared_runner
         return super unless shared_runner_build_limits_feature_enabled?
 
+        if ::Feature.enabled?(:ci_minutes_enforce_quota_for_public_projects)
+          enforce_minutes_based_on_cost_factors(super)
+        else
+          legacy_enforce_minutes_for_non_public_projects(super)
+        end
+      end
+
+      # rubocop: disable CodeReuse/ActiveRecord
+      def enforce_minutes_based_on_cost_factors(relation)
+        visibility_relation = ::Ci::Build.where(
+          projects: { visibility_level: runner.visibility_levels_without_minutes_quota })
+
+        enforce_limits_relation = ::Ci::Build.where('EXISTS (?)', builds_check_limit)
+
+        relation.merge(visibility_relation.or(enforce_limits_relation))
+      end
+      # rubocop: enable CodeReuse/ActiveRecord
+
+      # rubocop: disable CodeReuse/ActiveRecord
+      def legacy_enforce_minutes_for_non_public_projects(relation)
         # select projects which have allowed number of shared runner minutes or are public
-        super
+        relation
           .where("projects.visibility_level=? OR (#{builds_check_limit.to_sql})=1", # rubocop:disable GitlabSecurity/SqlInjection
                 ::Gitlab::VisibilityLevel::PUBLIC)
       end

ee/app/services/update_build_minutes_service.rb

@@ -4,7 +4,32 @@ class UpdateBuildMinutesService < BaseService
   def execute(build)
     return unless build.shared_runners_minutes_limit_enabled?
     return unless build.complete?
-    return unless build.duration
+    return unless build.duration&.positive?
+
+    if ::Feature.enabled?(:ci_minutes_track_for_public_projects, namespace)
+      count_projects_based_on_cost_factors(build)
+    else
+      legacy_count_non_public_projects(build)
+    end
+  end
+
+  private
+
+  def count_projects_based_on_cost_factors(build)
+    cost_factor = build.runner.minutes_cost_factor(project.visibility_level)
+    duration_with_cost_factor = (build.duration * cost_factor).to_i
+
+    return unless duration_with_cost_factor.positive?
+
+    ProjectStatistics.update_counters(project_statistics,
+      shared_runners_seconds: duration_with_cost_factor)
+
+    NamespaceStatistics.update_counters(namespace_statistics,
+      shared_runners_seconds: duration_with_cost_factor)
+  end
+
+  def legacy_count_non_public_projects(build)
+    return if project.public?
 
     ProjectStatistics.update_counters(project_statistics,
       shared_runners_seconds: build.duration)
@@ -13,8 +38,6 @@ class UpdateBuildMinutesService < BaseService
       shared_runners_seconds: build.duration)
   end
 
-  private
-
   def namespace_statistics
     namespace.namespace_statistics || namespace.create_namespace_statistics
   end

ee/changelogs/unreleased/210570-change-calc-logic.yml

+---
+title: Implement Shared Runner Minute Factors
+merge_request: 27792
+author:
+type: added

ee/spec/models/ci/build_spec.rb

@@ -41,11 +41,13 @@ describe Ci::Build do
   describe '#shared_runners_minutes_limit_enabled?' do
     subject { job.shared_runners_minutes_limit_enabled? }
 
+    shared_examples 'depends on runner presence and type' do
       context 'for shared runner' do
         before do
           job.runner = create(:ci_runner, :instance)
         end
 
+        context 'when project#shared_runners_minutes_limit_enabled? is true' do
           specify do
             expect(job.project).to receive(:shared_runners_minutes_limit_enabled?)
               .and_return(true)
@@ -54,6 +56,16 @@ describe Ci::Build do
           end
         end
 
+        context 'when project#shared_runners_minutes_limit_enabled? is false' do
+          specify do
+            expect(job.project).to receive(:shared_runners_minutes_limit_enabled?)
+              .and_return(false)
+
+            is_expected.to be_falsey
+          end
+        end
+      end
+
       context 'with specific runner' do
         before do
           job.runner = create(:ci_runner, :project)
@@ -67,6 +79,17 @@ describe Ci::Build do
       end
     end
 
+    it_behaves_like 'depends on runner presence and type'
+
+    context 'and :ci_minutes_enforce_quota_for_public_projects FF is disabled' do
+      before do
+        stub_feature_flags(ci_minutes_enforce_quota_for_public_projects: false)
+      end
+
+      it_behaves_like 'depends on runner presence and type'
+    end
+  end
+
   context 'updates pipeline minutes' do
     let(:job) { create(:ci_build, :running, pipeline: pipeline) }
 

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

@@ -18,4 +18,118 @@ describe EE::Ci::Runner do
       runner.tick_runner_queue
     end
   end
+
+  describe '#minutes_cost_factor' do
+    subject { runner.minutes_cost_factor(visibility_level) }
+
+    context 'with group type runner' do
+      let(:runner) { create(:ci_runner, :group) }
+
+      ::Gitlab::VisibilityLevel.options.each do |level_name, level_value|
+        context "with #{level_name}" do
+          let(:visibility_level) {level_value}
+
+          it { is_expected.to eq(0.0) }
+        end
+      end
+    end
+
+    context 'with project type runner' do
+      let(:runner) { create(:ci_runner, :project) }
+
+      ::Gitlab::VisibilityLevel.options.each do |level_name, level_value|
+        context "with #{level_name}" do
+          let(:visibility_level) {level_value}
+
+          it { is_expected.to eq(0.0) }
+        end
+      end
+    end
+
+    context 'with instance type runner' do
+      let(:runner) do
+        create(:ci_runner,
+               :instance,
+               private_projects_minutes_cost_factor: 1.1,
+               public_projects_minutes_cost_factor: 2.2)
+      end
+
+      context 'with private visibility level' do
+        let(:visibility_level) { ::Gitlab::VisibilityLevel::PRIVATE }
+
+        it { is_expected.to eq(1.1) }
+      end
+
+      context 'with public visibility level' do
+        let(:visibility_level) { ::Gitlab::VisibilityLevel::PUBLIC }
+
+        it { is_expected.to eq(2.2) }
+      end
+
+      context 'with internal visibility level' do
+        let(:visibility_level) { ::Gitlab::VisibilityLevel::INTERNAL }
+
+        it { is_expected.to eq(1.1) }
+      end
+
+      context 'with invalid visibility level' do
+        let(:visibility_level) { 123 }
+
+        it 'raises an error' do
+          expect { subject }.to raise_error(ArgumentError)
+        end
+      end
+    end
+  end
+
+  describe `#visibility_levels_without_minutes_quota` do
+    subject { runner.visibility_levels_without_minutes_quota }
+
+    context 'with group type runner' do
+      let(:runner) { create(:ci_runner, :group) }
+
+      it { is_expected.to match(::Gitlab::VisibilityLevel.options.values) }
+    end
+
+    context 'with project type runner' do
+      let(:runner) { create(:ci_runner, :project) }
+
+      it { is_expected.to match(::Gitlab::VisibilityLevel.options.values) }
+    end
+
+    context 'with instance type runner' do
+      context 'with both public and private cost factor being positive' do
+        let(:runner) do
+          create(:ci_runner,
+                :instance,
+                private_projects_minutes_cost_factor: 1.1,
+                public_projects_minutes_cost_factor: 2.2)
+        end
+
+        it { is_expected.to eq([]) }
+      end
+
+      context 'with both public and private cost factor being zero' do
+        let(:runner) do
+          create(:ci_runner,
+                :instance,
+                private_projects_minutes_cost_factor: 0.0,
+                public_projects_minutes_cost_factor: 0.0)
+        end
+
+        it { is_expected.to match(::Gitlab::VisibilityLevel.options.values) }
+      end
+
+      context 'with only private cost factor being positive' do
+        let(:runner) do
+          create(:ci_runner,
+                :instance,
+                private_projects_minutes_cost_factor: 1.0,
+                public_projects_minutes_cost_factor: 0.0)
+        end
+
+        it { is_expected.to match([::Gitlab::VisibilityLevel::PUBLIC]) }
+      end
+    end
+  end
 end

ee/spec/models/project_spec.rb

@@ -182,6 +182,42 @@ describe Project do
         expect(Project.find_by_service_desk_project_key('some_key')).to be_nil
       end
     end
+
+    describe '.with_shared_runners_limit_enabled' do
+      it 'does not return projects without shared runners' do
+        project_with_shared_runners = create(:project, shared_runners_enabled: true)
+        project_without_shared_runners = create(:project, shared_runners_enabled: false)
+
+        expect(described_class.with_shared_runners_limit_enabled).to include(project_with_shared_runners)
+        expect(described_class.with_shared_runners_limit_enabled).not_to include(project_without_shared_runners)
+      end
+
+      it 'return projects with shared runners with any visibility levels' do
+        public_project_with_shared_runners = create(:project, :public, shared_runners_enabled: true)
+        internal_project_with_shared_runners = create(:project, :internal, shared_runners_enabled: true)
+        private_project_with_shared_runners = create(:project, :private, shared_runners_enabled: true)
+
+        expect(described_class.with_shared_runners_limit_enabled).to include(public_project_with_shared_runners)
+        expect(described_class.with_shared_runners_limit_enabled).to include(internal_project_with_shared_runners)
+        expect(described_class.with_shared_runners_limit_enabled).to include(private_project_with_shared_runners)
+      end
+
+      context 'and :ci_minutes_enforce_quota_for_public_projects FF is disabled' do
+        before do
+          stub_feature_flags(ci_minutes_enforce_quota_for_public_projects: false)
+        end
+
+        it 'does not return public projects' do
+          public_project_with_shared_runners = create(:project, :public, shared_runners_enabled: true)
+          internal_project_with_shared_runners = create(:project, :internal, shared_runners_enabled: true)
+          private_project_with_shared_runners = create(:project, :private, shared_runners_enabled: true)
+
+          expect(described_class.with_shared_runners_limit_enabled).not_to include(public_project_with_shared_runners)
+          expect(described_class.with_shared_runners_limit_enabled).to include(internal_project_with_shared_runners)
+          expect(described_class.with_shared_runners_limit_enabled).to include(private_project_with_shared_runners)
+        end
+      end
+    end
   end
 
   describe 'validations' do
@@ -941,8 +977,16 @@ describe Project do
           project.visibility_level = Project::PUBLIC
         end
 
+        it { is_expected.to be_truthy }
+
+        context 'and :ci_minutes_enforce_quota_for_public_projects FF is disabled' do
+          before do
+            stub_feature_flags(ci_minutes_enforce_quota_for_public_projects: false)
+          end
+
           it { is_expected.to be_falsey }
         end
+      end
 
       context 'for internal project' do
         before do

ee/spec/services/ci/register_job_service_spec.rb

@@ -71,12 +71,31 @@ describe Ci::RegisterJobService do
           it_behaves_like 'does not return a build', 11
 
           context 'and project is public' do
+            context 'and public projects cost factor is 0 (default)' do
               before do
                 project.update(visibility_level: Project::PUBLIC)
               end
 
               it_behaves_like 'returns a build', 11
             end
+
+            context 'and public projects cost factor is > 0' do
+              before do
+                project.update(visibility_level: Project::PUBLIC)
+                shared_runner.update(public_projects_minutes_cost_factor: 1.1)
+              end
+
+              it_behaves_like 'does not return a build', 11
+
+              context 'and :ci_minutes_enforce_quota_for_public_projects FF is disabled' do
+                before do
+                  stub_feature_flags(ci_minutes_enforce_quota_for_public_projects: false)
+                end
+
+                it_behaves_like 'returns a build', 11
+              end
+            end
+          end
         end
 
         context 'and extra shared runners minutes purchased' do

ee/spec/services/update_build_minutes_service_spec.rb

@@ -5,7 +5,7 @@ require 'spec_helper'
 describe UpdateBuildMinutesService do
   describe '#perform' do
     let(:namespace) { create(:namespace, shared_runners_minutes_limit: 100) }
-    let(:project) { create(:project, namespace: namespace) }
+    let(:project) { create(:project, :public, namespace: namespace) }
     let(:pipeline) { create(:ci_pipeline, project: project) }
     let(:build) do
       create(:ci_build, :success,
@@ -16,16 +16,17 @@ describe UpdateBuildMinutesService do
     subject { described_class.new(project, nil).execute(build) }
 
     context 'with shared runner' do
-      let(:runner) { create(:ci_runner, :instance) }
+      let(:cost_factor) { 2.0 }
+      let(:runner) { create(:ci_runner, :instance, public_projects_minutes_cost_factor: cost_factor) }
 
-      it "creates a statistics and sets duration" do
+      it "creates a statistics and sets duration with applied cost factor" do
         subject
 
         expect(project.statistics.reload.shared_runners_seconds)
-          .to eq(build.duration.to_i)
+          .to eq(build.duration.to_i * 2)
 
         expect(namespace.namespace_statistics.reload.shared_runners_seconds)
-          .to eq(build.duration.to_i)
+          .to eq(build.duration.to_i * 2)
       end
 
       context 'when statistics are created' do
@@ -34,14 +35,14 @@ describe UpdateBuildMinutesService do
           namespace.create_namespace_statistics(shared_runners_seconds: 100)
         end
 
-        it "updates statistics and adds duration" do
+        it "updates statistics and adds duration with applied cost factor" do
           subject
 
           expect(project.statistics.reload.shared_runners_seconds)
-            .to eq(100 + build.duration.to_i)
+            .to eq(100 + build.duration.to_i * 2)
 
           expect(namespace.namespace_statistics.reload.shared_runners_seconds)
-            .to eq(100 + build.duration.to_i)
+            .to eq(100 + build.duration.to_i * 2)
         end
       end
 
@@ -53,7 +54,33 @@ describe UpdateBuildMinutesService do
           subject
 
           expect(root_ancestor.namespace_statistics.reload.shared_runners_seconds)
-            .to eq(build.duration.to_i)
+            .to eq(build.duration.to_i * 2)
+        end
+      end
+
+      context 'when cost factor has non-zero fractional part' do
+        let(:cost_factor) { 1.234 }
+
+        it 'truncates the result product value' do
+          subject
+
+          expect(project.statistics.reload.shared_runners_seconds)
+            .to eq((build.duration.to_i * 1.234).to_i)
+
+          expect(namespace.namespace_statistics.reload.shared_runners_seconds)
+            .to eq((build.duration.to_i * 1.234).to_i)
+        end
+      end
+
+      context 'when :ci_minutes_track_for_public_projects FF is disabled' do
+        before do
+          stub_feature_flags(ci_minutes_track_for_public_projects: false)
+        end
+
+        it "does not create/update statistics" do
+          subject
+
+          expect(namespace.namespace_statistics).to be_nil
         end
       end
     end

locale/gitlab.pot

@@ -15232,6 +15232,9 @@ msgstr ""
 msgid "Private profile"
 msgstr ""
 
+msgid "Private projects Minutes cost factor"
+msgstr ""
+
 msgid "Private projects can be created in your personal namespace with:"
 msgstr ""
 
@@ -16558,6 +16561,9 @@ msgstr ""
 msgid "Public pipelines"
 msgstr ""
 
+msgid "Public projects Minutes cost factor"
+msgstr ""
+
 msgid "Pull"
 msgstr ""
 

spec/controllers/admin/runners_controller_spec.rb

@@ -72,6 +72,30 @@ describe Admin::RunnersController do
 
       expect(response).to have_gitlab_http_status(:ok)
     end
+
+    describe 'Cost factors values' do
+      context 'when it is Gitlab.com' do
+        before do
+          expect(Gitlab).to receive(:com?).at_least(:once) { true }
+        end
+
+        it 'renders cost factors fields' do
+          get :show, params: { id: runner.id }
+
+          expect(response.body).to match /Private projects Minutes cost factor/
+          expect(response.body).to match /Public projects Minutes cost factor/
+        end
+      end
+
+      context 'when it is not Gitlab.com' do
+        it 'does not show cost factor fields' do
+          get :show, params: { id: runner.id }
+
+          expect(response.body).not_to match /Private projects Minutes cost factor/
+          expect(response.body).not_to match /Public projects Minutes cost factor/
+        end
+      end
+    end
   end
 
   describe '#update' do

spec/models/ci/runner_spec.rb

@@ -78,6 +78,36 @@ describe Ci::Runner do
           .to raise_error(ActiveRecord::RecordInvalid)
       end
     end
+
+    context 'cost factors validations' do
+      it 'dissalows :private_projects_minutes_cost_factor being nil' do
+        runner = build(:ci_runner, private_projects_minutes_cost_factor: nil)
+
+        expect(runner).to be_invalid
+        expect(runner.errors.full_messages).to include('Private projects minutes cost factor needs to be non-negative')
+      end
+
+      it 'dissalows :public_projects_minutes_cost_factor being nil' do
+        runner = build(:ci_runner, public_projects_minutes_cost_factor: nil)
+
+        expect(runner).to be_invalid
+        expect(runner.errors.full_messages).to include('Public projects minutes cost factor needs to be non-negative')
+      end
+
+      it 'dissalows :private_projects_minutes_cost_factor being negative' do
+        runner = build(:ci_runner, private_projects_minutes_cost_factor: -1.1)
+
+        expect(runner).to be_invalid
+        expect(runner.errors.full_messages).to include('Private projects minutes cost factor needs to be non-negative')
+      end
+
+      it 'dissalows :public_projects_minutes_cost_factor being negative' do
+        runner = build(:ci_runner, public_projects_minutes_cost_factor: -2.2)
+
+        expect(runner).to be_invalid
+        expect(runner.errors.full_messages).to include('Public projects minutes cost factor needs to be non-negative')
+      end
+    end
   end
 
   describe 'constraints' do

spec/services/ci/update_runner_service_spec.rb

@@ -23,6 +23,19 @@ describe Ci::UpdateRunnerService do
       end
     end
 
+    context 'with cost factor params' do
+      let(:params) { { public_projects_minutes_cost_factor: 1.1, private_projects_minutes_cost_factor: 2.2 }}
+
+      it 'updates the runner cost factors' do
+        expect(update).to be_truthy
+
+        runner.reload
+
+        expect(runner.public_projects_minutes_cost_factor).to eq(1.1)
+        expect(runner.private_projects_minutes_cost_factor).to eq(2.2)
+      end
+    end
+
     context 'when params are not valid' do
       let(:params) { { run_untagged: false } }