Commit 013ca207 authored by Allison Browne's avatar Allison Browne Committed by Douglas Barbosa Alexandre

Remove project destory transaction behind flag

Remove project destroy transaction so that the job can continue
deleting the objects it did not get to
parent 3cb408f1
...@@ -28,7 +28,7 @@ module Projects ...@@ -28,7 +28,7 @@ module Projects
Projects::UnlinkForkService.new(project, current_user).execute Projects::UnlinkForkService.new(project, current_user).execute
attempt_destroy_transaction(project) attempt_destroy(project)
system_hook_service.execute_hooks_for(project, :destroy) system_hook_service.execute_hooks_for(project, :destroy)
log_info("Project \"#{project.full_path}\" was deleted") log_info("Project \"#{project.full_path}\" was deleted")
...@@ -98,29 +98,35 @@ module Projects ...@@ -98,29 +98,35 @@ module Projects
log_error("Deletion failed on #{project.full_path} with the following message: #{message}") log_error("Deletion failed on #{project.full_path} with the following message: #{message}")
end end
def attempt_destroy_transaction(project) def attempt_destroy(project)
unless remove_registry_tags unless remove_registry_tags
raise_error(s_('DeleteProject|Failed to remove some tags in project container registry. Please try again or contact administrator.')) raise_error(s_('DeleteProject|Failed to remove some tags in project container registry. Please try again or contact administrator.'))
end end
project.leave_pool_repository project.leave_pool_repository
Project.transaction do if Gitlab::Ci::Features.project_transactionless_destroy?(project)
log_destroy_event destroy_project_related_records(project)
trash_relation_repositories! else
trash_project_repositories! Project.transaction { destroy_project_related_records(project) }
# Rails attempts to load all related records into memory before
# destroying: https://github.com/rails/rails/issues/22510
# This ensures we delete records in batches.
#
# Exclude container repositories because its before_destroy would be
# called multiple times, and it doesn't destroy any database records.
project.destroy_dependent_associations_in_batches(exclude: [:container_repositories, :snippets])
project.destroy!
end end
end end
def destroy_project_related_records(project)
log_destroy_event
trash_relation_repositories!
trash_project_repositories!
# Rails attempts to load all related records into memory before
# destroying: https://github.com/rails/rails/issues/22510
# This ensures we delete records in batches.
#
# Exclude container repositories because its before_destroy would be
# called multiple times, and it doesn't destroy any database records.
project.destroy_dependent_associations_in_batches(exclude: [:container_repositories, :snippets])
project.destroy!
end
def log_destroy_event def log_destroy_event
log_info("Attempting to destroy #{project.full_path} (#{project.id})") log_info("Attempting to destroy #{project.full_path} (#{project.id})")
end end
......
...@@ -17,10 +17,30 @@ module EE ...@@ -17,10 +17,30 @@ module EE
end end
end end
override :log_destroy_event # Removes physical repository in a Geo replicated secondary node
def log_destroy_event # There is no need to do any database operation as it will be
super # replicated by itself.
def geo_replicate
return unless ::Gitlab::Geo.secondary?
# Flush the cache for both repositories. This has to be done _before_
# removing the physical repositories as some expiration code depends on
# Git data (e.g. a list of branch names).
flush_caches(project)
trash_project_repositories!
log_info("Project \"#{project.name}\" was removed")
end
private
override :destroy_project_related_records
def destroy_project_related_records(project)
super && log_destroy_events
end
def log_destroy_events
log_geo_event(project) log_geo_event(project)
log_audit_event(project) log_audit_event(project)
end end
...@@ -39,24 +59,6 @@ module EE ...@@ -39,24 +59,6 @@ module EE
).create! ).create!
end end
# Removes physical repository in a Geo replicated secondary node
# There is no need to do any database operation as it will be
# replicated by itself.
def geo_replicate
return unless ::Gitlab::Geo.secondary?
# Flush the cache for both repositories. This has to be done _before_
# removing the physical repositories as some expiration code depends on
# Git data (e.g. a list of branch names).
flush_caches(project)
trash_project_repositories!
log_info("Project \"#{project.name}\" was removed")
end
private
def log_audit_event(project) def log_audit_event(project)
::AuditEventService.new( ::AuditEventService.new(
current_user, current_user,
......
...@@ -20,79 +20,93 @@ RSpec.describe Projects::DestroyService do ...@@ -20,79 +20,93 @@ RSpec.describe Projects::DestroyService do
stub_container_registry_tags(repository: :any, tags: []) stub_container_registry_tags(repository: :any, tags: [])
end end
context 'when project is a mirror' do shared_examples 'project destroy ee' do
it 'decrements capacity if mirror was scheduled' do context 'when project is a mirror' do
max_capacity = Gitlab::CurrentSettings.mirror_max_capacity let(:max_capacity) { Gitlab::CurrentSettings.mirror_max_capacity }
project_mirror = create(:project, :mirror, :repository, :import_scheduled) let_it_be(:project_mirror) { create(:project, :mirror, :repository, :import_scheduled) }
let(:result) { described_class.new(project_mirror, project_mirror.owner, {}).execute }
Gitlab::Mirror.increment_capacity(project_mirror.id)
before do
Gitlab::Mirror.increment_capacity(project_mirror.id)
end
expect do it 'decrements capacity if mirror was scheduled' do
described_class.new(project_mirror, project_mirror.owner, {}).execute expect {result}.to change { Gitlab::Mirror.available_capacity }.from(max_capacity - 1).to(max_capacity)
end.to change { Gitlab::Mirror.available_capacity }.from(max_capacity - 1).to(max_capacity) end
end end
end
context 'when running on a primary node' do context 'when running on a primary node' do
let_it_be(:primary) { create(:geo_node, :primary) } let_it_be(:primary) { create(:geo_node, :primary) }
let_it_be(:secondary) { create(:geo_node) } let_it_be(:secondary) { create(:geo_node) }
before do before do
stub_current_geo_node(primary) stub_current_geo_node(primary)
end end
it 'logs an event to the Geo event log' do
# Run Sidekiq immediately to check that renamed repository will be removed
Sidekiq::Testing.inline! do
expect(subject).to receive(:log_destroy_events).and_call_original
expect { subject.execute }.to change(Geo::RepositoryDeletedEvent, :count).by(1)
end
end
it 'logs an event to the Geo event log' do it 'does not log event to the Geo log if project deletion fails' do
# Run Sidekiq immediately to check that renamed repository will be removed
Sidekiq::Testing.inline! do
expect(subject).to receive(:log_destroy_event).and_call_original expect(subject).to receive(:log_destroy_event).and_call_original
expect { subject.execute }.to change(Geo::RepositoryDeletedEvent, :count).by(1) expect(project).to receive(:destroy!).and_raise(StandardError.new('Other error message'))
Sidekiq::Testing.inline! do
expect { subject.execute }.not_to change(Geo::RepositoryDeletedEvent, :count)
end
end end
end end
it 'does not log event to the Geo log if project deletion fails' do context 'audit events' do
expect(subject).to receive(:log_destroy_event).and_call_original include_examples 'audit event logging' do
expect_any_instance_of(Project) let(:operation) { subject.execute }
.to receive(:destroy!).and_raise(StandardError.new('Other error message'))
let(:fail_condition!) do
Sidekiq::Testing.inline! do expect(project).to receive(:destroy!).and_raise(StandardError.new('Other error message'))
expect { subject.execute }.not_to change(Geo::RepositoryDeletedEvent, :count) end
let(:attributes) do
{
author_id: user.id,
entity_id: project.id,
entity_type: 'Project',
details: {
remove: 'project',
author_name: user.name,
target_id: project.full_path,
target_type: 'Project',
target_details: project.full_path
}
}
end
end end
end end
end
context 'audit events' do context 'system hooks exception' do
include_examples 'audit event logging' do before do
let(:operation) { subject.execute } allow_any_instance_of(SystemHooksService).to receive(:execute_hooks_for).and_raise('something went wrong')
let(:fail_condition!) do
expect_any_instance_of(Project)
.to receive(:destroy!).and_raise(StandardError.new('Other error message'))
end end
let(:attributes) do it 'logs an audit event' do
{ expect(subject).to receive(:log_destroy_event).and_call_original
author_id: user.id, expect { subject.execute }.to change(AuditEvent, :count)
entity_id: project.id,
entity_type: 'Project',
details: {
remove: 'project',
author_name: user.name,
target_id: project.full_path,
target_type: 'Project',
target_details: project.full_path
}
}
end end
end end
end end
context 'system hooks exception' do context 'when project_transactionless_destroy enabled' do
it_behaves_like 'project destroy ee'
end
context 'when project_transactionless_destroy disabled', :sidekiq_inline do
before do before do
allow_any_instance_of(SystemHooksService).to receive(:execute_hooks_for).and_raise('something went wrong') stub_feature_flags(project_transactionless_destroy: false)
end end
it 'logs an audit event' do it_behaves_like 'project destroy ee'
expect(subject).to receive(:log_destroy_event).and_call_original
expect { subject.execute }.to change(AuditEvent, :count)
end
end end
end end
...@@ -79,6 +79,10 @@ module Gitlab ...@@ -79,6 +79,10 @@ module Gitlab
def self.expand_names_for_cross_pipeline_artifacts?(project) def self.expand_names_for_cross_pipeline_artifacts?(project)
::Feature.enabled?(:ci_expand_names_for_cross_pipeline_artifacts, project) ::Feature.enabled?(:ci_expand_names_for_cross_pipeline_artifacts, project)
end end
def self.project_transactionless_destroy?(project)
Feature.enabled?(:project_transactionless_destroy, project, default_enabled: false)
end
end end
end 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