From 7854219e4808db37fbc6cd9f50bce64c4910112b Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Thu, 31 Aug 2017 10:45:47 -0400 Subject: [PATCH] Add limit to number of commits that ReceiveCommits will validate Compared with simply walking the commits with JGit, running Gerrit commit validators is quite expensive. When pushing a large number of commits directly to a branch, validation may exceed the timeout allowed by AsyncReceiveCommits. For example, pushing the full Linux kernel history of 650k commits allows only 370 microseconds of validation time per commit, if validation is allowed to take up the full 4 minute default AsyncReceiveCommits limit. Gerrit's validators have never been particularly optimized, so it wouldn't be entirely surprising to see a timeout in this case, particularly if the Gerrit server is under moderate to heavy load. Add a limit configured with receive.maxBatchCommits, analogous to the existing receive.maxBatchChanges. The options are still separate: maxBatchChanges is about creating changes, which is a far more heavyweight operation as it needs to write change metadata, and accidentally pushing too many changes is a bigger mess to clean up. Change-Id: I4b81b1f99d9dafdc365ff66e0fb812877355e3b9 --- Documentation/config-gerrit.txt | 9 ++++ Documentation/error-messages.txt | 1 + Documentation/error-too-many-commits.txt | 20 +++++++ Documentation/user-upload.txt | 5 ++ .../acceptance/git/AbstractPushForReview.java | 52 ++++++++++++++++++- .../server/git/receive/ReceiveCommits.java | 19 +++++-- .../server/git/receive/ReceiveConfig.java | 2 + 7 files changed, 103 insertions(+), 5 deletions(-) create mode 100644 Documentation/error-too-many-commits.txt diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index 6bcf8512fb..a3a813ed92 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -3541,6 +3541,15 @@ number of changes for review by mistake. + Default is zero, no limit. +[[receive.maxBatchCommits]]receive.maxBatchCommits:: ++ +The maximum number of commits that Gerrit allows to be pushed in a batch +directly to a branch when link:user-upload.html#bypass_review[bypassing review]. +This limit can be bypassed if a user link:user-upload.html#skip_validation[skips +validation]. ++ +Default is 10000. + [[receive.maxObjectSizeLimit]]receive.maxObjectSizeLimit:: + Maximum allowed Git object size that 'receive-pack' will accept. diff --git a/Documentation/error-messages.txt b/Documentation/error-messages.txt index 2632254fe4..ca8dc75251 100644 --- a/Documentation/error-messages.txt +++ b/Documentation/error-messages.txt @@ -32,6 +32,7 @@ occurring and what can be done to solve it. * link:error-prohibited-by-gerrit.html[prohibited by Gerrit] * link:error-project-not-found.html[Project not found: ...] * link:error-same-change-id-in-multiple-changes.html[same Change-Id in multiple changes] +* link:error-too-many-commits.html[too many commits] * link:error-upload-denied.html[Upload denied for project \'...'] * link:error-not-allowed-to-upload-merges.html[you are not allowed to upload merges] diff --git a/Documentation/error-too-many-commits.txt b/Documentation/error-too-many-commits.txt new file mode 100644 index 0000000000..3e16220f23 --- /dev/null +++ b/Documentation/error-too-many-commits.txt @@ -0,0 +1,20 @@ += too many commits + +This error occurs when a push directly to a branch +link:user-upload.html#bypass_review[bypassing review] contains more commits than +the server is able to validate in a single batch. + +The recommended way to avoid this message is to use the +link:user-upload.html#skip_validation[`skip-validation` push option]. Depending +on the number of commits, it may also be feasible to split the push into smaller +batches. + +The actual limit is controlled by a +link:config-gerrit.html#receive.maxBatchCommits[server config option]. + +GERRIT +------ +Part of link:error-messages.html[Gerrit Error Messages] + +SEARCHBOX +--------- diff --git a/Documentation/user-upload.txt b/Documentation/user-upload.txt index 47e08c5180..556a8bbe35 100644 --- a/Documentation/user-upload.txt +++ b/Documentation/user-upload.txt @@ -549,6 +549,11 @@ This option only applies when pushing directly to a branch bypassing review. Validation also occurs when pushing new changes for review, and that type of validation cannot be skipped. +The `skip-validation` option is always required when pushing +link:error-too-many-commits.html[more than a certain number of commits]. This is +the recommended approach when pushing lots of old history, since some validators +would require rewriting history in order to make them pass. + [[auto_merge]] === Auto-Merge during Push diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java index 6face43fae..0dc1389664 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java @@ -23,10 +23,12 @@ import static com.google.gerrit.acceptance.GitUtil.pushHead; import static com.google.gerrit.acceptance.PushOneCommit.FILE_NAME; import static com.google.gerrit.common.FooterConstants.CHANGE_ID; import static com.google.gerrit.extensions.common.EditInfoSubject.assertThat; +import static com.google.gerrit.server.git.receive.ReceiveConstants.PUSH_OPTION_SKIP_VALIDATION; import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS; import static com.google.gerrit.server.project.Util.category; import static com.google.gerrit.server.project.Util.value; import static java.util.concurrent.TimeUnit.SECONDS; +import static java.util.stream.Collectors.joining; import static java.util.stream.Collectors.toList; import com.google.common.collect.ImmutableList; @@ -35,6 +37,7 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.collect.Streams; import com.google.gerrit.acceptance.AbstractDaemonTest; +import com.google.gerrit.acceptance.GerritConfig; import com.google.gerrit.acceptance.GitUtil; import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.TestAccount; @@ -62,10 +65,12 @@ import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.ChangeMessage; import com.google.gerrit.reviewdb.client.PatchSet; +import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.ChangeMessagesUtil; import com.google.gerrit.server.git.ProjectConfig; import com.google.gerrit.server.git.receive.ReceiveConstants; +import com.google.gerrit.server.group.SystemGroupBackend; import com.google.gerrit.server.mail.Address; import com.google.gerrit.server.project.Util; import com.google.gerrit.server.query.change.ChangeData; @@ -1766,6 +1771,29 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { assertThat(getPublishedComments(r.getChangeId())).isEmpty(); } + @GerritConfig(name = "receive.maxBatchCommits", value = "2") + @Test + public void maxBatchCommits() throws Exception { + List commits = new ArrayList<>(); + commits.addAll(initChanges(2)); + String master = "refs/heads/master"; + assertPushOk(pushHead(testRepo, master), master); + + commits.addAll(initChanges(3)); + assertPushRejected(pushHead(testRepo, master), master, "too many commits"); + + grantSkipValidation(project, master, SystemGroupBackend.REGISTERED_USERS); + PushResult r = + pushHead(testRepo, master, false, false, ImmutableList.of(PUSH_OPTION_SKIP_VALIDATION)); + assertPushOk(r, master); + + // No open changes; branch was advanced. + String q = commits.stream().map(ObjectId::name).collect(joining(" OR commit:", "commit:", "")); + assertThat(gApi.changes().query(q).get()).isEmpty(); + assertThat(gApi.projects().name(project.get()).branch(master).get().revision) + .isEqualTo(Iterables.getLast(commits).name()); + } + private DraftInput newDraft(String path, int line, String message) { DraftInput d = new DraftInput(); d.path = path; @@ -1839,11 +1867,21 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { } private List createChanges(int n, String refsFor) throws Exception { - return createChanges(n, refsFor, ImmutableList.of()); + return createChanges(n, refsFor, ImmutableList.of()); } private List createChanges(int n, String refsFor, List footerLines) throws Exception { + List commits = initChanges(n, footerLines); + assertPushOk(pushHead(testRepo, refsFor, false), refsFor); + return commits; + } + + private List initChanges(int n) throws Exception { + return initChanges(n, ImmutableList.of()); + } + + private List initChanges(int n, List footerLines) throws Exception { List commits = new ArrayList<>(n); for (int i = 1; i <= n; i++) { String msg = "Change " + i; @@ -1863,7 +1901,6 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { testRepo.getRevWalk().parseBody(c); commits.add(c); } - assertPushOk(pushHead(testRepo, refsFor, false), refsFor); return commits; } @@ -1934,4 +1971,15 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { config.getProject().setCreateNewChangeForAllNotInTarget(InheritableBoolean.TRUE); saveProjectConfig(project, config); } + + private void grantSkipValidation(Project.NameKey project, String ref, AccountGroup.UUID groupUuid) + throws Exception { + // See SKIP_VALIDATION implementation in default permission backend. + ProjectConfig config = projectCache.checkedGet(project).getConfig(); + Util.allow(config, Permission.FORGE_AUTHOR, groupUuid, ref); + Util.allow(config, Permission.FORGE_COMMITTER, groupUuid, ref); + Util.allow(config, Permission.FORGE_SERVER, groupUuid, ref); + Util.allow(config, Permission.PUSH_MERGE, groupUuid, "refs/for/" + ref); + saveProjectConfig(project, config); + } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java index 6a04174f49..e11cdf7095 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java @@ -2710,9 +2710,22 @@ class ReceiveCommits { ListMultimap existing = changeRefsById(); walk.markStart((RevCommit) parsedObject); markHeadsAsUninteresting(walk, cmd.getRefName()); - int i = 0; + int limit = receiveConfig.maxBatchCommits; + int n = 0; for (RevCommit c; (c = walk.next()) != null; ) { - i++; + if (++n > limit) { + logDebug("Number of new commits exceeds limit of {}", limit); + addMessage( + "Cannot push more than " + + limit + + " commits to " + + branch.get() + + " without " + + PUSH_OPTION_SKIP_VALIDATION + + " option"); + reject(cmd, "too many commits"); + return; + } if (existing.keySet().contains(c)) { continue; } else if (!validCommit(walk, perm, branch, cmd, c)) { @@ -2725,7 +2738,7 @@ class ReceiveCommits { missingFullName = false; } } - logDebug("Validated {} new commits", i); + logDebug("Validated {} new commits", n); } catch (IOException err) { cmd.setResult(REJECTED_MISSING_OBJECT); logError("Invalid pack upload; one or more objects weren't sent", err); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveConfig.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveConfig.java index 7be6dccec6..30d5071f43 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveConfig.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveConfig.java @@ -28,6 +28,7 @@ class ReceiveConfig { final boolean checkMagicRefs; final boolean checkReferencedObjectsAreReachable; final boolean allowDrafts; + final int maxBatchCommits; private final int systemMaxBatchChanges; private final AccountLimits.Factory limitsFactory; @@ -37,6 +38,7 @@ class ReceiveConfig { checkReferencedObjectsAreReachable = config.getBoolean("receive", null, "checkReferencedObjectsAreReachable", true); allowDrafts = config.getBoolean("change", null, "allowDrafts", true); + maxBatchCommits = config.getInt("receive", null, "maxBatchCommits", 10000); systemMaxBatchChanges = config.getInt("receive", "maxBatchChanges", 0); this.limitsFactory = limitsFactory; }