From a1912ccc894386b112faba2932f1dd98c03aea0e Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Thu, 13 Sep 2018 22:02:04 -0700 Subject: [PATCH 1/2] Delete container repository tags outside of transaction When there are many tags in a container repository, deleting them can exceed the default 60 second idle-in-transaction timeout in Sidekiq. We now explicitly delete them in the DestroyService before destroying the model. Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/51380 --- .../projects/container_repository/destroy_service.rb | 2 ++ changelogs/unreleased/sh-delete-tags-outside-transaction.yml | 5 +++++ spec/features/container_registry_spec.rb | 2 +- .../projects/container_repository/destroy_service_spec.rb | 1 + 4 files changed, 9 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/sh-delete-tags-outside-transaction.yml diff --git a/app/services/projects/container_repository/destroy_service.rb b/app/services/projects/container_repository/destroy_service.rb index a8e7eab6068..1f5af7970d6 100644 --- a/app/services/projects/container_repository/destroy_service.rb +++ b/app/services/projects/container_repository/destroy_service.rb @@ -6,6 +6,8 @@ module Projects def execute(container_repository) 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 end end diff --git a/changelogs/unreleased/sh-delete-tags-outside-transaction.yml b/changelogs/unreleased/sh-delete-tags-outside-transaction.yml new file mode 100644 index 00000000000..974da70251e --- /dev/null +++ b/changelogs/unreleased/sh-delete-tags-outside-transaction.yml @@ -0,0 +1,5 @@ +--- +title: Delete container repository tags outside of transaction +merge_request: 21679 +author: +type: fixed diff --git a/spec/features/container_registry_spec.rb b/spec/features/container_registry_spec.rb index 9986206f619..0dddf6d3ec5 100644 --- a/spec/features/container_registry_spec.rb +++ b/spec/features/container_registry_spec.rb @@ -39,7 +39,7 @@ describe "Container Registry", :js do visit_container_registry expect_any_instance_of(ContainerRepository) - .to receive(:delete_tags!).and_return(true) + .to receive(:delete_tags!).twice.and_return(true) click_on(class: 'js-remove-repo') end diff --git a/spec/services/projects/container_repository/destroy_service_spec.rb b/spec/services/projects/container_repository/destroy_service_spec.rb index 307ccc88865..af54e1b15e3 100644 --- a/spec/services/projects/container_repository/destroy_service_spec.rb +++ b/spec/services/projects/container_repository/destroy_service_spec.rb @@ -33,6 +33,7 @@ describe Projects::ContainerRepository::DestroyService do end it 'deletes the repository' do + expect(repository).to receive(:delete_tags!).twice.and_call_original expect { described_class.new(project, user).execute(repository) }.to change { ContainerRepository.all.count }.by(-1) end end From 311beef242ae4fc3dd666542ca01938fb9d0e15d Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Wed, 19 Sep 2018 05:37:02 -0700 Subject: [PATCH 2/2] Move registry destroy out of project transaction --- app/models/container_repository.rb | 2 -- app/services/projects/destroy_service.rb | 19 +++++++++++++++---- spec/features/container_registry_spec.rb | 2 +- .../destroy_service_spec.rb | 2 +- .../services/projects/destroy_service_spec.rb | 2 +- 5 files changed, 18 insertions(+), 9 deletions(-) diff --git a/app/models/container_repository.rb b/app/models/container_repository.rb index 93ca3a5daf4..2c08a8e1acf 100644 --- a/app/models/container_repository.rb +++ b/app/models/container_repository.rb @@ -8,8 +8,6 @@ class ContainerRepository < ActiveRecord::Base delegate :client, to: :registry - before_destroy :delete_tags! - # rubocop: disable CodeReuse/ServiceClass def registry @registry ||= begin diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb index 5090ebf8f51..210571b6b4e 100644 --- a/app/services/projects/destroy_service.rb +++ b/app/services/projects/destroy_service.rb @@ -133,11 +133,11 @@ module Projects end def attempt_destroy_transaction(project) - Project.transaction do - unless remove_legacy_registry_tags - raise_error('Failed to remove some tags in project container registry. Please try again or contact administrator.') - end + unless remove_registry_tags + raise_error('Failed to remove some tags in project container registry. Please try again or contact administrator.') + end + Project.transaction do log_destroy_event trash_repositories! @@ -156,6 +156,17 @@ module Projects log_info("Attempting to destroy #{project.full_path} (#{project.id})") 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 # for legacy image repository (when repository path equals project path). diff --git a/spec/features/container_registry_spec.rb b/spec/features/container_registry_spec.rb index 0dddf6d3ec5..9986206f619 100644 --- a/spec/features/container_registry_spec.rb +++ b/spec/features/container_registry_spec.rb @@ -39,7 +39,7 @@ describe "Container Registry", :js do visit_container_registry expect_any_instance_of(ContainerRepository) - .to receive(:delete_tags!).twice.and_return(true) + .to receive(:delete_tags!).and_return(true) click_on(class: 'js-remove-repo') end diff --git a/spec/services/projects/container_repository/destroy_service_spec.rb b/spec/services/projects/container_repository/destroy_service_spec.rb index af54e1b15e3..affcc66d2bb 100644 --- a/spec/services/projects/container_repository/destroy_service_spec.rb +++ b/spec/services/projects/container_repository/destroy_service_spec.rb @@ -33,7 +33,7 @@ describe Projects::ContainerRepository::DestroyService do end it 'deletes the repository' do - expect(repository).to receive(:delete_tags!).twice.and_call_original + expect(repository).to receive(:delete_tags!).and_call_original expect { described_class.new(project, user).execute(repository) }.to change { ContainerRepository.all.count }.by(-1) end end diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb index e428808ab68..beff499f2be 100644 --- a/spec/services/projects/destroy_service_spec.rb +++ b/spec/services/projects/destroy_service_spec.rb @@ -204,7 +204,7 @@ describe Projects::DestroyService do context 'when image repository deletion fails' do it 'raises an exception' do 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 end