[Filestore] emulate page faults in tests#6423
Conversation
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-relwithdebinfo have completed. Tip Planned checks for linux-x86_64-relwithdebinfo. 🟢 linux-x86_64-relwithdebinfo target: cloud/tasks/,cloud/storage/ (test time: 261s): all tests PASSED for commit 8ee844e.
🔴 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 4521s): some tests FAILED for commit 8ee844e.
🔴 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 59s): some tests FAILED for commit 8ee844e.
🔴 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 46s): some tests FAILED for commit 8ee844e.
|
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-relwithdebinfo have completed. Tip Planned checks for linux-x86_64-relwithdebinfo. 🟢 linux-x86_64-relwithdebinfo target: cloud/tasks/,cloud/storage/ (test time: 386s): all tests PASSED for commit 8ee844e.
🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 1996s): all tests PASSED for commit 8ee844e.
🔴 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 6644s): some tests FAILED for commit 8ee844e.
🔴 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 34s): some tests FAILED for commit 8ee844e.
🔴 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 30s): some tests FAILED for commit 8ee844e.
🟢 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 11718s): all tests PASSED for commit 8ee844e.
|
|
sharding tests are failing, looks like after TX restart we just choose different shard. Doesn't look important, we can use nullptr rescheduler for sharding tests |
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-relwithdebinfo have completed. Tip Planned checks for linux-x86_64-relwithdebinfo. 🟢 linux-x86_64-relwithdebinfo target: cloud/tasks/,cloud/storage/ (test time: 449s): all tests PASSED for commit 6ed4c36.
|
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-relwithdebinfo have completed. Tip Planned checks for linux-x86_64-relwithdebinfo. |
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-relwithdebinfo have completed. Tip Planned checks for linux-x86_64-relwithdebinfo. 🟢 linux-x86_64-relwithdebinfo target: cloud/tasks/,cloud/storage/ (test time: 332s): all tests PASSED for commit 73c5d86.
🔴 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 2037s): some tests FAILED for commit 73c5d86.
🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 211s): all tests PASSED for commit 73c5d86.
|
64941ab to
b4b3e2a
Compare
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-relwithdebinfo have completed. Tip Planned checks for linux-x86_64-relwithdebinfo. |
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-relwithdebinfo have completed. Tip Planned checks for linux-x86_64-relwithdebinfo. |
1b7eb43 to
61aeaa4
Compare
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-relwithdebinfo have completed. Tip Planned checks for linux-x86_64-relwithdebinfo. 🟢 linux-x86_64-relwithdebinfo target: cloud/tasks/,cloud/storage/ (test time: 327s): all tests PASSED for commit 61aeaa4.
🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 2257s): all tests PASSED for commit 61aeaa4.
|
b581d8f to
188a204
Compare
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-relwithdebinfo have completed. Tip Planned checks for linux-x86_64-relwithdebinfo. 🟢 linux-x86_64-relwithdebinfo target: cloud/tasks/,cloud/storage/ (test time: 403s): all tests PASSED for commit 188a204.
🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 2091s): all tests PASSED for commit 188a204.
|
188a204 to
fd7f6e1
Compare
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-relwithdebinfo have completed. Tip Planned checks for linux-x86_64-relwithdebinfo. |
20f90c7 to
1946380
Compare
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-relwithdebinfo have completed. Tip Planned checks for linux-x86_64-relwithdebinfo. |
5fa6e7f to
f036d34
Compare
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-relwithdebinfo have completed. Tip Planned checks for linux-x86_64-relwithdebinfo. 🟢 linux-x86_64-relwithdebinfo target: cloud/tasks/,cloud/storage/ (test time: 370s): all tests PASSED for commit f036d34.
|
f036d34 to
56603e3
Compare
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-relwithdebinfo have completed. Tip Planned checks for linux-x86_64-relwithdebinfo. 🟢 linux-x86_64-relwithdebinfo target: cloud/tasks/,cloud/storage/ (test time: 369s): all tests PASSED for commit f8e5f18.
🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 1968s): all tests PASSED for commit f8e5f18.
🟢 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 7188s): all tests PASSED for commit f8e5f18.
🟢 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 11894s): all tests PASSED for commit f8e5f18.
|
f8e5f18 to
e3ba974
Compare
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-relwithdebinfo have completed. Tip Planned checks for linux-x86_64-relwithdebinfo. 🟢 linux-x86_64-relwithdebinfo target: cloud/tasks/,cloud/storage/ (test time: 378s): all tests PASSED for commit e3ba974.
🔴 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 2028s): some tests FAILED for commit e3ba974.
🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 216s): all tests PASSED for commit e3ba974.
🟢 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 7164s): all tests PASSED for commit e3ba974.
🟢 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 11939s): all tests PASSED for commit e3ba974.
|
| { | ||
| return false; | ||
| const bool ret = RandomGen.GenRandReal4() < Probability; | ||
| Triggered |= ret; |
There was a problem hiding this comment.
It’s strange that the value of Triggered can change from false to true, but not vice versa.
There was a problem hiding this comment.
It is for handling transactions doing multiple reads, where not-the-last-one read is failed. Assumption is made here that if any read() in transaction fails, the whole transaction should be rescheduled.
Transition true->false happens in Reset().
e3ba974 to
c272d72
Compare
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-relwithdebinfo have completed. Tip Planned checks for linux-x86_64-relwithdebinfo.
🟢 linux-x86_64-relwithdebinfo target: cloud/tasks/,cloud/storage/ (test time: 360s): all tests PASSED for commit c272d72.
|
c272d72 to
7c0fbe8
Compare
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-relwithdebinfo have completed. Tip Planned checks for linux-x86_64-relwithdebinfo. 🟢 linux-x86_64-relwithdebinfo target: cloud/tasks/,cloud/storage/ (test time: 382s): all tests PASSED for commit 7c0fbe8.
🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 2224s): all tests PASSED for commit 7c0fbe8.
🟢 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 7281s): all tests PASSED for commit 7c0fbe8.
🟢 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 11921s): all tests PASSED for commit 7c0fbe8.
|
| { | ||
| using TTable = TIndexTabletSchema::FileSystem; | ||
|
|
||
| if (Y_UNLIKELY(ShouldFailReadInTest())) |
There was a problem hiding this comment.
looks very ugly
let's just make another implementation of IIndexTabletDatabase that would wrap another IIndexTabletDatabase with these if xxx -> return false things
There was a problem hiding this comment.
I was thinking about that, but there is also TIndexTabletDatabaseProxy inherited from TIndexTabletDatabase used in 50% of all places.
Also, TIndexTabletDatabaseProxy is used in some macros magic for method gen in PP that would be harder to customize:
FILESTORE_TABLET_INDEX_RO_TRANSACTIONS(
FILESTORE_IMPLEMENT_RO_TRANSACTION,
TTxIndexTablet,
TIndexTabletDatabaseProxy, <<<<
IIndexTabletDatabase);
So alternatives were:
- add new "FailingTabledDatabase" and parametrize TIndexTabletDatabaseProxy with base class
- re-implement TIndexTabletDatabaseProxy in a new class inherited from FailingTabledDatabase (copying its current logic).
- just making the base class uglier.
The last option seemed to be less intrusive in the end, but we can reconsider this, wyt?
There was a problem hiding this comment.
then in every place where it is created we will need ugly IF, so it will just change the place. Or do I miss something?
There was a problem hiding this comment.
we can do something like CreateIndexTabletDatabase and hide it in creation function. that way it will be nice in all sides. Will look definitely better. So, I am not sure that it worth it. But I would say, both options work for me, existing and proposed
There was a problem hiding this comment.
I don't like these if statements either.
Let's do it the other way around.
7c0fbe8 to
5707bad
Compare
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-relwithdebinfo have completed. Tip Planned checks for linux-x86_64-relwithdebinfo. |
5707bad to
21111c2
Compare
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-relwithdebinfo have completed. Tip Planned checks for linux-x86_64-relwithdebinfo. 🟢 linux-x86_64-relwithdebinfo target: cloud/tasks/,cloud/storage/ (test time: 483s): all tests PASSED for commit 21111c2.
🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 2183s): all tests PASSED for commit 21111c2.
🟢 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 6883s): all tests PASSED for commit 21111c2.
🟢 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 11691s): all tests PASSED for commit 21111c2.
|
21111c2 to
6e7afae
Compare
…6521) ### Notes This is a preliminary step required for fixes requested on review of the #6423. Idea is to later provide another implementation of that interface that would randomly fail read operations. This PR is part **1** of the following chain: - Rename IIndexTabletDatabase to INodeIndexTabletDatabase <<< **We are here** Idea is that this name better reflects limited amount of methods exposed by the inteface. - Introduce a new IIndexTabletDatabase containing every public method from TIndexTabletDatabase and makes TIndexTabletDatabase implement that interface. - Create TIndexTabletDatabase in tablet_actor_* via factory functions instead of creating the objects on stack. ### Issue #6467
### Notes This is a preliminary step required for fixes requested on review of the #6423. Idea is to later provide another implementation of that interface that would randomly fail read operations. This PR is part **2** of the following chain: - Rename IIndexTabletDatabase to INodeIndexTabletDatabase - Introduce a new IIndexTabletDatabase containing every public method from TIndexTabletDatabase and makes TIndexTabletDatabase implement that interface. <<< **We are here** - Create TIndexTabletDatabase in tablet_actor_* via factory functions instead of creating the objects on stack. ### Issue #6467
… functions (#6530) ### Notes This is a preliminary step required for fixes requested on review of the #6423. Idea is to later provide another implementation of that interface that would randomly fail read operations. This PR is part **3** of the following chain: - Rename IIndexTabletDatabase to INodeIndexTabletDatabase - Introduce a new IIndexTabletDatabase containing every public method from TIndexTabletDatabase and makes TIndexTabletDatabase implement that interface. - Create TIndexTabletDatabase in tablet_actor_* via factory functions instead of creating the objects on stack. **<<< **We are here**** ### Issue #6467
### Notes This change allows randomly to restart transactions in tests using TTestEnv. Random page faults are enabled for a subset of unit and integration tests. Currently there is some gap in testing transactions restarted due to "page faults" in YDB, which reschedule and restart read transactions after some delay. As we did not find a way to trigger page faults in YDB, I extended previously existing failure injection to all Read* methods of TInMemoryIndexState. This PR is a rework of #6423. ### Issue #6467
Notes
This change enables randomly restarting transactions in all tests using TTestEnv. Additionally, it allows to do the same in larger/integration tests and exposes new configuration in storage config.
Currently there is some gap in testing transactions restarted due to "page faults" in YDB, which reschedule and restart read transactions after some delay. As we did not find a way to trigger page faults in YDB, I extended previously existing failure injection to all Read* methods of TInMemoryIndexState.
Issue
#6467