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

Improve Kubeappsapis default config #3699

Merged
merged 5 commits into from
Nov 1, 2021

Conversation

antgamdia
Copy link
Contributor

@antgamdia antgamdia commented Oct 29, 2021

Description of the change

As outlined in #3567 (comment), this PR addresses one of the proposals to mitigate the effect of some memory peeks we don't control (that is, instant allocated memory prior to a GC pass). It mostly limits the number of concurrent operations. For instance, with 256MB only ~3-4 upgrades operations can be performed, due to we (well, the Helm library in this case) need to parse the data, validate it and other stuff.

Also, this PR increases the number of default replicas (as in the original Kubeops pod) from 1 to 2. This way, in case of an OOMKilled event, the user won't notice any error (unless they were doing many concurrent actions and they continue doing so).

Finally, it decreases the temporary memory increase we did for merging the deps PR. The value decreases from 512 to 256 MB.

Benefits

This PR tries to increase the velocity the GC is called (it has a "price", which is a slightly higher CPU consumption, but since we don't have any CPU-intensive process, I think the tradeoff is worth it. Nevertheless. the chosen value GOGC is rather arbitrary so a more thoughtful analysis would be required in order to determine the value that maximizes the overall performance. Note this is not a trivial thing and ever there are some commercial solutions for doing so (example).

Possible drawbacks

Even though this PR does its best to improve the overall performance and error rate, some changes in the code are still required. Also, note these changes just mitigates the problem, but do not solve it completely.

Applicable issues

Additional information

N/A

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
chart/kubeapps/Chart.yaml Outdated Show resolved Hide resolved
@@ -1626,7 +1626,7 @@ kubeappsapis:
pullSecrets: []
## @param kubeappsapis.replicaCount Number of frontend replicas to deploy
##
replicaCount: 1
replicaCount: 2
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, this should have been updated to 2 once we started using the kubeapps-apis in released code. (Or even better, I should have made it 2 from the start even when we weren't using it since it wasn't deployed in that case, zero cost without having to remember).

antgamdia and others added 2 commits November 1, 2021 10:25
@antgamdia antgamdia merged commit 263ba1e into vmware-tanzu:master Nov 1, 2021
@antgamdia antgamdia deleted the improveDefaultAPisConfig branch November 1, 2021 10:46
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.

Memory issues in kubeappsapis/helm-related logic: getChart fetches the whole repo each time it's invoked
2 participants