Ensure test files are deleted after tests

4 files changed, 52 insertions, 29 deletions

diff --git a/config/initializers/7_prometheus_metrics.rb b/config/initializers/7_prometheus_metrics.rb
index 987324a86c9..a2f8421f5d7 100644
--- a/config/initializers/7_prometheus_metrics.rb
+++ b/config/initializers/7_prometheus_metrics.rb
@@ -6,7 +6,7 @@ Prometheus::Client.configure do |config|
   config.initial_mmap_file_size = 4 * 1024
   config.multiprocess_files_dir = ENV['prometheus_multiproc_dir']
 
-  if Rails.env.development? && Rails.env.test?
+  if Rails.env.development? || Rails.env.test?
     config.multiprocess_files_dir ||= Rails.root.join('tmp/prometheus_multiproc_dir')
   end
 end
diff --git a/lib/gitlab/health_checks/base_abstract_check.rb b/lib/gitlab/health_checks/base_abstract_check.rb
index 7de6d4d9367..8b365dab185 100644
--- a/lib/gitlab/health_checks/base_abstract_check.rb
+++ b/lib/gitlab/health_checks/base_abstract_check.rb
@@ -27,10 +27,10 @@ module Gitlab
         Metric.new(name, value, labels)
       end
 
-      def with_timing(proc)
+      def with_timing
         start = Time.now
-        result = proc.call
-        yield result, Time.now.to_f - start.to_f
+        result = yield
+        [result, Time.now.to_f - start.to_f]
       end
 
       def catch_timeout(seconds, &block)
diff --git a/lib/gitlab/health_checks/fs_shards_check.rb b/lib/gitlab/health_checks/fs_shards_check.rb
index bebde857b16..45cd8f7eefc 100644
--- a/lib/gitlab/health_checks/fs_shards_check.rb
+++ b/lib/gitlab/health_checks/fs_shards_check.rb
@@ -10,52 +10,64 @@ module Gitlab
         def readiness
           repository_storages.map do |storage_name|
             begin
-              tmp_file_path = tmp_file_path(storage_name)
-
               if !storage_stat_test(storage_name)
                 HealthChecks::Result.new(false, 'cannot stat storage', shard: storage_name)
-              elsif !storage_write_test(tmp_file_path)
-                HealthChecks::Result.new(false, 'cannot write to storage', shard: storage_name)
-              elsif !storage_read_test(tmp_file_path)
-                HealthChecks::Result.new(false, 'cannot read from storage', shard: storage_name)
               else
-                HealthChecks::Result.new(true, nil, shard: storage_name)
+                with_temp_file(storage_name) do |tmp_file_path|
+                  if !storage_write_test(tmp_file_path)
+                    HealthChecks::Result.new(false, 'cannot write to storage', shard: storage_name)
+                  elsif !storage_read_test(tmp_file_path)
+                    HealthChecks::Result.new(false, 'cannot read from storage', shard: storage_name)
+                  else
+                    HealthChecks::Result.new(true, nil, shard: storage_name)
+                  end
+                end
               end
             rescue RuntimeError => ex
               message = "unexpected error #{ex} when checking storage #{storage_name}"
               Rails.logger.error(message)
               HealthChecks::Result.new(false, message, shard: storage_name)
-            ensure
-              delete_test_file(tmp_file_path)
             end
           end
         end
 
         def metrics
-          repository_storages.flat_map do |storage_name|
-            tmp_file_path = tmp_file_path(storage_name)
-            [
-              operation_metrics(:filesystem_accessible, :filesystem_access_latency_seconds, -> { storage_stat_test(storage_name) }, shard: storage_name),
-              operation_metrics(:filesystem_writable, :filesystem_write_latency_seconds, -> { storage_write_test(tmp_file_path) }, shard: storage_name),
-              operation_metrics(:filesystem_readable, :filesystem_read_latency_seconds, -> { storage_read_test(tmp_file_path) }, shard: storage_name)
-            ].flatten
+          res = []
+          repository_storages.each do |storage_name|
+            res << operation_metrics(:filesystem_accessible, :filesystem_access_latency_seconds, shard: storage_name) do
+              with_timing { storage_stat_test(storage_name) }
+            end
+
+            res << operation_metrics(:filesystem_writable, :filesystem_write_latency_seconds, shard: storage_name) do
+              with_temp_file(storage_name) do |tmp_file_path|
+                with_timing { storage_write_test(tmp_file_path) }
+              end
+            end
+
+            res << operation_metrics(:filesystem_readable, :filesystem_read_latency_seconds, shard: storage_name) do
+              with_temp_file(storage_name) do |tmp_file_path|
+                storage_write_test(tmp_file_path) # writes data used by read test
+                with_timing { storage_read_test(tmp_file_path) }
+              end
+            end
           end
+          res.flatten
         end
 
         private
 
-        def operation_metrics(ok_metric, latency_metric, operation, **labels)
-          with_timing operation do |result, elapsed|
-            [
-              metric(latency_metric, elapsed, **labels),
-              metric(ok_metric, result ? 1 : 0, **labels)
-            ]
-          end
+        def operation_metrics(ok_metric, latency_metric, **labels)
+          result, elapsed = yield
+          [
+            metric(latency_metric, elapsed, **labels),
+            metric(ok_metric, result ? 1 : 0, **labels)
+          ]
         rescue RuntimeError => ex
           Rails.logger.error("unexpected error #{ex} when checking #{ok_metric}")
           [metric(ok_metric, 0, **labels)]
         end
 
+
         def repository_storages
           @repository_storage ||= Gitlab::CurrentSettings.current_application_settings.repository_storages
         end
@@ -68,8 +80,13 @@ module Gitlab
           Gitlab::Popen.popen([TIMEOUT_EXECUTABLE, COMMAND_TIMEOUT].concat(cmd_args), *args, &block)
         end
 
-        def tmp_file_path(storage_name)
-          Dir::Tmpname.create(%w(fs_shards_check +deleted), path(storage_name)) { |path| path }
+        def with_temp_file(storage_name)
+          begin
+            temp_file_path = Dir::Tmpname.create(%w(fs_shards_check +deleted), path(storage_name)) { |path| path }
+            yield temp_file_path
+          ensure
+            delete_test_file(temp_file_path)
+          end
         end
 
         def path(storage_name)
diff --git a/spec/lib/gitlab/health_checks/fs_shards_check_spec.rb b/spec/lib/gitlab/health_checks/fs_shards_check_spec.rb
index 3de73a9ff65..947fb1b64d0 100644
--- a/spec/lib/gitlab/health_checks/fs_shards_check_spec.rb
+++ b/spec/lib/gitlab/health_checks/fs_shards_check_spec.rb
@@ -131,6 +131,12 @@ describe Gitlab::HealthChecks::FsShardsCheck do
           expect(subject).to include(an_object_having_attributes(name: :filesystem_read_latency_seconds, value: be >= 0))
           expect(subject).to include(an_object_having_attributes(name: :filesystem_write_latency_seconds, value: be >= 0))
         end
+
+        it 'cleans up files used for metrics' do
+          subject
+
+          expect(Dir.entries(tmp_dir).count).to eq(2)
+        end
       end
     end
   end