Read NoteDb config first from notedb.config

When designing the online migration process, we thought it would be
harmless to write data back to gerrit.config from a running server, and
to assume it would be persisted until the next restart.

Unfortunately, this assumption misses an important use case: when
gerrit.config is managed by Puppet or similar, the file may be
overwritten, losing any migration state.

Solve this problem by reading NoteDb config from notedb.config. This
config is stacked over gerrit.config in the @GerritServerConfig (the
same way that secure.config was prior to extracting the SecureStore
interface), so options may be set in either, but notedb.config takes
precedence. This specifically handles the use case where an admin sets
noteDb.changes.autoMigrate=true in gerrit.config using Puppet: the
migration tool will set autoMigrate=false in notedb.config at the end of
the process, and Puppet can maintain autoMigrate=true in gerrit.config
until an admin (optionally) comes through to copy the final state back
into gerrit.config.

Bug: Issue 7173
Change-Id: I14c69984bfd1c2cb559c73fbf5a4f036972b8cbd

Documentation/note-db.txt

@@ -137,14 +137,23 @@ There are several use cases for trial mode:
 To continue with the full migration after running the trial migration, use
 either the online or offline migration steps as normal. To revert to
 ReviewDb-only, remove `noteDb.changes.read` and `noteDb.changes.write` from
-`gerrit.config` and restart Gerrit.
+`notedb.config` and restart Gerrit.
 
 == Configuration
 
-The migration process works by setting a configuration option in `gerrit.config`
+The migration process works by setting a configuration option in `notedb.config`
 for each step in the process, then performing the corresponding data migration.
-In general, users should not set these options manually; this section serves
-primarily as a reference.
+
+Config options are read from `notedb.config` first, falling back to
+`gerrit.config`. If editing config manually, you may edit either file, but the
+migration process itself only touches `notedb.config`. This means if your
+`gerrit.config` is managed with Puppet or a similar tool, it can overwrite
+`gerrit.config` without affecting the migration process. You should not manage
+`notedb.config` with Puppet, but you may copy values back into `gerrit.config`
+and delete `notedb.config` at some later point after completing the migration.
+
+In general, users should not set the options described below manually; this
+section serves primarily as a reference.
 
 - `noteDb.changes.write=true`: During a ReviewDb write, the state of the change
   in NoteDb is written to the `note_db_state` field in the `Change` entity.

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/pgm/StandaloneNoteDbMigrationIT.java

@@ -61,6 +61,7 @@ import org.junit.Test;
 @NoHttpd
 public class StandaloneNoteDbMigrationIT extends StandaloneSiteTest {
   private StoredConfig gerritConfig;
+  private StoredConfig noteDbConfig;
 
   private Project.NameKey project;
   private Change.Id changeId;
@@ -69,10 +70,14 @@ public class StandaloneNoteDbMigrationIT extends StandaloneSiteTest {
   public void setUp() throws Exception {
     assume().that(NoteDbMode.get()).isEqualTo(NoteDbMode.OFF);
     gerritConfig = new FileBasedConfig(sitePaths.gerrit_config.toFile(), FS.detect());
+    // Unlike in the running server, for tests, we don't stack notedb.config on gerrit.config.
+    noteDbConfig = new FileBasedConfig(sitePaths.notedb_config.toFile(), FS.detect());
   }
 
   @Test
   public void rebuildOneChangeTrialMode() throws Exception {
+    assertNoAutoMigrateConfig(gerritConfig);
+    assertNoAutoMigrateConfig(noteDbConfig);
     assertNotesMigrationState(NotesMigrationState.REVIEW_DB);
     setUpOneChange();
 
@@ -101,6 +106,8 @@ public class StandaloneNoteDbMigrationIT extends StandaloneSiteTest {
 
   @Test
   public void migrateOneChange() throws Exception {
+    assertNoAutoMigrateConfig(gerritConfig);
+    assertNoAutoMigrateConfig(noteDbConfig);
     assertNotesMigrationState(NotesMigrationState.REVIEW_DB);
     setUpOneChange();
 
@@ -128,6 +135,8 @@ public class StandaloneNoteDbMigrationIT extends StandaloneSiteTest {
         assertThat(db.changes().get(id2)).isNull();
       }
     }
+    assertNoAutoMigrateConfig(gerritConfig);
+    assertAutoMigrateConfig(noteDbConfig, false);
   }
 
   @Test
@@ -151,6 +160,40 @@ public class StandaloneNoteDbMigrationIT extends StandaloneSiteTest {
 
   @Test
   public void onlineMigrationViaDaemon() throws Exception {
+    assertNoAutoMigrateConfig(gerritConfig);
+    assertNoAutoMigrateConfig(noteDbConfig);
+
+    testOnlineMigration(u -> startServer(u.module(), "--migrate-to-note-db", "true"));
+
+    assertNoAutoMigrateConfig(gerritConfig);
+    assertAutoMigrateConfig(noteDbConfig, false);
+  }
+
+  @Test
+  public void onlineMigrationViaConfig() throws Exception {
+    assertNoAutoMigrateConfig(gerritConfig);
+    assertNoAutoMigrateConfig(noteDbConfig);
+
+    testOnlineMigration(
+        u -> {
+          gerritConfig.setBoolean("noteDb", "changes", "autoMigrate", true);
+          gerritConfig.save();
+          return startServer(u.module());
+        });
+
+    // Auto-migration is turned off in notedb.config, which takes precedence, but is still on in
+    // gerrit.config. This means Puppet can continue overwriting gerrit.config without turning
+    // auto-migration back on.
+    assertAutoMigrateConfig(gerritConfig, true);
+    assertAutoMigrateConfig(noteDbConfig, false);
+  }
+
+  @FunctionalInterface
+  private interface StartServerWithMigration {
+    ServerContext start(IndexUpgradeController u) throws Exception;
+  }
+
+  private void testOnlineMigration(StartServerWithMigration start) throws Exception {
     assertNotesMigrationState(NotesMigrationState.REVIEW_DB);
     int prevVersion = ChangeSchemaDefinitions.INSTANCE.getPrevious().getVersion();
     int currVersion = ChangeSchemaDefinitions.INSTANCE.getLatest().getVersion();
@@ -166,7 +209,7 @@ public class StandaloneNoteDbMigrationIT extends StandaloneSiteTest {
     setOnlineUpgradeConfig(true);
 
     IndexUpgradeController u = new IndexUpgradeController(1);
-    try (ServerContext ctx = startServer(u.module(), "--migrate-to-note-db", "true")) {
+    try (ServerContext ctx = start.start(u)) {
       ChangeIndexCollection indexes = ctx.getInjector().getInstance(ChangeIndexCollection.class);
       assertThat(indexes.getSearchIndex().getSchema().getVersion()).isEqualTo(prevVersion);
 
@@ -199,8 +242,8 @@ public class StandaloneNoteDbMigrationIT extends StandaloneSiteTest {
   }
 
   private void assertNotesMigrationState(NotesMigrationState expected) throws Exception {
-    gerritConfig.load();
-    assertThat(NotesMigrationState.forConfig(gerritConfig)).hasValue(expected);
+    noteDbConfig.load();
+    assertThat(NotesMigrationState.forConfig(noteDbConfig)).hasValue(expected);
   }
 
   private ReviewDb openUnderlyingReviewDb(ServerContext ctx) throws Exception {
@@ -209,6 +252,17 @@ public class StandaloneNoteDbMigrationIT extends StandaloneSiteTest {
         .open();
   }
 
+  private static void assertNoAutoMigrateConfig(StoredConfig cfg) throws Exception {
+    cfg.load();
+    assertThat(cfg.getString("noteDb", "changes", "autoMigrate")).isNull();
+  }
+
+  private static void assertAutoMigrateConfig(StoredConfig cfg, boolean expected) throws Exception {
+    cfg.load();
+    assertThat(cfg.getString("noteDb", "changes", "autoMigrate")).isNotNull();
+    assertThat(cfg.getBoolean("noteDb", "changes", "autoMigrate", false)).isEqualTo(expected);
+  }
+
   private void setOnlineUpgradeConfig(boolean enable) throws Exception {
     gerritConfig.load();
     gerritConfig.setBoolean("index", null, "onlineUpgrade", enable);

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java

@@ -87,12 +87,13 @@ public class OnlineNoteDbMigrationIT extends AbstractDaemonTest {
   @Inject private Provider<NoteDbMigrator.Builder> migratorBuilderProvider;
   @Inject private Sequences sequences;
 
-  private FileBasedConfig gerritConfig;
+  private FileBasedConfig noteDbConfig;
 
   @Before
   public void setUp() throws Exception {
     assume().that(NoteDbMode.get()).isEqualTo(NoteDbMode.OFF);
-    gerritConfig = new FileBasedConfig(sitePaths.gerrit_config.toFile(), FS.detect());
+    // Unlike in the running server, for tests, we don't stack notedb.config on gerrit.config.
+    noteDbConfig = new FileBasedConfig(sitePaths.notedb_config.toFile(), FS.detect());
     assertNotesMigrationState(REVIEW_DB);
   }
 
@@ -367,27 +368,27 @@ public class OnlineNoteDbMigrationIT extends AbstractDaemonTest {
 
     migrate(b -> b.setStopAtStateForTesting(WRITE));
     assertNotesMigrationState(WRITE);
-    assertThat(NoteDbMigrator.getAutoMigrate(gerritConfig)).isFalse();
+    assertThat(NoteDbMigrator.getAutoMigrate(noteDbConfig)).isFalse();
 
     migrate(b -> b.setAutoMigrate(true).setStopAtStateForTesting(READ_WRITE_NO_SEQUENCE));
     assertNotesMigrationState(READ_WRITE_NO_SEQUENCE);
-    assertThat(NoteDbMigrator.getAutoMigrate(gerritConfig)).isTrue();
+    assertThat(NoteDbMigrator.getAutoMigrate(noteDbConfig)).isTrue();
 
     migrate(b -> b);
     assertNotesMigrationState(NOTE_DB);
-    assertThat(NoteDbMigrator.getAutoMigrate(gerritConfig)).isFalse();
+    assertThat(NoteDbMigrator.getAutoMigrate(noteDbConfig)).isFalse();
   }
 
   private void assertNotesMigrationState(NotesMigrationState expected) throws Exception {
     assertThat(NotesMigrationState.forNotesMigration(notesMigration)).hasValue(expected);
-    gerritConfig.load();
-    assertThat(NotesMigrationState.forConfig(gerritConfig)).hasValue(expected);
+    noteDbConfig.load();
+    assertThat(NotesMigrationState.forConfig(noteDbConfig)).hasValue(expected);
   }
 
   private void setNotesMigrationState(NotesMigrationState state) throws Exception {
-    gerritConfig.load();
-    state.setConfigValues(gerritConfig);
-    gerritConfig.save();
+    noteDbConfig.load();
+    state.setConfigValues(noteDbConfig);
+    noteDbConfig.save();
     notesMigration.setFrom(state);
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/config/GerritConfig.java

@@ -18,10 +18,12 @@ import com.google.gerrit.server.securestore.SecureStore;
 import org.eclipse.jgit.lib.Config;
 
 class GerritConfig extends Config {
+  private final Config gerritConfig;
   private final SecureStore secureStore;
 
-  GerritConfig(Config baseConfig, SecureStore secureStore) {
-    super(baseConfig);
+  GerritConfig(Config noteDbConfigOverGerritConfig, Config gerritConfig, SecureStore secureStore) {
+    super(noteDbConfigOverGerritConfig);
+    this.gerritConfig = gerritConfig;
     this.secureStore = secureStore;
   }
 
@@ -42,4 +44,11 @@ class GerritConfig extends Config {
     }
     return super.getStringList(section, subsection, name);
   }
+
+  @Override
+  public String toText() {
+    // Only show the contents of gerrit.config, hiding the implementation detail that some values
+    // may come from secure.config (or another secure store) and notedb.config.
+    return gerritConfig.toText();
+  }
 }

gerrit-server/src/main/java/com/google/gerrit/server/config/GerritServerConfigProvider.java

@@ -14,11 +14,18 @@
 
 package com.google.gerrit.server.config;
 
+import static java.util.stream.Collectors.joining;
+
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.server.notedb.NotesMigration;
 import com.google.gerrit.server.securestore.SecureStore;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 import com.google.inject.ProvisionException;
 import java.io.IOException;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.List;
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.storage.file.FileBasedConfig;
@@ -41,19 +48,46 @@ class GerritServerConfigProvider implements Provider<Config> {
 
   @Override
   public Config get() {
-    FileBasedConfig cfg = new FileBasedConfig(site.gerrit_config.toFile(), FS.DETECTED);
-
-    if (!cfg.getFile().exists()) {
+    FileBasedConfig baseConfig = loadConfig(null, site.gerrit_config);
+    if (!baseConfig.getFile().exists()) {
       log.info("No " + site.gerrit_config.toAbsolutePath() + "; assuming defaults");
-      return new GerritConfig(cfg, secureStore);
     }
 
+    FileBasedConfig noteDbConfigOverBaseConfig = loadConfig(baseConfig, site.notedb_config);
+    checkNoteDbConfig(noteDbConfigOverBaseConfig);
+
+    return new GerritConfig(noteDbConfigOverBaseConfig, baseConfig, secureStore);
+  }
+
+  private static FileBasedConfig loadConfig(@Nullable Config base, Path path) {
+    FileBasedConfig cfg = new FileBasedConfig(base, path.toFile(), FS.DETECTED);
     try {
       cfg.load();
     } catch (IOException | ConfigInvalidException e) {
       throw new ProvisionException(e.getMessage(), e);
     }
+    return cfg;
+  }
 
-    return new GerritConfig(cfg, secureStore);
+  private static void checkNoteDbConfig(FileBasedConfig noteDbConfig) {
+    List<String> bad = new ArrayList<>();
+    for (String section : noteDbConfig.getSections()) {
+      if (section.equals(NotesMigration.SECTION_NOTE_DB)) {
+        continue;
+      }
+      for (String subsection : noteDbConfig.getSubsections(section)) {
+        noteDbConfig
+            .getNames(section, subsection, false)
+            .forEach(n -> bad.add(section + "." + subsection + "." + n));
+      }
+      noteDbConfig.getNames(section, false).forEach(n -> bad.add(section + "." + n));
+    }
+    if (!bad.isEmpty()) {
+      throw new ProvisionException(
+          "Non-NoteDb config options not allowed in "
+              + noteDbConfig.getFile()
+              + ":\n"
+              + bad.stream().collect(joining("\n")));
+    }
   }
 }

gerrit-server/src/main/java/com/google/gerrit/server/config/SitePaths.java

@@ -53,6 +53,7 @@ public final class SitePaths {
 
   public final Path gerrit_config;
   public final Path secure_config;
+  public final Path notedb_config;
 
   public final Path ssl_keystore;
   public final Path ssh_key;
@@ -100,6 +101,7 @@ public final class SitePaths {
 
     gerrit_config = etc_dir.resolve("gerrit.config");
     secure_config = etc_dir.resolve("secure.config");
+    notedb_config = etc_dir.resolve("notedb.config");
 
     ssl_keystore = etc_dir.resolve("keystore");
     ssh_key = etc_dir.resolve("ssh_host_key");

gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java

@@ -320,6 +320,7 @@ public class NoteDbMigrator implements AutoCloseable {
   }
 
   private final FileBasedConfig gerritConfig;
+  private final FileBasedConfig noteDbConfig;
   private final SchemaFactory<ReviewDb> schemaFactory;
   private final GitRepositoryManager repoManager;
   private final AllProjectsName allProjects;
@@ -374,7 +375,6 @@ public class NoteDbMigrator implements AutoCloseable {
     this.userFactory = userFactory;
     this.globalNotesMigration = globalNotesMigration;
     this.primaryStorageMigrator = primaryStorageMigrator;
-    this.gerritConfig = new FileBasedConfig(sitePaths.gerrit_config.toFile(), FS.detect());
     this.executor = executor;
     this.projects = projects;
     this.changes = changes;
@@ -384,6 +384,11 @@ public class NoteDbMigrator implements AutoCloseable {
     this.forceRebuild = forceRebuild;
     this.sequenceGap = sequenceGap;
     this.autoMigrate = autoMigrate;
+
+    // Stack notedb.config over gerrit.config, in the same way as GerritServerConfigProvider.
+    this.gerritConfig = new FileBasedConfig(sitePaths.gerrit_config.toFile(), FS.detect());
+    this.noteDbConfig =
+        new FileBasedConfig(gerritConfig, sitePaths.notedb_config.toFile(), FS.detect());
   }
 
   @Override
@@ -564,9 +569,10 @@ public class NoteDbMigrator implements AutoCloseable {
   private Optional<NotesMigrationState> loadState() throws IOException {
     try {
       gerritConfig.load();
-      return NotesMigrationState.forConfig(gerritConfig);
+      noteDbConfig.load();
+      return NotesMigrationState.forConfig(noteDbConfig);
     } catch (ConfigInvalidException | IllegalArgumentException e) {
-      log.warn("error reading NoteDb migration options from " + gerritConfig.getFile(), e);
+      log.warn("error reading NoteDb migration options from " + noteDbConfig.getFile(), e);
       return Optional.empty();
     }
   }
@@ -597,9 +603,9 @@ public class NoteDbMigrator implements AutoCloseable {
                     ? "But found this state:\n" + actualOldState.get().toText()
                     : "But could not parse the current state"));
       }
-      newState.setConfigValues(gerritConfig);
-      additionalUpdates.accept(gerritConfig);
-      gerritConfig.save();
+      newState.setConfigValues(noteDbConfig);
+      additionalUpdates.accept(noteDbConfig);
+      noteDbConfig.save();
 
       // Only set in-memory state once it's been persisted to storage.
       globalNotesMigration.setFrom(newState);
@@ -610,9 +616,9 @@ public class NoteDbMigrator implements AutoCloseable {
 
   private void enableAutoMigrate() throws MigrationException {
     try {
-      gerritConfig.load();
-      setAutoMigrate(gerritConfig, true);
-      gerritConfig.save();
+      noteDbConfig.load();
+      setAutoMigrate(noteDbConfig, true);
+      noteDbConfig.save();
     } catch (ConfigInvalidException | IOException e) {
       throw new MigrationException("Error saving auto-migration config", e);
     }