-
Notifications
You must be signed in to change notification settings - Fork 472
Add nginx timeouts #413
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: improve-kraken-origin-logging
Are you sure you want to change the base?
Add nginx timeouts #413
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,3 +58,4 @@ env* | |
|
|
||
| # For integration tests. | ||
| .tmptest/ | ||
| .golangci.yml | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,6 +48,17 @@ blobserver: | |
| listener: | ||
| net: unix | ||
| addr: /tmp/kraken-origin.sock | ||
|
|
||
| # Timeout configurations (also used by nginx) | ||
| download_timeout: 5m # nginx proxy_read_timeout for downloads | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 15 minutes here as well
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| upload_timeout: 10m # nginx proxy_read_timeout/send_timeout for uploads | ||
| replication_timeout: 3m # nginx timeout for replication operations | ||
| backend_timeout: 2m # nginx proxy_connect_timeout | ||
| readiness_timeout: 30s # internal readiness check timeout | ||
|
|
||
| # Concurrency limits | ||
| max_concurrent_downloads: 10 | ||
| max_concurrent_uploads: 5 | ||
|
|
||
| nginx: | ||
| name: kraken-origin | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,8 +28,53 @@ server { | |
| gzip on; | ||
| gzip_types text/plain test/csv application/json; | ||
|
|
||
| # Timeout configurations from origin server config | ||
| proxy_connect_timeout {{.backend_timeout}}; | ||
| proxy_send_timeout {{.upload_timeout}}; | ||
| proxy_read_timeout {{.download_timeout}}; | ||
|
|
||
| # Keepalive settings | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you describe what is meant by these keep alive setting you are adding and why are they being added ?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added comments |
||
| proxy_buffering off; | ||
| proxy_request_buffering off; | ||
|
|
||
| location / { | ||
| proxy_pass http://{{.server}}; | ||
|
|
||
| # Pass original client info | ||
| proxy_set_header Host $host; | ||
| proxy_set_header X-Real-IP $remote_addr; | ||
| proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; | ||
| proxy_set_header X-Forwarded-Proto $scheme; | ||
| } | ||
|
|
||
| # Special handling for upload operations with longer timeout | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How will these addtional settings help ?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I understand the upload will happen ubuild -> kraken -> GCS, whereas replication will happen in between origin instances in out network, so I thought that it might have sense for splitting it, but if you think that there is not much sense in this I can put a single timeout. |
||
| location ~ ^/namespace/.*/blobs/.*/uploads { | ||
| proxy_pass http://{{.server}}; | ||
|
|
||
| # Use upload timeout for these operations | ||
| proxy_read_timeout {{.upload_timeout}}; | ||
| proxy_send_timeout {{.upload_timeout}}; | ||
|
|
||
| # Pass original client info | ||
| proxy_set_header Host $host; | ||
| proxy_set_header X-Real-IP $remote_addr; | ||
| proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; | ||
| proxy_set_header X-Forwarded-Proto $scheme; | ||
| } | ||
|
|
||
| # Replication operations with their own timeout | ||
| location ~ ^/namespace/.*/blobs/.*/remote { | ||
| proxy_pass http://{{.server}}; | ||
|
|
||
| # Use replication timeout for these operations | ||
| proxy_read_timeout {{.replication_timeout}}; | ||
| proxy_send_timeout {{.replication_timeout}}; | ||
|
|
||
| # Pass original client info | ||
| proxy_set_header Host $host; | ||
| proxy_set_header X-Real-IP $remote_addr; | ||
| proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; | ||
| proxy_set_header X-Forwarded-Proto $scheme; | ||
| } | ||
| } | ||
| ` | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,10 +29,36 @@ server { | |
| access_log {{.access_log_path}}; | ||
| error_log {{.error_log_path}}; | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here as well please describe in more detail
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added comments |
||
| # Timeout configurations from tracker server config | ||
| proxy_connect_timeout {{.readiness_timeout}}; | ||
| proxy_send_timeout {{.announce_timeout}}; | ||
| proxy_read_timeout {{.announce_timeout}}; | ||
|
|
||
| location / { | ||
| proxy_pass http://tracker; | ||
|
|
||
| # Pass original client info | ||
| proxy_set_header Host $host; | ||
| proxy_set_header X-Real-IP $remote_addr; | ||
| proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; | ||
| proxy_set_header X-Forwarded-Proto $scheme; | ||
| } | ||
|
|
||
| # Health and readiness checks with shorter timeout | ||
| location ~ ^/(health|readiness)$ { | ||
| proxy_pass http://tracker; | ||
|
|
||
| proxy_read_timeout {{.readiness_timeout}}; | ||
| proxy_send_timeout {{.readiness_timeout}}; | ||
|
|
||
| # Pass original client info | ||
| proxy_set_header Host $host; | ||
| proxy_set_header X-Real-IP $remote_addr; | ||
| proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; | ||
| proxy_set_header X-Forwarded-Proto $scheme; | ||
| } | ||
|
|
||
| # Metainfo requests need longer timeout (cached) | ||
| location ~* ^/namespace/.*/blobs/.*/metainfo$ { | ||
| proxy_pass http://tracker; | ||
|
|
||
|
|
@@ -41,6 +67,31 @@ server { | |
| proxy_cache_valid 200 5m; | ||
| proxy_cache_valid any 1s; | ||
| proxy_cache_lock on; | ||
|
|
||
| # Use metainfo timeout for these operations | ||
| proxy_read_timeout {{.metainfo_timeout}}; | ||
| proxy_send_timeout {{.metainfo_timeout}}; | ||
|
|
||
| # Pass original client info | ||
| proxy_set_header Host $host; | ||
| proxy_set_header X-Real-IP $remote_addr; | ||
| proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; | ||
| proxy_set_header X-Forwarded-Proto $scheme; | ||
| } | ||
|
|
||
| # Announce operations | ||
| location ~ ^/announce { | ||
| proxy_pass http://tracker; | ||
|
|
||
| # Use announce timeout for these operations | ||
| proxy_read_timeout {{.announce_timeout}}; | ||
| proxy_send_timeout {{.announce_timeout}}; | ||
|
|
||
| # Pass original client info | ||
| proxy_set_header Host $host; | ||
| proxy_set_header X-Real-IP $remote_addr; | ||
| proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; | ||
| proxy_set_header X-Forwarded-Proto $scheme; | ||
| } | ||
| } | ||
| ` | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,7 @@ | |
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
|
|
@@ -23,6 +23,7 @@ import ( | |
| "path" | ||
| "path/filepath" | ||
| "text/template" | ||
| "time" | ||
|
|
||
| "github.com/uber/kraken/nginx/config" | ||
| "github.com/uber/kraken/utils/httputil" | ||
|
|
@@ -249,3 +250,21 @@ func GetServer(net, addr string) string { | |
| } | ||
| return addr | ||
| } | ||
|
|
||
| func FormatDurationForNginx(d time.Duration) string { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please explain the purpose o fthis function
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added documentation |
||
| // Add 30s buffer to ensure Go server times out first for observability | ||
| bufferedDuration := d + (30 * time.Second) | ||
|
|
||
| if bufferedDuration >= time.Minute { | ||
| minutes := int(bufferedDuration.Minutes()) | ||
| if bufferedDuration == time.Duration(minutes)*time.Minute { | ||
| return fmt.Sprintf("%dm", minutes) | ||
| } | ||
| } | ||
| if bufferedDuration >= time.Second { | ||
| seconds := int(bufferedDuration.Seconds()) | ||
| return fmt.Sprintf("%ds", seconds) | ||
| } | ||
| // Fallback to milliseconds for very short durations | ||
| return fmt.Sprintf("%dms", bufferedDuration.Milliseconds()) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In line 256 we are already adding 30seconds Will we ever reach this codepath ?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Made it convert to seconds only |
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep it to 15 minutes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done