-
Notifications
You must be signed in to change notification settings - Fork 320
Modernize and fix CI #459
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
Modernize and fix CI #459
Conversation
71289a6
to
ea8eb90
Compare
internal/dlopen/dlopen_test.go
Outdated
// 10000 is the default golang thread limit, so adding more will likely fail | ||
// but this number is enough to reproduce the issue most of the time for me. | ||
count := 10000 | ||
count := 1000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is as well not document in any commit? IIRC I needed such a high number to reproduce the issue I fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad; this was not intended to be changed here. I experimented with this because locally it gave me a panic.
=== RUN TestDlopenThreadSafety
runtime/cgo: pthread_create failed: Resource temporarily unavailable
(and then Go prints a huge panic message with almost 10000 goroutines).
Let's see if it works in CI now. If not, we need to address this somehow:
- run this in a sub-process and check the result from the parent, considering
pthread_create failed
message as normal. - find another way to recover from the panic (currently I don't see a way);
- find a way to assess how many goroutines can be run (it's complicated);
- drop the test from the normal CI run;
- make the count much smaller (on my machine, it succeeds with
count := 1000
, fails with5000
and larger values).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah fair enough, it is certainly not that reasonable to spawn 10000 threads in a normal test and it is not like we are going to regress easily. The fix can be easily seen by human review, so yes skipping it by default is fine to me
1. Bump actions/setup-go and actions/checkout versions. 2. Merge build-minimum step into build as the only difference between those is go version used, and it can be placed into matrix. Signed-off-by: Kir Kolyshkin <[email protected]>
1. Bump actions/setup-go and actions/checkout versions. 2. Use ubuntu-latest instead of 20.04. 3. Bump go 1.15 -> 1.24. 4. Fix a typo in job step name. Signed-off-by: Kir Kolyshkin <[email protected]>
Those are not used anywhere in the script. Signed-off-by: Kir Kolyshkin <[email protected]>
Ubuntu-20.04 is no longer supported by GHA CI, and when switching to ubuntu-latest (which is 24.04 at the moment), tests built on the host no longer work inside a container, because they require newer glibc: Running tests... /opt/src/github.com/coreos/go-systemd/test_bins /opt/src/github.com/coreos/go-systemd - activation ./activation.test: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.34' not found (required by ./activation.test) ./activation.test: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.32' not found (required by ./activation.test) So, let's switch to building tests inside the container as well. Note: - the default Go version that comes with a distro is used for building; - there is no need to build sources. Signed-off-by: Kir Kolyshkin <[email protected]>
6f139e4
to
b16d98e
Compare
@Luap99 OK the current theory wrt TestDlopenThreadSafety panic is they've changed something in runtime so that the test is working fine when go 1.18 (and, presumably, newer) is used to compile it. It does not work with go 1.13 and go 1.15. I'm going to test it. |
@kolyshkin Ack, likely not worth to spend to much time trying to figure out what is going on these older versions. I am fine if the test gets skipped for now at least. btw I invited you to this repo so you can update the merge protection for the new task names yourself. |
dbadb91
to
6c3fd54
Compare
It panics in CI (see coreos#462), so let's skip it until a workaround is available. Signed-off-by: Kir Kolyshkin <[email protected]>
The main idea is to simplify branch protection rules, requiring just these all-done jobs to be passed. Signed-off-by: Kir Kolyshkin <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Bump various versions, fix various issues, making CI working again. See individual commits for more details.
One particular thing -- branch protection rules now need to be modified to only require
all-done
jobs (see the last commit).