Merge branch '15398-recurring-job' into 'master'

Container Expiration Policies recurring jobs

See merge request gitlab-org/gitlab!21593

app/models/ci/pipeline_schedule.rb

@@ -5,6 +5,7 @@ module Ci
     extend Gitlab::Ci::Model
     include Importable
     include StripAttribute
+    include Schedulable
 
     belongs_to :project
     belongs_to :owner, class_name: 'User'
@@ -18,13 +19,10 @@ module Ci
     validates :description, presence: true
     validates :variables, variable_duplicates: true
 
-    before_save :set_next_run_at
-
     strip_attributes :cron
 
     scope :active, -> { where(active: true) }
     scope :inactive, -> { where(active: false) }
-    scope :runnable_schedules, -> { active.where("next_run_at < ?", Time.now) }
     scope :preloaded, -> { preload(:owner, :project) }
 
     accepts_nested_attributes_for :variables, allow_destroy: true
@@ -62,12 +60,6 @@ module Ci
                          end
     end
 
-    def schedule_next_run!
-      save! # with set_next_run_at
-    rescue ActiveRecord::RecordInvalid
-      update_column(:next_run_at, nil) # update without validation
-    end
-
     def job_variables
       variables&.map(&:to_runner_variable) || []
     end

app/models/concerns/schedulable.rb

+# frozen_string_literal: true
+
+module Schedulable
+  extend ActiveSupport::Concern
+
+  included do
+    scope :runnable_schedules, -> { active.where("next_run_at < ?", Time.zone.now) }
+
+    before_save :set_next_run_at
+  end
+
+  def schedule_next_run!
+    save! # with set_next_run_at
+  rescue ActiveRecord::RecordInvalid
+    update_column(:next_run_at, nil) # update without validation
+  end
+
+  def set_next_run_at
+    raise NotImplementedError
+  end
+end

app/models/container_expiration_policy.rb

 # frozen_string_literal: true
 
 class ContainerExpirationPolicy < ApplicationRecord
+  include Schedulable
+
   belongs_to :project, inverse_of: :container_expiration_policy
 
+  delegate :container_repositories, to: :project
+
   validates :project, presence: true
   validates :enabled, inclusion: { in: [true, false] }
   validates :cadence, presence: true, inclusion: { in: ->(_) { self.cadence_options.stringify_keys } }
   validates :older_than, inclusion: { in: ->(_) { self.older_than_options.stringify_keys } }, allow_nil: true
   validates :keep_n, inclusion: { in: ->(_) { self.keep_n_options.keys } }, allow_nil: true
 
+  scope :active, -> { where(enabled: true) }
+  scope :preloaded, -> { preload(:project) }
+
   def self.keep_n_options
     {
       1 => _('%{tags} tag per image name') % { tags: 1 },
@@ -38,4 +45,8 @@ class ContainerExpirationPolicy < ApplicationRecord
       '90d': _('%{days} days until tags are automatically removed') % { days: 90 }
     }
   end
+
+  def set_next_run_at
+    self.next_run_at = Time.zone.now + ChronicDuration.parse(cadence).seconds
+  end
 end

app/services/container_expiration_policy_service.rb

+# frozen_string_literal: true
+
+class ContainerExpirationPolicyService < BaseService
+  def execute(container_expiration_policy)
+    container_expiration_policy.schedule_next_run!
+
+    container_expiration_policy.container_repositories.find_each do |container_repository|
+      CleanupContainerRepositoryWorker.perform_async(
+        current_user.id,
+        container_repository.id,
+        container_expiration_policy.attributes.except("created_at", "updated_at")
+      )
+    end
+  end
+end

app/workers/all_queues.yml

@@ -10,6 +10,7 @@
 - chaos:chaos_sleep
 
 - cronjob:admin_email
+- cronjob:container_expiration_policy
 - cronjob:expire_build_artifacts
 - cronjob:gitlab_usage_ping
 - cronjob:import_export_project_cleanup

app/workers/container_expiration_policy_worker.rb

+# frozen_string_literal: true
+
+class ContainerExpirationPolicyWorker
+  include ApplicationWorker
+  include CronjobQueue
+
+  feature_category :container_registry
+
+  def perform
+    ContainerExpirationPolicy.runnable_schedules.preloaded.find_each do |container_expiration_policy|
+      ContainerExpirationPolicyService.new(
+        container_expiration_policy.project, container_expiration_policy.project.owner
+      ).execute(container_expiration_policy)
+    end
+  end
+end

changelogs/unreleased/15398-recurring-job.yml

+---
+title: Add a cron job and worker to run the Container Expiration Policies
+merge_request: 21593
+author:
+type: added

config/initializers/1_settings.rb

@@ -467,6 +467,9 @@ Settings.cron_jobs['schedule_migrate_external_diffs_worker']['job_class'] = 'Sch
 Settings.cron_jobs['namespaces_prune_aggregation_schedules_worker'] ||= Settingslogic.new({})
 Settings.cron_jobs['namespaces_prune_aggregation_schedules_worker']['cron'] ||= '5 1 * * *'
 Settings.cron_jobs['namespaces_prune_aggregation_schedules_worker']['job_class'] = 'Namespaces::PruneAggregationSchedulesWorker'
+Settings.cron_jobs['container_expiration_policy_worker'] ||= Settingslogic.new({})
+Settings.cron_jobs['container_expiration_policy_worker']['cron'] ||= '50 * * * *'
+Settings.cron_jobs['container_expiration_policy_worker']['job_class'] = 'ContainerExpirationPolicyWorker'
 
 Gitlab.ee do
   Settings.cron_jobs['adjourned_group_deletion_worker'] ||= Settingslogic.new({})

spec/factories/container_expiration_policies.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :container_expiration_policy, class: 'ContainerExpirationPolicy' do
+    association :project, factory: [:project, :without_container_expiration_policy]
+    cadence { '1d' }
+    enabled { true }
+
+    trait :runnable do
+      after(:create) do |policy|
+        # next_run_at will be set before_save to Time.now + cadence, so this ensures the policy is active
+        policy.update_column(:next_run_at, Time.zone.now - 1.day)
+      end
+    end
+
+    trait :disabled do
+      enabled { false }
+    end
+  end
+end

spec/factories/projects.rb

@@ -137,6 +137,12 @@ FactoryBot.define do
       end
     end
 
+    trait :without_container_expiration_policy do
+      after(:build) do |project|
+        project.class.skip_callback(:create, :after, :create_container_expiration_policy, raise: false)
+      end
+    end
+
     # Build a custom repository by specifying a hash of `filename => content` in
     # the transient `files` attribute. Each file will be created in its own
     # commit, operating against the master branch. So, the following call:

spec/models/concerns/schedulable_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Schedulable do
+  shared_examples 'before_save callback' do
+    it 'updates next_run_at' do
+      expect { object.save! }.to change { object.next_run_at }
+    end
+  end
+
+  shared_examples '.runnable_schedules' do
+    it 'returns the runnable schedules' do
+      results = object.class.runnable_schedules
+
+      expect(results).to include(object)
+      expect(results).not_to include(non_runnable_object)
+    end
+  end
+
+  shared_examples '#schedule_next_run!' do
+    it 'saves the object and sets next_run_at' do
+      expect { object.schedule_next_run! }.to change { object.next_run_at }
+    end
+
+    it 'sets next_run_at to nil on error' do
+      expect(object).to receive(:save!).and_raise(ActiveRecord::RecordInvalid)
+
+      object.schedule_next_run!
+
+      expect(object.next_run_at).to be_nil
+    end
+  end
+
+  context 'for a pipeline_schedule' do
+    # let! is used to reset the next_run_at value before each spec
+    let(:object) do
+      Timecop.freeze(1.day.ago) do
+        create(:ci_pipeline_schedule, :hourly)
+      end
+    end
+
+    let(:non_runnable_object) { create(:ci_pipeline_schedule, :hourly) }
+
+    it_behaves_like '#schedule_next_run!'
+    it_behaves_like 'before_save callback'
+    it_behaves_like '.runnable_schedules'
+  end
+
+  context 'for a container_expiration_policy' do
+    # let! is used to reset the next_run_at value before each spec
+    let(:object) { create(:container_expiration_policy, :runnable) }
+    let(:non_runnable_object) { create(:container_expiration_policy) }
+
+    it_behaves_like '#schedule_next_run!'
+    it_behaves_like 'before_save callback'
+    it_behaves_like '.runnable_schedules'
+  end
+
+  describe '#next_run_at' do
+    let(:schedulable_instance) do
+      Class.new(ActiveRecord::Base) do
+        include Schedulable
+
+        # we need a table for the dummy class to operate
+        self.table_name = 'users'
+      end.new
+    end
+
+    it 'works' do
+      expect { schedulable_instance.set_next_run_at }.to raise_error(NotImplementedError)
+    end
+  end
+end

spec/models/container_expiration_policy_spec.rb

@@ -38,4 +38,39 @@ RSpec.describe ContainerExpirationPolicy, type: :model do
       it { is_expected.not_to allow_value('foo').for(:keep_n) }
     end
   end
+
+  describe '.preloaded' do
+    subject { described_class.preloaded }
+
+    before do
+      # container_expiration_policies are created for every new project
+      create_list(:project, 3)
+    end
+
+    it 'preloads the associations' do
+      subject
+
+      query = ActiveRecord::QueryRecorder.new { subject.each(&:project) }
+
+      expect(query.count).to eq(2)
+    end
+  end
+
+  describe '.runnable_schedules' do
+    subject { described_class.runnable_schedules }
+
+    let!(:policy) { create(:container_expiration_policy, :runnable) }
+
+    it 'returns the runnable schedule' do
+      is_expected.to eq([policy])
+    end
+
+    context 'when there are no runnable schedules' do
+      let!(:policy) { }
+
+      it 'returns an empty array' do
+        is_expected.to be_empty
+      end
+    end
+  end
 end

spec/services/container_expiration_policy_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ContainerExpirationPolicyService do
+  let_it_be(:user) { create(:user) }
+  let_it_be(:container_expiration_policy) { create(:container_expiration_policy, :runnable) }
+  let(:project) { container_expiration_policy.project }
+  let(:container_repository) { create(:container_repository, project: project) }
+
+  before do
+    project.add_maintainer(user)
+  end
+
+  describe '#execute' do
+    subject { described_class.new(project, user).execute(container_expiration_policy) }
+
+    it 'kicks off a cleanup worker for the container repository' do
+      expect(CleanupContainerRepositoryWorker).to receive(:perform_async)
+        .with(user.id, container_repository.id, anything)
+
+      subject
+    end
+
+    it 'sets next_run_at on the container_expiration_policy' do
+      subject
+
+      expect(container_expiration_policy.next_run_at).to be > Time.zone.now
+    end
+  end
+end

spec/workers/container_expiration_policy_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ContainerExpirationPolicyWorker do
+  include ExclusiveLeaseHelpers
+
+  subject { described_class.new.perform }
+
+  context 'With no container expiration policies' do
+    it 'Does not execute any policies' do
+      expect(ContainerExpirationPolicyService).not_to receive(:new)
+
+      subject
+    end
+  end
+
+  context 'With container expiration policies' do
+    context 'a valid policy' do
+      let!(:container_expiration_policy) { create(:container_expiration_policy, :runnable) }
+      let(:user) { container_expiration_policy.project.owner }
+
+      it 'runs the policy' do
+        service = instance_double(ContainerExpirationPolicyService, execute: true)
+
+        expect(ContainerExpirationPolicyService)
+          .to receive(:new).with(container_expiration_policy.project, user).and_return(service)
+
+        subject
+      end
+    end
+
+    context 'a disabled policy' do
+      let!(:container_expiration_policy) { create(:container_expiration_policy, :runnable, :disabled) }
+      let(:user) {container_expiration_policy.project.owner }
+
+      it 'does not run the policy' do
+        expect(ContainerExpirationPolicyService)
+          .not_to receive(:new).with(container_expiration_policy, user)
+
+        subject
+      end
+    end
+
+    context 'a policy that is not due for a run' do
+      let!(:container_expiration_policy) { create(:container_expiration_policy) }
+      let(:user) {container_expiration_policy.project.owner }
+
+      it 'does not run the policy' do
+        expect(ContainerExpirationPolicyService)
+          .not_to receive(:new).with(container_expiration_policy, user)
+
+        subject
+      end
+    end
+  end
+end