test: set a name for CI Docker containers

fanquake commented at 3:59 am on February 6, 2020: member

Addresses one part of #16664, by making it easier to identify CI containers that are running locally. By default Docker will generate random names, like peaceful_rubin, with this change, we explicitly set names for all containers.

fanquake added the label Tests on Feb 6, 2020

fanquake requested review from MarcoFalke on Feb 6, 2020

DrahtBot commented at 7:56 am on February 6, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18077 (net: Add NAT-PMP port forwarding support by hebasto)

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.

promag commented at 9:23 am on February 6, 2020: member

Concept ACK, and LGTM.

hebasto commented at 9:32 am on February 6, 2020: member

What are the actual benefits of exporting DOCKER_NAME variable? Do we use it somehow?

in ci/test/00_setup_env_mac_host.sh:9 in db4df60ebb outdated

5@@ -6,6 +6,7 @@
6 
7 export LC_ALL=C.UTF-8
8 
9+export DOCKER_NAME=native_macos

theStack commented at 12:36 pm on February 6, 2020:

Note that this line is without affect, as the native macOS native build doesn’t use Docker (more detailled explanation: the script of this build sets export RUN_CI_ON_HOST=true a few lines below, and the install script 04_install.sh only starts Docker if RUN_CI_ON_HOST is not set.)

fanquake commented at 12:41 pm on February 6, 2020:

Thanks, I’ve fixed this up.

fanquake force-pushed on Feb 6, 2020

theStack commented at 12:41 pm on February 6, 2020: member

Concept ACK.

What are the actual benefits of exporting DOCKER_NAME variable? Do we use it somehow?

It is used as argument for Docker (--name $DOCKER_NAME) to give the containers a concrete name instead of generating a random one, see #16623#pullrequestreview-275768283.

hebasto commented at 12:44 pm on February 6, 2020: member

It is used as argument for Docker (--name $DOCKER_NAME) to give the containers a concrete name instead of generating a random one, see #16623 (review).

I know it ;) What are container concrete names used for? What is wrong with random ones? (All questions are in context of CI)

theStack commented at 1:29 pm on February 6, 2020: member

What are container concrete names used for? What is wrong with random ones? (All questions are in context of CI)

From my perspective it would be a convenience feature right now: whenever as developer you make non-trivial changes in the CI scripts you very likely need to reproduce the travis build locally (as described in e.g. https://github.com/erdc/proteus/wiki/Replicating-the-TravisCI-Environment-on-your-Local-Machine) to dig deeper and inspect the container in some way (show logs, kill it, restart it, execute a command inside it etc. etc….) . For that it is way more comfortable to have deterministic names instead of the need to always find out the container hash or random name via docker ps first.

hebasto commented at 1:38 pm on February 6, 2020: member

From my perspective it would be a convenience feature right now: whenever as developer you make non-trivial changes in the CI scripts you very likely need to reproduce the travis build locally (as described in e.g. https://github.com/erdc/proteus/wiki/Replicating-the-TravisCI-Environment-on-your-Local-Machine) to dig deeper and inspect the container in some way (show logs, kill it, restart it, execute a command inside it etc. etc….) . For that it is way more comfortable to have deterministic names instead of the need to always find out the container or random name via docker ps first.

In case a developer keeps travis docker containers locally, it is up to her to patch scripts locally.

IMHO, no need for these changes to the repository (if the only use case is local running).

MarcoFalke commented at 1:50 pm on February 6, 2020: member

Just a hint that how to run locally is described here: https://github.com/bitcoin/bitcoin/tree/master/ci#running-a-stage-locally

Travis only provides the compute to use, we don’t rely on any of its software

theStack commented at 3:00 pm on February 6, 2020: member

In case a developer keeps travis docker containers locally, it is up to her to patch scripts locally.

IMHO, no need for these changes to the repository (if the only use case is local running).

While I agree that the changes are not strictly needed right now, I’d always prefer a reproducible behaviour to random behaviour in CI/testing, even if it concerns only something trivial as just container names.

Just a hint that how to run locally is described here: https://github.com/bitcoin/bitcoin/tree/master/ci#running-a-stage-locally

I’m aware of that, but just because all CI stages pass on a local (highly individual) development environment doesn’t necessarily mean it also does on the Travis environment. While working on #17900 locally everything was passing fine, but on Travis it failed. I didn’t see any alternative as to recreate the Travis environment in a Docker container locally to reproduce exactly what’s going an and what is different to my local development machine (that’s why I wrote “non-trivial” changes in my reasoning).

Travis only provides the compute to use, we don’t rely on any of its software

Quick counter-example: from where would the testing scripts get the variable $TRAVIS_OS_NAME from without Travis (set in .travis.yml)? I highly doubt that the CI setup as it is right now is as decoupled from Travis than most people think (primarily because it is almost never run locally by anyone, only from people making changes).

MarcoFalke commented at 5:00 pm on February 6, 2020: member

Quick counter-example: from where would the testing scripts get the variable $TRAVIS_OS_NAME from without Travis (set in .travis.yml)? I highly doubt that the CI setup as it is right now is as decoupled from Travis than most people think (primarily because it is almost never run locally by anyone, only from people making changes).

I do test all linux builds locally on an arm machine continuously. TRAVIS_OS_NAME is only used to distinguish osx from linux, and should work fine unless you want to run the osx build locally.

fanquake commented at 0:21 am on February 7, 2020: member

I don’t really understand the push back here. This is straightforward, and makes it easier to identify containers when you’re running tests locally. I can’t see why that’s something you should have to “patch scripts locally.” to do. However if this is so controversial it can just be closed.

hebasto commented at 7:03 am on February 7, 2020: member

@fanquake

However if this is so controversial it can just be closed.

Please don’t close.

This is straightforward, and makes it easier to identify containers when you’re running tests locally.

It was not clear for me from the OP. Now I’m convinced that these changes are useful.

Concept ACK. Could the PR description be more descriptive with adding “makes it easier to identify containers when you’re running tests locally”?

theStack approved

theStack commented at 3:48 pm on February 7, 2020: member

ACK https://github.com/bitcoin/bitcoin/pull/18081/commits/d87e029669542febe0f6f4a2177450ad16350786 (The occured travis fail is unrelated, has to do with low disk space for s390x build, see #18016)

fanquake commented at 11:18 pm on February 7, 2020: member

Could the PR description be more descriptive

I’ve updated the PR description.

in ci/test/00_setup_env.sh:37 in d87e029669 outdated

33@@ -34,6 +34,7 @@ export USE_BUSY_BOX=${USE_BUSY_BOX:-false}
34 export RUN_UNIT_TESTS=${RUN_UNIT_TESTS:-true}
35 export RUN_FUNCTIONAL_TESTS=${RUN_FUNCTIONAL_TESTS:-true}
36 export RUN_FUZZ_TESTS=${RUN_FUZZ_TESTS:-false}
37+export DOCKER_NAME=${DOCKER_NAME:-ubuntu_18_04}

MarcoFalke commented at 6:43 pm on February 9, 2020:

0export DOCKER_NAME=${DOCKER_NAME:-ci_unnamed}

Because not all unnamed docker containers might be running Ubuntu

MarcoFalke commented at 8:34 pm on February 9, 2020:

If you feel extra fancy, you might even rename the variable to CONTAINER_NAME to not cause confusion with DOCKER_NAME_TAG.

in ci/test/00_setup_env_arm.sh:20 in d87e029669 outdated

16@@ -17,6 +17,7 @@ if [ -n "$QEMU_USER_CMD" ]; then
17   export PACKAGES="$PACKAGES qemu-user"
18 fi
19 # Use debian to avoid 404 apt errors when cross compiling
20+export DOCKER_NAME=arm_linux_gnueabihf

MarcoFalke commented at 6:43 pm on February 9, 2020:

0export DOCKER_NAME=ci_arm_linux

MarcoFalke commented at 6:50 pm on February 9, 2020:

Oh, and this line should be moved, because the above comment applies to DOCKER_NAME_TAG

in ci/test/00_setup_env_i686_centos.sh:10 in d87e029669 outdated

 6@@ -7,6 +7,7 @@
 7 export LC_ALL=C.UTF-8
 8 
 9 export HOST=i686-pc-linux-gnu
10+export DOCKER_NAME=centos_7

MarcoFalke commented at 6:44 pm on February 9, 2020:

0export DOCKER_NAME=ci_i686_centos_7

to avoid name collisions with other local containers named centos_7

in ci/test/00_setup_env_mac.sh:9 in d87e029669 outdated

5@@ -6,6 +6,7 @@
6 
7 export LC_ALL=C.UTF-8
8 
9+export DOCKER_NAME=macos

MarcoFalke commented at 6:45 pm on February 9, 2020:

0export DOCKER_NAME=ci_macos_cross

in ci/test/00_setup_env_native_asan.sh:9 in d87e029669 outdated

5@@ -6,6 +6,7 @@
6 
7 export LC_ALL=C.UTF-8
8 
9+export DOCKER_NAME=native_asan

MarcoFalke commented at 6:46 pm on February 9, 2020:

0export DOCKER_NAME=ci_native_asan

MarcoFalke approved

MarcoFalke commented at 6:47 pm on February 9, 2020: member

ACK, though I’d prefer to use the ci_ prefix for the name to avoid name collisions with identically named local containers.

0Error: error creating container storage: the container name "centos_7" is already in use by "b61e5703bd1b6d0021dfc8d7b0b21cf2843721a4081bbdd7027c7c511e63a9b4". You have to remove that container to be able to reuse that name.: that name is already in use

test: set a name for CI Docker containers 9e111db088

fanquake force-pushed on Feb 10, 2020

fanquake commented at 12:05 pm on February 10, 2020: member

though I’d prefer to use the ci_ prefix for the name

Done, and addressed other comments.

MarcoFalke commented at 12:47 pm on February 10, 2020: member

ACK 9e111db088e4137865ae068d206c769994ea0a29

laanwj referenced this in commit b063cb690f on Feb 10, 2020

laanwj merged this on Feb 10, 2020

laanwj closed this on Feb 10, 2020

fanquake deleted the branch on Feb 12, 2020

DrahtBot locked this on Feb 15, 2022

test: set a name for CI Docker containers #18081

Conflicts