Commit d9c0c265 authored by Stan Hu's avatar Stan Hu
Browse files

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 1f2f1477697b
......@@ -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));
},
......
......@@ -18,14 +18,10 @@ def index
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 @@ def image
# 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
......
# 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 @@
- authorized_projects
- background_migration
- create_gpg_signature
- delete_container_repository
- delete_merged_branches
- delete_user
- 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 @@
- [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]
......
......@@ -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 ""
......
......@@ -86,9 +86,10 @@
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
......
# 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