Delete a container registry asynchronously

When a container registry has many tags, it's easy for the DELETE call to take more
than 60 seconds and fail. This can also leave the registry in a bad state with
null bytes since some of the images have been deleted with tags still pointing to them.
In addition, we have to prevent users from accidentally initiating the delete multiple
times or this could leave the registry with orphaned tags.

This commit also adds a flash message to notify the user the registry is scheduled
for deletion.

Closes #49926, #51063

11 files changed, 144 insertions, 13 deletions

diff --git a/app/assets/javascripts/registry/components/collapsible_container.vue b/app/assets/javascripts/registry/components/collapsible_container.vue
index 4116c4a0489..cea409aa130 100644
--- a/app/assets/javascripts/registry/components/collapsible_container.vue
+++ b/app/assets/javascripts/registry/components/collapsible_container.vue
@@ -6,6 +6,7 @@
   import tooltip from '../../vue_shared/directives/tooltip';
   import tableRegistry from './table_registry.vue';
   import { errorMessages, errorMessagesTypes } from '../constants';
+  import { __ } from '../../locale';
 
   export default {
     name: 'CollapsibeContainerRegisty',
@@ -46,7 +47,10 @@
 
       handleDeleteRepository() {
         this.deleteRepo(this.repo)
-          .then(() => this.fetchRepos())
+          .then(() => {
+            Flash(__('This container registry has been scheduled for deletion.'), 'notice');
+            this.fetchRepos();
+          })
           .catch(() => this.showError(errorMessagesTypes.DELETE_REPO));
       },
 
diff --git a/app/controllers/projects/registry/repositories_controller.rb b/app/controllers/projects/registry/repositories_controller.rb
index 32c0fc6d14a..ef0433795f4 100644
--- a/app/controllers/projects/registry/repositories_controller.rb
+++ b/app/controllers/projects/registry/repositories_controller.rb
@@ -18,14 +18,10 @@ module Projects
       end
 
       def destroy
-        if image.destroy
-          respond_to do |format|
-            format.json { head :no_content }
-          end
-        else
-          respond_to do |format|
-            format.json { head :bad_request }
-          end
+        DeleteContainerRepositoryWorker.perform_async(current_user.id, image.id)
+
+        respond_to do |format|
+          format.json { head :no_content }
         end
       end
 
@@ -41,10 +37,10 @@ module Projects
       # Needed to maintain a backwards compatibility.
       #
       def ensure_root_container_repository!
-        ContainerRegistry::Path.new(@project.full_path).tap do |path|
+        ::ContainerRegistry::Path.new(@project.full_path).tap do |path|
           break if path.has_repository?
 
-          ContainerRepository.build_from_path(path).tap do |repository|
+          ::ContainerRepository.build_from_path(path).tap do |repository|
             repository.save! if repository.has_tags?
           end
         end
diff --git a/app/services/projects/container_repository/destroy_service.rb b/app/services/projects/container_repository/destroy_service.rb
new file mode 100644
index 00000000000..a8e7eab6068
--- /dev/null
+++ b/app/services/projects/container_repository/destroy_service.rb
@@ -0,0 +1,13 @@
+# frozen_string_literal: true
+
+module Projects
+  module ContainerRepository
+    class DestroyService < BaseService
+      def execute(container_repository)
+        return false unless can?(current_user, :update_container_image, project)
+
+        container_repository.destroy
+      end
+    end
+  end
+end
diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml
index ae9dc8d4857..1eeb972cee9 100644
--- a/app/workers/all_queues.yml
+++ b/app/workers/all_queues.yml
@@ -87,6 +87,7 @@
 - authorized_projects
 - background_migration
 - create_gpg_signature
+- delete_container_repository
 - delete_merged_branches
 - delete_user
 - email_receiver
diff --git a/app/workers/delete_container_repository_worker.rb b/app/workers/delete_container_repository_worker.rb
new file mode 100644
index 00000000000..b703530d3a0
--- /dev/null
+++ b/app/workers/delete_container_repository_worker.rb
@@ -0,0 +1,34 @@
+# frozen_string_literal: true
+
+class DeleteContainerRepositoryWorker
+  include ApplicationWorker
+  include ExclusiveLeaseGuard
+
+  LEASE_TIMEOUT = 1.hour
+
+  attr_reader :container_repository
+
+  def perform(current_user_id, container_repository_id)
+    current_user = User.find_by(id: current_user_id)
+    @container_repository = ContainerRepository.find_by(id: container_repository_id)
+    project = container_repository&.project
+
+    return unless current_user && container_repository && project
+
+    # If a user accidentally attempts to delete the same container registry in quick succession,
+    # this can lead to orphaned tags.
+    try_obtain_lease do
+      Projects::ContainerRepository::DestroyService.new(project, current_user).execute(container_repository)
+    end
+  end
+
+  # For ExclusiveLeaseGuard concern
+  def lease_key
+    @lease_key ||= "container_repository:delete:#{container_repository.id}"
+  end
+
+  # For ExclusiveLeaseGuard concern
+  def lease_timeout
+    LEASE_TIMEOUT
+  end
+end
diff --git a/changelogs/unreleased/sh-delete-container-registry-async.yml b/changelogs/unreleased/sh-delete-container-registry-async.yml
new file mode 100644
index 00000000000..dfe0e812112
--- /dev/null
+++ b/changelogs/unreleased/sh-delete-container-registry-async.yml
@@ -0,0 +1,5 @@
+---
+title: Delete a container registry asynchronously
+merge_request: 21553
+author:
+type: fixed
diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml
index dc49403aca1..0e723cdeb9c 100644
--- a/config/sidekiq_queues.yml
+++ b/config/sidekiq_queues.yml
@@ -46,6 +46,7 @@
   - [project_service, 1]
   - [delete_user, 1]
   - [todos_destroyer, 1]
+  - [delete_container_repository, 1]
   - [delete_merged_branches, 1]
   - [authorized_projects, 1]
   - [expire_build_instance_artifacts, 1]
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 8b19e2bec61..d4d7913e65a 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -5832,6 +5832,9 @@ msgstr ""
 msgid "This branch has changed since you started editing. Would you like to create a new branch?"
 msgstr ""
 
+msgid "This container registry has been scheduled for deletion."
+msgstr ""
+
 msgid "This diff is collapsed."
 msgstr ""
 
diff --git a/spec/controllers/projects/registry/repositories_controller_spec.rb b/spec/controllers/projects/registry/repositories_controller_spec.rb
index 17769a14def..d11e42b411b 100644
--- a/spec/controllers/projects/registry/repositories_controller_spec.rb
+++ b/spec/controllers/projects/registry/repositories_controller_spec.rb
@@ -86,9 +86,10 @@ describe Projects::Registry::RepositoriesController do
           stub_container_registry_tags(repository: :any, tags: [])
         end
 
-        it 'deletes a repository' do
-          expect { delete_repository(repository) }.to change { ContainerRepository.all.count }.by(-1)
+        it 'schedules a job to delete a repository' do
+          expect(DeleteContainerRepositoryWorker).to receive(:perform_async).with(user.id, repository.id)
 
+          delete_repository(repository)
           expect(response).to have_gitlab_http_status(:no_content)
         end
       end
diff --git a/spec/services/projects/container_repository/destroy_service_spec.rb b/spec/services/projects/container_repository/destroy_service_spec.rb
new file mode 100644
index 00000000000..307ccc88865
--- /dev/null
+++ b/spec/services/projects/container_repository/destroy_service_spec.rb
@@ -0,0 +1,40 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Projects::ContainerRepository::DestroyService do
+  set(:user) { create(:user) }
+  set(:project) { create(:project, :private) }
+
+  subject { described_class.new(project, user) }
+
+  before do
+    stub_container_registry_config(enabled: true)
+  end
+
+  context 'when user does not have access to registry' do
+    let!(:repository) { create(:container_repository, :root, project: project) }
+
+    it 'does not delete a repository' do
+      expect { subject.execute(repository) }.not_to change { ContainerRepository.all.count }
+    end
+  end
+
+  context 'when user has access to registry' do
+    before do
+      project.add_developer(user)
+    end
+
+    context 'when root container repository exists' do
+      let!(:repository) { create(:container_repository, :root, project: project) }
+
+      before do
+        stub_container_registry_tags(repository: :any, tags: [])
+      end
+
+      it 'deletes the repository' do
+        expect { described_class.new(project, user).execute(repository) }.to change { ContainerRepository.all.count }.by(-1)
+      end
+    end
+  end
+end
diff --git a/spec/workers/delete_container_repository_worker_spec.rb b/spec/workers/delete_container_repository_worker_spec.rb
new file mode 100644
index 00000000000..8c40611a959
--- /dev/null
+++ b/spec/workers/delete_container_repository_worker_spec.rb
@@ -0,0 +1,33 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe DeleteContainerRepositoryWorker do
+  let(:registry) { create(:container_repository) }
+  let(:project) { registry.project }
+  let(:user) { project.owner }
+
+  subject { described_class.new }
+
+  describe '#perform' do
+    it 'executes the destroy service' do
+      service = instance_double(Projects::ContainerRepository::DestroyService)
+      expect(service).to receive(:execute)
+      expect(Projects::ContainerRepository::DestroyService).to receive(:new).with(project, user).and_return(service)
+
+      subject.perform(user.id, registry.id)
+    end
+
+    it 'does not raise error when user could not be found' do
+      expect do
+        subject.perform(-1, registry.id)
+      end.not_to raise_error
+    end
+
+    it 'does not raise error when registry could not be found' do
+      expect do
+        subject.perform(user.id, -1)
+      end.not_to raise_error
+    end
+  end
+end