opendev/gerrit
Code Issues Proposed changes

Split global consistency checks from the per-group sanity checks

Browse Source 
For the NoteDb migration, we are more interested in the former
problems, and it is unclear if our bulk migration tooling has the
capacity to contact arbitrary group backends.

Change-Id: I49eac82fb75a0e94148d262a76e82e97a1d3eb8d
This commit is contained in:
Han-Wen Nienhuys
2017-11-23 16:45:13 +01:00
parent 6cafa8c32a
commit 1a4d400a26
3 changed files with 256 additions and 179 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
java/com/google/gerrit/extensions/api/config/ConsistencyCheckInfo.java
View File

@@ -78,5 +78,13 @@ public class ConsistencyCheckInfo {
public String toString() {
return status.name() + ": " + message;
}
public static ConsistencyProblemInfo warning(String fmt, Object... args) {
return new ConsistencyProblemInfo(Status.WARNING, String.format(fmt, args));
}
public static ConsistencyProblemInfo error(String fmt, Object... args) {
return new ConsistencyProblemInfo(Status.ERROR, String.format(fmt, args));
}
}
}

200
java/com/google/gerrit/server/group/db/GroupsConsistencyChecker.java
View File

@@ -14,14 +14,12 @@
package com.google.gerrit.server.group.db;
import com.google.common.collect.BiMap;
import com.google.common.collect.HashBiMap;
import com.google.gerrit.common.data.GroupReference;
import static com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.ConsistencyProblemInfo.error;
import static com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.ConsistencyProblemInfo.warning;
import com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.ConsistencyProblemInfo;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.AccountGroup.NameKey;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.account.Accounts;
import com.google.gerrit.server.account.GroupBackend;
import com.google.gerrit.server.config.AllUsersName;
@@ -29,9 +27,7 @@ import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.group.InternalGroup;
import com.google.gerrit.server.notedb.GroupsMigration;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashSet;
import java.util.List;
@@ -41,34 +37,34 @@ import java.util.Set;
import javax.inject.Inject;
import javax.inject.Singleton;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectLoader;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.notes.Note;
import org.eclipse.jgit.notes.NoteMap;
import org.eclipse.jgit.revwalk.RevWalk;
/** Checks internal consistency of NoteDb storage of groups. */
/**
* Checks individual groups for oddities, such as cycles, non-existent subgroups, etc. Only works if
* we are writing to NoteDb.
*/
@Singleton
public class GroupsConsistencyChecker {
private final AllUsersName allUsersName;
private final GroupBackend groupBackend;
private final Accounts accounts;
private final AllUsersName allUsersName;
private final GitRepositoryManager repoManager;
private final GroupsNoteDbConsistencyChecker globalChecker;
private final GroupsMigration groupsMigration;
@Inject
GroupsConsistencyChecker(
Accounts accounts,
GroupBackend groupBackend,
AllUsersName allUsersName,
GroupBackend groupBackend,
Accounts accounts,
GitRepositoryManager repositoryManager,
GroupsNoteDbConsistencyChecker globalChecker,
GroupsMigration groupsMigration) {
this.accounts = accounts;
this.groupBackend = groupBackend;
this.allUsersName = allUsersName;
this.groupBackend = groupBackend;
this.accounts = accounts;
this.repoManager = repositoryManager;
this.globalChecker = globalChecker;
this.groupsMigration = groupsMigration;
}
@@ -79,89 +75,17 @@ public class GroupsConsistencyChecker {
}
try (Repository repo = repoManager.openRepository(allUsersName)) {
return check(repo);
}
}
public List<ConsistencyProblemInfo> check(Repository repo) throws IOException {
// Get all refs in an attempt to avoid seeing half committed group updates.
Map<String, Ref> refs = repo.getAllRefs();
List<ConsistencyProblemInfo> problems = new ArrayList<>();
Map<AccountGroup.UUID, InternalGroup> byUUID = new HashMap<>();
BiMap<AccountGroup.UUID, String> nameMap = HashBiMap.create();
readGroups(repo, refs, problems, byUUID);
readGroupNames(repo, refs, problems, nameMap);
// The sequential IDs are not keys in NoteDb, so no need to check them.
// No use continuing if we couldn't read the data.
if (!problems.isEmpty()) {
return problems;
}
problems = checkGlobalConsistency(byUUID, nameMap);
if (!problems.isEmpty()) {
return problems;
}
// Check subgroups and members.
for (InternalGroup g : byUUID.values()) {
problems.addAll(checkGroup(g, byUUID));
}
return problems;
}
/** Check invariants of the group refs with the groupname refs. */
private List<ConsistencyProblemInfo> checkGlobalConsistency(
Map<AccountGroup.UUID, InternalGroup> byUUID, BiMap<AccountGroup.UUID, String> nameMap) {
List<ConsistencyProblemInfo> problems = new ArrayList<>();
// Check consistency between the data coming from different refs.
for (AccountGroup.UUID uuid : byUUID.keySet()) {
if (!nameMap.containsKey(uuid)) {
problems.add(error("group %s has no entry in name map", uuid));
continue;
GroupsNoteDbConsistencyChecker.Result result = globalChecker.check(repo);
if (!result.problems.isEmpty()) {
return result.problems;
}
String noteName = nameMap.get(uuid);
String groupRefName = byUUID.get(uuid).getName();
if (!Objects.equals(noteName, groupRefName)) {
problems.add(
error(
"inconsistent name for group %s (name map %s vs. group ref %s)",
uuid, noteName, groupRefName));
for (InternalGroup g : result.uuidToGroupMap.values()) {
result.problems.addAll(checkGroup(g, result.uuidToGroupMap));
}
}
for (AccountGroup.UUID uuid : nameMap.keySet()) {
if (!byUUID.containsKey(uuid)) {
problems.add(
error(
"name map has entry (%s, %s), entry missing as group ref",
uuid, nameMap.get(uuid)));
}
return result.problems;
}
// No use delving further into inconsistent data.
if (!problems.isEmpty()) {
return problems;
}
// Check ids.
Map<AccountGroup.Id, InternalGroup> groupById = new HashMap<>();
for (InternalGroup g : byUUID.values()) {
InternalGroup before = groupById.get(g.getId());
if (before != null) {
problems.add(
error(
"shared group id %s for %s (%s) and %s (%s)",
g.getId(), before.getName(), before.getGroupUUID(), g.getName(), g.getGroupUUID()));
}
groupById.put(g.getId(), g);
}
return problems;
}
/** Checks the metadata for a single group for problems. */
@@ -207,78 +131,6 @@ public class GroupsConsistencyChecker {
return problems;
}
private void readGroupNames(
Repository repo,
Map<String, Ref> refs,
List<ConsistencyProblemInfo> problems,
BiMap<AccountGroup.UUID, String> result)
throws IOException {
Ref ref = refs.get(RefNames.REFS_GROUPNAMES);
if (ref == null) {
problems.add(
new ConsistencyProblemInfo(
groupsMigration.readFromNoteDb()
? ConsistencyProblemInfo.Status.ERROR
: ConsistencyProblemInfo.Status.WARNING,
String.format("ref %s does not exist", RefNames.REFS_GROUPNAMES)));
return;
}
try (RevWalk rw = new RevWalk(repo)) {
NoteMap nm = NoteMap.read(rw.getObjectReader(), rw.parseCommit(ref.getObjectId()));
for (Note note : nm) {
ObjectLoader ld = rw.getObjectReader().open(note.getData());
byte[] data = ld.getCachedBytes();
GroupReference gRef;
try {
gRef = GroupNameNotes.getFromNoteData(data);
} catch (ConfigInvalidException e) {
problems.add(
error(
"notename entry %s: %s does not parse: %s",
note, new String(data, StandardCharsets.UTF_8), e.getMessage()));
continue;
}
ObjectId nameKey = GroupNameNotes.getNoteKey(new NameKey(gRef.getName()));
if (!Objects.equals(nameKey, note)) {
problems.add(error("notename entry %s does not match name %s", note, gRef.getName()));
}
// We trust SHA1 to have no collisions, so no need to check uniqueness of name.
result.put(gRef.getUUID(), gRef.getName());
}
}
}
private void readGroups(
Repository repo,
Map<String, Ref> refs,
List<ConsistencyProblemInfo> problems,
Map<AccountGroup.UUID, InternalGroup> byUUID)
throws IOException {
for (Map.Entry<String, Ref> entry : refs.entrySet()) {
if (!entry.getKey().startsWith(RefNames.REFS_GROUPS)) {
continue;
}
AccountGroup.UUID uuid = AccountGroup.UUID.fromRef(entry.getKey());
if (uuid == null) {
problems.add(error("null UUID from %s", entry.getKey()));
continue;
}
try {
GroupConfig cfg =
GroupConfig.loadForGroupSnapshot(repo, uuid, entry.getValue().getObjectId());
byUUID.put(uuid, cfg.getLoadedGroup().get());
} catch (ConfigInvalidException e) {
problems.add(error("group %s does not parse: %s", uuid, e.getMessage()));
}
}
}
/** checkCycle walks through root's subgroups recursively, and checks for cycles. */
private List<ConsistencyProblemInfo> checkCycle(
InternalGroup root, Map<AccountGroup.UUID, InternalGroup> byUUID) {
@@ -315,14 +167,4 @@ public class GroupsConsistencyChecker {
}
return problems;
}
private ConsistencyProblemInfo warning(String fmt, Object... args) {
return new ConsistencyProblemInfo(
ConsistencyProblemInfo.Status.WARNING, String.format(fmt, args));
}
private ConsistencyProblemInfo error(String fmt, Object... args) {
return new ConsistencyProblemInfo(
ConsistencyProblemInfo.Status.ERROR, String.format(fmt, args));
}
}

227
java/com/google/gerrit/server/group/db/GroupsNoteDbConsistencyChecker.java Normal file
View File

@@ -0,0 +1,227 @@
// 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.server.group.db;
import static com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.ConsistencyProblemInfo.error;
import static com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.ConsistencyProblemInfo.warning;
import com.google.common.collect.BiMap;
import com.google.common.collect.HashBiMap;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.GroupReference;
import com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.ConsistencyProblemInfo;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.group.InternalGroup;
import com.google.gerrit.server.notedb.GroupsMigration;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import javax.inject.Inject;
import javax.inject.Singleton;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectLoader;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.notes.Note;
import org.eclipse.jgit.notes.NoteMap;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
/** Check the referential integrity of NoteDb group storage. */
@Singleton
public class GroupsNoteDbConsistencyChecker {
private final GroupsMigration groupsMigration;
/**
* The result of a consistency check. The UUID map is only non-null if no problems were detected.
*/
public static class Result {
public List<ConsistencyProblemInfo> problems;
@Nullable public Map<AccountGroup.UUID, InternalGroup> uuidToGroupMap;
}
@Inject
GroupsNoteDbConsistencyChecker(GroupsMigration groupsMigration) {
this.groupsMigration = groupsMigration;
}
/**
* Checks for problems with the given All-Users repo. Returns null if we are not writing to
* NoteDb.
*/
@Nullable
public Result check(Repository repo) throws IOException {
if (!groupsMigration.writeToNoteDb()) {
return null;
}
Result r = doCheck(repo);
if (!r.problems.isEmpty()) {
r.uuidToGroupMap = null;
}
return r;
}
@Nullable
private Result doCheck(Repository repo) throws IOException {
Result result = new Result();
result.problems = new ArrayList<>();
result.uuidToGroupMap = new HashMap<>();
BiMap<AccountGroup.UUID, String> uuidNameBiMap = HashBiMap.create();
// Get all refs in an attempt to avoid seeing half committed group updates.
Map<String, Ref> refs = repo.getAllRefs();
readGroups(repo, refs, result);
readGroupNames(repo, refs, result, uuidNameBiMap);
// The sequential IDs are not keys in NoteDb, so no need to check them.
if (!result.problems.isEmpty()) {
return result;
}
// Continue checking if we could read data without problems.
result.problems.addAll(checkGlobalConsistency(result.uuidToGroupMap, uuidNameBiMap));
return result;
}
private void readGroups(Repository repo, Map<String, Ref> refs, Result result)
throws IOException {
for (Map.Entry<String, Ref> entry : refs.entrySet()) {
if (!entry.getKey().startsWith(RefNames.REFS_GROUPS)) {
continue;
}
AccountGroup.UUID uuid = AccountGroup.UUID.fromRef(entry.getKey());
if (uuid == null) {
result.problems.add(error("null UUID from %s", entry.getKey()));
continue;
}
try {
GroupConfig cfg =
GroupConfig.loadForGroupSnapshot(repo, uuid, entry.getValue().getObjectId());
result.uuidToGroupMap.put(uuid, cfg.getLoadedGroup().get());
} catch (ConfigInvalidException e) {
result.problems.add(error("group %s does not parse: %s", uuid, e.getMessage()));
}
}
}
private void readGroupNames(
Repository repo,
Map<String, Ref> refs,
Result result,
BiMap<AccountGroup.UUID, String> uuidNameBiMap)
throws IOException {
Ref ref = refs.get(RefNames.REFS_GROUPNAMES);
if (ref == null) {
String msg = String.format("ref %s does not exist", RefNames.REFS_GROUPNAMES);
result.problems.add(groupsMigration.readFromNoteDb() ? error(msg) : warning(msg));
return;
}
try (RevWalk rw = new RevWalk(repo)) {
RevCommit c = rw.parseCommit(ref.getObjectId());
NoteMap nm = NoteMap.read(rw.getObjectReader(), c);
for (Note note : nm) {
ObjectLoader ld = rw.getObjectReader().open(note.getData());
byte[] data = ld.getCachedBytes();
GroupReference gRef;
try {
gRef = GroupNameNotes.getFromNoteData(data);
} catch (ConfigInvalidException e) {
result.problems.add(
error(
"notename entry %s: %s does not parse: %s",
note, new String(data, StandardCharsets.UTF_8), e.getMessage()));
continue;
}
ObjectId nameKey = GroupNameNotes.getNoteKey(new AccountGroup.NameKey(gRef.getName()));
if (!Objects.equals(nameKey, note)) {
result.problems.add(
error("notename entry %s does not match name %s", note, gRef.getName()));
}
// We trust SHA1 to have no collisions, so no need to check uniqueness of name.
uuidNameBiMap.put(gRef.getUUID(), gRef.getName());
}
}
}
/** Check invariants of the group refs with the group name refs. */
private List<ConsistencyProblemInfo> checkGlobalConsistency(
Map<AccountGroup.UUID, InternalGroup> uuidToGroupMap,
BiMap<AccountGroup.UUID, String> uuidNameBiMap) {
List<ConsistencyProblemInfo> problems = new ArrayList<>();
// Check consistency between the data coming from different refs.
for (AccountGroup.UUID uuid : uuidToGroupMap.keySet()) {
if (!uuidNameBiMap.containsKey(uuid)) {
problems.add(error("group %s has no entry in name map", uuid));
continue;
}
String noteName = uuidNameBiMap.get(uuid);
String groupRefName = uuidToGroupMap.get(uuid).getName();
if (!Objects.equals(noteName, groupRefName)) {
problems.add(
error(
"inconsistent name for group %s (name map %s vs. group ref %s)",
uuid, noteName, groupRefName));
}
}
for (AccountGroup.UUID uuid : uuidNameBiMap.keySet()) {
if (!uuidToGroupMap.containsKey(uuid)) {
problems.add(
error(
"name map has entry (%s, %s), entry missing as group ref",
uuid, uuidNameBiMap.get(uuid)));
}
}
if (problems.isEmpty()) {
// Check ids.
Map<AccountGroup.Id, InternalGroup> groupById = new HashMap<>();
for (InternalGroup g : uuidToGroupMap.values()) {
InternalGroup before = groupById.get(g.getId());
if (before != null) {
problems.add(
error(
"shared group id %s for %s (%s) and %s (%s)",
g.getId(),
before.getName(),
before.getGroupUUID(),
g.getName(),
g.getGroupUUID()));
}
groupById.put(g.getId(), g);
}
}
return problems;
}
}