Follow on from #48384
Now that we have working ram and sanity checks, we need to make sure they work together properly.
Right now, as the server starts we are checking the backends in isolation.
Lets say we have 1GB of ram. We have two backends, that each request 768MB.
Both would pass the sanity check, as they check the free memory at the time, independent of each other. Then both start and over time would allocate up to 1.5GB potentially causing swap or worse OOM conditions.
Our backend sizing checks should check the sum of all backends and all caches together, before declaring "sane".
This will have to be done pre-backend init.
I propose:
Alter the server start up procedure such that:
start slapd load the plugins (but don't start the DB's / backend yet) trigger each backend to "attempt" the autosizing procedure based on the admin's rules. retrieve all the cache sizings validate the sum of all caches iff valid: start the backends and dbs else: stop the server with lots of big warnings, and the complete set of information related to memory and cache sizings.
attachment 0001-Ticket-48617-Server-ram-checks-work-in-isolation.patch
I think the only question I have is if we "predict" the system will OOM should be return the SLAPI_FAIL_GENERAL or just continue with the warning as something for support / users to look for in case of an outage?
Looks good to me.
Here's a short (sad) history of the cache auto-sizing... To reduce the administrators' burden, the cache auto-sizing was implemented, but it rather caused more OOM mainly by the fragmentation. We had to ask the customers to set the cache size much lower than the available size to prevent OOM. So, we don't recommend the cache auto-sizing these days nor test it with high priority. If we could overcome the fragmentation issue with jemalloc, it might be nice to advertise cache auto-sizing more.
Plus the automated test suite used to test the cache auto-sizing, but the calculation in the test was not accurate enough and it reported too many errors (most of them were false positive), and they were stopped running at one point...
Regarding the line 183, you could either 1) reduce the percentage of li_cache_autosize and retry until it becomes sane or 2) ignore cache auto-sizing and use the nsslapd-cachememsize with the warning message or 3) just exit with the error message there to ask for the administrator to fix the situation by him/herself or 4) ...? Please pick up the most reasonable solution you think of. {{{ 164 176 / autosizing dbCache and entryCache / 165 177 if (li->li_cache_autosize > 0) { 166 178 zone_pages = (li->li_cache_autosize * pages) / 100; 167 / now split it according to user prefs / 179 size_t zone_size = zone_pages * pagesize; 180 / This is how much we "might" use, lets check it's sane. / 181 issane = util_is_cachesize_sane(&zone_size); 182 if (!issane) { 183 / Do we shut down here? Or accept the reduced allocation? / 184 } }}} Other than that, you have my ack.
Replying to [comment:3 nhosoi]:
Looks good to me. Here's a short (sad) history of the cache auto-sizing... To reduce the administrators' burden, the cache auto-sizing was implemented, but it rather caused more OOM mainly by the fragmentation. We had to ask the customers to set the cache size much lower than the available size to prevent OOM. So, we don't recommend the cache auto-sizing these days nor test it with high priority. If we could overcome the fragmentation issue with jemalloc, it might be nice to advertise cache auto-sizing more. Plus the automated test suite used to test the cache auto-sizing, but the calculation in the test was not accurate enough and it reported too many errors (most of them were false positive), and they were stopped running at one point...
Mmm. That's a very interesting history. I did suspect something related to the memory fragmentation and OOM was an issue there ...
Well, at the moment, when we set zone_size, if util_is_cachesize_sane() returs !issane, it actually reduces the value of zone_size. Then a few lines later, zone_size is actually re-broken out to the be the values for the caches. So this already sets a smaller value for us to accept.
It's more a question of, do we want to allow the autosize to down-grade like that if the autosize will exceed memory? Or should we error? I'm tending to the automatic downgrade just because that's the point of the "automatic" function, is to, well, automatically make safe decisions.
attachment 0002-Ticket-48617-Server-ram-checks-work-in-isolation.patch
Added a new version of this with your comments taken into consideration, and a slight tweak of the code to make the path a bit cleaner. Tested both with and without autosizing options.
Thank you! I see. You wanted to check once at the end. A nice idea! {{{ 247 / Right, it's time to panic / 248 LDAPDebug( LDAP_DEBUG_ANY, "CRITICAL: It is highly likely your memory configuration will EXCEED your systems memory.\n", 0, 0, 0 ); }}} Now, I have one question... Do you think we could point out some suspicious config param that should be fixed with some recommendation? It might be appreciated by less experienced administrators. If not a good idea, just ignore it. My 2 yen...
No I think it's a great idea! I'll add some extra messages now.
attachment 0003-Ticket-48617-Server-ram-checks-work-in-isolation.patch
Right, now in both cases it prints an appropriate messages. For autosizing:
{{{ [12/May/2016:13:09:42.270710331 +1000] util_is_cachesize_sane - WARNING adjusted cachesize to 1098831872 [12/May/2016:13:09:42.284974977 +1000] util_is_cachesize_sane - WARNING: Cachesize not sane [12/May/2016:13:09:42.295996558 +1000] Your autosized cache values have been reduced. Likely your nsslapd-cache-autosize percentage is too high. [12/May/2016:13:09:42.307112333 +1000] This can be corrected by altering the values of nsslapd-cache-autosize, nsslapd-cache-autosize-split and nsslapd-dncachememsize [12/May/2016:13:09:42.318003029 +1000] cache autosizing. found 12202680k physical memory [12/May/2016:13:09:42.328929320 +1000] cache autosizing. found 5267380k avaliable [12/May/2016:13:09:42.343850789 +1000] cache autosizing: db cache: 3160428k, each entry cache (4 total): 526736k }}}
For manual tuned:
{{{ [12/May/2016:13:04:20.659222789 +1000] Total entry cache size: 18635292672 B; dbcache size: 4498395136 B; available memory size: 7177060352 B; [12/May/2016:13:04:20.670417329 +1000] util_is_cachesize_sane - WARNING adjusted cachesize to 1087056896 [12/May/2016:13:04:20.681391225 +1000] util_is_cachesize_sane - WARNING: Cachesize not sane [12/May/2016:13:04:20.692828832 +1000] CRITICAL: It is highly likely your memory configuration will EXCEED your systems memory. [12/May/2016:13:04:20.706138200 +1000] Total entry cache size: 18635292672 B; dbcache size: 4498395136 B; available memory size: 7177060352 B; [12/May/2016:13:04:20.721341939 +1000] This can be corrected by altering the values of nsslapd-dbcachesize, nsslapd-cachememsize and nsslapd-dncachememsize [12/May/2016:13:04:20.736040514 +1000] Failed to start database plugin ldbm database }}}
Wow... These messages are certainly better than ever... Thanks a lot to make this happen, William!!
0003-Ticket-48617-Server-ram-checks-work-in-isolation.patch​ is acked.
Ticket has been cloned to Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1335618
commit 9eee3a8 Total 8 (delta 6), reused 0 (delta 0) To ssh://git.fedorahosted.org/git/389/ds.git 77e6044..9eee3a8 master -> master
Thanks for the review and the feedback!
commit 9eee3a8 introduced a covscan failure.
As covscan reported, issane assigned at the line 238 (autosizing importcache) is overwritten at the line 253. {{{ 32 ldbm_back_start( Slapi_PBlock *pb )
230 / autosizing importCache / 231 if (li->li_import_cache_autosize > 0) {
237 size_t import_size = import_pages * pagesize; 238 issane = util_is_cachesize_sane(&import_size);
252 size_t total_size = total_cache_size + (PRUint64)li->li_dbcachesize; 253 issane = util_is_cachesize_sane(&total_size); }}} Also, could you please declare a variable at the top of a block instead of doing so in the middle as being done at the line 237 and 252? Thanks!
Another failure: {{{ 1. Defect type: CLANG_WARNING 1. 389-ds-base-1.3.5.4/ldap/servers/slapd/back-ldbm/start.c:267:5: warning: Function call argument is an uninitialized value
}}} Uninitialized msg could be passed to LDAPDebug(LDAP_DEBUG_ANY, msg, 0,0,0). Better to initialize it with an empty string "" when it is declared. {{{ 41 char msg; / This will be set by one of the two cache sizing paths below. */ }}}
The third one. It may not be so relevant, but covscan thinks availpages is not initialized and the garbage value could reach the error logging at the line 262. {{{ 2. Defect type: CLANG_WARNING 1. 389-ds-base-1.3.5.4/ldap/servers/slapd/back-ldbm/start.c:262:84: warning: The left operand of '*' is a garbage value
}}} Maybe, if any of the configuration values are invalid, we could return SLAPI_FAIL_GENERAL next to the line 121? {{{ 115 if ((li->li_cache_autosize > 100) || 116 (li->li_cache_autosize_split > 100) || 117 (li->li_import_cache_autosize > 100) || 118 ((li->li_cache_autosize > 0) && (li->li_import_cache_autosize > 0) && 119 (li->li_cache_autosize + li->li_import_cache_autosize > 100))) { 120 LDAPDebug( LDAP_DEBUG_ANY, "cache autosizing: bad settings, " 121 "value or sum of values can not larger than 100.\n", 0, 0, 0 ); 122 } else { 123 if (util_info_sys_pages(&pagesize, &pages, &procpages, &availpages) != 0) { }}}
Fix the coverity scan issues. 0001-Ticket-48617-Coverity-fixes.patch
I have run covscan on this, and it is happy.
commit d4eb7d0 Total 7 (delta 5), reused 0 (delta 0) To ssh://git.fedorahosted.org/git/389/ds.git d975431..d4eb7d0 master -> master
Metadata Update from @firstyear: - Issue assigned to firstyear - Issue set to the milestone: 1.3.5.5
389-ds-base is moving from Pagure to Github. This means that new issues and pull requests will be accepted only in 389-ds-base's github repository.
This issue has been cloned to Github and is available here: - https://github.com/389ds/389-ds-base/issues/1785
If you want to receive further updates on the issue, please navigate to the github issue and click on subscribe button.
subscribe
Thank you for understanding. We apologize for all inconvenience.
Metadata Update from @spichugi: - Issue close_status updated to: wontfix (was: Fixed)
Login to comment on this ticket.