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

[Grafana] Set GOMAXPROCS and GOMEMLIMIT environment variables based on container resources #3138

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jnoordsij
Copy link

Set GOMAXPROCS and GOMEMLIMIT environment variables based on container resources. This should reduce potential CPU throttling and OOMKills on containers.

The resourceFieldRef is a very specific Kubernetes directive that is created specifically for passing resource-related values, which rounds up the CPU value to the nearest whole number (e.g. 250m to 1) and passes the memory as a numeric value; so 64Mi would result in the environment variable being set to 67108864. This by design makes it completely compatible with Go's API.

An example is documented within Kubernetes documentation itself: https://kubernetes.io/docs/tasks/inject-data-application/environment-variable-expose-pod-information/#use-container-fields-as-values-for-environment-variables.

Inspired by traefik/traefik-helm-chart#1029.

Implementation notes:

  • This was only added to the "main" Grafana container, given it's only relevant for Go-based applications
  • This idea can probably be applied to other charts within this repository; I currently have no intention of following-up with this myself, but anyone coming across this, feel free to do so!

@CLAassistant
Copy link

CLAassistant commented May 18, 2024

CLA assistant check
All committers have signed the CLA.

@jkroepke
Copy link
Contributor

I would not recommend to set GOMAXPROCS like that. It's counterproductive.

GOMAXPROCS sets the maximum number of CPUs that can be executing simultaneously

However, if you give grafana cpu=2 on a 16 core machine, then the Grafana pod will still get 16 cores assigned, but the pod is only able to use 1/8 of each CPU core.

With GOMAXPROCS, grafana may only use 2 cores of them and would not longer benefit form all assigned resources.

See also a 7 years old discussion that promethues-operator: prometheus-operator/prometheus-operator#501

@jnoordsij
Copy link
Author

I'm all fine with leaving GOMAXPROCS out, even though I still think it's beneficial from the things I've seen/read. From what I've seen, as soon as you actually use Kubernetes limits, you will experience some degree of throttling in practice and by setting GOMAXPROCS accordingly this is somewhat mitigated. The https://github.com/uber-go/automaxprocs is a widely-used tool that does a very similar thing, which was also added as opt-in to Prometheus operator.

However, I do agree all results and opinions I've seen are mixed and this is potentially not a better-for-everyone solution. If removing this part (or making it opt-in/opt-out rather than mandatory) is considered preferable, just let me know.

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.

None yet

3 participants