opendev/gerrit
Code Issues Proposed changes

Enable testing of Gerrit in slave mode

Browse Source 
Recently we had a few breakages of running Gerrit in slave mode (e.g.
due to trying to access secondary indexes which don't exist on slaves).
These breakages were only noticed after user reports since we don't have
any integration tests for Gerrit slaves at all.

This change enables integration tests against Gerrit slaves. It is easy
to start the Gerrit daemon in slave mode but then the server is in
read-only mode and we can't create any test data using the Gerrit API.
To be able to test against a slave server which is populated with test
data we first start the daemon as master and then after the test data
has been created we restart the daemon in slave mode.

If the server is restarted in slave mode we must preserve the content
of the test site, the repositories and the database content.

Preserving the content of the test site is not a problem since the test
site folders are only deleted at the very end after executing all tests.

The git repositories and the database may exist only in memory and in
this case we must make sure that the same instances are used when the
daemon is restarted in slave mode.

As in-memory database we use H2 [1]. The contents of an in-memory H2
database are gone as soon as the last connection to the database is
closed. This is why we must ensure to keep a database connection open
when then server in master mode is shutdown and we intend to restart it
as slave.

When the server is restarted we must make sure to reopen SSH connections
and reinject members into the test class and into the project resettter.

Tests that want to run against a Gerrit slave can use the normal Gerrit
API to setup the test data and then use the 'restartAsGerritSlave'
method from AbstractDaemonTest to restart the daemon in slave mode. For
these tests it is required to use the @Sandboxed annotation so that they
get an own server instance. Restarting the common server instance as
slave would break other tests using this server.

As an example the SshCommandsIT test is extended to check the list of
available commands in master and slave mode. Also it executes all
available commands in both modes.

[1] http://www.h2database.com/html/features.html#in_memory_databases

Change-Id: I2856e04ffa9fe4aac63747d4fc463581f7a1a6d6
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2017-10-20 11:01:07 +02:00
committed by David Pursehouse
parent b368a125cb
commit 33390fe8ac
8 changed files with 321 additions and 119 deletions
Download Patch File Download Diff File Expand all files Collapse all files

65
java/com/google/gerrit/acceptance/AbstractDaemonTest.java
View File

@@ -18,7 +18,6 @@ import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth.assert_;
import static com.google.common.truth.Truth8.assertThat;
import static com.google.common.truth.TruthJUnit.assume;
import static com.google.gerrit.acceptance.GitUtil.initSsh;
import static com.google.gerrit.extensions.api.changes.SubmittedTogetherOption.NON_VISIBLE_CHANGES;
import static com.google.gerrit.reviewdb.client.Patch.COMMIT_MSG;
import static com.google.gerrit.reviewdb.client.Patch.MERGE_LIST;
@@ -129,6 +128,7 @@ import com.google.gwtorm.server.OrmException;
import com.google.gwtorm.server.SchemaFactory;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.jcraft.jsch.JSchException;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.InputStream;
@@ -201,8 +201,10 @@ public abstract class AbstractDaemonTest {
}
beforeTest(description);
try (ProjectResetter resetter = resetProjects(projectResetter.builder())) {
AbstractDaemonTest.this.resetter = resetter;
base.evaluate();
} finally {
AbstractDaemonTest.this.resetter = null;
afterTest();
}
}
@@ -267,6 +269,7 @@ public abstract class AbstractDaemonTest {
@Inject private SchemaFactory<ReviewDb> reviewDbProvider;
@Inject private Groups groups;
private ProjectResetter resetter;
private List<Repository> toClose;
@Before
@@ -330,6 +333,16 @@ public abstract class AbstractDaemonTest {
.build();
}
protected void restartAsSlave() throws Exception {
closeSsh();
server = GerritServer.restartAsSlave(server);
server.getTestInjector().injectMembers(this);
if (resetter != null) {
server.getTestInjector().injectMembers(resetter);
}
initSsh();
}
protected static Config submitWholeTopicEnabledConfig() {
Config cfg = new Config();
cfg.setBoolean("change", null, "submitWholeTopic", true);
@@ -396,20 +409,7 @@ public abstract class AbstractDaemonTest {
adminRestSession = new RestSession(server, admin);
userRestSession = new RestSession(server, user);
if (testRequiresSsh
&& SshMode.useSsh()
&& (adminSshSession == null || userSshSession == null)) {
// Create Ssh sessions
initSsh(admin);
Context ctx = newRequestContext(user);
atrScope.set(ctx);
userSshSession = ctx.getSession();
userSshSession.open();
ctx = newRequestContext(admin);
atrScope.set(ctx);
adminSshSession = ctx.getSession();
adminSshSession.open();
}
initSsh();
resourcePrefix =
UNSAFE_PROJECT_NAME
@@ -422,6 +422,23 @@ public abstract class AbstractDaemonTest {
testRepo = cloneProject(project, getCloneAsAccount(description));
}
protected void initSsh() throws JSchException {
if (testRequiresSsh
&& SshMode.useSsh()
&& (adminSshSession == null || userSshSession == null)) {
// Create Ssh sessions
GitUtil.initSsh(admin);
Context ctx = newRequestContext(user);
atrScope.set(ctx);
userSshSession = ctx.getSession();
userSshSession.open();
ctx = newRequestContext(admin);
atrScope.set(ctx);
adminSshSession = ctx.getSession();
adminSshSession.open();
}
}
private TestAccount getCloneAsAccount(Description description) {
TestProjectInput ann = description.getAnnotation(TestProjectInput.class);
return accountCreator.get(ann != null ? ann.cloneAs() : "admin");
@@ -557,12 +574,7 @@ public abstract class AbstractDaemonTest {
repo.close();
}
db.close();
if (adminSshSession != null) {
adminSshSession.close();
}
if (userSshSession != null) {
userSshSession.close();
}
closeSsh();
if (server != commonServer) {
server.close();
server = null;
@@ -570,6 +582,17 @@ public abstract class AbstractDaemonTest {
NoteDbMode.resetFromEnv(notesMigration);
}
protected void closeSsh() {
if (adminSshSession != null) {
adminSshSession.close();
adminSshSession = null;
}
if (userSshSession != null) {
userSshSession.close();
userSshSession = null;
}
}
protected TestRepository<?>.CommitBuilder commitBuilder() throws Exception {
return testRepo.branch("HEAD").commit().insertChangeId();
}

62
java/com/google/gerrit/acceptance/GerritServer.java
View File

@@ -16,6 +16,7 @@ package com.google.gerrit.acceptance;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState;
import com.google.auto.value.AutoValue;
import com.google.common.base.MoreObjects;
@@ -27,6 +28,7 @@ import com.google.gerrit.lucene.LuceneIndexModule;
import com.google.gerrit.pgm.Daemon;
import com.google.gerrit.pgm.Init;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.config.SitePath;
import com.google.gerrit.server.git.receive.AsyncReceiveCommits;
import com.google.gerrit.server.ssh.NoSshModule;
import com.google.gerrit.server.util.ManualRequestContext;
@@ -35,6 +37,8 @@ import com.google.gerrit.server.util.SocketUtil;
import com.google.gerrit.server.util.SystemLog;
import com.google.gerrit.testing.FakeEmailSender;
import com.google.gerrit.testing.GroupNoteDbMode;
import com.google.gerrit.testing.InMemoryDatabase;
import com.google.gerrit.testing.InMemoryRepositoryManager;
import com.google.gerrit.testing.NoteDbChecker;
import com.google.gerrit.testing.NoteDbMode;
import com.google.gerrit.testing.SshMode;
@@ -261,7 +265,7 @@ public class GerritServer implements AutoCloseable {
if (!desc.memory()) {
init(desc, baseConfig, site);
}
return start(desc, baseConfig, site, null);
return start(desc, baseConfig, site, null, null, null);
} catch (Exception e) {
TempFileUtil.recursivelyDelete(site.toFile());
throw e;
@@ -278,6 +282,10 @@ public class GerritServer implements AutoCloseable {
* initialize this directory. Can be retrieved from the returned instance via {@link
* #getSitePath()}.
* @param testSysModule optional additional module to add to the system injector.
* @param inMemoryRepoManager {@link InMemoryRepositoryManager} that should be used if the site is
* started in memory
* @param inMemoryDatabaseInstance {@link com.google.gerrit.testing.InMemoryDatabase.Instance}
* that should be used if the site is started in memory
* @param additionalArgs additional command-line arguments for the daemon program; only allowed if
* the test is not in-memory.
* @return started server.
@@ -288,6 +296,8 @@ public class GerritServer implements AutoCloseable {
Config baseConfig,
Path site,
@Nullable Module testSysModule,
@Nullable InMemoryRepositoryManager inMemoryRepoManager,
@Nullable InMemoryDatabase.Instance inMemoryDatabaseInstance,
String... additionalArgs)
throws Exception {
checkArgument(site != null, "site is required (even for in-memory server");
@@ -307,16 +317,24 @@ public class GerritServer implements AutoCloseable {
daemon.setEmailModuleForTesting(new FakeEmailSender.Module());
daemon.setAdditionalSysModuleForTesting(testSysModule);
daemon.setEnableSshd(desc.useSsh());
daemon.setSlave(baseConfig.getBoolean("container", "slave", false));
if (desc.memory()) {
checkArgument(additionalArgs.length == 0, "cannot pass args to in-memory server");
return startInMemory(desc, site, baseConfig, daemon);
return startInMemory(
desc, site, baseConfig, daemon, inMemoryRepoManager, inMemoryDatabaseInstance);
}
return startOnDisk(desc, site, daemon, serverStarted, additionalArgs);
}
private static GerritServer startInMemory(
Description desc, Path site, Config baseConfig, Daemon daemon) throws Exception {
Description desc,
Path site,
Config baseConfig,
Daemon daemon,
@Nullable InMemoryRepositoryManager inMemoryRepoManager,
@Nullable InMemoryDatabase.Instance inMemoryDatabaseInstance)
throws Exception {
Config cfg = desc.buildConfig(baseConfig);
mergeTestConfig(cfg);
// Set the log4j configuration to an invalid one to prevent system logs
@@ -329,7 +347,9 @@ public class GerritServer implements AutoCloseable {
daemon.setEnableHttpd(desc.httpd());
daemon.setLuceneModule(LuceneIndexModule.singleVersionAllLatest(0));
daemon.setDatabaseForTesting(
ImmutableList.<Module>of(new InMemoryTestingDatabaseModule(cfg, site)));
ImmutableList.<Module>of(
new InMemoryTestingDatabaseModule(
cfg, site, inMemoryRepoManager, inMemoryDatabaseInstance)));
daemon.start();
return new GerritServer(desc, null, createTestInjector(daemon), daemon, null);
}
@@ -483,6 +503,40 @@ public class GerritServer implements AutoCloseable {
return desc;
}
public static GerritServer restartAsSlave(GerritServer server) throws Exception {
checkState(server.desc.sandboxed(), "restarting as slave requires @Sandboxed");
Path site = server.testInjector.getInstance(Key.get(Path.class, SitePath.class));
Config cfg = server.testInjector.getInstance(Key.get(Config.class, GerritServerConfig.class));
cfg.setBoolean("container", null, "slave", true);
InMemoryRepositoryManager inMemoryRepoManager = null;
if (hasBinding(server.testInjector, InMemoryRepositoryManager.class)) {
inMemoryRepoManager = server.testInjector.getInstance(InMemoryRepositoryManager.class);
}
InMemoryDatabase.Instance dbInstance = null;
if (hasBinding(server.testInjector, InMemoryDatabase.class)) {
InMemoryDatabase inMemoryDatabase = server.testInjector.getInstance(InMemoryDatabase.class);
dbInstance = inMemoryDatabase.getDbInstance();
dbInstance.setKeepOpen(true);
}
try {
server.close();
server.daemon.stop();
return start(server.desc, cfg, site, null, inMemoryRepoManager, dbInstance);
} finally {
if (dbInstance != null) {
dbInstance.setKeepOpen(false);
}
}
}
private static boolean hasBinding(Injector injector, Class<?> clazz) {
return injector.getExistingBinding(Key.get(clazz)) != null;
}
@Override
public void close() throws Exception {
try {

23
java/com/google/gerrit/acceptance/InMemoryTestingDatabaseModule.java
View File

@@ -16,6 +16,7 @@ package com.google.gerrit.acceptance;
import static com.google.inject.Scopes.SINGLETON;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.events.LifecycleListener;
import com.google.gerrit.lifecycle.LifecycleModule;
import com.google.gerrit.metrics.DisabledMetricMaker;
@@ -48,6 +49,7 @@ import com.google.inject.Provides;
import com.google.inject.ProvisionException;
import com.google.inject.Singleton;
import com.google.inject.TypeLiteral;
import com.google.inject.util.Providers;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
@@ -58,10 +60,18 @@ import org.eclipse.jgit.lib.Config;
class InMemoryTestingDatabaseModule extends LifecycleModule {
private final Config cfg;
private final Path sitePath;
@Nullable private final InMemoryRepositoryManager repoManager;
@Nullable private final InMemoryDatabase.Instance inMemoryDatabaseInstance;
InMemoryTestingDatabaseModule(Config cfg, Path sitePath) {
InMemoryTestingDatabaseModule(
Config cfg,
Path sitePath,
@Nullable InMemoryRepositoryManager repoManager,
@Nullable InMemoryDatabase.Instance inMemoryDatabaseInstance) {
this.cfg = cfg;
this.sitePath = sitePath;
this.repoManager = repoManager;
this.inMemoryDatabaseInstance = inMemoryDatabaseInstance;
makeSiteDirs(sitePath);
}
@@ -72,8 +82,12 @@ class InMemoryTestingDatabaseModule extends LifecycleModule {
// TODO(dborowitz): Use jimfs.
bind(Path.class).annotatedWith(SitePath.class).toInstance(sitePath);
bind(GitRepositoryManager.class).to(InMemoryRepositoryManager.class);
bind(InMemoryRepositoryManager.class).in(SINGLETON);
if (repoManager != null) {
bind(GitRepositoryManager.class).toInstance(repoManager);
} else {
bind(GitRepositoryManager.class).to(InMemoryRepositoryManager.class);
bind(InMemoryRepositoryManager.class).in(SINGLETON);
}
bind(MetricMaker.class).to(DisabledMetricMaker.class);
bind(DataSourceType.class).to(InMemoryH2Type.class);
@@ -83,6 +97,7 @@ class InMemoryTestingDatabaseModule extends LifecycleModule {
TypeLiteral<SchemaFactory<ReviewDb>> schemaFactory =
new TypeLiteral<SchemaFactory<ReviewDb>>() {};
bind(schemaFactory).to(NotesMigrationSchemaFactory.class);
bind(InMemoryDatabase.Instance.class).toProvider(Providers.of(inMemoryDatabaseInstance));
bind(Key.get(schemaFactory, ReviewDbFactory.class)).to(InMemoryDatabase.class);
bind(InMemoryDatabase.class).in(SINGLETON);
bind(ChangeBundleReader.class).to(GwtormChangeBundleReader.class);
@@ -132,7 +147,7 @@ class InMemoryTestingDatabaseModule extends LifecycleModule {
@Override
public void stop() {
mem.drop();
mem.getDbInstance().drop();
}
}

11
java/com/google/gerrit/acceptance/ProjectResetter.java
View File

@@ -120,11 +120,12 @@ public class ProjectResetter implements AutoCloseable {
}
}
private final GitRepositoryManager repoManager;
private final AllUsersName allUsersName;
@Nullable private final AccountCreator accountCreator;
@Nullable private final AccountCache accountCache;
@Nullable private final ProjectCache projectCache;
@Inject private GitRepositoryManager repoManager;
@Inject private AllUsersName allUsersName;
@Inject @Nullable private AccountCreator accountCreator;
@Inject @Nullable private AccountCache accountCache;
@Inject @Nullable private ProjectCache projectCache;
private final Multimap<Project.NameKey, String> refsPatternByProject;
private final Multimap<Project.NameKey, RefState> savedRefStatesByProject;

2
java/com/google/gerrit/acceptance/StandaloneSiteTest.java
View File

@@ -149,7 +149,7 @@ public abstract class StandaloneSiteTest {
private GerritServer startImpl(@Nullable Module testSysModule, String... additionalArgs)
throws Exception {
return GerritServer.start(
serverDesc, baseConfig, sitePaths.site_path, testSysModule, additionalArgs);
serverDesc, baseConfig, sitePaths.site_path, testSysModule, null, null, additionalArgs);
}
protected static void runGerrit(String... args) throws Exception {