Merge branch '208193-limit-delete-tags-service-runtime' into 'master'

Add timeout support in Projects::ContainerRepository::DeleteTagsService#execute

See merge request gitlab-org/gitlab!36319

app/helpers/application_settings_helper.rb

@@ -327,7 +327,8 @@ module ApplicationSettingsHelper
       :group_import_limit,
       :group_export_limit,
       :group_download_export_limit,
-      :wiki_page_max_content_bytes
+      :wiki_page_max_content_bytes,
+      :container_registry_delete_tags_service_timeout
     ]
   end
 

app/helpers/container_registry_helper.rb

+# frozen_string_literal: true
+
+module ContainerRegistryHelper
+  def limit_delete_tags_service?
+    Feature.enabled?(:container_registry_expiration_policies_throttling) &&
+      ContainerRegistry::Client.supports_tag_delete?
+  end
+end

app/models/application_setting.rb

@@ -282,6 +282,9 @@ class ApplicationSetting < ApplicationRecord
 
   validates :hashed_storage_enabled, inclusion: { in: [true], message: _("Hashed storage can't be disabled anymore for new projects") }
 
+  validates :container_registry_delete_tags_service_timeout,
+            numericality: { only_integer: true, greater_than_or_equal_to: 0 }
+
   SUPPORTED_KEY_TYPES.each do |type|
     validates :"#{type}_key_restriction", presence: true, key_restriction: { type: type }
   end

app/models/application_setting_implementation.rb

@@ -163,7 +163,8 @@ module ApplicationSettingImplementation
         user_default_external: false,
         user_default_internal_regex: nil,
         user_show_add_ssh_key_message: true,
-        wiki_page_max_content_bytes: 50.megabytes
+        wiki_page_max_content_bytes: 50.megabytes,
+        container_registry_delete_tags_service_timeout: 100
       }
     end
 

app/services/projects/container_repository/gitlab/delete_tags_service.rb

@@ -5,6 +5,11 @@ module Projects
     module Gitlab
       class DeleteTagsService
         include BaseServiceUtility
+        include ::Gitlab::Utils::StrongMemoize
+
+        DISABLED_TIMEOUTS = [nil, 0].freeze
+
+        TimeoutError = Class.new(StandardError)
 
         def initialize(container_repository, tag_names)
           @container_repository = container_repository
@@ -17,12 +22,42 @@ module Projects
         def execute
           return success(deleted: []) if @tag_names.empty?
 
+          delete_tags
+        rescue TimeoutError => e
+          ::Gitlab::ErrorTracking.track_exception(e, tags_count: @tag_names&.size, container_repository_id: @container_repository&.id)
+          error('timeout while deleting tags')
+        end
+
+        private
+
+        def delete_tags
+          start_time = Time.zone.now
+
           deleted_tags = @tag_names.select do |name|
+            raise TimeoutError if timeout?(start_time)
+
             @container_repository.delete_tag_by_name(name)
           end
 
           deleted_tags.any? ? success(deleted: deleted_tags) : error('could not delete tags')
         end
+
+        def timeout?(start_time)
+          return false unless throttling_enabled?
+          return false if service_timeout.in?(DISABLED_TIMEOUTS)
+
+          (Time.zone.now - start_time) > service_timeout
+        end
+
+        def throttling_enabled?
+          strong_memoize(:feature_flag) do
+            Feature.enabled?(:container_registry_expiration_policies_throttling)
+          end
+        end
+
+        def service_timeout
+          ::Gitlab::CurrentSettings.current_application_settings.container_registry_delete_tags_service_timeout
+        end
       end
     end
   end

app/services/projects/container_repository/third_party/delete_tags_service.rb

@@ -15,7 +15,7 @@ module Projects
         # This is a hack as the registry doesn't support deleting individual
         # tags. This code effectively pushes a dummy image and assigns the tag to it.
         # This way when the tag is deleted only the dummy image is affected.
-        # This is used to preverse compatibility with third-party registries that
+        # This is used to preserve compatibility with third-party registries that
         # don't support fast delete.
         # See https://gitlab.com/gitlab-org/gitlab/issues/15737 for a discussion
         def execute

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

@@ -14,5 +14,11 @@
         .form-text.text-muted
           = _("Existing projects will be able to use expiration policies. Avoid enabling this if an external Container Registry is being used, as there is a performance risk if many images exist on one project.")
           = link_to icon('question-circle'), help_page_path('user/packages/container_registry/index', anchor: 'use-with-external-container-registries')
+    - if limit_delete_tags_service?
+      .form-group
+        = f.label :container_registry_delete_tags_service_timeout, _('Cleanup policy maximum processing time (seconds)'), class: 'label-bold'
+        = f.number_field :container_registry_delete_tags_service_timeout, min: 0, class: 'form-control'
+        .form-text.text-muted
+          = _("Tags are deleted until the timeout is reached. Any remaining tags are included the next time the policy runs. To remove the time limit, set it to 0.")
 
   = f.submit 'Save changes', class: "btn btn-success"

changelogs/unreleased/208193-limit-delete-tags-service-runtime.yml

+---
+title: Add timeout support in the delete tags service for the GitLab Registry
+merge_request: 36319
+author:
+type: changed

config/feature_flags/development/container_registry_expiration_policies_throttling.yml

+---
+name: container_registry_expiration_policies_throttling
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/36319
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/238190
+group: group::package
+type: development
+default_enabled: false

db/migrate/20200710113437_add_container_registry_delete_tags_service_timeout_to_application_settings.rb

+# frozen_string_literal: true
+
+class AddContainerRegistryDeleteTagsServiceTimeoutToApplicationSettings < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def up
+    add_column(
+      :application_settings,
+      :container_registry_delete_tags_service_timeout,
+      :integer,
+      default: 250,
+      null: false
+    )
+  end
+
+  def down
+    remove_column(:application_settings, :container_registry_delete_tags_service_timeout)
+  end
+end

db/schema_migrations/20200710113437

+3d49c22b718c5b4af0a7372584fe12ab730e1ffca501c7f582f7d01200708eb1
\ No newline at end of file

db/structure.sql

@@ -9266,6 +9266,7 @@ CREATE TABLE public.application_settings (
     wiki_page_max_content_bytes bigint DEFAULT 52428800 NOT NULL,
     elasticsearch_indexed_file_size_limit_kb integer DEFAULT 1024 NOT NULL,
     enforce_namespace_storage_limit boolean DEFAULT false NOT NULL,
+    container_registry_delete_tags_service_timeout integer DEFAULT 250 NOT NULL,
     CONSTRAINT check_51700b31b5 CHECK ((char_length(default_branch_name) <= 255)),
     CONSTRAINT check_9c6c447a13 CHECK ((char_length(maintenance_mode_message) <= 255)),
     CONSTRAINT check_d03919528d CHECK ((char_length(container_registry_vendor) <= 255)),

doc/user/packages/container_registry/index.md

@@ -533,6 +533,11 @@ The cleanup policy:
 1. Excludes from the list any tags matching the `name_regex_keep` value (tags to preserve).
 1. Finally, the remaining tags in the list are deleted from the Container Registry.
 
+CAUTION: **Warning:**
+On GitLab.com, the execution time for the cleanup policy is limited, and some of the tags may remain in
+the Container Registry after the policy runs. The next time the policy runs, the remaining tags are included,
+so it may take multiple runs for all tags to be deleted.
+
 ### Create a cleanup policy
 
 You can create a cleanup policy in [the API](#use-the-cleanup-policy-api) or the UI.

lib/container_registry/client.rb

@@ -21,6 +21,17 @@ module ContainerRegistry
     # Taken from: FaradayMiddleware::FollowRedirects
     REDIRECT_CODES = Set.new [301, 302, 303, 307]
 
+    def self.supports_tag_delete?
+      registry_config = Gitlab.config.registry
+      return false unless registry_config.enabled && registry_config.api_url.present?
+
+      return true if ::Gitlab.com?
+
+      token = Auth::ContainerRegistryAuthenticationService.access_token([], [])
+      client = new(registry_config.api_url, token: token)
+      client.supports_tag_delete?
+    end
+
     def initialize(base_uri, options = {})
       @base_uri = base_uri
       @options = options

locale/gitlab.pot

@@ -5009,6 +5009,9 @@ msgstr ""
 msgid "Cleanup policy for tags"
 msgstr ""
 
+msgid "Cleanup policy maximum processing time (seconds)"
+msgstr ""
+
 msgid "Clear"
 msgstr ""
 
@@ -24103,6 +24106,9 @@ msgstr ""
 msgid "Tags"
 msgstr ""
 
+msgid "Tags are deleted until the timeout is reached. Any remaining tags are included the next time the policy runs. To remove the time limit, set it to 0."
+msgstr ""
+
 msgid "Tags feed"
 msgstr ""
 

spec/features/admin/admin_settings_spec.rb

@@ -285,6 +285,55 @@ RSpec.describe 'Admin updates settings', :clean_gitlab_redis_shared_state, :do_n
         expect(current_settings.auto_devops_domain).to eq('domain.com')
         expect(page).to have_content "Application settings saved successfully"
       end
+
+      context 'Container Registry' do
+        context 'delete tags service execution timeout' do
+          let(:feature_flag_enabled) { true }
+          let(:client_support) { true }
+
+          before do
+            stub_container_registry_config(enabled: true)
+            stub_feature_flags(container_registry_expiration_policies_throttling: feature_flag_enabled)
+            allow(ContainerRegistry::Client).to receive(:supports_tag_delete?).and_return(client_support)
+          end
+
+          RSpec.shared_examples 'not having service timeout settings' do
+            it 'lacks the timeout settings' do
+              visit ci_cd_admin_application_settings_path
+
+              expect(page).not_to have_content "Container Registry delete tags service execution timeout"
+            end
+          end
+
+          context 'with feature flag enabled' do
+            context 'with client supporting tag delete' do
+              it 'changes the timeout' do
+                visit ci_cd_admin_application_settings_path
+
+                page.within('.as-registry') do
+                  fill_in 'application_setting_container_registry_delete_tags_service_timeout', with: 400
+                  click_button 'Save changes'
+                end
+
+                expect(current_settings.container_registry_delete_tags_service_timeout).to eq(400)
+                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 service timeout settings'
+            end
+          end
+
+          context 'with feature flag disabled' do
+            let(:feature_flag_enabled) { false }
+
+            it_behaves_like 'not having service timeout settings'
+          end
+        end
+      end
     end
 
     context 'Repository page' do

spec/helpers/container_registry_helper_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ContainerRegistryHelper do
+  using RSpec::Parameterized::TableSyntax
+
+  describe '#limit_delete_tags_service?' do
+    subject { helper.limit_delete_tags_service? }
+
+    where(:feature_flag_enabled, :client_support, :expected_result) do
+      true  | true  | true
+      true  | false | false
+      false | true  | false
+      false | false | false
+    end
+
+    with_them do
+      before do
+        stub_feature_flags(container_registry_expiration_policies_throttling: feature_flag_enabled)
+        allow(ContainerRegistry::Client).to receive(:supports_tag_delete?).and_return(client_support)
+      end
+
+      it { is_expected.to eq(expected_result) }
+    end
+  end
+end

spec/lib/container_registry/client_spec.rb

@@ -289,4 +289,57 @@ RSpec.describe ContainerRegistry::Client do
       end
     end
   end
+
+  describe '.supports_tag_delete?' do
+    let(:registry_enabled) { true }
+    let(:registry_api_url) { 'http://sandbox.local' }
+    let(:registry_tags_support_enabled) { true }
+    let(:is_on_dot_com) { false }
+
+    subject { described_class.supports_tag_delete? }
+
+    before do
+      allow(::Gitlab).to receive(:com?).and_return(is_on_dot_com)
+      stub_container_registry_config(enabled: registry_enabled, api_url: registry_api_url, key: 'spec/fixtures/x509_certificate_pk.key')
+      stub_registry_tags_support(registry_tags_support_enabled)
+    end
+
+    context 'with the registry enabled' do
+      it { is_expected.to be true }
+
+      context 'without an api url' do
+        let(:registry_api_url) { '' }
+
+        it { is_expected.to be false }
+      end
+
+      context 'on .com' do
+        let(:is_on_dot_com) { true }
+
+        it { is_expected.to be true }
+      end
+
+      context 'when registry server does not support tag deletion' do
+        let(:registry_tags_support_enabled) { false }
+
+        it { is_expected.to be false }
+      end
+    end
+
+    context 'with the registry disabled' do
+      let(:registry_enabled) { false }
+
+      it { is_expected.to be false }
+    end
+
+    def stub_registry_tags_support(supported = true)
+      status_code = supported ? 200 : 404
+      stub_request(:options, "#{registry_api_url}/v2/name/tags/reference/tag")
+        .to_return(
+          status: status_code,
+          body: '',
+          headers: { 'Allow' => 'DELETE' }
+        )
+    end
+  end
 end

spec/models/application_setting_spec.rb

@@ -71,6 +71,8 @@ RSpec.describe ApplicationSetting do
     it { is_expected.not_to allow_value('three').for(:push_event_activities_limit) }
     it { is_expected.not_to allow_value(nil).for(:push_event_activities_limit) }
 
+    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(:snippet_size_limit).only_integer.is_greater_than(0) }
     it { is_expected.to validate_numericality_of(:wiki_page_max_content_bytes).only_integer.is_greater_than_or_equal_to(1024) }
     it { is_expected.to validate_presence_of(:max_artifacts_size) }

spec/services/projects/container_repository/delete_tags_service_spec.rb

@@ -90,6 +90,10 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do
 
     subject { service.execute(repository) }
 
+    before do
+      stub_feature_flags(container_registry_expiration_policies_throttling: false)
+    end
+
     context 'without permissions' do
       it { is_expected.to include(status: :error) }
     end
@@ -119,6 +123,18 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do
 
             it_behaves_like 'logging a success response'
           end
+
+          context 'with a timeout error' do
+            before do
+              expect_next_instance_of(::Projects::ContainerRepository::Gitlab::DeleteTagsService) do |delete_service|
+                expect(delete_service).to receive(:delete_tags).and_raise(::Projects::ContainerRepository::Gitlab::DeleteTagsService::TimeoutError)
+              end
+            end
+
+            it { is_expected.to include(status: :error, message: 'timeout while deleting tags') }
+
+            it_behaves_like 'logging an error response', message: 'timeout while deleting tags'
+          end
         end
 
         context 'and the feature is disabled' do

spec/services/projects/container_repository/gitlab/delete_tags_service_spec.rb

@@ -12,13 +12,21 @@ RSpec.describe Projects::ContainerRepository::Gitlab::DeleteTagsService do
 
     subject { service.execute }
 
-    context 'with tags to delete' do
+    before do
+      stub_feature_flags(container_registry_expiration_policies_throttling: false)
+    end
+
+    RSpec.shared_examples 'deleting tags' do
       it 'deletes the tags by name' do
         stub_delete_reference_requests(tags)
         expect_delete_tag_by_names(tags)
 
         is_expected.to eq(status: :success, deleted: tags)
       end
+    end
+
+    context 'with tags to delete' do
+      it_behaves_like 'deleting tags'
 
       it 'succeeds when tag delete returns 404' do
         stub_delete_reference_requests('A' => 200, 'Ba' => 404)
@@ -41,6 +49,47 @@ RSpec.describe Projects::ContainerRepository::Gitlab::DeleteTagsService do
           it { is_expected.to eq(status: :error, message: 'could not delete tags') }
         end
       end
+
+      context 'with throttling enabled' do
+        let(:timeout) { 10 }
+
+        before do
+          stub_feature_flags(container_registry_expiration_policies_throttling: true)
+          stub_application_setting(container_registry_delete_tags_service_timeout: timeout)
+        end
+
+        it_behaves_like 'deleting tags'
+
+        context 'with timeout' do
+          context 'set to a valid value' do
+            before do
+              allow(Time.zone).to receive(:now).and_return(10, 15, 25) # third call to Time.zone.now will be triggering the timeout
+              stub_delete_reference_requests('A' => 200)
+            end
+
+            it { is_expected.to include(status: :error, message: 'timeout while deleting tags') }
+
+            it 'tracks the exception' do
+              expect(::Gitlab::ErrorTracking)
+                .to receive(:track_exception).with(::Projects::ContainerRepository::Gitlab::DeleteTagsService::TimeoutError, tags_count: tags.size, container_repository_id: repository.id)
+
+              subject
+            end
+          end
+
+          context 'set to 0' do
+            let(:timeout) { 0 }
+
+            it_behaves_like 'deleting tags'
+          end
+
+          context 'set to nil' do
+            let(:timeout) { nil }
+
+            it_behaves_like 'deleting tags'
+          end
+        end
+      end
     end
 
     context 'with empty tags' do