[#1068] Apply what a running backend takes of a configuration change, and ask for a restart for what it does not

[vharseko]

Fixes #1068. Stacked on #1066 (cba9d511b5, on master 0e039c6473): the same lines of both applyConfigurationChange, both XMLs and the ordinal after its 630; JEStorageTest exists only from #999 on. Merge after #1066.

What was wrong

Nine properties of the JE and PDB backends were neither applied to a running backend nor marked as requiring a restart, so dsconfig reported a change of them applied and the backend ran on unchanged until it was next opened. JEStorage.applyConfigurationChange handled db-directory, its permissions and the disk thresholds and left the environment - configured once, at the open - as it was; nothing in the server called Environment.setMutableConfig. The same went for a native property changed through je-property, mutable or not (not in the issue's table, same cause).

Two things the issue had wrong, found on the way:

• a property marked requires-admin-action in the XML reaches the reference documentation and property help alone - dsconfig prints nothing about it at set time, and a change result without adminActionRequired carries nothing to the error log either. So "as db-log-file-max already does" was not true of the change result: the marking is all those properties had. The old RootContainer reported every changed immutable parameter in the change result, which the server logs as a warning (WARN 647); that shape is restored for all of them.
• db-txn-write-no-sync is on by default, so dsconfig set-backend-prop --set db-txn-no-sync:true on a JE backend yields a durability which sets both flags. Nothing checked that at change time: the change was admitted and written, and the backend failed to open on ERR_CONFIG_JEB_DURABILITY_CONFLICT at its next restart. (aChangeWhichLeavesTheCacheSizeAloneAsksForNothing of [#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 used exactly that as its "unrelated change"; it now sets the write flag off as well.)

What this does

JE - applyToEnvironment builds the environment configuration the changed configuration describes (ConfigurableEnvironment.toEnvironmentConfig) and hands it to Environment.setMutableConfig, which takes of it what JE accepts while it runs: db-cleaner-min-utilization, db-run-cleaner, db-evictor-core-threads, db-evictor-max-threads, db-evictor-keep-alive, db-num-cleaner-threads, the durability (db-txn-no-sync / db-txn-write-no-sync, every way - a configuration which sets neither now sets COMMIT_SYNC explicitly, since JE leaves the durability an environment has in place when handed none) and a mutable je-property - removed as well as set: the environment keeps the value it runs with of a parameter it is not handed, so a mutable parameter the configuration no longer sets is handed JE's default explicitly, and one whose default JE does not take as a value (je.cleaner.readSize, whose 0 stands for "computed at the open") asks for a restart with 631 instead. All but the cache: je.maxMemory / je.maxMemoryPercent are mutable too, but the cache stays with the memory reserved for it until the restart #1063 asks for, so the change hands the environment its current values back. Every immutable JE parameter whose value differs from the running environment's is reported with the new NOTE_CONFIG_DB_PROPERTY_REQUIRES_RESTART (631), naming the property as dsconfig knows it (or the JE property name for je-property), the value the environment runs with and the one configured. An import's environment is left alone - it runs on a configuration of its own and the backend opens again on the changed one once the import is over; held to the import's configuration, every property the import sets differently would ask for a restart.

A change which moves db-directory as well is still applied and reported: both storages returned on any message after the permission checks, and the note of the moved directory is one - they now return on the result code an error sets.

toEnvironmentConfig is the build alone - no cache size probe against the memory quota (#1067), no level set on the JE loggers (that moves to parseConfigEntry, the open's road) - so that a change can be checked against it: isConfigurationChangeAcceptable and isConfigurationAcceptable now refuse a conflicting durability and a native property JE does not know before the change is written.

PDB - db-checkpointer-wakeup-interval is set on the PersistIt configuration at the open alone and Persistit.setConfiguration refuses once one is set, so a change of it reports 631 against the interval the database opened with (db.getConfiguration().getCheckpointInterval(), no new field), and the property is marked component-restart in PDBBackendConfiguration.xml. je-property's definition says which of its changes wait for a restart.

Left as they are: db-logging-level and db-logging-file-handler-on keep their component-restart marking. The JUL level is set by the open alone, as before; the file handler's level (je.env.fileLoggingLevel) is mutable in JE and so follows a change from now on - the marking is conservative about it, not wrong.

Tests

JEStorageTest: the six mapped properties reach env.getMutableConfig(); the durability follows the change every way; a mutable je-property is applied and an immutable one asks for a restart with 631; a removed mutable one goes back to JE's default, or asks for a restart where JE refuses its default; a change which moves the directory is still applied; a change of db-cache-percent leaves the live cache alone; db-log-file-max asks for a restart with 631; a change while open leaves the cache where the open reserved it (the #1063 interplay); a change during an import leaves the import's environment alone; a change while closed touches nothing; a durability which sets both flags and an unknown native property are refused by both acceptability checks. PDBStorageTest: a changed interval asks for a restart with 631 and the database keeps its own, held against the database across two changes; a change which moves the directory still reports the interval; a change while closed asks for nothing.

Verified locally: the issue reproduced on master, on the #999 head and on the #1066 head with a direct TestNG repro of every row (JE 7/7 red, PDB 1/1 red, db-txn-no-sync on PDB green as a control); the new tests are red without the fix (5 JE + 1 PDB) and green with it; mutants (no setMutableConfig, cache not pinned, immutables unreported, import's environment held to the configuration, PDB silent) each red on their own case; regression set of 18 classes / 240 tests (FailedBackendOpenTest, PDB/JE TestCase + Encrypted, ReplayedConfigChangeTest, OnDiskMergeImporterTest, the pluggable tree tests, ImportLDIF/RebuildIndex/VerifyIndex, BackendConfigManagerTestCase) green.

[vharseko]

Rebased onto the restacked #1066 (725ff9af7d, itself on master now that #999 is merged); the commit is unchanged - git range-diff =. Head is 4bc401fec2; the first line of the description names the new base.

Re-run green on the rebased head: PDBStorageTest (22), JEStorageTest (17), FailedBackendOpenTest (8), JETestCase / PDBTestCase (35 each).

[vharseko]

Restacked on the new head of #1066 (64f3cffd00, itself rebased onto master now that #994 is merged): 4bc401fec2 → 4ea1d18893. No conflict of its own — the same commit over the moved base; the conflict #994 caused was in #1066's tail of backend.properties, and 630-631 now follow #994's 624-627. JEStorageTest and PDBStorageTest on the new head: 39/39 green. Description updated (base SHA).

[vharseko]

Restacked on the new head of #1066 (bd9d9d133f, itself rebased onto master 7cebc65e3c): 4ea1d18893 → d2da8dd0e1. No conflict of its own - the same commit over the moved base, and range-diff is clean apart from the context of 630-631, which now follow #998's 628-629. Both ordinals are unchanged.

No change of its own to re-test either; test-compile of the reactor on the new head is green, and CI runs on it. Description updated (base SHA).

[maximthomas]

praise: The change goes where #1068 says the gap is and reports through the channel that reaches the log.

• JEStorage.applyToEnvironment (JEStorage.java:1441-1465) is the first Environment.setMutableConfig call in src/main, and the six mapped properties plus the durability every way reach the running environment (aChangeOfWhatJETakesWhileItRunsReachesTheEnvironment, aChangeOfTheDurabilityReachesTheEnvironmentEveryWay).
• Every differing immutable parameter goes into the change result with NOTE_CONFIG_DB_PROPERTY_REQUIRES_RESTART (631) and adminActionRequired, not only into the XML marking, and the both-flags durability is now refused by isConfigurationChangeAcceptable (checkEnvironmentConfiguration, JEStorage.java:1337) instead of failing the next open.
• Green on CI at this head: JEStorageTest 17/17 and PDBStorageTest 22/22 on build-maven (ubuntu-latest, 11).

---

issue (blocking): Removing a mutable je-property leaves the running environment on the old value, and the change result reports it applied.

opendj-server-legacy/src/main/java/org/opends/server/backends/jeb/JEStorage.java:1449, :1465; opendj-maven-plugin/src/main/resources/config/xml/org/forgerock/opendj/server/config/JEBackendConfiguration.xml:733

applyToEnvironment builds next from cfg and hands it to env.setMutableConfig(next), and the loop skips every mutable parameter. In JE 18.3.12, EnvironmentImpl.doSetMutableConfig clones the running config and EnvironmentMutableConfig.copyMutablePropsTo copies only the names next sets explicitly (its Properties has no defaults). So after dsconfig set-backend-prop --remove je-property:je.cleaner.minAge=5, next no longer sets je.cleaner.minAge, and the environment runs with 5 until the next open. The change result carries no message and no admin action. That is the "reported as applied, runs on unchanged" outcome #1068 removes, on a road the description claims ("a mutable je-property is applied") and the new je-property synopsis documents ("... and the change result says so"). Replacing a value is applied, and removing an immutable one is reported: getConfigParam resolves an unset name to JE's default, so the diff sees it. Only the mutable removal is dropped. Mapped properties and durability are always set by toEnvironmentConfig and are not affected.

    for (ConfigParam param : new TreeMap<>(EnvironmentParams.SUPPORTED_PARAMS).values())
    {
      // Replication parameters are not set through an environment configuration; a multi-value
      // parameter is not read as one value. Neither is set by this storage.
      if (param.isForReplication() || param.isMultiValueParam())
      {
        continue;
      }
      final String runningValue = running.getConfigParam(param.getName());
      final String nextValue = next.getConfigParam(param.getName());
      if (Objects.equals(runningValue, nextValue))
      {
        continue;
      }
      if (param.isMutable())
      {
        // setMutableConfig copies only what the configuration handed to it sets: a parameter the
        // configuration no longer sets goes back to JE's default rather than keeping its value.
        next.setConfigParam(param.getName(), nextValue);
      }
      else
      {
        ccr.setAdminActionRequired(true);
        ccr.addMessage(NOTE_CONFIG_DB_PROPERTY_REQUIRES_RESTART.get(
            ConfigurableEnvironment.configuredNameOf(param.getName()), cfg.getBackendId(), runningValue, nextValue));
      }
    }

Pin (JEStorageTest, red at this head): apply a cfg whose getJEProperty() is new TreeSet<>(Arrays.asList(CLEANER_MIN_AGE + "=5")), assert env.getMutableConfig().getConfigParam(CLEANER_MIN_AGE) is "5", then apply createBackendCfg() and assert getMessages() is empty and the parameter is back to the value read before the first change.

---

issue (non-blocking): After an open the quota refused, every modify of the backend entry is refused. This is #1066's blocking item, carried here by the stacked commit.

opendj-server-legacy/src/main/java/org/opends/server/backends/jeb/JEStorage.java:1289, opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java:1557

(newSize <= reservedCacheSize || quota.isMemoryAvailable(newSize - reservedCacheSize)) with reservedCacheSize == 0 refuses a modify that leaves the cache alone. That includes the db-checkpointer-wakeup-interval change this PR reports on, and TaskUtils.disableBackend for an online import, rebuild or restore. It comes from bd9d9d1 (#1066) and d2da8dd leaves it alone; the new checks only follow the &&. It is blocking on #1066, not twice. Rebasing onto #1066's fix clears it here.

---

issue (non-blocking): A modify that also moves db-directory returns before applyToEnvironment and the checkpoint arm run.

opendj-server-legacy/src/main/java/org/opends/server/backends/jeb/JEStorage.java:1393, :1416; opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java:1639, :1660

The permissions block is entered whenever the directory differs, and if (!ccr.getMessages().isEmpty()) return ccr; then returns because NOTE_CONFIG_DB_DIR_REQUIRES_RESTART is already in the result, not because of an error. A modify of db-directory plus db-txn-no-sync, a je-property, or the PDB interval therefore gets the directory note only: the durability is not applied, no 631 names the immutable change, and config = cfg is skipped, so every later modify before the restart takes the same return. The return predates this PR, but the new application and reporting sit below it. The directory note already asks for a restart, which limits the harm. StorageUtils.addErrorMessage sets the result code, so test that instead of whether the result has messages, in both storages (with import org.forgerock.opendj.ldap.ResultCode;):

        if (ccr.getResultCode() != ResultCode.SUCCESS)
        {
          return ccr;
        }

---

suggestion (non-blocking): No case changes db-cache-percent while the storage is open, so nothing pins the MAX_MEMORY_PERCENT copy.

opendj-server-legacy/src/main/java/org/opends/server/backends/jeb/JEStorage.java:1463; opendj-server-legacy/src/test/java/org/opends/server/backends/jeb/JEStorageTest.java:492, :655

Every test cfg stubs getDBCachePercent() to 20, and the one cache case (aChangeWhileOpenLeavesTheCacheWhereTheOpenReservedIt) changes db-cache-size on a SMALL_CACHE environment. The copy is therefore a no-op in every case, and deleting :1463 survives the file. On a percent-sized backend (db-cache-size 0, the default), dropping the copy would let a percent change resize the live cache while the quota still holds the open's reservation.

  @Test
  public void aCachePercentChangedWhileOpenLeavesTheCacheWhereTheOpenReservedIt() throws Exception
  {
    final Environment env = environmentOf(storage);
    final long cacheAtOpen = env.getMutableConfig().getCacheSize();
    final JEBackendCfg cfg = createBackendCfg();
    when(cfg.getDBCachePercent()).thenReturn(30);

    assertThat(storage.applyConfigurationChange(cfg).adminActionRequired()).isTrue();
    assertThat(env.getMutableConfig().getCacheSize()).isEqualTo(cacheAtOpen);
  }

Pin: red with :1463 deleted.

---

suggestion (non-blocking): No test pins that the PDB checkpoint arm compares against the interval the database runs with rather than against the previous configuration.

opendj-server-legacy/src/test/java/org/opends/server/backends/pdb/PDBStorageTest.java:716; opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java:1660

Both interval cases make one change from the open state, where config's interval equals the database's. The mutant cfg.getDBCheckpointerWakeupInterval() != config.getDBCheckpointerWakeupInterval(), with the from-value read off config, gives the same text on the open case and stays silent on the closed one. Under it, a pending restart stops being reported after an unrelated change. Append to aCheckpointIntervalChangedWhileOpenAsksForARestart:

    // a later change which leaves the interval where the first one put it still asks for the restart
    assertThat(storage.applyConfigurationChange(cfg).adminActionRequired()).isTrue();
    // one which puts it back to what the database runs asks for nothing
    assertThat(storage.applyConfigurationChange(createBackendCfg()).getMessages()).isEmpty();

Pin: both lines go red under the mutant.

[vharseko]

Round head: 871db80bb8, one commit on top of the PR commit. I first restacked the PR commit onto #1066's round head cba9d511b5 (on master 0e039c6473): d2da8dd0e1 → 01e598fe5a. There were two mechanical conflicts, backend.properties (630's new text, then 631) and the admission line of JEStorage.isConfigurationChangeAcceptable (#1066's new rule, then checkEnvironmentConfiguration). JEStorageTest against #1065's imports and constants is resolved as a union. Two of #1066's new JE cases, aCacheSizedByPercentAsksForARestartOnlyWhenThePercentChanges and aChangeWhichLeavesTheCacheSizeAloneIsAdmittedAfterARefusedReservation, set db-txn-no-sync alone. With db-txn-write-no-sync on by default, this PR refuses that durability, so both now set the write flag off as well, the same fix aChangeWhichLeavesTheCacheSizeAloneAsksForNothing already had. The commit message is unchanged.

Blocking: a removed mutable je-property stays in the environment. Confirmed in JE 18.3.12: doSetMutableConfig clones the running configuration and copyMutablePropsTo copies only what next sets. I did not take your snippet as is. It hands JE's default to setConfigParam, and two of the 81 mutable non-replication parameters refuse their own default:

• je.cleaner.readSize has a default of "0", which JE's Cleaner reads as "computed at the open", but setConfigParam refuses anything below 128. Under the snippet, --remove je-property:je.cleaner.readSize=16384 makes applyConfigurationChange fail with an IllegalArgumentException after the change is already written.
• je.txn.durability has a default of null (NPE). It is not reachable today, since toEnvironmentConfig always sets the durability.

So applyToEnvironment now skips a mutable parameter that next sets (isConfigParamSet), and hands JE's default for one it no longer sets through resetsToDefault. Where JE refuses the default, it reports 631 and asks for a restart, as for an immutable parameter.

• Your pin: aRemovedNativePropertyGoesBackToJEsDefault (je.cleaner.minAge=5, then removed: no message, and the parameter is back to its value at the open).
• The fallback: aRemovedNativePropertyJECannotPutBackWhileItRunsAsksForARestart (je.cleaner.readSize=16384, then removed: SUCCESS, 631 16384 → 0, and the environment keeps 16384).

Non-blocking: every modify refused after a refused open. This is fixed by #1066's round and carried here by the restack: aChangeWhichLeavesTheCacheSizeAloneIsAdmittedAfterARefusedReservation is green on this head with checkEnvironmentConfiguration after the &&.

Non-blocking: a moved db-directory returns early. Both storages now return on ccr.getResultCode() != ResultCode.SUCCESS after checkDBDirPermissions and setDBDirPermissions. The first checkDBDirExistsOrCanCreate check stays as it is, since the directory note is not in the result yet at that point.

• JEStorageTest.aChangeWhichMovesTheDirectoryIsStillAppliedToTheEnvironment: the directory, db-txn-no-sync and db-log-file-max move together; the result is the directory note plus 631, and COMMIT_NO_SYNC is applied.
• PDBStorageTest.aChangeWhichMovesTheDirectoryStillReportsTheRest: the directory and the checkpoint interval move together; the result is the directory note plus 631.

Suggestion: the percent pin. Taken as aCachePercentChangedWhileOpenLeavesTheCacheWhereTheOpenReservedIt, with your body.

Suggestion: the checkpoint interval across two changes. Your two lines are appended to aCheckpointIntervalChangedWhileOpenAsksForARestart.

Runs. Reactor verify: JEStorageTest 34/34 and PDBStorageTest 28/28. Directly under TestNG: FailedBackendOpenTest 8/8 and BackendConfigManagerTestCase 11/11. Mutants, one JVM per run:

mutant	red
mutable parameters skipped again (the previous head)	aRemovedNativePropertyGoesBackToJEsDefault, aRemovedNativePropertyJECannotPutBackWhileItRunsAsksForARestart
your snippet as is (no fallback)	aRemovedNativePropertyJECannotPutBackWhileItRunsAsksForARestart
JE: !ccr.getMessages().isEmpty() after the permissions check	aChangeWhichMovesTheDirectoryIsStillAppliedToTheEnvironment
PDB: the same	aChangeWhichMovesTheDirectoryStillReportsTheRest
MAX_MEMORY_PERCENT copy deleted	aCachePercentChangedWhileOpenLeavesTheCacheWhereTheOpenReservedIt
PDB interval compared against config	aCheckpointIntervalChangedWhileOpenAsksForARestart

Each mutant is red on exactly the cases listed and green elsewhere.