Merge branch '323066-limit-for-scheduled-pipelines' into 'master'

Add limit for daily scheduled pipelines for projects [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!62826

app/models/ci/pipeline_schedule.rb

@@ -3,6 +3,7 @@
 module Ci
   class PipelineSchedule < ApplicationRecord
     extend Gitlab::Ci::Model
+    extend ::Gitlab::Utils::Override
     include Importable
     include StripAttribute
     include CronSchedulable
@@ -55,6 +56,13 @@ module Ci
       variables&.map(&:to_runner_variable) || []
     end
 
+    override :set_next_run_at
+    def set_next_run_at
+      self.next_run_at = ::Ci::PipelineSchedules::CalculateNextRunService # rubocop: disable CodeReuse/ServiceClass
+                           .new(project)
+                           .execute(self, fallback_method: method(:calculate_next_run_at))
+    end
+
     private
 
     def worker_cron_expression

app/models/concerns/cron_schedulable.rb

@@ -4,23 +4,28 @@ module CronSchedulable
   extend ActiveSupport::Concern
   include Schedulable
 
+  def set_next_run_at
+    self.next_run_at = calculate_next_run_at
+  end
+
+  private
+
   ##
   # The `next_run_at` column is set to the actual execution date of worker that
   # triggers the schedule. This way, a schedule like `*/1 * * * *` won't be triggered
   # in a short interval when the worker runs irregularly by Sidekiq Memory Killer.
-  def set_next_run_at
+  def calculate_next_run_at
     now = Time.zone.now
+
     ideal_next_run = ideal_next_run_from(now)
 
-    self.next_run_at = if ideal_next_run == cron_worker_next_run_from(now)
-                         ideal_next_run
-                       else
-                         cron_worker_next_run_from(ideal_next_run)
-                       end
+    if ideal_next_run == cron_worker_next_run_from(now)
+      ideal_next_run
+    else
+      cron_worker_next_run_from(ideal_next_run)
+    end
   end
 
-  private
-
   def ideal_next_run_from(start_time)
     next_time_from(start_time, cron, cron_timezone)
   end

app/services/ci/pipeline_schedules/calculate_next_run_service.rb

+# frozen_string_literal: true
+
+module Ci
+  module PipelineSchedules
+    class CalculateNextRunService < BaseService
+      include Gitlab::Utils::StrongMemoize
+
+      def execute(schedule, fallback_method:)
+        @schedule = schedule
+
+        return fallback_method.call unless ::Feature.enabled?(:ci_daily_limit_for_pipeline_schedules, project, default_enabled: :yaml)
+        return fallback_method.call unless plan_cron&.cron_valid?
+
+        now = Time.zone.now
+
+        schedule_next_run = schedule_cron.next_time_from(now)
+        return schedule_next_run if worker_cron.match?(schedule_next_run) && plan_cron.match?(schedule_next_run)
+
+        plan_next_run = plan_cron.next_time_from(now)
+        return plan_next_run if worker_cron.match?(plan_next_run)
+
+        worker_next_run = worker_cron.next_time_from(now)
+        return worker_next_run if plan_cron.match?(worker_next_run)
+
+        worker_cron.next_time_from(plan_next_run)
+      end
+
+      private
+
+      def schedule_cron
+        strong_memoize(:schedule_cron) do
+          Gitlab::Ci::CronParser.new(@schedule.cron, @schedule.cron_timezone)
+        end
+      end
+
+      def worker_cron
+        strong_memoize(:worker_cron) do
+          Gitlab::Ci::CronParser.new(worker_cron_expression, Time.zone.name)
+        end
+      end
+
+      def plan_cron
+        strong_memoize(:plan_cron) do
+          daily_scheduled_pipeline_limit = project.actual_limits.limit_for(:ci_daily_pipeline_schedule_triggers)
+
+          next unless daily_scheduled_pipeline_limit
+
+          every_x_minutes = (1.day.in_minutes / daily_scheduled_pipeline_limit).to_i
+
+          Gitlab::Ci::CronParser.parse_natural("every #{every_x_minutes} minutes", Time.zone.name)
+        end
+      end
+
+      def worker_cron_expression
+        Settings.cron_jobs['pipeline_schedule_worker']['cron']
+      end
+    end
+  end
+end

config/feature_flags/development/ci_daily_limit_for_pipeline_schedules.yml

+---
+name: ci_daily_limit_for_pipeline_schedules
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/62826
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/332333
+milestone: '14.0'
+type: development
+group: group::pipeline authoring
+default_enabled: false

db/migrate/20210526190259_add_ci_daily_pipeline_schedule_triggers_to_plan_limits.rb

+# frozen_string_literal: true
+
+class AddCiDailyPipelineScheduleTriggersToPlanLimits < ActiveRecord::Migration[6.0]
+  def change
+    add_column(:plan_limits, :ci_daily_pipeline_schedule_triggers, :integer, default: 0, null: false)
+  end
+end

db/migrate/20210526190553_insert_ci_daily_pipeline_schedule_triggers_plan_limits.rb

+# frozen_string_literal: true
+
+class InsertCiDailyPipelineScheduleTriggersPlanLimits < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  EVERY_5_MINUTES = (1.day.in_minutes / 5).to_i
+  EVERY_HOUR = 1.day.in_hours.to_i
+
+  def up
+    return unless Gitlab.com?
+
+    create_or_update_plan_limit('ci_daily_pipeline_schedule_triggers', 'free', EVERY_HOUR)
+    create_or_update_plan_limit('ci_daily_pipeline_schedule_triggers', 'bronze', EVERY_5_MINUTES)
+    create_or_update_plan_limit('ci_daily_pipeline_schedule_triggers', 'silver', EVERY_5_MINUTES)
+    create_or_update_plan_limit('ci_daily_pipeline_schedule_triggers', 'gold', EVERY_5_MINUTES)
+  end
+
+  def down
+    return unless Gitlab.com?
+
+    create_or_update_plan_limit('ci_daily_pipeline_schedule_triggers', 'free', 0)
+    create_or_update_plan_limit('ci_daily_pipeline_schedule_triggers', 'bronze', 0)
+    create_or_update_plan_limit('ci_daily_pipeline_schedule_triggers', 'silver', 0)
+    create_or_update_plan_limit('ci_daily_pipeline_schedule_triggers', 'gold', 0)
+  end
+end

db/schema_migrations/20210526190259

+ae2829a06f02ff3e1adc977f5e789b17d1f760e6aaa40be44586cc6a90870c4a
\ No newline at end of file

db/schema_migrations/20210526190553

+824e0930de14587f6ccaeb6b5fbec16676d243550a2dfd3a5999b67dfc16d4c8
\ No newline at end of file

db/structure.sql

@@ -16242,7 +16242,8 @@ CREATE TABLE plan_limits (
     helm_max_file_size bigint DEFAULT 5242880 NOT NULL,
     ci_registered_group_runners integer DEFAULT 1000 NOT NULL,
     ci_registered_project_runners integer DEFAULT 1000 NOT NULL,
-    web_hook_calls integer DEFAULT 0 NOT NULL
+    web_hook_calls integer DEFAULT 0 NOT NULL,
+    ci_daily_pipeline_schedule_triggers integer DEFAULT 0 NOT NULL
 );
 
 CREATE SEQUENCE plan_limits_id_seq

lib/gitlab/ci/cron_parser.rb

@@ -6,6 +6,10 @@ module Gitlab
       VALID_SYNTAX_SAMPLE_TIME_ZONE = 'UTC'
       VALID_SYNTAX_SAMPLE_CRON = '* * * * *'
 
+      def self.parse_natural(expression, cron_timezone = 'UTC')
+        new(Fugit::Nat.parse(expression)&.original, cron_timezone)
+      end
+
       def initialize(cron, cron_timezone = 'UTC')
         @cron = cron
         @cron_timezone = timezone_name(cron_timezone)
@@ -27,6 +31,10 @@ module Gitlab
         try_parse_cron(VALID_SYNTAX_SAMPLE_CRON, @cron_timezone).present?
       end
 
+      def match?(time)
+        cron_line.match?(time)
+      end
+
       private
 
       def timezone_name(timezone)

spec/migrations/insert_ci_daily_pipeline_schedule_triggers_plan_limits_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'migrate', '20210526190553_insert_ci_daily_pipeline_schedule_triggers_plan_limits.rb')
+
+RSpec.describe InsertCiDailyPipelineScheduleTriggersPlanLimits do
+  let_it_be(:plans) { table(:plans) }
+  let_it_be(:plan_limits) { table(:plan_limits) }
+
+  context 'when on Gitlab.com' do
+    let(:free_plan) { plans.create!(name: 'free') }
+    let(:bronze_plan) { plans.create!(name: 'bronze') }
+    let(:silver_plan) { plans.create!(name: 'silver') }
+    let(:gold_plan) { plans.create!(name: 'gold') }
+
+    before do
+      allow(Gitlab).to receive(:com?).and_return(true)
+
+      plan_limits.create!(plan_id: free_plan.id)
+      plan_limits.create!(plan_id: bronze_plan.id)
+      plan_limits.create!(plan_id: silver_plan.id)
+      plan_limits.create!(plan_id: gold_plan.id)
+    end
+
+    it 'correctly migrates up and down' do
+      reversible_migration do |migration|
+        migration.before -> {
+          expect(plan_limits.pluck(:plan_id, :ci_daily_pipeline_schedule_triggers)).to contain_exactly(
+            [free_plan.id, 0],
+            [bronze_plan.id, 0],
+            [silver_plan.id, 0],
+            [gold_plan.id, 0]
+          )
+        }
+
+        migration.after -> {
+          expect(plan_limits.pluck(:plan_id, :ci_daily_pipeline_schedule_triggers)).to contain_exactly(
+            [free_plan.id, 24],
+            [bronze_plan.id, 288],
+            [silver_plan.id, 288],
+            [gold_plan.id, 288]
+          )
+        }
+      end
+    end
+  end
+
+  context 'when on self hosted' do
+    let(:free_plan) { plans.create!(name: 'free') }
+
+    before do
+      allow(Gitlab).to receive(:com?).and_return(false)
+
+      plan_limits.create!(plan_id: free_plan.id)
+    end
+
+    it 'does nothing' do
+      reversible_migration do |migration|
+        migration.before -> {
+          expect(plan_limits.pluck(:plan_id, :ci_daily_pipeline_schedule_triggers)).to contain_exactly(
+            [free_plan.id, 0]
+          )
+        }
+
+        migration.after -> {
+          expect(plan_limits.pluck(:plan_id, :ci_daily_pipeline_schedule_triggers)).to contain_exactly(
+            [free_plan.id, 0]
+          )
+        }
+      end
+    end
+  end
+end

spec/models/ci/pipeline_schedule_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe Ci::PipelineSchedule do
+  let_it_be(:project) { create_default(:project) }
+
   subject { build(:ci_pipeline_schedule) }
 
   it { is_expected.to belong_to(:project) }
@@ -18,7 +20,7 @@ RSpec.describe Ci::PipelineSchedule do
   it { is_expected.to respond_to(:next_run_at) }
 
   it_behaves_like 'includes Limitable concern' do
-    subject { build(:ci_pipeline_schedule) }
+    subject { build(:ci_pipeline_schedule, project: project) }
   end
 
   describe 'validations' do
@@ -103,26 +105,46 @@ RSpec.describe Ci::PipelineSchedule do
   end
 
   describe '#set_next_run_at' do
-    let(:pipeline_schedule) { create(:ci_pipeline_schedule, :nightly) }
-    let(:ideal_next_run_at) { pipeline_schedule.send(:ideal_next_run_from, Time.zone.now) }
-    let(:cron_worker_next_run_at) { pipeline_schedule.send(:cron_worker_next_run_from, Time.zone.now) }
+    using RSpec::Parameterized::TableSyntax
+
+    where(:worker_cron, :schedule_cron, :plan_limit, :ff_enabled, :now, :result) do
+      '0 1 2 3 *'   | '0 1 * * *'   | nil                                          | true  | Time.zone.local(2021, 3, 2, 1, 0)    | Time.zone.local(2022, 3, 2, 1, 0)
+      '0 1 2 3 *'   | '0 1 * * *'   | (1.day.in_minutes / 1.hour.in_minutes).to_i  | true  | Time.zone.local(2021, 3, 2, 1, 0)    | Time.zone.local(2022, 3, 2, 1, 0)
+      '0 1 2 3 *'   | '0 1 * * *'   | (1.day.in_minutes / 1.hour.in_minutes).to_i  | false | Time.zone.local(2021, 3, 2, 1, 0)    | Time.zone.local(2022, 3, 2, 1, 0)
+      '*/5 * * * *' | '*/1 * * * *' | nil                                          | true  | Time.zone.local(2021, 5, 27, 11, 0)  | Time.zone.local(2021, 5, 27, 11, 5)
+      '*/5 * * * *' | '*/1 * * * *' | (1.day.in_minutes / 1.hour.in_minutes).to_i  | true  | Time.zone.local(2021, 5, 27, 11, 0)  | Time.zone.local(2021, 5, 27, 12, 0)
+      '*/5 * * * *' | '*/1 * * * *' | (1.day.in_minutes / 1.hour.in_minutes).to_i  | false | Time.zone.local(2021, 5, 27, 11, 0)  | Time.zone.local(2021, 5, 27, 11, 5)
+      '*/5 * * * *' | '*/1 * * * *' | (1.day.in_minutes / 10).to_i                 | true  | Time.zone.local(2021, 5, 27, 11, 0)  | Time.zone.local(2021, 5, 27, 11, 10)
+      '*/5 * * * *' | '*/1 * * * *' | 200                                          | true  | Time.zone.local(2021, 5, 27, 11, 0)  | Time.zone.local(2021, 5, 27, 11, 10)
+      '*/5 * * * *' | '*/1 * * * *' | 200                                          | false | Time.zone.local(2021, 5, 27, 11, 0)  | Time.zone.local(2021, 5, 27, 11, 5)
+      '*/5 * * * *' | '0 * * * *'   | nil                                          | true  | Time.zone.local(2021, 5, 27, 11, 0)  | Time.zone.local(2021, 5, 27, 12, 5)
+      '*/5 * * * *' | '0 * * * *'   | (1.day.in_minutes / 10).to_i                 | true  | Time.zone.local(2021, 5, 27, 11, 0)  | Time.zone.local(2021, 5, 27, 12, 0)
+      '*/5 * * * *' | '0 * * * *'   | (1.day.in_minutes / 10).to_i                 | false | Time.zone.local(2021, 5, 27, 11, 0)  | Time.zone.local(2021, 5, 27, 12, 5)
+      '*/5 * * * *' | '0 * * * *'   | (1.day.in_minutes / 1.hour.in_minutes).to_i  | true  | Time.zone.local(2021, 5, 27, 11, 0)  | Time.zone.local(2021, 5, 27, 12, 0)
+      '*/5 * * * *' | '0 * * * *'   | (1.day.in_minutes / 2.hours.in_minutes).to_i | true  | Time.zone.local(2021, 5, 27, 11, 0)  | Time.zone.local(2021, 5, 27, 12, 5)
+    end
+
+    with_them do
+      let(:pipeline_schedule) { create(:ci_pipeline_schedule, cron: schedule_cron) }
 
-    context 'when PipelineScheduleWorker runs at a specific interval' do
       before do
         allow(Settings).to receive(:cron_jobs) do
-          {
-            'pipeline_schedule_worker' => {
-              'cron' => '0 1 2 3 *'
-            }
-          }
+          { 'pipeline_schedule_worker' => { 'cron' => worker_cron } }
         end
+
+        create(:plan_limits, :default_plan, ci_daily_pipeline_schedule_triggers: plan_limit) if plan_limit
+        stub_feature_flags(ci_daily_limit_for_pipeline_schedules: false) unless ff_enabled
+
+        # Setting this here to override initial save with the current time
+        pipeline_schedule.next_run_at = now
       end
 
-      it "updates next_run_at to the sidekiq worker's execution time" do
-        expect(pipeline_schedule.next_run_at.min).to eq(0)
-        expect(pipeline_schedule.next_run_at.hour).to eq(1)
-        expect(pipeline_schedule.next_run_at.day).to eq(2)
-        expect(pipeline_schedule.next_run_at.month).to eq(3)
+      it 'updates next_run_at' do
+        travel_to(now) do
+          pipeline_schedule.set_next_run_at
+
+          expect(pipeline_schedule.next_run_at).to eq(result)
+        end
       end
     end
 

spec/models/plan_limits_spec.rb

@@ -211,6 +211,7 @@ RSpec.describe PlanLimits do
         storage_size_limit
         daily_invites
         web_hook_calls
+        ci_daily_pipeline_schedule_triggers
       ] + disabled_max_artifact_size_columns
     end