diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2021-10-20 11:43:02 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2021-10-20 11:43:02 +0300 |
commit | d9ab72d6080f594d0b3cae15f14b3ef2c6c638cb (patch) | |
tree | 2341ef426af70ad1e289c38036737e04b0aa5007 /spec/services/projects/container_repository/cleanup_tags_service_spec.rb | |
parent | d6e514dd13db8947884cd58fe2a9c2a063400a9b (diff) |
Add latest changes from gitlab-org/gitlab@14-4-stable-eev14.4.0-rc42
Diffstat (limited to 'spec/services/projects/container_repository/cleanup_tags_service_spec.rb')
-rw-r--r-- | spec/services/projects/container_repository/cleanup_tags_service_spec.rb | 541 |
1 files changed, 352 insertions, 189 deletions
diff --git a/spec/services/projects/container_repository/cleanup_tags_service_spec.rb b/spec/services/projects/container_repository/cleanup_tags_service_spec.rb index eed22416868..289bbf4540e 100644 --- a/spec/services/projects/container_repository/cleanup_tags_service_spec.rb +++ b/spec/services/projects/container_repository/cleanup_tags_service_spec.rb @@ -2,14 +2,14 @@ require 'spec_helper' -RSpec.describe Projects::ContainerRepository::CleanupTagsService do +RSpec.describe Projects::ContainerRepository::CleanupTagsService, :clean_gitlab_redis_cache do using RSpec::Parameterized::TableSyntax let_it_be(:user) { create(:user) } let_it_be(:project, reload: true) { create(:project, :private) } - let_it_be(:repository) { create(:container_repository, :root, project: project) } - let(:service) { described_class.new(project, user, params) } + let(:repository) { create(:container_repository, :root, project: project) } + let(:service) { described_class.new(repository, user, params) } let(:tags) { %w[latest A Ba Bb C D E] } before do @@ -39,291 +39,442 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do end describe '#execute' do - subject { service.execute(repository) } + subject { service.execute } - context 'when no params are specified' do - let(:params) { {} } + shared_examples 'reading and removing tags' do |caching_enabled: true| + context 'when no params are specified' do + let(:params) { {} } - it 'does not remove anything' do - expect_any_instance_of(Projects::ContainerRepository::DeleteTagsService) - .not_to receive(:execute) + it 'does not remove anything' do + expect_any_instance_of(Projects::ContainerRepository::DeleteTagsService) + .not_to receive(:execute) + expect_no_caching - is_expected.to eq(expected_service_response(before_truncate_size: 0, after_truncate_size: 0, before_delete_size: 0)) - end - end - - context 'when regex matching everything is specified' do - shared_examples 'removes all matches' do - it 'does remove all tags except latest' do - expect_delete(%w(A Ba Bb C D E)) - - is_expected.to eq(expected_service_response(deleted: %w(A Ba Bb C D E))) + is_expected.to eq(expected_service_response(before_truncate_size: 0, after_truncate_size: 0, before_delete_size: 0)) end end - let(:params) do - { 'name_regex_delete' => '.*' } - end + context 'when regex matching everything is specified' do + shared_examples 'removes all matches' do + it 'does remove all tags except latest' do + expect_no_caching - it_behaves_like 'removes all matches' + expect_delete(%w(A Ba Bb C D E)) + + is_expected.to eq(expected_service_response(deleted: %w(A Ba Bb C D E))) + end + end - context 'with deprecated name_regex param' do let(:params) do - { 'name_regex' => '.*' } + { 'name_regex_delete' => '.*' } end it_behaves_like 'removes all matches' + + context 'with deprecated name_regex param' do + let(:params) do + { 'name_regex' => '.*' } + end + + it_behaves_like 'removes all matches' + end end - end - context 'with invalid regular expressions' do - RSpec.shared_examples 'handling an invalid regex' do - it 'keeps all tags' do - expect(Projects::ContainerRepository::DeleteTagsService) - .not_to receive(:new) - subject + context 'with invalid regular expressions' do + shared_examples 'handling an invalid regex' do + it 'keeps all tags' do + expect_no_caching + + expect(Projects::ContainerRepository::DeleteTagsService) + .not_to receive(:new) + + subject + end + + it { is_expected.to eq(status: :error, message: 'invalid regex') } + + it 'calls error tracking service' do + expect(Gitlab::ErrorTracking).to receive(:log_exception).and_call_original + + subject + end end - it { is_expected.to eq(status: :error, message: 'invalid regex') } + context 'when name_regex_delete is invalid' do + let(:params) { { 'name_regex_delete' => '*test*' } } + + it_behaves_like 'handling an invalid regex' + end - it 'calls error tracking service' do - expect(Gitlab::ErrorTracking).to receive(:log_exception).and_call_original + context 'when name_regex is invalid' do + let(:params) { { 'name_regex' => '*test*' } } - subject + it_behaves_like 'handling an invalid regex' end - end - context 'when name_regex_delete is invalid' do - let(:params) { { 'name_regex_delete' => '*test*' } } + context 'when name_regex_keep is invalid' do + let(:params) { { 'name_regex_keep' => '*test*' } } - it_behaves_like 'handling an invalid regex' + it_behaves_like 'handling an invalid regex' + end end - context 'when name_regex is invalid' do - let(:params) { { 'name_regex' => '*test*' } } + context 'when delete regex matching specific tags is used' do + let(:params) do + { 'name_regex_delete' => 'C|D' } + end - it_behaves_like 'handling an invalid regex' - end + it 'does remove C and D' do + expect_delete(%w(C D)) - context 'when name_regex_keep is invalid' do - let(:params) { { 'name_regex_keep' => '*test*' } } + expect_no_caching - it_behaves_like 'handling an invalid regex' - end - end + is_expected.to eq(expected_service_response(deleted: %w(C D), before_truncate_size: 2, after_truncate_size: 2, before_delete_size: 2)) + end - context 'when delete regex matching specific tags is used' do - let(:params) do - { 'name_regex_delete' => 'C|D' } - end + context 'with overriding allow regex' do + let(:params) do + { 'name_regex_delete' => 'C|D', + 'name_regex_keep' => 'C' } + end - it 'does remove C and D' do - expect_delete(%w(C D)) + it 'does not remove C' do + expect_delete(%w(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 + expect_no_caching - context 'with overriding allow regex' do - let(:params) do - { 'name_regex_delete' => 'C|D', - 'name_regex_keep' => 'C' } + is_expected.to eq(expected_service_response(deleted: %w(D), before_truncate_size: 1, after_truncate_size: 1, before_delete_size: 1)) + end end - it 'does not remove C' do - expect_delete(%w(D)) + context 'with name_regex_delete overriding deprecated name_regex' do + let(:params) do + { 'name_regex' => 'C|D', + 'name_regex_delete' => 'D' } + end + + it 'does not remove C' do + expect_delete(%w(D)) + + expect_no_caching - is_expected.to eq(expected_service_response(deleted: %w(D), before_truncate_size: 1, after_truncate_size: 1, before_delete_size: 1)) + 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 - context 'with name_regex_delete overriding deprecated name_regex' do + context 'with allow regex value' do let(:params) do - { 'name_regex' => 'C|D', - 'name_regex_delete' => 'D' } + { 'name_regex_delete' => '.*', + 'name_regex_keep' => 'B.*' } end - it 'does not remove C' do - expect_delete(%w(D)) + it 'does not remove B*' do + expect_delete(%w(A C D E)) + + expect_no_caching - is_expected.to eq(expected_service_response(deleted: %w(D), before_truncate_size: 1, after_truncate_size: 1, before_delete_size: 1)) + 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 - context 'with allow regex value' do - let(:params) do - { 'name_regex_delete' => '.*', - 'name_regex_keep' => 'B.*' } - end + context 'when keeping only N tags' do + let(:params) do + { 'name_regex' => 'A|B.*|C', + 'keep_n' => 1 } + end - it 'does not remove B*' do - expect_delete(%w(A C D E)) + it 'sorts tags by date' do + expect_delete(%w(Bb Ba C)) - 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 + expect_no_caching - context 'when keeping only N tags' do - let(:params) do - { 'name_regex' => 'A|B.*|C', - 'keep_n' => 1 } + expect(service).to receive(:order_by_date).and_call_original + + 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 - it 'sorts tags by date' do - expect_delete(%w(Bb Ba C)) + context 'when not keeping N tags' do + let(:params) do + { 'name_regex' => 'A|B.*|C' } + end + + it 'does not sort tags by date' do + expect_delete(%w(A Ba Bb C)) - expect(service).to receive(:order_by_date).and_call_original + expect_no_caching - 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 + expect(service).not_to receive(:order_by_date) - context 'when not keeping N tags' do - let(:params) do - { 'name_regex' => 'A|B.*|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 - it 'does not sort tags by date' do - expect_delete(%w(A Ba Bb C)) + context 'when removing keeping only 3' do + let(:params) do + { 'name_regex_delete' => '.*', + 'keep_n' => 3 } + end - expect(service).not_to receive(:order_by_date) + it 'does remove B* and C as they are the oldest' do + expect_delete(%w(Bb Ba 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 + expect_no_caching - context 'when removing keeping only 3' do - let(:params) do - { 'name_regex_delete' => '.*', - 'keep_n' => 3 } + is_expected.to eq(expected_service_response(deleted: %w(Bb Ba C), before_delete_size: 3)) + end end - it 'does remove B* and C as they are the oldest' do - expect_delete(%w(Bb Ba C)) + context 'when removing older than 1 day' do + let(:params) do + { 'name_regex_delete' => '.*', + 'older_than' => '1 day' } + end + + it 'does remove B* and C as they are older than 1 day' do + expect_delete(%w(Ba Bb C)) - is_expected.to eq(expected_service_response(deleted: %w(Bb Ba C), before_delete_size: 3)) - end - end + expect_no_caching - context 'when removing older than 1 day' do - let(:params) do - { 'name_regex_delete' => '.*', - 'older_than' => '1 day' } + is_expected.to eq(expected_service_response(deleted: %w(Ba Bb C), before_delete_size: 3)) + end end - it 'does remove B* and C as they are older than 1 day' do - expect_delete(%w(Ba Bb C)) + context 'when combining all parameters' do + let(:params) do + { 'name_regex_delete' => '.*', + 'keep_n' => 1, + 'older_than' => '1 day' } + end + + it 'does remove B* and C' do + expect_delete(%w(Bb Ba C)) - is_expected.to eq(expected_service_response(deleted: %w(Ba Bb C), before_delete_size: 3)) - end - end + expect_no_caching - context 'when combining all parameters' do - let(:params) do - { 'name_regex_delete' => '.*', - 'keep_n' => 1, - 'older_than' => '1 day' } + is_expected.to eq(expected_service_response(deleted: %w(Bb Ba C), before_delete_size: 3)) + end end - it 'does remove B* and C' do - expect_delete(%w(Bb Ba C)) + context 'when running a container_expiration_policy' do + let(:user) { nil } - is_expected.to eq(expected_service_response(deleted: %w(Bb Ba C), before_delete_size: 3)) - end - end + context 'with valid container_expiration_policy param' do + let(:params) do + { 'name_regex_delete' => '.*', + 'keep_n' => 1, + 'older_than' => '1 day', + 'container_expiration_policy' => true } + end - context 'when running a container_expiration_policy' do - let(:user) { nil } + it 'succeeds without a user' do + expect_delete(%w(Bb Ba C), container_expiration_policy: true) - context 'with valid container_expiration_policy param' do - let(:params) do - { 'name_regex_delete' => '.*', - 'keep_n' => 1, - 'older_than' => '1 day', - 'container_expiration_policy' => true } + caching_enabled ? expect_caching : expect_no_caching + + is_expected.to eq(expected_service_response(deleted: %w(Bb Ba C), before_delete_size: 3)) + end end - it 'succeeds without a user' do - expect_delete(%w(Bb Ba C), container_expiration_policy: true) + context 'without container_expiration_policy param' do + let(:params) do + { 'name_regex_delete' => '.*', + 'keep_n' => 1, + 'older_than' => '1 day' } + end - is_expected.to eq(expected_service_response(deleted: %w(Bb Ba C), before_delete_size: 3)) + it 'fails' do + is_expected.to eq(status: :error, message: 'access denied') + end end end - context 'without container_expiration_policy param' do + context 'truncating the tags list' do let(:params) do - { 'name_regex_delete' => '.*', - 'keep_n' => 1, - 'older_than' => '1 day' } + { + '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 + expect_no_caching + + 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) + 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 - it 'fails' do - is_expected.to eq(status: :error, message: 'access denied') + 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 - context 'truncating the tags list' do + context 'caching' do let(:params) do { 'name_regex_delete' => '.*', - 'keep_n' => 1 + 'keep_n' => 1, + 'older_than' => '1 day', + 'container_expiration_policy' => true + } + end + + let(:tags_and_created_ats) do + { + 'A' => 1.hour.ago, + 'Ba' => 5.days.ago, + 'Bb' => 5.days.ago, + 'C' => 1.month.ago, + 'D' => nil, + 'E' => nil } 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 + let(:cacheable_tags) { tags_and_created_ats.reject { |_, value| value.nil? } } - 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 - ) + before do + expect_delete(%w(Bb Ba C), container_expiration_policy: true) + travel_to(Time.zone.local(2021, 9, 2, 12, 0, 0)) + # We froze time so we need to set the created_at stubs again + stub_digest_config('sha256:configA', 1.hour.ago) + stub_digest_config('sha256:configB', 5.days.ago) + stub_digest_config('sha256:configC', 1.month.ago) + end - expect(result).to eq(service_response) - end + after do + travel_back 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 + it 'caches the created_at values' do + ::Gitlab::Redis::Cache.with do |redis| + expect_mget(redis, tags_and_created_ats.keys) + + expect_set(redis, cacheable_tags) + end + + expect(subject).to include(cached_tags_count: 0) end - with_them do + context 'with cached values' 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) + ::Gitlab::Redis::Cache.with do |redis| + redis.set(cache_key('C'), rfc3339(1.month.ago)) + end + end + + it 'uses them' do + ::Gitlab::Redis::Cache.with do |redis| + expect_mget(redis, tags_and_created_ats.keys) + + # because C is already in cache, it should not be cached again + expect_set(redis, cacheable_tags.except('C')) + end + + # We will ping the container registry for all tags *except* for C because it's cached + expect(ContainerRegistry::Blob).to receive(:new).with(repository, "digest" => "sha256:configA").and_call_original + expect(ContainerRegistry::Blob).to receive(:new).with(repository, "digest" => "sha256:configB").twice.and_call_original + expect(ContainerRegistry::Blob).not_to receive(:new).with(repository, "digest" => "sha256:configC") + expect(ContainerRegistry::Blob).to receive(:new).with(repository, "digest" => "sha256:configD").and_call_original + + expect(subject).to include(cached_tags_count: 1) + end + end + + def expect_mget(redis, keys) + expect(redis).to receive(:mget).with(keys.map(&method(:cache_key))).and_call_original + end + + def expect_set(redis, tags) + tags.each do |tag_name, created_at| + ex = 1.day.seconds - (Time.zone.now - created_at).seconds + if ex > 0 + expect(redis).to receive(:set).with(cache_key(tag_name), rfc3339(created_at), ex: ex.to_i) end end + end + + def cache_key(tag_name) + "container_repository:{#{repository.id}}:tag:#{tag_name}:created_at" + end + + def rfc3339(date_time) + # DateTime rfc3339 is different ActiveSupport::TimeWithZone rfc3339 + # The caching will use DateTime rfc3339 + DateTime.rfc3339(date_time.rfc3339).rfc3339 + end + end + + context 'with container_registry_expiration_policies_caching enabled for the project' do + before do + stub_feature_flags(container_registry_expiration_policies_caching: project) + end + + it_behaves_like 'reading and removing tags', caching_enabled: true + end - original_size = 7 - keep_n = 1 + context 'with container_registry_expiration_policies_caching disabled' do + before do + stub_feature_flags(container_registry_expiration_policies_caching: false) + end + + it_behaves_like 'reading and removing tags', caching_enabled: false + end - 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 - ) + context 'with container_registry_expiration_policies_caching not enabled for the project' do + let_it_be(:another_project) { create(:project) } + + before do + stub_feature_flags(container_registry_expiration_policies_caching: another_project) end + + it_behaves_like 'reading and removing tags', caching_enabled: false end end @@ -368,7 +519,19 @@ RSpec.describe Projects::ContainerRepository::CleanupTagsService do original_size: original_size, before_truncate_size: before_truncate_size, after_truncate_size: after_truncate_size, - before_delete_size: before_delete_size + before_delete_size: before_delete_size, + cached_tags_count: 0 }.compact.merge(deleted_size: deleted&.size) end + + def expect_no_caching + expect(::Gitlab::Redis::Cache).not_to receive(:with) + end + + def expect_caching + ::Gitlab::Redis::Cache.with do |redis| + expect(redis).to receive(:mget).and_call_original + expect(redis).to receive(:set).and_call_original + end + end end |