ci: move ASan job to GitHub Actions from Cirrus CI #30193

m3dwards · 2024-05-29T14:53:48Z

PR for moving the ASAN + LSAN + USDT + friends job to github actions from Cirrus.

The motivation for this PR is that this task needs a full VM (or bare metal) to function, because of the tracepoints. It can not run in a container on an arbitrary Linux, because the outside machine must exactly match the specification of the distro used in the CI task config. This requires more maintenance for the persistent worker, and I think moving to GHA will reduce the maintenance burden, or at least make it possible for anyone to work on.

Also, it makes it easier to run the task on forks (bitcoin-inquisition, bitcoin-knots, devel forks, ...) without having to set-up a real machine.

DrahtBot · 2024-05-29T14:53:51Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, hebasto, achow101
Concept ACK	0xB10C, kristapsk

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30164 ([PoC] ci: Add FreeBSD GitHub Actions job by hebasto)
#29274 (Support self-hosted Cirrus workers on forks by Sjors)

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.

maflcko · 2024-05-29T15:03:09Z

Concept ACK, may review tomorrow if GHA is back online by then.

hebasto · 2024-05-29T15:05:26Z

Concept ACK.

m3dwards · 2024-05-29T15:21:52Z

Question: Do we want to keep this job whole or split parts out (such as just running the USDT tests)?

If we are keeping it whole as it is with ASAN and LSAN etc, I will investigate if we can get the IPV6 tests working. If I can't get them working, I would like to update the python IPV6 check as it's currently passing even though IPV6 isn't working on the runner.

willcl-ark · 2024-05-30T08:37:24Z

.github/workflows/ci.yml

Is a single job chosen for a reson here? The free hosted GHA runners have 4 (v)CPUs available AFAIK...

I have been hitting issues on github actions runners cancelling the job after 4 minutes ,which is during compile, so this was an attempt to reduce the CPU load. It hasn't helped. See actions/runner-images#9848 (comment)

0xB10C · 2024-05-30T10:01:11Z

Concept ACK! Thanks for working on this!

maflcko · 2024-05-31T19:21:17Z

Please remove the Cirrus task in the same commit, to make review easier.

m3dwards · 2024-06-03T12:03:49Z

@maflcko @hebasto @fanquake do we want to just extract the USDT part of the job into Github Actions?

maflcko · 2024-06-03T12:13:26Z

Not sure. It seems more work and overhead to maintain two tasks, instead of one, no?

maflcko · 2024-06-03T12:21:09Z

Well, if you really wanted to do it, you could move it into the "test each commit" task, and make that one run every time, to keep the number of tasks the same.

m3dwards · 2024-06-03T17:10:07Z

@maflcko happy to keep the job together as it is. Nothing prevents a split later if it's desired. I will look into the functional tests that require IPV6.

maflcko

lgtm, except for the feedback

maflcko · 2024-06-07T12:24:56Z

ci/test/02_run_container.sh

Please clarify in the name that this was set up for CI

Also, it could split this up into a separate commit/pull, as this affects more than just this one task?

maflcko · 2024-06-07T12:26:38Z

ci/test/02_run_container.sh

I don't think this works, does it? installation happens when the image is built (docker build), not when the tests are compiled and run.

See the Cirrus CI comment:

# In the image build step, no external environment variables are available, # so any settings will need to be written to the settings env file:

Oh you are right. This was a late change after I had already had the USDT tests running and didn't check that they were still running 🤦

maflcko · 2024-06-07T12:26:46Z

ci/test/00_setup_env_native_asan.sh

I was under the impression that these things were not relevant for these sanitisation tests but if they are I will add them back in.

The gui tests and benchmarks should also be running under the sanitizer, otherwise bugs may be missed in them.

In theory, only the fuzz binary compilation could be excluded, because there is a separate task for the fuzz tests under asan. But either seems fine.

Allows IPV6 functional tests to run inside the container

maflcko · 2024-06-09T08:06:58Z

ci/test/02_run_container.sh

Not sure about exposing this. This is an expert-only setting. Anyone changing it needs to understand exactly what they are doing, so they might as well modify the ci/test/00_setup_env_native_asan.sh config file manually.

I'd prefer to keep the sed approach as-is. If you want to change it, it may be better to do in a separate pull request. This way, the focus here is kept on only moving the task from one infra to the other.

Initially I changed it as I didn't understand why it needed the sed, but after I did understand, I thought it was nicer for someone to be able to run the USDT tests outside of CI by changing an environment variable rather than changing the contents of a file.

Happy to change it back to the sed approach.

I thought it was nicer for someone to be able to run the USDT tests outside of CI by changing an environment variable

This is not enough. They will have to set up the exact system (kernel) outside the CI, as is used inside the CI. Either in a VM or on metal.

Also the new build arg may invalidate the layer cache of unrelated images.

Seems fine, if you feel strongly. Happy to ACK either approach.

No strong feelings, switched it back to sed approach.

maflcko · 2024-06-09T14:25:37Z

ci/test/02_run_container.sh

Is this a fix for a bug outside of GitHub Actions? If yes, it would be good to have exact steps to test locally. Maybe even in a separate pull request.

Yes, there are two issues that this fixes. One is with the RPC server and another is with -proxy. Both issues stem from the fact that the containers have IPV6 but only on the loopback interface (::1).

I have made a separate PR for the -proxy one which is #30245 (comment)

There is an existing (closed) issue for the bind problem which is #13155 but this is harder to fix as it's in libevent. I was planning on adding my comments to that closed issue and perhaps reopening it.

If both these issues were fixed then we could remove this IPV6 network and all the tests should continue passing, including the IPV6 ones.

maflcko · 2024-06-10T19:49:21Z

.cirrus.yml

Should remove the noble type completely in this file

Moving it from Cirrus CI so it can be easier to maintain and used by forks

maflcko · 2024-06-12T15:30:33Z

review ACK 9eea51d

lgtm

kristapsk · 2024-06-12T16:31:23Z

Concept ACK

hebasto

ACK 9eea51d.

achow101 · 2024-06-17T19:35:42Z

ACK 9eea51d

maflcko

found a nit

maflcko · 2024-06-12T16:52:50Z

.github/workflows/ci.yml

+    timeout-minutes: 120
+    env:
+      FILE_ENV: "./ci/test/00_setup_env_native_asan.sh"
+      INSTALL_BCC_TRACING_TOOLS: true


nit: Can remove this.

PR to remove #30299

maflcko · 2024-06-12T16:58:35Z

.github/workflows/ci.yml

+        run: sed -i "s|\${INSTALL_BCC_TRACING_TOOLS}|true|g" ./ci/test/00_setup_env_native_asan.sh
+
+      - name: CI script
+        run: ./ci/test_run_all.sh


nit: could you link to a failing run? Just to double check that CI_FAILFAST_TEST_LEAVE_DANGLING works?

Failing run: https://github.com/m3dwards/bitcoin/actions/runs/9354822480/job/25748533616

Looks ok to me, shows exit code 1.

For completeness, here is a run with a failing test with CI_FAILFAST_TEST_LEAVE_DANGLING removed: https://github.com/m3dwards/bitcoin/actions/runs/9562790625/job/26359909811. The job fails with exit code 137. The windows job behaves differently but I don't think that uses --failfast

518b06c ci: remove unused bcc variable from workflow (Max Edwards) Pull request description: Fixes #30193 (comment) ACKs for top commit: maflcko: lgtm ACK 518b06c Tree-SHA512: c87364670e26e15176ee21372a2cc100db0c275a5cffb37cc33ec4c2d85d6067b593bd4a6dea37bf478d2af197786df9dfac3cfb76db023c8db37184bb104458

maflcko · 2024-06-19T11:07:18Z

.github/workflows/ci.yml

+        id: ccache-cache
+        uses: actions/cache/restore@v4
+        with:
+          path: ${{ env.CCACHE_DIR }}


https://github.com/bitcoin/bitcoin/actions/runs/9579271766/job/26411361263#step:7:12

Warning: Path Validation Error: Path(s) specified in the action for caching do(es) not exist, hence no cache is being saved.

CCache is currently written to cache docker volume which I assume on Cirrus CI gets shared between multiple jobs? As a github actions is more idempotent / ephemeral this isn't going to work as the ccache dir on the host is left empty at the end of the run.

I can see that this is the same strategy used for depends output and sources and previous_releases.

The solution could be to modify 02_run_container.sh script to have "GHA mode" where instead of using docker volumes we just bind mount to directories on the host that the github cache action can copy from / to.

Ah right. I forgot about this. Your suggestion sounds good.

Allows IPV6 functional tests to run inside the container Github-Pull: bitcoin#30193 Rebased-From: 4b527fa

Moving it from Cirrus CI so it can be easier to maintain and used by forks Github-Pull: bitcoin#30193 Rebased-From: 9eea51d

fanquake · 2024-06-19T11:54:30Z

Backported to 27.x in #30305.

b3093eb doc: Update rel notes for 27.x (fanquake) 6338f92 upnp: add compatibility for miniupnpc 2.2.8 (Cory Fields) f34e446 ci: remove unused bcc variable from workflow (Max Edwards) 0d524b1 ci: move Asan / LSan / USDT job to Github Actions (Max Edwards) 43c40dd ci: add IPV6 network to ci container (Max Edwards) Pull request description: Backports: * #30193 * #30283 * #30299 ACKs for top commit: willcl-ark: ACK b3093eb stickies-v: ACK b3093eb Tree-SHA512: 325149f2b388072276e10fae2ebb7d8f3f5138d75f237c0182a09c631334fc2af9c2fe500db31bf41e94d4f154771e3cd386f8eb0d09d7a1ad656f637b71e735

da205dd ci: increase available ccache size to 300MB (Max Edwards) 4ecbbd9 ci: add option for running tests without volume (Max Edwards) Pull request description: Fixes: #30193 (comment) Cache wasn't being saved when run on GHA because the default behaviour of the CI script was to store cache items in a docker volume. This works on Cirrus CI as the volumes are shared but it does not work on Github Actions in which each run is ephemeral. Kept the default behaviour the same so hopefully this continues to work for the Cirrus CI jobs. ACKs for top commit: maflcko: utACK da205dd hebasto: ACK da205dd. Tree-SHA512: 3b35482c0628adb60574a1462181ecfcb06cb237ed48beb6fe9aa51110be82f863dc9147e7f8d82960450aa6ecc3a24a70e3c8283fd24cdad075dbfb8fc93095

hebasto · 2024-07-14T07:23:38Z

There is nothing to port to the CMake staging branch. Deleting the "Needs CMake port" label.

DrahtBot added the Tests label May 29, 2024

m3dwards force-pushed the move-asan-to-gha branch from ad401e0 to 9d26b3b Compare May 29, 2024 15:07

m3dwards mentioned this pull request May 29, 2024

CI test Sjors/bitcoin#30

Closed

DrahtBot mentioned this pull request May 29, 2024

ci: Add FreeBSD GitHub Actions job #30164

Closed

willcl-ark reviewed May 30, 2024

View reviewed changes

hebasto added the Needs CMake port label Jun 5, 2024

m3dwards force-pushed the move-asan-to-gha branch 2 times, most recently from d9326c8 to 3fd7bf0 Compare June 7, 2024 11:47

maflcko suggested changes Jun 7, 2024

View reviewed changes

m3dwards changed the title ~~ci: move ASAN job to GitHub Actions from Cirrus CI~~ ci: move ASan job to GitHub Actions from Cirrus CI Jun 7, 2024

This was referenced Jun 7, 2024

[DO NOT MERGE] cmake: Migrate CI scripts to CMake-based build system -- WIP #29790

Closed

Remove the legacy wallet and BDB dependency #28710

Merged

m3dwards force-pushed the move-asan-to-gha branch from 3fd7bf0 to 7896174 Compare June 8, 2024 15:53

ci: add IPV6 network to ci container

4b527fa

Allows IPV6 functional tests to run inside the container

m3dwards force-pushed the move-asan-to-gha branch from 7896174 to b4b4f27 Compare June 8, 2024 16:10

DrahtBot added the CI failed label Jun 8, 2024

maflcko reviewed Jun 9, 2024

View reviewed changes

maflcko reviewed Jun 10, 2024

View reviewed changes

ci: move Asan / LSan / USDT job to Github Actions

9eea51d

Moving it from Cirrus CI so it can be easier to maintain and used by forks

m3dwards force-pushed the move-asan-to-gha branch from b4b4f27 to 9eea51d Compare June 12, 2024 13:21

DrahtBot requested review from 0xB10C and hebasto June 12, 2024 15:30

m3dwards marked this pull request as ready for review June 12, 2024 15:37

DrahtBot removed the CI failed label Jun 12, 2024

DrahtBot mentioned this pull request Jun 13, 2024

Fix issues with CI on forks #29274

Merged

hebasto approved these changes Jun 17, 2024

View reviewed changes

achow101 merged commit d97ddbe into bitcoin:master Jun 17, 2024

m3dwards deleted the move-asan-to-gha branch June 17, 2024 20:38

maflcko reviewed Jun 18, 2024

View reviewed changes

m3dwards mentioned this pull request Jun 18, 2024

ci: remove unused bcc variable from workflow #30299

Merged

maflcko reviewed Jun 19, 2024

View reviewed changes

willcl-ark mentioned this pull request Jun 19, 2024

ci: Move more tasks to GHA? #30304

Closed

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Jun 19, 2024

ci: add IPV6 network to ci container

43c40dd

Allows IPV6 functional tests to run inside the container Github-Pull: bitcoin#30193 Rebased-From: 4b527fa

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Jun 19, 2024

ci: move Asan / LSan / USDT job to Github Actions

0d524b1

Moving it from Cirrus CI so it can be easier to maintain and used by forks Github-Pull: bitcoin#30193 Rebased-From: 9eea51d

fanquake mentioned this pull request Jun 19, 2024

[27.x] More backports #30305

Merged

m3dwards mentioned this pull request Jun 19, 2024

ci: add option for running tests without volume #30310

Merged

hebasto removed the Needs CMake port label Jul 14, 2024

maflcko mentioned this pull request Feb 20, 2025

ci: Intermittent failure "Could not resolve host: github.com" #31889

Closed

bitcoin locked and limited conversation to collaborators Jul 14, 2025

ci: move ASan job to GitHub Actions from Cirrus CI #30193

ci: move ASan job to GitHub Actions from Cirrus CI #30193

Uh oh!

Conversation

m3dwards commented May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

maflcko commented May 29, 2024

Uh oh!

hebasto commented May 29, 2024

Uh oh!

m3dwards commented May 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xB10C commented May 30, 2024

Uh oh!

maflcko commented May 31, 2024

Uh oh!

m3dwards commented Jun 3, 2024

Uh oh!

maflcko commented Jun 3, 2024

Uh oh!

maflcko commented Jun 3, 2024

Uh oh!

m3dwards commented Jun 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

m3dwards Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Jun 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jun 12, 2024

Uh oh!

kristapsk commented Jun 12, 2024

Uh oh!

hebasto left a comment

m3dwards commented May 29, 2024 •

edited

Loading

DrahtBot commented May 29, 2024 •

edited

Loading

m3dwards commented Jun 3, 2024 •

edited

Loading

m3dwards Jun 7, 2024 •

edited

Loading

maflcko Jun 9, 2024 •

edited

Loading

m3dwards Jun 18, 2024 •

edited

Loading