Avoid sorting tags by date unless keeping N

Sorting tags by date requires us to get a tag manifest configuration
file, which is an additional network request per tag.

Unless we're keeping only the N last tags, it's unnecessary to sort tags
by date.

app/services/projects/container_repository/cleanup_tags_service.rb

@@ -8,53 +8,30 @@ module Projects
         return error('access denied') unless can_destroy?
 
         tags = container_repository.tags
-        tags_by_digest = group_by_digest(tags)
-
         tags = without_latest(tags)
         tags = filter_by_name(tags)
-        tags = with_manifest(tags)
-        tags = order_by_date(tags)
         tags = filter_keep_n(tags)
         tags = filter_by_older_than(tags)
 
-        deleted_tags = delete_tags(tags, tags_by_digest)
-
-        success(deleted: deleted_tags.map(&:name))
+        delete_tags(container_repository, tags)
       end
 
       private
 
-      def delete_tags(tags_to_delete, tags_by_digest)
-        deleted_digests = group_by_digest(tags_to_delete).select do |digest, tags|
-          delete_tag_digest(tags, tags_by_digest[digest])
-        end
-
-        deleted_digests.values.flatten
-      end
-
-      def delete_tag_digest(tags, other_tags)
-        # Issue: https://gitlab.com/gitlab-org/gitlab-foss/issues/21405
-        # we have to remove all tags due
-        # to Docker Distribution bug unable
-        # to delete single tag
-        return unless tags.count == other_tags.count
+      def delete_tags(container_repository, tags)
+        return success(deleted: []) unless tags.any?
 
-        # delete all tags
-        tags.map(&:unsafe_delete)
-      end
+        tag_names = tags.map(&:name)
 
-      def group_by_digest(tags)
-        tags.group_by(&:digest)
+        Projects::ContainerRepository::DeleteTagsService
+          .new(container_repository.project, current_user, tags: tag_names)
+          .execute(container_repository)
       end
 
       def without_latest(tags)
         tags.reject(&:latest?)
       end
 
-      def with_manifest(tags)
-        tags.select(&:valid?)
-      end
-
       def order_by_date(tags)
         now = DateTime.now
         tags.sort_by { |tag| tag.created_at || now }.reverse
@@ -74,6 +51,9 @@ module Projects
       end
 
       def filter_keep_n(tags)
+        return tags unless params['keep_n']
+
+        tags = order_by_date(tags)
         tags.drop(params['keep_n'].to_i)
       end
 

changelogs/unreleased/208220-improve-performance-of-the-container-repository-cleanup-tags-servi.yml

+---
+title: Improve performance of the container repository cleanup tags service
+merge_request: 27441
+author:
+type: performance

spec/services/projects/container_repository/cleanup_tags_service_spec.rb

@@ -41,7 +41,8 @@ describe Projects::ContainerRepository::CleanupTagsService do
       let(:params) { {} }
 
       it 'does not remove anything' do
-        expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag_by_digest)
+        expect_any_instance_of(Projects::ContainerRepository::DeleteTagsService)
+          .not_to receive(:execute)
 
         is_expected.to include(status: :success, deleted: [])
       end
@@ -49,15 +50,10 @@ describe Projects::ContainerRepository::CleanupTagsService do
 
     context 'when regex matching everything is specified' do
       shared_examples 'removes all matches' do
-        it 'does remove B* and C' do
-          # The :A cannot be removed as config is shared with :latest
-          # The :E cannot be removed as it does not have valid manifest
+        it 'does remove all tags except latest' do
+          expect_delete(%w(A Ba Bb C D E))
 
-          expect_delete('sha256:configB').twice
-          expect_delete('sha256:configC')
-          expect_delete('sha256:configD')
-
-          is_expected.to include(status: :success, deleted: %w(D Bb Ba C))
+          is_expected.to include(status: :success, deleted: %w(A Ba Bb C D E))
         end
       end
 
@@ -82,10 +78,9 @@ describe Projects::ContainerRepository::CleanupTagsService do
       end
 
       it 'does remove C and D' do
-        expect_delete('sha256:configC')
-        expect_delete('sha256:configD')
+        expect_delete(%w(C D))
 
-        is_expected.to include(status: :success, deleted: %w(D C))
+        is_expected.to include(status: :success, deleted: %w(C D))
       end
 
       context 'with overriding allow regex' do
@@ -95,7 +90,7 @@ describe Projects::ContainerRepository::CleanupTagsService do
         end
 
         it 'does not remove C' do
-          expect_delete('sha256:configD')
+          expect_delete(%w(D))
 
           is_expected.to include(status: :success, deleted: %w(D))
         end
@@ -108,36 +103,52 @@ describe Projects::ContainerRepository::CleanupTagsService do
         end
 
         it 'does not remove C' do
-          expect_delete('sha256:configD')
+          expect_delete(%w(D))
 
           is_expected.to include(status: :success, deleted: %w(D))
         end
       end
     end
 
-    context 'when removing a tagged image that is used by another tag' do
+    context 'with allow regex value' do
       let(:params) do
-        { 'name_regex_delete' => 'Ba' }
+        { 'name_regex_delete' => '.*',
+          'name_regex_keep' => 'B.*' }
       end
 
-      it 'does not remove the tag' do
-        # Issue: https://gitlab.com/gitlab-org/gitlab-foss/issues/21405
+      it 'does not remove B*' do
+        expect_delete(%w(A C D E))
 
-        is_expected.to include(status: :success, deleted: [])
+        is_expected.to include(status: :success, deleted: %w(A C D E))
       end
     end
 
-    context 'with allow regex value' do
+    context 'when keeping only N tags' do
       let(:params) do
-        { 'name_regex_delete' => '.*',
-          'name_regex_keep' => 'B.*' }
+        { 'name_regex' => 'A|B.*|C',
+          'keep_n' => 1 }
       end
 
-      it 'does not remove B*' do
-        expect_delete('sha256:configC')
-        expect_delete('sha256:configD')
+      it 'sorts tags by date' do
+        expect_delete(%w(Bb Ba C))
+
+        expect(service).to receive(:order_by_date).and_call_original
 
-        is_expected.to include(status: :success, deleted: %w(D C))
+        is_expected.to include(status: :success, deleted: %w(Bb Ba C))
+      end
+    end
+
+    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).not_to receive(:order_by_date)
+
+        is_expected.to include(status: :success, deleted: %w(A Ba Bb C))
       end
     end
 
@@ -147,10 +158,10 @@ describe Projects::ContainerRepository::CleanupTagsService do
           'keep_n' => 3 }
       end
 
-      it 'does remove C as it is oldest' do
-        expect_delete('sha256:configC')
+      it 'does remove B* and C as they are the oldest' do
+        expect_delete(%w(Bb Ba C))
 
-        is_expected.to include(status: :success, deleted: %w(C))
+        is_expected.to include(status: :success, deleted: %w(Bb Ba C))
       end
     end
 
@@ -161,10 +172,9 @@ describe Projects::ContainerRepository::CleanupTagsService do
       end
 
       it 'does remove B* and C as they are older than 1 day' do
-        expect_delete('sha256:configB').twice
-        expect_delete('sha256:configC')
+        expect_delete(%w(Ba Bb C))
 
-        is_expected.to include(status: :success, deleted: %w(Bb Ba C))
+        is_expected.to include(status: :success, deleted: %w(Ba Bb C))
       end
     end
 
@@ -176,8 +186,7 @@ describe Projects::ContainerRepository::CleanupTagsService do
       end
 
       it 'does remove B* and C' do
-        expect_delete('sha256:configB').twice
-        expect_delete('sha256:configC')
+        expect_delete(%w(Bb Ba C))
 
         is_expected.to include(status: :success, deleted: %w(Bb Ba C))
       end
@@ -195,8 +204,7 @@ describe Projects::ContainerRepository::CleanupTagsService do
         end
 
         it 'succeeds without a user' do
-          expect_delete('sha256:configB').twice
-          expect_delete('sha256:configC')
+          expect_delete(%w(Bb Ba C))
 
           is_expected.to include(status: :success, deleted: %w(Bb Ba C))
         end
@@ -238,9 +246,14 @@ describe Projects::ContainerRepository::CleanupTagsService do
     end
   end
 
-  def expect_delete(digest)
-    expect_any_instance_of(ContainerRegistry::Client)
-      .to receive(:delete_repository_tag_by_digest)
-      .with(repository.path, digest) { true }
+  def expect_delete(tags)
+    expect(Projects::ContainerRepository::DeleteTagsService)
+      .to receive(:new)
+      .with(repository.project, user, tags: tags)
+      .and_call_original
+
+    expect_any_instance_of(Projects::ContainerRepository::DeleteTagsService)
+      .to receive(:execute)
+      .with(repository) { { status: :success, deleted: tags } }
   end
 end