[qa] Handle potential cookie race when starting node #12902
Conversation
|
I triggered this issue while running the tests on the 0.16 branch: But this is also straightforward to reproduce on master; I was able to do so by adding a small sleep in |
|
Considering that we usually clean up the datadir when the test was successful, wouldn't it make more sense to delete the cookie when it was left behind due to a crash? That would also limit the changes to Concept ACK |
|
@MarcoFalke I did consider that as well, also I considered changing the way we use the test framework so that if a cookie file is already present we instruct bitcoind to use it... None of these options seems all that great to me as this is pretty messy, but I figure that changing Edit: I should add -- normally the cookie file is deleted by bitcoind when it shuts down cleanly, not the test framework (I think). |
jnewbery
left a comment
jnewbery
left a comment
There was a problem hiding this comment.
Seems reasonable. If there really is an authorization failure, then wait_for_rpc_connection() will eventually timeout anyway.
| # using an old cookie file which is overwritten when the | ||
| # node starts up, so treat this the same as "No RPC | ||
| # credentials" and do nothing. | ||
| pass |
There was a problem hiding this comment.
Perhaps log, since this is an unexpected condition?
That was my initial response to this as well.
Correct. |
If a cookie file exists in a datadir prior to node startup, it must have been leftover from a prior unclean shutdown. As bitcoind will overwrite it anyway, delete it before starting up to prevent the test framework from inadvertently trying to connect using stale credentials.
sdaftuar
force-pushed
the
2018-04-improve-dbcrash-restarts
branch
from
02b3c4a to
75d0e4c
Compare
|
@laanwj @MarcoFalke I reworked this PR to delete cookie files, if present, as suggested. I avoided putting this logic directly in Original version of this PR is here for comparison: sdaftuar@02b3c4a Also updated OP to account for new behavior. |
|
utACK 75d0e4c |
|
Tested ACK 75d0e4c. I'd be tempted to put the new functionality directly into |
No strong opinion, but it looked like |
75d0e4c [qa] Delete cookie file before starting node (Suhas Daftuar) Pull request description: When a node is restarted during a test after an unclean shutdown (such as with -dbcrashratio), it's possible an old cookie file was left behind. This can cause a race condition when restarting the node, where the test framework might try to connect using credentials from the old cookie file, just as the node will generate new credentials and overwrite the old file. Delete any such cookie file if present prior to startup. Tree-SHA512: ae1e8bf8fd20e07c32b0715025693bb28b0e3dd34f328cae4346abf579b0c97b5db1c02782e1c46b7a3b6058d268b6d46b668e847658a6eed0be857ffb0d65dc
|
post-utACK 75d0e4c |
If a cookie file exists in a datadir prior to node startup, it must have been leftover from a prior unclean shutdown. As bitcoind will overwrite it anyway, delete it before starting up to prevent the test framework from inadvertently trying to connect using stale credentials. Github-Pull: bitcoin#12902 Rebased-From: 75d0e4c
If a cookie file exists in a datadir prior to node startup, it must have been leftover from a prior unclean shutdown. As bitcoind will overwrite it anyway, delete it before starting up to prevent the test framework from inadvertently trying to connect using stale credentials. Github-Pull: bitcoin#12902 Rebased-From: 75d0e4c
…node 75d0e4c [qa] Delete cookie file before starting node (Suhas Daftuar) Pull request description: When a node is restarted during a test after an unclean shutdown (such as with -dbcrashratio), it's possible an old cookie file was left behind. This can cause a race condition when restarting the node, where the test framework might try to connect using credentials from the old cookie file, just as the node will generate new credentials and overwrite the old file. Delete any such cookie file if present prior to startup. Tree-SHA512: ae1e8bf8fd20e07c32b0715025693bb28b0e3dd34f328cae4346abf579b0c97b5db1c02782e1c46b7a3b6058d268b6d46b668e847658a6eed0be857ffb0d65dc
…node 75d0e4c [qa] Delete cookie file before starting node (Suhas Daftuar) Pull request description: When a node is restarted during a test after an unclean shutdown (such as with -dbcrashratio), it's possible an old cookie file was left behind. This can cause a race condition when restarting the node, where the test framework might try to connect using credentials from the old cookie file, just as the node will generate new credentials and overwrite the old file. Delete any such cookie file if present prior to startup. Tree-SHA512: ae1e8bf8fd20e07c32b0715025693bb28b0e3dd34f328cae4346abf579b0c97b5db1c02782e1c46b7a3b6058d268b6d46b668e847658a6eed0be857ffb0d65dc
5 participants
When a node is restarted during a test after an unclean shutdown (such
as with -dbcrashratio), it's possible an old cookie file was left
behind. This can cause a race condition when restarting the node, where
the test framework might try to connect using credentials from the
old cookie file, just as the node will generate new credentials and
overwrite the old file.
Delete any such cookie file if present prior to startup.