Skip to content
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

ci: Temporarily disable bpfcc-tools #29788

Merged
merged 1 commit into from
Apr 4, 2024
Merged

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Apr 2, 2024

This works around package install errors, such as https://github.com/bitcoin/bitcoin/runs/23354020361. Should be possible to reproduce locally via apt update && apt install bpfcc-tools on noble:

 python3-bpfcc : Depends: libbpfcc (>= 0.29.1+ds-1ubuntu4) but it is not going to be installed

@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 2, 2024

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 hebasto, TheCharlatan

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

@DrahtBot DrahtBot added the Tests label Apr 2, 2024
@maflcko
Copy link
Member Author

maflcko commented Apr 2, 2024

(moved to description)

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fac012c, I have reviewed the code, it looks OK. And CI is green.

@achow101
Copy link
Member

achow101 commented Apr 2, 2024

Disabling bpfcc-tools means that CI no longer runs any of the USDT tests. Any idea when it could be re-enabled?

@hebasto
Copy link
Member

hebasto commented Apr 2, 2024

Any idea when it could be re-enabled?

I guess, the package issue should be resolved by the Ubuntu 24.04 release date (this month).

@achow101
Copy link
Member

achow101 commented Apr 2, 2024

It seems unwise to have CI that runs for every PR to be dependent on an unstable OS. From what I can tell, the purpose of using Ubuntu noble is to get access to clang 18. Instead of doing that, could we instead use the LTS release and just use the llvm package repo to get the clang versions we want?

@0xB10C
Copy link
Contributor

0xB10C commented Apr 3, 2024

I'm missing context for this change. Why no PR description and no commit message body?

@maflcko
Copy link
Member Author

maflcko commented Apr 3, 2024

It seems unwise to have CI that runs for every PR to be dependent on an unstable OS. From what I can tell, the purpose of using Ubuntu noble is to get access to clang 18. Instead of doing that, could we instead use the LTS release and just use the llvm package repo to get the clang versions we want?

I agree, but that is not possible, because the bpfcc-tools on the Ubuntu 22.04 LTS were outdated as well. So they'd require a ppa as well, see #27510.

I am not sure if two third-party PPAs are more stable than a vanilla Ubuntu, but I won't object a pull request that implements the changes. (I won't be working on this myself).

If someone implements this, it may also be good to move the runner to a GHA 22.04 VM, to lift the requirements from the self-hosted runners. (But I won't be working on this myself either)

Personally I think it is fine to disable the bpf tests for a few days, or even weeks, as the code is unlikely to break. Even if it were to break, and people didn't run the tests locally, and missed it, it should not be too hard to fix them after the temporary disable.

@maflcko
Copy link
Member Author

maflcko commented Apr 3, 2024

I'm missing context for this change. Why no PR description and no commit message body?

I put it in the second comment. Edited and moved to the description.

@Sjors
Copy link
Member

Sjors commented Apr 3, 2024

Personally I think it is fine to disable the bpf tests for a few days, or even weeks, as the code is unlikely to break.

To be on the very safe side, maybe open an issue to undo it and tag that for v28.0.

@maflcko
Copy link
Member Author

maflcko commented Apr 3, 2024

If someone implements this, it may also be good to move the runner to a GHA 22.04 VM, to lift the requirements from the self-hosted runners. (But I won't be working on this myself either)

Obviously this would expose the CI to GH changing the kernel below the CI without notice, e.g. bitcoin-core/secp256k1@05bfab6 . So it may or may not be more or less fragile.

I agree that the CI task is fragile, because it assumes an exact kernel to be known and available, but I honestly don't know which alternative is less fragile. Suggestions are more than welcome.

@0xB10C
Copy link
Contributor

0xB10C commented Apr 3, 2024

I'm missing context for this change. Why no PR description and no commit message body?

I put it in the second comment. Edited and moved to the description.

Ok, I did a bit more digging: I was missing the context that #29765 upgraded the CI images to a newer Ubuntu version and that the Cirrus CI / ASan + LSan + UBSan + integer, no depends, USDT job is now failing. (edit: only the i686 job was upgraded in #29786).

Is it an option to drop the USDT tests from the Cirrus CI / ASan + LSan + UBSan + integer, no depends, USDT job and move them to a GH actions 23.04 VM if "lifting the requirements from the self-hosted runners" is the general direction we want to go? (probably makes sense to merge this sooner than later before adding a GH actions VM job to have CI green again?).

@hebasto
Copy link
Member

hebasto commented Apr 3, 2024

Is it an option to drop the USDT tests from the Cirrus CI / ASan + LSan + UBSan + integer, no depends, USDT job and move them to a GH actions 23.04 VM if "lifting the requirements from the self-hosted runners" is the general direction we want to go? (probably makes sense to merge this sooner than later before adding a GH actions VM job to have CI green again?).

I'll look into it.

@maflcko
Copy link
Member Author

maflcko commented Apr 3, 2024

Ok, I did a bit more digging: I was missing the context that #29765 upgraded the CI images to a newer Ubuntu version and that the Cirrus CI / ASan + LSan + UBSan + integer, no depends, USDT job is now failing.

29765 didn't do the switch, it was done in fa83b65 last year.

Is it an option to drop the USDT tests from the Cirrus CI / ASan + LSan + UBSan + integer, no depends, USDT job and move them to a GH actions 23.04 VM if "lifting the requirements from the self-hosted runners" is the general direction we want to go? (probably makes sense to merge this sooner than later before adding a GH actions VM job to have CI green again?).

23.04 does not exist. Only ubuntu-22.04, no?

Again, I am not sure what the best solution is, but if someone wants to propose something else and receive the git blame, I am all-in.

@0xB10C
Copy link
Contributor

0xB10C commented Apr 3, 2024

GH actions 23.04 VM

GitHub currently only supports: ubuntu-latest (22.04), ubuntu-22.04, and ubuntu-20.04

https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners/about-github-hosted-runners#standard-github-hosted-runners-for-public-repositories

@maflcko
Copy link
Member Author

maflcko commented Apr 4, 2024

I am happy to close this pull, if people don't want it.

However, I don't see why running the CI not at all is better than skipping just the bpfcc part.

Copy link
Contributor

@TheCharlatan TheCharlatan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fac012c

@fanquake
Copy link
Member

fanquake commented Apr 4, 2024

I think merging this temporarily is fine. I think the relevant upstream issue is https://bugs.launchpad.net/ubuntu/+source/bpfcc/+bug/2052813 ? I'm assuming this is going to be resolved soon. Note that the installation issues only seem to be with x86_64? Running apt update && apt install bpfcc-tools on noble, on an aarch64 machine, currently works. I thought that might be because it doesn't seem to depend on libbpfcc 0.29.1+ds-1ubuntu4, but that is also currently available/installable:

# cat /etc/lsb-release 
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=24.04
DISTRIB_CODENAME=noble
DISTRIB_DESCRIPTION="Ubuntu Noble Numbat (development branch)"
# apt install libbpfcc
libbpfcc is already the newest version (0.29.1+ds-1ubuntu4).
# apt install bpfcc-tools
bpfcc-tools is already the newest version (0.29.1+ds-1ubuntu4).

@fanquake fanquake merged commit 5de68e4 into bitcoin:master Apr 4, 2024
15 of 16 checks passed
@maflcko
Copy link
Member Author

maflcko commented Apr 4, 2024

#29804

@maflcko maflcko deleted the 2404-ci-bcfcc- branch April 4, 2024 10:25
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Apr 5, 2024
@fanquake
Copy link
Member

fanquake commented Apr 5, 2024

Pulled this into 27.x.

Pttn added a commit to RiecoinTeam/Riecoin that referenced this pull request Apr 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants