Skip to content

[chore] Add cgroupruntime extension integration tests #39017

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

Merged
merged 24 commits into from
Apr 7, 2025

Conversation

rogercoll
Copy link
Contributor

@rogercoll rogercoll commented Mar 27, 2025

Description

Reverts #37224 with a few additional changes:

  • Decouple sudo integration tests from non-privileged ones from the makefile. -> prevent sudo promp when running integration tests
  • Adds a specific intengration-sudo-tests check in the CI that only targets the cgroupruntime extension as being the only component that currently requires this setup.
  • Sets darwin and windows as unsupported platforms (it requires cgroup file system).

Link to tracking issue

Fixes

Testing

Adds the integration tests required for is addition into the contrib binary.

Documentation

@rogercoll rogercoll force-pushed the add_cgroupruntime_integration branch from 57c1f8e to a28617f Compare March 27, 2025 16:45
@rogercoll rogercoll changed the title [chore] Add cgroupruntime integration tests [chore] Add cgroupruntime extension integration tests Mar 28, 2025
@rogercoll rogercoll marked this pull request as ready for review March 28, 2025 08:30
@rogercoll rogercoll requested a review from a team as a code owner March 28, 2025 08:30
@rogercoll
Copy link
Contributor Author

Looks like the integration test is flaky: Error: Process completed with exit code 143. https://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/14124954048/job/39571961728?pr=39017

My guess is that the tests modifies the cpu and memory limits and as GitHub-hosted runners have strict resource limits it kills the process/test. Checking if we could reduce the resource changes limits.

@rogercoll
Copy link
Contributor Author

Memory usage of the tests:

=== RUN   TestECSCgroupV2SudoIntegration/90%_the_max_cgroup_memory_and_4_GOMAXPROCS_w/_4096_container_cpu_16_task_cpu
Memory Usage: Alloc = 1 MiB, Sys = 7 MiB, HeapInUse = 2 MiB, HeapAlloc = 1 MiB, HeapReleased = 1 MiB
Total RSS Memory: 46.30 MB
=== RUN   TestECSCgroupV2SudoIntegration/70%_of_the_max_cgroup_memory_and_1_GOMAXPROCS_w/_2048_container_cpu_2_task_cpu
Memory Usage: Alloc = 1 MiB, Sys = 12 MiB, HeapInUse = 2 MiB, HeapAlloc = 1 MiB, HeapReleased = 4 MiB
Total RSS Memory: 49.23 MB
=== RUN   TestECSCgroupV2SudoIntegration/70%_of_the_max_cgroup_memory_and_1_GOMAXPROCS_w/_1024_container_cpu_4_task_cpu
Memory Usage: Alloc = 1 MiB, Sys = 12 MiB, HeapInUse = 2 MiB, HeapAlloc = 1 MiB, HeapReleased = 4 MiB
Total RSS Memory: 49.85 MB
=== RUN   TestECSCgroupV2SudoIntegration/80%_of_the_max_cgroup_memory_and_4_GOMAXPROCS_w/_4096_container_cpu_0_task_cpu
Memory Usage: Alloc = 1 MiB, Sys = 12 MiB, HeapInUse = 2 MiB, HeapAlloc = 1 MiB, HeapReleased = 4 MiB
Total RSS Memory: 50.48 MB

The integration tests were setting the cgroup max memory to the 10% of 128 Mb, thus in some runs triggering the OOM killer -> 134 exit code.

Increasing the cgroup max memory setting from 128Mb to 4GB (should not be an issue if the system has less total memory) seems to fix the issue.

@@ -7,6 +7,7 @@ status:
distributions: [contrib]
codeowners:
active: [mx-psi, rogercoll]
unsupported_platforms: [darwin, windows]
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not related to the test you're adding?

@atoulme atoulme merged commit 3f160ea into open-telemetry:main Apr 7, 2025
171 checks passed
@github-actions github-actions bot added this to the next release milestone Apr 7, 2025
dmathieu pushed a commit to dmathieu/opentelemetry-collector-contrib that referenced this pull request Apr 8, 2025
…#39017)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

Reverts
open-telemetry#37224
with a few additional changes:

- Decouple `sudo` integration tests from non-privileged ones from the
makefile. -> **prevent sudo promp when running integration tests**
- Adds a specific `intengration-sudo-tests` check in the CI that only
targets the cgroupruntime extension as being the only component that
currently requires this setup.
- Sets darwin and windows as unsupported platforms (it requires cgroup
file system).

<!-- 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
Adds the integration tests required for is addition into the contrib
binary.

<!--Describe the documentation added.-->
#### Documentation

<!--Please delete paragraphs that you did not use before submitting.-->

---------

Co-authored-by: Antoine Toulme <[email protected]>
Co-authored-by: Antoine Toulme <[email protected]>
LucianoGiannotti pushed a commit to LucianoGiannotti/opentelemetry-collector-contrib that referenced this pull request Apr 9, 2025
…#39017)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

Reverts
open-telemetry#37224
with a few additional changes:

- Decouple `sudo` integration tests from non-privileged ones from the
makefile. -> **prevent sudo promp when running integration tests**
- Adds a specific `intengration-sudo-tests` check in the CI that only
targets the cgroupruntime extension as being the only component that
currently requires this setup.
- Sets darwin and windows as unsupported platforms (it requires cgroup
file system).

<!-- 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
Adds the integration tests required for is addition into the contrib
binary.

<!--Describe the documentation added.-->
#### Documentation

<!--Please delete paragraphs that you did not use before submitting.-->

---------

Co-authored-by: Antoine Toulme <[email protected]>
Co-authored-by: Antoine Toulme <[email protected]>
Fiery-Fenix pushed a commit to Fiery-Fenix/opentelemetry-collector-contrib that referenced this pull request Apr 24, 2025
…#39017)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

Reverts
open-telemetry#37224
with a few additional changes:

- Decouple `sudo` integration tests from non-privileged ones from the
makefile. -> **prevent sudo promp when running integration tests**
- Adds a specific `intengration-sudo-tests` check in the CI that only
targets the cgroupruntime extension as being the only component that
currently requires this setup.
- Sets darwin and windows as unsupported platforms (it requires cgroup
file system).

<!-- 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
Adds the integration tests required for is addition into the contrib
binary.

<!--Describe the documentation added.-->
#### Documentation

<!--Please delete paragraphs that you did not use before submitting.-->

---------

Co-authored-by: Antoine Toulme <[email protected]>
Co-authored-by: Antoine Toulme <[email protected]>
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.

3 participants