release-19.2: fix server hangs on HTTP timeouts #42539
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
knz
changed the title
|
LGTM but I'll defer to @bdarnell about postponing this for a few days until after 19.2.1. |
bdarnell
approved these changes
Nov 18, 2019
knz
force-pushed
the
backport19.2-42536-42537
branch
from
78513bc to
3cb4d5c
Compare
TestRoachLint has problematic memory usage which makes it inadequate to run on every CI, see issue cockroachdb#42594. This patch moves it to nightly. Release note: None
The `shadow` and `printfuncs` testvet invocations are not amenable to parallelization well. This patch fixes it. Release note: None
Previously, various parts of CockroachDB were using the `http` package's Get/Post/Head methods/functions directly. This was defective for two reasons: - it did not implement a request timeout, which could cause requests to hang forever if the server was inoperative. - it did not obey context cancellation, which would force the CockroachDB server to hang forever while trying to quit if some HTTP server was blocking. The last point in particular may cause excessive operator friction when restarting a server, by forcing a node to be killed and potentially disrupting operation. This patch fixes the issue by introducing a new `httputil.Client` class which implements a customizable timeout, see its constructor `NewClientWithTimeout`. The default instance in `httputil.DefaultClient` uses a 3 second timeout. It also introduces context-aware variants of the `http` methods/functions `Get`, `Post` and `Head`. Note that these variants do not _actually_ obey context cancellation when the code is compiled with Go 1.12 or prior. The new behavior only kicks in with Go 1.13 or later (it requires the new feature `NewRequestWithContext`). However, the usability problem should still be somewhat alleviated in Go 1.12 builds thanks to the request timeout implemented by the new `httputil.Client`. The following components in CockroachDB are then adapted to use the new interface: - avro registration for changefeed encoders: timeout + cancellation - telemetry updates: timeout + cancellation - binfetcher: timeout + cancellation - cloudinfo: timeout + cancellation - docgen: timeout, no cancellation (interactive tool) - roachprod install: timeout, no cancellation (interactive tool) - test server: timeout, no cancellation (tests have timeouts already) Release note (bug fix): CockroachDB will now less likely hang in an inconvenient/inoperative state if it attempts to access an external HTTP server that blocks or is overloaded. A possible symptom of the bug is a node failing to shut down upon `cockroach quit`. This bug is present since at least version 2.0.
Prior to this patch, the CLI interactive tests were running with diagnostic reporting (including update server checks, telemetry etc) enabled. This was unintentional—having implemented the tests initially, I always worked under the assumption that the acceptance test runner was disabling telemetry upfront. I only discovered the opposite was true recently, seeing tests fail with mysterious errors when the telemetry server happens to be mis-behaving. This patch fixes the situation by opting the interactive tests out of telemetry by default. The two tests that check it (with a sensible fallback) now opt back in explicitly. Release note: None
knz
force-pushed
the
backport19.2-42536-42537
branch
from
3cb4d5c to
24d3fa9
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport:
httputil.Clientand methods, & use them" (*: newhttputil.Clientand methods, & use them #42536)Please see individual PRs for details.
/cc @cockroachdb/release