feat(podtemplate): calculate GOMEMLIMIT at 90% of memory limit to reduce OOM

[shlomitubul]

What does this PR do?

chart current pod template injects
GOMEMLIMIT = pod memory limits, it leaves no headroom for Go GC to run before the process hits the cgroup ceiling, leading to OOM kills. In my tests, I show great improvement (in terms of oom events and steady memory usage) with lower values, like 90%.

Motivation

Multiple OOMs that were gone once we changed GOMEMLIMIT value

More

• Yes, I updated the tests accordingly
• Yes, I updated the schema accordingly
• Yes, I ran make test and all the tests passed

[jnoordsij]

Hey there!

Thanks a lot for this contribution. I'm personally a great fan of the performance and stability improvements that setting GOMEMLIMIT introduces, which is one of the reasons I've submitted various PRs elsewhere to add this functionality to Helm charts.

As such, I think it's great to allow more flexibility on this from the chart side. Although it does come with some maintenance burden and slightly complicated helpers, I still consider this beneficial to improve a lot of usecases.

However, I do wonder a bit what would be the most suitable 'default' here to pick. The current implementation is of course the simplest, but as you've clearly seen in your use-case, it may not be the best choice for everyone. But I think that using 80% will effectively be the same: it will be suitable to some use-cases, but might also impact some use-cases which are currently doing fine in a negative way. Given that decreasing the limit may result in more gc runs, which may result in more CPU consumption and thus eventually potential slower response times, I think it is essential that we pick a number that is both generic enough, yet still customisable for people aiming to tweak their own setup.

For the choice of default value, in cert-manager/cert-manager#6977 (comment) I was provided with some great feedback on it. Based on those references, with the (indirectly referenced) Go documentation itself being the most relevant in my opinion, I would suggest that probably a value between 90 and 95 would be more accurate as 'default'.

I think a default deviating from the recommended 90-95% or the simple current 100% is something we should only do if we actually have some very clear benchmarks for a representative use-case that suggests another value is more suitable.

So in that regard I would suggest:

• allowing tuning as @darkweaver87 suggested is essential to introduce this change;
• using a default value of 90% to 95% is more suitable based on current knowledge.

[shlomitubul]

Hey @jnoordsij, I appreciate your comment and review. I applied @darkweaver87 and your suggestion

[darkweaver87]

LGTM

[mloiseleur]

@shlomitubul You need to update the schema. See make schema

[shlomitubul]

@shlomitubul You need to update the schema. See make schema

@mloiseleur done

[mloiseleur]

@shlomitubul And also "make docs", to update VALUES.md

[jnoordsij]

LGTM (after running make docs)