diff options
author | Andrew Newdigate <andrew@gitlab.com> | 2017-11-01 16:38:59 +0300 |
---|---|---|
committer | Andrew Newdigate <andrew@gitlab.com> | 2017-11-01 16:38:59 +0300 |
commit | 4068bfb61d135b6f6c11d353da8c335d06beaf42 (patch) | |
tree | 0102418f54abdcc802670986f2f164319fcf4c30 | |
parent | 830ba638861f45c50af5210fff18955108397b61 (diff) |
Move gitaly timeouts into application settings, as per Jacob's review
-rw-r--r-- | app/helpers/application_settings_helper.rb | 2 | ||||
-rw-r--r-- | app/models/application_setting.rb | 9 | ||||
-rw-r--r-- | app/views/admin/application_settings/_form.html.haml | 18 | ||||
-rw-r--r-- | db/migrate/20171101130535_add_gitaly_timeout_properties_to_application_settings.rb | 19 | ||||
-rw-r--r-- | db/schema.rb | 4 | ||||
-rw-r--r-- | lib/api/settings.rb | 3 | ||||
-rw-r--r-- | lib/gitlab/gitaly_client.rb | 33 | ||||
-rw-r--r-- | lib/gitlab/gitaly_client/commit_service.rb | 30 | ||||
-rw-r--r-- | lib/gitlab/gitaly_client/repository_service.rb | 4 |
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 |