Merge branch '263110-improve-cleanup-policies-selection-during-their-execution' into 'master'

Improve cleanup policies selection during their execution

See merge request gitlab-org/gitlab!44748

app/models/container_expiration_policy.rb

@@ -3,6 +3,7 @@
 class ContainerExpirationPolicy < ApplicationRecord
   include Schedulable
   include UsageStatistics
+  include EachBatch
 
   belongs_to :project, inverse_of: :container_expiration_policy
 
@@ -19,6 +20,16 @@ class ContainerExpirationPolicy < ApplicationRecord
   scope :active, -> { where(enabled: true) }
   scope :preloaded, -> { preload(project: [:route]) }
 
+  def self.executable
+    runnable_schedules.where(
+      'EXISTS (?)',
+      ContainerRepository.select(1)
+                         .where(
+                           'container_repositories.project_id = container_expiration_policies.project_id'
+                         )
+    )
+  end
+
   def self.keep_n_options
     {
       1 => _('%{tags} tag per image name') % { tags: 1 },

app/workers/container_expiration_policy_worker.rb

@@ -7,13 +7,15 @@ class ContainerExpirationPolicyWorker # rubocop:disable Scalability/IdempotentWo
   feature_category :container_registry
 
   def perform
-    ContainerExpirationPolicy.runnable_schedules.preloaded.find_each do |container_expiration_policy|
-      with_context(project: container_expiration_policy.project,
-                   user: container_expiration_policy.project.owner) do |project:, user:|
-        ContainerExpirationPolicyService.new(project, user)
-          .execute(container_expiration_policy)
-      rescue ContainerExpirationPolicyService::InvalidPolicyError => e
-        Gitlab::ErrorTracking.log_exception(e, container_expiration_policy_id: container_expiration_policy.id)
+    ContainerExpirationPolicy.executable.preloaded.each_batch do |relation|
+      relation.each do |container_expiration_policy|
+        with_context(project: container_expiration_policy.project,
+                    user: container_expiration_policy.project.owner) do |project:, user:|
+          ContainerExpirationPolicyService.new(project, user)
+            .execute(container_expiration_policy)
+        rescue ContainerExpirationPolicyService::InvalidPolicyError => e
+          Gitlab::ErrorTracking.log_exception(e, container_expiration_policy_id: container_expiration_policy.id)
+        end
       end
     end
   end

changelogs/unreleased/263110-improve-cleanup-policies-selection-during-their-execution.yml

+---
+title: Exclude policies with no container repositories when executing them
+merge_request: 44748
+author:
+type: fixed

db/migrate/20201009090954_add_index_with_project_id_to_container_expiration_policies.rb

+# frozen_string_literal: true
+
+class AddIndexWithProjectIdToContainerExpirationPolicies < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  INDEX_NAME = 'idx_container_exp_policies_on_project_id_next_run_at_enabled'
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :container_expiration_policies, [:project_id, :next_run_at, :enabled], name: INDEX_NAME
+  end
+
+  def down
+    remove_concurrent_index_by_name :container_expiration_policies, INDEX_NAME
+  end
+end

db/schema_migrations/20201009090954

+d0944a864a1a89e9339eb1f8ffab683df1a5bb90f7b7a16cabd4871f34d1cd48
\ No newline at end of file

db/structure.sql

@@ -19622,6 +19622,8 @@ CREATE INDEX idx_audit_events_on_entity_id_desc_author_id_created_at ON audit_ev
 
 CREATE INDEX idx_ci_pipelines_artifacts_locked ON ci_pipelines USING btree (ci_ref_id, id) WHERE (locked = 1);
 
+CREATE INDEX idx_container_exp_policies_on_project_id_next_run_at_enabled ON container_expiration_policies USING btree (project_id, next_run_at, enabled);
+
 CREATE INDEX idx_deployment_clusters_on_cluster_id_and_kubernetes_namespace ON deployment_clusters USING btree (cluster_id, kubernetes_namespace);
 
 CREATE UNIQUE INDEX idx_deployment_merge_requests_unique_index ON deployment_merge_requests USING btree (deployment_id, merge_request_id);

spec/models/container_expiration_policy_spec.rb

@@ -104,6 +104,18 @@ RSpec.describe ContainerExpirationPolicy, type: :model do
     end
   end
 
+  describe '.executable' do
+    subject { described_class.executable }
+
+    let_it_be(:policy1) { create(:container_expiration_policy, :runnable) }
+    let_it_be(:container_repository1) { create(:container_repository, project: policy1.project) }
+    let_it_be(:policy2) { create(:container_expiration_policy, :runnable) }
+    let_it_be(:container_repository2) { create(:container_repository, project: policy2.project) }
+    let_it_be(:policy3) { create(:container_expiration_policy, :runnable) }
+
+    it { is_expected.to contain_exactly(policy1, policy2) }
+  end
+
   describe '#disable!' do
     let_it_be(:container_expiration_policy) { create(:container_expiration_policy) }
 

spec/workers/container_expiration_policy_worker_spec.rb

@@ -7,19 +7,24 @@ RSpec.describe ContainerExpirationPolicyWorker do
 
   subject { described_class.new.perform }
 
-  context 'With no container expiration policies' do
-    it 'Does not execute any policies' do
+  RSpec.shared_examples 'not executing any policy' do
+    it 'does not run any policy' do
       expect(ContainerExpirationPolicyService).not_to receive(:new)
 
       subject
     end
   end
 
+  context 'With no container expiration policies' do
+    it_behaves_like 'not executing any policy'
+  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 }
+    let_it_be(:container_expiration_policy, reload: true) { create(:container_expiration_policy, :runnable) }
+    let_it_be(:container_repository) { create(:container_repository, project: container_expiration_policy.project) }
+    let_it_be(:user) { container_expiration_policy.project.owner }
 
+    context 'a valid policy' do
       it 'runs the policy' do
         service = instance_double(ContainerExpirationPolicyService, execute: true)
 
@@ -31,33 +36,30 @@ RSpec.describe ContainerExpirationPolicyWorker do
     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
+      before do
+        container_expiration_policy.disable!
       end
+
+      it_behaves_like 'not executing any policy'
     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 }
+      before do
+        container_expiration_policy.update_column(:next_run_at, 2.minutes.from_now)
+      end
 
-      it 'does not run the policy' do
-        expect(ContainerExpirationPolicyService)
-          .not_to receive(:new).with(container_expiration_policy, user)
+      it_behaves_like 'not executing any policy'
+    end
 
-        subject
+    context 'a policy linked to no container repository' do
+      before do
+        container_expiration_policy.container_repositories.delete_all
       end
+
+      it_behaves_like 'not executing any policy'
     end
 
     context 'an invalid policy' do
-      let_it_be(:container_expiration_policy) { create(:container_expiration_policy, :runnable) }
-      let_it_be(:user) {container_expiration_policy.project.owner }
-
       before do
         container_expiration_policy.update_column(:name_regex, '*production')
       end