From 62820dd73dcc79f5da65387e3ffb4384eca559c1 Mon Sep 17 00:00:00 2001
From: Bob Van Landuyt <bob@vanlanduyt.co>
Date: Mon, 25 Nov 2019 14:00:40 +0100
Subject: [PATCH] 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.
---
.../bvl-remove-cleanup-feature-flag.yml | 6 ++++
lib/gitlab/gpg.rb | 7 ++---
spec/lib/gitlab/gpg_spec.rb | 28 +++++++++++++------
3 files changed, 28 insertions(+), 13 deletions(-)
create mode 100644 changelogs/unreleased/bvl-remove-cleanup-feature-flag.yml
diff --git a/changelogs/unreleased/bvl-remove-cleanup-feature-flag.yml b/changelogs/unreleased/bvl-remove-cleanup-feature-flag.yml
new file mode 100644
index 000000000000..31ec5157b1fc
--- /dev/null
+++ b/changelogs/unreleased/bvl-remove-cleanup-feature-flag.yml
@@ -0,0 +1,6 @@
+---
+title: Try longer to clean up after using a gpg-keychain and raise exption if the
+ cleanup fails
+merge_request: 20718
+author:
+type: fixed
diff --git a/lib/gitlab/gpg.rb b/lib/gitlab/gpg.rb
index 1dce26efc658..829e64b11a44 100644
--- a/lib/gitlab/gpg.rb
+++ b/lib/gitlab/gpg.rb
@@ -5,7 +5,7 @@ module Gpg
extend self
CleanupError = Class.new(StandardError)
- BG_CLEANUP_RUNTIME_S = 2
+ BG_CLEANUP_RUNTIME_S = 10
FG_CLEANUP_RUNTIME_S = 0.5
MUTEX = Mutex.new
@@ -107,19 +107,18 @@ def optimistic_using_tmp_keychain
begin
cleanup_tmp_dir(tmp_dir)
rescue CleanupError => e
+ folder_contents = Dir.children(tmp_dir)
# This means we left a GPG-agent process hanging. Logging the problem in
# sentry will make this more visible.
Gitlab::Sentry.track_exception(e,
issue_url: 'https://gitlab.com/gitlab-org/gitlab/issues/20918',
- extra: { tmp_dir: tmp_dir })
+ extra: { tmp_dir: tmp_dir, contents: folder_contents })
end
tmp_keychains_removed.increment unless File.exist?(tmp_dir)
end
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
# race condition:
# The `gpg-agent` agent process may clean up some files as well while
diff --git a/spec/lib/gitlab/gpg_spec.rb b/spec/lib/gitlab/gpg_spec.rb
index 52d6a86f7d06..cd5933908217 100644
--- a/spec/lib/gitlab/gpg_spec.rb
+++ b/spec/lib/gitlab/gpg_spec.rb
@@ -177,6 +177,25 @@
end.not_to raise_error
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|
let(:tmp_dir) do
tmp_dir = Dir.mktmpdir
@@ -211,15 +230,6 @@
expect(File.exist?(tmp_dir)).to be false
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
it_behaves_like 'multiple deletion attempts of the tmp-dir', described_class::FG_CLEANUP_RUNTIME_S
--
GitLab