Add nginx timeouts #413
Add nginx timeouts #413
Conversation
hweawer
force-pushed
the
improve-kraken-origin-logging
branch
from
4c3f228 to
58bfdd6
Compare
hweawer
force-pushed
the
nginx-timeouts-large-blobs
branch
from
3d729cb to
100f8d0
Compare
hweawer
force-pushed
the
improve-kraken-origin-logging
branch
from
58bfdd6 to
28f4b6f
Compare
hweawer
force-pushed
the
nginx-timeouts-large-blobs
branch
from
dc6a30d to
0441c6d
Compare
gkeesh7
left a comment
gkeesh7
left a comment
There was a problem hiding this comment.
Please address the review comments and explain in detail what each config change does
config/agent/base.yaml
Outdated
|
|
||
| agentserver: | ||
| # Timeout configurations (also used by nginx) | ||
| download_timeout: 5m # nginx proxy_read_timeout for downloads |
There was a problem hiding this comment.
Let's keep it to 15 minutes
config/origin/base.yaml
Outdated
| addr: /tmp/kraken-origin.sock | ||
|
|
||
| # Timeout configurations (also used by nginx) | ||
| download_timeout: 5m # nginx proxy_read_timeout for downloads |
nginx/config/origin.go
Outdated
| proxy_send_timeout {{.upload_timeout}}; | ||
| proxy_read_timeout {{.download_timeout}}; | ||
| # Keepalive settings |
There was a problem hiding this comment.
Can you describe what is meant by these keep alive setting you are adding and why are they being added ?
| proxy_set_header X-Forwarded-Proto $scheme; | ||
| } | ||
| # Special handling for upload operations with longer timeout |
There was a problem hiding this comment.
How will these addtional settings help ?
There was a problem hiding this comment.
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.
nginx/nginx.go
Outdated
| return fmt.Sprintf("%ds", seconds) | ||
| } | ||
| // Fallback to milliseconds for very short durations | ||
| return fmt.Sprintf("%dms", bufferedDuration.Milliseconds()) |
There was a problem hiding this comment.
In line 256 we are already adding 30seconds Will we ever reach this codepath ?
There was a problem hiding this comment.
Made it convert to seconds only
| access_log {{.access_log_path}}; | ||
| error_log {{.error_log_path}}; | ||
There was a problem hiding this comment.
Same here as well please describe in more detail
| return addr | ||
| } | ||
|
|
||
| func FormatDurationForNginx(d time.Duration) string { |
There was a problem hiding this comment.
Please explain the purpose o fthis function
There was a problem hiding this comment.
Added documentation
|
@hweawer can you rebase the changes and run against the updated CI ? |
🛠 Technical Implementation
🎁 Benefits