Merge branch 'bvl-validate-changed-values' into 'master'

Only validate gitaly timeouts when they changed

Closes #208432

See merge request gitlab-org/gitlab!26292

app/models/application_setting.rb

@@ -172,6 +172,7 @@ class ApplicationSetting < ApplicationRecord
 
   validates :gitaly_timeout_default,
             presence: true,
+            if: :gitaly_timeout_default_changed?,
             numericality: {
               only_integer: true,
               greater_than_or_equal_to: 0,
@@ -180,6 +181,7 @@ class ApplicationSetting < ApplicationRecord
 
   validates :gitaly_timeout_medium,
             presence: true,
+            if: :gitaly_timeout_medium_changed?,
             numericality: { only_integer: true, greater_than_or_equal_to: 0 }
   validates :gitaly_timeout_medium,
             numericality: { less_than_or_equal_to: :gitaly_timeout_default },
@@ -190,6 +192,7 @@ class ApplicationSetting < ApplicationRecord
 
   validates :gitaly_timeout_fast,
             presence: true,
+            if: :gitaly_timeout_fast_changed?,
             numericality: { only_integer: true, greater_than_or_equal_to: 0 }
   validates :gitaly_timeout_fast,
             numericality: { less_than_or_equal_to: :gitaly_timeout_default },

changelogs/unreleased/bvl-validate-changed-values.yml

+---
+title: Fix saving preferences with unrelated changes when gitaly timeouts became invalid.
+merge_request: 26292
+author:
+type: fixed

doc/user/admin_area/settings/gitaly_timeouts.md

+---
+type: reference
+---
+
+# Gitaly timeouts
+
+![gitaly timeouts](img/gitaly_timeouts.png)
+
+3 timeout types can be configured to make sure that long running
+Gitaly calls don't needlessly take up resources.
+
+- Default timeout
+
+This timeout is the default for most Gitaly calls.
+It should be shorter than the worker timeout that can be configured
+for
+[Puma](https://docs.gitlab.com/omnibus/settings/puma.html#puma-settings)
+or [Unicorn](https://docs.gitlab.com/omnibus/settings/unicorn.html).
+This makes sure that Gitaly calls made within a web request cannot
+exceed these the entire request timeout.
+
+The default for this timeout is 55 seconds.
+
+- Fast timeout
+
+This is the timeout for very short Gitaly calls.
+
+The default for this timeout is 10 seconds.
+
+- Medium timeout
+
+This timeout should be between the default and the fast timeout
+
+The default for this timeout is 30 seconds.

doc/user/admin_area/settings/index.md

@@ -24,6 +24,7 @@ include:
 - [Protected paths](protected_paths.md) **(CORE ONLY)**
 - [Help messages for the `/help` page and the login page](help_page.md)
 - [Push event activities limit and bulk push events](push_event_activities_limit.md)
+- [Gitaly timeouts](gitaly_timeouts.md)
 
 NOTE: **Note:**
 You can change the [first day of the week](../../profile/preferences.md) for the entire GitLab instance

spec/models/application_setting_spec.rb

@@ -380,6 +380,12 @@ describe ApplicationSetting do
 
         expect(subject).to be_invalid
       end
+
+      it 'does not prevent from saving when gitaly timeouts were previously invalid' do
+        subject.update_column(:gitaly_timeout_default, Settings.gitlab.max_request_duration_seconds + 1)
+
+        expect(subject.reload).to be_valid
+      end
     end
 
     describe 'enforcing terms' do