Commit 5830d114 authored by Stan Hu's avatar Stan Hu

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
parent 272281e4
...@@ -6,6 +6,7 @@ ...@@ -6,6 +6,7 @@
import tooltip from '../../vue_shared/directives/tooltip'; import tooltip from '../../vue_shared/directives/tooltip';
import tableRegistry from './table_registry.vue'; import tableRegistry from './table_registry.vue';
import { errorMessages, errorMessagesTypes } from '../constants'; import { errorMessages, errorMessagesTypes } from '../constants';
import { __ } from '../../locale';
export default { export default {
name: 'CollapsibeContainerRegisty', name: 'CollapsibeContainerRegisty',
...@@ -46,7 +47,10 @@ ...@@ -46,7 +47,10 @@
handleDeleteRepository() { handleDeleteRepository() {
this.deleteRepo(this.repo) 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)); .catch(() => this.showError(errorMessagesTypes.DELETE_REPO));
}, },
......
...@@ -18,14 +18,10 @@ module Projects ...@@ -18,14 +18,10 @@ module Projects
end end
def destroy def destroy
if image.destroy DeleteContainerRepositoryWorker.perform_async(current_user.id, image.id)
respond_to do |format|
format.json { head :no_content } respond_to do |format|
end format.json { head :no_content }
else
respond_to do |format|
format.json { head :bad_request }
end
end end
end end
...@@ -41,10 +37,10 @@ module Projects ...@@ -41,10 +37,10 @@ module Projects
# Needed to maintain a backwards compatibility. # Needed to maintain a backwards compatibility.
# #
def ensure_root_container_repository! 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? 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? repository.save! if repository.has_tags?
end end
end end
......
# 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
...@@ -87,6 +87,7 @@ ...@@ -87,6 +87,7 @@
- authorized_projects - authorized_projects
- background_migration - background_migration
- create_gpg_signature - create_gpg_signature
- delete_container_repository
- delete_merged_branches - delete_merged_branches
- delete_user - delete_user
- email_receiver - email_receiver
......
# 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
---
title: Delete a container registry asynchronously
merge_request: 21553
author:
type: fixed
...@@ -46,6 +46,7 @@ ...@@ -46,6 +46,7 @@
- [project_service, 1] - [project_service, 1]
- [delete_user, 1] - [delete_user, 1]
- [todos_destroyer, 1] - [todos_destroyer, 1]
- [delete_container_repository, 1]
- [delete_merged_branches, 1] - [delete_merged_branches, 1]
- [authorized_projects, 1] - [authorized_projects, 1]
- [expire_build_instance_artifacts, 1] - [expire_build_instance_artifacts, 1]
......
...@@ -5832,6 +5832,9 @@ msgstr "" ...@@ -5832,6 +5832,9 @@ msgstr ""
msgid "This branch has changed since you started editing. Would you like to create a new branch?" msgid "This branch has changed since you started editing. Would you like to create a new branch?"
msgstr "" msgstr ""
msgid "This container registry has been scheduled for deletion."
msgstr ""
msgid "This diff is collapsed." msgid "This diff is collapsed."
msgstr "" msgstr ""
......
...@@ -86,9 +86,10 @@ describe Projects::Registry::RepositoriesController do ...@@ -86,9 +86,10 @@ describe Projects::Registry::RepositoriesController do
stub_container_registry_tags(repository: :any, tags: []) stub_container_registry_tags(repository: :any, tags: [])
end end
it 'deletes a repository' do it 'schedules a job to delete a repository' do
expect { delete_repository(repository) }.to change { ContainerRepository.all.count }.by(-1) expect(DeleteContainerRepositoryWorker).to receive(:perform_async).with(user.id, repository.id)
delete_repository(repository)
expect(response).to have_gitlab_http_status(:no_content) expect(response).to have_gitlab_http_status(:no_content)
end end
end end
......
# 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
# 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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment