diff options
| -rw-r--r-- | lib/gitlab/health_checks/fs_shards_check.rb | 17 | ||||
| -rw-r--r-- | spec/lib/gitlab/health_checks/fs_shards_check_spec.rb | 83 |
2 files changed, 78 insertions, 22 deletions
diff --git a/lib/gitlab/health_checks/fs_shards_check.rb b/lib/gitlab/health_checks/fs_shards_check.rb index df962d203b7..e78b7f22e03 100644 --- a/lib/gitlab/health_checks/fs_shards_check.rb +++ b/lib/gitlab/health_checks/fs_shards_check.rb @@ -2,6 +2,9 @@ module Gitlab module HealthChecks class FsShardsCheck extend BaseAbstractCheck + RANDOM_STRING = SecureRandom.hex(1000).freeze + COMMAND_TIMEOUT = '1'.freeze + TIMEOUT_EXECUTABLE = 'timeout'.freeze class << self def readiness @@ -41,8 +44,6 @@ module Gitlab private - RANDOM_STRING = SecureRandom.hex(1000).freeze - def operation_metrics(ok_metric, latency_metric, operation, **labels) with_timing operation do |result, elapsed| [ @@ -63,8 +64,8 @@ module Gitlab @storage_paths ||= Gitlab.config.repositories.storages end - def with_timeout(args) - %w{timeout 1}.concat(args) + def exec_with_timeout(cmd_args, *args, &block) + Gitlab::Popen.popen([TIMEOUT_EXECUTABLE, COMMAND_TIMEOUT].concat(cmd_args), *args, &block) end def tmp_file_path(storage_name) @@ -78,7 +79,7 @@ module Gitlab def storage_stat_test(storage_name) stat_path = File.join(path(storage_name), '.') begin - _, status = Gitlab::Popen.popen(with_timeout(%W{ stat #{stat_path} })) + _, status = exec_with_timeout(%W{ stat #{stat_path} }) status == 0 rescue Errno::ENOENT File.exist?(stat_path) && File::Stat.new(stat_path).readable? @@ -86,7 +87,7 @@ module Gitlab end def storage_write_test(tmp_path) - _, status = Gitlab::Popen.popen(with_timeout(%W{ tee #{tmp_path} })) do |stdin| + _, status = exec_with_timeout(%W{ tee #{tmp_path} }) do |stdin| stdin.write(RANDOM_STRING) end status == 0 @@ -96,7 +97,7 @@ module Gitlab end def storage_read_test(tmp_path) - _, status = Gitlab::Popen.popen(with_timeout(%W{ diff #{tmp_path} - })) do |stdin| + _, status = exec_with_timeout(%W{ diff #{tmp_path} - }) do |stdin| stdin.write(RANDOM_STRING) end status == 0 @@ -106,7 +107,7 @@ module Gitlab end def delete_test_file(tmp_path) - _, status = Gitlab::Popen.popen(with_timeout(%W{ rm -f #{tmp_path} })) + _, status = exec_with_timeout(%W{ rm -f #{tmp_path} }) status == 0 rescue Errno::ENOENT File.delete(tmp_path) rescue Errno::ENOENT 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 45ccd3d6459..61c10d47434 100644 --- a/spec/lib/gitlab/health_checks/fs_shards_check_spec.rb +++ b/spec/lib/gitlab/health_checks/fs_shards_check_spec.rb @@ -1,6 +1,24 @@ require 'spec_helper' describe Gitlab::HealthChecks::FsShardsCheck do + def command_exists?(command) + _, status = Gitlab::Popen.popen(%W{ #{command} 1 echo }) + status == 0 + rescue Errno::ENOENT + false + end + + def timeout_command + @timeout_command ||= + if command_exists?('timeout') + 'timeout' + elsif command_exists?('gtimeout') + 'gtimeout' + else + '' + end + end + let(:metric_class) { Gitlab::HealthChecks::Metric } let(:result_class) { Gitlab::HealthChecks::Result } let(:repository_storages) { [:default] } @@ -15,6 +33,7 @@ describe Gitlab::HealthChecks::FsShardsCheck do before do allow(described_class).to receive(:repository_storages) { repository_storages } allow(described_class).to receive(:storages_paths) { storages_paths } + stub_const('Gitlab::HealthChecks::FsShardsCheck::TIMEOUT_EXECUTABLE', timeout_command) end after do @@ -78,40 +97,76 @@ describe Gitlab::HealthChecks::FsShardsCheck do }.with_indifferent_access end - it { is_expected.to include(metric_class.new(:filesystem_accessible, 0, shard: :default)) } - it { is_expected.to include(metric_class.new(:filesystem_readable, 0, shard: :default)) } - it { is_expected.to include(metric_class.new(:filesystem_writable, 0, shard: :default)) } + it { is_expected.to all(have_attributes(labels: { shard: :default })) } + + it { is_expected.to include(an_object_having_attributes(name: :filesystem_accessible, value: 0)) } + it { is_expected.to include(an_object_having_attributes(name: :filesystem_readable, value: 0)) } + it { is_expected.to include(an_object_having_attributes(name: :filesystem_writable, value: 0)) } - it { is_expected.to include(have_attributes(name: :filesystem_access_latency, value: be >= 0, labels: { shard: :default })) } - it { is_expected.to include(have_attributes(name: :filesystem_read_latency, value: be >= 0, labels: { shard: :default })) } - it { is_expected.to include(have_attributes(name: :filesystem_write_latency, value: be >= 0, labels: { shard: :default })) } + it { is_expected.to include(an_object_having_attributes(name: :filesystem_access_latency, value: be >= 0)) } + it { is_expected.to include(an_object_having_attributes(name: :filesystem_read_latency, value: be >= 0)) } + it { is_expected.to include(an_object_having_attributes(name: :filesystem_write_latency, value: be >= 0)) } end context 'storage points to directory that has both read and write rights' do before do FileUtils.chmod_R(0755, tmp_dir) end + it { is_expected.to all(have_attributes(labels: { shard: :default })) } - it { is_expected.to include(metric_class.new(:filesystem_accessible, 1, shard: :default)) } - it { is_expected.to include(metric_class.new(:filesystem_readable, 1, shard: :default)) } - it { is_expected.to include(metric_class.new(:filesystem_writable, 1, shard: :default)) } + it { is_expected.to include(an_object_having_attributes(name: :filesystem_accessible, value: 1)) } + it { is_expected.to include(an_object_having_attributes(name: :filesystem_readable, value: 1)) } + it { is_expected.to include(an_object_having_attributes(name: :filesystem_writable, value: 1)) } - it { is_expected.to include(have_attributes(name: :filesystem_access_latency, value: be >= 0, labels: { shard: :default })) } - it { is_expected.to include(have_attributes(name: :filesystem_read_latency, value: be >= 0, labels: { shard: :default })) } - it { is_expected.to include(have_attributes(name: :filesystem_write_latency, value: be >= 0, labels: { shard: :default })) } + it { is_expected.to include(an_object_having_attributes(name: :filesystem_access_latency, value: be >= 0)) } + it { is_expected.to include(an_object_having_attributes(name: :filesystem_read_latency, value: be >= 0)) } + it { is_expected.to include(an_object_having_attributes(name: :filesystem_write_latency, value: be >= 0)) } + end + end + end + + context 'when timeout kills fs checks' do + before do + stub_const('Gitlab::HealthChecks::FsShardsCheck::COMMAND_TIMEOUT', '1') + + allow(described_class).to receive(:exec_with_timeout).and_wrap_original { |m| m.call(%w(sleep 60)) } + FileUtils.chmod_R(0755, tmp_dir) + end + + describe '#readiness' do + subject { described_class.readiness } + + it { is_expected.to include(result_class.new(false, 'cannot stat storage', shard: :default)) } + 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, value: be >= 0)) + expect(subject).to include(an_object_having_attributes(name: :filesystem_read_latency, value: be >= 0)) + expect(subject).to include(an_object_having_attributes(name: :filesystem_write_latency, value: be >= 0)) end end end context 'when popen always finds required binaries' do before do - allow(Gitlab::Popen).to receive(:popen).and_wrap_original do |method, *args, &block| + allow(described_class).to receive(:exec_with_timeout).and_wrap_original do |method, *args, &block| begin method.call(*args, &block) - rescue RuntimeError + rescue RuntimeError, Errno::ENOENT raise 'expected not to happen' end end + + stub_const('Gitlab::HealthChecks::FsShardsCheck::COMMAND_TIMEOUT', '10') end it_behaves_like 'filesystem checks' |