Commit 807e1e82 authored by David Fernandez's avatar David Fernandez Committed by Bob Van Landuyt

Limit list size when cleaning up registry tags

If the throttling feature flag is enabled, the list of
tags to delete will be limited. If the actual list is bigger than
the limit, the original list is truncated.
parent ed41b143
...@@ -303,6 +303,9 @@ class ApplicationSetting < ApplicationRecord ...@@ -303,6 +303,9 @@ class ApplicationSetting < ApplicationRecord
validates :container_registry_delete_tags_service_timeout, validates :container_registry_delete_tags_service_timeout,
numericality: { only_integer: true, greater_than_or_equal_to: 0 } numericality: { only_integer: true, greater_than_or_equal_to: 0 }
validates :container_registry_cleanup_tags_service_max_list_size,
numericality: { only_integer: true, greater_than_or_equal_to: 0 }
validates :container_registry_expiration_policies_worker_capacity, validates :container_registry_expiration_policies_worker_capacity,
numericality: { only_integer: true, greater_than_or_equal_to: 0 } numericality: { only_integer: true, greater_than_or_equal_to: 0 }
......
...@@ -4,6 +4,8 @@ module ContainerExpirationPolicies ...@@ -4,6 +4,8 @@ module ContainerExpirationPolicies
class CleanupService class CleanupService
attr_reader :repository attr_reader :repository
SERVICE_RESULT_FIELDS = %i[original_size before_truncate_size after_truncate_size before_delete_size].freeze
def initialize(repository) def initialize(repository)
@repository = repository @repository = repository
end end
...@@ -13,28 +15,37 @@ module ContainerExpirationPolicies ...@@ -13,28 +15,37 @@ module ContainerExpirationPolicies
repository.start_expiration_policy! repository.start_expiration_policy!
result = Projects::ContainerRepository::CleanupTagsService service_result = Projects::ContainerRepository::CleanupTagsService
.new(project, nil, policy_params.merge('container_expiration_policy' => true)) .new(project, nil, policy_params.merge('container_expiration_policy' => true))
.execute(repository) .execute(repository)
if result[:status] == :success if service_result[:status] == :success
repository.update!( repository.update!(
expiration_policy_cleanup_status: :cleanup_unscheduled, expiration_policy_cleanup_status: :cleanup_unscheduled,
expiration_policy_started_at: nil, expiration_policy_started_at: nil,
expiration_policy_completed_at: Time.zone.now expiration_policy_completed_at: Time.zone.now
) )
success(:finished) success(:finished, service_result)
else else
repository.cleanup_unfinished! repository.cleanup_unfinished!
success(:unfinished) success(:unfinished, service_result)
end end
end end
private private
def success(cleanup_status) def success(cleanup_status, service_result)
ServiceResponse.success(message: "cleanup #{cleanup_status}", payload: { cleanup_status: cleanup_status, container_repository_id: repository.id }) payload = {
cleanup_status: cleanup_status,
container_repository_id: repository.id
}
SERVICE_RESULT_FIELDS.each do |field|
payload["cleanup_tags_service_#{field}".to_sym] = service_result[field]
end
ServiceResponse.success(message: "cleanup #{cleanup_status}", payload: payload)
end end
def policy_params def policy_params
......
...@@ -8,12 +8,26 @@ module Projects ...@@ -8,12 +8,26 @@ module Projects
return error('invalid regex') unless valid_regex? return error('invalid regex') unless valid_regex?
tags = container_repository.tags tags = container_repository.tags
original_size = tags.size
tags = without_latest(tags) tags = without_latest(tags)
tags = filter_by_name(tags) tags = filter_by_name(tags)
before_truncate_size = tags.size
tags = truncate(tags)
after_truncate_size = tags.size
tags = filter_keep_n(tags) tags = filter_keep_n(tags)
tags = filter_by_older_than(tags) tags = filter_by_older_than(tags)
delete_tags(container_repository, tags) delete_tags(container_repository, tags).tap do |result|
result[:original_size] = original_size
result[:before_truncate_size] = before_truncate_size
result[:after_truncate_size] = after_truncate_size
result[:before_delete_size] = tags.size
result[:status] = :error if before_truncate_size != after_truncate_size
end
end end
private private
...@@ -23,12 +37,14 @@ module Projects ...@@ -23,12 +37,14 @@ module Projects
tag_names = tags.map(&:name) tag_names = tags.map(&:name)
Projects::ContainerRepository::DeleteTagsService service = Projects::ContainerRepository::DeleteTagsService.new(
.new(container_repository.project, container_repository.project,
current_user, current_user,
tags: tag_names, tags: tag_names,
container_expiration_policy: params['container_expiration_policy']) container_expiration_policy: params['container_expiration_policy']
.execute(container_repository) )
service.execute(container_repository)
end end
def without_latest(tags) def without_latest(tags)
...@@ -54,7 +70,7 @@ module Projects ...@@ -54,7 +70,7 @@ module Projects
return tags unless params['keep_n'] return tags unless params['keep_n']
tags = order_by_date(tags) tags = order_by_date(tags)
tags.drop(params['keep_n'].to_i) tags.drop(keep_n)
end end
def filter_by_older_than(tags) def filter_by_older_than(tags)
...@@ -83,6 +99,31 @@ module Projects ...@@ -83,6 +99,31 @@ module Projects
::Gitlab::ErrorTracking.log_exception(e, project_id: project.id) ::Gitlab::ErrorTracking.log_exception(e, project_id: project.id)
false false
end end
def truncate(tags)
return tags unless throttling_enabled?
return tags if max_list_size == 0
# truncate the list to make sure that after the #filter_keep_n
# execution, the resulting list will be max_list_size
truncated_size = max_list_size + keep_n
return tags if tags.size <= truncated_size
tags.sample(truncated_size)
end
def throttling_enabled?
Feature.enabled?(:container_registry_expiration_policies_throttling)
end
def max_list_size
::Gitlab::CurrentSettings.current_application_settings.container_registry_cleanup_tags_service_max_list_size.to_i
end
def keep_n
params['keep_n'].to_i
end
end end
end end
end end
...@@ -12,6 +12,14 @@ module ContainerExpirationPolicies ...@@ -12,6 +12,14 @@ module ContainerExpirationPolicies
worker_resource_boundary :unknown worker_resource_boundary :unknown
idempotent! idempotent!
LOG_ON_DONE_FIELDS = %i[
cleanup_status
cleanup_tags_service_original_size
cleanup_tags_service_before_truncate_size
cleanup_tags_service_after_truncate_size
cleanup_tags_service_before_delete_size
].freeze
def perform_work def perform_work
return unless throttling_enabled? return unless throttling_enabled?
return unless container_repository return unless container_repository
...@@ -26,7 +34,7 @@ module ContainerExpirationPolicies ...@@ -26,7 +34,7 @@ module ContainerExpirationPolicies
result = ContainerExpirationPolicies::CleanupService.new(container_repository) result = ContainerExpirationPolicies::CleanupService.new(container_repository)
.execute .execute
log_extra_metadata_on_done(:cleanup_status, result.payload[:cleanup_status]) log_on_done(result)
end end
def remaining_work_count def remaining_work_count
...@@ -92,5 +100,15 @@ module ContainerExpirationPolicies ...@@ -92,5 +100,15 @@ module ContainerExpirationPolicies
def log_info(extra_structure) def log_info(extra_structure)
logger.info(structured_payload(extra_structure)) logger.info(structured_payload(extra_structure))
end end
def log_on_done(result)
LOG_ON_DONE_FIELDS.each do |field|
value = result.payload[field]
next if value.nil?
log_extra_metadata_on_done(field, value)
end
end
end end
end end
---
title: Limit the number of container tags to delete when deleting them in bulk
merge_request: 49961
author:
type: changed
# frozen_string_literal: true
class AddContainerRegistryCleanupTagsServiceMaxListSizeToApplicationSettings < ActiveRecord::Migration[6.0]
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
def change
add_column(:application_settings, :container_registry_cleanup_tags_service_max_list_size, :integer, default: 200, null: false)
end
end
# frozen_string_literal: true
class AddAppSettingsContainerRegCleanupTagsServiceMaxListSizeConstraint < ActiveRecord::Migration[6.0]
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
CONSTRAINT_NAME = 'app_settings_container_reg_cleanup_tags_max_list_size_positive'
disable_ddl_transaction!
def up
add_check_constraint :application_settings, 'container_registry_cleanup_tags_service_max_list_size >= 0', CONSTRAINT_NAME
end
def down
remove_check_constraint :application_settings, CONSTRAINT_NAME
end
end
e3eb306d7956e396f038f07a20c674929fc4aae3e188b24e1087305b3fea8b4d
\ No newline at end of file
59906a3528499dd9c0b4c30088539299c08383ad3b36be8c862a7cc9ef1d50b2
\ No newline at end of file
...@@ -9377,6 +9377,8 @@ CREATE TABLE application_settings ( ...@@ -9377,6 +9377,8 @@ CREATE TABLE application_settings (
disable_feed_token boolean DEFAULT false NOT NULL, disable_feed_token boolean DEFAULT false NOT NULL,
personal_access_token_prefix text, personal_access_token_prefix text,
rate_limiting_response_text text, rate_limiting_response_text text,
container_registry_cleanup_tags_service_max_list_size integer DEFAULT 200 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_registry_exp_policies_worker_capacity_positive CHECK ((container_registry_expiration_policies_worker_capacity >= 0)), CONSTRAINT app_settings_registry_exp_policies_worker_capacity_positive CHECK ((container_registry_expiration_policies_worker_capacity >= 0)),
CONSTRAINT check_17d9558205 CHECK ((char_length((kroki_url)::text) <= 1024)), CONSTRAINT check_17d9558205 CHECK ((char_length((kroki_url)::text) <= 1024)),
CONSTRAINT check_2dba05b802 CHECK ((char_length(gitpod_url) <= 255)), CONSTRAINT check_2dba05b802 CHECK ((char_length(gitpod_url) <= 255)),
......
...@@ -313,6 +313,13 @@ You can run this at most once an hour for a given container repository. This ...@@ -313,6 +313,13 @@ You can run this at most once an hour for a given container repository. This
action doesn't delete blobs. To delete them and recycle disk space, action doesn't delete blobs. To delete them and recycle disk space,
[run the garbage collection](https://docs.gitlab.com/omnibus/maintenance/README.html#removing-unused-layers-not-referenced-by-manifests). [run the garbage collection](https://docs.gitlab.com/omnibus/maintenance/README.html#removing-unused-layers-not-referenced-by-manifests).
WARNING:
The number of tags deleted by this API is limited on GitLab.com
because of the scale of the Container Registry there.
If your Container Registry has a large number of tags to delete,
only some of them will be deleted, and you might need to call this API multiple times.
To schedule tags for automatic deletion, use a [cleanup policy](../user/packages/container_registry/index.md#cleanup-policy) instead.
NOTE: NOTE:
In GitLab 12.4 and later, individual tags are deleted. In GitLab 12.4 and later, individual tags are deleted.
For more details, see the [discussion](https://gitlab.com/gitlab-org/gitlab/-/issues/15737). For more details, see the [discussion](https://gitlab.com/gitlab-org/gitlab/-/issues/15737).
......
...@@ -72,6 +72,7 @@ RSpec.describe ApplicationSetting do ...@@ -72,6 +72,7 @@ RSpec.describe ApplicationSetting do
it { is_expected.not_to allow_value(nil).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(: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 validate_numericality_of(:container_registry_expiration_policies_worker_capacity).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(:snippet_size_limit).only_integer.is_greater_than(0) }
......
...@@ -17,7 +17,7 @@ RSpec.describe ContainerExpirationPolicies::CleanupService do ...@@ -17,7 +17,7 @@ RSpec.describe ContainerExpirationPolicies::CleanupService do
it 'completely clean up the repository' do it 'completely clean up the repository' do
expect(Projects::ContainerRepository::CleanupTagsService) expect(Projects::ContainerRepository::CleanupTagsService)
.to receive(:new).with(project, nil, cleanup_tags_service_params).and_return(cleanup_tags_service) .to receive(:new).with(project, nil, cleanup_tags_service_params).and_return(cleanup_tags_service)
expect(cleanup_tags_service).to receive(:execute).with(repository).and_return(status: :success) expect(cleanup_tags_service).to receive(:execute).with(repository).and_return(status: :success)
response = subject response = subject
...@@ -34,10 +34,14 @@ RSpec.describe ContainerExpirationPolicies::CleanupService do ...@@ -34,10 +34,14 @@ RSpec.describe ContainerExpirationPolicies::CleanupService do
end end
context 'without a successful cleanup tags service execution' do context 'without a successful cleanup tags service execution' do
it 'partially clean up the repository' do let(:cleanup_tags_service_response) { { status: :error, message: 'timeout' } }
before do
expect(Projects::ContainerRepository::CleanupTagsService) expect(Projects::ContainerRepository::CleanupTagsService)
.to receive(:new).and_return(double(execute: { status: :error, message: 'timeout' })) .to receive(:new).and_return(double(execute: cleanup_tags_service_response))
end
it 'partially clean up the repository' do
response = subject response = subject
aggregate_failures "checking the response and container repositories" do aggregate_failures "checking the response and container repositories" do
...@@ -49,6 +53,39 @@ RSpec.describe ContainerExpirationPolicies::CleanupService do ...@@ -49,6 +53,39 @@ RSpec.describe ContainerExpirationPolicies::CleanupService do
expect(repository.expiration_policy_completed_at).to eq(nil) expect(repository.expiration_policy_completed_at).to eq(nil)
end end
end end
context 'with a truncated cleanup tags service response' do
let(:cleanup_tags_service_response) do
{
status: :error,
original_size: 1000,
before_truncate_size: 800,
after_truncate_size: 200,
before_delete_size: 100
}
end
it 'partially clean up the repository' do
response = subject
aggregate_failures "checking the response and container repositories" do
expect(response.success?).to eq(true)
expect(response.payload)
.to include(
cleanup_status: :unfinished,
container_repository_id: repository.id,
cleanup_tags_service_original_size: 1000,
cleanup_tags_service_before_truncate_size: 800,
cleanup_tags_service_after_truncate_size: 200,
cleanup_tags_service_before_delete_size: 100
)
expect(ContainerRepository.waiting_for_cleanup.count).to eq(1)
expect(repository.reload.cleanup_unfinished?).to be_truthy
expect(repository.expiration_policy_started_at).not_to eq(nil)
expect(repository.expiration_policy_completed_at).to eq(nil)
end
end
end
end end
context 'with no repository' do context 'with no repository' do
......
...@@ -3,11 +3,14 @@ ...@@ -3,11 +3,14 @@
require 'spec_helper' require 'spec_helper'
RSpec.describe Projects::ContainerRepository::CleanupTagsService do RSpec.describe Projects::ContainerRepository::CleanupTagsService do
using RSpec::Parameterized::TableSyntax
let_it_be(:user) { create(:user) } let_it_be(:user) { create(:user) }
let_it_be(:project, reload: true) { create(:project, :private) } let_it_be(:project, reload: true) { create(:project, :private) }
let_it_be(:repository) { create(:container_repository, :root, project: project) } let_it_be(:repository) { create(:container_repository, :root, project: project) }
let(:service) { described_class.new(project, user, params) } let(:service) { described_class.new(project, user, params) }
let(:tags) { %w[latest A Ba Bb C D E] }
before do before do
project.add_maintainer(user) project.add_maintainer(user)
...@@ -16,7 +19,8 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do ...@@ -16,7 +19,8 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do
stub_container_registry_tags( stub_container_registry_tags(
repository: repository.path, repository: repository.path,
tags: %w(latest A Ba Bb C D E)) tags: tags
)
stub_tag_digest('latest', 'sha256:configA') stub_tag_digest('latest', 'sha256:configA')
stub_tag_digest('A', 'sha256:configA') stub_tag_digest('A', 'sha256:configA')
...@@ -30,6 +34,8 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do ...@@ -30,6 +34,8 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do
stub_digest_config('sha256:configB', 5.days.ago) stub_digest_config('sha256:configB', 5.days.ago)
stub_digest_config('sha256:configC', 1.month.ago) stub_digest_config('sha256:configC', 1.month.ago)
stub_digest_config('sha256:configD', nil) stub_digest_config('sha256:configD', nil)
stub_feature_flags(container_registry_expiration_policies_throttling: false)
end end
describe '#execute' do describe '#execute' do
...@@ -42,7 +48,7 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do ...@@ -42,7 +48,7 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do
expect_any_instance_of(Projects::ContainerRepository::DeleteTagsService) expect_any_instance_of(Projects::ContainerRepository::DeleteTagsService)
.not_to receive(:execute) .not_to receive(:execute)
is_expected.to include(status: :success, deleted: []) is_expected.to eq(expected_service_response(before_truncate_size: 0, after_truncate_size: 0, before_delete_size: 0))
end end
end end
...@@ -51,7 +57,7 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do ...@@ -51,7 +57,7 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do
it 'does remove all tags except latest' do it 'does remove all tags except latest' do
expect_delete(%w(A Ba Bb C D E)) expect_delete(%w(A Ba Bb C D E))
is_expected.to include(status: :success, deleted: %w(A Ba Bb C D E)) is_expected.to eq(expected_service_response(deleted: %w(A Ba Bb C D E)))
end end
end end
...@@ -78,12 +84,7 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do ...@@ -78,12 +84,7 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do
subject subject
end end
it 'returns an error' do it { is_expected.to eq(status: :error, message: 'invalid regex') }
response = subject
expect(response[:status]).to eq(:error)
expect(response[:message]).to eq('invalid regex')
end
it 'calls error tracking service' do it 'calls error tracking service' do
expect(Gitlab::ErrorTracking).to receive(:log_exception).and_call_original expect(Gitlab::ErrorTracking).to receive(:log_exception).and_call_original
...@@ -119,7 +120,7 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do ...@@ -119,7 +120,7 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do
it 'does remove C and D' do it 'does remove C and D' do
expect_delete(%w(C D)) expect_delete(%w(C D))
is_expected.to include(status: :success, deleted: %w(C D)) is_expected.to eq(expected_service_response(deleted: %w(C D), before_truncate_size: 2, after_truncate_size: 2, before_delete_size: 2))
end end
context 'with overriding allow regex' do context 'with overriding allow regex' do
...@@ -131,7 +132,7 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do ...@@ -131,7 +132,7 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do
it 'does not remove C' do it 'does not remove C' do
expect_delete(%w(D)) expect_delete(%w(D))
is_expected.to include(status: :success, deleted: %w(D)) is_expected.to eq(expected_service_response(deleted: %w(D), before_truncate_size: 1, after_truncate_size: 1, before_delete_size: 1))
end end
end end
...@@ -144,7 +145,7 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do ...@@ -144,7 +145,7 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do
it 'does not remove C' do it 'does not remove C' do
expect_delete(%w(D)) expect_delete(%w(D))
is_expected.to include(status: :success, deleted: %w(D)) is_expected.to eq(expected_service_response(deleted: %w(D), before_truncate_size: 1, after_truncate_size: 1, before_delete_size: 1))
end end
end end
end end
...@@ -158,7 +159,7 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do ...@@ -158,7 +159,7 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do
it 'does not remove B*' do it 'does not remove B*' do
expect_delete(%w(A C D E)) expect_delete(%w(A C D E))
is_expected.to include(status: :success, deleted: %w(A C D E)) is_expected.to eq(expected_service_response(deleted: %w(A C D E), before_truncate_size: 4, after_truncate_size: 4, before_delete_size: 4))
end end
end end
...@@ -173,7 +174,7 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do ...@@ -173,7 +174,7 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do
expect(service).to receive(:order_by_date).and_call_original expect(service).to receive(:order_by_date).and_call_original
is_expected.to include(status: :success, deleted: %w(Bb Ba C)) is_expected.to eq(expected_service_response(deleted: %w(Bb Ba C), before_truncate_size: 4, after_truncate_size: 4, before_delete_size: 3))
end end
end end
...@@ -187,7 +188,7 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do ...@@ -187,7 +188,7 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do
expect(service).not_to receive(:order_by_date) expect(service).not_to receive(:order_by_date)
is_expected.to include(status: :success, deleted: %w(A Ba Bb C)) is_expected.to eq(expected_service_response(deleted: %w(A Ba Bb C), before_truncate_size: 4, after_truncate_size: 4, before_delete_size: 4))
end end
end end
...@@ -200,7 +201,7 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do ...@@ -200,7 +201,7 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do
it 'does remove B* and C as they are the oldest' do it 'does remove B* and C as they are the oldest' do
expect_delete(%w(Bb Ba C)) expect_delete(%w(Bb Ba C))
is_expected.to include(status: :success, deleted: %w(Bb Ba C)) is_expected.to eq(expected_service_response(deleted: %w(Bb Ba C), before_delete_size: 3))
end end
end end
...@@ -213,7 +214,7 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do ...@@ -213,7 +214,7 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do
it 'does remove B* and C as they are older than 1 day' do it 'does remove B* and C as they are older than 1 day' do
expect_delete(%w(Ba Bb C)) expect_delete(%w(Ba Bb C))
is_expected.to include(status: :success, deleted: %w(Ba Bb C)) is_expected.to eq(expected_service_response(deleted: %w(Ba Bb C), before_delete_size: 3))
end end
end end
...@@ -227,7 +228,7 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do ...@@ -227,7 +228,7 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do
it 'does remove B* and C' do it 'does remove B* and C' do
expect_delete(%w(Bb Ba C)) expect_delete(%w(Bb Ba C))
is_expected.to include(status: :success, deleted: %w(Bb Ba C)) is_expected.to eq(expected_service_response(deleted: %w(Bb Ba C), before_delete_size: 3))
end end
end end
...@@ -245,7 +246,7 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do ...@@ -245,7 +246,7 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do
it 'succeeds without a user' do it 'succeeds without a user' do
expect_delete(%w(Bb Ba C), container_expiration_policy: true) expect_delete(%w(Bb Ba C), container_expiration_policy: true)
is_expected.to include(status: :success, deleted: %w(Bb Ba C)) is_expected.to eq(expected_service_response(deleted: %w(Bb Ba C), before_delete_size: 3))
end end
end end
...@@ -257,10 +258,73 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do ...@@ -257,10 +258,73 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do
end end
it 'fails' do it 'fails' do
is_expected.to include(status: :error, message: 'access denied') is_expected.to eq(status: :error, message: 'access denied')
end end
end end
end end
context 'truncating the tags list' do
let(:params) do
{
'name_regex_delete' => '.*',
'keep_n' => 1
}
end
shared_examples 'returning the response' do |status:, original_size:, before_truncate_size:, after_truncate_size:, before_delete_size:|
it 'returns the response' do
result = subject
service_response = expected_service_response(
status: status,
original_size: original_size,
before_truncate_size: before_truncate_size,
after_truncate_size: after_truncate_size,
before_delete_size: before_delete_size,
deleted: nil
)
expect(result).to eq(service_response.compact)
end
end
where(:feature_flag_enabled, :max_list_size, :delete_tags_service_status, :expected_status, :expected_truncated) do
false | 10 | :success | :success | false
false | 10 | :error | :error | false
false | 3 | :success | :success | false
false | 3 | :error | :error | false
false | 0 | :success | :success | false
false | 0 | :error | :error | false
true | 10 | :success | :success | false
true | 10 | :error | :error | false
true | 3 | :success | :error | true
true | 3 | :error | :error | true
true | 0 | :success | :success | false
true | 0 | :error | :error | false
end
with_them do
before do
stub_feature_flags(container_registry_expiration_policies_throttling: feature_flag_enabled)
stub_application_setting(container_registry_cleanup_tags_service_max_list_size: max_list_size)
allow_next_instance_of(Projects::ContainerRepository::DeleteTagsService) do |service|
expect(service).to receive(:execute).and_return(status: delete_tags_service_status)
end
end
original_size = 7
keep_n = 1
it_behaves_like(
'returning the response',
status: params[:expected_status],
original_size: original_size,
before_truncate_size: original_size - keep_n,
after_truncate_size: params[:expected_truncated] ? params[:max_list_size] + keep_n : original_size - keep_n,
before_delete_size: params[:expected_truncated] ? params[:max_list_size] : original_size - keep_n - 1 # one tag is filtered out with older_than filter
)
end
end
end end
private private
...@@ -295,4 +359,16 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do ...@@ -295,4 +359,16 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do
.to receive(:execute) .to receive(:execute)
.with(repository) { { status: :success, deleted: tags } } .with(repository) { { status: :success, deleted: tags } }
end end
# all those -1 because the default tags on L13 have a "latest" that will be filtered out
def expected_service_response(status: :success, deleted: [], original_size: tags.size, before_truncate_size: tags.size - 1, after_truncate_size: tags.size - 1, before_delete_size: tags.size - 1)
{
status: status,
deleted: deleted,
original_size: original_size,
before_truncate_size: before_truncate_size,
after_truncate_size: after_truncate_size,
before_delete_size: before_delete_size
}.compact
end
end end
...@@ -19,23 +19,34 @@ RSpec.describe ContainerExpirationPolicies::CleanupContainerRepositoryWorker do ...@@ -19,23 +19,34 @@ RSpec.describe ContainerExpirationPolicies::CleanupContainerRepositoryWorker do
RSpec.shared_examples 'handling all repository conditions' do RSpec.shared_examples 'handling all repository conditions' do
it 'sends the repository for cleaning' do it 'sends the repository for cleaning' do
service_response = cleanup_service_response(repository: repository)
expect(ContainerExpirationPolicies::CleanupService) expect(ContainerExpirationPolicies::CleanupService)
.to receive(:new).with(repository).and_return(double(execute: cleanup_service_response(repository: repository))) .to receive(:new).with(repository).and_return(double(execute: service_response))
expect(worker).to receive(:log_extra_metadata_on_done).with(:cleanup_status, :finished) expect_log_extra_metadata(service_response: service_response)
expect(worker).to receive(:log_extra_metadata_on_done).with(:container_repository_id, repository.id)
subject subject
end end
context 'with unfinished cleanup' do context 'with unfinished cleanup' do
it 'logs an unfinished cleanup' do it 'logs an unfinished cleanup' do
service_response = cleanup_service_response(status: :unfinished, repository: repository)
expect(ContainerExpirationPolicies::CleanupService) expect(ContainerExpirationPolicies::CleanupService)
.to receive(:new).with(repository).and_return(double(execute: cleanup_service_response(status: :unfinished, repository: repository))) .to receive(:new).with(repository).and_return(double(execute: service_response))
expect(worker).to receive(:log_extra_metadata_on_done).with(:cleanup_status, :unfinished) expect_log_extra_metadata(service_response: service_response, cleanup_status: :unfinished)
expect(worker).to receive(:log_extra_metadata_on_done).with(:container_repository_id, repository.id)
subject subject
end end
context 'with a truncated list of tags to delete' do
it 'logs an unfinished cleanup' do
service_response = cleanup_service_response(status: :unfinished, repository: repository, cleanup_tags_service_after_truncate_size: 10, cleanup_tags_service_before_delete_size: 5)
expect(ContainerExpirationPolicies::CleanupService)
.to receive(:new).with(repository).and_return(double(execute: service_response))
expect_log_extra_metadata(service_response: service_response, cleanup_status: :unfinished)
subject
end
end
end end
context 'with policy running shortly' do context 'with policy running shortly' do
...@@ -87,10 +98,10 @@ RSpec.describe ContainerExpirationPolicies::CleanupContainerRepositoryWorker do ...@@ -87,10 +98,10 @@ RSpec.describe ContainerExpirationPolicies::CleanupContainerRepositoryWorker do
let_it_be(:another_repository) { create(:container_repository, :cleanup_unfinished) } let_it_be(:another_repository) { create(:container_repository, :cleanup_unfinished) }
it 'process the cleanup scheduled repository first' do it 'process the cleanup scheduled repository first' do
service_response = cleanup_service_response(repository: repository)
expect(ContainerExpirationPolicies::CleanupService) expect(ContainerExpirationPolicies::CleanupService)
.to receive(:new).with(repository).and_return(double(execute: cleanup_service_response(repository: repository))) .to receive(:new).with(repository).and_return(double(execute: service_response))
expect(worker).to receive(:log_extra_metadata_on_done).with(:cleanup_status, :finished) expect_log_extra_metadata(service_response: service_response)
expect(worker).to receive(:log_extra_metadata_on_done).with(:container_repository_id, repository.id)
subject subject
end end
...@@ -105,10 +116,10 @@ RSpec.describe ContainerExpirationPolicies::CleanupContainerRepositoryWorker do ...@@ -105,10 +116,10 @@ RSpec.describe ContainerExpirationPolicies::CleanupContainerRepositoryWorker do
end end
it 'process the repository with the oldest expiration_policy_started_at' do it 'process the repository with the oldest expiration_policy_started_at' do
service_response = cleanup_service_response(repository: repository)
expect(ContainerExpirationPolicies::CleanupService) expect(ContainerExpirationPolicies::CleanupService)
.to receive(:new).with(repository).and_return(double(execute: cleanup_service_response(repository: repository))) .to receive(:new).with(repository).and_return(double(execute: service_response))
expect(worker).to receive(:log_extra_metadata_on_done).with(:cleanup_status, :finished) expect_log_extra_metadata(service_response: service_response)
expect(worker).to receive(:log_extra_metadata_on_done).with(:container_repository_id, repository.id)
subject subject
end end
...@@ -164,8 +175,27 @@ RSpec.describe ContainerExpirationPolicies::CleanupContainerRepositoryWorker do ...@@ -164,8 +175,27 @@ RSpec.describe ContainerExpirationPolicies::CleanupContainerRepositoryWorker do
end end
end end
def cleanup_service_response(status: :finished, repository:) def cleanup_service_response(status: :finished, repository:, cleanup_tags_service_original_size: 100, cleanup_tags_service_before_truncate_size: 80, cleanup_tags_service_after_truncate_size: 80, cleanup_tags_service_before_delete_size: 50)
ServiceResponse.success(message: "cleanup #{status}", payload: { cleanup_status: status, container_repository_id: repository.id }) ServiceResponse.success(
message: "cleanup #{status}",
payload: {
cleanup_status: status,
container_repository_id: repository.id,
cleanup_tags_service_original_size: cleanup_tags_service_original_size,
cleanup_tags_service_before_truncate_size: cleanup_tags_service_before_truncate_size,
cleanup_tags_service_after_truncate_size: cleanup_tags_service_after_truncate_size,
cleanup_tags_service_before_delete_size: cleanup_tags_service_before_delete_size
}.compact
)
end
def expect_log_extra_metadata(service_response:, cleanup_status: :finished)
expect(worker).to receive(:log_extra_metadata_on_done).with(:cleanup_status, cleanup_status)
expect(worker).to receive(:log_extra_metadata_on_done).with(:container_repository_id, repository.id)
expect(worker).to receive(:log_extra_metadata_on_done).with(:cleanup_tags_service_original_size, service_response.payload[:cleanup_tags_service_original_size])
expect(worker).to receive(:log_extra_metadata_on_done).with(:cleanup_tags_service_before_truncate_size, service_response.payload[:cleanup_tags_service_before_truncate_size])
expect(worker).to receive(:log_extra_metadata_on_done).with(:cleanup_tags_service_after_truncate_size, service_response.payload[:cleanup_tags_service_after_truncate_size])
expect(worker).to receive(:log_extra_metadata_on_done).with(:cleanup_tags_service_before_delete_size, service_response.payload[:cleanup_tags_service_before_delete_size])
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment