Allow configuring new circuitbreaker settings from the UI and API

7 files changed, 64 insertions, 11 deletions

diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb
index 1ee8911bb1a..cd1ecaadb85 100644
--- a/app/helpers/application_settings_helper.rb
+++ b/app/helpers/application_settings_helper.rb
@@ -120,6 +120,15 @@ module ApplicationSettingsHelper
     message.html_safe
   end
 
+  def circuitbreaker_access_retries_help_text
+    _('The number of attempts GitLab will make to access a storage.')
+  end
+
+  def circuitbreaker_backoff_threshold_help_text
+    _("The number of failures after which GitLab will start temporarily "\
+      "disabling access to a storage shard on a host")
+  end
+
   def circuitbreaker_failure_wait_time_help_text
     _("When access to a storage fails. GitLab will prevent access to the "\
       "storage for the time specified here. This allows the filesystem to "\
@@ -144,6 +153,8 @@ module ApplicationSettingsHelper
       :akismet_api_key,
       :akismet_enabled,
       :auto_devops_enabled,
+      :circuitbreaker_access_retries,
+      :circuitbreaker_backoff_threshold,
       :circuitbreaker_failure_count_threshold,
       :circuitbreaker_failure_reset_time,
       :circuitbreaker_failure_wait_time,
diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb
index 4dda276bb41..f266e7db6da 100644
--- a/app/models/application_setting.rb
+++ b/app/models/application_setting.rb
@@ -153,13 +153,25 @@ class ApplicationSetting < ActiveRecord::Base
             presence: true,
             numericality: { greater_than_or_equal_to: 0 }
 
-  validates :circuitbreaker_failure_count_threshold,
+  validates :circuitbreaker_backoff_threshold,
+            :circuitbreaker_failure_count_threshold,
             :circuitbreaker_failure_wait_time,
             :circuitbreaker_failure_reset_time,
             :circuitbreaker_storage_timeout,
             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_each :circuitbreaker_backoff_threshold do |record, attr, value|
+    if value.to_i >= record.circuitbreaker_failure_count_threshold
+      record.errors.add(attr, _("The circuitbreaker backoff threshold should be "\
+                                "lower than the failure count threshold"))
+    end
+  end
+
   SUPPORTED_KEY_TYPES.each do |type|
     validates :"#{type}_key_restriction", presence: true, key_restriction: { type: type }
   end
diff --git a/app/views/admin/application_settings/_form.html.haml b/app/views/admin/application_settings/_form.html.haml
index 2b23af9212e..3a4d5ce0b5c 100644
--- a/app/views/admin/application_settings/_form.html.haml
+++ b/app/views/admin/application_settings/_form.html.haml
@@ -533,11 +533,23 @@
   %fieldset
     %legend Git Storage Circuitbreaker settings
     .form-group
-      = f.label :circuitbreaker_failure_count_threshold, _('Maximum git storage failures'), class: 'control-label col-sm-2'
+      = f.label :circuitbreaker_access_retries, _('Number of access attempts'), class: 'control-label col-sm-2'
       .col-sm-10
-        = f.number_field :circuitbreaker_failure_count_threshold, class: 'form-control'
+        = f.number_field :circuitbreaker_access_retries, class: 'form-control'
         .help-block
-          = circuitbreaker_failure_count_help_text
+          = circuitbreaker_access_retries_help_text
+    .form-group
+      = f.label :circuitbreaker_storage_timeout, _('Seconds to wait for a storage access attempt'), class: 'control-label col-sm-2'
+      .col-sm-10
+        = f.number_field :circuitbreaker_storage_timeout, class: 'form-control'
+        .help-block
+          = circuitbreaker_storage_timeout_help_text
+    .form-group
+      = f.label :circuitbreaker_backoff_threshold, _('Number of failures before backing off'), class: 'control-label col-sm-2'
+      .col-sm-10
+        = f.number_field :circuitbreaker_backoff_threshold, class: 'form-control'
+        .help-block
+          = circuitbreaker_backoff_threshold_help_text
     .form-group
       = f.label :circuitbreaker_failure_wait_time, _('Seconds to wait after a storage failure'), class: 'control-label col-sm-2'
       .col-sm-10
@@ -545,17 +557,17 @@
         .help-block
           = circuitbreaker_failure_wait_time_help_text
     .form-group
-      = f.label :circuitbreaker_failure_reset_time, _('Seconds before reseting failure information'), class: 'control-label col-sm-2'
+      = f.label :circuitbreaker_failure_count_threshold, _('Maximum git storage failures'), class: 'control-label col-sm-2'
       .col-sm-10
-        = f.number_field :circuitbreaker_failure_reset_time, class: 'form-control'
+        = f.number_field :circuitbreaker_failure_count_threshold, class: 'form-control'
         .help-block
-          = circuitbreaker_failure_reset_time_help_text
+          = circuitbreaker_failure_count_help_text
     .form-group
-      = f.label :circuitbreaker_storage_timeout, _('Seconds to wait for a storage access attempt'), class: 'control-label col-sm-2'
+      = f.label :circuitbreaker_failure_reset_time, _('Seconds before reseting failure information'), class: 'control-label col-sm-2'
       .col-sm-10
-        = f.number_field :circuitbreaker_storage_timeout, class: 'form-control'
+        = f.number_field :circuitbreaker_failure_reset_time, class: 'form-control'
         .help-block
-          = circuitbreaker_storage_timeout_help_text
+          = circuitbreaker_failure_reset_time_help_text
 
   %fieldset
     %legend Repository Checks
diff --git a/doc/administration/img/circuitbreaker_config.png b/doc/administration/img/circuitbreaker_config.png
index 9250d38297c..e811d173634 100644
--- a/doc/administration/img/circuitbreaker_config.png
+++ b/doc/administration/img/circuitbreaker_config.png
diff --git a/doc/administration/repository_storage_paths.md b/doc/administration/repository_storage_paths.md
index efcabd69822..2ee8d78b2f0 100644
--- a/doc/administration/repository_storage_paths.md
+++ b/doc/administration/repository_storage_paths.md
@@ -109,6 +109,11 @@ 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
diff --git a/doc/api/settings.md b/doc/api/settings.md
index 664f3ef7b77..4e24e4bbfc3 100644
--- a/doc/api/settings.md
+++ b/doc/api/settings.md
@@ -69,6 +69,8 @@ PUT /application/settings
 | `after_sign_up_text`                     | string           | no                                            | Text shown to the user after signing up                                                                                                                                                                                                                                                                                                                                                                                                                   |
 | `akismet_api_key`                        | string           | no                                            | API key for akismet spam protection                                                                                                                                                                                                                                                                                                                                                                                                                       |
 | `akismet_enabled`                        | boolean          | no                                            | Enable or disable akismet spam protection                                                                                                                                                                                                                                                                                                                                                                                                                 |
+| `circuitbreaker_access_retries           | integer          | no                                            | The number of attempts GitLab will make to access a storage.                                                                                                                                                                                                                                                                                                                                                                                              |
+| `circuitbreaker_backoff_threshold        | integer          | no                                            | The number of failures after which GitLab will start temporarily disabling access to a storage shard on a host.                                                                                                                                                                                                                                                                                                                                                                                                                                                        |
 | `circuitbreaker_failure_count_threshold` | integer          | no                                            | The number of failures of after which GitLab will completely prevent access to the storage.                                                                                                                                                                                                                                                                                                                                                               |
 | `circuitbreaker_failure_reset_time`      | integer          | no                                            | Time in seconds GitLab will keep storage failure information. When no failures occur during this time, the failure information is reset.                                                                                                                                                                                                                                                                                                                  |
 | `circuitbreaker_failure_wait_time`       | integer          | no                                            | Time in seconds GitLab will block access to a failing storage to allow it to recover.                                                                                                                                                                                                                                                                                                                                                                     |
diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb
index 30495fd4f5e..47b7150d36f 100644
--- a/spec/models/application_setting_spec.rb
+++ b/spec/models/application_setting_spec.rb
@@ -115,7 +115,8 @@ describe ApplicationSetting do
     end
 
     context 'circuitbreaker settings' do
-      [:circuitbreaker_failure_count_threshold,
+      [:circuitbreaker_backoff_threshold,
+       :circuitbreaker_failure_count_threshold,
        :circuitbreaker_failure_wait_time,
        :circuitbreaker_failure_reset_time,
        :circuitbreaker_storage_timeout].each do |field|
@@ -125,6 +126,16 @@ describe ApplicationSetting do
                            .is_greater_than_or_equal_to(0)
         end
       end
+
+      it 'requires the `backoff_threshold` to be lower than the `failure_count_threshold`' do
+        setting.circuitbreaker_failure_count_threshold = 10
+        setting.circuitbreaker_backoff_threshold = 15
+        failure_message = "The circuitbreaker backoff threshold should be lower "\
+                          "than the failure count threshold"
+
+        expect(setting).not_to be_valid
+        expect(setting.errors[:circuitbreaker_backoff_threshold]).to include(failure_message)
+      end
     end
 
     context 'repository storages' do