Commit 895e1b3e authored by Pawel Chojnacki's avatar Pawel Chojnacki

Stop abusing subject to store results,

+ add helper methods to cleanup fs_shards metrics
parent 37f27079
--- ---
title: Ensure fs metrics test files are deleted title: Ensure filesystem metrics test files are deleted
merge_request: merge_request:
author: author:
...@@ -32,26 +32,13 @@ module Gitlab ...@@ -32,26 +32,13 @@ module Gitlab
end end
def metrics def metrics
res = [] repository_storages.flat_map do |storage_name|
repository_storages.each do |storage_name| [
res << operation_metrics(:filesystem_accessible, :filesystem_access_latency_seconds, shard: storage_name) do storage_stat_metrics(storage_name),
with_timing { storage_stat_test(storage_name) } storage_write_metrics(storage_name),
end storage_read_metrics(storage_name)
].flatten
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 end
res.flatten
end end
private private
...@@ -81,19 +68,26 @@ module Gitlab ...@@ -81,19 +68,26 @@ module Gitlab
def with_temp_file(storage_name) def with_temp_file(storage_name)
begin 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 yield temp_file_path
ensure ensure
delete_test_file(temp_file_path) delete_test_file(temp_file_path)
end end
end end
def path(storage_name) def storage_path(storage_name)
storages_paths&.dig(storage_name, 'path') storages_paths&.dig(storage_name, 'path')
end 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) def storage_stat_test(storage_name)
stat_path = File.join(path(storage_name), '.') stat_path = File.join(storage_path(storage_name), '.')
begin begin
_, status = exec_with_timeout(%W{ stat #{stat_path} }) _, status = exec_with_timeout(%W{ stat #{stat_path} })
status == 0 status == 0
...@@ -122,11 +116,27 @@ module Gitlab ...@@ -122,11 +116,27 @@ module Gitlab
file_contents == RANDOM_STRING file_contents == RANDOM_STRING
end end
def delete_test_file(tmp_path) def storage_stat_metrics(storage_name)
_, status = exec_with_timeout(%W{ rm -f #{tmp_path} }) operation_metrics(:filesystem_accessible, :filesystem_access_latency_seconds, shard: storage_name) do
status == 0 with_timing { storage_stat_test(storage_name) }
rescue Errno::ENOENT end
File.delete(tmp_path) rescue Errno::ENOENT 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 end
end end
......
...@@ -64,9 +64,7 @@ describe Gitlab::HealthChecks::FsShardsCheck do ...@@ -64,9 +64,7 @@ describe Gitlab::HealthChecks::FsShardsCheck do
it 'cleans up files used for testing' do it 'cleans up files used for testing' do
expect(described_class).to receive(:storage_write_test).with(any_args).and_call_original expect(described_class).to receive(:storage_write_test).with(any_args).and_call_original
subject expect { subject }.not_to change(Dir.entries(tmp_dir), :count)
expect(Dir.entries(tmp_dir).count).to eq(2)
end end
context 'read test fails' do context 'read test fails' do
...@@ -88,8 +86,6 @@ describe Gitlab::HealthChecks::FsShardsCheck do ...@@ -88,8 +86,6 @@ describe Gitlab::HealthChecks::FsShardsCheck do
end end
describe '#metrics' do describe '#metrics' do
subject { described_class.metrics }
context 'storage points to not existing folder' do context 'storage points to not existing folder' do
let(:storages_paths) do let(:storages_paths) do
{ {
...@@ -104,14 +100,15 @@ describe Gitlab::HealthChecks::FsShardsCheck do ...@@ -104,14 +100,15 @@ describe Gitlab::HealthChecks::FsShardsCheck do
end end
it 'provides metrics' do it 'provides metrics' do
expect(subject).to all(have_attributes(labels: { shard: :default })) metrics = described_class.metrics
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(metrics).to all(have_attributes(labels: { shard: :default }))
expect(subject).to include(an_object_having_attributes(name: :filesystem_read_latency_seconds, value: be >= 0)) expect(metrics).to include(an_object_having_attributes(name: :filesystem_accessible, value: 0))
expect(subject).to include(an_object_having_attributes(name: :filesystem_write_latency_seconds, value: be >= 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 end
...@@ -121,21 +118,19 @@ describe Gitlab::HealthChecks::FsShardsCheck do ...@@ -121,21 +118,19 @@ describe Gitlab::HealthChecks::FsShardsCheck do
end end
it 'provides metrics' do it 'provides metrics' do
expect(subject).to all(have_attributes(labels: { shard: :default })) metrics = described_class.metrics
expect(subject).to include(an_object_having_attributes(name: :filesystem_accessible, value: 1)) expect(metrics).to all(have_attributes(labels: { shard: :default }))
expect(subject).to include(an_object_having_attributes(name: :filesystem_readable, value: 1)) expect(metrics).to include(an_object_having_attributes(name: :filesystem_accessible, value: 1))
expect(subject).to include(an_object_having_attributes(name: :filesystem_writable, 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(subject).to include(an_object_having_attributes(name: :filesystem_access_latency_seconds, value: be >= 0)) expect(metrics).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(metrics).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)) expect(metrics).to include(an_object_having_attributes(name: :filesystem_write_latency_seconds, value: be >= 0))
end end
it 'cleans up files used for metrics' do it 'cleans up files used for metrics' do
subject expect { described_class.metrics }.not_to change(Dir.entries(tmp_dir), :count)
expect(Dir.entries(tmp_dir).count).to eq(2)
end end
end end
end end
...@@ -156,18 +151,16 @@ describe Gitlab::HealthChecks::FsShardsCheck do ...@@ -156,18 +151,16 @@ describe Gitlab::HealthChecks::FsShardsCheck do
end end
describe '#metrics' do describe '#metrics' do
subject { described_class.metrics }
it 'provides metrics' do it 'provides metrics' do
expect(subject).to all(have_attributes(labels: { shard: :default })) metrics = described_class.metrics
expect(subject).to include(an_object_having_attributes(name: :filesystem_accessible, value: 0)) expect(metrics).to all(have_attributes(labels: { shard: :default }))
expect(subject).to include(an_object_having_attributes(name: :filesystem_readable, value: 0)) expect(metrics).to include(an_object_having_attributes(name: :filesystem_accessible, value: 0))
expect(subject).to include(an_object_having_attributes(name: :filesystem_writable, 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(subject).to include(an_object_having_attributes(name: :filesystem_access_latency_seconds, value: be >= 0)) expect(metrics).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(metrics).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)) expect(metrics).to include(an_object_having_attributes(name: :filesystem_write_latency_seconds, value: be >= 0))
end end
end end
end end
......
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