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

[chore] Add extension/cgroupruntime integration tests #36617

Merged
merged 23 commits into from
Dec 18, 2024

Conversation

rogercoll
Copy link
Contributor

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. 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

Documentation

Copy link
Member

@mx-psi mx-psi left a 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

.chloggen/cgroup_integration.yaml Outdated Show resolved Hide resolved
extension/cgroupruntimeextension/integration_test.go Outdated Show resolved Hide resolved
@rogercoll rogercoll changed the title [extension/cgroupruntime] Add GOMEMLIMIT/GOMAXPROCS integration tests [chore] Add extension/cgroupruntime integration tests Dec 11, 2024
@rogercoll
Copy link
Contributor Author

Checking why the CI is failing 👀

@rogercoll
Copy link
Contributor Author

rogercoll commented Dec 12, 2024

@mx-psi I changed a bit the Go build approach to use it with the -run flag as well. The reasoning was because files (tests) without Go tags will run by default, meaning that when we run the integrations tests, we are also running the unit tests ones. So only adding the sudo Go build tag was also running the unit tests as a privileged user and some components were failing.

At the moment, we have the following scenarios when running the mod-integration-test Makefile target:

  • All Integration tests that do not contain "Sudo" in its function name (-skip Sudo) and the integration build tag + Unit tests → run with the default user
  • All Integration tests that contain "Sudo" in their function name and the integration build tag → run as sudo

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.

@mx-psi
Copy link
Member

mx-psi commented Dec 17, 2024

@rogercoll LGTM

I changed a bit the Go build approach to use it with the -run flag as well. The reasoning was because files (tests) without Go tags will run by default, meaning that when we run the integrations tests, we are also running the unit tests ones. So only adding the sudo Go build tag was also running the unit tests as a privileged user and some components were failing.

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.

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 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 😄

@mx-psi
Copy link
Member

mx-psi commented Dec 17, 2024

Also, can you fix the merge conflict?

@rogercoll
Copy link
Contributor Author

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.

Sure, I gathered the details here #36817

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 😄

Sounds good :), I was not aware of sudo also available on Windows. If we want to only run them on the CI at some point, we would need to modify the current go test command with [ -n "$CI" ] && $(GOTESTSUM) $(GOTESTSUM_OPT) --packages="./..." -- $(GOTEST_OPT_WITH_INTEGRATION_SUDO)

@rogercoll
Copy link
Contributor Author

Also, can you fix the merge conflict?

Fixed 🤞

@mx-psi mx-psi merged commit 2ab75f3 into open-telemetry:main Dec 18, 2024
161 checks passed
@github-actions github-actions bot added this to the next release milestone Dec 18, 2024
mterhar pushed a commit to mterhar/opentelemetry-collector-contrib that referenced this pull request Dec 19, 2024
…#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 <[email protected]>
@r0mdau
Copy link

r0mdau commented Dec 20, 2024

How do you ensure the integration test is running ?

Locally, this test is skip because I am not running in a containerized environment.

// checkCgroupSystem skips the test if is not run in a cgroupv2 system
func checkCgroupSystem(tb testing.TB) {

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

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.

[extension/cgroupruntime] Add cgroup integration tests
4 participants