From 76ab17ae2fb78f0b5f951a8c210df23dd03453f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Pereira?= Date: Mon, 20 Jan 2020 13:18:47 +0000 Subject: [PATCH] Improve performance of the Container Registry delete tags API Use the new DELETE /v2//tags/reference/ endpoint provided by the GitLab Container Registry to delete a tag by name instead of digest (used as fallback for third-party registries). --- app/models/container_repository.rb | 6 +- .../delete_tags_service.rb | 24 +- ...the-container-registry-delete-tags-api.yml | 5 + lib/container_registry/client.rb | 33 ++- lib/container_registry/tag.rb | 2 +- spec/lib/container_registry/client_spec.rb | 53 +++++ spec/models/container_repository_spec.rb | 34 ++- .../cleanup_tags_service_spec.rb | 4 +- .../delete_tags_service_spec.rb | 214 +++++++++++++----- 9 files changed, 307 insertions(+), 68 deletions(-) create mode 100644 changelogs/unreleased/31832-improve-performance-of-the-container-registry-delete-tags-api.yml diff --git a/app/models/container_repository.rb b/app/models/container_repository.rb index 152aa7b3218e39..fcbfda8fbc270b 100644 --- a/app/models/container_repository.rb +++ b/app/models/container_repository.rb @@ -77,7 +77,11 @@ def delete_tags! 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) diff --git a/app/services/projects/container_repository/delete_tags_service.rb b/app/services/projects/container_repository/delete_tags_service.rb index 88ff3c2c9dfeca..d19f275e928cd0 100644 --- a/app/services/projects/container_repository/delete_tags_service.rb +++ b/app/services/projects/container_repository/delete_tags_service.rb @@ -14,12 +14,25 @@ def execute(container_repository) 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 @@ def smart_delete(container_repository, tag_names) 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 = {} diff --git a/changelogs/unreleased/31832-improve-performance-of-the-container-registry-delete-tags-api.yml b/changelogs/unreleased/31832-improve-performance-of-the-container-registry-delete-tags-api.yml new file mode 100644 index 00000000000000..a803f08b884dc5 --- /dev/null +++ b/changelogs/unreleased/31832-improve-performance-of-the-container-registry-delete-tags-api.yml @@ -0,0 +1,5 @@ +--- +title: Improve performance of the Container Registry delete tags API +merge_request: 23325 +author: +type: performance diff --git a/lib/container_registry/client.rb b/lib/container_registry/client.rb index bc0347f6ea1d9d..12f7f04634fcfa 100644 --- a/lib/container_registry/client.rb +++ b/lib/container_registry/client.rb @@ -6,6 +6,8 @@ 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 @@ def repository_tag_digest(name, reference) 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//tags/reference/, 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 @@ def blob(name, digest, type = nil) 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 @@ def faraday_redirect conn.adapter :net_http end end + + def delete_if_exists(path) + result = faraday.delete(path) + + result.success? || result.status == 404 + end end end diff --git a/lib/container_registry/tag.rb b/lib/container_registry/tag.rb index 3c308258a3f7f9..e1a2891e43a929 100644 --- a/lib/container_registry/tag.rb +++ b/lib/container_registry/tag.rb @@ -118,7 +118,7 @@ def total_size 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 diff --git a/spec/lib/container_registry/client_spec.rb b/spec/lib/container_registry/client_spec.rb index a493b96b1e444c..5d2334a6d8f9ec 100644 --- a/spec/lib/container_registry/client_spec.rb +++ b/spec/lib/container_registry/client_spec.rb @@ -146,4 +146,57 @@ def stub_upload(path, content, digest, status = 200) 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 diff --git a/spec/models/container_repository_spec.rb b/spec/models/container_repository_spec.rb index 0a3065140bf859..5ed812652c5e78 100644 --- a/spec/models/container_repository_spec.rb +++ b/spec/models/container_repository_spec.rb @@ -85,7 +85,7 @@ 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 @@ 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 @@ 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 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 78b969c8a0eab5..cd4d1e3fe67110 100644 --- a/spec/services/projects/container_repository/cleanup_tags_service_spec.rb +++ b/spec/services/projects/container_repository/cleanup_tags_service_spec.rb @@ -41,7 +41,7 @@ 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 @@ def stub_digest_config(digest, created_at) 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 diff --git a/spec/services/projects/container_repository/delete_tags_service_spec.rb b/spec/services/projects/container_repository/delete_tags_service_spec.rb index decbbb7597fa8c..e17e4b6f7c9fd5 100644 --- a/spec/services/projects/container_repository/delete_tags_service_spec.rb +++ b/spec/services/projects/container_repository/delete_tags_service_spec.rb @@ -18,10 +18,6 @@ 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,82 +34,178 @@ project.add_developer(user) end - context 'when no params are specified' do - let(:params) { {} } + 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) } - it 'does not remove anything' do - expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag) + before do + allow(repository.client).to receive(:supports_tag_delete?).and_return(true) + end - is_expected.to include(status: :error) - end - end + context 'with tags to delete' do + let_it_be(:tags) { %w[A Ba] } - context 'with empty tags' do - let(:tags) { [] } + it 'deletes the tags by name' do + stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/tags/reference/A") + .to_return(status: 200, body: "") - it 'does not remove anything' do - expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag) + stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/tags/reference/Ba") + .to_return(status: 200, body: "") - is_expected.to include(status: :error) - end - end + 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 - context 'with tags to delete' do - let(:tags) { %w[A Ba] } + 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 - it 'deletes the tags using a dummy image' do - stub_upload("{\n \"config\": {\n }\n}", 'sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3') + context 'with empty tags' do + let_it_be(:tags) { [] } - stub_request(:put, "http://registry.gitlab/v2/#{repository.path}/manifests/A") - .to_return(status: 200, body: "", headers: { 'docker-content-digest' => 'sha256:dummy' }) + it 'does not remove anything' do + expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag_by_name) - stub_request(:put, "http://registry.gitlab/v2/#{repository.path}/manifests/Ba") - .to_return(status: 200, body: "", headers: { 'docker-content-digest' => 'sha256:dummy' }) + 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 - expect_delete_tag('sha256:dummy') + it 'fallbacks to slow delete' do + expect(service).not_to receive(:fast_delete) + expect(service).to receive(:slow_delete).with(repository, tags) - is_expected.to include(status: :success) + 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) } - it 'succedes when tag delete returns 404' do - stub_upload("{\n \"config\": {\n }\n}", 'sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3') + before do + stub_tag_digest('latest', 'sha256:configA') + stub_tag_digest('A', 'sha256:configA') + stub_tag_digest('Ba', 'sha256:configB') - stub_request(:put, "http://registry.gitlab/v2/#{repository.path}/manifests/A") - .to_return(status: 200, body: "", headers: { 'docker-content-digest' => 'sha256:dummy' }) + allow(repository.client).to receive(:supports_tag_delete?).and_return(false) + end - stub_request(:put, "http://registry.gitlab/v2/#{repository.path}/manifests/Ba") - .to_return(status: 200, body: "", headers: { 'docker-content-digest' => 'sha256:dummy' }) + context 'when no params are specified' do + let_it_be(:params) { {} } - stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/manifests/sha256:dummy") - .to_return(status: 404, body: "", headers: {}) + it 'does not remove anything' do + expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag_by_digest) - is_expected.to include(status: :success) + is_expected.to include(status: :error) + end end - context 'with failures' do - context 'when the dummy manifest generation fails' do - before do - stub_upload("{\n \"config\": {\n }\n}", 'sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3', success: false) - 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_digest) + + is_expected.to include(status: :error) + end + end + + context 'with tags to delete' do + let_it_be(:tags) { %w[A Ba] } - it { is_expected.to include(status: :error) } + it 'deletes the tags using a dummy image' do + stub_upload("{\n \"config\": {\n }\n}", 'sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3') + + stub_request(:put, "http://registry.gitlab/v2/#{repository.path}/manifests/A") + .to_return(status: 200, body: "", headers: { 'docker-content-digest' => 'sha256:dummy' }) + + 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_by_digest('sha256:dummy') + + is_expected.to include(status: :success) end - context 'when updating the tags fails' do - before do - stub_upload("{\n \"config\": {\n }\n}", 'sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3') + 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") - .to_return(status: 500, body: "", headers: { 'docker-content-digest' => 'sha256:dummy' }) + stub_request(:put, "http://registry.gitlab/v2/#{repository.path}/manifests/A") + .to_return(status: 200, body: "", headers: { 'docker-content-digest' => 'sha256:dummy' }) - stub_request(:put, "http://registry.gitlab/v2/#{repository.path}/manifests/Ba") - .to_return(status: 500, body: "", headers: { 'docker-content-digest' => 'sha256:dummy' }) + stub_request(:put, "http://registry.gitlab/v2/#{repository.path}/manifests/Ba") + .to_return(status: 200, body: "", headers: { 'docker-content-digest' => 'sha256:dummy' }) + + stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/manifests/sha256:dummy") + .to_return(status: 404, body: "", headers: {}) + + is_expected.to include(status: :success) + end - stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/manifests/sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3") - .to_return(status: 200, body: "", headers: {}) + context 'with failures' do + context 'when the dummy manifest generation fails' do + before do + stub_upload("{\n \"config\": {\n }\n}", 'sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3', success: false) + end + + it { is_expected.to include(status: :error) } end - it { is_expected.to include(status: :error) } + context 'when updating the tags fails' do + before do + stub_upload("{\n \"config\": {\n }\n}", 'sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3') + + stub_request(:put, "http://registry.gitlab/v2/#{repository.path}/manifests/A") + .to_return(status: 500, body: "", headers: { 'docker-content-digest' => 'sha256:dummy' }) + + stub_request(:put, "http://registry.gitlab/v2/#{repository.path}/manifests/Ba") + .to_return(status: 500, body: "", headers: { 'docker-content-digest' => 'sha256:dummy' }) + + stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/manifests/sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3") + .to_return(status: 200, body: "", headers: {}) + end + + it { is_expected.to include(status: :error) } + end end end end @@ -141,9 +233,21 @@ def stub_upload(content, digest, success: true) .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 -- GitLab