Add a basic consistency checker for changes

In the past, Gerrit bugs, lack of transactions, and unreliable NoSQL
backends have at various times produced a bewildering variety of
corrupt states. Similarly, we are not immune from bugs being
introduced in the future.

Add a tool to detect and explain some of these possible states.

Change-Id: Ia91b35b140bf05254877f413003d12cf779b775c
This commit is contained in:
Dave Borowitz
2014-11-06 15:24:04 -08:00
committed by David Pursehouse
parent 2d8580003b
commit fd508cab05
8 changed files with 679 additions and 0 deletions

View File

@@ -1134,6 +1134,52 @@ Adds or updates the change in the secondary index.
HTTP/1.1 204 No Content
----
[[check-change]]
=== Check change
--
'GET /changes/link:#change-id[\{change-id\}]/check'
--
Performs consistency checks on the change, and returns a
link:#check-result[CheckResult] entity.
.Request
----
GET /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/check HTTP/1.0
----
.Response
----
HTTP/1.1 200 OK
Content-Disposition: attachment
Content-Type: application/json;charset=UTF-8
)]}'
{
"change": {
"id": "myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940",
"project": "myProject",
"branch": "master",
"change_id": "I8473b95934b5732ac55d26311a706c9c2bde9940",
"subject": "Implementing Feature X",
"status": "NEW",
"created": "2013-02-01 09:59:32.126000000",
"updated": "2013-02-21 11:16:36.775000000",
"mergeable": true,
"insertions": 34,
"deletions": 101,
"_sortkey": "0023412400000f7d",
"_number": 3965,
"owner": {
"name": "John Doe"
}
},
"messages": [
"Current patch set 1 not found"
]
}
----
[[edit-endpoints]]
== Change Edit Endpoints
@@ -3861,6 +3907,24 @@ path within change edit.
|`restore_path`|optional|Path to file to restore.
|===========================
[[check-result]]
=== CheckResult
The `CheckResult` entity contains the results of a consistency check on
a change.
[options="header",cols="1,6"]
|===========================
|Field Name|Description
|`change`|
link:#change-info[ChangeInfo] entity containing information about the change,
as in link:#get-change[Get Change] with no options. Some fields not marked
optional may be missing if a consistency check failed, but at least
`id`, `project`, `branch`, and `_number` will be present.
|`messages`|
List of messages describing potential problems with the change. May be
empty if no problems were found.
|===========================
GERRIT
------
Part of link:index.html[Gerrit Code Review]

View File

@@ -204,6 +204,7 @@ java_test(
'//lib:guava',
'//lib:gwtorm',
'//lib:junit',
'//lib:truth',
'//lib/guice:guice',
'//lib/guice:guice-assistedinject',
'//lib/jgit:jgit',

View File

@@ -0,0 +1,76 @@
// Copyright (C) 2014 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.server.change;
import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.server.account.AccountInfo;
import com.google.gerrit.server.change.ChangeJson.ChangeInfo;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@Singleton
public class Check implements RestReadView<ChangeResource> {
private static final Logger log = LoggerFactory.getLogger(Check.class);
private final Provider<ConsistencyChecker> checkerProvider;
private final ChangeJson json;
@Inject
Check(Provider<ConsistencyChecker> checkerProvider,
ChangeJson json) {
this.checkerProvider = checkerProvider;
this.json = json;
}
@Override
public CheckResult apply(ChangeResource rsrc) {
CheckResult result = new CheckResult();
result.messages = checkerProvider.get().check(rsrc.getChange());
try {
result.change = json.format(rsrc);
} catch (OrmException e) {
// Even with no options there are a surprising number of dependencies in
// ChangeJson. Fall back to a very basic implementation with no
// dependencies if this fails.
String msg = "Error rendering final ChangeInfo";
log.warn(msg, e);
result.messages.add(msg);
result.change = basicChangeInfo(rsrc.getChange());
}
return result;
}
private static ChangeInfo basicChangeInfo(Change c) {
ChangeInfo info = new ChangeInfo();
info.project = c.getProject().get();
info.branch = c.getDest().getShortName();
info.topic = c.getTopic();
info.changeId = c.getKey().get();
info.subject = c.getSubject();
info.status = c.getStatus();
info.owner = new AccountInfo(c.getOwner());
info.created = c.getCreatedOn();
info.updated = c.getLastUpdatedOn();
info._number = c.getId().get();
info.finish();
return info;
}
}

View File

@@ -0,0 +1,24 @@
// Copyright (C) 2014 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.server.change;
import com.google.gerrit.server.change.ChangeJson.ChangeInfo;
import java.util.List;
public class CheckResult {
public ChangeInfo change;
public List<String> messages;
}

View File

@@ -0,0 +1,260 @@
// Copyright (C) 2014 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.server.change;
import com.google.common.base.Function;
import com.google.common.collect.Collections2;
import com.google.common.collect.Multimap;
import com.google.common.collect.MultimapBuilder;
import com.google.common.collect.Ordering;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import org.eclipse.jgit.errors.IncorrectObjectTypeException;
import org.eclipse.jgit.errors.MissingObjectException;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.Map;
/**
* Checks changes for various kinds of inconsistency and corruption.
* <p>
* A single instance may be reused for checking multiple changes, but not
* concurrently.
*/
public class ConsistencyChecker {
private static final Logger log =
LoggerFactory.getLogger(ConsistencyChecker.class);
private final Provider<ReviewDb> db;
private final GitRepositoryManager repoManager;
private Change change;
private Repository repo;
private RevWalk rw;
private PatchSet currPs;
private RevCommit currPsCommit;
private List<String> messages;
@Inject
ConsistencyChecker(Provider<ReviewDb> db,
GitRepositoryManager repoManager) {
this.db = db;
this.repoManager = repoManager;
reset();
}
private void reset() {
change = null;
repo = null;
rw = null;
messages = new ArrayList<>();
}
public List<String> check(Change c) {
reset();
change = c;
try {
checkImpl();
return messages;
} finally {
if (rw != null) {
rw.release();
}
if (repo != null) {
repo.close();
}
}
}
private void checkImpl() {
checkOwner();
checkCurrentPatchSetEntity();
// All checks that require the repo.
if (!openRepo()) {
return;
}
if (!checkPatchSets()) {
return;
}
checkMerged();
}
private void checkOwner() {
try {
if (db.get().accounts().get(change.getOwner()) == null) {
messages.add("Missing change owner: " + change.getOwner());
}
} catch (OrmException e) {
error("Failed to look up owner", e);
}
}
private void checkCurrentPatchSetEntity() {
try {
PatchSet.Id psId = change.currentPatchSetId();
currPs = db.get().patchSets().get(psId);
if (currPs == null) {
messages.add(String.format("Current patch set %d not found", psId.get()));
}
} catch (OrmException e) {
error("Failed to look up current patch set", e);
}
}
private boolean openRepo() {
Project.NameKey project = change.getDest().getParentKey();
try {
repo = repoManager.openRepository(project);
rw = new RevWalk(repo);
return true;
} catch (RepositoryNotFoundException e) {
return error("Destination repository not found: " + project, e);
} catch (IOException e) {
return error("Failed to open repository: " + project, e);
}
}
private boolean checkPatchSets() {
List<PatchSet> all;
try {
all = db.get().patchSets().byChange(change.getId()).toList();
} catch (OrmException e) {
return error("Failed to look up patch sets", e);
}
Function<PatchSet, Integer> toPsId = new Function<PatchSet, Integer>() {
@Override
public Integer apply(PatchSet in) {
return in.getId().get();
}
};
Multimap<ObjectId, PatchSet> bySha = MultimapBuilder.hashKeys(all.size())
.treeSetValues(Ordering.natural().onResultOf(toPsId))
.build();
for (PatchSet ps : all) {
ObjectId objId;
String rev = ps.getRevision().get();
int psNum = ps.getId().get();
try {
objId = ObjectId.fromString(rev);
} catch (IllegalArgumentException e) {
error(String.format("Invalid revision on patch set %d: %s", psNum, rev),
e);
continue;
}
bySha.put(objId, ps);
RevCommit psCommit = parseCommit(
objId, String.format("patch set %d", psNum));
if (psCommit == null) {
continue;
}
if (ps.getId().equals(change.currentPatchSetId())) {
currPsCommit = psCommit;
}
}
for (Map.Entry<ObjectId, Collection<PatchSet>> e
: bySha.asMap().entrySet()) {
if (e.getValue().size() > 1) {
messages.add(String.format("Multiple patch sets pointing to %s: %s",
e.getKey().name(),
Collections2.transform(e.getValue(), toPsId)));
}
}
return currPs != null && currPsCommit != null;
}
private void checkMerged() {
String refName = change.getDest().get();
Ref dest;
try {
dest = repo.getRef(refName);
} catch (IOException e) {
messages.add("Failed to look up destination ref: " + refName);
return;
}
if (dest == null) {
messages.add("Destination ref not found (may be new branch): "
+ change.getDest().get());
return;
}
RevCommit tip = parseCommit(dest.getObjectId(),
"destination ref " + refName);
if (tip == null) {
return;
}
boolean merged;
try {
merged = rw.isMergedInto(currPsCommit, tip);
} catch (IOException e) {
messages.add("Error checking whether patch set " + currPs.getId().get()
+ " is merged");
return;
}
if (merged && change.getStatus() != Change.Status.MERGED) {
messages.add(String.format("Patch set %d (%s) is merged into destination"
+ " ref %s (%s), but change status is %s", currPs.getId().get(),
currPsCommit.name(), refName, tip.name(), change.getStatus()));
// TODO(dborowitz): Just fix it.
} else if (!merged && change.getStatus() == Change.Status.MERGED) {
messages.add(String.format("Patch set %d (%s) is not merged into"
+ " destination ref %s (%s), but change status is %s",
currPs.getId().get(), currPsCommit.name(), refName, tip.name(),
change.getStatus()));
}
}
private RevCommit parseCommit(ObjectId objId, String desc) {
try {
return rw.parseCommit(objId);
} catch (MissingObjectException e) {
messages.add(String.format("Object missing: %s: %s", desc, objId.name()));
} catch (IncorrectObjectTypeException e) {
messages.add(String.format("Not a commit: %s: %s", desc, objId.name()));
} catch (IOException e) {
messages.add(String.format("Failed to look up: %s: %s", desc, objId.name()));
}
return null;
}
private boolean error(String msg, Throwable t) {
messages.add(msg);
// TODO(dborowitz): Expose stack trace to administrators.
log.warn("Error in consistency check of change " + change.getId(), t);
return false;
}
}

View File

@@ -52,6 +52,7 @@ public class Module extends RestApiModule {
get(CHANGE_KIND, "topic").to(GetTopic.class);
get(CHANGE_KIND, "in").to(IncludedIn.class);
get(CHANGE_KIND, "hashtags").to(GetHashtags.class);
get(CHANGE_KIND, "check").to(Check.class);
put(CHANGE_KIND, "topic").to(PutTopic.class);
delete(CHANGE_KIND, "topic").to(PutTopic.class);
delete(CHANGE_KIND).to(DeleteDraftChange.class);

View File

@@ -0,0 +1,242 @@
// Copyright (C) 2014 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.server.change;
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.testutil.TestChanges.incrementPatchSet;
import static com.google.gerrit.testutil.TestChanges.newChange;
import static com.google.gerrit.testutil.TestChanges.newPatchSet;
import static java.util.Collections.singleton;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.testutil.InMemoryDatabase;
import com.google.gerrit.testutil.InMemoryRepositoryManager;
import com.google.inject.util.Providers;
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.RefUpdate;
import org.eclipse.jgit.revwalk.RevCommit;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
public class ConsistencyCheckerTest {
private InMemoryDatabase schemaFactory;
private ReviewDb db;
private InMemoryRepositoryManager repoManager;
private ConsistencyChecker checker;
private TestRepository<InMemoryRepository> repo;
private Project.NameKey project;
private Account.Id userId;
private RevCommit tip;
@Before
public void setUp() throws Exception {
schemaFactory = InMemoryDatabase.newDatabase();
schemaFactory.create();
db = schemaFactory.open();
repoManager = new InMemoryRepositoryManager();
checker = new ConsistencyChecker(Providers.<ReviewDb> of(db), repoManager);
project = new Project.NameKey("repo");
repo = new TestRepository<>(repoManager.createRepository(project));
userId = new Account.Id(1);
db.accounts().insert(singleton(new Account(userId, TimeUtil.nowTs())));
tip = repo.branch("master").commit().create();
}
@After
public void tearDown() throws Exception {
if (db != null) {
db.close();
}
if (schemaFactory != null) {
InMemoryDatabase.drop(schemaFactory);
}
}
@Test
public void validNewChange() throws Exception {
Change c = newChange(project, userId);
db.changes().insert(singleton(c));
RevCommit commit1 = repo.branch(c.currentPatchSetId().toRefName()).commit()
.parent(tip).create();
PatchSet ps1 = newPatchSet(c.currentPatchSetId(), commit1, userId);
db.patchSets().insert(singleton(ps1));
incrementPatchSet(c);
RevCommit commit2 = repo.branch(c.currentPatchSetId().toRefName()).commit()
.parent(tip).create();
PatchSet ps2 = newPatchSet(c.currentPatchSetId(), commit2, userId);
db.patchSets().insert(singleton(ps2));
assertThat(checker.check(c)).isEmpty();
}
@Test
public void validMergedChange() throws Exception {
Change c = newChange(project, userId);
c.setStatus(Change.Status.MERGED);
db.changes().insert(singleton(c));
RevCommit commit1 = repo.branch(c.currentPatchSetId().toRefName()).commit()
.parent(tip).create();
PatchSet ps1 = newPatchSet(c.currentPatchSetId(), commit1, userId);
db.patchSets().insert(singleton(ps1));
incrementPatchSet(c);
RevCommit commit2 = repo.branch(c.currentPatchSetId().toRefName()).commit()
.parent(tip).create();
PatchSet ps2 = newPatchSet(c.currentPatchSetId(), commit2, userId);
db.patchSets().insert(singleton(ps2));
repo.branch(c.getDest().get()).update(commit2);
assertThat(checker.check(c)).isEmpty();
}
@Test
public void missingOwner() throws Exception {
Change c = newChange(project, new Account.Id(2));
db.changes().insert(singleton(c));
RevCommit commit = repo.branch(c.currentPatchSetId().toRefName()).commit()
.parent(tip).create();
PatchSet ps = newPatchSet(c.currentPatchSetId(), commit, userId);
db.patchSets().insert(singleton(ps));
assertThat(checker.check(c)).containsExactly("Missing change owner: 2");
}
@Test
public void missingRepo() throws Exception {
Change c = newChange(new Project.NameKey("otherproject"), userId);
db.changes().insert(singleton(c));
PatchSet ps = newPatchSet(c.currentPatchSetId(),
ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"), userId);
db.patchSets().insert(singleton(ps));
assertThat(checker.check(c))
.containsExactly("Destination repository not found: otherproject");
}
@Test
public void invalidRevision() throws Exception {
Change c = newChange(project, userId);
db.changes().insert(singleton(c));
PatchSet ps = new PatchSet(c.currentPatchSetId());
ps.setRevision(new RevId("fooooooooooooooooooooooooooooooooooooooo"));
ps.setUploader(userId);
ps.setCreatedOn(TimeUtil.nowTs());
db.patchSets().insert(singleton(ps));
incrementPatchSet(c);
RevCommit commit2 = repo.branch(c.currentPatchSetId().toRefName()).commit()
.parent(tip).create();
PatchSet ps2 = newPatchSet(c.currentPatchSetId(), commit2, userId);
db.patchSets().insert(singleton(ps2));
assertThat(checker.check(c)).containsExactly(
"Invalid revision on patch set 1:"
+ " fooooooooooooooooooooooooooooooooooooooo");
}
@Test
public void patchSetObjectMissing() throws Exception {
Change c = newChange(project, userId);
db.changes().insert(singleton(c));
PatchSet ps = newPatchSet(c.currentPatchSetId(),
ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"), userId);
db.patchSets().insert(singleton(ps));
assertThat(checker.check(c)).containsExactly(
"Object missing: patch set 1: deadbeefdeadbeefdeadbeefdeadbeefdeadbeef");
}
@Test
public void currentPatchSetMissing() throws Exception {
Change c = newChange(project, userId);
db.changes().insert(singleton(c));
assertThat(checker.check(c))
.containsExactly("Current patch set 1 not found");
}
@Test
public void duplicatePatchSetRevisions() throws Exception {
Change c = newChange(project, userId);
db.changes().insert(singleton(c));
RevCommit commit1 = repo.branch(c.currentPatchSetId().toRefName()).commit()
.parent(tip).create();
PatchSet ps1 = newPatchSet(c.currentPatchSetId(), commit1, userId);
db.patchSets().insert(singleton(ps1));
incrementPatchSet(c);
PatchSet ps2 = newPatchSet(c.currentPatchSetId(), commit1, userId);
db.patchSets().insert(singleton(ps2));
assertThat(checker.check(c)).containsExactly("Multiple patch sets pointing to "
+ commit1.name() + ": [1, 2]");
}
@Test
public void missingDestRef() throws Exception {
RefUpdate ru = repo.getRepository().updateRef("refs/heads/master");
ru.setForceUpdate(true);
assertThat(ru.delete()).isEqualTo(RefUpdate.Result.FORCED);
Change c = newChange(project, userId);
db.changes().insert(singleton(c));
RevCommit commit = repo.commit().create();
PatchSet ps = newPatchSet(c.currentPatchSetId(), commit, userId);
db.patchSets().insert(singleton(ps));
assertThat(checker.check(c)).containsExactly(
"Destination ref not found (may be new branch): master");
}
@Test
public void mergedChangeIsNotMerged() throws Exception {
Change c = newChange(project, userId);
c.setStatus(Change.Status.MERGED);
db.changes().insert(singleton(c));
RevCommit commit = repo.branch(c.currentPatchSetId().toRefName()).commit()
.parent(tip).create();
PatchSet ps = newPatchSet(c.currentPatchSetId(), commit, userId);
db.patchSets().insert(singleton(ps));
assertThat(checker.check(c)).containsExactly(
"Patch set 1 (" + commit.name() + ") is not merged into destination ref"
+ " master (" + tip.name() + "), but change status is MERGED");
}
@Test
public void newChangeIsMerged() throws Exception {
Change c = newChange(project, userId);
db.changes().insert(singleton(c));
RevCommit commit = repo.branch(c.currentPatchSetId().toRefName()).commit()
.parent(tip).create();
PatchSet ps = newPatchSet(c.currentPatchSetId(), commit, userId);
db.patchSets().insert(singleton(ps));
repo.branch(c.getDest().get()).update(commit);
assertThat(checker.check(c)).containsExactly(
"Patch set 1 (" + commit.name() + ") is merged into destination ref"
+ " master (" + commit.name() + "), but change status is NEW");
}
}

View File

@@ -24,6 +24,7 @@ import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetInfo;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.config.AllUsersName;
@@ -39,6 +40,7 @@ import com.google.gwtorm.server.OrmException;
import com.google.inject.Injector;
import org.easymock.EasyMock;
import org.eclipse.jgit.lib.ObjectId;
import java.util.concurrent.atomic.AtomicInteger;
@@ -66,6 +68,15 @@ public class TestChanges {
return c;
}
public static PatchSet newPatchSet(PatchSet.Id id, ObjectId revision,
Account.Id userId) {
PatchSet ps = new PatchSet(id);
ps.setRevision(new RevId(revision.name()));
ps.setUploader(userId);
ps.setCreatedOn(TimeUtil.nowTs());
return ps;
}
public static ChangeUpdate newUpdate(Injector injector,
GitRepositoryManager repoManager, NotesMigration migration, Change c,
final AllUsersNameProvider allUsers, final IdentifiedUser user)