Move gitaly timeouts into application settings, as per Jacob's review

9 files changed, 96 insertions, 26 deletions

diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb
index 1ee8911bb1a..467daf0d99a 100644
--- a/app/helpers/application_settings_helper.rb
+++ b/app/helpers/application_settings_helper.rb
@@ -166,6 +166,8 @@ module ApplicationSettingsHelper
       :ed25519_key_restriction,
       :email_author_in_body,
       :enabled_git_access_protocol,
+      :gitaly_timeout_default,
+      :gitaly_timeout_fast,
       :gravatar_enabled,
       :hashed_storage_enabled,
       :help_page_hide_commercial_content,
diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb
index 4dda276bb41..4e04a8e1b8c 100644
--- a/app/models/application_setting.rb
+++ b/app/models/application_setting.rb
@@ -160,6 +160,11 @@ class ApplicationSetting < ActiveRecord::Base
             presence: true,
             numericality: { only_integer: true, greater_than_or_equal_to: 0 }
 
+  validates :gitaly_timeout_fast,
+            :gitaly_timeout_default,
+            presence: true,
+            numericality: { only_integer: true, greater_than_or_equal_to: 0 }
+
   SUPPORTED_KEY_TYPES.each do |type|
     validates :"#{type}_key_restriction", presence: true, key_restriction: { type: type }
   end
@@ -286,7 +291,9 @@ class ApplicationSetting < ActiveRecord::Base
       two_factor_grace_period: 48,
       user_default_external: false,
       polling_interval_multiplier: 1,
-      usage_ping_enabled: Settings.gitlab['usage_ping_enabled']
+      usage_ping_enabled: Settings.gitlab['usage_ping_enabled'],
+      gitaly_timeout_fast: 10,
+      gitaly_timeout_default: 60
     }
   end
 
diff --git a/app/views/admin/application_settings/_form.html.haml b/app/views/admin/application_settings/_form.html.haml
index 2b23af9212e..f8f029c7763 100644
--- a/app/views/admin/application_settings/_form.html.haml
+++ b/app/views/admin/application_settings/_form.html.haml
@@ -707,6 +707,24 @@
           Number of Git pushes after which 'git gc' is run.
 
   %fieldset
+    %legend Gitaly Timeouts
+    .form-group
+      = f.label :gitaly_timeout_default, 'Default Timeout Period', class: 'control-label col-sm-2'
+      .col-sm-10
+        = f.number_field :gitaly_timeout_default, class: 'form-control'
+        .help-block
+          Timeout for Gitaly calls from the GitLab application (in seconds). This timeout is not enforced
+          for git fetch/push operations or Sidekiq jobs.
+    .form-group
+      = f.label :gitaly_timeout_fast, 'Fast Timeout Period', class: 'control-label col-sm-2'
+      .col-sm-10
+        = f.number_field :gitaly_timeout_fast, class: 'form-control'
+        .help-block
+          Fast operation timeout (in seconds). Some Gitaly operations are expected to be fast.
+          If they exceed this threshold, there may be a problem with a storage shard and 'failing fast'
+          can help maintain the stability of the GitLab instance.
+
+  %fieldset
     %legend Web terminal
     .form-group
       = f.label :terminal_max_session_time, 'Max session time', class: 'control-label col-sm-2'
diff --git a/db/migrate/20171101130535_add_gitaly_timeout_properties_to_application_settings.rb b/db/migrate/20171101130535_add_gitaly_timeout_properties_to_application_settings.rb
new file mode 100644
index 00000000000..d91831ba479
--- /dev/null
+++ b/db/migrate/20171101130535_add_gitaly_timeout_properties_to_application_settings.rb
@@ -0,0 +1,19 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddGitalyTimeoutPropertiesToApplicationSettings < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    add_column :application_settings,
+               :gitaly_timeout_default,
+               :integer,
+              default: 55
+    add_column :application_settings,
+               :gitaly_timeout_fast,
+               :integer,
+               default: 10
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index c2c04873d4d..3ce69ecb6e2 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20171012101043) do
+ActiveRecord::Schema.define(version: 20171101130535) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -138,6 +138,8 @@ ActiveRecord::Schema.define(version: 20171012101043) do
     t.integer "circuitbreaker_failure_wait_time", default: 30
     t.integer "circuitbreaker_failure_reset_time", default: 1800
     t.integer "circuitbreaker_storage_timeout", default: 30
+    t.integer "gitaly_timeout_default", default: 55
+    t.integer "gitaly_timeout_fast", default: 15
   end
 
   create_table "audit_events", force: :cascade do |t|
diff --git a/lib/api/settings.rb b/lib/api/settings.rb
index 851b226e9e5..421a44eeaf4 100644
--- a/lib/api/settings.rb
+++ b/lib/api/settings.rb
@@ -122,6 +122,9 @@ module API
       optional :terminal_max_session_time, type: Integer, desc: 'Maximum time for web terminal websocket connection (in seconds). Set to 0 for unlimited time.'
       optional :polling_interval_multiplier, type: BigDecimal, desc: 'Interval multiplier used by endpoints that perform polling. Set to 0 to disable polling.'
 
+      optional :gitaly_timeout_default, type: Integer, desc: 'Default Gitaly timeout, in seconds. Set to 0 to disable timeouts.'
+      optional :gitaly_timeout_fast, type: Integer, desc: 'Gitaly fast operation timeout, in seconds. Set to 0 to disable timeouts.'
+
       ApplicationSetting::SUPPORTED_KEY_TYPES.each do |type|
         optional :"#{type}_key_restriction",
                  type: Integer,
diff --git a/lib/gitlab/gitaly_client.rb b/lib/gitlab/gitaly_client.rb
index 1c168272424..ef164e4d8f9 100644
--- a/lib/gitlab/gitaly_client.rb
+++ b/lib/gitlab/gitaly_client.rb
@@ -30,11 +30,6 @@ module Gitlab
     MAXIMUM_GITALY_CALLS = 30
     CLIENT_NAME = (Sidekiq.server? ? 'gitlab-sidekiq' : 'gitlab-web').freeze
 
-    # The default timeout on all Gitaly calls
-    DEFAULT_TIMEOUT = Sidekiq.server? ? 0.seconds : 50.seconds
-    FAST_TIMEOUT = 10.seconds
-    MEDIUM_TIMEOUT = 30.seconds
-
     MUTEX = Mutex.new
     private_constant :MUTEX
 
@@ -93,7 +88,7 @@ module Gitlab
     #   kwargs.merge(deadline: Time.now + 10)
     # end
     #
-    def self.call(storage, service, rpc, request, timeout: DEFAULT_TIMEOUT)
+    def self.call(storage, service, rpc, request, timeout: nil)
       start = Process.clock_gettime(Process::CLOCK_MONOTONIC)
       enforce_gitaly_request_limits(:call)
 
@@ -118,7 +113,10 @@ module Gitlab
 
       result = { metadata: metadata }
 
-      return result unless !timeout.nil? && timeout > 0
+      # nil timeout indicates that we should use the default
+      timeout = default_timeout if timeout.nil?
+
+      return result unless timeout > 0
 
       # Do not use `Time.now` for deadline calculation, since it
       # will be affected by Timecop in some tests, but grpc's c-core
@@ -298,6 +296,27 @@ module Gitlab
       Google::Protobuf::RepeatedField.new(:bytes, a.map { |s| self.encode(s) } )
     end
 
+    # The default timeout on all Gitaly calls
+    def self.default_timeout
+      return 0 if Sidekiq.server?
+
+      Gitlab::CurrentSettings.current_application_settings.gitaly_timeout_default
+    end
+
+    def self.fast_timeout
+      gitaly_timeout_fast = Gitlab::CurrentSettings.current_application_settings.gitaly_timeout_fast
+
+      [gitaly_timeout_fast, Gitlab::CurrentSettings.current_application_settings.gitaly_timeout_default].min
+    end
+
+    # Medium timeout is enforced in Sidekiq
+    # it's the lowest of thrice the fast timeout or the default timeout
+    def self.medium_timeout
+      gitaly_timeout_medium = Gitlab::CurrentSettings.current_application_settings.gitaly_timeout_fast * 3
+
+      [gitaly_timeout_medium, Gitlab::CurrentSettings.current_application_settings.gitaly_timeout_default].min
+    end
+
     # Count a stack. Used for n+1 detection
     def self.count_stack
       return unless RequestStore.active?
diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb
index 45be84ff097..1abf418c37d 100644
--- a/lib/gitlab/gitaly_client/commit_service.rb
+++ b/lib/gitlab/gitaly_client/commit_service.rb
@@ -16,7 +16,7 @@ module Gitlab
           revision: GitalyClient.encode(revision)
         )
 
-        response = GitalyClient.call(@repository.storage, :commit_service, :list_files, request, timeout: GitalyClient::MEDIUM_TIMEOUT)
+        response = GitalyClient.call(@repository.storage, :commit_service, :list_files, request, timeout: GitalyClient.medium_timeout)
         response.flat_map do |msg|
           msg.paths.map { |d| d.dup.force_encoding(Encoding::UTF_8) }
         end
@@ -29,7 +29,7 @@ module Gitlab
           child_id: child_id
         )
 
-        GitalyClient.call(@repository.storage, :commit_service, :commit_is_ancestor, request, timeout: GitalyClient::FAST_TIMEOUT).value
+        GitalyClient.call(@repository.storage, :commit_service, :commit_is_ancestor, request, timeout: GitalyClient.fast_timeout).value
       end
 
       def diff(from, to, options = {})
@@ -77,7 +77,7 @@ module Gitlab
           limit: limit.to_i
         )
 
-        response = GitalyClient.call(@repository.storage, :commit_service, :tree_entry, request, timeout: GitalyClient::MEDIUM_TIMEOUT)
+        response = GitalyClient.call(@repository.storage, :commit_service, :tree_entry, request, timeout: GitalyClient.medium_timeout)
 
         entry = nil
         data = ''
@@ -102,7 +102,7 @@ module Gitlab
           path: path.present? ? GitalyClient.encode(path) : '.'
         )
 
-        response = GitalyClient.call(@repository.storage, :commit_service, :get_tree_entries, request, timeout: GitalyClient::MEDIUM_TIMEOUT)
+        response = GitalyClient.call(@repository.storage, :commit_service, :get_tree_entries, request, timeout: GitalyClient.medium_timeout)
 
         response.flat_map do |message|
           message.entries.map do |gitaly_tree_entry|
@@ -129,7 +129,7 @@ module Gitlab
         request.before = Google::Protobuf::Timestamp.new(seconds: options[:before].to_i) if options[:before].present?
         request.path = options[:path] if options[:path].present?
 
-        GitalyClient.call(@repository.storage, :commit_service, :count_commits, request, timeout: GitalyClient::MEDIUM_TIMEOUT).count
+        GitalyClient.call(@repository.storage, :commit_service, :count_commits, request, timeout: GitalyClient.medium_timeout).count
       end
 
       def last_commit_for_path(revision, path)
@@ -139,7 +139,7 @@ module Gitlab
           path: GitalyClient.encode(path.to_s)
         )
 
-        gitaly_commit = GitalyClient.call(@repository.storage, :commit_service, :last_commit_for_path, request, timeout: GitalyClient::FAST_TIMEOUT).commit
+        gitaly_commit = GitalyClient.call(@repository.storage, :commit_service, :last_commit_for_path, request, timeout: GitalyClient.fast_timeout).commit
         return unless gitaly_commit
 
         Gitlab::Git::Commit.new(@repository, gitaly_commit)
@@ -152,7 +152,7 @@ module Gitlab
           to: to
         )
 
-        response = GitalyClient.call(@repository.storage, :commit_service, :commits_between, request, timeout: GitalyClient::MEDIUM_TIMEOUT)
+        response = GitalyClient.call(@repository.storage, :commit_service, :commits_between, request, timeout: GitalyClient.medium_timeout)
         consume_commits_response(response)
       end
 
@@ -165,7 +165,7 @@ module Gitlab
         )
         request.order = opts[:order].upcase if opts[:order].present?
 
-        response = GitalyClient.call(@repository.storage, :commit_service, :find_all_commits, request, timeout: GitalyClient::MEDIUM_TIMEOUT)
+        response = GitalyClient.call(@repository.storage, :commit_service, :find_all_commits, request, timeout: GitalyClient.medium_timeout)
         consume_commits_response(response)
       end
 
@@ -179,7 +179,7 @@ module Gitlab
           offset: offset.to_i
         )
 
-        response = GitalyClient.call(@repository.storage, :commit_service, :commits_by_message, request, timeout: GitalyClient::MEDIUM_TIMEOUT)
+        response = GitalyClient.call(@repository.storage, :commit_service, :commits_by_message, request, timeout: GitalyClient.medium_timeout)
         consume_commits_response(response)
       end
 
@@ -197,7 +197,7 @@ module Gitlab
           path: GitalyClient.encode(path)
         )
 
-        response = GitalyClient.call(@repository.storage, :commit_service, :raw_blame, request, timeout: GitalyClient::MEDIUM_TIMEOUT)
+        response = GitalyClient.call(@repository.storage, :commit_service, :raw_blame, request, timeout: GitalyClient.medium_timeout)
         response.reduce("") { |memo, msg| memo << msg.data }
       end
 
@@ -207,7 +207,7 @@ module Gitlab
           revision: GitalyClient.encode(revision)
         )
 
-        response = GitalyClient.call(@repository.storage, :commit_service, :find_commit, request, timeout: GitalyClient::MEDIUM_TIMEOUT)
+        response = GitalyClient.call(@repository.storage, :commit_service, :find_commit, request, timeout: GitalyClient.medium_timeout)
 
         response.commit
       end
@@ -217,7 +217,7 @@ module Gitlab
           repository: @gitaly_repo,
           revision: GitalyClient.encode(revision)
         )
-        response = GitalyClient.call(@repository.storage, :diff_service, :commit_patch, request, timeout: GitalyClient::MEDIUM_TIMEOUT)
+        response = GitalyClient.call(@repository.storage, :diff_service, :commit_patch, request, timeout: GitalyClient.medium_timeout)
 
         response.sum(&:data)
       end
@@ -227,7 +227,7 @@ module Gitlab
           repository: @gitaly_repo,
           revision: GitalyClient.encode(revision)
         )
-        GitalyClient.call(@repository.storage, :commit_service, :commit_stats, request, timeout: GitalyClient::MEDIUM_TIMEOUT)
+        GitalyClient.call(@repository.storage, :commit_service, :commit_stats, request, timeout: GitalyClient.medium_timeout)
       end
 
       def find_commits(options)
@@ -245,7 +245,7 @@ module Gitlab
 
         request.paths = GitalyClient.encode_repeated(Array(options[:path])) if options[:path].present?
 
-        response = GitalyClient.call(@repository.storage, :commit_service, :find_commits, request, timeout: GitalyClient::MEDIUM_TIMEOUT)
+        response = GitalyClient.call(@repository.storage, :commit_service, :find_commits, request, timeout: GitalyClient.medium_timeout)
 
         consume_commits_response(response)
       end
@@ -259,7 +259,7 @@ module Gitlab
         request_params.merge!(Gitlab::Git::DiffCollection.collection_limits(options).to_h)
 
         request = Gitaly::CommitDiffRequest.new(request_params)
-        response = GitalyClient.call(@repository.storage, :diff_service, :commit_diff, request, timeout: GitalyClient::MEDIUM_TIMEOUT)
+        response = GitalyClient.call(@repository.storage, :diff_service, :commit_diff, request, timeout: GitalyClient.medium_timeout)
         GitalyClient::DiffStitcher.new(response)
       end
 
diff --git a/lib/gitlab/gitaly_client/repository_service.rb b/lib/gitlab/gitaly_client/repository_service.rb
index 9d5646128ca..ab80fc4c3fb 100644
--- a/lib/gitlab/gitaly_client/repository_service.rb
+++ b/lib/gitlab/gitaly_client/repository_service.rb
@@ -10,7 +10,7 @@ module Gitlab
       def exists?
         request = Gitaly::RepositoryExistsRequest.new(repository: @gitaly_repo)
 
-        response = GitalyClient.call(@storage, :repository_service, :repository_exists, request, timeout: GitalyClient::FAST_TIMEOUT)
+        response = GitalyClient.call(@storage, :repository_service, :repository_exists, request, timeout: GitalyClient.fast_timeout)
 
         response.exists
       end
@@ -64,7 +64,7 @@ module Gitlab
 
       def has_local_branches?
         request = Gitaly::HasLocalBranchesRequest.new(repository: @gitaly_repo)
-        response = GitalyClient.call(@storage, :repository_service, :has_local_branches, request, timeout: GitalyClient::FAST_TIMEOUT)
+        response = GitalyClient.call(@storage, :repository_service, :has_local_branches, request, timeout: GitalyClient.fast_timeout)
 
         response.value
       end