Page MenuHomePhabricator

test: test availability of ports before assigning them
ClosedPublic

Authored by PiRK on Jul 22 2022, 15:57.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABC7448f5395e7e: test: test availability of ports before assigning them
Summary

Previously the functional tests would use a range of ports for each test process's RPC connections and peer connections, and assume these ports are available. If a port happened to be already used, the test would fail because the nodes could not bind on the address:

OSError: [Errno 98] error while attempting to bind on address ('127.0.0.1', 21325): address already in use

The port that occasionally caused issues is 21325, which is used by trezord. We could try to find another range of consecutive available ports, but after a few attempts this turns out to not be easy. With functional tests running in parallel, and with the max number of peers for each individual test having recently increased, a wider range of ports is now necessary. Better to actually check if a port is available.

With this change, each port's availability is checked before using it, and if the tried port is unavailable it is simply incremented until one is found. A cache is used to ensure the same port is returned when the functions are called multiple times for the same node/peer number.

Test Plan

ninja check-functional

Diff Detail

Event Timeline

fix mypy lint error: annotate types for dict key and value

don't reinvent memoization, use existing standard library @cache decorator.

PiRK published this revision for review.Jul 23 2022, 13:19

Tail of the build log:

[767/820] Running utility command for check-bitcoin-upgrade-activated-cashaddrenc_tests
[768/820] bitcoin: testing crypto_tests
[769/820] bitcoin: testing cashaddr_tests
[770/820] Running utility command for check-bitcoin-crypto_tests
[771/820] Running utility command for check-bitcoin-cashaddr_tests
[772/820] bitcoin-upgrade-activated: testing addrman_tests
[773/820] Running utility command for check-bitcoin-upgrade-activated-addrman_tests
[774/820] bitcoin-upgrade-activated: testing dbwrapper_tests
[775/820] Running utility command for check-bitcoin-upgrade-activated-dbwrapper_tests
[776/820] bitcoin-upgrade-activated: testing blockcheck_tests
[777/820] Running utility command for check-bitcoin-upgrade-activated-blockcheck_tests
[778/820] bitcoin-upgrade-activated: testing checkdatasig_tests
[779/820] Running utility command for check-bitcoin-upgrade-activated-checkdatasig_tests
[780/820] bitcoin-upgrade-activated: testing crypto_tests
[781/820] Running utility command for check-bitcoin-upgrade-activated-crypto_tests
[782/820] bitcoin-upgrade-activated: testing wallet_tests
[783/820] Running utility command for check-bitcoin-upgrade-activated-wallet_tests
[784/820] Linking CXX executable src/avalanche/test/test-avalanche
[785/820] bitcoin: testing transaction_tests
[786/820] Running utility command for check-bitcoin-transaction_tests
[787/820] avalanche: testing init_tests
[788/820] Running utility command for check-avalanche-init_tests
[789/820] avalanche: testing delegation_tests
[790/820] Running utility command for check-avalanche-delegation_tests
[791/820] avalanche: testing proofcomparator_tests
[792/820] Running utility command for check-avalanche-proofcomparator_tests
[793/820] avalanche: testing proofpool_tests
[794/820] Running utility command for check-avalanche-proofpool_tests
[795/820] avalanche: testing proof_tests
[796/820] Running utility command for check-avalanche-proof_tests
[797/820] Linking CXX executable src/qt/test/test_bitcoin-qt
[798/820] bitcoin-upgrade-activated: testing intmath_tests
[799/820] Running utility command for check-bitcoin-upgrade-activated-intmath_tests
[800/820] bitcoin-upgrade-activated: testing cuckoocache_tests
[801/820] Running utility command for check-bitcoin-upgrade-activated-cuckoocache_tests
[802/820] avalanche: testing compactproofs_tests
[803/820] Running utility command for check-avalanche-compactproofs_tests
[804/820] bitcoin: testing coinselector_tests
[805/820] Running utility command for check-bitcoin-coinselector_tests
[806/820] bitcoin-qt: testing test_bitcoin-qt
[807/820] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[808/820] avalanche: testing processor_tests
[809/820] Running utility command for check-avalanche-processor_tests
[810/820] avalanche: testing peermanager_tests
[811/820] Running utility command for check-avalanche-peermanager_tests
[812/820] bitcoin-upgrade-activated: testing coins_tests
[813/820] Running utility command for check-bitcoin-upgrade-activated-coins_tests
[814/820] Running bitcoin-upgrade-activated test suite
PASSED: bitcoin-upgrade-activated test suite
[815/820] bitcoin: testing coins_tests
[816/820] Running utility command for check-bitcoin-coins_tests
[817/820] Running bitcoin test suite
PASSED: bitcoin test suite
[818/820] avalanche: testing voterecord_tests
[819/820] Running utility command for check-avalanche-voterecord_tests
[820/820] Running avalanche test suite
PASSED: avalanche test suite
ninja: build stopped: cannot make progress due to previous errors.
Build build-diff failed with exit code 1
PiRK edited the summary of this revision. (Show Details)

functools.cache is not available prior to python version 3.9. Use functools.lru_cache(max_size=None) instead.

Tail of the build log:

wallet_abandonconflict.py                              | ✓ Passed  | 7 s
wallet_address_types.py                                | ✓ Passed  | 12 s
wallet_address_types.py --descriptors                  | ✓ Passed  | 8 s
wallet_avoidreuse.py                                   | ✓ Passed  | 4 s
wallet_avoidreuse.py --descriptors                     | ✓ Passed  | 5 s
wallet_backup.py                                       | ✓ Passed  | 27 s
wallet_balance.py                                      | ✓ Passed  | 13 s
wallet_balance.py --descriptors                        | ✓ Passed  | 10 s
wallet_basic.py                                        | ✓ Passed  | 19 s
wallet_coinbase_category.py                            | ✓ Passed  | 1 s
wallet_create_tx.py                                    | ✓ Passed  | 6 s
wallet_createwallet.py                                 | ✓ Passed  | 2 s
wallet_createwallet.py --descriptors                   | ✓ Passed  | 2 s
wallet_createwallet.py --usecli                        | ✓ Passed  | 3 s
wallet_descriptor.py                                   | ✓ Passed  | 6 s
wallet_disable.py                                      | ✓ Passed  | 1 s
wallet_dump.py                                         | ✓ Passed  | 5 s
wallet_encryption.py                                   | ✓ Passed  | 5 s
wallet_encryption.py --descriptors                     | ✓ Passed  | 5 s
wallet_hd.py                                           | ✓ Passed  | 7 s
wallet_hd.py --descriptors                             | ✓ Passed  | 5 s
wallet_import_rescan.py                                | ✓ Passed  | 6 s
wallet_import_with_label.py                            | ✓ Passed  | 1 s
wallet_importdescriptors.py                            | ✓ Passed  | 7 s
wallet_importmulti.py                                  | ✓ Passed  | 3 s
wallet_importprunedfunds.py                            | ✓ Passed  | 2 s
wallet_importprunedfunds.py --descriptors              | ✓ Passed  | 2 s
wallet_keypool.py                                      | ✓ Passed  | 3 s
wallet_keypool_topup.py                                | ✓ Passed  | 2 s
wallet_keypool_topup.py --descriptors                  | ✓ Passed  | 3 s
wallet_labels.py                                       | ✓ Passed  | 2 s
wallet_labels.py --descriptors                         | ✓ Passed  | 2 s
wallet_listreceivedby.py                               | ✓ Passed  | 41 s
wallet_listsinceblock.py                               | ✓ Passed  | 5 s
wallet_listsinceblock.py --descriptors                 | ✓ Passed  | 8 s
wallet_listtransactions.py                             | ✓ Passed  | 4 s
wallet_listtransactions.py --descriptors               | ✓ Passed  | 3 s
wallet_multiwallet.py                                  | ✓ Passed  | 42 s
wallet_multiwallet.py --usecli                         | ✓ Passed  | 10 s
wallet_reorgsrestore.py                                | ✓ Passed  | 3 s
wallet_resendwallettransactions.py                     | ✓ Passed  | 4 s
wallet_send.py                                         | ✓ Passed  | 8 s
wallet_startup.py                                      | ✓ Passed  | 2 s
wallet_txn_clone.py                                    | ✓ Passed  | 2 s
wallet_txn_clone.py --mineblock                        | ✓ Passed  | 3 s
wallet_txn_doublespend.py                              | ✓ Passed  | 1 s
wallet_txn_doublespend.py --mineblock                  | ✓ Passed  | 3 s
wallet_watchonly.py                                    | ✓ Passed  | 1 s
wallet_watchonly.py --usecli                           | ✓ Passed  | 1 s

ALL                                                    | ✓ Passed  | 1386 s (accumulated) 
Runtime: 279 s

----------------------------------------------------------------------
Ran 10 tests in 0.093s

OK

ninja: build stopped: cannot make progress due to previous errors.
Build build-diff failed with exit code 1

What's the impact on performance ?

What's the impact on performance ?

Checking one port takes about 10 µs if the port is available (which is true most of the time), 100 µs if the port is unavailable.

Running p2p_port the first time for an available port takes less than 400 µs. And running it again with the same n parameter (cached result) takes about 2 µs.

I'm not sure how to assess the number of time a port is assigned in the entire test suite, but assuming there are about 100 tests that assign about 64 ports, the total impact would be 2.5 s on the entire test suite.

Running ninja check-functional on master takes: Runtime 154s Acumulated 1374s
Running ninja check-functional on master after stopping trezord to avoid p2p_add_connections.py timing out takes: Runtime 158s Acumulated 1343s
Running ninja check-functional on this commit takes: Runtime 152s Accumulated 1355s

The total impact seems less than the normal variability.

Fabien requested changes to this revision.Jul 27 2022, 10:11
Fabien added inline comments.
test/functional/test_framework/util.py
344 ↗(On Diff #34499)

n is unused

352–361 ↗(On Diff #34499)

This is over complicated for no reason, you can get rid of all the local variables. And better, if PortSeed.n failed testing PortSeed.n + 1 will not retry the first previous 4 unavailable ports

This revision now requires changes to proceed.Jul 27 2022, 10:11

Simplify p2p_port and rpc_port functions as suggested in review.
Refactor the initialization code for the port counters for more clarity.
Move globals together with other related constants (PORT_MIN, PORT_RANGE)

A type: ignore annotation is needed because mypy incorrectly assumes that the globals can be None (after being initialized) and complains about potentially incrementing None

Fabien requested changes to this revision.Aug 17 2022, 20:09
Fabien added inline comments.
test/functional/test_framework/util.py
367 ↗(On Diff #34684)

that's still unused

This revision now requires changes to proceed.Aug 17 2022, 20:09
PiRK requested review of this revision.Aug 18 2022, 07:30
PiRK added inline comments.
test/functional/test_framework/util.py
367 ↗(On Diff #34684)

It is used by the lru_cache wrapper to cache the return value for the function for each given n value.

I think the test framework relies on the fact that calling the function multiple times with the same n always returns the same port: the port is first written in the bitcoin.conf file and then a TestNode is created with the same port).

We could avoid using this confusing property of caching by ensuring that we call p2p_port and rpc_port only once for each n:

Screenshot from 2022-08-18 09-29-25.png (716×684 px, 53 KB)

344 ↗(On Diff #34499)

It is not really unused. It is used by the lru_cache wrapper as an index to retrieve a port previously cached. I think we want the function to always return the same port for a given node index.

test/functional/test_framework/util.py
367 ↗(On Diff #34684)

Note that the change in my screenshot is not sufficient. initialize_datadir is called all over the place in test_framework.py. It would probably require more refactoring to ensure each peer gets the correct port assigned in all configuration files. Caching the ports is the easiest way to make sure it is working correctly.

test/functional/test_framework/util.py
367 ↗(On Diff #34684)

OK, I got it. It's worth a comment in the code though as one might come later on this and clean up inadvertently

add more explicit comment about parameter n not being unused, contrary to appearances.

The build failed due to an unexpected infrastructure outage. The administrators have been notified to investigate. Sorry for the inconvenience.
The build failed due to an unexpected infrastructure outage. The administrators have been notified to investigate. Sorry for the inconvenience.
This revision is now accepted and ready to land.Aug 24 2022, 22:30