Merge branch 'stable-3.10' into stable-3.11 * stable-3.10: Prevent the synchronous replication of create-refs via apply-object Flag ref-updates events creating a new ref Fix the create/delete of ref-specs in FetchOne delta Report deletions as succeeded in the fetch-ref-replicated events Change-Id: Iab9798f9327e2661e3c937c16ed8bd0b7cb677b3
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/FetchOne.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/FetchOne.java index 5e7fda2..939a05b 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/FetchOne.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/FetchOne.java
@@ -228,14 +228,16 @@ // set may risk to have both update and delete for the same // ref, which would be an issue: the delta is an unordered set // and executing updates and deletes for the same ref may have - // unexpected results, depending on which one you execute firt. - // - // Delete any previous ref-spec as delete or update for the same - // refname so that the subsequent addition will make sure that - // there is only one operation per refName. - delta.remove(ref); + // unexpected results, depending on which one you execute first. + + // Delete any previous ref-spec as delete + delta.remove(FetchRefSpec.fromRef(":" + ref.refName())); + + // Delete any previous ref-spec as update delta.remove(FetchRefSpec.fromRef(ref.refName())); + // Add the new ref as-is for making sure that + // there is only one operation per refName. delta.add(ref); repLog.trace("[{}] Added ref {} for replication from {}", taskIdHex, ref, uri); }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/ReplicationQueue.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/ReplicationQueue.java index 495915d..b94189e 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/ReplicationQueue.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/ReplicationQueue.java
@@ -48,6 +48,7 @@ import com.googlesource.gerrit.plugins.replication.pull.client.FetchRestApiClient; import com.googlesource.gerrit.plugins.replication.pull.client.HttpResult; import com.googlesource.gerrit.plugins.replication.pull.client.HttpResultUtils; +import com.googlesource.gerrit.plugins.replication.pull.filter.ApplyObjectBannedCreateRefsFilter; import com.googlesource.gerrit.plugins.replication.pull.filter.ApplyObjectsRefsFilter; import com.googlesource.gerrit.plugins.replication.pull.filter.ExcludedRefsFilter; import java.io.IOException; @@ -107,6 +108,7 @@ private final String instanceId; private final boolean useBatchUpdateEvents; private ApplyObjectsRefsFilter applyObjectsRefsFilter; + private final ApplyObjectBannedCreateRefsFilter applyObjectsBannedCreateRefsFilter; @Inject ReplicationQueue( @@ -122,6 +124,7 @@ @GerritInstanceId String instanceId, @GerritServerConfig Config gerritConfig, ApplyObjectsRefsFilter applyObjectsRefsFilter, + ApplyObjectBannedCreateRefsFilter applyObjectsBannedCreateRefsFilter, ShutdownState shutdownState) { workQueue = wq; dispatcher = dis; @@ -138,6 +141,7 @@ this.useBatchUpdateEvents = gerritConfig.getBoolean("event", "stream-events", "enableBatchRefUpdatedEvents", false); this.applyObjectsRefsFilter = applyObjectsRefsFilter; + this.applyObjectsBannedCreateRefsFilter = applyObjectsBannedCreateRefsFilter; } @Override @@ -367,8 +371,9 @@ .map(ref -> toBatchApplyObject(project, ref, state)) .collect(Collectors.toList()); - if (!containsLargeOrDeletedRefs(refsBatch)) { - return ((source) -> callBatchSendObject(source, project, refsBatch, eventCreatedOn, state)); + if (!containsLargeOrDeletedRefs(refsBatch) + && !hasCreateRefsBannedFromApplyObject(refsBatch)) { + return (source -> callBatchSendObject(source, project, refsBatch, eventCreatedOn, state)); } } catch (UncheckedIOException e) { stateLog.error("Falling back to calling fetch", e, state); @@ -381,7 +386,8 @@ try { Optional<RevisionData> maybeRevisionData = revReaderProvider.get().read(project, event.objectId(), event.refName(), 0); - return BatchApplyObjectData.create(event.refName(), maybeRevisionData); + return BatchApplyObjectData.create( + event.refName(), maybeRevisionData, event.isDelete(), event.isCreate()); } catch (IOException e) { stateLog.error( String.format( @@ -397,6 +403,13 @@ return batchApplyObjectData.stream().anyMatch(e -> e.revisionData().isEmpty()); } + private boolean hasCreateRefsBannedFromApplyObject( + List<BatchApplyObjectData> batchApplyObjectData) { + return batchApplyObjectData.stream() + .filter(BatchApplyObjectData::isCreate) + .anyMatch(e -> applyObjectsBannedCreateRefsFilter.match(e.refName())); + } + private Optional<HttpResult> callSendObject( FetchApiClient fetchClient, String remoteName, @@ -820,9 +833,10 @@ String refName, ObjectId objectId, long eventCreatedOn, - boolean isDelete) { + boolean isDelete, + boolean isCreate) { return new AutoValue_ReplicationQueue_ReferenceUpdatedEvent( - projectName, refName, objectId, eventCreatedOn, isDelete); + projectName, refName, objectId, eventCreatedOn, isDelete, isCreate); } static ReferenceUpdatedEvent from(RefUpdateAttribute refUpdateAttribute, long eventCreatedOn) { @@ -831,7 +845,8 @@ refUpdateAttribute.refName, ObjectId.fromString(refUpdateAttribute.newRev), eventCreatedOn, - ZEROS_OBJECTID.equals(refUpdateAttribute.newRev)); + ZEROS_OBJECTID.equals(refUpdateAttribute.newRev), + ZEROS_OBJECTID.equals(refUpdateAttribute.oldRev)); } public abstract String projectName(); @@ -843,6 +858,8 @@ public abstract long eventCreatedOn(); public abstract boolean isDelete(); + + public abstract boolean isCreate(); } @FunctionalInterface
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/DeleteRefCommand.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/DeleteRefCommand.java index fe558e5..c3f91d6 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/DeleteRefCommand.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/DeleteRefCommand.java
@@ -73,7 +73,7 @@ name.get(), r, sourceLabel); - return new RefUpdateState(r, RefUpdate.Result.IO_FAILURE); + return new RefUpdateState(":" + r, RefUpdate.Result.IO_FAILURE); } }) .toList(); @@ -95,7 +95,7 @@ taskIdRef, refName, name); - return new RefUpdateState(refName, RefUpdate.Result.NO_CHANGE); + return new RefUpdateState(":" + refName, RefUpdate.Result.NO_CHANGE); } repLog.info( @@ -112,7 +112,7 @@ Optional<Ref> ref = getRef(name, refName); if (!ref.isPresent()) { logger.atFine().log("[%s] Ref %s was not found in project %s", taskIdRef, refName, name); - return new RefUpdateState(refName, RefUpdate.Result.NO_CHANGE); + return new RefUpdateState(":" + refName, RefUpdate.Result.NO_CHANGE); } RefUpdateState deleteResult = deleteRef(name, ref.get()); @@ -149,7 +149,7 @@ u.setForceUpdate(true); result = u.delete(); - return new RefUpdateState(ref.getName(), result); + return new RefUpdateState(":" + ref.getName(), result); } }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/data/BatchApplyObjectData.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/data/BatchApplyObjectData.java index 5b17002..95ba9a9 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/data/BatchApplyObjectData.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/api/data/BatchApplyObjectData.java
@@ -20,9 +20,24 @@ @AutoValue public abstract class BatchApplyObjectData { - public static BatchApplyObjectData create(String refName, Optional<RevisionData> revisionData) + public static BatchApplyObjectData create( + String refName, Optional<RevisionData> revisionData, boolean isDelete, boolean isCreate) throws IllegalArgumentException { - return new AutoValue_BatchApplyObjectData(refName, revisionData, false); + return new AutoValue_BatchApplyObjectData(refName, revisionData, isDelete, isCreate); + } + + public static BatchApplyObjectData newDeleteRef(String refName) throws IllegalArgumentException { + return create(refName, Optional.empty(), true, false); + } + + public static BatchApplyObjectData newUpdateRef( + String refName, Optional<RevisionData> revisionData) throws IllegalArgumentException { + return create(refName, revisionData, false, false); + } + + public static BatchApplyObjectData newCreateRef( + String refName, Optional<RevisionData> revisionData) throws IllegalArgumentException { + return create(refName, revisionData, false, true); } public abstract String refName(); @@ -31,6 +46,8 @@ public abstract boolean isDelete(); + public abstract boolean isCreate(); + @Override public String toString() { return String.format(
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/pull/filter/ApplyObjectBannedCreateRefsFilter.java b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/filter/ApplyObjectBannedCreateRefsFilter.java new file mode 100644 index 0000000..d7c685a --- /dev/null +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/pull/filter/ApplyObjectBannedCreateRefsFilter.java
@@ -0,0 +1,37 @@ +// Copyright (C) 2025 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.googlesource.gerrit.plugins.replication.pull.filter; + +import com.google.common.collect.ImmutableList; +import com.google.inject.Inject; +import com.google.inject.Singleton; +import com.googlesource.gerrit.plugins.replication.api.ReplicationConfig; +import java.util.List; +import org.eclipse.jgit.lib.Config; + +@Singleton +public class ApplyObjectBannedCreateRefsFilter extends RefsFilter { + @Inject + public ApplyObjectBannedCreateRefsFilter(ReplicationConfig replicationConfig) { + super(replicationConfig); + } + + @Override + protected List<String> getRefNamePatterns(Config cfg) { + String[] applyObjectBannedCreateRefs = + cfg.getStringList("replication", null, "applyObjectBannedCreateRefs"); + return ImmutableList.copyOf(applyObjectBannedCreateRefs); + } +}
diff --git a/src/main/resources/Documentation/config.md b/src/main/resources/Documentation/config.md index 546df2e..da350ad 100644 --- a/src/main/resources/Documentation/config.md +++ b/src/main/resources/Documentation/config.md
@@ -132,6 +132,33 @@ : If true, the default fetch refspec will be set to use forced update to the local repository when no refspec is given. By default, false. + +replication.applyObjectBannedCreateRefs +: Specify which refs created should be banned from being + replicated synchronously via apply-object. + + It can be provided more than once, and supports three formats: regular expressions, + wildcard matching, and single ref matching. All three formats match are case-sensitive. + + Values starting with a caret `^` are treated as regular + expressions. For the regular expressions details please follow + official [java documentation](https://docs.oracle.com/javase/tutorial/essential/regex/). + + Please note that regular expressions could also be used + with inverse match. + + Values that are not regular expressions and end in `*` are + treated as wildcard matches. Wildcards match refs whose + name agrees from the beginning until the trailing `*`. So + `foo/b*` would match the refs `foo/b`, `foo/bar`, and + `foo/baz`, but neither `foobar`, nor `bar/foo/baz`. + + Values that are neither regular expressions nor wildcards are + treated as single ref matches. So `foo/bar` matches only + the ref `foo/bar`, but no other refs. + + By default, set to empty (all create refs are replicated synchronously). + replication.lockErrorMaxRetries : Number of times to retry a replication operation if a lock error is detected.
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/FetchOneTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/FetchOneTest.java index 73715d6..6ce3dbe 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/FetchOneTest.java +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/FetchOneTest.java
@@ -70,6 +70,7 @@ private final String TEST_PROJECT_NAME = "FetchOneTest"; private final Project.NameKey PROJECT_NAME = Project.NameKey.parse(TEST_PROJECT_NAME); private final String TEST_REF = "refs/heads/refForReplicationTask"; + private final String TEST_DELETE_REF = ":" + TEST_REF; private final FetchRefSpec TEST_REF_SPEC = FetchRefSpec.fromRef(TEST_REF); private final String URI_PATTERN = "http://test.com/" + TEST_PROJECT_NAME + ".git"; private final TestMetricMaker testMetricMaker = new TestMetricMaker(); @@ -178,6 +179,26 @@ } @Test + public void shouldDeleteRefWhenAddingDeleteRefSpec() throws IOException { + setupRemoteConfigMock(List.of(ALL_REFS_SPEC)); + objectUnderTest.addRefs(refSpecsSetOf(TEST_REF)); + objectUnderTest.addRefs(refSpecsSetOf(TEST_DELETE_REF)); + + assertThat(objectUnderTest.getFetchRefSpecs()) + .isEqualTo(List.of(FetchRefSpec.fromRef(TEST_DELETE_REF))); + } + + @Test + public void shouldAddRefWhenAddingRefSpecToDeleteRefSpec() throws IOException { + setupRemoteConfigMock(List.of(ALL_REFS_SPEC)); + objectUnderTest.addRefs(refSpecsSetOf(TEST_DELETE_REF)); + objectUnderTest.addRefs(refSpecsSetOf(TEST_REF)); + + assertThat(objectUnderTest.getFetchRefSpecs()) + .isEqualTo(List.of(FetchRefSpec.fromRef(TEST_REF + ":" + TEST_REF))); + } + + @Test public void shouldReturnExistingStates() { assertThat(createTestStates(TEST_REF_SPEC, 1)) .isEqualTo(objectUnderTest.getStates().get(TEST_REF_SPEC));
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/ReplicationQueueTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/ReplicationQueueTest.java index 1c8a8ea..7f4ca4e 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/ReplicationQueueTest.java +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/ReplicationQueueTest.java
@@ -57,6 +57,7 @@ import com.googlesource.gerrit.plugins.replication.pull.client.FetchApiClient; import com.googlesource.gerrit.plugins.replication.pull.client.FetchRestApiClient; import com.googlesource.gerrit.plugins.replication.pull.client.HttpResult; +import com.googlesource.gerrit.plugins.replication.pull.filter.ApplyObjectBannedCreateRefsFilter; import com.googlesource.gerrit.plugins.replication.pull.filter.ApplyObjectsRefsFilter; import com.googlesource.gerrit.plugins.replication.pull.filter.ExcludedRefsFilter; import java.io.IOException; @@ -88,6 +89,8 @@ private static final String TEST_REF_NAME = "refs/meta/heads/anyref"; private static final Project.NameKey PROJECT = Project.nameKey("defaultProject"); + private static final String OLD_OBJECT_ID = + ObjectId.fromString("00f11fd1e3206333235603f889837bad2692da4b").getName(); private static final String NEW_OBJECT_ID = ObjectId.fromString("3c1ddc050d7906adb0e29bc3bc46af8749b2f63b").getName(); @@ -110,6 +113,7 @@ @Mock HttpResult httpResult; @Mock HttpResult batchHttpResult; @Mock ApplyObjectsRefsFilter applyObjectsRefsFilter; + @Mock ApplyObjectBannedCreateRefsFilter applyObjectsBannedCreateRefsFilter; @Mock Config config; ApplyObjectMetrics applyObjectMetrics; @@ -207,6 +211,7 @@ LOCAL_INSTANCE_ID, config, applyObjectsRefsFilter, + applyObjectsBannedCreateRefsFilter, shutdownState); } @@ -238,6 +243,7 @@ LOCAL_INSTANCE_ID, config, applyObjectsRefsFilter, + applyObjectsBannedCreateRefsFilter, shutdownState); Event event = new TestEvent("refs/changes/01/1/meta"); @@ -332,6 +338,7 @@ LOCAL_INSTANCE_ID, config, applyObjectsRefsFilter, + applyObjectsBannedCreateRefsFilter, shutdownState); } @@ -420,6 +427,34 @@ } @Test + public void shouldCallApplyObjectWhenCreatedRefNotMatchesApplyObjectBannedCreateRefsFilter() + throws Exception { + String bannedRef = "refs/heads/banned"; + Event event = generateBatchCreateRefEvent("refs/heads/not-banned"); + objectUnderTest.start(); + lenient().when(applyObjectsBannedCreateRefsFilter.match(eq(bannedRef))).thenReturn(true); + + objectUnderTest.onEvent(event); + + verify(fetchRestApiClient).callBatchSendObject(any(), any(), anyLong(), any()); + verify(fetchRestApiClient, never()).callBatchFetch(any(), any(), any()); + } + + @Test + public void shouldFallbackToCallBatchFetchWhenCreatedRefMatchesApplyObjectBannedCreateRefsFilter() + throws Exception { + String bannedRef = "refs/heads/banned"; + Event event = generateBatchCreateRefEvent("refs/heads/banned"); + objectUnderTest.start(); + when(applyObjectsBannedCreateRefsFilter.match(eq(bannedRef))).thenReturn(true); + + objectUnderTest.onEvent(event); + + verify(fetchRestApiClient).callBatchFetch(any(), any(), any()); + verify(fetchRestApiClient, never()).callBatchSendObject(any(), any(), anyLong(), any()); + } + + @Test public void shouldFallbackToApplyAllParentObjectsWhenParentObjectIsMissingOnMetaRef() throws Exception { Event event = generateBatchRefUpdateEvent("refs/changes/01/1/meta"); @@ -536,6 +571,7 @@ LOCAL_INSTANCE_ID, config, applyObjectsRefsFilter, + applyObjectsBannedCreateRefsFilter, shutdownState); Event event = generateBatchRefUpdateEvent("refs/multi-site/version"); objectUnderTest.onEvent(event); @@ -618,13 +654,21 @@ } private BatchRefUpdateEvent generateBatchRefUpdateEvent(String... refs) { + return generateBatchRefEvent(OLD_OBJECT_ID, NEW_OBJECT_ID, refs); + } + + private BatchRefUpdateEvent generateBatchCreateRefEvent(String... refs) { + return generateBatchRefEvent(ObjectId.zeroId().name(), NEW_OBJECT_ID, refs); + } + + private BatchRefUpdateEvent generateBatchRefEvent(String oldRev, String newRev, String[] refs) { List<RefUpdateAttribute> refUpdates = Arrays.stream(refs) .map( ref -> { RefUpdateAttribute upd = new RefUpdateAttribute(); - upd.newRev = NEW_OBJECT_ID; - upd.oldRev = ObjectId.zeroId().getName(); + upd.newRev = newRev; + upd.oldRev = oldRev; upd.project = PROJECT.get(); upd.refName = ref; return upd;
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/client/FetchRestApiClientBase.java b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/client/FetchRestApiClientBase.java index 7f0df41..cfff177 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/replication/pull/client/FetchRestApiClientBase.java +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/pull/client/FetchRestApiClientBase.java
@@ -624,7 +624,7 @@ List<BatchApplyObjectData> batchApplyObjects = new ArrayList<>(); batchApplyObjects.add( - BatchApplyObjectData.create(refName, Optional.of(createSampleRevisionData("a")))); + BatchApplyObjectData.newUpdateRef(refName, Optional.of(createSampleRevisionData("a")))); objectUnderTest.callBatchSendObject( Project.nameKey("test_repo"), batchApplyObjects, eventCreatedOn, new URIish(api)); @@ -647,8 +647,8 @@ RevisionData revisionA = createSampleRevisionData("a"); RevisionData revisionB = createSampleRevisionData("b"); String refNameB = "refs/heads/b"; - batchApplyObjects.add(BatchApplyObjectData.create(refName, Optional.of(revisionA))); - batchApplyObjects.add(BatchApplyObjectData.create(refNameB, Optional.of(revisionB))); + batchApplyObjects.add(BatchApplyObjectData.newUpdateRef(refName, Optional.of(revisionA))); + batchApplyObjects.add(BatchApplyObjectData.newUpdateRef(refNameB, Optional.of(revisionB))); objectUnderTest.callBatchSendObject( Project.nameKey("test_repo"), batchApplyObjects, eventCreatedOn, new URIish(api));