Merge branch '31832-improve-performance-of-the-container-registry-delete-tags-api' into 'master'

Improve performance of the Container Registry delete tags API

See merge request gitlab-org/gitlab!23325

app/models/container_repository.rb

@@ -77,7 +77,11 @@ class ContainerRepository < ApplicationRecord
   end
 
   def delete_tag_by_digest(digest)
-    client.delete_repository_tag(self.path, digest)
+    client.delete_repository_tag_by_digest(self.path, digest)
+  end
+
+  def delete_tag_by_name(name)
+    client.delete_repository_tag_by_name(self.path, name)
   end
 
   def self.build_from_path(path)

app/services/projects/container_repository/delete_tags_service.rb

@@ -14,12 +14,25 @@ module Projects
 
       private
 
+      # Delete tags by name with a single DELETE request. This is only supported
+      # by the GitLab Container Registry fork. See
+      # https://gitlab.com/gitlab-org/gitlab/-/merge_requests/23325 for details.
+      def fast_delete(container_repository, tag_names)
+        deleted_tags = tag_names.select do |name|
+          container_repository.delete_tag_by_name(name)
+        end
+
+        deleted_tags.any? ? success(deleted: deleted_tags) : error('could not delete tags')
+      end
+
       # Replace a tag on the registry with a dummy tag.
       # 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
+      # don't support fast delete.
       # See https://gitlab.com/gitlab-org/gitlab/issues/15737 for a discussion
-      def smart_delete(container_repository, tag_names)
+      def slow_delete(container_repository, tag_names)
         # generates the blobs for the dummy image
         dummy_manifest = container_repository.client.generate_empty_manifest(container_repository.path)
         return error('could not generate manifest') if dummy_manifest.nil?
@@ -36,6 +49,15 @@ module Projects
         end
       end
 
+      def smart_delete(container_repository, tag_names)
+        fast_delete_enabled = Feature.enabled?(:container_registry_fast_tag_delete, default_enabled: true)
+        if fast_delete_enabled && container_repository.client.supports_tag_delete?
+          fast_delete(container_repository, tag_names)
+        else
+          slow_delete(container_repository, tag_names)
+        end
+      end
+
       # update the manifests of the tags with the new dummy image
       def replace_tag_manifests(container_repository, dummy_manifest, tag_names)
         deleted_tags = {}

changelogs/unreleased/31832-improve-performance-of-the-container-registry-delete-tags-api.yml

+---
+title: Improve performance of the Container Registry delete tags API
+merge_request: 23325
+author:
+type: performance

lib/container_registry/client.rb

@@ -6,6 +6,8 @@ require 'digest'
 
 module ContainerRegistry
   class Client
+    include Gitlab::Utils::StrongMemoize
+
     attr_accessor :uri
 
     DOCKER_DISTRIBUTION_MANIFEST_V2_TYPE = 'application/vnd.docker.distribution.manifest.v2+json'
@@ -35,10 +37,25 @@ module ContainerRegistry
       response.headers['docker-content-digest'] if response.success?
     end
 
-    def delete_repository_tag(name, reference)
-      result = faraday.delete("/v2/#{name}/manifests/#{reference}")
+    def delete_repository_tag_by_digest(name, reference)
+      delete_if_exists("/v2/#{name}/manifests/#{reference}")
+    end
 
-      result.success? || result.status == 404
+    def delete_repository_tag_by_name(name, reference)
+      delete_if_exists("/v2/#{name}/tags/reference/#{reference}")
+    end
+
+    # Check if the registry supports tag deletion. This is only supported by the
+    # GitLab registry fork. The fastest and safest way to check this is to send
+    # an OPTIONS request to /v2/<name>/tags/reference/<tag>, using a random
+    # repository name and tag (the registry won't check if they exist).
+    # Registries that support tag deletion will reply with a 200 OK and include
+    # the DELETE method in the Allow header. Others reply with an 404 Not Found.
+    def supports_tag_delete?
+      strong_memoize(:supports_tag_delete) do
+        response = faraday.run_request(:options, '/v2/name/tags/reference/tag', '', {})
+        response.success? && response.headers['allow']&.include?('DELETE')
+      end
     end
 
     def upload_raw_blob(path, blob)
@@ -86,9 +103,7 @@ module ContainerRegistry
     end
 
     def delete_blob(name, digest)
-      result = faraday.delete("/v2/#{name}/blobs/#{digest}")
-
-      result.success? || result.status == 404
+      delete_if_exists("/v2/#{name}/blobs/#{digest}")
     end
 
     def put_tag(name, reference, manifest)
@@ -163,6 +178,12 @@ module ContainerRegistry
         conn.adapter :net_http
       end
     end
+
+    def delete_if_exists(path)
+      result = faraday.delete(path)
+
+      result.success? || result.status == 404
+    end
   end
 end
 

lib/container_registry/tag.rb

@@ -118,7 +118,7 @@ module ContainerRegistry
     def unsafe_delete
       return unless digest
 
-      client.delete_repository_tag(repository.path, digest)
+      client.delete_repository_tag_by_digest(repository.path, digest)
     end
   end
 end

spec/lib/container_registry/client_spec.rb

@@ -146,4 +146,57 @@ describe ContainerRegistry::Client do
       expect(subject).to eq 'sha256:123'
     end
   end
+
+  describe '#delete_repository_tag_by_name' do
+    subject { client.delete_repository_tag_by_name('group/test', 'a') }
+
+    context 'when the tag exists' do
+      before do
+        stub_request(:delete, "http://container-registry/v2/group/test/tags/reference/a")
+          .to_return(status: 200, body: "")
+      end
+
+      it { is_expected.to be_truthy }
+    end
+
+    context 'when the tag does not exist' do
+      before do
+        stub_request(:delete, "http://container-registry/v2/group/test/tags/reference/a")
+          .to_return(status: 404, body: "")
+      end
+
+      it { is_expected.to be_truthy }
+    end
+
+    context 'when an error occurs' do
+      before do
+        stub_request(:delete, "http://container-registry/v2/group/test/tags/reference/a")
+          .to_return(status: 500, body: "")
+      end
+
+      it { is_expected.to be_falsey }
+    end
+  end
+
+  describe '#supports_tag_delete?' do
+    subject { client.supports_tag_delete? }
+
+    context 'when the server supports tag deletion' do
+      before do
+        stub_request(:options, "http://container-registry/v2/name/tags/reference/tag")
+          .to_return(status: 200, body: "", headers: { 'Allow' => 'DELETE' })
+      end
+
+      it { is_expected.to be_truthy }
+    end
+
+    context 'when the server does not support tag deletion' do
+      before do
+        stub_request(:options, "http://container-registry/v2/name/tags/reference/tag")
+          .to_return(status: 404, body: "")
+      end
+
+      it { is_expected.to be_falsey }
+    end
+  end
 end

spec/models/container_repository_spec.rb

@@ -85,7 +85,7 @@ describe ContainerRepository do
     context 'when action succeeds' do
       it 'returns status that indicates success' do
         expect(repository.client)
-          .to receive(:delete_repository_tag)
+          .to receive(:delete_repository_tag_by_digest)
           .twice
           .and_return(true)
 
@@ -96,7 +96,7 @@ describe ContainerRepository do
     context 'when action fails' do
       it 'returns status that indicates failure' do
         expect(repository.client)
-          .to receive(:delete_repository_tag)
+          .to receive(:delete_repository_tag_by_digest)
           .twice
           .and_return(false)
 
@@ -105,6 +105,36 @@ describe ContainerRepository do
     end
   end
 
+  describe '#delete_tag_by_name' do
+    let(:repository) do
+      create(:container_repository, name: 'my_image',
+                                    tags: { latest: '123', rc1: '234' },
+                                    project: project)
+    end
+
+    context 'when action succeeds' do
+      it 'returns status that indicates success' do
+        expect(repository.client)
+          .to receive(:delete_repository_tag_by_name)
+          .with(repository.path, "latest")
+          .and_return(true)
+
+        expect(repository.delete_tag_by_name('latest')).to be_truthy
+      end
+    end
+
+    context 'when action fails' do
+      it 'returns status that indicates failure' do
+        expect(repository.client)
+          .to receive(:delete_repository_tag_by_name)
+          .with(repository.path, "latest")
+          .and_return(false)
+
+        expect(repository.delete_tag_by_name('latest')).to be_falsey
+      end
+    end
+  end
+
   describe '#location' do
     context 'when registry is running on a custom port' do
       before do

spec/services/projects/container_repository/cleanup_tags_service_spec.rb

@@ -41,7 +41,7 @@ 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)
+        expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag_by_digest)
 
         is_expected.to include(status: :success, deleted: [])
       end
@@ -156,7 +156,7 @@ describe Projects::ContainerRepository::CleanupTagsService do
 
   def expect_delete(digest)
     expect_any_instance_of(ContainerRegistry::Client)
-      .to receive(:delete_repository_tag)
+      .to receive(:delete_repository_tag_by_digest)
       .with(repository.path, digest) { true }
   end
 end

spec/services/projects/container_repository/delete_tags_service_spec.rb

@@ -18,10 +18,6 @@ describe Projects::ContainerRepository::DeleteTagsService do
     stub_container_registry_tags(
       repository: repository.path,
       tags: %w(latest A Ba Bb C D E))
-
-    stub_tag_digest('latest', 'sha256:configA')
-    stub_tag_digest('A', 'sha256:configA')
-    stub_tag_digest('Ba', 'sha256:configB')
   end
 
   describe '#execute' do
@@ -38,28 +34,123 @@ describe Projects::ContainerRepository::DeleteTagsService do
         project.add_developer(user)
       end
 
+      context 'when the registry supports fast delete' do
+        context 'and the feature is enabled' do
+          let_it_be(:project) { create(:project, :private) }
+          let_it_be(:repository) { create(:container_repository, :root, project: project) }
+
+          before do
+            allow(repository.client).to receive(:supports_tag_delete?).and_return(true)
+          end
+
+          context 'with tags to delete' do
+            let_it_be(:tags) { %w[A Ba] }
+
+            it 'deletes the tags by name' do
+              stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/tags/reference/A")
+                .to_return(status: 200, body: "")
+
+              stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/tags/reference/Ba")
+                .to_return(status: 200, body: "")
+
+              expect_delete_tag_by_name('A')
+              expect_delete_tag_by_name('Ba')
+
+              is_expected.to include(status: :success)
+            end
+
+            it 'succeeds when tag delete returns 404' do
+              stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/tags/reference/A")
+                .to_return(status: 200, body: "")
+
+              stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/tags/reference/Ba")
+                .to_return(status: 404, body: "")
+
+              is_expected.to include(status: :success)
+            end
+
+            context 'with failures' do
+              context 'when the delete request fails' do
+                before do
+                  stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/tags/reference/A")
+                  .to_return(status: 500, body: "")
+
+                  stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/tags/reference/Ba")
+                  .to_return(status: 500, body: "")
+                end
+
+                it { is_expected.to include(status: :error) }
+              end
+            end
+          end
+
+          context 'when no params are specified' do
+            let_it_be(:params) { {} }
+
+            it 'does not remove anything' do
+              expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag_by_name)
+
+              is_expected.to include(status: :error)
+            end
+          end
+
+          context 'with empty tags' do
+            let_it_be(:tags) { [] }
+
+            it 'does not remove anything' do
+              expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag_by_name)
+
+              is_expected.to include(status: :error)
+            end
+          end
+        end
+        context 'and the feature is disabled' do
+          before do
+            stub_feature_flags(container_registry_fast_tag_delete: false)
+          end
+
+          it 'fallbacks to slow delete' do
+            expect(service).not_to receive(:fast_delete)
+            expect(service).to receive(:slow_delete).with(repository, tags)
+
+            subject
+          end
+        end
+      end
+      context 'when the registry does not support fast delete' do
+        let_it_be(:project) { create(:project, :private) }
+        let_it_be(:repository) { create(:container_repository, :root, project: project) }
+
+        before do
+          stub_tag_digest('latest', 'sha256:configA')
+          stub_tag_digest('A', 'sha256:configA')
+          stub_tag_digest('Ba', 'sha256:configB')
+
+          allow(repository.client).to receive(:supports_tag_delete?).and_return(false)
+        end
+
         context 'when no params are specified' do
-        let(:params) { {} }
+          let_it_be(:params) { {} }
 
           it 'does not remove anything' do
-          expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag)
+            expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag_by_digest)
 
             is_expected.to include(status: :error)
           end
         end
 
         context 'with empty tags' do
-        let(:tags) { [] }
+          let_it_be(:tags) { [] }
 
           it 'does not remove anything' do
-          expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag)
+            expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag_by_digest)
 
             is_expected.to include(status: :error)
           end
         end
 
         context 'with tags to delete' do
-        let(:tags) { %w[A Ba] }
+          let_it_be(:tags) { %w[A Ba] }
 
           it 'deletes the tags using a dummy image' do
             stub_upload("{\n  \"config\": {\n  }\n}", 'sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3')
@@ -70,12 +161,12 @@ describe Projects::ContainerRepository::DeleteTagsService do
             stub_request(:put, "http://registry.gitlab/v2/#{repository.path}/manifests/Ba")
               .to_return(status: 200, body: "", headers: { 'docker-content-digest' => 'sha256:dummy' })
 
-          expect_delete_tag('sha256:dummy')
+            expect_delete_tag_by_digest('sha256:dummy')
 
             is_expected.to include(status: :success)
           end
 
-        it 'succedes when tag delete returns 404' do
+          it 'succeeds when tag delete returns 404' do
             stub_upload("{\n  \"config\": {\n  }\n}", 'sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3')
 
             stub_request(:put, "http://registry.gitlab/v2/#{repository.path}/manifests/A")
@@ -119,6 +210,7 @@ describe Projects::ContainerRepository::DeleteTagsService do
         end
       end
     end
+  end
 
   private
 
@@ -141,9 +233,21 @@ describe Projects::ContainerRepository::DeleteTagsService do
       .with(repository.path, content, digest) { double(success?: success ) }
   end
 
-  def expect_delete_tag(digest)
+  def expect_delete_tag_by_digest(digest)
     expect_any_instance_of(ContainerRegistry::Client)
-      .to receive(:delete_repository_tag)
+      .to receive(:delete_repository_tag_by_digest)
       .with(repository.path, digest) { true }
+
+    expect_any_instance_of(ContainerRegistry::Client)
+      .not_to receive(:delete_repository_tag_by_name)
+  end
+
+  def expect_delete_tag_by_name(name)
+    expect_any_instance_of(ContainerRegistry::Client)
+      .to receive(:delete_repository_tag_by_name)
+      .with(repository.path, name) { true }
+
+    expect_any_instance_of(ContainerRegistry::Client)
+      .not_to receive(:delete_repository_tag_by_digest)
   end
 end