From 6dcaddf465517e4a694557784f342a915384c389 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Sat, 21 Apr 2018 18:30:21 +0200 Subject: [PATCH 1/2] Delete index after each test in elasticsearch tests Since change I6539845e0 the index is not deleted after each test method because of a problem where the index was not deleted quickly enough and the index for the next test could not be created due to the previous one still existing. The indices are all kept and then only deleted at the end of the run. Leaving all the indices results in running out of file descriptors which results in tests failing when many tests are run in sequence. Since we now give each test index a unique name, we no longer suffer from the original problem described above. We can safely attempt to delete the index and it doesn't matter if it does not get deleted quickly enough and still exists when the next test is started. The tests for accounts, groups and projects are not currently affected by this problem because they have far fewer test methods than the tests for changes. The same fix is done for those tests anyway to make sure we do not run into the same problem in future. Bug: Issue 8816 Change-Id: I1646f5b802dd2d15b974d4c4d75ab337c4c6f462 --- .../testing/ElasticTestUtils.java | 32 +++++++++++++++++-- .../ElasticQueryAccountsTest.java | 14 +++++++- .../ElasticQueryChangesTest.java | 14 +++++++- .../elasticsearch/ElasticQueryGroupsTest.java | 14 +++++++- 4 files changed, 69 insertions(+), 5 deletions(-) diff --git a/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/testing/ElasticTestUtils.java b/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/testing/ElasticTestUtils.java index ca9673e20a..9ac582a46a 100644 --- a/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/testing/ElasticTestUtils.java +++ b/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/testing/ElasticTestUtils.java @@ -109,8 +109,36 @@ public final class ElasticTestUtils { return new ElasticNodeInfo(node, elasticDir, getHttpPort(node)); } - public static void deleteAllIndexes(ElasticNodeInfo nodeInfo) { - nodeInfo.node.client().admin().indices().prepareDelete("_all").execute(); + public static void deleteAllIndexes(ElasticNodeInfo nodeInfo, String prefix) { + Schema changeSchema = ChangeSchemaDefinitions.INSTANCE.getLatest(); + nodeInfo + .node + .client() + .admin() + .indices() + .prepareDelete(String.format("%s%s_%04d", prefix, CHANGES, changeSchema.getVersion())) + .execute() + .actionGet(); + + Schema accountSchema = AccountSchemaDefinitions.INSTANCE.getLatest(); + nodeInfo + .node + .client() + .admin() + .indices() + .prepareDelete(String.format("%s%s_%04d", prefix, ACCOUNTS, accountSchema.getVersion())) + .execute() + .actionGet(); + + Schema groupSchema = GroupSchemaDefinitions.INSTANCE.getLatest(); + nodeInfo + .node + .client() + .admin() + .indices() + .prepareDelete(String.format("%s%s_%04d", prefix, GROUPS, groupSchema.getVersion())) + .execute() + .actionGet(); } public static class NodeInfo { diff --git a/gerrit-elasticsearch/src/test/java/com/google/gerrit/elasticsearch/ElasticQueryAccountsTest.java b/gerrit-elasticsearch/src/test/java/com/google/gerrit/elasticsearch/ElasticQueryAccountsTest.java index 6b3ebc5b64..57654360c9 100644 --- a/gerrit-elasticsearch/src/test/java/com/google/gerrit/elasticsearch/ElasticQueryAccountsTest.java +++ b/gerrit-elasticsearch/src/test/java/com/google/gerrit/elasticsearch/ElasticQueryAccountsTest.java @@ -22,6 +22,7 @@ import com.google.inject.Guice; import com.google.inject.Injector; import java.util.concurrent.ExecutionException; import org.eclipse.jgit.lib.Config; +import org.junit.After; import org.junit.AfterClass; import org.junit.BeforeClass; @@ -46,11 +47,22 @@ public class ElasticQueryAccountsTest extends AbstractQueryAccountsTest { } } + private String testName() { + return testName.getMethodName().toLowerCase() + "_"; + } + + @After + public void cleanupIndex() { + if (nodeInfo != null) { + ElasticTestUtils.deleteAllIndexes(nodeInfo, testName()); + } + } + @Override protected Injector createInjector() { Config elasticsearchConfig = new Config(config); InMemoryModule.setDefaults(elasticsearchConfig); - String indicesPrefix = testName.getMethodName().toLowerCase() + "_"; + String indicesPrefix = testName(); ElasticTestUtils.configure(elasticsearchConfig, nodeInfo.port, indicesPrefix); ElasticTestUtils.createAllIndexes(nodeInfo, indicesPrefix); return Guice.createInjector(new InMemoryModule(elasticsearchConfig, notesMigration)); diff --git a/gerrit-elasticsearch/src/test/java/com/google/gerrit/elasticsearch/ElasticQueryChangesTest.java b/gerrit-elasticsearch/src/test/java/com/google/gerrit/elasticsearch/ElasticQueryChangesTest.java index 9ae667cb79..ee90449f10 100644 --- a/gerrit-elasticsearch/src/test/java/com/google/gerrit/elasticsearch/ElasticQueryChangesTest.java +++ b/gerrit-elasticsearch/src/test/java/com/google/gerrit/elasticsearch/ElasticQueryChangesTest.java @@ -24,6 +24,7 @@ import com.google.inject.Injector; import java.util.concurrent.ExecutionException; import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.lib.Config; +import org.junit.After; import org.junit.AfterClass; import org.junit.BeforeClass; import org.junit.Rule; @@ -53,11 +54,22 @@ public class ElasticQueryChangesTest extends AbstractQueryChangesTest { } } + private String testName() { + return testName.getMethodName().toLowerCase() + "_"; + } + + @After + public void cleanupIndex() { + if (nodeInfo != null) { + ElasticTestUtils.deleteAllIndexes(nodeInfo, testName()); + } + } + @Override protected Injector createInjector() { Config elasticsearchConfig = new Config(config); InMemoryModule.setDefaults(elasticsearchConfig); - String indicesPrefix = testName.getMethodName().toLowerCase() + "_"; + String indicesPrefix = testName(); ElasticTestUtils.configure(elasticsearchConfig, nodeInfo.port, indicesPrefix); ElasticTestUtils.createAllIndexes(nodeInfo, indicesPrefix); return Guice.createInjector(new InMemoryModule(elasticsearchConfig, notesMigration)); diff --git a/gerrit-elasticsearch/src/test/java/com/google/gerrit/elasticsearch/ElasticQueryGroupsTest.java b/gerrit-elasticsearch/src/test/java/com/google/gerrit/elasticsearch/ElasticQueryGroupsTest.java index c7f7475bbf..b9912d3a9a 100644 --- a/gerrit-elasticsearch/src/test/java/com/google/gerrit/elasticsearch/ElasticQueryGroupsTest.java +++ b/gerrit-elasticsearch/src/test/java/com/google/gerrit/elasticsearch/ElasticQueryGroupsTest.java @@ -22,6 +22,7 @@ import com.google.inject.Guice; import com.google.inject.Injector; import java.util.concurrent.ExecutionException; import org.eclipse.jgit.lib.Config; +import org.junit.After; import org.junit.AfterClass; import org.junit.BeforeClass; @@ -46,11 +47,22 @@ public class ElasticQueryGroupsTest extends AbstractQueryGroupsTest { } } + private String testName() { + return testName.getMethodName().toLowerCase() + "_"; + } + + @After + public void cleanupIndex() { + if (nodeInfo != null) { + ElasticTestUtils.deleteAllIndexes(nodeInfo, testName()); + } + } + @Override protected Injector createInjector() { Config elasticsearchConfig = new Config(config); InMemoryModule.setDefaults(elasticsearchConfig); - String indicesPrefix = testName.getMethodName().toLowerCase() + "_"; + String indicesPrefix = testName(); ElasticTestUtils.configure(elasticsearchConfig, nodeInfo.port, indicesPrefix); ElasticTestUtils.createAllIndexes(nodeInfo, indicesPrefix); return Guice.createInjector(new InMemoryModule(elasticsearchConfig, notesMigration)); From 679c04efe0acae5026c00dfd259808d104026c73 Mon Sep 17 00:00:00 2001 From: Inderjot Kaur Ratol Date: Mon, 23 Apr 2018 13:54:54 -0400 Subject: [PATCH 2/2] Add more debug logging to account creation As explained in issue 8803 new LDAP users are still facing issues due to missing DB entries. These extra log statements can help to get a more focused idea of the problem in the future. Bug: Issue 8803 Change-Id: I03df282d0611259d196160082cd787ed8232bb79 --- .../google/gerrit/server/account/AccountManager.java | 5 +++++ .../google/gerrit/server/account/ChangeUserName.java | 11 +++++++++-- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java index 4c62287f60..a4c39e07aa 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java @@ -214,10 +214,12 @@ public class AccountManager { private AuthResult create(ReviewDb db, AuthRequest who) throws OrmException, AccountException, IOException, ConfigInvalidException { Account.Id newId = new Account.Id(db.nextAccountId()); + log.debug("Assigning new Id {} to account", newId); Account account = new Account(newId, TimeUtil.nowTs()); ExternalId extId = ExternalId.createWithEmail(who.getExternalIdKey(), newId, who.getEmailAddress()); + log.debug("Created external Id: {}", extId); account.setFullName(who.getDisplayName()); account.setPreferredEmail(extId.email()); @@ -272,10 +274,13 @@ public class AccountManager { db.accountGroupMembers().insert(Collections.singleton(m)); } + log.debug("Username from AuthRequest: {}", who.getUserName()); if (who.getUserName() != null) { + log.debug("Setting username for: {}", who.getUserName()); // Only set if the name hasn't been used yet, but was given to us. // IdentifiedUser user = userFactory.create(newId); + log.debug("Identified user {} was created from {}", user, who.getUserName()); try { changeUserNameFactory.create(db, user, who.getUserName()).call(); } catch (NameAlreadyUsedException e) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/ChangeUserName.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/ChangeUserName.java index 978331eca8..c9a7aabf21 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/ChangeUserName.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/ChangeUserName.java @@ -33,13 +33,16 @@ import java.util.Collection; import java.util.concurrent.Callable; import java.util.regex.Pattern; import org.eclipse.jgit.errors.ConfigInvalidException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** Operation to change the username of an account. */ public class ChangeUserName implements Callable { - public static final String USERNAME_CANNOT_BE_CHANGED = "Username cannot be changed."; - + private static final Logger log = LoggerFactory.getLogger(ChangeUserName.class); private static final Pattern USER_NAME_PATTERN = Pattern.compile(Account.USER_NAME_PATTERN); + public static final String USERNAME_CANNOT_BE_CHANGED = "Username cannot be changed."; + /** Generic factory to change any user's username. */ public interface Factory { ChangeUserName create(ReviewDb db, IdentifiedUser user, String newUsername); @@ -79,6 +82,9 @@ public class ChangeUserName implements Callable { .filter(e -> e.isScheme(SCHEME_USERNAME)) .collect(toSet()); if (!old.isEmpty()) { + log.error( + "External id with scheme \"username:\" already exists for the user {}", + user.getAccountId()); throw new IllegalStateException(USERNAME_CANNOT_BE_CHANGED); } @@ -97,6 +103,7 @@ public class ChangeUserName implements Callable { } } externalIdsUpdate.insert(db, ExternalId.create(key, user.getAccountId(), null, password)); + log.info("Created the new external Id with key: {}", key); } catch (OrmDuplicateKeyException dupeErr) { // If we are using this identity, don't report the exception. //