Merge branch 'zj-circuit-breaker-removal' into 'master'

Remove Git circuit breaker

Closes #45405

See merge request gitlab-org/gitlab-ce!22212

app/controllers/admin/health_check_controller.rb

@@ -3,12 +3,5 @@
 class Admin::HealthCheckController < Admin::ApplicationController
   def show
     @errors = HealthCheck::Utils.process_checks(['standard'])
-    @failing_storage_statuses = Gitlab::Git::Storage::Health.for_failing_storages
-  end
-
-  def reset_storage_health
-    Gitlab::Git::Storage::FailureInfo.reset_all!
-    redirect_to admin_health_check_path,
-                notice: _('Git storage health information has been reset')
   end
 end

app/controllers/application_controller.rb

@@ -66,7 +66,7 @@ class ApplicationController < ActionController::Base
     head :forbidden, retry_after: Gitlab::Auth::UniqueIpsLimiter.config.unique_ips_limit_time_window
   end
-  rescue_from Gitlab::Git::Storage::Inaccessible, GRPC::Unavailable, Gitlab::Git::CommandError do |exception|
+  rescue_from GRPC::Unavailable, Gitlab::Git::CommandError do |exception|
     log_exception(exception)
     headers['Retry-After'] = exception.retry_after if exception.respond_to?(:retry_after)

app/controllers/health_controller.rb

 # frozen_string_literal: true
 class HealthController < ActionController::Base
-  protect_from_forgery with: :exception, except: :storage_check, prepend: true
+  protect_from_forgery with: :exception, prepend: true
   include RequiresWhitelistedMonitoringClient
   CHECKS = [
@@ -25,15 +25,6 @@ class HealthController < ActionController::Base
     render_check_results(results)
   end
-  def storage_check
-    results = Gitlab::Git::Storage::Checker.check_all
-
-    render json: {
-             check_interval: Gitlab::CurrentSettings.current_application_settings.circuitbreaker_check_interval,
-             results: results
-           }
-  end
-
   private
   def render_check_results(results)

app/helpers/application_settings_helper.rb

@@ -108,37 +108,6 @@ module ApplicationSettingsHelper
     options_for_select(options, selected)
   end
-  def circuitbreaker_failure_count_help_text
-    health_link = link_to(s_('AdminHealthPageLink|health page'), admin_health_check_path)
-    api_link = link_to(s_('CircuitBreakerApiLink|circuitbreaker api'), help_page_path("api/repository_storage_health"))
-    message = _("The number of failures of after which GitLab will completely "\
-                "prevent access to the storage. The number of failures can be "\
-                "reset in the admin interface: %{link_to_health_page} or using "\
-                "the %{api_documentation_link}.")
-    message = message % { link_to_health_page: health_link, api_documentation_link: api_link }
-
-    message.html_safe
-  end
-
-  def circuitbreaker_access_retries_help_text
-    _('The number of attempts GitLab will make to access a storage.')
-  end
-
-  def circuitbreaker_failure_reset_time_help_text
-    _("The time in seconds GitLab will keep failure information. When no "\
-      "failures occur during this time, information about the mount is reset.")
-  end
-
-  def circuitbreaker_storage_timeout_help_text
-    _("The time in seconds GitLab will try to access storage. After this time a "\
-      "timeout error will be raised.")
-  end
-
-  def circuitbreaker_check_interval_help_text
-    _("The time in seconds between storage checks. When a previous check did "\
-      "complete yet, GitLab will skip a check.")
-  end
-
   def visible_attributes
     [
       :admin_notification_email,
@@ -150,11 +119,6 @@ module ApplicationSettingsHelper
       :authorized_keys_enabled,
       :auto_devops_enabled,
       :auto_devops_domain,
-      :circuitbreaker_access_retries,
-      :circuitbreaker_check_interval,
-      :circuitbreaker_failure_count_threshold,
-      :circuitbreaker_failure_reset_time,
-      :circuitbreaker_storage_timeout,
       :clientside_sentry_dsn,
       :clientside_sentry_enabled,
       :container_registry_token_expire_delay,

app/helpers/storage_health_helper.rb

-# frozen_string_literal: true
-
-module StorageHealthHelper
-  def failing_storage_health_message(storage_health)
-    storage_name = content_tag(:strong, h(storage_health.storage_name))
-    host_names = h(storage_health.failing_on_hosts.to_sentence)
-    translation_params = { storage_name: storage_name,
-                           host_names: host_names,
-                           failed_attempts: storage_health.total_failures }
-
-    translation = n_('%{storage_name}: failed storage access attempt on host:',
-                     '%{storage_name}: %{failed_attempts} failed storage access attempts:',
-                     storage_health.total_failures) % translation_params
-
-    translation.html_safe
-  end
-
-  def message_for_circuit_breaker(circuit_breaker)
-    maximum_failures = circuit_breaker.failure_count_threshold
-    current_failures = circuit_breaker.failure_count
-
-    translation_params = { number_of_failures: current_failures,
-                           maximum_failures: maximum_failures }
-
-    if circuit_breaker.circuit_broken?
-      s_("%{number_of_failures} of %{maximum_failures} failures. GitLab will not "\
-         "retry automatically. Reset storage information when the problem is "\
-         "resolved.") % translation_params
-    else
-      _("%{number_of_failures} of %{maximum_failures} failures. GitLab will "\
-        "allow access on the next attempt.") % translation_params
-    end
-  end
-end

app/models/application_setting.rb

@@ -4,6 +4,7 @@ class ApplicationSetting < ActiveRecord::Base
   include CacheableAttributes
   include CacheMarkdownField
   include TokenAuthenticatable
+  include IgnorableColumn
   add_authentication_token_field :runners_registration_token
   add_authentication_token_field :health_check_access_token
@@ -27,6 +28,12 @@ class ApplicationSetting < ActiveRecord::Base
   serialize :domain_blacklist, Array # rubocop:disable Cop/ActiveRecordSerialize
   serialize :repository_storages # rubocop:disable Cop/ActiveRecordSerialize
+  ignore_column :circuitbreaker_failure_count_threshold
+  ignore_column :circuitbreaker_failure_reset_time
+  ignore_column :circuitbreaker_storage_timeout
+  ignore_column :circuitbreaker_access_retries
+  ignore_column :circuitbreaker_check_interval
+
   cache_markdown_field :sign_in_text
   cache_markdown_field :help_page_text
   cache_markdown_field :shared_runners_text, pipeline: :plain_markdown
@@ -150,17 +157,6 @@ class ApplicationSetting < ActiveRecord::Base
             presence: true,
             numericality: { greater_than_or_equal_to: 0 }
-  validates :circuitbreaker_failure_count_threshold,
-            :circuitbreaker_failure_reset_time,
-            :circuitbreaker_storage_timeout,
-            :circuitbreaker_check_interval,
-            presence: true,
-            numericality: { only_integer: true, greater_than_or_equal_to: 0 }
-
-  validates :circuitbreaker_access_retries,
-            presence: true,
-            numericality: { only_integer: true, greater_than_or_equal_to: 1 }
-
   validates :gitaly_timeout_default,
             presence: true,
             numericality: { only_integer: true, greater_than_or_equal_to: 0 }

app/views/admin/application_settings/_repository_storage.html.haml

@@ -20,32 +20,5 @@
           Manage repository storage paths. Learn more in the
           = succeed "." do
             = link_to "repository storages documentation", help_page_path("administration/repository_storage_paths")
-    .sub-section
-      %h4 Circuit breaker
-      .form-group
-        = f.label :circuitbreaker_check_interval, _('Check interval'), class: 'label-bold'
-        = f.number_field :circuitbreaker_check_interval, class: 'form-control'
-        .form-text.text-muted
-          = circuitbreaker_check_interval_help_text
-      .form-group
-        = f.label :circuitbreaker_access_retries, _('Number of access attempts'), class: 'label-bold'
-        = f.number_field :circuitbreaker_access_retries, class: 'form-control'
-        .form-text.text-muted
-          = circuitbreaker_access_retries_help_text
-      .form-group
-        = f.label :circuitbreaker_storage_timeout, _('Seconds to wait for a storage access attempt'), class: 'label-bold'
-        = f.number_field :circuitbreaker_storage_timeout, class: 'form-control'
-        .form-text.text-muted
-          = circuitbreaker_storage_timeout_help_text
-      .form-group
-        = f.label :circuitbreaker_failure_count_threshold, _('Maximum git storage failures'), class: 'label-bold'
-        = f.number_field :circuitbreaker_failure_count_threshold, class: 'form-control'
-        .form-text.text-muted
-          = circuitbreaker_failure_count_help_text
-      .form-group
-        = f.label :circuitbreaker_failure_reset_time, _('Seconds before reseting failure information'), class: 'label-bold'
-        = f.number_field :circuitbreaker_failure_reset_time, class: 'form-control'
-        .form-text.text-muted
-          = circuitbreaker_failure_reset_time_help_text
   = f.submit 'Save changes', class: "btn btn-success qa-save-changes-button"

app/views/admin/application_settings/repository.html.haml

@@ -20,7 +20,7 @@
     %button.btn.btn-default.js-settings-toggle{ type: 'button' }
       = expanded_by_default? ? _('Collapse') : _('Expand')
     %p
-      = _('Configure storage path and circuit breaker settings.')
+      = _('Configure storage path settings.')
   .settings-content
     = render 'repository_storage'

app/views/admin/health_check/_failing_storages.html.haml

-- if failing_storages.any?
-  = _('There are problems accessing Git storage: ')
-  %ul
-    - failing_storages.each do |storage_health|
-      %li
-        = failing_storage_health_message(storage_health)
-        %ul
-          - storage_health.failing_circuit_breakers.each do |circuit_breaker|
-            %li
-              #{circuit_breaker.hostname}: #{message_for_circuit_breaker(circuit_breaker)}
-
-  = _("Access to failing storages has been temporarily disabled to allow the mount to recover. Reset storage information after the issue has been resolved to allow access again.")
-  .prepend-top-10
-    = button_to _("Reset git storage health information"), reset_storage_health_admin_health_check_path,
-      method: :post, class: 'btn btn-default'

app/views/admin/health_check/show.html.haml

 - @no_container = true
 - page_title _('Health Check')
-- no_errors = @errors.blank? && @failing_storage_statuses.blank?
+- no_errors = @errors.blank?
 %div{ class: container_class }
   %h3.page-title= page_title
@@ -39,4 +39,3 @@
         #{ s_('HealthCheck|No Health Problems Detected') }
       - else
         = @errors
-        = render partial: 'failing_storages', object: @failing_storage_statuses

bin/storage_check

-#!/usr/bin/env ruby
-
-require 'optparse'
-require 'net/http'
-require 'json'
-require 'socket'
-require 'logger'
-
-require_relative '../lib/gitlab/storage_check'
-
-Gitlab::StorageCheck::CLI.start!(ARGV)

changelogs/unreleased/zj-circuit-breaker-removal.yml

+---
+title: Remove Git circuit breaker
+merge_request: 22212
+author:
+type: removed

config/routes.rb

@@ -56,7 +56,6 @@ Rails.application.routes.draw do
     # '/-/health' implemented by BasicHealthMiddleware
     get 'liveness' => 'health#liveness'
     get 'readiness' => 'health#readiness'
-    post 'storage_check' => 'health#storage_check'
     resources :metrics, only: [:index]
     mount Peek::Railtie => '/peek', as: 'peek_routes'

config/routes/admin.rb

@@ -69,9 +69,7 @@ namespace :admin do
   end
   resource :logs, only: [:show]
-  resource :health_check, controller: 'health_check', only: [:show] do
-    post :reset_storage_health
-  end
+  resource :health_check, controller: 'health_check', only: [:show]
   resource :background_jobs, controller: 'background_jobs', only: [:show]
   resource :system_info, controller: 'system_info', only: [:show]
   resources :requests_profiles, only: [:index, :show], param: :name, constraints: { name: /.+\.html/ }

db/post_migrate/20181008200441_remove_circuit_breaker.rb

+# frozen_string_literal: true
+
+class RemoveCircuitBreaker < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  CIRCUIT_BREAKER_COLUMS_WITH_DEFAULT = {
+    circuitbreaker_failure_count_threshold: 3,
+    circuitbreaker_failure_reset_time: 1800,
+    circuitbreaker_storage_timeout: 15,
+    circuitbreaker_access_retries: 3,
+    circuitbreaker_check_interval: 1
+  }.freeze
+
+  def up
+    CIRCUIT_BREAKER_COLUMS_WITH_DEFAULT.keys.each do |column|
+      remove_column(:application_settings, column) if column_exists?(:application_settings, column)
+    end
+  end
+
+  def down
+    CIRCUIT_BREAKER_COLUMS_WITH_DEFAULT.each do |column, default|
+      add_column_with_default(:application_settings, column, :integer, default: default) unless column_exists?(:application_settings, column)
+    end
+  end
+end

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: 20181008145359) do
+ActiveRecord::Schema.define(version: 20181008200441) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -139,10 +139,6 @@ ActiveRecord::Schema.define(version: 20181008145359) do
     t.boolean "hashed_storage_enabled", default: false, null: false
     t.boolean "project_export_enabled", default: true, null: false
     t.boolean "auto_devops_enabled", default: true, null: false
-    t.integer "circuitbreaker_failure_count_threshold", default: 3
-    t.integer "circuitbreaker_failure_reset_time", default: 1800
-    t.integer "circuitbreaker_storage_timeout", default: 15
-    t.integer "circuitbreaker_access_retries", default: 3
     t.boolean "throttle_unauthenticated_enabled", default: false, null: false
     t.integer "throttle_unauthenticated_requests_per_period", default: 3600, null: false
     t.integer "throttle_unauthenticated_period_in_seconds", default: 3600, null: false
@@ -152,7 +148,6 @@ ActiveRecord::Schema.define(version: 20181008145359) do
     t.boolean "throttle_authenticated_web_enabled", default: false, null: false
     t.integer "throttle_authenticated_web_requests_per_period", default: 7200, null: false
     t.integer "throttle_authenticated_web_period_in_seconds", default: 3600, null: false
-    t.integer "circuitbreaker_check_interval", default: 1, null: false
     t.boolean "password_authentication_enabled_for_web"
     t.boolean "password_authentication_enabled_for_git", default: true
     t.integer "gitaly_timeout_default", default: 55, null: false

doc/administration/monitoring/prometheus/gitlab_metrics.md

@@ -45,9 +45,6 @@ The following metrics are available:
 | redis_ping_success                | Gauge     | 9.4   | Whether or not the last redis ping succeeded |
 | redis_ping_latency_seconds        | Gauge     | 9.4   | Round trip time of the redis ping |
 | user_session_logins_total         | Counter   | 9.4   | Counter of how many users have logged in |
-| filesystem_circuitbreaker_latency_seconds | Gauge | 9.5 | Time spent validating if a storage is accessible |
-| filesystem_circuitbreaker         | Gauge     | 9.5   | Whether or not the circuit for a certain shard is broken or not |
-| circuitbreaker_storage_check_duration_seconds | Histogram | 10.3 | Time a single storage probe took |
 | failed_login_captcha_total        | Gauge | 11.0 | Counter of failed CAPTCHA attempts during login |
 | successful_login_captcha_total    | Gauge | 11.0 | Counter of successful CAPTCHA attempts during login |

doc/administration/repository_storage_paths.md

@@ -97,55 +97,6 @@ be stored via the **Application Settings** in the Admin area.
 Beginning with GitLab 8.13.4, multiple paths can be chosen. New projects will be
 randomly placed on one of the selected paths.
-## Handling failing repository storage
-
-> [Introduced][ce-11449] in GitLab 9.5.
-
-When GitLab detects access to the repositories storage fails repeatedly, it can
-gracefully prevent attempts to access the storage. This might be useful when
-the repositories are stored somewhere on the network.
-
-This can be configured from the admin interface:
-
-![circuitbreaker configuration](img/circuitbreaker_config.png)
-
-**Number of access attempts**: The number of attempts GitLab will make to access a
-storage when probing a shard.
-
-**Number of failures before backing off**: The number of failures after which
-GitLab will start temporarily disabling access to a storage shard on a host.
-
-**Maximum git storage failures:** The number of failures of after which GitLab will
-completely prevent access to the storage. The number of failures can be reset in
-the admin interface: `https://gitlab.example.com/admin/health_check` or using the
-[api](../api/repository_storage_health.md) to allow access to the storage again.
-
-**Seconds to wait after a storage failure:** When access to a storage fails. GitLab
-will prevent access to the storage for the time specified here. This allows the
-filesystem to recover.
-
-**Seconds before reseting failure information:** The time in seconds GitLab will
-keep failure information. When no failures occur during this time, information about the
-mount is reset.
-
-**Seconds to wait for a storage access attempt:** The time in seconds GitLab will
-try to access storage. After this time a timeout error will be raised.
-
-To enable the circuitbreaker for repository storage you can flip the feature flag from a rails console:
-
-```
-Feature.enable('git_storage_circuit_breaker')
-```
-
-Alternatively it can be enabled by setting `true` in the `GIT_STORAGE_CIRCUIT_BREAKER` environment variable.
-This approach would be used when enabling the circuit breaker on a single host.
-
-When storage failures occur, this will be visible in the admin interface like this:
-
-![failing storage](img/failing_storage.png)
-
-To allow access to all storages, click the `Reset git storage health information` button.
-
 [ce-4578]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4578
 [restart-gitlab]: restart_gitlab.md#installations-from-source
 [reconfigure-gitlab]: restart_gitlab.md#omnibus-gitlab-reconfigure