Skip to content

ci: Install required packages when running in container #26571

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

Closed
wants to merge 1 commit into from

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Nov 25, 2022

On the master branch, the "[ASan + LSan + UBSan + integer, no depends, USDT] [jammy]" CI task fails when being run in a Docker container:

$ MAKEJOBS="-j16" FILE_ENV="./ci/test/00_setup_env_native_asan.sh" ./ci/test_run_all.sh
...
bash: line 1: add-apt-repository: command not found

This PR installs all required packages.

@hebasto hebasto added the Tests label Nov 25, 2022
@DrahtBot
Copy link
Contributor

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

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

@hebasto
Copy link
Member Author

hebasto commented Nov 25, 2022

cc @0xB10C @MarcoFalke

@maflcko
Copy link
Member

maflcko commented Nov 25, 2022

I don't think you can run this test in docker, so I don't think this patch helps overall?

@0xB10C
Copy link
Contributor

0xB10C commented Nov 25, 2022

Are the interface_usdt_*.py functional test run for you in your local docker? I think they might be skipped anyway?

@fanquake
Copy link
Member

I don't think you can run this test in docker, so I don't think this patch helps overall?

Yea. This doesn't work in Docker on Cirrus, and running locally it wouldn't work without at least modifying additional container permissions (not even sure it'd work then).

@hebasto
Copy link
Member Author

hebasto commented Nov 25, 2022

I don't think you can run this test in docker...

Yea. This doesn't work in Docker on Cirrus, and running locally it wouldn't work without at least modifying additional container permissions (not even sure it'd work then).

Are the interface_usdt_*.py functional test run for you in your local docker?

On my Ubuntu 22.04:

$ MAKEJOBS="-j16" FILE_ENV="./ci/test/00_setup_env_native_asan.sh" ./ci/test_run_all.sh
...
interface_usdt_coinselection.py                        | ○ Skipped | 1 s
interface_usdt_net.py                                  | ○ Skipped | 1 s
interface_usdt_utxocache.py                            | ○ Skipped | 1 s
interface_usdt_validation.py                           | ○ Skipped | 1 s
...

@maflcko
Copy link
Member

maflcko commented Nov 25, 2022

Yes, if the host happens to be Ubuntu 22.04, then it works. However, for any other host, with a different kernel, it will likely not

@hebasto
Copy link
Member Author

hebasto commented Nov 27, 2022

@MarcoFalke

Yes, if the host happens to be Ubuntu 22.04, then it works. However, for any other host, with a different kernel, it will likely not

What system/kernel can you suggest me to try on to confirm or deny your conjecture?

@0xB10C
Copy link
Contributor

0xB10C commented Nov 27, 2022

I don't think you can run this test in docker...

Yea. This doesn't work in Docker on Cirrus, and running locally it wouldn't work without at least modifying additional container permissions (not even sure it'd work then).

Are the interface_usdt_*.py functional test run for you in your local docker?

On my Ubuntu 22.04:

$ MAKEJOBS="-j16" FILE_ENV="./ci/test/00_setup_env_native_asan.sh" ./ci/test_run_all.sh
...
interface_usdt_coinselection.py                        | ○ Skipped | 1 s
interface_usdt_net.py                                  | ○ Skipped | 1 s
interface_usdt_utxocache.py                            | ○ Skipped | 1 s
interface_usdt_validation.py                           | ○ Skipped | 1 s
...

As expected, the tests are skipped. The tests check for privileges (required to attach to the tracepoints via the Linux kernel) and I'd expect that most users will run their local CI with their unprivileged user.

edit: To be clear: I think we should skip the install of the packages for local CI runs. Doesn't make sense to install the packages only for them not being used.

@maflcko
Copy link
Member

maflcko commented Nov 28, 2022

What system/kernel can you suggest me to try on to confirm or deny your conjecture?

Not sure, but for example on Fedora 36 (fresh install):

$ dnf install podman -y
$ podman run -it --rm ubuntu:jammy

export DEBIAN_FRONTEND=noninteractive && apt update && apt install htop git vim -y && git clone https://github.com/bitcoin/bitcoin.git ./bitcoin-core && cd ./bitcoin-core

DANGER_RUN_CI_ON_HOST="1" MAKEJOBS="-j9" FILE_ENV="./ci/test/00_setup_env_native_asan.sh" ./ci/test_run_all.sh

The failure will be:

 test  2022-11-28T09:52:31.579000Z TestFramework (INFO): hook into the coin_selection tracepoints 
 test  2022-11-28T09:52:31.614000Z TestFramework (ERROR): Unexpected exception caught during testing 
                                   Traceback (most recent call last):
                                     File "/bitcoin-core/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 133, in main
                                       self.run_test()
                                     File "/bitcoin-core/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/interface_usdt_coinselection.py", line 166, in run_test
                                       self.bpf = BPF(text=coinselection_tracepoints_program, usdt_contexts=[ctx], debug=0)
                                     File "/usr/lib/python3/dist-packages/bcc/__init__.py", line 476, in __init__
                                       raise Exception("Failed to compile BPF module %s" % (src_file or "<text>"))
                                   Exception: Failed to compile BPF module <text>


interface_usdt_coinselection.py                        | ✖ Failed  | 1 s

@0xB10C
Copy link
Contributor

0xB10C commented Nov 28, 2022

What system/kernel can you suggest me to try on to confirm or deny your conjecture?

Not sure, but for example on Fedora 36 (fresh install):

$ dnf install podman -y
$ podman run -it --rm ubuntu:jammy

export DEBIAN_FRONTEND=noninteractive && apt update && apt install htop git vim -y && git clone https://github.com/bitcoin/bitcoin.git ./bitcoin-core && cd ./bitcoin-core

DANGER_RUN_CI_ON_HOST="1" MAKEJOBS="-j9" FILE_ENV="./ci/test/00_setup_env_native_asan.sh" ./ci/test_run_all.sh

The failure will be:

 test  2022-11-28T09:52:31.579000Z TestFramework (INFO): hook into the coin_selection tracepoints 
 test  2022-11-28T09:52:31.614000Z TestFramework (ERROR): Unexpected exception caught during testing 
                                   Traceback (most recent call last):
                                     File "/bitcoin-core/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 133, in main
                                       self.run_test()
                                     File "/bitcoin-core/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/interface_usdt_coinselection.py", line 166, in run_test
                                       self.bpf = BPF(text=coinselection_tracepoints_program, usdt_contexts=[ctx], debug=0)
                                     File "/usr/lib/python3/dist-packages/bcc/__init__.py", line 476, in __init__
                                       raise Exception("Failed to compile BPF module %s" % (src_file or "<text>"))
                                   Exception: Failed to compile BPF module <text>


interface_usdt_coinselection.py                        | ✖ Failed  | 1 s

Docker containers don't bring their own kernel. They share the kernel from the host OS. Tracing with bcc on a Ubuntu 22.04 container on a Ubuntu 22.04 works, because the kernel and the kernel headers used to compile the BPF byte code on the host and in the container are the same. Most other host OS and container OS combinations won't work.

There is libbpf (a new BPF library in the Linux kernel), which as far as I understand should work in docker containers too. It would be nice to use this in the future, once there are Python bindings and its API is somewhat stable. An alternative is to run the tests in a VM...

@hebasto
Copy link
Member Author

hebasto commented Nov 28, 2022

Anyway, skipping or failing USDT-specific functional tests differ from an ability to run a sanitizer-instrumented CI task locally.

@maflcko
Copy link
Member

maflcko commented Nov 28, 2022

Not sure. I think it is good to fail early instead of silently wasting several hours of CPU and then prod the dev to start from scratch.

@0xB10C
Copy link
Contributor

0xB10C commented Nov 28, 2022

Not sure. I think it is good to fail early instead of silently wasting several hours of CPU and then prod the dev to start from scratch.

I would have proposed an alternative to this PR that only runs the USDT interface tests in Cirrus and never locally. But if I understand you correctly, that's not what we want, correct? Passing the CI locally first and then only falling in Cirrus wastes several hours of CPU and the dev needs to start from scratch.

@hebasto
Copy link
Member Author

hebasto commented Nov 28, 2022

Not sure. I think it is good to fail early instead of silently wasting several hours of CPU and then prod the dev to start from scratch.

Being interested in "ASan + LSan + UBSan + integer" part, I'm OK with failing USDT-specific tests. It is not a "wasting several hours of CPU" at all, at least for me.

Anyway, I can use this patch privately :)

Closing.

@hebasto hebasto closed this Nov 28, 2022
@maflcko
Copy link
Member

maflcko commented Nov 28, 2022

Being interested in "ASan + LSan + UBSan + integer" part, I'm OK with failing USDT-specific tests. It is not a "wasting several hours of CPU" at all, at least for me.

Not sure. --failfast may be set by the CI, so no test may be run, and you will have to start from scratch.

Anyway, I can use this patch privately :)

Yeah, I do the same, but my patch is something like:

diff --git a/ci/test/00_setup_env_native_asan.sh b/ci/test/00_setup_env_native_asan.sh
index 4f1792a9f0..db5669d25d 100755
--- a/ci/test/00_setup_env_native_asan.sh
+++ b/ci/test/00_setup_env_native_asan.sh
@@ -9,11 +9,10 @@ export LC_ALL=C.UTF-8
 # We install an up-to-date 'bpfcc-tools' package from an untrusted PPA.
 # This can be dropped with the next Ubuntu or Debian release that includes up-to-date packages.
 # See the if-then in ci/test/04_install.sh too.
-export ADD_UNTRUSTED_BPFCC_PPA=true
 
 export CONTAINER_NAME=ci_native_asan
 export PACKAGES="systemtap-sdt-dev bpfcc-tools clang llvm python3-zmq qtbase5-dev qttools5-dev-tools libevent-dev bsdmainutils libboost-dev libdb5.3++-dev libminiupnpc-dev libnatpmp-dev libzmq3-dev libqrencode-dev libsqlite3-dev"
 export DOCKER_NAME_TAG=ubuntu:22.04  # May not run in docker unless --enable-usdt is dropped
 export NO_DEPENDS=1
 export GOAL="install"
-export BITCOIN_CONFIG="--enable-usdt --enable-zmq --with-incompatible-bdb --with-gui=qt5 CPPFLAGS='-DARENA_DEBUG -DDEBUG_LOCKORDER' --with-sanitizers=address,integer,undefined CC=clang CXX=clang++"
+export BITCOIN_CONFIG="--disable-usdt--enable-zmq --with-incompatible-bdb --with-gui=qt5 CPPFLAGS='-DARENA_DEBUG -DDEBUG_LOCKORDER' --with-sanitizers=address,integer,undefined CC=clang CXX=clang++"

0xB10C added a commit to 0xB10C/bitcoin that referenced this pull request Nov 28, 2022
As mentioned in bitcoin#26571, the task running the USDT interface tests
fail when run in docker. cc7335e
in bitcoin#25528 added that the tests are run in a **VM** in Cirrus CI.
Running them locally in docker containers might not work:

- We use [bcc] as tracing toolkit which requires the kernel headers
  to compile the BPF bytecode. As docker containers use the hosts
  kernel and don't run their own, there is a potential for mismatches
  between kernel headers available in the container and the host
  kernel. This results in a failure loading the BPF byte code.
- Privilges are required to load the BPF byte code into the kernel.
  Normally, the docker containers aren't run with these.
- We currently use an untrusted third-party PPA to install the
  bpfcc-tools package on Ubuntu 22.04. Using this on a local dev
  system could be a security risk.

To not hinder the ASan + LSan + UBSan part of the CI task, the USDT
tests are disabled on non-CirrusCI runs.

[bcc]: https://github.com/iovisor/bcc
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Dec 2, 2022
2811f40 ci: only run USDT interface tests on CirrusCI (0xb10c)

Pull request description:

  As mentioned in bitcoin#26571, the task running the USDT interface tests fail when run in docker. cc7335e in bitcoin#25528 added that the tests are run in a **VM** in Cirrus CI. Running them locally in docker containers might not work:

  - We use [bcc] as tracing toolkit which requires the kernel headers to compile the BPF bytecode. As docker containers use the hosts kernel and don't run their own, there is a potential for mismatches between kernel headers available in the container and the host kernel. This results in a failure loading the BPF byte code.
  - Privilges are required to load the BPF byte code into the kernel. Normally, the docker containers aren't run with these.
  - We currently use an untrusted third-party PPA to install the bpfcc-tools package on Ubuntu 22.04. Using this on a local dev system could be a security risk.

  To not hinder the ASan + LSan + UBSan part of the CI task, the USDT tests are disabled on non-CirrusCI runs.

  [bcc]: https://github.com/iovisor/bcc

Top commit has no ACKs.

Tree-SHA512: 7c159b59630b36d5ed927b501fa0ad6f54ff3d98d0902f975cc8122b4147a7f828175807d40a470a85f0f3b6eeb79307d3465a287dbd2f3d75b803769ad0b6e7
@bitcoin bitcoin locked and limited conversation to collaborators Nov 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants