Validate replication.config at Gerrit startup
When using a multi-site setup, the gerrit.replicateOnStartUp flag
on the replication.config will create a split-brain across the
cluster because assumes that the off-line master node is the
source of truth for the world, which is clearly NOT the case.
In a multi-site setup, there are always nodes up-and-running
somewhere. If a node was off, it will be very likely to be behind
and should *never* force the other nodes to comply with its
offline status.
Change-Id: I32c91dbc56fd16e35886480ffdfb258ddc42a2fa
diff --git a/src/main/java/com/googlesource/gerrit/plugins/multisite/Configuration.java b/src/main/java/com/googlesource/gerrit/plugins/multisite/Configuration.java
index e5b6190..58a11e9 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/multisite/Configuration.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/multisite/Configuration.java
@@ -26,9 +26,11 @@
import com.google.gerrit.server.config.SitePaths;
import com.google.inject.Inject;
import com.google.inject.Singleton;
+import com.google.inject.spi.Message;
import com.googlesource.gerrit.plugins.multisite.forwarder.events.EventFamily;
import java.io.IOException;
import java.util.Arrays;
+import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
@@ -54,6 +56,7 @@
public static final String PLUGIN_NAME = "multi-site";
public static final String MULTI_SITE_CONFIG = PLUGIN_NAME + ".config";
+ public static final String REPLICATION_CONFIG = "replication.config";
static final String INSTANCE_ID_FILE = "instanceId.data";
@@ -79,15 +82,17 @@
private final Supplier<KafkaSubscriber> subscriber;
private final Supplier<Kafka> kafka;
private final Supplier<ZookeeperConfig> zookeeperConfig;
+ private final Supplier<Collection<Message>> replicationConfigValidation;
@Inject
Configuration(SitePaths sitePaths) {
- this(new FileBasedConfig(sitePaths.etc_dir.resolve(MULTI_SITE_CONFIG).toFile(), FS.DETECTED));
+ this(getConfigFile(sitePaths, MULTI_SITE_CONFIG), getConfigFile(sitePaths, REPLICATION_CONFIG));
}
@VisibleForTesting
- public Configuration(final Config cfg) {
- Supplier<Config> lazyCfg = lazyLoad(cfg);
+ public Configuration(Config multiSiteConfig, Config replicationConfig) {
+ Supplier<Config> lazyCfg = lazyLoad(multiSiteConfig);
+ replicationConfigValidation = lazyValidateReplicatioConfig(replicationConfig);
kafka = memoize(() -> new Kafka(lazyCfg));
publisher = memoize(() -> new KafkaPublisher(lazyCfg));
subscriber = memoize(() -> new KafkaSubscriber(lazyCfg));
@@ -125,6 +130,14 @@
return subscriber.get();
}
+ public Collection<Message> validate() {
+ return replicationConfigValidation.get();
+ }
+
+ private static FileBasedConfig getConfigFile(SitePaths sitePaths, String configFileName) {
+ return new FileBasedConfig(sitePaths.etc_dir.resolve(configFileName).toFile(), FS.DETECTED);
+ }
+
private Supplier<Config> lazyLoad(Config config) {
if (config instanceof FileBasedConfig) {
return memoize(
@@ -143,6 +156,28 @@
return ofInstance(config);
}
+ private Supplier<Collection<Message>> lazyValidateReplicatioConfig(Config replicationConfig) {
+ if (replicationConfig instanceof FileBasedConfig) {
+ FileBasedConfig fileConfig = (FileBasedConfig) replicationConfig;
+ try {
+ fileConfig.load();
+ return memoize(() -> validateReplicationConfig(replicationConfig));
+ } catch (IOException | ConfigInvalidException e) {
+ return ofInstance(Arrays.asList(new Message("Unable to load replication.config", e)));
+ }
+ }
+ return ofInstance(validateReplicationConfig(replicationConfig));
+ }
+
+ private Collection<Message> validateReplicationConfig(Config replicationConfig) {
+ if (replicationConfig.getBoolean("gerrit", "replicateOnStartup", false)) {
+ return Arrays.asList(
+ new Message(
+ "Invalid replication.config: gerrit.replicateOnStartup has to be set to 'false' for multi-site setups"));
+ }
+ return Collections.emptyList();
+ }
+
private static int getInt(
Supplier<Config> cfg, String section, String subSection, String name, int defaultValue) {
try {
diff --git a/src/main/java/com/googlesource/gerrit/plugins/multisite/Module.java b/src/main/java/com/googlesource/gerrit/plugins/multisite/Module.java
index a471b04..b933635 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/multisite/Module.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/multisite/Module.java
@@ -18,10 +18,12 @@
import com.google.gerrit.lifecycle.LifecycleModule;
import com.google.gerrit.server.config.SitePaths;
import com.google.gson.Gson;
+import com.google.inject.CreationException;
import com.google.inject.Inject;
import com.google.inject.Provides;
import com.google.inject.ProvisionException;
import com.google.inject.Singleton;
+import com.google.inject.spi.Message;
import com.googlesource.gerrit.plugins.multisite.broker.BrokerGson;
import com.googlesource.gerrit.plugins.multisite.broker.GsonProvider;
import com.googlesource.gerrit.plugins.multisite.cache.CacheModule;
@@ -39,6 +41,7 @@
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
+import java.util.Collection;
import java.util.UUID;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -77,6 +80,11 @@
+ "and then reload the multi-site plugin.");
}
+ Collection<Message> validationErrors = config.validate();
+ if (!validationErrors.isEmpty()) {
+ throw new CreationException(validationErrors);
+ }
+
listener().to(Log4jMessageLogger.class);
bind(MessageLogger.class).to(Log4jMessageLogger.class);
diff --git a/src/test/java/com/googlesource/gerrit/plugins/multisite/ConfigurationTest.java b/src/test/java/com/googlesource/gerrit/plugins/multisite/ConfigurationTest.java
index 0f7b30b..4ceead9 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/multisite/ConfigurationTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/multisite/ConfigurationTest.java
@@ -43,14 +43,16 @@
private static final int THREAD_POOL_SIZE = 1;
private Config globalPluginConfig;
+ private Config replicationConfig;
@Before
public void setUp() {
globalPluginConfig = new Config();
+ replicationConfig = new Config();
}
private Configuration getConfiguration() {
- return new Configuration(globalPluginConfig);
+ return new Configuration(globalPluginConfig, replicationConfig);
}
@Test
@@ -203,4 +205,11 @@
assertThat(property).isNull();
}
+
+ @Test
+ public void shouldReturnValidationErrorsWhenReplicationOnStartupIsEnabled() throws Exception {
+ Config replicationConfig = new Config();
+ replicationConfig.setBoolean("gerrit", null, "replicateOnStartup", true);
+ assertThat(new Configuration(globalPluginConfig, replicationConfig).validate()).isNotEmpty();
+ }
}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/multisite/kafka/consumer/EventConsumerIT.java b/src/test/java/com/googlesource/gerrit/plugins/multisite/kafka/consumer/EventConsumerIT.java
index 674e8de..3e8b3ca 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/multisite/kafka/consumer/EventConsumerIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/multisite/kafka/consumer/EventConsumerIT.java
@@ -50,6 +50,7 @@
import java.util.concurrent.LinkedBlockingQueue;
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;
+import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
@@ -100,7 +101,7 @@
this.config =
new FileBasedConfig(
sitePaths.etc_dir.resolve(Configuration.MULTI_SITE_CONFIG).toFile(), FS.DETECTED);
- this.multiSiteModule = new Module(new Configuration(config), noteDb, true);
+ this.multiSiteModule = new Module(new Configuration(config, new Config()), noteDb, true);
}
@Override
diff --git a/src/test/java/com/googlesource/gerrit/plugins/multisite/validation/dfsrefdb/zookeeper/ZookeeperTestContainerSupport.java b/src/test/java/com/googlesource/gerrit/plugins/multisite/validation/dfsrefdb/zookeeper/ZookeeperTestContainerSupport.java
index 7b9ebb5..e7e7838 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/multisite/validation/dfsrefdb/zookeeper/ZookeeperTestContainerSupport.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/multisite/validation/dfsrefdb/zookeeper/ZookeeperTestContainerSupport.java
@@ -87,7 +87,7 @@
Configuration.ZookeeperConfig.KEY_MIGRATE,
migrationMode);
- configuration = new Configuration(splitBrainconfig);
+ configuration = new Configuration(splitBrainconfig, new Config());
this.curator = configuration.getZookeeperConfig().buildCurator();
}
