Draft: Fix the gitaly_default_timeout validation
What does this MR do and why?
Before, the validation incorrectly used
Settings.gitlab.max_request_duration_seconds
, which gets it's value based on
the old worker_timeout
setting, which isn't actually used for timeouts
anymore (since the switch to Puma). It should instead get the worker timeout from
GITLAB_RAILS_RACK_TIMEOUT
.
Before:
- Set
GITLAB_RAILS_RACK_TIMEOUT
to 600 in gitlab.rb. gitlab-ctl reconfigure
- Try to raise the Gitaly timeout to 61.
- Get an error:
Gitaly timeout default must be less than or equal to 57
57 is the default for max_request_duration_seconds
, which is derived from worker_timeout
.
I think this would be my first non-docs, non-string change to this project. I could be missing important things.
Related ticket
I ran into this when helping a customer on a Support ticket (internal).
How to set up and validate locally
With this change, the above steps should change to:
- Set
GITLAB_RAILS_RACK_TIMEOUT
to 600 in gitlab.rb. gitlab-ctl reconfigure
- Try to raise the Gitaly timeout to 61.
- No error
🎉
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.
Todo
-
Change the spec -
Find if this is the correct approach to begin with. - I asked about the best way to get
GITLAB_RAILS_RACK_TIMEOUT
, and the answer was from the environment variable directly (as opposed to from Puma or something). - Should this be a deeper change?
- Right now, the
max_request_duration_seconds
defaults to 57, or 60 (theworker_timeout
default) * .95. Do we still need it to actually be a shorter duration than the actual worker timeout? If so, I'll change the docs to reflect that.
- I asked about the best way to get