Fix transient CI errors by increasing command execution timeouts from 1s to 30s

+ actually make local tests correctly detect wether 'timeout' or 'gtimeout' is available

3 files changed, 73 insertions, 7 deletions

diff --git a/lib/gitlab/health_checks/fs_shards_check.rb b/lib/gitlab/health_checks/fs_shards_check.rb
index df962d203b7..90612af3d63 100644
--- a/lib/gitlab/health_checks/fs_shards_check.rb
+++ b/lib/gitlab/health_checks/fs_shards_check.rb
@@ -42,6 +42,7 @@ module Gitlab
         private
 
         RANDOM_STRING = SecureRandom.hex(1000).freeze
+        COMMAND_TIMEOUT = 1.second
 
         def operation_metrics(ok_metric, latency_metric, operation, **labels)
           with_timing operation do |result, elapsed|
@@ -64,7 +65,11 @@ module Gitlab
         end
 
         def with_timeout(args)
-          %w{timeout 1}.concat(args)
+          %W{timeout #{COMMAND_TIMEOUT.to_i}}.concat(args)
+        end
+
+        def exec_with_timeout(cmd_args, *args, &block)
+          Gitlab::Popen.popen(with_timeout(cmd_args), *args, &block)
         end
 
         def tmp_file_path(storage_name)
@@ -78,7 +83,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 +91,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 +101,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 +111,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..289c93ecde7 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,8 @@
 require 'spec_helper'
 
 describe Gitlab::HealthChecks::FsShardsCheck do
+  include TimeoutHelper
+
   let(:metric_class) { Gitlab::HealthChecks::Metric }
   let(:result_class) { Gitlab::HealthChecks::Result }
   let(:repository_storages) { [:default] }
@@ -103,15 +105,51 @@ describe Gitlab::HealthChecks::FsShardsCheck do
     end
   end
 
+  context 'when timeout kills fs checks' do
+    let(:timeout_seconds) { 1.to_s }
+
+    before do
+      skip 'timeout or gtimeout not available' unless any_timeout_command_exists?
+
+      allow(described_class).to receive(:with_timeout) { [timeout_command, timeout_seconds].concat(%w{ sleep 2 }) }
+      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 { 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 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 })) }
+    end
+  end
+
   context 'when popen always finds required binaries' do
+    let(:timeout_seconds) { 30.to_s }
     before do
-      allow(Gitlab::Popen).to receive(:popen).and_wrap_original do |method, *args, &block|
+      skip 'timeout or gtimeout not available' unless any_timeout_command_exists?
+
+      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
+
+      allow(described_class).to receive(:with_timeout) do |args, &block|
+        [timeout_command, timeout_seconds].concat(args)
+      end
     end
 
     it_behaves_like 'filesystem checks'
diff --git a/spec/support/timeout_helper.rb b/spec/support/timeout_helper.rb
new file mode 100644
index 00000000000..8b1c14ad2d5
--- /dev/null
+++ b/spec/support/timeout_helper.rb
@@ -0,0 +1,23 @@
+module TimeoutHelper
+  def command_exists?(command)
+    _, status = Gitlab::Popen.popen(%W{ #{command} 1 echo })
+    status == 0
+  rescue Errno::ENOENT
+    false
+  end
+
+  def any_timeout_command_exists?
+    command_exists?('timeout') || command_exists?('gtimeout')
+  end
+
+  def timeout_command
+    @timeout_command ||=
+      if command_exists?('timeout')
+        'timeout'
+      elsif command_exists?('gtimeout')
+        'gtimeout'
+      else
+        ''
+      end
+  end
+end