[#1063] Give back what the open reserved rather than what the configuration says by then, and ask for a restart when the cache size changes - #1066
Open
vharseko wants to merge 4 commits into
Conversation
…n listeners only once it has opened EntryContainer registered itself and its two configuration managers from its constructor, and only close() takes them off again. open() caught StorageRuntimeException alone, so a ConfigException - a VLV filter or sort order which does not parse, an index type the attribute has no matching rule for - left a container nothing holds a reference to, registered on the configuration of a backend which did not start. Register the five at the end of a successful open() instead, and catch every failure there rather than the storage ones alone. Hold each index in its map before opening it: an attribute index registers its listener at the end of open() and a VLV index from its constructor, so the one being opened was not yet one close() could find - the hole the existing catch already had. RootContainer gives back what a failed open took: the entry containers it registered, its own listener and the storage, the last only when this call is what opened it. openAndRegisterEntryContainers runs inside a write the storage may replay, so it now gives up what a rolled back attempt registered before opening again; without it a write-write conflict failed the backend with ERR_ENTRY_CONTAINER_ALREADY_REGISTERED. Fixes OpenIdentityPlatform#993
…ok, and pin the give-back of the root container Review round 2 of OpenIdentityPlatform#999. PDBStorage.open() and startImport() give back what the attempt took before it failed - the cache size buildConfiguration drew from the memory quota, the listener the constructor registered on the backend configuration, and the database when the open got that far - as JDBCStorage.open already does. Every failed enable of a PDB backend drained one cache size for the life of the JVM and left a storage answering the configuration changes of a backend which is not running. The guard against a double open runs before anything is taken, and close() releases the quota once and tolerates a database the failed open registered no monitor for. RootContainer registers an entry container as soon as it has opened, before its highest entry ID is read: a container which opened has registered every listener it ever will, and only what the registry holds is given back. The rationale of the storageOpened arm is the true one: the storage's own open() threw, and what that open took is the storage's own to give back - no root container is ever opened over a storage another one holds. FailedBackendOpenTest pins the give-back loop with a second base DN, the registration of an opened container, the root container's own listener when the storage did not open, and the positive twin - one registration of each listener once a container has opened, counted per occurrence. PDBStorageTest pins the give-back, a close() which follows it, and the refusal of a double open.
…k, and pin the give-back past the database open JEStorage had the shape PDBStorage had before the previous round: the quota taken in buildConfiguration ahead of open0, the double-open guard behind it, a failed `new Environment` leaving the quota and the constructor's listener behind with nobody to close the storage, and a close() which released the quota on every call - three times on the import road, for two acquisitions. It gets the same moves: rejectIfOpen() ahead of buildConfiguration in open() and startImport(), openOrGiveBack() around open0(), the quota released once, an environment without a monitor tolerated. Both storages now give back the quota, the listener and the monitored directory ahead of the database, so that a database whose own close throws keeps nothing else. JEStorageTest, new: the three cases of PDBStorageTest over a directory the server cannot use - a locked directory is not a JE road inside a JVM, DbEnvPool shares the environment. PDBStorageTest gains the case past the database open, with the disk monitor refusing the directory: the volume, the monitor and the quota are given back. FailedBackendOpenTest: a case whose expected failure does not come closes the backend it opened after all, so the case which follows fails on its own assertion rather than on the base DN the previous one left registered.
…han what the configuration says by then, and ask for a restart when the cache size changes PDBStorage and JEStorage reserved their cache size from the memory quota by reading config in buildConfiguration and released it by reading config again in close(). applyConfigurationChange swapped config in between without touching the quota or the cache, and neither db-cache-size nor db-cache-percent was marked as needing a restart, so a cache grown from 64 MB to 128 MB while the backend ran released 128 against 64 taken at the next disable - the one an online import makes included - and the quota believed 64 MB free that the server did not have, for the life of the JVM; a shrink left the difference reserved by nobody. The running cache was the old size throughout. Both storages now keep two numbers of their own: the cache size of the configuration they opened with, and of it what the quota granted - a tryAcquire it refused, which an open at startup is not checked against, reserved nothing and used to be released all the same. close() gives back the granted size. isConfigurationChangeAcceptable admits the difference to what is held rather than to config, which a change admitted but not yet applied has already moved to the new size. applyConfigurationChange on an open storage whose cache size the change moves sets adminActionRequired and says so (NOTE_CONFIG_DB_CACHE_REQUIRES_RESTART): PersistIt cannot resize a buffer pool once the database is open, and JEStorage has never resized its environment. The two properties are marked component-restart in both configuration XMLs, as db-directory is. PDBStorageTest and JEStorageTest, six cases each: the grow and the shrink give back what was taken, the change asks for a restart and names both sizes, a change which leaves the cache alone asks for nothing, admission is against what is held, and a reservation the quota refused is not given back.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PDBStorageandJEStoragereserve their cache size from the server'sMemoryQuotawhen they open and release itwhen they close - both times by reading the configuration they hold at that moment. A change of
db-cache-sizeordb-cache-percenton a running backend swaps that configuration (applyConfigurationChangeends inconfig = cfg)without touching the quota or the cache, and neither property is marked as needing a restart. So the storage reserves
the old size and releases the new one at the next close - the disable an online
import-ldifmakes included - andthe quota drifts by the difference for the life of the JVM: a cache grown from 64 MB to 128 MB leaves the quota
believing 64 MB free that the server does not have; a shrink leaves 64 MB reserved by nobody. The running cache is
the old size throughout, and
dsconfigreports the change applied.Fixes #1063.
What changes
Both storages keep two numbers of their own instead of reading
configtwice:configuredCacheSize- the cache size of the configuration the storage opened with (for PDB, what the buffer poolwas built to);
reservedCacheSize- of it, what the quota granted.acquireMemoryis atryAcquire: refused, it reservesnothing, and the return value used to be ignored in both
buildConfigurations, so a close released a size thatwas never taken. That is reachable without any change - the server does not call
isConfigurationAcceptableforthe backends it opens at startup.
close()gives backreservedCacheSize.isConfigurationChangeAcceptableadmits the difference to what is heldrather than to
config: once a change has been admitted but not applied,computeSize(config)is already the newsize while the reservation is the old one, and a second change was admitted for a difference nobody would reserve.
applyConfigurationChangeon an open storage whose cache size the change moves setsadminActionRequiredand addsNOTE_CONFIG_DB_CACHE_REQUIRES_RESTART(630), naming the size the backend runs with and the one now configured.db-cache-sizeanddb-cache-percentare markedcomponent-restartinPDBBackendConfiguration.xmlandJEBackendConfiguration.xml, asdb-directoryis in the same files, sodsconfigsays so too.Why a restart rather than a resize
PersistIt sizes its buffer pool when the database opens and has no way to resize it (
Persistit.setConfigurationrefuses a second configuration). JE could -
je.maxMemoryandje.maxMemoryPercentare mutable throughEnvironment.setMutableConfig- butJEStoragehas never resized its environment, and it is not alone: none of theJE properties the pluggable backend left without
requires-admin-actionis applied live since OPENDJ-1719 droppedthe
setMutableConfigroad of the old backend. Restoring that road for all of them is #1068; here the twostorages take the same shape, and the quota follows the cache that actually runs.
Stacked on #999
The fix builds on the
close()of #999 (the quota given back once, ahead of the database) and onJEStorageTest,which #999 introduces; the branch sits on its head (
fd50d19dcb). Merge after #999.Tests
PDBStorageTestandJEStorageTest, six cases each, over a mockedServerContextwith a freshMemoryQuota:aCacheGrownWhileOpenIsGivenBackAsItWasTaken,aCacheShrunkWhileOpenIsGivenBackAsItWasTaken- the quota isback where it started after open, change, close;
aCacheSizeChangedWhileOpenAsksForARestart-adminActionRequired, the message id and both sizes;aChangeWhichLeavesTheCacheSizeAloneAsksForNothing- a change of another property asks for nothing, as before;aCacheSizeChangeIsAdmittedAgainstWhatTheStorageHolds- with 64 MB held and a change to 128 MB pending, 256 MB isrefused and 192 MB admitted at 129 MB free;
aReservationTheQuotaRefusedIsNotGivenBackOnClose- 32 MB free, a 64 MB cache opens, and the close leaves 32 MB.Five of the six were red on the head of #999 before the fix (the sixth pins existing behaviour): +64 MB, −64 MB,
admission of 256 MB, no admin action, 96 MB after the close of a refused reservation.
Mutants, each run against both classes:
close()releasingconfiguredCacheSizeinstead of the reserved size(the refused-reservation case red, nothing else), admission against
computeSize(config)(the admission case),setAdminActionRequireddropped (the restart case), and the old release byconfig(grow, shrink and the refusedreservation).
Regression set (one JVM per class, the fixed storages first on the classpath):
FailedBackendOpenTest,PDBTestCase,EncryptedPDBTestCase,JETestCase,EncryptedJETestCase,ReplayedConfigChangeTest,OnDiskMergeImporterTest,PersistentCompressedSchemaTest,DN2IDTest,StateTest,ID2EntryTest,ID2ChildrenCountTest,BulkCursorTest,DefaultIndexTest,ImportLDIFTestCase,RebuildIndexTestCase,VerifyIndexTestCase,BackendConfigManagerTestCase- 240 tests, 0 failures (FailedBackendOpenTestandPDBTestCasere-run after a collision on the admin port 65534 with another JVM on the machine).Not in this PR
ConfigurableEnvironment.validateDbCacheSizetakesdb-cache-sizefrom the server's quota as aprobe and never gives it back, so every open of a JE backend with an explicit size leaks one cache size. A
reservation outside the pair this PR closes; the mock quota the tests here assert on does not see it.
db-checkpointer-wakeup-interval) that are neither applied livenor marked as needing a restart.