canonical / mysql-operator

Machine charm for MySQL following the operator framework
https://charmhub.io/mysql
Apache License 2.0
7 stars 10 forks source link

DPE-3706 Fix max_connections calculation and expose experimental max_connections field #429

Closed paulomach closed 2 months ago

paulomach commented 5 months ago

Issue

Max connections calculations is not considering already allocated memory for innodb_buffer_pool_size, resulting in overcommit (and OOM kill risk)

Solution

Fixes #407

codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 66.56%. Comparing base (e7c1809) to head (ae74a89). Report is 17 commits behind head on main.

:exclamation: Current head ae74a89 differs from pull request most recent head c443bd3

Please upload reports for the commit c443bd3 to get more accurate results.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #429 +/- ## ========================================== + Coverage 66.25% 66.56% +0.30% ========================================== Files 17 17 Lines 3180 3170 -10 Branches 424 419 -5 ========================================== + Hits 2107 2110 +3 + Misses 935 928 -7 + Partials 138 132 -6 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

nobuto-m commented 4 months ago

Hi all, after discussing about this here and internally, I think I should request this patch to be on hold for the time being.

The fundamental challenge is not immediately solved at this point, and the proposed patch is good as is to prevent the OOM from happening. However, it will limit the available max_connections to 25% from the current state and that may make it harder to run workload with a machine with a fair amount of memory. Until we fix the original challenge, I think it might be better to leave it as is since 1. OOM doesn't happen immediately after the service start and 2. MySQL dynamically allocates memory for actually data in innodb buffer pool size and for incoming connections. So we can keep this optimistic way of memory allocation for the time being.

In the meantime, I appreciate @paulomach for taking the initiative on this matter so far.

taurus-forever commented 3 months ago

please don't merge without my explicit approval

@delgod is it still valid blocker?