Merge branch 'pedropombeiro/321368/account-only-recent-runners' into 'master'

Account only recent runners in plan limit for registered runners [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!62670

app/models/ci/runner.rb

@@ -60,6 +60,7 @@ module Ci
     scope :active, -> { where(active: true) }
     scope :paused, -> { where(active: false) }
     scope :online, -> { where('contacted_at > ?', online_contact_time_deadline) }
+    scope :recent, -> { where('ci_runners.created_at > :date OR ci_runners.contacted_at > :date', date: 3.months.ago) }
     # The following query using negation is cheaper than using `contacted_at <= ?`
     # because there are less runners online than have been created. The
     # resulting query is quickly finding online ones and then uses the regular

app/models/ci/runner_namespace.rb

@@ -7,6 +7,7 @@ module Ci
 
     self.limit_name = 'ci_registered_group_runners'
     self.limit_scope = :group
+    self.limit_relation = :recent_runners
     self.limit_feature_flag = :ci_runner_limits
 
     belongs_to :runner, inverse_of: :runner_namespaces
@@ -16,6 +17,10 @@ module Ci
     validates :runner_id, uniqueness: { scope: :namespace_id }
     validate :group_runner_type
 
+    def recent_runners
+      ::Ci::Runner.belonging_to_group(namespace_id).recent
+    end
+
     private
 
     def group_runner_type

app/models/ci/runner_project.rb

@@ -7,11 +7,16 @@ module Ci
 
     self.limit_name = 'ci_registered_project_runners'
     self.limit_scope = :project
+    self.limit_relation = :recent_runners
     self.limit_feature_flag = :ci_runner_limits
 
     belongs_to :runner, inverse_of: :runner_projects
     belongs_to :project, inverse_of: :runner_projects
 
+    def recent_runners
+      ::Ci::Runner.belonging_to_project(project_id).recent
+    end
+
     validates :runner_id, uniqueness: { scope: :project_id }
   end
 end

app/models/concerns/limitable.rb

@@ -6,6 +6,7 @@ module Limitable
 
   included do
     class_attribute :limit_scope
+    class_attribute :limit_relation
     class_attribute :limit_name
     class_attribute :limit_feature_flag
     self.limit_name = self.name.demodulize.tableize
@@ -28,7 +29,7 @@ module Limitable
     return unless scope_relation
     return if limit_feature_flag && ::Feature.disabled?(limit_feature_flag, scope_relation, default_enabled: :yaml)
 
-    relation = self.class.where(limit_scope => scope_relation)
+    relation = limit_relation ? self.public_send(limit_relation) : self.class.where(limit_scope => scope_relation) # rubocop:disable GitlabSecurity/PublicSend
     limits = scope_relation.actual_limits
 
     check_plan_limit_not_exceeded(limits, relation)

doc/administration/instance_limits.md

@@ -482,10 +482,9 @@ Plan.default.actual_limits.update!(ci_max_artifact_size_junit: 10)
 
 > [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/321368) in GitLab 13.12.
 
-The total number of registered runners is limited at the group and project
-levels. Each time a new runner is registered, GitLab checks these limits. A
-runner's registration fails if it exceeds the limit for the scope determined by
-the runner registration token.
+The total number of registered runners is limited at the group and project levels. Each time a new runner is registered,
+GitLab checks these limits against runners that have been active in the last 3 months.
+A runner's registration fails if it exceeds the limit for the scope determined by the runner registration token.
 
 - GitLab SaaS subscribers have different limits defined per plan, affecting all projects using that plan.
 - Self-managed GitLab Premium and Ultimate limits are defined by a default plan that affects all projects:

spec/models/ci/runner_spec.rb

@@ -273,6 +273,20 @@ RSpec.describe Ci::Runner do
     end
   end
 
+  describe '.recent' do
+    subject { described_class.recent }
+
+    before do
+      @runner1 = create(:ci_runner, :instance, contacted_at: nil, created_at: 2.months.ago)
+      @runner2 = create(:ci_runner, :instance, contacted_at: nil, created_at: 3.months.ago)
+      @runner3 = create(:ci_runner, :instance, contacted_at: 1.month.ago, created_at: 2.months.ago)
+      @runner4 = create(:ci_runner, :instance, contacted_at: 1.month.ago, created_at: 3.months.ago)
+      @runner5 = create(:ci_runner, :instance, contacted_at: 3.months.ago, created_at: 5.months.ago)
+    end
+
+    it { is_expected.to eq([@runner1, @runner3, @runner4])}
+  end
+
   describe '.online' do
     subject { described_class.online }
 

spec/models/concerns/limitable_spec.rb

 # frozen_string_literal: true
 
-require 'spec_helper'
+require 'fast_spec_helper'
+require 'active_model'
 
 RSpec.describe Limitable do
   let(:minimal_test_class) do
@@ -35,6 +36,28 @@ RSpec.describe Limitable do
 
       instance.valid?(:create)
     end
+
+    context 'with custom relation' do
+      before do
+        MinimalTestClass.limit_relation = :custom_relation
+      end
+
+      it 'triggers custom limit_relation' do
+        instance = MinimalTestClass.new
+
+        def instance.project
+          @project ||= Object.new
+        end
+
+        limits = Object.new
+        custom_relation = Object.new
+        expect(instance).to receive(:custom_relation).and_return(custom_relation)
+        expect(instance.project).to receive(:actual_limits).and_return(limits)
+        expect(limits).to receive(:exceeded?).with(instance.class.name.demodulize.tableize, custom_relation).and_return(false)
+
+        instance.valid?(:create)
+      end
+    end
   end
 
   context 'with global limit' do

spec/requests/api/ci/runner/runners_post_spec.rb

@@ -94,7 +94,7 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
 
           context 'when it exceeds the application limits' do
             before do
-              create(:ci_runner, runner_type: :project_type, projects: [project])
+              create(:ci_runner, runner_type: :project_type, projects: [project], contacted_at: 1.second.ago)
               create(:plan_limits, :default_plan, ci_registered_project_runners: 1)
             end
 
@@ -106,6 +106,22 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
               expect(project.runners.reload.size).to eq(1)
             end
           end
+
+          context 'when abandoned runners cause application limits to not be exceeded' do
+            before do
+              create(:ci_runner, runner_type: :project_type, projects: [project], created_at: 14.months.ago, contacted_at: 13.months.ago)
+              create(:plan_limits, :default_plan, ci_registered_project_runners: 1)
+            end
+
+            it 'creates runner' do
+              request
+
+              expect(response).to have_gitlab_http_status(:created)
+              expect(json_response['message']).to be_nil
+              expect(project.runners.reload.size).to eq(2)
+              expect(project.runners.recent.size).to eq(1)
+            end
+          end
         end
 
         context 'when group token is used' do
@@ -135,7 +151,7 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
 
           context 'when it exceeds the application limits' do
             before do
-              create(:ci_runner, runner_type: :group_type, groups: [group])
+              create(:ci_runner, runner_type: :group_type, groups: [group], contacted_at: nil, created_at: 1.month.ago)
               create(:plan_limits, :default_plan, ci_registered_group_runners: 1)
             end
 
@@ -147,6 +163,23 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
               expect(group.runners.reload.size).to eq(1)
             end
           end
+
+          context 'when abandoned runners cause application limits to not be exceeded' do
+            before do
+              create(:ci_runner, runner_type: :group_type, groups: [group], created_at: 4.months.ago, contacted_at: 3.months.ago)
+              create(:ci_runner, runner_type: :group_type, groups: [group], contacted_at: nil, created_at: 4.months.ago)
+              create(:plan_limits, :default_plan, ci_registered_group_runners: 1)
+            end
+
+            it 'creates runner' do
+              request
+
+              expect(response).to have_gitlab_http_status(:created)
+              expect(json_response['message']).to be_nil
+              expect(group.runners.reload.size).to eq(3)
+              expect(group.runners.recent.size).to eq(1)
+            end
+          end
         end
       end