-
Notifications
You must be signed in to change notification settings - Fork 464
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 all 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 |
|---|---|---|
|
|
@@ -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, | ||
|
|
@@ -29,10 +29,40 @@ 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: Maximum time to establish connection to tracker backend. | ||
| # Uses readiness_timeout since connection establishment should be fast for | ||
| # healthy tracker instances. Slow connections indicate network or server issues. | ||
| 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 +71,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" | ||
|
|
@@ -169,7 +170,7 @@ func Run(config Config, params map[string]interface{}, opts ...Option) error { | |
| } | ||
|
|
||
| // Create root directory for generated files for nginx. | ||
| if err := os.MkdirAll(_genDir, 0775); err != nil { | ||
| if err := os.MkdirAll(_genDir, 0o775); err != nil { | ||
| return err | ||
| } | ||
|
|
||
|
|
@@ -197,7 +198,7 @@ func Run(config Config, params map[string]interface{}, opts ...Option) error { | |
| cabundle.Close() | ||
| } | ||
|
|
||
| if err := os.MkdirAll(config.CacheDir, 0775); err != nil { | ||
| if err := os.MkdirAll(config.CacheDir, 0o775); err != nil { | ||
| return err | ||
| } | ||
|
|
||
|
|
@@ -211,11 +212,11 @@ func Run(config Config, params map[string]interface{}, opts ...Option) error { | |
| } | ||
|
|
||
| conf := filepath.Join(_genDir, config.Name) | ||
| if err := ioutil.WriteFile(conf, src, 0755); err != nil { | ||
| if err := ioutil.WriteFile(conf, src, 0o755); err != nil { | ||
| return fmt.Errorf("write src: %s", err) | ||
| } | ||
|
|
||
| stdout, err := os.OpenFile(config.StdoutLogPath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644) | ||
| stdout, err := os.OpenFile(config.StdoutLogPath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644) | ||
| if err != nil { | ||
| return fmt.Errorf("open stdout log: %s", err) | ||
| } | ||
|
|
@@ -249,3 +250,33 @@ func GetServer(net, addr string) string { | |
| } | ||
| return addr | ||
| } | ||
|
|
||
| // FormatDurationForNginx converts a Go time.Duration to an nginx-compatible timeout string. | ||
| // | ||
| // This function adds a 30-second buffer to the input duration to ensure that the Go server | ||
| // times out before nginx does. This approach provides better observability and error handling | ||
| // because the Go application can return structured error responses with proper HTTP status codes, | ||
| // rather than nginx returning generic 504 Gateway Timeout errors. | ||
| // | ||
| // Timeout Strategy: | ||
| // - Go server timeout: d (original duration) | ||
| // - Nginx timeout: d + 30s (buffered duration) | ||
| // - This ensures Go responds with proper errors before nginx cuts the connection | ||
| // | ||
| // Format: Always returns seconds format (e.g., "60s", "150s", "3600s") for simplicity. | ||
| // Nginx accepts both seconds and minutes formats, so this approach works universally. | ||
| // | ||
| // Examples: | ||
| // | ||
| // FormatDurationForNginx(5 * time.Minute) // "330s" (5m + 30s = 330s) | ||
| // FormatDurationForNginx(2 * time.Minute) // "150s" (2m + 30s = 150s) | ||
| // FormatDurationForNginx(30 * time.Second) // "60s" (30s + 30s = 60s) | ||
| // FormatDurationForNginx(10 * time.Second) // "40s" (10s + 30s = 40s) | ||
| // FormatDurationForNginx(500 * time.Millisecond) // "30s" (500ms + 30s = 30.5s → 30s) | ||
| // | ||
| // Note: Nginx accepts both "60s" and "1m" formats. This function uses seconds for consistency. | ||
| 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 |
||
| bufferedDuration := d + (30 * time.Second) | ||
| seconds := int(bufferedDuration.Seconds()) | ||
| return fmt.Sprintf("%ds", seconds) | ||
| } | ||
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.
How will these addtional settings help ?
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.
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.