Run feature_bind_port_(discover|externalip).py in CI #33362
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33362. ReviewsSee the guideline for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. LLM Linter (✨ experimental)Possible typos and grammar issues:
drahtbot_id_5_m |
vasild
force-pushed
the
ci_add1111addr
branch
from
6917740 to
f047fce
Compare
|
|
vasild
force-pushed
the
ci_add1111addr
branch
from
f047fce to
9b30b0b
Compare
|
|
…dd comprehensive test coverage test: use tor_port to prevent bind conflicts test: Update feature_bind_port_discover.py instructions Replace deprecated ifconfig with modern ip commands and updated instructions for non-loopback interfaces.
… skip condition Instead of requiring a run with an explicit `--ihave1111and2222`, detect whether the routable addresses are set up and if not, then skip the test. To detect whether the addresses are set use `bitcoind` - start it and ask it to bind on them and see if it will error with "Unable to bind". Since this is what the tests do anyway, just start the nodes and see if an exception will be raised like `FailedToStartError` / "Unable to bind". This makes it possible for the CI to run `feature_bind_port_discover.py` and `feature_bind_port_externalip.py` by just setting up the addresses, without having to explicitly provide `--ihave1111and2222`.
Also explicitly specify which addresses from the docker network to assign to the VM. With `1.1.1.5` and `1111:1111::5` set on the machine, the tests `feature_bind_port_discover.py` and `feature_bind_port_externalip.py` will run.
vasild
force-pushed
the
ci_add1111addr
branch
from
9b30b0b to
e245da3
Compare
|
|
|
🐙 This pull request conflicts with the target branch and needs rebase. |
| ).stdout.strip() | ||
| os.environ["CI_CONTAINER_ID"] = container_id | ||
|
|
||
| run(["docker", "network", "connect", "--ip=1.1.1.5", "ci-ip4net", container_id]) |
There was a problem hiding this comment.
The container has been attached to both the default bridge network and ci-ip4net, hence two
IPv4 addresses. Do you think getifaddrs() may return the bridge IP first, and cause Discover() to find the wrong address?
If thats the case then I think it might be worth it to disconnect from bridge after connecting to ci-ip4net.
| CI_CCACHE_MOUNT = f"type=bind,src={os.environ['CCACHE_DIR']},dst={os.environ['CCACHE_DIR']}" | ||
|
|
||
| run(["docker", "network", "create", "--ipv6", "--subnet", "1111:1111::/112", "ci-ip6net"], check=False) | ||
| run(["docker", "network", "create", "--subnet", "1.1.1.0/24", "ci-ip4net"], check=False) |
There was a problem hiding this comment.
Would it make sense to use RFC-5737 here? (I think they are reserved for documentation and testing)
| try: | ||
| self.start_nodes() | ||
| except FailedToStartError as e: | ||
| for node in self.nodes: |
There was a problem hiding this comment.
This error/cleanup code is duplicated in both files (with the exception of printing 1 or 2 addresses), would it be worth extracting this into a helper in test_node.py or somewhere?
5 participants
This PR includes #32757 (first two commits here).
Then, on top of that:
feature_bind_port_discover.pyandfeature_bind_port_externalip.pyrequire a routable address on the machine to run. Since that was not predictably available on CI, those tests required a manual setting up of IP addresses (e.g. usingifconfig) and then running the tests with a command line option telling them that the addresses are set up. The tests were not run in CI and got rot.Change that to auto-detect, from the tests, whether the needed IP addresses are present and if yes, run the test, otherwise skip it. Also change the CI to configure the needed addresses when running the functional tests. This way the tests will be run regularly on CI.
Fixes: #31293
Fixes: #31336