-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
roachtest: clearrange/checks=false failed #34897
Comments
SHA: https://github.com/cockroachdb/cockroach/commits/d7c56dcb87c8f187e50303c8e32a64836c42515c Parameters: To repro, try:
Failed test: https://teamcity.cockroachdb.com/viewLog.html?buildId=1139797&tab=buildLog
|
SHA: https://github.com/cockroachdb/cockroach/commits/4fc4d63ddddeb507564bfe53c6cd79549ff2fd27 Parameters: To repro, try:
Failed test: https://teamcity.cockroachdb.com/viewLog.html?buildId=1141650&tab=buildLog
|
SHA: https://github.com/cockroachdb/cockroach/commits/8e9a1e310e3e8e37f091b7ca8bd204084ad9e2e5 Parameters: To repro, try:
Failed test: https://teamcity.cockroachdb.com/viewLog.html?buildId=1142461&tab=buildLog
|
SHA: https://github.com/cockroachdb/cockroach/commits/bd80a74f882a583d6bb2a04dfdb57b49254bc7ba Parameters: To repro, try:
Failed test: https://teamcity.cockroachdb.com/viewLog.html?buildId=1143393&tab=buildLog
|
This second to last one looks like it's really a test timeout and that the test, ironically, got stuck importing a tiny table:
14:13:59 is very close to the test timeout of 6:30h (the test run started at 7:42:56). |
The last one is probably similar, except in this case the (Note that this is all before even dropping a table) |
In the last one, n10 took ~15s to start. It's unclear where all that time went, for example there's a 5s gap very early ( Starting the Store also seems to eat 5s. Anyway, it takes a while to start is the bottom line here. I wonder why cockroach/pkg/server/server.go Lines 1375 to 1380 in 475958d
but that's probably for a good reason. Intuitively I would've thought readiness would be signaled with this message
Anyway, the more interesting failure is that in which the RESTORE just gets stuck basically forever.
|
See cockroachdb#34897 (comment). Release note: None
Yeah, this should probably be later. It should at least be moved below the |
Filed #35070 for the
readiness signaling.
|
See cockroachdb#34897 (comment). Release note: None
35050: roachtest: misc clearrange improvements r=petermattis a=tbg Touches #34897. Release note: None Co-authored-by: Tobias Schottdorf <[email protected]>
SHA: https://github.com/cockroachdb/cockroach/commits/46d1ab4c06edfd37d875114972c55b44105acd83 Parameters: To repro, try:
Failed test: https://teamcity.cockroachdb.com/viewLog.html?buildId=1147903&tab=buildLog
|
SHA: https://github.com/cockroachdb/cockroach/commits/d0a93d286ee42ceb94f986b6a06a1afd52cf914e Parameters: To repro, try:
Failed test: https://teamcity.cockroachdb.com/viewLog.html?buildId=1149020&tab=buildLog
|
SHA: https://github.com/cockroachdb/cockroach/commits/eaad50f0ea64985a0d7da05abb00cc9f321c5fa9 Parameters: To repro, try:
Failed test: https://teamcity.cockroachdb.com/viewLog.html?buildId=1149743&tab=buildLog
|
SHA: https://github.com/cockroachdb/cockroach/commits/032c4980720abc1bdd71e4428e4111e6e6383297 Parameters: To repro, try:
Failed test: https://teamcity.cockroachdb.com/viewLog.html?buildId=1158877&tab=buildLog
|
SHA: https://github.com/cockroachdb/cockroach/commits/d888b76df319571afe4d5816f1a1f0f53905653f Parameters: To repro, try:
Failed test: https://teamcity.cockroachdb.com/viewLog.html?buildId=1159641&tab=buildLog
|
SHA: https://github.com/cockroachdb/cockroach/commits/de1793532332fb64fca27cafe92d2481d900a5a0 Parameters: To repro, try:
Failed test: https://teamcity.cockroachdb.com/viewLog.html?buildId=1160394&tab=buildLog
|
SHA: https://github.com/cockroachdb/cockroach/commits/959dcf7de0f94cfcfa0062387b109adebd1f11da Parameters: To repro, try:
Failed test: https://teamcity.cockroachdb.com/viewLog.html?buildId=1163702&tab=buildLog
|
SHA: https://github.com/cockroachdb/cockroach/commits/38bb1e7905b89f911bd74be4f5830217ffb7b343 Parameters: To repro, try:
Failed test: https://teamcity.cockroachdb.com/viewLog.html?buildId=1168752&tab=buildLog
|
SHA: https://github.com/cockroachdb/cockroach/commits/5f7de3c35348e847dc0d4ab1ba8d76574e1706c2 Parameters: To repro, try:
Failed test: https://teamcity.cockroachdb.com/viewLog.html?buildId=1169980&tab=buildLog
|
SHA: https://github.com/cockroachdb/cockroach/commits/9d058d53c8a82fceb2205f1827c26f1bf36c32ba Parameters: To repro, try:
Failed test: https://teamcity.cockroachdb.com/viewLog.html?buildId=1172386&tab=buildLog
|
No, it does not. This only applies to the engines that are used as stores. This means that if you're seeing the disk checker crash because of activity on the temp engines, then it's because the temp engines starve IO for the main engine.
I think @petermattis had a past investigation in which CRDB performed poorly because some other process was filling up the OS' write buffer and so even small syncs done by RocksDB could take a long time. Perhaps something similar is going on here.
Interesting, how can that be? They're entirely different RocksDB instances. How does that work?
Just curious, what was the transient issue? We're really trying to get rid of transient issues. |
@tbg's memory is good. Linux's default behavior is to allow too large an amount of dirty file data to accumulate before flushing. When the flushing does occur it can cause significant disk IO starvation for everything on the system. I can't recall off hand what to look for, but
The thread pool could be shared between RocksDB instances, but I'm pretty sure we don't share it. We do share the block cache. |
Where? Am I looking at the wrong place? cockroach/pkg/storage/engine/temp_engine.go Lines 22 to 50 in adf23fa
|
I'm a lying liar and you caught me! More accurately, I misremembered. We share the block cache between all of the stores on a node. I wonder if we should use that same shared block cache for the temp store. The current 0-size block cache might make random access |
I have some low level strace data showing the tiny WAL write syncs getting stuck behind huge temp store sstable syncs. For example:
That option might make more sense together with a rate limiter on background writes. That'd cause us to pause occasionally when generating large files allowing some of the async writebacks to finish. I don't know what rate limit to pick though. If we had some estimate of the device's max throughput that'd be enough info for us to use auto-tuned rate limiter. |
In RocksDB the
I think this one was caused by my home Internet connection. |
Oh, interesting. I had assumed the file syncing was synchronous. I wonder if we should be passing |
Globals!!! I'm pretty sure that sharing is unintentional. I think the temp-store should have a separate thread pool with 1 high priority and 1 low priority thread. And sharing the thread pool across all stores on a node is definitely not what was intended. Yikes! |
Yikes indeed. I'm excited by the better place we'll be in once this investigation is resolved, though. |
SHA: https://github.com/cockroachdb/cockroach/commits/134478e4dde16919eb86efb81fb22d8cce8a9701 Parameters: To repro, try:
Failed test: https://teamcity.cockroachdb.com/viewLog.html?buildId=1236263&tab=buildLog
|
Following up on this series, we tried a fourth thing today. I only ran one experiment which contained changes (1), (2), and (4). Like usual it was run with |
Was the (1), (2), and (4) experiment run on ZFS? That's an excellent improvement. I think it would be worthwhile to make sure the result is stable and run the test multiple times (you can do this in parallel using I believe you mentioned elsewhere that ext4 is able to complete the import portion of this test significantly faster. Is that result reproducible? |
SHA: https://github.com/cockroachdb/cockroach/commits/509c5b130fb1ad0042beb74e083817aa68e4fc92 Parameters: To repro, try:
Failed test: https://teamcity.cockroachdb.com/viewLog.html?buildId=1237002&tab=buildLog
|
That one was on ZFS. OK, I ran a few more experiments to answer your questions. Summary of all our results from yesterday and today:
So the results are pretty stable, and looks like (1) and (2) don't make a big difference, but also don't hurt. I realized also we don't really have a baseline since these are all run with |
Glad to see that the import times are stable. This is great progress and I appreciate your diligence in testing here. I'm surprised at how large the difference is between ZFS and ext4. Looks like (4) is the important change, though I'm totally fine with (1) and (2) as well. |
Thanks, hope I didn't run up our GCE bill too much :).
Got a baseline using |
This comment has been minimized.
This comment has been minimized.
Includes the following changes, all of which have landed upstream. - cockroachdb/rocksdb#27: "ldb: set `total_order_seek` for scans" - cockroachdb/rocksdb#28: "Fix cockroachdb#3840: only `SyncClosedLogs` for multiple CFs" - cockroachdb/rocksdb#29: "Optionally wait on bytes_per_sync to smooth I/O" - cockroachdb/rocksdb#30: "Option string/map/file can set env from object registry" Also made the RocksDB changes that we decided in cockroachdb#34897: - Do not sync WAL before installing flush result. This is achieved by backporting cockroachdb/rocksdb#28; no configuration change is necessary. - Do not sync WAL ever for temp stores. This is achieved by setting `wal_bytes_per_sync = 0`. - Limit size of final syncs when generating SSTs. This is achieved by backporting cockroachdb/rocksdb#29 and turning it on with `strict_bytes_per_sync = true`. Release note: None
37172: c-deps: bump rocksdb for multiple backported PRs r=ajkr a=ajkr Includes the following changes, all of which have landed upstream. - cockroachdb/rocksdb#27: "ldb: set `total_order_seek` for scans" - cockroachdb/rocksdb#28: "Fix #3840: only `SyncClosedLogs` for multiple CFs" - cockroachdb/rocksdb#29: "Optionally wait on bytes_per_sync to smooth I/O" - cockroachdb/rocksdb#30: "Option string/map/file can set env from object registry" Also made the RocksDB changes that we decided in #34897: - Do not sync WAL before installing flush result. This is achieved by backporting cockroachdb/rocksdb#28; no configuration change is necessary. - Do not sync WAL ever for temp stores. This is achieved by setting `wal_bytes_per_sync = 0`. - Limit size of final syncs when generating SSTs. This is achieved by backporting cockroachdb/rocksdb#29 and turning it on with `strict_bytes_per_sync = true`. Release note: None Co-authored-by: Andrew Kryczka <[email protected]>
Failing on zfs snapshot, that's strange. Well, I plan to play with these tests over the next week or so, and will investigate it then if I happen to see it. |
Includes the following changes, all of which have landed upstream. - cockroachdb/rocksdb#27: "ldb: set `total_order_seek` for scans" - cockroachdb/rocksdb#28: "Fix cockroachdb#3840: only `SyncClosedLogs` for multiple CFs" - cockroachdb/rocksdb#29: "Optionally wait on bytes_per_sync to smooth I/O" - cockroachdb/rocksdb#30: "Option string/map/file can set env from object registry" Also made the RocksDB changes that we decided in cockroachdb#34897: - Do not sync WAL before installing flush result. This is achieved by backporting cockroachdb/rocksdb#28; no configuration change is necessary. - Do not sync WAL ever for temp stores. This is achieved by setting `wal_bytes_per_sync = 0`. - Limit size of final syncs when generating SSTs. This is achieved by backporting cockroachdb/rocksdb#29 and turning it on with `strict_bytes_per_sync = true`. Release note: None
SHA: https://github.com/cockroachdb/cockroach/commits/acba091f04f3d8ecabf51009bf394951fbd3643c
Parameters:
To repro, try:
Failed test: https://teamcity.cockroachdb.com/viewLog.html?buildId=1137872&tab=buildLog
The text was updated successfully, but these errors were encountered: