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

Bump base memory requirements for python and go #3473

Merged

Conversation

jcreixell
Copy link
Contributor

@jcreixell jcreixell commented Nov 19, 2024

Description:

  • When auto-instrumenting applications, I have noticed that default memory limits are too tight for some languages. This leads to the following:
    • Intermitent OOMKilled events in init container when auto-instrumenting python applications. Eventually the pods are able to start.
    • OOMKilled events for sidecar containers in go applications. The pods are not able to start.
  • 64Mi seems to be enough to fix these issues. While some tweaking by users may still be necessary, the operator should work out-of-the-box for all supported languages.

Link to tracking Issue(s):
#3479

  - When auto-instrumenting applications, I have noticed that default memory
    limits are too tight for some languages. This leads to the
    following:
    - Intermitent OOMKilled events in init container when auto-instrumenting python applications.
      Eventually the pods are able to start.
    - OOMKilled events for sidecar containers in go applications. The
      pods are not able to start.
  - 64Mi seems to be enough to fix these issues. While some tweaking by users may still be necessary,
    the operator should work out-of-the-box for all supported languages.
@jcreixell jcreixell requested a review from a team as a code owner November 19, 2024 16:58
Copy link
Contributor

@iblancasa iblancasa left a comment

Choose a reason for hiding this comment

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

Please, add a changelog

@swiatekm
Copy link
Contributor

Can you reproduce the OOMKills in a kind cluster? I'm not against bumping the requirements, but I don't understand why these containers would need it. The only thing they actually do is copy some files over to a shared volume.

@jcreixell
Copy link
Contributor Author

I am able to consistently reproduce the go OOMs with both k3d and kind:

kind

Interestingly, the python OOMs only happen with k3d, not kind.

Screenshot from 2024-11-20 18-33-10

Screenshot from 2024-11-20 18-33-55

I am using a standard setup following the README. For testing, I use the rolldice applications with their manual instrumentation code stripped out.

(I will move this to an issue and link it in the changelog)

Copy link
Contributor

@swiatekm swiatekm left a comment

Choose a reason for hiding this comment

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

This looks good to me, and I'm fine merging it as-is. But I'd like to understand why our e2e tests don't catch it, and ideally modify them to do so. @jcreixell could you open a follow-up issue to track that?

@jcreixell
Copy link
Contributor Author

@swiatekm sure, there we go #3484

@swiatekm swiatekm merged commit 5481793 into open-telemetry:main Nov 21, 2024
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants