Merge branch 'stable-3.11' into stable-3.12 * stable-3.11: Throw lockFailureException when RefDBLock occurs Add 3rd site in the local test environment Use fanout replication config for the local test environment Sort ref-names before locking them in filterAndLock Manage change deletion performed by pull-replication Change-Id: If9a6f1e37e61fa42d024183c17b73e0e51f3d6e9
diff --git a/setup_local_env/configs/replication.config b/setup_local_env/configs/replication.config index ff0c248..7aa26fc 100644 --- a/setup_local_env/configs/replication.config +++ b/setup_local_env/configs/replication.config
@@ -1,15 +1,3 @@ -[remote "$REPLICA_INSTANCE_ID"] - $PULL_REPLICATION_URL - $PULL_REPLICATION_API_URL - fetch = +refs/*:refs/* - mirror = true - timeout = 600 - rescheduleDelay = 15 - replicationDelay = $REPLICATION_DELAY_SEC - createMissingRepositories = true - replicateProjectDeletions = true - replicateHiddenProjects = true - threads = 5 [gerrit] autoReload = true replicateOnStartup = false
diff --git a/setup_local_env/configs/replication.remote b/setup_local_env/configs/replication.remote new file mode 100644 index 0000000..5724708 --- /dev/null +++ b/setup_local_env/configs/replication.remote
@@ -0,0 +1,12 @@ +[remote] + $PULL_REPLICATION_URL + $PULL_REPLICATION_API_URL + fetch = +refs/*:refs/* + mirror = true + timeout = 600 + rescheduleDelay = 15 + replicationDelay = $REPLICATION_DELAY_SEC + createMissingRepositories = true + replicateProjectDeletions = true + replicateHiddenProjects = true + threads = 5 \ No newline at end of file
diff --git a/setup_local_env/haproxy-config/haproxy.cfg b/setup_local_env/haproxy-config/haproxy.cfg index de4f709..eb8bce4 100644 --- a/setup_local_env/haproxy-config/haproxy.cfg +++ b/setup_local_env/haproxy-config/haproxy.cfg
@@ -45,6 +45,7 @@ http-check expect status 200 server node1 $HA_GERRIT_SITE1_HOSTNAME:$HA_GERRIT_SITE1_HTTPD_PORT check inter 10s server node2 $HA_GERRIT_SITE2_HOSTNAME:$HA_GERRIT_SITE2_HTTPD_PORT check inter 10s backup + server node3 $HA_GERRIT_SITE3_HOSTNAME:$HA_GERRIT_SITE3_HTTPD_PORT check inter 10s backup backend write-backendnodes mode http @@ -56,6 +57,7 @@ http-check expect status 200 server node1 $HA_GERRIT_SITE1_HOSTNAME:$HA_GERRIT_SITE1_HTTPD_PORT check inter 10s server node2 $HA_GERRIT_SITE2_HOSTNAME:$HA_GERRIT_SITE2_HTTPD_PORT check inter 10s backup + server node3 $HA_GERRIT_SITE3_HOSTNAME:$HA_GERRIT_SITE3_HTTPD_PORT check inter 10s backup backend ssh mode tcp @@ -66,4 +68,5 @@ timeout server 5m server ssh_node1 $HA_GERRIT_SITE1_HOSTNAME:$HA_GERRIT_SITE1_SSHD_PORT check inter 10s check port $HA_GERRIT_SITE1_HTTPD_PORT inter 10s server ssh_node2 $HA_GERRIT_SITE2_HOSTNAME:$HA_GERRIT_SITE2_SSHD_PORT check inter 10s check port $HA_GERRIT_SITE2_HTTPD_PORT inter 10s backup + server ssh_node3 $HA_GERRIT_SITE3_HOSTNAME:$HA_GERRIT_SITE3_SSHD_PORT check inter 10s check port $HA_GERRIT_SITE2_HTTPD_PORT inter 10s backup
diff --git a/setup_local_env/setup.sh b/setup_local_env/setup.sh index b6cdab4..06fd7c7 100755 --- a/setup_local_env/setup.sh +++ b/setup_local_env/setup.sh
@@ -78,6 +78,23 @@ done } +function copy_replication_remote_file { + fanout_template_file=$SCRIPT_DIR/configs/replication.remote + + CONFIG_TEST_SITE=$1 + export REPLICATION_HTTPD_PORT=$2 + export REPLICATION_HOSTNAME=$3 + export PULL_REPLICATION_URL=$(get_pull_replication_url $REPLICATION_HOSTNAME) + export PULL_REPLICATION_API_URL=$(get_pull_replication_api_url $REPLICATION_HOSTNAME) + export REPLICA_INSTANCE_ID=$4 + fanout_file=$REPLICA_INSTANCE_ID.config + + mkdir -p $CONFIG_TEST_SITE/replication + + echo "Replacing variables for file $fanout_template_file and copying to $CONFIG_TEST_SITE/replication/$fanout_file" + cat $fanout_template_file | envsubst | sed 's/#{name}#/${name}/g' > $CONFIG_TEST_SITE/replication/$fanout_file +} + function start_ha_proxy { export HA_GERRIT_CANONICAL_HOSTNAME=$GERRIT_CANONICAL_HOSTNAME @@ -87,11 +104,14 @@ export HA_GERRIT_SITE1_HOSTNAME=$GERRIT_1_HOSTNAME export HA_GERRIT_SITE2_HOSTNAME=$GERRIT_2_HOSTNAME + export HA_GERRIT_SITE3_HOSTNAME=$GERRIT_3_HOSTNAME export HA_GERRIT_SITE1_HTTPD_PORT=$GERRIT_1_HTTPD_PORT export HA_GERRIT_SITE2_HTTPD_PORT=$GERRIT_2_HTTPD_PORT + export HA_GERRIT_SITE3_HTTPD_PORT=$GERRIT_3_HTTPD_PORT export HA_GERRIT_SITE1_SSHD_PORT=$GERRIT_1_SSHD_PORT export HA_GERRIT_SITE2_SSHD_PORT=$GERRIT_2_SSHD_PORT + export HA_GERRIT_SITE3_SSHD_PORT=$GERRIT_3_SSHD_PORT cat $SCRIPT_DIR/haproxy-config/haproxy.cfg | envsubst > $HA_PROXY_CONFIG_DIR/haproxy.cfg @@ -133,13 +153,28 @@ CONFIG_TEST_SITE_2=$LOCATION_TEST_SITE_2/etc GERRIT_SITE2_REMOTE_DEBUG_PORT="5006" GERRIT_SITE2_INSTANCE_ID="instance-2" + # SITE 3 + GERRIT_SITE3_HOSTNAME=$7 + GERRIT_SITE3_HTTPD_PORT=$8 + GERRIT_SITE3_SSHD_PORT=$9 + CONFIG_TEST_SITE_3=$LOCATION_TEST_SITE_3/etc + GERRIT_SITE3_REMOTE_DEBUG_PORT="5007" + GERRIT_SITE3_INSTANCE_ID="instance-3" # Set config SITE1 copy_config_files $CONFIG_TEST_SITE_1 $GERRIT_SITE1_HTTPD_PORT $LOCATION_TEST_SITE_1 $GERRIT_SITE1_SSHD_PORT $GERRIT_SITE2_HTTPD_PORT $GERRIT_SITE1_HOSTNAME $GERRIT_SITE2_HOSTNAME $GERRIT_SITE1_REMOTE_DEBUG_PORT $GERRIT_SITE1_INSTANCE_ID $GERRIT_SITE2_INSTANCE_ID - + copy_replication_remote_file $CONFIG_TEST_SITE_1 $GERRIT_SITE2_HTTPD_PORT $GERRIT_SITE2_HOSTNAME $GERRIT_SITE2_INSTANCE_ID + copy_replication_remote_file $CONFIG_TEST_SITE_1 $GERRIT_SITE3_HTTPD_PORT $GERRIT_SITE3_HOSTNAME $GERRIT_SITE3_INSTANCE_ID # Set config SITE2 copy_config_files $CONFIG_TEST_SITE_2 $GERRIT_SITE2_HTTPD_PORT $LOCATION_TEST_SITE_2 $GERRIT_SITE2_SSHD_PORT $GERRIT_SITE1_HTTPD_PORT $GERRIT_SITE1_HOSTNAME $GERRIT_SITE2_HOSTNAME $GERRIT_SITE2_REMOTE_DEBUG_PORT $GERRIT_SITE2_INSTANCE_ID $GERRIT_SITE1_INSTANCE_ID + copy_replication_remote_file $CONFIG_TEST_SITE_2 $GERRIT_SITE1_HTTPD_PORT $GERRIT_SITE1_HOSTNAME $GERRIT_SITE1_INSTANCE_ID + copy_replication_remote_file $CONFIG_TEST_SITE_2 $GERRIT_SITE3_HTTPD_PORT $GERRIT_SITE3_HOSTNAME $GERRIT_SITE3_INSTANCE_ID + + # Set config SITE3 + copy_config_files $CONFIG_TEST_SITE_3 $GERRIT_SITE3_HTTPD_PORT $LOCATION_TEST_SITE_3 $GERRIT_SITE3_SSHD_PORT $GERRIT_SITE1_HTTPD_PORT $GERRIT_SITE1_HOSTNAME $GERRIT_SITE3_HOSTNAME $GERRIT_SITE3_REMOTE_DEBUG_PORT $GERRIT_SITE3_INSTANCE_ID $GERRIT_SITE1_INSTANCE_ID + copy_replication_remote_file $CONFIG_TEST_SITE_3 $GERRIT_SITE1_HTTPD_PORT $GERRIT_SITE1_HOSTNAME $GERRIT_SITE1_INSTANCE_ID + copy_replication_remote_file $CONFIG_TEST_SITE_3 $GERRIT_SITE2_HTTPD_PORT $GERRIT_SITE2_HOSTNAME $GERRIT_SITE2_INSTANCE_ID } function is_docker_desktop { @@ -406,10 +441,13 @@ export GERRIT_CANONICAL_PORT=${GERRIT_CANONICAL_PORT:-"8080"} GERRIT_1_HOSTNAME=${GERRIT_1_HOSTNAME:-"localhost"} GERRIT_2_HOSTNAME=${GERRIT_2_HOSTNAME:-"localhost"} +GERRIT_3_HOSTNAME=${GERRIT_3_HOSTNAME:-"localhost"} GERRIT_1_HTTPD_PORT=${GERRIT_1_HTTPD_PORT:-"18080"} GERRIT_2_HTTPD_PORT=${GERRIT_2_HTTPD_PORT:-"18081"} +GERRIT_3_HTTPD_PORT=${GERRIT_3_HTTPD_PORT:-"18082"} GERRIT_1_SSHD_PORT=${GERRIT_1_SSHD_PORT:-"39418"} GERRIT_2_SSHD_PORT=${GERRIT_2_SSHD_PORT:-"49418"} +GERRIT_3_SSHD_PORT=${GERRIT_3_SSHD_PORT:-"59418"} export REPLICATION_DELAY_SEC=${REPLICATION_DELAY_SEC:-"5"} export SSH_ADVERTISED_PORT=${SSH_ADVERTISED_PORT:-"29418"} HTTPS_ENABLED=${HTTPS_ENABLED:-"false"} @@ -418,6 +456,7 @@ export COMMON_LOCATION=$DEPLOYMENT_LOCATION/gerrit_setup LOCATION_TEST_SITE_1=$COMMON_LOCATION/instance-1 LOCATION_TEST_SITE_2=$COMMON_LOCATION/instance-2 +LOCATION_TEST_SITE_3=$COMMON_LOCATION/instance-3 HA_PROXY_CONFIG_DIR=$COMMON_LOCATION/ha-proxy-config HA_PROXY_CERTIFICATES_DIR="$HA_PROXY_CONFIG_DIR/certificates" PROMETHEUS_CONFIG_DIR=$COMMON_LOCATION/prometheus-config @@ -430,7 +469,7 @@ export FAKE_NFS=$COMMON_LOCATION/fake_nfs if [ "$JUST_CLEANUP_ENV" = "true" ];then - cleanup_environment $LOCATION_TEST_SITE_1 $LOCATION_TEST_SITE_2 $COMMON_LOCATION + cleanup_environment $LOCATION_TEST_SITE_1 $LOCATION_TEST_SITE_2 $LOCATION_TEST_SITE_3 $COMMON_LOCATION exit 0 fi @@ -500,10 +539,10 @@ # New installation if [ $NEW_INSTALLATION = "true" ]; then - cleanup_environment $LOCATION_TEST_SITE_1 $LOCATION_TEST_SITE_2 $COMMON_LOCATION + cleanup_environment $LOCATION_TEST_SITE_1 $LOCATION_TEST_SITE_2 $LOCATION_TEST_SITE_3 $COMMON_LOCATION echo "Setting up directories" - mkdir -p $LOCATION_TEST_SITE_1 $LOCATION_TEST_SITE_2 $HA_PROXY_CERTIFICATES_DIR $FAKE_NFS + mkdir -p $LOCATION_TEST_SITE_1 $LOCATION_TEST_SITE_2 $LOCATION_TEST_SITE_3 $HA_PROXY_CERTIFICATES_DIR $FAKE_NFS java -jar $DEPLOYMENT_LOCATION/gerrit.war init --batch --no-auto-start --install-all-plugins --dev -d $LOCATION_TEST_SITE_1 # Deploying TLS certificates @@ -544,18 +583,19 @@ # Replicating environment echo "Replicating environment" cp -fR $LOCATION_TEST_SITE_1/* $LOCATION_TEST_SITE_2 + cp -fR $LOCATION_TEST_SITE_1/* $LOCATION_TEST_SITE_3 - echo "Link pullreplication plugin" - ln -s $LOCATION_TEST_SITE_1/plugins/pull-replication.jar $LOCATION_TEST_SITE_1/lib/pull-replication.jar - ln -s $LOCATION_TEST_SITE_2/plugins/pull-replication.jar $LOCATION_TEST_SITE_2/lib/pull-replication.jar + for location_test_site in $LOCATION_TEST_SITE_1 $LOCATION_TEST_SITE_2 $LOCATION_TEST_SITE_3 + do + echo "Link pullreplication plugin on $location_test_site" + ln -s $location_test_site/plugins/pull-replication.jar $location_test_site/lib/pull-replication.jar - echo "Link multi-site library to plugin directory" - ln -s $LOCATION_TEST_SITE_1/lib/multi-site.jar $LOCATION_TEST_SITE_1/plugins/multi-site.jar - ln -s $LOCATION_TEST_SITE_2/lib/multi-site.jar $LOCATION_TEST_SITE_2/plugins/multi-site.jar + echo "Link multi-site library to plugin directory on $location_test_site" + ln -s $location_test_site/lib/multi-site.jar $location_test_site/plugins/multi-site.jar - echo "Copy pull-replication plugin" - cp -f $DEPLOYMENT_LOCATION/pull-replication.jar $LOCATION_TEST_SITE_1/plugins/pull-replication.jar - cp -f $DEPLOYMENT_LOCATION/pull-replication.jar $LOCATION_TEST_SITE_2/plugins/pull-replication.jar + echo "Copy pull-replication plugin on $location_test_site" + cp -f $DEPLOYMENT_LOCATION/pull-replication.jar $location_test_site/plugins/pull-replication.jar + done fi @@ -577,16 +617,15 @@ prepare_broker_data echo "Re-deploying configuration files" -deploy_config_files $GERRIT_1_HOSTNAME $GERRIT_1_HTTPD_PORT $GERRIT_1_SSHD_PORT $GERRIT_2_HOSTNAME $GERRIT_2_HTTPD_PORT $GERRIT_2_SSHD_PORT -echo "Move replication and delete-project plugins to /lib on gerrit site 1" -mv $LOCATION_TEST_SITE_1/plugins/{replication,delete-project}.jar $LOCATION_TEST_SITE_1/lib/. -echo "Starting gerrit site 1" -$LOCATION_TEST_SITE_1/bin/gerrit.sh restart -echo "Move replication plugin to /lib on gerrit site 2" -mv $LOCATION_TEST_SITE_2/plugins/{replication,delete-project}.jar $LOCATION_TEST_SITE_2/lib/. -echo "Starting gerrit site 2" -$LOCATION_TEST_SITE_2/bin/gerrit.sh restart +deploy_config_files $GERRIT_1_HOSTNAME $GERRIT_1_HTTPD_PORT $GERRIT_1_SSHD_PORT $GERRIT_2_HOSTNAME $GERRIT_2_HTTPD_PORT $GERRIT_2_SSHD_PORT $GERRIT_3_HOSTNAME $GERRIT_3_HTTPD_PORT $GERRIT_3_SSHD_PORT +for location_test_site in $LOCATION_TEST_SITE_1 $LOCATION_TEST_SITE_2 $LOCATION_TEST_SITE_3 +do + echo "Move replication and delete-project plugins to /lib on gerrit $location_test_site" + mv $location_test_site/plugins/{replication,delete-project}.jar $location_test_site/lib/. + echo "Starting gerrit $location_test_site" + $location_test_site/bin/gerrit.sh restart +done if [[ $(ps -ax | grep haproxy | grep "gerrit_setup/ha-proxy-config" | awk '{print $1}' | wc -l) -lt 1 ]];then echo "Starting haproxy" @@ -604,10 +643,12 @@ echo "GERRIT HA-PROXY: $GERRIT_CANONICAL_WEB_URL" echo "GERRIT-1: http://$GERRIT_1_HOSTNAME:$GERRIT_1_HTTPD_PORT" echo "GERRIT-2: http://$GERRIT_2_HOSTNAME:$GERRIT_2_HTTPD_PORT" +echo "GERRIT-3: http://$GERRIT_3_HOSTNAME:$GERRIT_3_HTTPD_PORT" echo "Prometheus: http://localhost:9090" echo echo "Site-1: $LOCATION_TEST_SITE_1" echo "Site-2: $LOCATION_TEST_SITE_2" +echo "Site-3: $LOCATION_TEST_SITE_3" echo echo "$HTTPS_CLONE_MSG" echo
diff --git a/src/main/java/com/googlesource/gerrit/plugins/multisite/index/FetchRefReplicatedEventHandler.java b/src/main/java/com/googlesource/gerrit/plugins/multisite/index/FetchRefReplicatedEventHandler.java index f47cea5..69cdb06 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/multisite/index/FetchRefReplicatedEventHandler.java +++ b/src/main/java/com/googlesource/gerrit/plugins/multisite/index/FetchRefReplicatedEventHandler.java
@@ -22,22 +22,31 @@ import com.google.gerrit.server.config.GerritInstanceId; import com.google.gerrit.server.events.Event; import com.google.gerrit.server.events.EventListener; +import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.index.change.ChangeIndexer; import com.google.inject.Inject; import com.googlesource.gerrit.plugins.multisite.forwarder.Context; import com.googlesource.gerrit.plugins.replication.pull.FetchRefReplicatedEvent; import com.googlesource.gerrit.plugins.replication.pull.ReplicationState; +import java.io.IOException; import org.eclipse.jgit.errors.MissingObjectException; +import org.eclipse.jgit.lib.RefUpdate; +import org.eclipse.jgit.lib.Repository; public class FetchRefReplicatedEventHandler implements EventListener { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); + private final GitRepositoryManager gitRepositoryManager; private ChangeIndexer changeIndexer; private final String instanceId; @Inject - FetchRefReplicatedEventHandler(ChangeIndexer changeIndexer, @GerritInstanceId String instanceId) { + FetchRefReplicatedEventHandler( + ChangeIndexer changeIndexer, + @GerritInstanceId String instanceId, + GitRepositoryManager gitRepositoryManager) { this.changeIndexer = changeIndexer; this.instanceId = instanceId; + this.gitRepositoryManager = gitRepositoryManager; } @Override @@ -48,8 +57,16 @@ if (event instanceof FetchRefReplicatedEvent && isLocalEvent(event)) { FetchRefReplicatedEvent fetchRefReplicatedEvent = (FetchRefReplicatedEvent) event; - boolean isMetaRef = RefNames.isNoteDbMetaRef(fetchRefReplicatedEvent.getRefName()); - if (!RefNames.isRefsChanges(fetchRefReplicatedEvent.getRefName()) + // This removal of the ':' prefix is needed because of Issue 426470297 in the + // pull-replication plugin + // that may leave the deleted refs notification as a ref-spec instead of the deleted + // ref-name. + String refName = + fetchRefReplicatedEvent.ref.startsWith(":") + ? fetchRefReplicatedEvent.ref.substring(1) + : fetchRefReplicatedEvent.ref; + boolean isMetaRef = RefNames.isNoteDbMetaRef(refName); + if (!RefNames.isRefsChanges(refName) || !fetchRefReplicatedEvent .getStatus() .equals(ReplicationState.RefFetchResult.SUCCEEDED.toString())) { @@ -60,23 +77,64 @@ logger.atFine().log( "Indexing ref '%s' for project %s", fetchRefReplicatedEvent.getRefName(), projectNameKey.get()); - Change.Id changeId = Change.Id.fromRef(fetchRefReplicatedEvent.getRefName()); - if (changeId != null && isMetaRef) { - catchStorageExceptionForMissingUnknown( - () -> changeIndexer.index(projectNameKey, changeId), - "Skipping indexing of " - + projectNameKey - + "~" - + changeId.get() - + " because its patch-sets aren't replicated yet"); - } else if (changeId != null) { - changeIndexer.index(projectNameKey, changeId); - } else { - logger.atWarning().log( - "Couldn't get changeId from refName. Skipping indexing of change %s for project %s", - fetchRefReplicatedEvent.getRefName(), projectNameKey.get()); + Change.Id changeId = Change.Id.fromRef(refName); + + try (Repository repo = gitRepositoryManager.openRepository(projectNameKey)) { + if (changeId != null) { + RefUpdate.Result fetchRefReplicatedEventResult = + fetchRefReplicatedEvent.getRefUpdateResult(); + if ((RefUpdate.Result.FORCED.equals(fetchRefReplicatedEventResult) + || RefUpdate.Result.NO_CHANGE.equals(fetchRefReplicatedEventResult)) + && repo.exactRef(refName) == null) { + logger.atInfo().log( + "Deleting change '%s' from index following the forced delete of '%s' on project" + + " %s", + changeId, refName, projectNameKey.get()); + // NOTE: The change has been already deleted and we just don't know if it was a + // locally + // created change or imported, because the meta-data is not there anymore. + // In the future, it would be best to have that information passed to the event by the + // pull-replication plugin *before* the delete, and added to the + // FetchRefReplicatedEvent, + // so that multi-site can use it for computing the virtual change-id and assuring the + // delete + // of the imported changes as well. + if (isMetaRef) { + catchStorageExceptionForMissingUnknown( + () -> changeIndexer.delete(changeId), + "Skipping deleting from index of " + + projectNameKey + + "~" + + changeId.get() + + " because its patch-sets aren't replicated yet"); + } else { + changeIndexer.delete(changeId); + } + } else { + logger.atInfo().log( + "Indexing change '%s' following update of '%s' on project %s", + changeId, refName, projectNameKey.get()); + if (isMetaRef) { + catchStorageExceptionForMissingUnknown( + () -> changeIndexer.index(projectNameKey, changeId), + "Skipping indexing of " + + projectNameKey + + "~" + + changeId.get() + + " because its patch-sets aren't replicated yet"); + } else { + changeIndexer.index(projectNameKey, changeId); + } + } + } else { + logger.atWarning().log( + "Couldn't get changeId from refName. Skipping indexing of change %s for project %s", + refName, projectNameKey.get()); + } } } + } catch (IOException e) { + logger.atSevere().withCause(e).log("Unable to process event %s", event); } finally { Context.setForwardedEvent(isForwarded); }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/MultisiteReplicationFetchFilter.java b/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/MultisiteReplicationFetchFilter.java index 89310bf..8423d2e 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/MultisiteReplicationFetchFilter.java +++ b/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/MultisiteReplicationFetchFilter.java
@@ -19,6 +19,7 @@ import com.gerritforge.gerrit.globalrefdb.GlobalRefDbLockException; import com.gerritforge.gerrit.globalrefdb.RefDbLockException; import com.gerritforge.gerrit.globalrefdb.validation.SharedRefDatabaseWrapper; +import com.google.common.collect.ImmutableSortedSet; import com.google.common.collect.Sets; import com.google.common.flogger.FluentLogger; import com.google.gerrit.entities.Project; @@ -26,6 +27,7 @@ import com.google.inject.Inject; import com.google.inject.Singleton; import com.googlesource.gerrit.plugins.multisite.Configuration; +import com.googlesource.gerrit.plugins.replication.pull.FetchOne.LockFailureException; import com.googlesource.gerrit.plugins.replication.pull.ReplicationFetchFilter; import java.io.IOException; import java.util.Collections; @@ -34,6 +36,7 @@ import java.util.Map; import java.util.Optional; import java.util.Set; +import java.util.SortedSet; import java.util.stream.Collectors; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.Ref; @@ -96,20 +99,25 @@ @Override public Map<String, AutoCloseable> filterAndLock(String projectName, Set<String> fetchRefs) - throws RefDbLockException { + throws LockFailureException { + // Use a SortedSet to ensure that they are always locked in the same order and therefore + // prevents potential deadlocks where multiple fetch tasks are mutually locking each other + // by locking different refs in different orders. + SortedSet<String> sortedFetchRefs = ImmutableSortedSet.copyOf(fetchRefs); Project.NameKey projectKey = Project.nameKey(projectName); Set<String> filteredRefs = new HashSet<>(); Map<String, AutoCloseable> refLocks = new HashMap<>(); try { - for (String ref : fetchRefs) { + for (String ref : sortedFetchRefs) { refLocks.put(ref, sharedRefDb.lockLocalRef(projectKey, ref)); } - filteredRefs.addAll(filter(projectName, fetchRefs)); + filteredRefs.addAll(filter(projectName, sortedFetchRefs)); } catch (RefDbLockException lockException) { filteredRefs.clear(); - throw lockException; + throw new LockFailureException( + "Unable to lock refs " + sortedFetchRefs + " for project " + projectName, lockException); } finally { - for (String excludedRef : Sets.difference(fetchRefs, filteredRefs)) { + for (String excludedRef : Sets.difference(sortedFetchRefs, filteredRefs)) { AutoCloseable excludedLock = refLocks.remove(excludedRef); if (excludedLock != null) { try {
diff --git a/src/test/java/com/googlesource/gerrit/plugins/multisite/index/FetchRefReplicatedEventHandlerTest.java b/src/test/java/com/googlesource/gerrit/plugins/multisite/index/FetchRefReplicatedEventHandlerTest.java index e7e0d8a..f7ef474 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/multisite/index/FetchRefReplicatedEventHandlerTest.java +++ b/src/test/java/com/googlesource/gerrit/plugins/multisite/index/FetchRefReplicatedEventHandlerTest.java
@@ -18,6 +18,7 @@ import static com.google.gerrit.testing.GerritJUnit.assertThrows; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; @@ -27,6 +28,7 @@ import com.google.gerrit.entities.Change; import com.google.gerrit.entities.Project; import com.google.gerrit.exceptions.StorageException; +import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.index.change.ChangeIndexer; import com.googlesource.gerrit.plugins.replication.pull.Context; import com.googlesource.gerrit.plugins.replication.pull.FetchRefReplicatedEvent; @@ -35,6 +37,7 @@ import org.eclipse.jgit.errors.MissingObjectException; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.RefUpdate; +import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.transport.URIish; import org.junit.Before; import org.junit.Test; @@ -42,20 +45,46 @@ public class FetchRefReplicatedEventHandlerTest { private static final String LOCAL_INSTANCE_ID = "local-instance-id"; private static final String REMOTE_INSTANCE_ID = "remote-instance-id"; + private static final Project.NameKey PROJECT_NAME_KEY = Project.nameKey("testProject"); private ChangeIndexer changeIndexerMock; + private GitRepositoryManager gitRepositoryManager; + private Repository repository; private FetchRefReplicatedEventHandler fetchRefReplicatedEventHandler; private static URIish sourceUri; @Before public void setUp() throws Exception { changeIndexerMock = mock(ChangeIndexer.class); + gitRepositoryManager = mock(GitRepositoryManager.class); + repository = mock(Repository.class); + doReturn(repository).when(gitRepositoryManager).openRepository(any()); fetchRefReplicatedEventHandler = - new FetchRefReplicatedEventHandler(changeIndexerMock, LOCAL_INSTANCE_ID); + new FetchRefReplicatedEventHandler( + changeIndexerMock, LOCAL_INSTANCE_ID, gitRepositoryManager); sourceUri = new URIish("git://aSourceNode/testProject.git"); } @Test public void onEventShouldIndexExistingChange() { + String ref = "refs/changes/41/41/meta"; + Change.Id changeId = Change.Id.fromRef(ref); + try { + Context.setLocalEvent(true); + fetchRefReplicatedEventHandler.onEvent( + newFetchRefReplicatedEvent( + PROJECT_NAME_KEY, + ref, + ReplicationState.RefFetchResult.SUCCEEDED, + LOCAL_INSTANCE_ID, + RefUpdate.Result.FAST_FORWARD)); + verify(changeIndexerMock, times(1)).index(eq(PROJECT_NAME_KEY), eq(changeId)); + } finally { + Context.unsetLocalEvent(); + } + } + + @Test + public void onEventShouldDeleteChangeFromIndex() { Project.NameKey projectNameKey = Project.nameKey("testProject"); String ref = "refs/changes/41/41/meta"; Change.Id changeId = Change.Id.fromRef(ref); @@ -63,8 +92,12 @@ Context.setLocalEvent(true); fetchRefReplicatedEventHandler.onEvent( newFetchRefReplicatedEvent( - projectNameKey, ref, ReplicationState.RefFetchResult.SUCCEEDED, LOCAL_INSTANCE_ID)); - verify(changeIndexerMock, times(1)).index(eq(projectNameKey), eq(changeId)); + projectNameKey, + ref, + ReplicationState.RefFetchResult.SUCCEEDED, + LOCAL_INSTANCE_ID, + RefUpdate.Result.FORCED)); + verify(changeIndexerMock, times(1)).delete(eq(changeId)); } finally { Context.unsetLocalEvent(); } @@ -86,7 +119,11 @@ .index(eq(projectNameKey), eq(changeId)); fetchRefReplicatedEventHandler.onEvent( newFetchRefReplicatedEvent( - projectNameKey, ref, ReplicationState.RefFetchResult.SUCCEEDED, LOCAL_INSTANCE_ID)); + projectNameKey, + ref, + ReplicationState.RefFetchResult.SUCCEEDED, + LOCAL_INSTANCE_ID, + RefUpdate.Result.FAST_FORWARD)); verify(changeIndexerMock, times(1)).index(eq(projectNameKey), eq(changeId)); } finally { Context.unsetLocalEvent(); @@ -113,7 +150,11 @@ () -> fetchRefReplicatedEventHandler.onEvent( newFetchRefReplicatedEvent( - projectNameKey, ref, RefFetchResult.SUCCEEDED, LOCAL_INSTANCE_ID))); + projectNameKey, + ref, + RefFetchResult.SUCCEEDED, + LOCAL_INSTANCE_ID, + RefUpdate.Result.FAST_FORWARD))); assertThat(indexException).isEqualTo(storageExceptionForMissingObject); verify(changeIndexerMock, times(1)).index(eq(projectNameKey), eq(changeId)); } finally { @@ -128,7 +169,11 @@ Change.Id changeId = Change.Id.fromRef(ref); fetchRefReplicatedEventHandler.onEvent( newFetchRefReplicatedEvent( - projectNameKey, ref, ReplicationState.RefFetchResult.SUCCEEDED, REMOTE_INSTANCE_ID)); + projectNameKey, + ref, + ReplicationState.RefFetchResult.SUCCEEDED, + REMOTE_INSTANCE_ID, + RefUpdate.Result.FAST_FORWARD)); verify(changeIndexerMock, never()).index(eq(projectNameKey), eq(changeId)); } @@ -139,7 +184,11 @@ Change.Id changeId = Change.Id.fromRef(ref); fetchRefReplicatedEventHandler.onEvent( newFetchRefReplicatedEvent( - projectNameKey, ref, ReplicationState.RefFetchResult.SUCCEEDED, LOCAL_INSTANCE_ID)); + projectNameKey, + ref, + ReplicationState.RefFetchResult.SUCCEEDED, + LOCAL_INSTANCE_ID, + RefUpdate.Result.FAST_FORWARD)); verify(changeIndexerMock).index(eq(projectNameKey), eq(changeId)); } @@ -150,7 +199,8 @@ Project.nameKey("testProject"), "invalidRef", ReplicationState.RefFetchResult.SUCCEEDED, - LOCAL_INSTANCE_ID)); + LOCAL_INSTANCE_ID, + RefUpdate.Result.FAST_FORWARD)); verify(changeIndexerMock, never()).index(any(), any()); } @@ -160,7 +210,11 @@ String ref = "refs/changes/41/41/meta"; fetchRefReplicatedEventHandler.onEvent( newFetchRefReplicatedEvent( - projectNameKey, ref, ReplicationState.RefFetchResult.FAILED, LOCAL_INSTANCE_ID)); + projectNameKey, + ref, + ReplicationState.RefFetchResult.FAILED, + LOCAL_INSTANCE_ID, + RefUpdate.Result.FAST_FORWARD)); verify(changeIndexerMock, never()).index(any(), any()); } @@ -170,15 +224,23 @@ String ref = "refs/changes/41/41/meta"; fetchRefReplicatedEventHandler.onEvent( newFetchRefReplicatedEvent( - projectNameKey, ref, ReplicationState.RefFetchResult.NOT_ATTEMPTED, LOCAL_INSTANCE_ID)); + projectNameKey, + ref, + ReplicationState.RefFetchResult.NOT_ATTEMPTED, + LOCAL_INSTANCE_ID, + RefUpdate.Result.FAST_FORWARD)); verify(changeIndexerMock, never()).index(any(), any()); } private FetchRefReplicatedEvent newFetchRefReplicatedEvent( - Project.NameKey projectNameKey, String ref, RefFetchResult fetchResult, String instanceId) { + Project.NameKey projectNameKey, + String ref, + RefFetchResult fetchResult, + String instanceId, + RefUpdate.Result refUpdateResult) { FetchRefReplicatedEvent event = new FetchRefReplicatedEvent( - projectNameKey.get(), ref, sourceUri, fetchResult, RefUpdate.Result.FAST_FORWARD); + projectNameKey.get(), ref, sourceUri, fetchResult, refUpdateResult); event.instanceId = instanceId; return event; }