Commit 6d3c1254 authored by Vasilii Iakliushin's avatar Vasilii Iakliushin Committed by Markus Koller

Explicitly destroy webhooks and logs before the project deletion

Contributes to https://gitlab.com/gitlab-org/gitlab/-/issues/26259

**Problem**

There are foreign keys with DELETE CASCADE option defined for
`web_hooks` and `web_hook_logs` tables. Database tries to delete them
in the scope of delete project query. For some cases, the query
duration exceeds the statement_timeout setting. As the result, the
project is stuck in a half-deleted state.

**Solution**

Run WebHooks::DestroyService before `project.destroy` call and ensure
that we delete webhooks efficiently in batches.
parent df225b39
......@@ -116,6 +116,7 @@ module Projects
log_destroy_event
trash_relation_repositories!
trash_project_repositories!
destroy_web_hooks! if Feature.enabled?(:destroy_webhooks_before_the_project, project, default_enabled: :yaml)
# Rails attempts to load all related records into memory before
# destroying: https://github.com/rails/rails/issues/22510
......@@ -131,6 +132,23 @@ module Projects
log_info("Attempting to destroy #{project.full_path} (#{project.id})")
end
# The project can have multiple webhooks with hundreds of thousands of web_hook_logs.
# By default, they are removed with "DELETE CASCADE" option defined via foreign_key.
# But such queries can exceed the statement_timeout limit and fail to delete the project.
# (see https://gitlab.com/gitlab-org/gitlab/-/issues/26259)
#
# To prevent that we use WebHooks::DestroyService. It deletes logs in batches and
# produces smaller and faster queries to the database.
def destroy_web_hooks!
project.hooks.find_each do |web_hook|
result = ::WebHooks::DestroyService.new(current_user).sync_destroy(web_hook)
unless result[:status] == :success
raise_error(s_('DeleteProject|Failed to remove webhooks. Please try again or contact administrator.'))
end
end
end
def remove_registry_tags
return true unless Gitlab.config.registry.enabled
return false unless remove_legacy_registry_tags
......
---
name: destroy_webhooks_before_the_project
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/59754
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/328393
milestone: '13.12'
type: development
group: group::source code
default_enabled: false
......@@ -10531,6 +10531,9 @@ msgstr ""
msgid "DeleteProject|Failed to remove some tags in project container registry. Please try again or contact administrator."
msgstr ""
msgid "DeleteProject|Failed to remove webhooks. Please try again or contact administrator."
msgstr ""
msgid "DeleteProject|Failed to remove wiki repository. Please try again or contact administrator."
msgstr ""
......
......@@ -418,6 +418,54 @@ RSpec.describe Projects::DestroyService, :aggregate_failures do
end
end
context 'when project has webhooks' do
let!(:web_hook1) { create(:project_hook, project: project) }
let!(:web_hook2) { create(:project_hook, project: project) }
let!(:another_project_web_hook) { create(:project_hook) }
let!(:web_hook_log) { create(:web_hook_log, web_hook: web_hook1) }
it 'deletes webhooks and logs related to project' do
expect_next_instance_of(WebHooks::DestroyService, user) do |instance|
expect(instance).to receive(:sync_destroy).with(web_hook1).and_call_original
end
expect_next_instance_of(WebHooks::DestroyService, user) do |instance|
expect(instance).to receive(:sync_destroy).with(web_hook2).and_call_original
end
expect do
destroy_project(project, user)
end.to change(WebHook, :count).by(-2)
.and change(WebHookLog, :count).by(-1)
end
context 'when an error is raised deleting webhooks' do
before do
allow_next_instance_of(WebHooks::DestroyService) do |instance|
allow(instance).to receive(:sync_destroy).and_return(message: 'foo', status: :error)
end
end
it_behaves_like 'handles errors thrown during async destroy', "Failed to remove webhooks"
end
context 'when "destroy_webhooks_before_the_project" flag is disabled' do
before do
stub_feature_flags(destroy_webhooks_before_the_project: false)
end
it 'does not call WebHooks::DestroyService' do
expect(WebHooks::DestroyService).not_to receive(:new)
expect do
destroy_project(project, user)
end.to change(WebHook, :count).by(-2)
.and change(WebHookLog, :count).by(-1)
expect(another_project_web_hook.reload).to be
end
end
end
context 'error while destroying', :sidekiq_inline do
let!(:pipeline) { create(:ci_pipeline, project: project) }
let!(:builds) { create_list(:ci_build, 2, :artifacts, pipeline: pipeline) }
......
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