Commit e7df8070 authored by Robert Speicher's avatar Robert Speicher

Merge branch 'sh-delete-tags-outside-transaction' into 'master'

Delete container repository tags outside of transaction

Closes #51380

See merge request gitlab-org/gitlab-ce!21679
parents 3f51ec21 311beef2
...@@ -8,8 +8,6 @@ class ContainerRepository < ActiveRecord::Base ...@@ -8,8 +8,6 @@ class ContainerRepository < ActiveRecord::Base
delegate :client, to: :registry delegate :client, to: :registry
before_destroy :delete_tags!
# rubocop: disable CodeReuse/ServiceClass # rubocop: disable CodeReuse/ServiceClass
def registry def registry
@registry ||= begin @registry ||= begin
......
...@@ -6,6 +6,8 @@ module Projects ...@@ -6,6 +6,8 @@ module Projects
def execute(container_repository) def execute(container_repository)
return false unless can?(current_user, :update_container_image, project) return false unless can?(current_user, :update_container_image, project)
# Delete tags outside of the transaction to avoid hitting an idle-in-transaction timeout
container_repository.delete_tags!
container_repository.destroy container_repository.destroy
end end
end end
......
...@@ -133,11 +133,11 @@ module Projects ...@@ -133,11 +133,11 @@ module Projects
end end
def attempt_destroy_transaction(project) def attempt_destroy_transaction(project)
Project.transaction do unless remove_registry_tags
unless remove_legacy_registry_tags
raise_error('Failed to remove some tags in project container registry. Please try again or contact administrator.') raise_error('Failed to remove some tags in project container registry. Please try again or contact administrator.')
end end
Project.transaction do
log_destroy_event log_destroy_event
trash_repositories! trash_repositories!
...@@ -156,6 +156,17 @@ module Projects ...@@ -156,6 +156,17 @@ module Projects
log_info("Attempting to destroy #{project.full_path} (#{project.id})") log_info("Attempting to destroy #{project.full_path} (#{project.id})")
end end
def remove_registry_tags
return false unless remove_legacy_registry_tags
project.container_repositories.find_each do |container_repository|
service = Projects::ContainerRepository::DestroyService.new(project, current_user)
service.execute(container_repository)
end
true
end
## ##
# This method makes sure that we correctly remove registry tags # This method makes sure that we correctly remove registry tags
# for legacy image repository (when repository path equals project path). # for legacy image repository (when repository path equals project path).
......
---
title: Delete container repository tags outside of transaction
merge_request: 21679
author:
type: fixed
...@@ -33,6 +33,7 @@ describe Projects::ContainerRepository::DestroyService do ...@@ -33,6 +33,7 @@ describe Projects::ContainerRepository::DestroyService do
end end
it 'deletes the repository' do it 'deletes the repository' do
expect(repository).to receive(:delete_tags!).and_call_original
expect { described_class.new(project, user).execute(repository) }.to change { ContainerRepository.all.count }.by(-1) expect { described_class.new(project, user).execute(repository) }.to change { ContainerRepository.all.count }.by(-1)
end end
end end
......
...@@ -204,7 +204,7 @@ describe Projects::DestroyService do ...@@ -204,7 +204,7 @@ describe Projects::DestroyService do
context 'when image repository deletion fails' do context 'when image repository deletion fails' do
it 'raises an exception' do it 'raises an exception' do
expect_any_instance_of(ContainerRepository) expect_any_instance_of(ContainerRepository)
.to receive(:delete_tags!).and_return(false) .to receive(:delete_tags!).and_raise(RuntimeError)
expect(destroy_project(project, user)).to be false expect(destroy_project(project, user)).to be false
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