Commit 62820dd7 authored by Bob Van Landuyt's avatar Bob Van Landuyt

Remove feature flag for GPG cleanup

This will make sure that when cleanup fails, an exception will be
raised.

This also increases the time we'll try to cleanup in Sidekiq, as it
appeared we still had some failures there.
parent 4e3839d8
---
title: Try longer to clean up after using a gpg-keychain and raise exption if the
cleanup fails
merge_request: 20718
author:
type: fixed
...@@ -5,7 +5,7 @@ module Gitlab ...@@ -5,7 +5,7 @@ module Gitlab
extend self extend self
CleanupError = Class.new(StandardError) CleanupError = Class.new(StandardError)
BG_CLEANUP_RUNTIME_S = 2 BG_CLEANUP_RUNTIME_S = 10
FG_CLEANUP_RUNTIME_S = 0.5 FG_CLEANUP_RUNTIME_S = 0.5
MUTEX = Mutex.new MUTEX = Mutex.new
...@@ -107,19 +107,18 @@ module Gitlab ...@@ -107,19 +107,18 @@ module Gitlab
begin begin
cleanup_tmp_dir(tmp_dir) cleanup_tmp_dir(tmp_dir)
rescue CleanupError => e rescue CleanupError => e
folder_contents = Dir.children(tmp_dir)
# This means we left a GPG-agent process hanging. Logging the problem in # This means we left a GPG-agent process hanging. Logging the problem in
# sentry will make this more visible. # sentry will make this more visible.
Gitlab::Sentry.track_exception(e, Gitlab::Sentry.track_exception(e,
issue_url: 'https://gitlab.com/gitlab-org/gitlab/issues/20918', issue_url: 'https://gitlab.com/gitlab-org/gitlab/issues/20918',
extra: { tmp_dir: tmp_dir }) extra: { tmp_dir: tmp_dir, contents: folder_contents })
end end
tmp_keychains_removed.increment unless File.exist?(tmp_dir) tmp_keychains_removed.increment unless File.exist?(tmp_dir)
end end
def cleanup_tmp_dir(tmp_dir) def cleanup_tmp_dir(tmp_dir)
return FileUtils.remove_entry(tmp_dir, true) if Feature.disabled?(:gpg_cleanup_retries)
# Retry when removing the tmp directory failed, as we may run into a # Retry when removing the tmp directory failed, as we may run into a
# race condition: # race condition:
# The `gpg-agent` agent process may clean up some files as well while # The `gpg-agent` agent process may clean up some files as well while
......
...@@ -177,6 +177,25 @@ describe Gitlab::Gpg do ...@@ -177,6 +177,25 @@ describe Gitlab::Gpg do
end.not_to raise_error end.not_to raise_error
end end
it 'tracks an exception when cleaning up the tmp dir fails' do
expected_exception = described_class::CleanupError.new('cleanup failed')
expected_tmp_dir = nil
expect(described_class).to receive(:cleanup_tmp_dir).and_raise(expected_exception)
allow(Gitlab::Sentry).to receive(:track_exception)
described_class.using_tmp_keychain do
expected_tmp_dir = described_class.current_home_dir
FileUtils.touch(File.join(expected_tmp_dir, 'dummy.file'))
end
expect(Gitlab::Sentry).to have_received(:track_exception).with(
expected_exception,
issue_url: 'https://gitlab.com/gitlab-org/gitlab/issues/20918',
extra: { tmp_dir: expected_tmp_dir, contents: ['dummy.file'] }
)
end
shared_examples 'multiple deletion attempts of the tmp-dir' do |seconds| shared_examples 'multiple deletion attempts of the tmp-dir' do |seconds|
let(:tmp_dir) do let(:tmp_dir) do
tmp_dir = Dir.mktmpdir tmp_dir = Dir.mktmpdir
...@@ -211,15 +230,6 @@ describe Gitlab::Gpg do ...@@ -211,15 +230,6 @@ describe Gitlab::Gpg do
expect(File.exist?(tmp_dir)).to be false expect(File.exist?(tmp_dir)).to be false
end end
it 'does not retry when the feature flag is disabled' do
stub_feature_flags(gpg_cleanup_retries: false)
expect(FileUtils).to receive(:remove_entry).with(tmp_dir, true).and_call_original
expect(Retriable).not_to receive(:retriable)
described_class.using_tmp_keychain {}
end
end end
it_behaves_like 'multiple deletion attempts of the tmp-dir', described_class::FG_CLEANUP_RUNTIME_S it_behaves_like 'multiple deletion attempts of the tmp-dir', described_class::FG_CLEANUP_RUNTIME_S
......
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