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

test_framework: detect failure of bitcoind startup #7744

Merged
merged 1 commit into from Mar 29, 2016

Conversation

Projects
None yet
3 participants
@laanwj
Member

laanwj commented Mar 25, 2016

Replace the bitcoin-cli -rpcwait after spawning bitcoind with our own loop that detects when bitcoind exits prematurely (before RPC is up).

This prevents a hang in such a case (see #7463).

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 25, 2016

Member

Not ready to merge yet. I've tested this with a -salvagewallet loop:

Unexpected exception caught during testing: bitcoind exited with status 1 during initialization
  File "/data/src/bitcoin/qa/rpc-tests/test_framework/test_framework.py", line 135, in main
    self.run_test()
  File "./wallet.py", line 295, in run_test
    self.nodes = start_nodes(3, self.options.tmpdir, [[m]] * 3)
  File "/data/src/bitcoin/qa/rpc-tests/test_framework/util.py", line 283, in start_nodes
    return [ start_node(i, dirname, extra_args[i], rpchost, binary=binary[i]) for i in range(num_nodes) ]
  File "/data/src/bitcoin/qa/rpc-tests/test_framework/util.py", line 267, in start_node
    wait_for_bitcoind_start(bitcoind_processes[i], url, i)
  File "/data/src/bitcoin/qa/rpc-tests/test_framework/util.py", line 144, in wait_for_bitcoind_start
    raise Exception('bitcoind exited with status %i during initialization' % process.returncode)
Stopping nodes

Even though reporting is better, it still hangs after "Stopping nodes", going to investigate why.

Member

laanwj commented Mar 25, 2016

Not ready to merge yet. I've tested this with a -salvagewallet loop:

Unexpected exception caught during testing: bitcoind exited with status 1 during initialization
  File "/data/src/bitcoin/qa/rpc-tests/test_framework/test_framework.py", line 135, in main
    self.run_test()
  File "./wallet.py", line 295, in run_test
    self.nodes = start_nodes(3, self.options.tmpdir, [[m]] * 3)
  File "/data/src/bitcoin/qa/rpc-tests/test_framework/util.py", line 283, in start_nodes
    return [ start_node(i, dirname, extra_args[i], rpchost, binary=binary[i]) for i in range(num_nodes) ]
  File "/data/src/bitcoin/qa/rpc-tests/test_framework/util.py", line 267, in start_node
    wait_for_bitcoind_start(bitcoind_processes[i], url, i)
  File "/data/src/bitcoin/qa/rpc-tests/test_framework/util.py", line 144, in wait_for_bitcoind_start
    raise Exception('bitcoind exited with status %i during initialization' % process.returncode)
Stopping nodes

Even though reporting is better, it still hangs after "Stopping nodes", going to investigate why.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 25, 2016

Member

Should be fixed now. The problem was that start_nodes can fail with part of the nodes already started.
wait_bitcoinds will subsequently wait forever for them to exit (as it uses a global variable to communicate bitcoind_processes).
One-to-last commit makes sure that they're stopped properly in that case.

Member

laanwj commented Mar 25, 2016

Should be fixed now. The problem was that start_nodes can fail with part of the nodes already started.
wait_bitcoinds will subsequently wait forever for them to exit (as it uses a global variable to communicate bitcoind_processes).
One-to-last commit makes sure that they're stopped properly in that case.

test_framework: detect failure of bitcoind startup
Replace the `bitcoin-cli -rpcwait` after spawning bitcoind
with our own loop that detects when bitcoind exits prematurely.

And if one node fails to start, stop the others.

This prevents a hang in such a case (see #7463).
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 26, 2016

Member

This is faster than the current -rpcwait construction, so #7745 triggers more, seems I need to solve that too here.
Thanks Marco Falke for fixing this already.

Member

laanwj commented Mar 26, 2016

This is faster than the current -rpcwait construction, so #7745 triggers more, seems I need to solve that too here.
Thanks Marco Falke for fixing this already.

try:
for i in range(num_nodes):
rpcs.append(start_node(i, dirname, extra_args[i], rpchost, binary=binary[i]))
except: # If one node failed to start, stop the others

This comment has been minimized.

@MarcoFalke

MarcoFalke Mar 28, 2016

Member

Nice!

@MarcoFalke
@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 28, 2016

Member

Good idea!
utACK 018b60c

Member

jonasschnelli commented Mar 28, 2016

Good idea!
utACK 018b60c

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke
Member

MarcoFalke commented Mar 29, 2016

utACK 018b60c

@laanwj laanwj merged commit 018b60c into bitcoin:master Mar 29, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Mar 29, 2016

Merge #7744: test_framework: detect failure of bitcoind startup
018b60c test_framework: detect failure of bitcoind startup (Wladimir J. van der Laan)
@@ -130,11 +131,33 @@ def initialize_datadir(dirname, n):
f.write("listenonion=0\n")
return datadir
def rpc_url(i, rpchost=None):
return "http://rt:rt@%s:%d" % (rpchost or '127.0.0.1', rpc_port(i))

This comment has been minimized.

@MarcoFalke

MarcoFalke Apr 10, 2016

Member

Nit: rpcbind_test.py no longer works after this.

@MarcoFalke

MarcoFalke Apr 10, 2016

Member

Nit: rpcbind_test.py no longer works after this.

This comment has been minimized.

@laanwj

laanwj Apr 15, 2016

Member

We should probably re-add that test to one of the standard sequence, otherwise it will just bitrot

@laanwj

laanwj Apr 15, 2016

Member

We should probably re-add that test to one of the standard sequence, otherwise it will just bitrot

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Jun 9, 2016

nomnombtc added a commit to nomnombtc/bitcoin that referenced this pull request Nov 12, 2016

nomnombtc added a commit to nomnombtc/bitcoin that referenced this pull request Nov 12, 2016

nomnombtc added a commit to nomnombtc/bitcoin that referenced this pull request Nov 13, 2016

ftrader added a commit to ftrader-bitcoinunlimited/hardfork_prototype_1_mvf-bu that referenced this pull request Dec 9, 2016

cherry-pick Core PR#7744 to abort test instead of freezing up
This is a merge of bitcoin/bitcoin#7744 / MVF-Core PR#16.

Instead of freezing up qa tests, wallet.py will fail on some 32-bit platforms where wallet salvage operation sometimes fails due to unknown reasons.

See also: bitcoin/bitcoin#7463 about the -salvagewallet problem.

Core has decided to disable the salvage part of the test in bitcoin/bitcoin#8038 , we may implement a Skip() of this test on 32-bit or disable that test step in a similar way.
The wallet salvage problem is definitely unrelated to any fork programming.

@dagurval dagurval referenced this pull request Aug 29, 2017

Merged

Python 3 for qa #257

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment