Merge branch '346845-cleanup-policies-application-settings' into 'master'

Add an application setting for cleanup policy caching

See merge request gitlab-org/gitlab!80902

app/helpers/application_settings_helper.rb

@@ -212,6 +212,7 @@ module ApplicationSettingsHelper
       :auto_devops_enabled,
       :auto_devops_domain,
       :container_expiration_policies_enable_historic_entries,
+      :container_registry_expiration_policies_caching,
       :container_registry_token_expire_delay,
       :default_artifacts_expire_in,
       :default_branch_name,

app/models/application_setting.rb

@@ -362,6 +362,9 @@ class ApplicationSetting < ApplicationRecord
             :container_registry_expiration_policies_worker_capacity,
             numericality: { only_integer: true, greater_than_or_equal_to: 0 }
 
+  validates :container_registry_expiration_policies_caching,
+            inclusion: { in: [true, false], message: _('must be a boolean value') }
+
   validates :container_registry_import_max_tags_count,
             :container_registry_import_max_retries,
             :container_registry_import_start_max_retries,

app/models/application_setting_implementation.rb

@@ -220,6 +220,7 @@ module ApplicationSettingImplementation
         container_registry_delete_tags_service_timeout: 250,
         container_registry_expiration_policies_worker_capacity: 4,
         container_registry_cleanup_tags_service_max_list_size: 200,
+        container_registry_expiration_policies_caching: true,
         container_registry_import_max_tags_count: 100,
         container_registry_import_max_retries: 3,
         container_registry_import_start_max_retries: 50,

app/services/projects/container_repository/cleanup_tags_service.rb

@@ -145,8 +145,10 @@ module Projects
       end
 
       def caching_enabled?
-        container_expiration_policy &&
-          older_than.present?
+        result = ::Gitlab::CurrentSettings.current_application_settings.container_registry_expiration_policies_caching &&
+                 container_expiration_policy &&
+                 older_than.present?
+        !!result
       end
 
       def throttling_enabled?

app/views/admin/application_settings/_registry.html.haml

@@ -30,5 +30,13 @@
         = f.number_field :container_registry_cleanup_tags_service_max_list_size, min: 0, class: 'form-control'
         .form-text.text-muted
           = _("The maximum number of tags that a single worker accepts for cleanup. If the number of tags goes above this limit, the list of tags to delete is truncated to this number. To remove this limit, set it to 0.")
+      .form-group
+        .form-check
+          = f.check_box :container_registry_expiration_policies_caching, class: 'form-check-input'
+          = f.label :container_registry_expiration_policies_caching, class: 'form-check-label' do
+            = _("Enable container expiration caching.")
+          .form-text.text-muted
+            = _("When enabled, cleanup polices execute faster but put more load on Redis.")
+            = link_to sprite_icon('question-o'), help_page_path('user/packages/container_registry/reduce_container_registry_storage', anchor: 'set-cleanup-limits-to-conserve-resources')
 
   = f.submit _('Save changes'), class: "gl-button btn btn-confirm"

db/migrate/20220217100008_add_container_registry_expiration_policies_caching_to_application_settings.rb

+# frozen_string_literal: true
+
+class AddContainerRegistryExpirationPoliciesCachingToApplicationSettings < Gitlab::Database::Migration[1.0]
+  enable_lock_retries!
+
+  def up
+    add_column :application_settings, :container_registry_expiration_policies_caching, :boolean, null: false, default: true
+  end
+
+  def down
+    remove_column :application_settings, :container_registry_expiration_policies_caching
+  end
+end

db/schema_migrations/20220217100008

+f52d88262879c40d9ac60a74853b7070036f244fd5f7957c59bbfceb343811d1
\ No newline at end of file

db/structure.sql

@@ -11250,6 +11250,7 @@ CREATE TABLE application_settings (
     ed25519_sk_key_restriction integer DEFAULT 0 NOT NULL,
     users_get_by_id_limit integer DEFAULT 300 NOT NULL,
     users_get_by_id_limit_allowlist text[] DEFAULT '{}'::text[] NOT NULL,
+    container_registry_expiration_policies_caching boolean DEFAULT true NOT NULL,
     CONSTRAINT app_settings_container_reg_cleanup_tags_max_list_size_positive CHECK ((container_registry_cleanup_tags_service_max_list_size >= 0)),
     CONSTRAINT app_settings_dep_proxy_ttl_policies_worker_capacity_positive CHECK ((dependency_proxy_ttl_group_policy_worker_capacity >= 0)),
     CONSTRAINT app_settings_ext_pipeline_validation_service_url_text_limit CHECK ((char_length(external_pipeline_validation_service_url) <= 255)),

doc/api/settings.md

@@ -53,6 +53,10 @@ Example response:
   "gravatar_enabled" : true,
   "sign_in_text" : null,
   "container_expiration_policies_enable_historic_entries": true,
+  "container_registry_cleanup_tags_service_max_list_size": 200,
+  "container_registry_delete_tags_service_timeout": 250,
+  "container_registry_expiration_policies_caching": true,
+  "container_registry_expiration_policies_worker_capacity": 4,
   "container_registry_token_expire_delay": 5,
   "repository_storages_weighted": {"default": 100},
   "plantuml_enabled": false,
@@ -158,6 +162,11 @@ Example response:
   "external_authorization_service_timeout": 0.5,
   "user_oauth_applications": true,
   "after_sign_out_path": "",
+  "container_expiration_policies_enable_historic_entries": true,
+  "container_registry_cleanup_tags_service_max_list_size": 200,
+  "container_registry_delete_tags_service_timeout": 250,
+  "container_registry_expiration_policies_caching": true,
+  "container_registry_expiration_policies_worker_capacity": 4,
   "container_registry_token_expire_delay": 5,
   "repository_storages": ["default"],
   "plantuml_enabled": false,
@@ -248,6 +257,11 @@ listed in the descriptions of the relevant settings.
 | `automatic_purchased_storage_allocation` | boolean          | no                                   | Enabling this permits automatic allocation of purchased storage in a namespace. |
 | `check_namespace_plan` **(PREMIUM)**     | boolean          | no                                   | Enabling this makes only licensed EE features available to projects if the project namespace's plan includes the feature or if the project is public. |
 | `commit_email_hostname`                  | string           | no                                   | Custom hostname (for private commit emails). |
+| `container_expiration_policies_enable_historic_entries`  | boolean | no                            | Enable [cleanup policies](../user/packages/container_registry/reduce_container_registry_storage.md#enable-the-cleanup-policy) for all projects. |
+| `container_registry_cleanup_tags_service_max_list_size`  | integer | no                            | The maximum number of tags that can be deleted in a single execution of [cleanup policies](../user/packages/container_registry/reduce_container_registry_storage.md#set-cleanup-limits-to-conserve-resources). |
+| `container_registry_delete_tags_service_timeout`  | integer | no                                   | The maximum time, in seconds, that the cleanup process can take to delete a batch of tags for [cleanup policies](../user/packages/container_registry/reduce_container_registry_storage.md#set-cleanup-limits-to-conserve-resources). |
+| `container_registry_expiration_policies_caching`  | boolean | no                                   | Caching during the execution of [cleanup policies](../user/packages/container_registry/reduce_container_registry_storage.md#set-cleanup-limits-to-conserve-resources). |
+| `container_registry_expiration_policies_worker_capacity`  | integer          | no                  | Number of workers for [cleanup policies](../user/packages/container_registry/reduce_container_registry_storage.md#set-cleanup-limits-to-conserve-resources). |
 | `container_registry_token_expire_delay`  | integer          | no                                   | Container Registry token duration in minutes. |
 | `deactivate_dormant_users`               | boolean          | no                                   | Enable [automatic deactivation of dormant users](../user/admin_area/moderate_users.md#automatically-deactivate-dormant-users). |
 | `default_artifacts_expire_in`            | string           | no                                   | Set the default expiration time for each job's artifacts. |

doc/user/packages/container_registry/reduce_container_registry_storage.md

@@ -192,6 +192,9 @@ To prevent server resource starvation, the following application settings are av
   deleted in a single execution. Additional tags must be deleted in another execution. We recommend
   starting with a low number and increasing it after monitoring that container images are properly
   deleted. The default value is `200`.
+- `container_registry_expiration_policies_caching`: enable or disable tag creation timestamp caching
+  during execution of policies. Cached timestamps are stored in [Redis](../../../development/architecture.md#redis).
+  Enabled by default.
 
 For self-managed instances, those settings can be updated in the [Rails console](../../../administration/operations/rails_console.md#starting-a-rails-console-session):
 

locale/gitlab.pot

@@ -13562,6 +13562,9 @@ msgstr ""
 msgid "Enable container expiration and retention policies for projects created earlier than GitLab 12.7."
 msgstr ""
 
+msgid "Enable container expiration caching."
+msgstr ""
+
 msgid "Enable delayed project deletion by default for newly-created groups."
 msgstr ""
 
@@ -41392,6 +41395,9 @@ msgstr ""
 msgid "When enabled, SSH keys with no expiry date or an invalid expiration date are no longer accepted. Leave blank for no limit."
 msgstr ""
 
+msgid "When enabled, cleanup polices execute faster but put more load on Redis."
+msgstr ""
+
 msgid "When enabled, existing personal access tokens may be revoked. Leave blank for no limit."
 msgstr ""
 

spec/features/admin/admin_settings_spec.rb

@@ -373,7 +373,8 @@ RSpec.describe 'Admin updates settings' do
           {
             container_registry_delete_tags_service_timeout: 'Container Registry delete tags service execution timeout',
             container_registry_expiration_policies_worker_capacity: 'Cleanup policy maximum workers running concurrently',
-            container_registry_cleanup_tags_service_max_list_size: 'Cleanup policy maximum number of tags to be deleted'
+            container_registry_cleanup_tags_service_max_list_size: 'Cleanup policy maximum number of tags to be deleted',
+            container_registry_expiration_policies_caching: 'Enable container expiration caching'
           }
         end
 
@@ -422,6 +423,38 @@ RSpec.describe 'Admin updates settings' do
             end
           end
         end
+
+        context 'for container registry setting container_registry_expiration_policies_caching' do
+          context 'with feature flag enabled' do
+            context 'with client supporting tag delete' do
+              it 'updates container_registry_expiration_policies_caching' do
+                old_value = current_settings.container_registry_expiration_policies_caching
+
+                visit ci_cd_admin_application_settings_path
+
+                page.within('.as-registry') do
+                  find('#application_setting_container_registry_expiration_policies_caching.form-check-input').click
+                  click_button 'Save changes'
+                end
+
+                expect(current_settings.container_registry_expiration_policies_caching).to eq(!old_value)
+                expect(page).to have_content "Application settings saved successfully"
+              end
+            end
+
+            context 'with client not supporting tag delete' do
+              let(:client_support) { false }
+
+              it_behaves_like 'not having container registry setting', :container_registry_expiration_policies_caching
+            end
+          end
+
+          context 'with feature flag disabled' do
+            let(:feature_flag_enabled) { false }
+
+            it_behaves_like 'not having container registry setting', :container_registry_expiration_policies_caching
+          end
+        end
       end
     end
 

spec/models/application_setting_spec.rb

@@ -76,6 +76,8 @@ RSpec.describe ApplicationSetting do
     it { is_expected.to validate_numericality_of(:container_registry_delete_tags_service_timeout).only_integer.is_greater_than_or_equal_to(0) }
     it { is_expected.to validate_numericality_of(:container_registry_cleanup_tags_service_max_list_size).only_integer.is_greater_than_or_equal_to(0) }
     it { is_expected.to validate_numericality_of(:container_registry_expiration_policies_worker_capacity).only_integer.is_greater_than_or_equal_to(0) }
+    it { is_expected.to allow_value(true).for(:container_registry_expiration_policies_caching) }
+    it { is_expected.to allow_value(false).for(:container_registry_expiration_policies_caching) }
 
     it { is_expected.to validate_numericality_of(:container_registry_import_max_tags_count).only_integer.is_greater_than_or_equal_to(0) }
     it { is_expected.to validate_numericality_of(:container_registry_import_max_retries).only_integer.is_greater_than_or_equal_to(0) }

spec/services/projects/container_repository/cleanup_tags_service_spec.rb

@@ -267,12 +267,30 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService, :clean_gitlab_
             'container_expiration_policy' => true }
         end
 
-        it 'succeeds without a user' do
+        before do
           expect_delete(%w(Bb Ba C), container_expiration_policy: true)
+        end
+
+        it { is_expected.to eq(expected_service_response(deleted: %w(Bb Ba C), before_delete_size: 3)) }
+
+        context 'caching' do
+          it 'expects caching to be used' do
+            expect_caching
 
-          expect_caching
+            subject
+          end
+
+          context 'when setting set to false' do
+            before do
+              stub_application_setting(container_registry_expiration_policies_caching: false)
+            end
 
-          is_expected.to eq(expected_service_response(deleted: %w(Bb Ba C), before_delete_size: 3))
+            it 'does not use caching' do
+              expect_no_caching
+
+              subject
+            end
+          end
         end
       end