Stop abusing subject to store results,

+ add helper methods to cleanup fs_shards metrics

3 files changed, 67 insertions, 64 deletions

diff --git a/changelogs/unreleased/pawel-ensure_temp_files_are_deleted_in_fs_metrics-35457.yml b/changelogs/unreleased/pawel-ensure_temp_files_are_deleted_in_fs_metrics-35457.yml
index 4906232237f..1186dc59dc7 100644
--- a/changelogs/unreleased/pawel-ensure_temp_files_are_deleted_in_fs_metrics-35457.yml
+++ b/changelogs/unreleased/pawel-ensure_temp_files_are_deleted_in_fs_metrics-35457.yml
@@ -1,4 +1,4 @@
 ---
-title: Ensure fs metrics test files are deleted
+title: Ensure filesystem metrics test files are deleted
 merge_request:
 author:
diff --git a/lib/gitlab/health_checks/fs_shards_check.rb b/lib/gitlab/health_checks/fs_shards_check.rb
index ab18701b3bf..ddd1aaa7043 100644
--- a/lib/gitlab/health_checks/fs_shards_check.rb
+++ b/lib/gitlab/health_checks/fs_shards_check.rb
@@ -32,26 +32,13 @@ module Gitlab
         end
 
         def metrics
-          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
+          repository_storages.flat_map do |storage_name|
+            [
+              storage_stat_metrics(storage_name),
+              storage_write_metrics(storage_name),
+              storage_read_metrics(storage_name)
+            ].flatten
           end
-          res.flatten
         end
 
         private
@@ -81,19 +68,26 @@ module Gitlab
 
         def with_temp_file(storage_name)
           begin
-            temp_file_path = Dir::Tmpname.create(%w(fs_shards_check +deleted), path(storage_name)) { |path| path }
+            temp_file_path = Dir::Tmpname.create(%w(fs_shards_check +deleted), storage_path(storage_name)) { |path| path }
             yield temp_file_path
           ensure
             delete_test_file(temp_file_path)
           end
         end
 
-        def path(storage_name)
+        def storage_path(storage_name)
           storages_paths&.dig(storage_name, 'path')
         end
 
+        def delete_test_file(tmp_path)
+          _, status = exec_with_timeout(%W{ rm -f #{tmp_path} })
+          status == 0
+        rescue Errno::ENOENT
+          File.delete(tmp_path) rescue Errno::ENOENT
+        end
+
         def storage_stat_test(storage_name)
-          stat_path = File.join(path(storage_name), '.')
+          stat_path = File.join(storage_path(storage_name), '.')
           begin
             _, status = exec_with_timeout(%W{ stat #{stat_path} })
             status == 0
@@ -122,11 +116,27 @@ module Gitlab
           file_contents == RANDOM_STRING
         end
 
-        def delete_test_file(tmp_path)
-          _, status = exec_with_timeout(%W{ rm -f #{tmp_path} })
-          status == 0
-        rescue Errno::ENOENT
-          File.delete(tmp_path) rescue Errno::ENOENT
+        def storage_stat_metrics(storage_name)
+          operation_metrics(:filesystem_accessible, :filesystem_access_latency_seconds, shard: storage_name) do
+            with_timing { storage_stat_test(storage_name) }
+          end
+        end
+
+        def storage_write_metrics(storage_name)
+          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
+        end
+
+        def storage_read_metrics(storage_name)
+          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
       end
     end
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 947fb1b64d0..0a8dfa3bbdd 100644
--- a/spec/lib/gitlab/health_checks/fs_shards_check_spec.rb
+++ b/spec/lib/gitlab/health_checks/fs_shards_check_spec.rb
@@ -64,9 +64,7 @@ describe Gitlab::HealthChecks::FsShardsCheck do
         it 'cleans up files used for testing' do
           expect(described_class).to receive(:storage_write_test).with(any_args).and_call_original
 
-          subject
-
-          expect(Dir.entries(tmp_dir).count).to eq(2)
+          expect { subject }.not_to change(Dir.entries(tmp_dir), :count)
         end
 
         context 'read test fails' do
@@ -88,8 +86,6 @@ describe Gitlab::HealthChecks::FsShardsCheck do
     end
 
     describe '#metrics' do
-      subject { described_class.metrics }
-
       context 'storage points to not existing folder' do
         let(:storages_paths) do
           {
@@ -104,14 +100,15 @@ describe Gitlab::HealthChecks::FsShardsCheck do
         end
 
         it 'provides metrics' do
-          expect(subject).to all(have_attributes(labels: { shard: :default }))
-          expect(subject).to include(an_object_having_attributes(name: :filesystem_accessible, value: 0))
-          expect(subject).to include(an_object_having_attributes(name: :filesystem_readable, value: 0))
-          expect(subject).to include(an_object_having_attributes(name: :filesystem_writable, value: 0))
-
-          expect(subject).to include(an_object_having_attributes(name: :filesystem_access_latency_seconds, value: be >= 0))
-          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))
+          metrics = described_class.metrics
+
+          expect(metrics).to all(have_attributes(labels: { shard: :default }))
+          expect(metrics).to include(an_object_having_attributes(name: :filesystem_accessible, value: 0))
+          expect(metrics).to include(an_object_having_attributes(name: :filesystem_readable, value: 0))
+          expect(metrics).to include(an_object_having_attributes(name: :filesystem_writable, value: 0))
+          expect(metrics).to include(an_object_having_attributes(name: :filesystem_access_latency_seconds, value: be >= 0))
+          expect(metrics).to include(an_object_having_attributes(name: :filesystem_read_latency_seconds, value: be >= 0))
+          expect(metrics).to include(an_object_having_attributes(name: :filesystem_write_latency_seconds, value: be >= 0))
         end
       end
 
@@ -121,21 +118,19 @@ describe Gitlab::HealthChecks::FsShardsCheck do
         end
 
         it 'provides metrics' do
-          expect(subject).to all(have_attributes(labels: { shard: :default }))
-
-          expect(subject).to include(an_object_having_attributes(name: :filesystem_accessible, value: 1))
-          expect(subject).to include(an_object_having_attributes(name: :filesystem_readable, value: 1))
-          expect(subject).to include(an_object_having_attributes(name: :filesystem_writable, value: 1))
-
-          expect(subject).to include(an_object_having_attributes(name: :filesystem_access_latency_seconds, value: be >= 0))
-          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))
+          metrics = described_class.metrics
+
+          expect(metrics).to all(have_attributes(labels: { shard: :default }))
+          expect(metrics).to include(an_object_having_attributes(name: :filesystem_accessible, value: 1))
+          expect(metrics).to include(an_object_having_attributes(name: :filesystem_readable, value: 1))
+          expect(metrics).to include(an_object_having_attributes(name: :filesystem_writable, value: 1))
+          expect(metrics).to include(an_object_having_attributes(name: :filesystem_access_latency_seconds, value: be >= 0))
+          expect(metrics).to include(an_object_having_attributes(name: :filesystem_read_latency_seconds, value: be >= 0))
+          expect(metrics).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)
+          expect { described_class.metrics }.not_to change(Dir.entries(tmp_dir), :count)
         end
       end
     end
@@ -156,18 +151,16 @@ describe Gitlab::HealthChecks::FsShardsCheck do
     end
 
     describe '#metrics' do
-      subject { described_class.metrics }
-
       it 'provides metrics' do
-        expect(subject).to all(have_attributes(labels: { shard: :default }))
-
-        expect(subject).to include(an_object_having_attributes(name: :filesystem_accessible, value: 0))
-        expect(subject).to include(an_object_having_attributes(name: :filesystem_readable, value: 0))
-        expect(subject).to include(an_object_having_attributes(name: :filesystem_writable, value: 0))
-
-        expect(subject).to include(an_object_having_attributes(name: :filesystem_access_latency_seconds, value: be >= 0))
-        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))
+        metrics = described_class.metrics
+
+        expect(metrics).to all(have_attributes(labels: { shard: :default }))
+        expect(metrics).to include(an_object_having_attributes(name: :filesystem_accessible, value: 0))
+        expect(metrics).to include(an_object_having_attributes(name: :filesystem_readable, value: 0))
+        expect(metrics).to include(an_object_having_attributes(name: :filesystem_writable, value: 0))
+        expect(metrics).to include(an_object_having_attributes(name: :filesystem_access_latency_seconds, value: be >= 0))
+        expect(metrics).to include(an_object_having_attributes(name: :filesystem_read_latency_seconds, value: be >= 0))
+        expect(metrics).to include(an_object_having_attributes(name: :filesystem_write_latency_seconds, value: be >= 0))
       end
     end
   end