WIP: Add option to buffer proxy requests
This change implements a simplified form of NGINX's proxy_request_buffering in gitlab-workhorse. It will allow us to turn off proxy_request_buffering in NGINX and use it selectively, buffering only requests that go to Unicorn.
Selective request buffering saves time and disk IO for requests that are handled by gitlab-workhorse. For example, a 1GB LFS upload will currently be buffered entirely by NGINX (on local disk) before being copied to LFS storage by gitlab-workhorse. With selective request buffering we would be able to write that file directly to LFS storage.
The implementation uses one fixed-size dynamically allocated memory buffer per request. If a request has an empty body we do not allocate a dynamic buffer. If the request buffer exceeds the dynamic buffer size we write it to a tempfile instead. The current implementation creates one tempfile per overflow request.
Merge request reports
Activity
@jacobvosmaer-gitlab related if this MR happens https://gitlab.com/gitlab-org/omnibus-gitlab/issues/945?
@marin yes this is the reincarnation of that idea now that all requests flow through gitlab-workhorse.
The code in this MR is off by default. If we merge this I would add an option in Omnibus to disable proxy_request_buffering so we would be able to turn this on on a couple of gitlab.com machines.
Tangentially related: it might be interesting to turn off proxy_request_buffering in the Omnibus template for registry. cc @ayufan
Edit: https://gitlab.com/gitlab-org/omnibus-gitlab/issues/1567
Edited by Jacob Vosmaer (GitLab)@ayufan are you talking about this MR or disabling proxy_request_buffering for registry?
mentioned in issue gitlab-com/infrastructure#399 (closed)
Added 47 commits:
-
656d551a...15dcdbd4 - 45 commits from branch
master
- 99f04acb - Merge branch 'master' into buffer-requests
- 24248b3e - Make more use of stdlib
-
656d551a...15dcdbd4 - 45 commits from branch
mentioned in merge request !94 (closed)
mentioned in merge request !102 (merged)
This is not moving along. It's an interesting idea but I'm not sure if we need it right now.
@nick.thomas shall we close this?
@nick.thomas @jacobvosmaer-gitlab I wonder if we do need it. We found large LFS files failing to upload unless buffering were turned off in nginx: https://gitlab.com/gitlab-org/gitlab-ce/issues/31871#note_29478568
Edited by Stan Hu@stanhu for routes that should never have request buffering enabled, we can already disable it in gitlab-rails or workhorse by setting the appropriate headers per-route.
I see a fix for that made it into omnibus: https://gitlab.com/gitlab-org/omnibus-gitlab/merge_requests/1569 but I think it would be more appropriate to do it in workhorse. WDYT @jacobvosmaer-gitlab ?
Edit: created https://gitlab.com/gitlab-org/gitlab-workhorse/issues/130 to discuss
further edit: I got confused between request and response buffering here. Workhorse can't control request buffering, but can control response buffering.
Edited by Nick Thomasmentioned in issue #130