feat(env): Add automatic memory limit handling

[dongjiang1989]

Description

Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request.
If it fixes a bug or resolves a feature request, be sure to link to that issue.

add auto GOMEMLIMIT setting

Type of change

What type of changes does your code introduce to the Prometheus operator? Put an x in the box that apply.

• CHANGE (fix or feature that would cause existing functionality to not work as expected)
• FEATURE (non-breaking change which adds functionality)
• BUGFIX (non-breaking change which fixes an issue)
• ENHANCEMENT (non-breaking change which improves existing functionality)
• NONE (if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)

Verification

Please check the Prometheus-Operator testing guidelines for recommendations about automated tests.

Changelog entry

Please put a one-line changelog entry below. This will be copied to the changelog file during the release process.

add auto  GOMEMLIMIT setting

[nicolastakashi]

LGTM 🥳
Just a simple question, on the PR about MAXPROC I think you also added to config reloader, should we do the same?

[dongjiang1989]

LGTM 🥳 Just a simple question, on the PR about MAXPROC I think you also added to config reloader, should we do the same?

Thanks. Done added.

[simonpasquier]

Unlike the gomaxprocs change, I wonder if it shouldn't be an opt-in feature? My rationale is that GOMAXPROCS is always set to "some" value while GOMEMLIMIT is unset by default (e.g. math.MaxInt64).

Also should we really consider the system memory if no k8s limits are set? More food for thoughts from https://www.ardanlabs.com/blog/2024/02/kubernetes-memory-limits-go.html:

If you’re not going to use K8s memory limits, then don’t do anything with GOMEMLIMIT. The Go runtime is really good at finding the sweet spot for your memory requirements.

[simonpasquier]

To follow-up with my last comment, I'd suggest that we treat <= 0 values as "don't set GOMEMLIMIT" and > 1 values as 1.
We can set the default to 0, thus making it an opt-in feature for now (we can still change it later). Also I'd only take into account the cgroup memory limit in the calculation and I would highlight in the arg description that you have to set memory limits to benefit from the functionality. What do you think?

[dongjiang1989]

To follow-up with my last comment, I'd suggest that we treat <= 0 values as "don't set GOMEMLIMIT" and > 1 values as 1. We can set the default to 0, thus making it an opt-in feature for now (we can still change it later). Also I'd only take into account the cgroup memory limit in the calculation and I would highlight in the arg description that you have to set memory limits to benefit from the functionality. What do you think?

Thank. Got it. @simonpasquier , PTAL

[simonpasquier]

Thanks! Can you make the same change for the admission webhook too? I'd also advise to factorize the argument parsing in internal/goruntime to avoid duplication.

[dongjiang1989]

Apply the same modification to the admission webhook as was done. Setting auto-gomemlimit-ratio to 0.

[simonpasquier]

pkg/operator/feature_gates*.go aren't need anymore, right? Also the default isn't 0 for all binaries.

[ArthurSens]

Sorry for all the bumps here @dongjiang1989! After all the discussions on the PR, I think we're aligned on not using a feature-gate at all, but just disable the feature if -auto-gomemlimit-ratio is not set.

This means that we'd need to revert all changes made that are related to feature-gates. Thanks for the patience 🙂

[dongjiang1989]

Sorry for all the bumps here @dongjiang1989! After all the discussions on the PR, I think we're aligned on not using a feature-gate at all, but just disable the feature if -auto-gomemlimit-ratio is not set.

This means that we'd need to revert all changes made that are related to feature-gates. Thanks for the patience 🙂

Thanks. Got it. Please re-check. @ArthurSens

[simonpasquier]

thanks!