Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #15779 +/- ##
==========================================
- Coverage 66.04% 66.03% -0.01%
==========================================
Files 1067 1067
Lines 93546 93554 +8
Branches 2337 2337
==========================================
+ Hits 61781 61782 +1
- Misses 27141 27148 +7
Partials 4624 4624
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| expectErrMsg: fleet.RunScriptHostTimeoutErrMsg, | ||
| }, | ||
| // TODO: this would take 5 minutes to run, we don't want that kind of slowdown in our test suite | ||
| // but can be useful to have around for manual testing. |
There was a problem hiding this comment.
Ideally we'd have a simple way to reduce the timeout duration, but for now this is only possible within the server/service package and the response with shorter timeouts is well tested in this package (in the integration tests), so I just commented-it out.
| } | ||
| } | ||
| apiHandler.ServeHTTP(rw, req) | ||
| }) |
There was a problem hiding this comment.
This is ugly, but the issue is in the promhttp third-party package, they wrap the raw http.ResponseWriter but don't provide the Unwrap method required to unwrap it back to the original value, and this is required for ResponseController to work properly.
In the meantime and pressed by time I went with this ugly approach, but I'm sure the prometheus folks would be open to integrate the change as this is the right way to do it since Go 1.20, and then we could have a cleaner approach and do this timeout extension where the route is defined, e.g. with a ue.WithHTTPMiddleware(...).POST("/...") (which I attempted at first before noticing the prometheus thing).
gillespi314
left a comment
gillespi314
left a comment
There was a problem hiding this comment.
Looks good! Happy to review again if there are further changes.
roperzh
left a comment
roperzh
left a comment
There was a problem hiding this comment.
Pulled and tested and works perfectly
|
@georgekarrv @sabrinabuckets per @lukeheath's request I'm including this in the release. |
for #15196. The main problem was that we have two timeouts: 1. The timeout used by the host to kill the script execution 2. The timeout used by the server to wait for the script results Before the changes in #15779, the server timeout was longer than the host timeout, but we inadvertently set both values to 5 minutes, which breaks the logic we have to handle both kinds of timeouts.
4 participants
#15196 This is the work of @ghernandez345 except for adding the
ResponseControllerthing in Go to override the server timeout for that specific sync endpoint so that the calls don't timeout waiting for a script response (the default HTTP server timeout was 90s for our server).Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/ororbit/changes/.See Changes files for more information.
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements)