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
diff --git a/Documentation/note-db.txt b/Documentation/note-db.txt
index 728d630..815cda8 100644
--- a/Documentation/note-db.txt
+++ b/Documentation/note-db.txt
@@ -137,14 +137,23 @@
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.
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/pgm/StandaloneNoteDbMigrationIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/pgm/StandaloneNoteDbMigrationIT.java
index 0517675..8c50b90 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/pgm/StandaloneNoteDbMigrationIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/pgm/StandaloneNoteDbMigrationIT.java
@@ -61,6 +61,7 @@
@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 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 @@
@Test
public void migrateOneChange() throws Exception {
+ assertNoAutoMigrateConfig(gerritConfig);
+ assertNoAutoMigrateConfig(noteDbConfig);
assertNotesMigrationState(NotesMigrationState.REVIEW_DB);
setUpOneChange();
@@ -128,6 +135,8 @@
assertThat(db.changes().get(id2)).isNull();
}
}
+ assertNoAutoMigrateConfig(gerritConfig);
+ assertAutoMigrateConfig(noteDbConfig, false);
}
@Test
@@ -151,6 +160,40 @@
@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 @@
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 @@
}
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 @@
.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);
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java
index 0292aca..8aa2497 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java
@@ -87,12 +87,13 @@
@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 @@
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);
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritConfig.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritConfig.java
index fdc3b7f..0715f8e 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritConfig.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritConfig.java
@@ -18,10 +18,12 @@
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 @@
}
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();
+ }
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritServerConfigProvider.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritServerConfigProvider.java
index 494b63a..1a59c52 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritServerConfigProvider.java
+++ b/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 @@
@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")));
+ }
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/SitePaths.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/SitePaths.java
index c27f5b9..3748bfd 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/config/SitePaths.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/SitePaths.java
@@ -53,6 +53,7 @@
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 @@
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");
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java
index 7323c2e..61a0cd6 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java
@@ -320,6 +320,7 @@
}
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 @@
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 @@
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 @@
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 @@
? "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 @@
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);
}
