-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[chore] Add extension/cgroupruntime integration tests #36617
Conversation
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.
From the Collector SIG 2024-12-11 meeting, we think this approach is okay to take
Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
Checking why the CI is failing 👀 |
@mx-psi I changed a bit the Go build approach to use it with the At the moment, we have the following scenarios when running the
Another downside of this solution is that if anyone runs the integrations Make target locally, the sudo prompt will appear. That is an issue because maybe in some systems the sudo tool is not available (Windows), wdyt if we only run those tests on the CI (check that the CI env variable is defined)? How could I document all the previous? I am totally open to any suggestion of improvement, this solution looks very specific to this component. |
@rogercoll LGTM
Can you file an issue about this? From what we talked it seemed like we were running unit tests twice under the same conditions, I would like to avoid that.
I feel like that is okay? Sudo for Windows is now a thing and I would not be suprised about other tests already having this issue of only running correctly on CI. I suggest we don't care about this until somebody complains 😄 |
Also, can you fix the merge conflict? |
Sure, I gathered the details here #36817
Sounds good :), I was not aware of |
Fixed 🤞 |
…#36617) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description Adds some integration tests for the extension. It uses the `containerd/cgroups` package to modify the current process's allocated cgroup resources and assert the corresponding values for GOMEMLIMIT/GOMAXPROCS set by the extension. <!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. --> #### Link to tracking issue Fixes open-telemetry#36545 <!--Describe what testing was performed and which tests were added.--> #### Testing Cgroup resources modification requires privileged access in GHA runner instances, thus the test must be run with `sudo`. The `go` toolchain has an `exec` flag to run tests binary(s) via another binary such as sudo. The Makefile has been modified to run Go tests files with build tag `integration` && `sudo` with the sudo command. I am not very confident with this solution, as I could not find any other component requiring privileged execution for its integration tests and the "go test -tags=integration,sudo" would run for all of them. I am all ears on other testing strategies for this use case. Similar strategy in cgroups package https://github.com/containerd/cgroups/blob/main/.github/workflows/ci.yml#L101 <!--Describe the documentation added.--> #### Documentation <!--Please delete paragraphs that you did not use before submitting.--> --------- Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
How do you ensure the integration test is running ? Locally, this test is skip because I am not running in a containerized environment.
I wonder if within Github Action this test is Skip too. Why asking ? I added integration test for a future PR to integrate gomaxecs : r0mdau#1 |
I think here you can find the execution output: https://github.com/r0mdau/opentelemetry-collector-contrib/actions/runs/12438898261/job/34732589575?pr=1#step:5:118 The CI test is named |
Description
Adds some integration tests for the extension. It uses the
containerd/cgroups
package to modify the current process's allocated cgroup resources and assert the corresponding values for GOMEMLIMIT/GOMAXPROCS set by the extension.Link to tracking issue
Fixes #36545
Testing
Cgroup resources modification requires privileged access in GHA runner instances, thus the test must be run with
sudo
. Thego
toolchain has anexec
flag to run tests binary(s) via another binary such as sudo. The Makefile has been modified to run Go tests files with build tagintegration
&&sudo
with the sudo command. I am not very confident with this solution, as I could not find any other component requiring privileged execution for its integration tests and the "go test -tags=integration,sudo" would run for all of them. I am all ears on other testing strategies for this use case.Similar strategy in cgroups package https://github.com/containerd/cgroups/blob/main/.github/workflows/ci.yml#L101
Documentation