Merge changes from topic "notedb-config"

* changes:
  Read NoteDb config first from notedb.config
  OnlineNoteDbMigrationIT: Use local disk
  MergeableFileBasedConfig: Add subsection values recursively
This commit is contained in:
Dave Borowitz
2017-09-12 17:30:32 +00:00
committed by Gerrit Code Review
10 changed files with 287 additions and 35 deletions

View File

@@ -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.

View File

@@ -1,6 +1,12 @@
load("//tools/bzl:java.bzl", "java_library2")
load("//tools/bzl:junit.bzl", "junit_tests")
SRCS = glob(["src/test/java/com/google/gerrit/acceptance/*.java"])
TEST_SRCS = ["src/test/java/com/google/gerrit/acceptance/MergeableFileBasedConfigTest.java"]
SRCS = glob(
["src/test/java/com/google/gerrit/acceptance/*.java"],
exclude = TEST_SRCS,
)
PROVIDED = [
"//gerrit-common:annotations",
@@ -77,3 +83,14 @@ java_doc(
title = "Gerrit Acceptance Test Framework Documentation",
visibility = ["//visibility:public"],
)
junit_tests(
name = "acceptance_framework_tests",
srcs = TEST_SRCS,
deps = [
":lib",
"//lib:guava",
"//lib:truth",
"//lib/jgit/org.eclipse.jgit:jgit",
],
)

View File

@@ -40,7 +40,7 @@ public class MergeableFileBasedConfig extends FileBasedConfig {
}
for (String section : s.getSections()) {
for (String subsection : s.getSubsections(section)) {
for (String name : s.getNames(section, subsection)) {
for (String name : s.getNames(section, subsection, true)) {
setStringList(
section,
subsection,

View File

@@ -0,0 +1,118 @@
// Copyright (C) 2017 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.gerrit.acceptance;
import static com.google.common.truth.Truth.assertThat;
import static java.nio.charset.StandardCharsets.UTF_8;
import com.google.common.collect.ImmutableList;
import java.io.File;
import java.nio.file.Files;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.util.FS;
import org.junit.Test;
public class MergeableFileBasedConfigTest {
@Test
public void mergeNull() throws Exception {
MergeableFileBasedConfig cfg = newConfig();
cfg.setString("foo", null, "bar", "value");
String expected = "[foo]\n\tbar = value\n";
assertConfig(cfg, expected);
cfg.merge(null);
assertConfig(cfg, expected);
}
@Test
public void mergeFlatConfig() throws Exception {
MergeableFileBasedConfig cfg = newConfig();
cfg.setString("foo", null, "bar1", "value1");
cfg.setString("foo", null, "bar2", "value2");
cfg.setString("foo", "sub", "bar1", "value1");
cfg.setString("foo", "sub", "bar2", "value2");
assertConfig(
cfg,
"[foo]\n"
+ "\tbar1 = value1\n"
+ "\tbar2 = value2\n"
+ "[foo \"sub\"]\n"
+ "\tbar1 = value1\n"
+ "\tbar2 = value2\n");
Config toMerge = new Config();
toMerge.setStringList("foo", null, "bar2", ImmutableList.of("merge1", "merge2"));
toMerge.setStringList("foo", "sub", "bar2", ImmutableList.of("merge1", "merge2"));
cfg.merge(toMerge);
assertConfig(
cfg,
"[foo]\n"
+ "\tbar1 = value1\n"
+ "\tbar2 = merge1\n"
+ "\tbar2 = merge2\n"
+ "[foo \"sub\"]\n"
+ "\tbar1 = value1\n"
+ "\tbar2 = merge1\n"
+ "\tbar2 = merge2\n");
}
@Test
public void mergeStackedConfig() throws Exception {
MergeableFileBasedConfig cfg = newConfig();
cfg.setString("foo", null, "bar1", "value1");
cfg.setString("foo", null, "bar2", "value2");
cfg.setString("foo", "sub", "bar1", "value1");
cfg.setString("foo", "sub", "bar2", "value2");
assertConfig(
cfg,
"[foo]\n"
+ "\tbar1 = value1\n"
+ "\tbar2 = value2\n"
+ "[foo \"sub\"]\n"
+ "\tbar1 = value1\n"
+ "\tbar2 = value2\n");
Config base = new Config();
Config toMerge = new Config(base);
base.setStringList("foo", null, "bar2", ImmutableList.of("merge1", "merge2"));
base.setStringList("foo", "sub", "bar2", ImmutableList.of("merge1", "merge2"));
cfg.merge(toMerge);
assertConfig(
cfg,
"[foo]\n"
+ "\tbar1 = value1\n"
+ "\tbar2 = merge1\n"
+ "\tbar2 = merge2\n"
+ "[foo \"sub\"]\n"
+ "\tbar1 = value1\n"
+ "\tbar2 = merge1\n"
+ "\tbar2 = merge2\n");
}
private MergeableFileBasedConfig newConfig() throws Exception {
File f = File.createTempFile(getClass().getSimpleName(), ".config");
f.deleteOnExit();
return new MergeableFileBasedConfig(f, FS.detect());
}
private void assertConfig(MergeableFileBasedConfig cfg, String expected) throws Exception {
assertThat(cfg.toText()).isEqualTo(expected);
cfg.save();
assertThat(new String(Files.readAllBytes(cfg.getFile().toPath()), UTF_8)).isEqualTo(expected);
}
}

View File

@@ -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);

View File

@@ -32,6 +32,7 @@ import com.google.gerrit.acceptance.GerritConfig;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.Sandboxed;
import com.google.gerrit.acceptance.UseLocalDisk;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
@@ -65,6 +66,7 @@ import org.junit.Before;
import org.junit.Test;
@Sandboxed
@UseLocalDisk
@NoHttpd
public class OnlineNoteDbMigrationIT extends AbstractDaemonTest {
private static final String INVALID_STATE = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef";
@@ -85,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);
}
@@ -365,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);
}

View File

@@ -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();
}
}

View File

@@ -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")));
}
}
}

View File

@@ -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");

View File

@@ -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);
}